Below two test cases in test-animation-pausing.html failed on a try [1] with the conformant Promise handling and performing micro task checkpoint in Animation tick. * play() overrides animation-play-state * play() flushes pending changes to animation-play-state first Initially I thought it was caused by the same reason as bug 1422995, but actually it's not, it was the same issue as bug 1418268. mPendingReadyTime is clamped to the current time, then computed style did not change in the next frame. That's said, I think, as for these test cases, we don't need to check the computed style in the *very* next frame. So I think we can use waitForNextFrame there, that means check the computed style in the frame after the next frame. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=683622b6bba07caec14c439a56aadd843b40f532&selectedJob=149497688

Brian, what do you think about the mitigating way, i.e. using waitForNextFrame?

Yeah, that tests seems wrong. It's totally valid for an implementation to still be at time zero when the ready promise fires. Waiting for a frame seems fine (I suppose even just querying animation.playState would be fine too but that's complicated by the fact that we have a pref that changes the behavior there, so let's not do that yet).

Comment on attachment 8934754 [details] Bug 1423078 - Use assert_greater_than instead of assert_true. https://reviewboard.mozilla.org/r/205640/#review211230

Comment on attachment 8934755 [details] Bug 1423078 - Use waitForNextFrame() instead of waitForFrame() to make sure the second restyle happen after the initial paint. https://reviewboard.mozilla.org/r/205642/#review211236 Really we should rewrite these -- since we control the value of the pending-member pref in these tests, we can just test playState now so we don't need to check marginLeft. But this is fine for now. There's just one test below where waiting for the next frame doesn't seem to make sense. ::: dom/animation/test/css-animations/file_animation-pausing.html:65 (Diff revision 1) > var previousAnimVal; > > return animation.ready.then(function() { > div.style.animationPlayState = 'running'; > cs.animationPlayState; // Trigger style resolution > - return waitForFrame(); > + return waitForNextFrame(); It's not clear why this is necessary. I think what this test wants to do, is: 1. Use play() to override animation-play-state 2. Make animation-play-state running 3. Make animation-play-state paused 4. Check that the animation is paused. It's only at (4)--where we're looking at the value of margin-left--that we actually care about how many frames we've rendered. Although, so long as we're determining if we're paused based on the marginLeft, it would be good to make sure we wait long enough at (1) for the value to become non-zero. Ideally we should rewrite this test as: var animation = div.getAnimations()[0]; assert_equals(getMarginLeft(cs), 0, 'Initial value of margin-left is zero'); // Override animation-play-state using API animation.play(); assert_equals( animation.playState, 'running', 'Should be running after calling play()' ); // Make animation-play-state also running div.style.animationPlayState = 'running'; cs.animationPlayState; // Trigger style resolution assert_equals( animation.playState, 'running', 'Should still be running after setting animation-play-state: running' ); // Then make animation-play-state paused div.style.animationPlayState = 'running'; assert_equals(animation.playState, 'paused'); If for some reason we need to keep using marginLeft then I think this should probably be: var animation = div.getAnimations()[0]; assert_equals(getMarginLeft(cs), 0, 'Initial value of margin-left is zero'); // Override animation-play-state using API animation.play(); var previousAnimVal; return animation.ready.then(waitForNextFrame).then(function() { previousAnimVal = getMarginLeft(cs); assert_greater_than(previousAnimVal, 0, 'Animation should be moved past its first frame'); // Make animation-play-state also running div.style.animationPlayState = 'running'; cs.animationPlayState; // Trigger style resolution // Make animation-play-state paused div.style.animationPlayState = 'paused'; return animation.ready; }).then(waitForNextFrame).then(function() { assert_equals(getMarginLeft(cs), previousAnimVal, 'Animated value of margin-left does not change when' + ' paused by style'); }); ::: dom/animation/test/css-transitions/file_animation-pausing.html:27 (Diff revision 1) > var animation = div.getAnimations()[0]; > assert_equals(getMarginLeft(cs), 0, > 'Initial value of margin-left is zero'); > var previousAnimVal = getMarginLeft(cs); > > - animation.ready.then(waitForFrame).then(t.step_func(function() { > + animation.ready.then(waitForNextFrame).then(t.step_func(function() { (I just noticed we're using t.step_func here but we don't need that with promise_test. http://web-platform-tests.org/writing-tests/testharness-api.html#promise-tests)

Comment on attachment 8934756 [details] Bug 1423078 - Use waitForNextFrame() instead of waitForFrame() to make sure the next frame. https://reviewboard.mozilla.org/r/205644/#review211240

(In reply to Brian Birtles (:birtles) from comment #8) > Comment on attachment 8934755 [details] > Bug 1423078 - Use waitForNextFrame() instead of waitForFrame() to make sure > the second restyle happen after the initial paint. > > https://reviewboard.mozilla.org/r/205642/#review211236 > > Really we should rewrite these -- since we control the value of the > pending-member pref in these tests, we can just test playState now so we > don't need to check marginLeft. > > But this is fine for now. There's just one test below where waiting for the > next frame doesn't seem to make sense. > > ::: dom/animation/test/css-animations/file_animation-pausing.html:65 > (Diff revision 1) > > var previousAnimVal; > > > > return animation.ready.then(function() { > > div.style.animationPlayState = 'running'; > > cs.animationPlayState; // Trigger style resolution > > - return waitForFrame(); > > + return waitForNextFrame(); > > It's not clear why this is necessary. Indeed, we don't need the change at least for this test case. But I just wanted to avoid mis-usage in this kind of situation, so I did change places that we do wait for Animation.ready after creating an animation and call waitForFrame(). My concern does not matter if we rewrite these tests with playState soonish though. Anyway I filed bug 1423411 for the rewrite. And I will leave the change as it is here for the concern. Thanks! > ::: dom/animation/test/css-transitions/file_animation-pausing.html:27 > (Diff revision 1) > > var animation = div.getAnimations()[0]; > > assert_equals(getMarginLeft(cs), 0, > > 'Initial value of margin-left is zero'); > > var previousAnimVal = getMarginLeft(cs); > > > > - animation.ready.then(waitForFrame).then(t.step_func(function() { > > + animation.ready.then(waitForNextFrame).then(t.step_func(function() { > > (I just noticed we're using t.step_func here but we don't need that with > promise_test. > http://web-platform-tests.org/writing-tests/testharness-api.html#promise- > tests) css-transitions/file_animation-pausing.html still uses async_test, I don't know why.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4cede8efde31 Use assert_greater_than instead of assert_true. r=birtles https://hg.mozilla.org/integration/autoland/rev/04e16e55a671 Use waitForNextFrame() instead of waitForFrame() to make sure the second restyle happen after the initial paint. r=birtles https://hg.mozilla.org/integration/autoland/rev/95da7e13e0bd Use waitForNextFrame() instead of waitForFrame() to make sure the next frame. r=birtles

https://hg.mozilla.org/mozilla-central/rev/4cede8efde31 https://hg.mozilla.org/mozilla-central/rev/04e16e55a671 https://hg.mozilla.org/mozilla-central/rev/95da7e13e0bd