From bug 1193394 comment 48 the try run suggests we have the following failures: Mochitest-chrome-1 (e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=141558420&repo=try) * dom/animation/test/chrome/test_animation_observers_async.html [single_animation_cancelled_fill] records after animation end - number of records - got +0, expected 1 [pause] records after animation end - number of records - got +0, expected 1 [pause_while_pause_pending] records after animation end - number of records - got +0, expected 1 [aborted_pause] records after animation end - number of records - got +0, expected 1 [single_animation_cancelled_fill:subtree] records after animation end - number of records - got +0, expected 1 [pause:subtree] records after animation end - number of records - got +0, expected 1 [pause_while_pause_pending:subtree] records after animation end - number of records - got +0, expected 1 [aborted_pause:subtree] records after animation end - number of records - got +0, expected 1 * dom/animation/test/chrome/test_animation_performance_warning.html An animation has: transform - An animation has: transform: assert_equals: runningOnCompositor property should match expected false but got true (Followed by many many more) * dom/animation/test/chrome/test_running_on_compositor.html animation is added to compositor when timing.duration is made longer than the current time... (And many more) Mochitest-1 (Opt) / Mochitest-2 (Chrome) (e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=141558420&repo=try) * dom/animation/test/css-animations/test_event-dispatch.html Idle -> After - Idle -> After: assert_equals: expected (number) 100 but got (undefined) undefined Before -> After - Before -> After: assert_equals: expected (number) 100 but got (undefined) undefined After -> Before - After -> Before: assert_equals: expected 0 but got 100 * dom/animation/test/css-animations/test_event-order.html Test same events are ordered by elements Test start and iteration events are ordered by time Test iteration and end events are ordered by time Test start and end events are sorted correctly when fired simultaneously * dom/animation/test/css-transitions/test_event-dispatch.html Idle or Pending -> Active - Idle or Pending -> Active Idle or Pending -> After - Idle or Pending -> After Before -> After - Before -> After After -> Before - After -> Before Calculating the interval start and end time with negative start delay Calculating the interval start and end time with negative end delay * dom/animation/test/mozilla/test_restyles.html CSS animations running on the main-thread should update style on the main thread - got 4, expected (And many many more) I've yet to check for DevTools tests.

As discussed on IRC -- perhaps we can make these tests pass *with* the promise changes, mark them as failing, and then land them just before the promise changes (and then mark them as passing).

I came up with an idea to detect whether we have the conformant Promise handling in test. Though most of test cases have been rewritten to be able to run with/without the conformant Promise handling. Only two test cases test_restyles.html need to be tweaked depending on the Promise handling. With the setup to detect the handling, we can avoid the window that the two test cases accidentally fail by other commits. This is a try with the conformant Promise handling: https://treeherder.mozilla.org/#/jobs?repo=try&revision=483e697b5ae92cd231e2fb80c9f6e30781c5a371 This is a try without it for comparison: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c347453a47ba0dd375a53d9a6eea4c68993f23bb

The try on Android showed me that; 1) test_event-dispatch.html still fails on Android 2) test_composite.html fails on Android too 3) two test cases in test_restyles.html intermittently fail on Android I did intend to use this bug for fixing test_restyles.html but I should open a new bug for it. Note that I've already fixed 3) locally.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > The try on Android showed me that; > > 1) test_event-dispatch.html still fails on Android To be correct, test_event-dispatch.html fails with disabling stylo. :/

Comment on attachment 8926641 [details] Bug 1413817 - Use arrow function in file_restyles.html. https://reviewboard.mozilla.org/r/197878/#review203074

Comment on attachment 8926642 [details] Bug 1413817 - Add a note for observeStyling. https://reviewboard.mozilla.org/r/197880/#review203076 ::: dom/animation/test/mozilla/file_restyles.html:48 (Diff revision 1) > +// NOTE: This function must be called right after browser processd > +// requestAnimationFrames because this function counts requestAnimationFrame What does this mean "after browser processd requestAnimationFrames"? As in, in an IntersectionObserver callback? (Nit: processed)

Comment on attachment 8926643 [details] Bug 1413817 - Run requestAnimationFrame before resolving a Promise in waitForWheelEvent(). https://reviewboard.mozilla.org/r/197882/#review203080 ::: dom/animation/test/mozilla/file_restyles.html:94 (Diff revision 1) > - resolve); > + () => { > + requestAnimationFrame(resolve); > + }); Isn't it better for the call site to wait for the frame? Rather than hiding it in waitForWheelEvent? Shouldn't waitForWheelEvent just wait for a wheel event?

