Closed Bug 1259344 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(2 files, 10 obsolete files)

8.20 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
12.66 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
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
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
Summary: Move animation mochitest of play/cancel/playbackrate api to web-platform-tests → Move animation mochitest of play/cancel/playstate api 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?
Attachment #8735700 - Attachment is obsolete: true
Attachment #8736071 - Flags: review?(bbirtles)
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?
Attachment #8736071 - Flags: review?(bbirtles) → review+
Thanks Brian.

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

Could you please confirm above point?
Attachment #8736071 - Attachment is obsolete: true
Attachment #8736599 - Flags: review?(bbirtles)
I'm Sorry.
I separated the patch. This patch was reviewed by brian.
So carrying forward r+ from Brian(comment #5)
Attachment #8736599 - Attachment is obsolete: true
Attachment #8736599 - Flags: review?(bbirtles)
Attachment #8736606 - Flags: review+
Hi Brian.

As mentioned at comment #6, I used promise_test instead of async_test in mochitest.
Could you please confirm this patch?
Attachment #8736610 - Flags: review?(bbirtles)
Oh, I submitted wrong patch.
Attachment #8736610 - Attachment is obsolete: true
Attachment #8736610 - Flags: review?(bbirtles)
Attachment #8736612 - Flags: review?(bbirtles)
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!
Attachment #8736612 - Flags: review?(bbirtles) → review+
Rebased previous patch. carrying forward "r+" from birtles.
Attachment #8736606 - Attachment is obsolete: true
Attachment #8737017 - Flags: review+
Rebased. Carrying "r+" from birtles.
Attachment #8737017 - Attachment is obsolete: true
Attachment #8737981 - Flags: review+
Rebased. Carrying "r+" from birtles.
Attachment #8736612 - Attachment is obsolete: true
Attachment #8737982 - Flags: review+
Keywords: checkin-needed
has problems to apply, could you take a look, maybe just need rebasing to inbound
Flags: needinfo?(mantaroh)
Keywords: checkin-needed
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 )
Attachment #8737981 - Attachment is obsolete: true
Flags: needinfo?(mantaroh)
Attachment #8738033 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Sorry, Previous patch is wrong. I reattach correct patch.
Attachment #8738033 - Attachment is obsolete: true
Attachment #8738035 - Flags: review+
Keywords: checkin-needed
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 ?
Flags: needinfo?(mantaroh)
Attachment #8738035 - Attachment description: Move animation mochitest(play/cancel/playstate) to web-platform tests. → Part1.Move animation mochitest(play/cancel/playstate) to web-platform tests.
Flags: needinfo?(mantaroh)
Attachment #8737982 - Attachment description: Use promise_test instead of async_test in css-animation mochitests. → Part2.Use promise_test instead of async_test in css-animation mochitests.
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.
(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!
https://hg.mozilla.org/mozilla-central/rev/3a1f7178cc48
https://hg.mozilla.org/mozilla-central/rev/fabbda4a082a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.