1259344 - Move animation mochitest of play/cancel/playstate api to web-platform-tests

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Mantaroh Yoshinaga[:mantaroh]]

This bug is same as bug 1258972.
We can move test of several API using Element.animate().

* dom/animation/test/css-animations/file_animation-play.html
* dom/animation/test/css-animations/file_animation-cancel.html
* dom/animation/test/css-animations/file_animation-playstate.html

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

In file_animation-cancel.html, we used the css animation features[1]. I think that those tests should be in web-platform/tests/web-animation/animation-effect-timing. (fill-mode/animationPlayState)

[1] https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/dom/animation/test/css-animations/file_animation-cancel.html#34-48,126-159

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3779bab43952

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

fixed comment bug 1258972 comment #10.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbe3ce162255

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

Hi brian,
I moved play/cancel/playState tests to web-platform tests.
As mentioned bug1258972 comment #10, I had left some cancel/playState tests for css-animation.

Could you please review this patch?

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8736071 [details] [diff] [review]
Move animation mochitest(play/cancel/playstate) to web-platform tests.

Review of attachment 8736071 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thank you!

r=me with comments addressed.

::: dom/animation/test/css-animations/file_animation-cancel.html
@@ -28,5 @@
> -    assert_equals(getComputedStyle(div).transform, 'none',
> -                  'transform style is no longer animated after cancelling');
> -    t.done();
> -  }));
> -}, 'Animated style is cleared after cancelling a running CSS animation');

We should keep this test. It's important that cancel() is actually respected for CSS animations (and not re-triggered when the style updates).

@@ -79,5 @@
> -  animation.currentTime = 50 * 1000;
> -  assert_equals(getComputedStyle(div).marginLeft, '50px',
> -                'margin-left style is updated when cancelled animation is'
> -                + ' seeked');
> -}, 'After cancelling an animation, it can still be seeked');

I think we should leave this one too.

When CSS animations are cancelled, they enter a rare state where they are still CSS animations but they have no owning element. It is described a little here:

  https://drafts.csswg.org/css-animations-2/#owning-element-section

So I think we need to keep this test to check that we can still use the animation when it has no owning element.

@@ -99,5 @@
> -    assert_equals(animation.playState, 'running',
> -                  'Animation succeeds in running after being re-started');
> -    t.done();
> -  }));
> -}, 'After cancelling an animation, it can still be re-used');

For the same reason, I think we need to keep this test too.

::: dom/animation/test/css-animations/file_animation-playstate.html
@@ -19,5 @@
> -  animation.ready.then(t.step_func(function() {
> -    assert_equals(animation.playState, 'running');
> -    t.done();
> -  }));
> -}, 'Animation returns correct playState when running');

I think we should actually check that CSS animations also are initially in the play-pending state. So, I think we should keep this test.

@@ -48,5 @@
> -  animation.ready.then(t.step_func(function() {
> -    assert_equals(animation.playState, 'paused');
> -    t.done();
> -  }));
> -}, 'Animation.playState updates when paused by script');

This whole area of playing/pausing is one of the most tricky interactions between CSS animations and the Web Animations API so I think we actually want to keep this this.

@@ -70,5 @@
> -
> -  var animation = div.getAnimations()[0];
> -  animation.cancel();
> -  assert_equals(animation.playState, 'idle');
> -}, 'Animation returns correct playState when cancelled');

This test is also probably worth keeping (since, as with the cancel tests, it covers an edge case where we have a CSS animation that no longer has an owning element).

::: testing/web-platform/tests/web-animations/animation/cancel.html
@@ +12,5 @@
> +'use strict';
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({'transform': ['none', 'translate(100px)']},

Nit: We don't need to put single quotes around 'transform' here. Just transform is fine.

@@ +21,5 @@
> +    animation.cancel();
> +    assert_equals(getComputedStyle(div).transform, 'none',
> +                  'transform style is no longer animated after cancelling');
> +  });
> +}, 'Animated style is cleared after calling the Animation.cancel()');

Nit: No need for 'the' here

@@ +25,5 @@
> +}, 'Animated style is cleared after calling the Animation.cancel()');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({'marginLeft': ['0px', '100px']},

Nit: just marginLeft (no single quotes) is fine.

@@ +32,5 @@
> +  animation.cancel();
> +  assert_equals(getComputedStyle(div).marginLeft, '0px',
> +                'margin-left style is not animated after cancelling');
> +
> +  animation.currentTime = 50 * 1000;

50 * MS_PER_SEC?

@@ +40,5 @@
> +}, 'After cancelling an animation, it can still be seeked');
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({'marginLeft':['100px', '200px']},

marginLeft

@@ +53,5 @@
> +    return animation.ready;
> +  }).then(function() {
> +    assert_equals(animation.playState, 'running',
> +                  'Animation succeeds in running after being re-started');
> +    t.done();

I don't think we need t.done() right?

::: testing/web-platform/tests/web-animations/animation/play.html
@@ +12,5 @@
> +'use strict';
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({ 'transform': ['none', 'translate(10px)']},

Just transform

Then again, maybe we don' need any animation properties? Just {}?

@@ +13,5 @@
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({ 'transform': ['none', 'translate(10px)']},
> +                              { duration : 100 * MS_PER_SEC, iterations : Infinity});

> 80 chars?

@@ +17,5 @@
> +                              { duration : 100 * MS_PER_SEC, iterations : Infinity});
> +  return animation.ready.then(function() {
> +    // Seek to a time outside the active range so that play() will have to
> +    // snap back to the start
> +    animation.currentTime = -5000;

-5 * MS_PER_SEC?

@@ +22,5 @@
> +    animation.playbackRate = -1;
> +
> +    assert_throws('InvalidStateError',
> +                  function () { animation.play(); },
> +                  'Expect InvalidStateError exception on calling play() ' +

Nit: s/Expect/Expected/

@@ +27,5 @@
> +                  'with a negative playbackRate and infinite-duration ' +
> +                  'animation');
> +  });
> +}, 'play() throws when seeking an infinite-duration animation played in ' +
> +   'reverse');