Comment on attachment 8926644 [details] Bug 1413817 - Change style before waiting for a single requestAnimationFrame. https://reviewboard.mozilla.org/r/197884/#review203082 It would be helpful to describe *why* this change is necessary, but it's not a big deal.

Comment on attachment 8926645 [details] Bug 1413817 - Ensure the state just after requestAnimationFrame callback before callling observeStyling(). https://reviewboard.mozilla.org/r/197886/#review203086 I haven't gone through all the cases here but this seems heavy-handed. We should wait for what we actually need (and if we wait on paints as a way of ignoring a restyle, we should either document that or simply force the restyle so we don't need to wait.) ::: dom/animation/test/mozilla/file_restyles.html:130 (Diff revision 1) > - await animation.ready; > + await waitForPaintsAndFrame(); > + > ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); > > var markers = await observeStyling(5); Isn't what we really want to say: await animation.ready; ok(!SpecialPowers.wrap(animation).isRunningOnCompositor)); await waitForPaints(); // Wait for initial restyle to finish var markers = await observeStyling(5); ? Similarly for a number of other cases. ::: dom/animation/test/mozilla/file_restyles.html:176 (Diff revision 1) > - await animation.ready; > + await waitForPaintsAndFrame(); > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > div.animationDuration = '200s'; > > var markers = await observeStyling(5); Although in cases like this where it's the restyle we're causing that we want to ignore, perhaps all we need is: await animation.ready; ok(SpecialPowers.wrap(animation).isRunningOnCompositor); div.animationDuration = '200s'; getComputedStyle(div).animationDuration; var markers = await observeStyling(5); ::: dom/animation/test/mozilla/file_restyles.html:212 (Diff revision 1) > + await waitForPaintsAndFrame(); > + > await animation.finished; Is this right? As I understand it, with the new microtask behavior `animation.finished` will resolve before we process restyles. In that case, isn't what we want to say: await animation.finished; await waitForPaints(); // Wait for final restyle to be processed ? ::: dom/animation/test/mozilla/file_restyles.html:237 (Diff revision 1) > + await waitForPaintsAndFrame(); > + > await animation.finished; Likewise here

Comment on attachment 8926649 [details] Bug 1413817 - Wait for a frame after waiting animation.finished. https://reviewboard.mozilla.org/r/197894/#review203092 ::: commit-message-9639e:3 (Diff revision 1) > +We occasionally observed a restyle record on Android there, probably it's > +the last restyling to remove the animation. Ideally we should wait a paint > +and a frame there, but given that the restyle record is not always observed, > +just waiting a frame should be a safer way, if a paint does not happen, the > +test would be timed out. Does forcing a restyle here fix it?

