Fix animation tests that rely on Firefox's non-conformant Promise handling

ASSIGNED
Assigned to

Status

()

Core
DOM: Animation
ASSIGNED
20 days ago
12 days ago

People

(Reporter: birtles, Assigned: hiro)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
birtles
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
(Reporter)

Description

20 days ago
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.
(Assignee)

Updated

20 days ago
Duplicate of this bug: 1388979
(Reporter)

Comment 2

15 days ago
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).
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Updated

15 days ago
Blocks: 1415007
(Assignee)

Updated

15 days ago
No longer blocks: 1415007
Depends on: 1415007
(Assignee)

Updated

15 days ago
Depends on: 1415010
(Assignee)

Updated

15 days ago
Depends on: 1415042
(Assignee)

Updated

14 days ago
Depends on: 1415346
(Assignee)

Updated

14 days ago
No longer depends on: 1415007
(Assignee)

Updated

14 days ago
Depends on: 1415399
(Assignee)

Updated

14 days ago
Summary: Fix animation tests that rely on Firefox's non-comformant Promise handling → Fix animation tests that rely on Firefox's non-conformant Promise handling
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. :/
(Assignee)

Updated

13 days ago
Depends on: 1415729
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

13 days ago
Depends on: 1415734
(Reporter)

Comment 15

13 days ago
mozreview-review
Comment on attachment 8926641 [details]
Bug 1413817 - Use arrow function in file_restyles.html.

https://reviewboard.mozilla.org/r/197878/#review203074
Attachment #8926641 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 16

13 days ago
mozreview-review
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)
(Reporter)

Comment 17

13 days ago
mozreview-review
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?
(Reporter)

Comment 18

13 days ago
mozreview-review
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.
Attachment #8926644 - Flags: review?(bbirtles) → review+
(Reporter)

Comment 19

13 days ago
mozreview-review
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
Attachment #8926645 - Flags: review?(bbirtles)
(Reporter)

Comment 20

13 days ago
mozreview-review
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().
(Reporter)

Comment 23

13 days ago
(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?
(Reporter)

Comment 24

13 days ago
(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.
(Reporter)

Comment 26

13 days ago
(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.
(Reporter)

Comment 28

13 days ago
mozreview-review
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();
Attachment #8926646 - Flags: review?(bbirtles)
(Reporter)

Comment 29

13 days ago
(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.
(Reporter)

Comment 30

13 days ago
mozreview-review
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/ ?
Attachment #8926647 - Flags: review?(bbirtles) → review+
(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.
(Reporter)

Comment 32

13 days ago
(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.
(Reporter)

Comment 33

13 days ago
mozreview-review
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"

?
Attachment #8926648 - Flags: review?(bbirtles) → review+
(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.
(Assignee)

Updated

13 days ago
Depends on: 1415780
(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?
(Reporter)

Comment 37

12 days ago
mozreview-review
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.
Attachment #8926643 - Flags: review?(bbirtles)
(Reporter)

Comment 38

12 days ago
mozreview-review
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.
Attachment #8926649 - Flags: review?(bbirtles)
(Reporter)

Comment 39

12 days ago
mozreview-review
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.
Attachment #8926642 - Flags: review?(bbirtles)
You need to log in before you can comment on or make changes to this bug.