Nit: blank line after this.

::: testing/web-platform/tests/web-animations/animation/playstate.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Animation.playState</title>

I think we should name this file playState.html

@@ +18,5 @@
> +  assert_equals(animation.playState, 'pending');
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'running');
> +  });
> +}, 'Animation returns correct playState when running');

Maybe we should change this description to:

'Animation.playState reports \'pending\' -> \'running\' when initially played' ?

(I think this test was probably originally written before we implemented the 'pending' state so the description doesn't refer to it.)

@@ +29,5 @@
> +  assert_equals(animation.playState, 'pending');
> +  return animation.ready.then(function() {
> +    assert_equals(animation.playState, 'paused');
> +  });
> +}, 'Animation.playState updates when paused by script');

Nit: We don't need to say 'by script' here.

We should probably change this test description to, 'Animation.playState reports \'pending\' -> \'paused\' when pausing'

@@ +42,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({}, 100 * MS_PER_SEC);
> +  animation.cancel();
> +  animation.currentTime = 50 * 1000;

50 * MS_PER_SEC?

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

Thanks Brian.

I addressed previous patch. And I cahnged'async_test' to 'promise_test' in css animation tests.

Could you please confirm above point?

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

I'm Sorry.
I separated the patch. This patch was reviewed by brian.
So carrying forward r+ from Brian(comment #5)

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Use promise_test instead of async_test in css-animation mochi tests.

Hi Brian.

As mentioned at comment #6, I used promise_test instead of async_test in mochitest.
Could you please confirm this patch?

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: bug1259344-using_promise_test.patch

Oh, I submitted wrong patch.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8736612 [details] [diff] [review]
bug1259344-using_promise_test.patch

Review of attachment 8736612 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that's a lot easier to review!

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

Rebased previous patch. carrying forward "r+" from birtles.

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

Rebased. Carrying "r+" from birtles.

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part2.Use promise_test instead of async_test in css-animation mochitests.

Rebased. Carrying "r+" from birtles.

Comment 14

[Carsten Book [:Tomcat]]

has problems to apply, could you take a look, maybe just need rebasing to inbound

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Move animation mochitest(play/cancel/playstate) to web-platform tests.

Thanks Tomcat.
(In reply to Carsten Book [:Tomcat] from comment #14)
> has problems to apply, could you take a look, maybe just need rebasing to
> inbound
I rebased the patch.
(Maybe, mochitests has changed at bug 1242051 )

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1.Move animation mochitest(play/cancel/playstate) to web-platform tests.

Sorry, Previous patch is wrong. I reattach correct patch.

Comment 17

[Carsten Book [:Tomcat]]

Hi,

failed to apply:

patching file dom/animation/test/css-animations/file_animation-playstate.html
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file dom/animation/test/css-animations/file_animation-playstate.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1259344-using_promise_test.patch

could you take a look ?

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Tomcat,

(In reply to Carsten Book [:Tomcat] from comment #17)
> Hi,
> 
> failed to apply:
> 
> patching file dom/animation/test/css-animations/file_animation-playstate.html
> Hunk #1 FAILED at 2
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/animation/test/css-animations/file_animation-playstate.html.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> bug1259344-using_promise_test.patch
> 
> could you take a look ?
Sorry, I forgot adding the order number to the patch name. The patches of this bug need to apply orderly.
Could you please apply the patch again?

Thanks.

Comment 19

[Carsten Book [:Tomcat]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #18)
> Tomcat,
> 
> (In reply to Carsten Book [:Tomcat] from comment #17)
> > Hi,
> > 
> > failed to apply:
> > 
> > patching file dom/animation/test/css-animations/file_animation-playstate.html
> > Hunk #1 FAILED at 2
> > 1 out of 1 hunks FAILED -- saving rejects to file
> > dom/animation/test/css-animations/file_animation-playstate.html.rej
> > patch failed, unable to continue (try -v)
> > patch failed, rejects left in working directory
> > errors during apply, please fix and qrefresh
> > bug1259344-using_promise_test.patch
> > 
> > could you take a look ?
> Sorry, I forgot adding the order number to the patch name. The patches of
> this bug need to apply orderly.
> Could you please apply the patch again?
> 
> Thanks.

np :) done!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1f7178cc48
https://hg.mozilla.org/integration/mozilla-inbound/rev/fabbda4a082a

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a1f7178cc48
https://hg.mozilla.org/mozilla-central/rev/fabbda4a082a