(In reply to Brian Birtles (:birtles) from comment #20) > Comment on attachment 8926649 [details] > Bug 1413817 - Wait for a frame after waiting animation.finished. > > https://reviewboard.mozilla.org/r/197894/#review203092 > > ::: commit-message-9639e:3 > (Diff revision 1) > > +We occasionally observed a restyle record on Android there, probably it's > > +the last restyling to remove the animation. Ideally we should wait a paint > > +and a frame there, but given that the restyle record is not always observed, > > +just waiting a frame should be a safer way, if a paint does not happen, the > > +test would be timed out. > > Does forcing a restyle here fix it? It would work. What is the best practice to flush style for an element? I'd put it in testcommon.js.

(In reply to Brian Birtles (:birtles) from comment #19) > Although in cases like this where it's the restyle we're causing that we > want to ignore, perhaps all we need is: > > await animation.ready; > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > div.animationDuration = '200s'; > getComputedStyle(div).animationDuration; > > var markers = await observeStyling(5); What I am really worrying about is exactly this case. In this case, when we call observeStyling(), it is right after Animation.ready, that means we are before we process requestAnimationFrame callbacks, thus the first observeStyling (i.e. the first requestAnimationFrame) is mostly no-op. In this particular case, it might be not a matter at all, but it's really error-prone, and will be hard to notice what the matter is there. This case is exactly what we should avoid, for this reason I'd left the comment for observeStyle().

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22) > (In reply to Brian Birtles (:birtles) from comment #19) > > > Although in cases like this where it's the restyle we're causing that we > > want to ignore, perhaps all we need is: > > > > await animation.ready; > > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > > > div.animationDuration = '200s'; > > getComputedStyle(div).animationDuration; > > > > var markers = await observeStyling(5); > > What I am really worrying about is exactly this case. In this case, when we > call observeStyling(), it is right after Animation.ready, that means we are > before we process requestAnimationFrame callbacks, thus the first > observeStyling (i.e. the first requestAnimationFrame) is mostly no-op. In > this particular case, it might be not a matter at all, but it's really > error-prone, and will be hard to notice what the matter is there. This case > is exactly what we should avoid, for this reason I'd left the comment for > observeStyle(). What is error-prone about it?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21) > (In reply to Brian Birtles (:birtles) from comment #20) > > Comment on attachment 8926649 [details] > > Bug 1413817 - Wait for a frame after waiting animation.finished. > > > > https://reviewboard.mozilla.org/r/197894/#review203092 > > > > ::: commit-message-9639e:3 > > (Diff revision 1) > > > +We occasionally observed a restyle record on Android there, probably it's > > > +the last restyling to remove the animation. Ideally we should wait a paint > > > +and a frame there, but given that the restyle record is not always observed, > > > +just waiting a frame should be a safer way, if a paint does not happen, the > > > +test would be timed out. > > > > Does forcing a restyle here fix it? > > It would work. What is the best practice to flush style for an element? > I'd put it in testcommon.js. For these two tests, we are transitioning/animating opacity so I'd just add a line to that test: getComputedStyle(div).opacity; // Flush any final restyles from removing the animation

(In reply to Brian Birtles (:birtles) from comment #23) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #22) > > (In reply to Brian Birtles (:birtles) from comment #19) > > > > > Although in cases like this where it's the restyle we're causing that we > > > want to ignore, perhaps all we need is: > > > > > > await animation.ready; > > > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > > > > > div.animationDuration = '200s'; > > > getComputedStyle(div).animationDuration; > > > > > > var markers = await observeStyling(5); > > > > What I am really worrying about is exactly this case. In this case, when we > > call observeStyling(), it is right after Animation.ready, that means we are > > before we process requestAnimationFrame callbacks, thus the first > > observeStyling (i.e. the first requestAnimationFrame) is mostly no-op. In > > this particular case, it might be not a matter at all, but it's really > > error-prone, and will be hard to notice what the matter is there. This case > > is exactly what we should avoid, for this reason I'd left the comment for > > observeStyle(). > > What is error-prone about it? That's exactly the test case. > > > await animation.ready; > > > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > > > > > div.animationDuration = '200s'; > > > getComputedStyle(div).animationDuration; > > > > > > var markers = await observeStyling(5); In this test case, (I haven't looked following code in this test though), but if the test expects the animation is not throttled in the next 5 frames, the test fails. Since we get 4 restyle records there because of the first no-op requestAnimationFrame.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25) > (In reply to Brian Birtles (:birtles) from comment #23) > > > > await animation.ready; > > > > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > > > > > > > div.animationDuration = '200s'; > > > > getComputedStyle(div).animationDuration; > > > > > > > > var markers = await observeStyling(5); > > In this test case, (I haven't looked following code in this test though), > but if the test expects the animation is not throttled in the next 5 frames, > the test fails. Since we get 4 restyle records there because of the first > no-op requestAnimationFrame. But in this case we're asserting that there are no restyles. Hence we want to ignore the restyle that comes from setting the animationDuration so we force the restyle. If you have a test where you expect `observeStyling(5)` to produce five restyle records then you shouldn't flush style.

(In reply to Brian Birtles (:birtles) from comment #26) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #25) > > (In reply to Brian Birtles (:birtles) from comment #23) > > > > > await animation.ready; > > > > > ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > > > > > > > > > > div.animationDuration = '200s'; > > > > > getComputedStyle(div).animationDuration; > > > > > > > > > > var markers = await observeStyling(5); > > > > In this test case, (I haven't looked following code in this test though), > > but if the test expects the animation is not throttled in the next 5 frames, > > the test fails. Since we get 4 restyle records there because of the first > > no-op requestAnimationFrame. > > But in this case we're asserting that there are no restyles. Hence we want > to ignore the restyle that comes from setting the animationDuration so we > force the restyle. > > If you have a test where you expect `observeStyling(5)` to produce five > restyle records then you shouldn't flush style. Style flush is not a problem, the problem is we are state after animation.ready. Without the style flush the test will fail if the test expectes 5 styles there.

Comment on attachment 8926646 [details] Bug 1413817 - Add a function to check detect whether have conformant Promise handling and set the flag to represent it. https://reviewboard.mozilla.org/r/197888/#review203100 ::: dom/animation/test/mozilla/file_restyles.html:129 (Diff revision 1) > + var div = addDiv(null, { style: 'animation: background-color 100s' }); > + > + var timeAtAnimationStart; > + var eventPromise = new Promise(resolve => { > + div.addEventListener('animationstart', () => { > + timeAtAnimationStart = document.timeline.currentTime; > + resolve(); > + }); > + }); > + > + var animation = div.getAnimations()[0]; > + > + await eventPromise; > + await waitForFrame(); > + > + // If we have the conformant Promise handling, |eventPromise| is fulfilled > + // just after the 'animationstart' callback and Promise in waitForFrame() > + // is also fulfilled right after requestAnimationFrame callback, so > + // both happen on the same tick. > + hasConformantPromiseHandling = > + (document.timeline.currentTime == timeAtAnimationStart); > + > + await ensureElementRemoval(div); I think this could be a lot simpler. e.g. async function isConformant() { return new Promise(resolve => { let resolvedPromise = false; requestAnimationFrame(() => { Promise.resolve().then(() => { resolvedPromise = true; }); }); requestAnimationFrame(() => { resolve(resolvedPromise); }); }); } Then: hasConformantPromiseHandling = await isConformant();

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > Style flush is not a problem, the problem is we are state after > animation.ready. Without the style flush the test will fail if the test > expectes 5 styles there. The test expects 0 styles there.

Comment on attachment 8926647 [details] Bug 1413817 - Tweak only_one_restyling_after_finish_is_called depending on the conformant Promise handling. https://reviewboard.mozilla.org/r/197890/#review203106 ::: dom/animation/test/mozilla/file_restyles.html:230 (Diff revision 1) > + 'Animations running on the compositor should only update style ' + > + 'once after finish() is called'); s/once/twice/ ?

(In reply to Brian Birtles (:birtles) from comment #29) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > Style flush is not a problem, the problem is we are state after > > animation.ready. Without the style flush the test will fail if the test > > expectes 5 styles there. > > The test expects 0 styles there. Yep, I know. I am talking about the case if the test expects 5. I am confident there are such test cases. Ok, Even though the test expects 0 styles and waits for 5 requestAnimationFrame, but actually we have no chance to process restyling in the first requestAnimationFrame, so it's the same thing to observe 4 frames. It's really odd.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31) > (In reply to Brian Birtles (:birtles) from comment #29) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > > Style flush is not a problem, the problem is we are state after > > > animation.ready. Without the style flush the test will fail if the test > > > expectes 5 styles there. > > > > The test expects 0 styles there. > > Yep, I know. I am talking about the case if the test expects 5. I am > confident there are such test cases. Ok, Even though the test expects 0 > styles and waits for 5 requestAnimationFrame, but actually we have no chance > to process restyling in the first requestAnimationFrame, so it's the same > thing to observe 4 frames. It's really odd. I'm saying each test should test what it means to test. We shouldn't be looking for a blanket fix for all tests. If there is a test that expects 5 restyles we shouldn't be flushing style before waiting for restyles. Let's work out what each test should be waiting for and should be testing.

Comment on attachment 8926648 [details] Bug 1413817 - Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling. https://reviewboard.mozilla.org/r/197892/#review203108 ::: dom/animation/test/mozilla/file_restyles.html:418 (Diff revision 1) > + > + // Make sure we start from the state right after requestAnimationFrame. > + await waitForFrame(); I'm curious why this is necessary. The first time we pass through the loop we can't possibly enter the first `if ()` branch since the time won't have changed. And we won't observe any restyling because we've just flushed styles (when we called getAnimations()), right? And after the first time through the loop we are definitely in a rAF callback. Are there other things we are expecting to trigger restyles that we might accidentally observe if we don't do this? Am I missing something? ::: dom/animation/test/mozilla/file_restyles.html:440 (Diff revision 1) > + // unthrottled in this tick, let's see what observes in this tick's > + // restyling process. "...been unthrottled in this tick so let's if we restyle animations in this frame" ?

(In reply to Brian Birtles (:birtles) from comment #33) > Comment on attachment 8926648 [details] > Bug 1413817 - Add a another variant of > restyling_transform_animations_in_scrolled_out_element for the conformant > Promise handling. > > https://reviewboard.mozilla.org/r/197892/#review203108 > > ::: dom/animation/test/mozilla/file_restyles.html:418 > (Diff revision 1) > > + > > + // Make sure we start from the state right after requestAnimationFrame. > > + await waitForFrame(); > > I'm curious why this is necessary. > > The first time we pass through the loop we can't possibly enter the first > `if ()` branch since the time won't have changed. > > And we won't observe any restyling because we've just flushed styles (when > we called getAnimations()), right? > > And after the first time through the loop we are definitely in a rAF > callback. > > Are there other things we are expecting to trigger restyles that we might > accidentally observe if we don't do this? Am I missing something? The main reason is to match refresh driver's time. Without the waitForFrame(), it's in the pushPrefEnv callback. The callback is done at the end of tick, so if we got the refresh driver's time without the wait, we would get the previous time. Actually we do styling at the previous time in such case, so the test might work as well, but might not work.

(In reply to Brian Birtles (:birtles) from comment #32) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #31) > > (In reply to Brian Birtles (:birtles) from comment #29) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > > > Style flush is not a problem, the problem is we are state after > > > > animation.ready. Without the style flush the test will fail if the test > > > > expectes 5 styles there. > > > > > > The test expects 0 styles there. > > > > Yep, I know. I am talking about the case if the test expects 5. I am > > confident there are such test cases. Ok, Even though the test expects 0 > > styles and waits for 5 requestAnimationFrame, but actually we have no chance > > to process restyling in the first requestAnimationFrame, so it's the same > > thing to observe 4 frames. It's really odd. > > I'm saying each test should test what it means to test. We shouldn't be > looking for a blanket fix for all tests. > > If there is a test that expects 5 restyles we shouldn't be flushing style > before waiting for restyles. > > Let's work out what each test should be waiting for and should be testing. That's what I'd like to avoid. There are two typical test cases; var div = addDiv(null, { style: 'animation: opacity 100s' }); var animation = div.getAnimations()[0]; await animation.ready; ok(SpecialPowers.wrap(animation).isRunningOnCompositor); var markers = await observeStyling(5); is(markers.length, 0, This works fine with the conformant Promise handling (To be precise, it's not fine since the first observeStyling is no-op) var div = addDiv(null, { style: 'animation: background-color 100s' }); var animation = div.getAnimations()[0]; await animation.ready; ok(!SpecialPowers.wrap(animation).isRunningOnCompositor); var markers = await observeStyling(5); is(markers.length, 5, This does not work, it gets 4 restyles even if there is a flushing style. We need to advance refresh driver to the state right after requestAnimationFrame callbacks.

To be clear, what I am concerned is inconsistency. The second case needs waitForFrame() after animation.ready (putting it before observeStyle is somewhat confusing since it might not be needed sometimes), whereas the first case does not need it. Adding a wrapper for Animation Promise and waiting requestAnimationFrame in the wrapper is a mitigate solution?

Comment on attachment 8926643 [details] Bug 1413817 - Run requestAnimationFrame before resolving a Promise in waitForWheelEvent(). https://reviewboard.mozilla.org/r/197882/#review203628 I think this is generally fine but I don't think we should hide the requestAnimationFrame call inside waitForWheelEvent. We should keep waitForWheelEvent just doing one thing, and hiding the requestAnimationFrame call would make the test that uses it harder to understand (since we'll assume we are after the restyle). We should just add that at each call site (similar to what we discussed in test_animaton_observers_async.html). Clearing review request for now just because I expect the patch will change a lot--otherwise the approach seems fine.

Comment on attachment 8926649 [details] Bug 1413817 - Wait for a frame after waiting animation.finished. https://reviewboard.mozilla.org/r/197894/#review203638 As discussed, clearing the review on this patch for now until we know what is going on.

Comment on attachment 8926642 [details] Bug 1413817 - Add a note for observeStyling. https://reviewboard.mozilla.org/r/197880/#review203648 Clearing review because I think we want to try making observeStyling not include the first rAF callback if it is the same frame? ::: dom/animation/test/mozilla/file_restyles.html:48 (Diff revision 1) > +// NOTE: This function must be called right after browser processd > +// requestAnimationFrames because this function counts requestAnimationFrame Ok, I think I've understood this comment now. I think I'd like to either: 1) Use the same approach as we discussed for getNextFrame to make sure it counts actual frames that have passed. OR 2) Update the comment to describe a little more clearly how it works (rather than saying "you must do it this way") perhaps even with a code example. But probably (1) is better if it works.

Closing since all dependent bugs have been fixed now. Once bug 1193394 gets landed, a new orange in test_restyles.html will appear (see in the try in bug 1428692 comment 8). I'd like to handle it in a new intermittent bug (I'd like to avoid introducing more complex code in the test file).