1415399 - Make test_animaton_observers_async.html work with conformant Promise handling

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c2f16ca83316dbc9c24ca020109b8f87cec36a3

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415399 - Use arrow function in test_animation_observers_{async,sync}.html.

Review commit: https://reviewboard.mozilla.org/r/197478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197478/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415399 - Advance the time to right after requestAnimationFrame was processed when we received animation events.

Some test cases in test_animation_observers_async.html are not supposed to
process their code between styling and requestAnimationFrame.  For example;

  target.style = 'animation: none';
  yield waitForFrame();

If we processed the first line in the example inside the callback of an
animation event, the new style would have not been processed after the second
line (i.e. after a requestAnimationFrame) since requestAnimationFrame is
processed soon after the animation event's callback.  To mitigate this,
we wait for a requestAnimationFrame in the event callback here.  Though we
can't use this mitigating way for timing-specific test cases
(e.g. test_restyles.html), we can safely use it in
test_animation_observers_async.html since we don't check animation records
frame by frame.

Review commit: https://reviewboard.mozilla.org/r/197480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197480/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

The problematic situation here is pretty similar to the previous commit, but
this time it's caused by Animation.ready.  Animation.ready is normally
fulfilled in Animation::Tick() which is called at the very beginning of
refresh driver's tick (just before animation event handling), so the same thing
in the previous commit happens here too.

Note that I did intentionally leave other Animation.ready(s) that did not break
with conformant Promise handling (bug 1193394) since waiting for a frame makes
this test much slower (we did actually split the files due to the slowness in
bug 1283754).  Hopefully once we have the conformant Promise handling, we will
know this kind of mistakes on our CI if someone tries to add it (actually that
someone should know the mistakes locally though).

Review commit: https://reviewboard.mozilla.org/r/197482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197482/

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8926222 [details]
Bug 1415399 - Use arrow function in test_animation_observers_{async,sync}.html.

https://reviewboard.mozilla.org/r/197478/#review202714

Comment 5

[Brian Birtles (:birtles)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

https://reviewboard.mozilla.org/r/197482/#review202708

::: commit-message-01317:1
(Diff revision 1)
> +Bug - Wait for a frame instead of waiting Animation.ready Promise. r?birtles

I'm guessing this wants a bug number?

::: dom/animation/test/chrome/test_animation_observers_async.html:974
(Diff revision 1)
> -    yield animations[0].ready;
> +    // Note: We have to wait for a frame instead of Animation.ready since we
> +    // modify style for the target element and wait for a frame below.
> +    yield waitForFrame();

I don't quite follow.

I thought the change here was something like:

Current behavior:

  Tick:
    Run style observers
      Tick timeline
        Tick animations
          Resolve ready promise
          (queues task)
    (Run CSS animation/transition event callbacks)
    Run requestAnimationFrame callbacks
    Restyle
  Finally run promise tasks

New behavior:

  Tick:
    Microtask checkpoint - any promise tasks are run now
    Run style observers
      Tick timeline
        Tick animations
          Resolve ready promise
          (queues task)
    (Do we also run a microtask checkpoint here?)
    (Run CSS animation/transition event callbacks)
    Run requestAnimationFrame callbacks
    Restyle

Perhaps I should review this after part 2 is ready? At very least, I think this comment could use some work.

Comment 6

[Brian Birtles (:birtles)]

For my own reference, the spec link is: https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Comment on attachment 8926224 [details]
> Bug - Wait for a frame instead of waiting Animation.ready Promise.
> 
> https://reviewboard.mozilla.org/r/197482/#review202708
> 
> ::: commit-message-01317:1
> (Diff revision 1)
> > +Bug - Wait for a frame instead of waiting Animation.ready Promise. r?birtles
> 
> I'm guessing this wants a bug number?
> 
> ::: dom/animation/test/chrome/test_animation_observers_async.html:974
> (Diff revision 1)
> > -    yield animations[0].ready;
> > +    // Note: We have to wait for a frame instead of Animation.ready since we
> > +    // modify style for the target element and wait for a frame below.
> > +    yield waitForFrame();
> 
> I don't quite follow.
> 
> I thought the change here was something like:
> 
> Current behavior:
> 
>   Tick:
>     Run style observers
>       Tick timeline
>         Tick animations
>           Resolve ready promise
>           (queues task)
>     (Run CSS animation/transition event callbacks)
>     Run requestAnimationFrame callbacks
>     Restyle
>   Finally run promise tasks
> 
> New behavior:
> 
>   Tick:
>     Microtask checkpoint - any promise tasks are run now
>     Run style observers
>       Tick timeline
>         Tick animations
>           Resolve ready promise
>           (queues task)
>     (Do we also run a microtask checkpoint here?)

Yes, right.

So, once after we waited for an Animation.ready Promise, we are in the state before requestAnimationFrame, so

  yield animation.ready;
  target.style.animation = 'none'; // Here we are in the middle between "Tick animations" and requestAnimationFrame callbacks.
  yield waitForFrame(); // This is fulfilled before we process styling.

Another mitigating way is use waitForPaints() instead of waitForFrame() but even with waitForPaint(), other similar problem will happen since MozAfterPaint is processed after a paint, so we have no chance to process restyling between the MozAfterPaint event and requestAnimationFrame.  Anyway, either way is fine for me, but we have no silver bullet for now.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926223 [details]
Bug 1415399 - Advance the time to right after requestAnimationFrame was processed when we received animation events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197480/diff/1-2/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197482/diff/1-2/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926223 [details]
Bug 1415399 - Advance the time to right after requestAnimationFrame was processed when we received animation events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197480/diff/2-3/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197482/diff/2-3/

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8926223 [details]
Bug 1415399 - Advance the time to right after requestAnimationFrame was processed when we received animation events.

https://reviewboard.mozilla.org/r/197480/#review203070

::: commit-message-88a84:3
(Diff revision 3)
> +Some test cases in test_animation_observers_async.html are not supposed to
> +process their code between styling and requestAnimationFrame.  For example;
> +
> +  target.style = 'animation: none';
> +  yield waitForFrame();
> +
> +If we processed the first line in the example inside the callback of an
> +animation event, the new style would have not been processed after the second
> +line (i.e. after a requestAnimationFrame) since requestAnimationFrame is
> +processed soon after the animation event's callback.  To mitigate this,

This could be more clear. For example,


HTML defines[1] that within a frame we perform the following steps in order
(omitting unrelated steps):

  1. Run CSS animation/transition event handlers
  2. Run requestAnimationFrame callbacks
  3. Restyle

Now, suppose that in (1) we run the following code:

  target.style = 'animation: none';
  yield waitForFrame();

In (2) we will resolve the Promise returned by the call to `waitForFrame`.
Prior to bug 1193394, the callback for that Promise would not be run until after
(3). However, after bug 1193394 it will be run before (3). That means that the
code following after `yield waitForFrame()` will run before the restyle
triggered by the previous line is processed.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering

::: dom/animation/test/chrome/test_animation_observers_async.html:110
(Diff revision 3)
>  // Adds an event listener and returns a Promise that is resolved when the
>  // event listener is called.
>  function await_event(aElement, aEventName) {
>    return new Promise(aResolve => {
>      function listener(aEvent) {
>        aElement.removeEventListener(aEventName, listener);
> -      aResolve();
> +      requestAnimationFrame(aResolve);
>      }
>      aElement.addEventListener(aEventName, listener);
>    });
>  }

This doesn't seem like the right fix. A function called await_event should just wait for an event.

In the example from the commit message, we should just flush styles (or wait two frames if need be).

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

https://reviewboard.mozilla.org/r/197482/#review203072

::: dom/animation/test/chrome/test_animation_observers_async.html:973
(Diff revision 3)
>      // Wait to ensure no change is dispatched
> -    yield animations[0].ready;
> +    // Note: We have to wait for a frame instead of Animation.ready since we
> +    // modify style for the target element and wait for a frame below.
> +    yield waitForFrame();
>      assert_records([], "records after redundant pause()");
>  
>      // Cancel the animation.
>      e.style = "";
>  
>      // Wait for the single removal notification.
>      yield waitForFrame();

This doesn't seem like the right fix here either. We should just flush styles (and if that ever proves problematic, we should wait two frames and add a comment saying "We need to wait two frames because this restyle won't be processed before requestAnimationFrames are next run").

The same problem would occur if we were to use regular events without wrapping them in Promises so we shouldn't wait on something different (a frame) just because Promises previously worked differently.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #12)
> Comment on attachment 8926223 [details]
> Bug 1415399 - Advance the time to right after requestAnimationFrame was
> processed when we received animation events.
> 
> https://reviewboard.mozilla.org/r/197480/#review203070
> 
> ::: commit-message-88a84:3
> (Diff revision 3)
> > +Some test cases in test_animation_observers_async.html are not supposed to
> > +process their code between styling and requestAnimationFrame.  For example;
> > +
> > +  target.style = 'animation: none';
> > +  yield waitForFrame();
> > +
> > +If we processed the first line in the example inside the callback of an
> > +animation event, the new style would have not been processed after the second
> > +line (i.e. after a requestAnimationFrame) since requestAnimationFrame is
> > +processed soon after the animation event's callback.  To mitigate this,
> 
> This could be more clear. For example,
> 
> 
> HTML defines[1] that within a frame we perform the following steps in order
> (omitting unrelated steps):
> 
>   1. Run CSS animation/transition event handlers
>   2. Run requestAnimationFrame callbacks
>   3. Restyle
> 
> Now, suppose that in (1) we run the following code:
> 
>   target.style = 'animation: none';
>   yield waitForFrame();
> 
> In (2) we will resolve the Promise returned by the call to `waitForFrame`.
> Prior to bug 1193394, the callback for that Promise would not be run until
> after
> (3). However, after bug 1193394 it will be run before (3). That means that
> the
> code following after `yield waitForFrame()` will run before the restyle
> triggered by the previous line is processed.
> 
> [1]
> https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering
> 
> ::: dom/animation/test/chrome/test_animation_observers_async.html:110
> (Diff revision 3)
> >  // Adds an event listener and returns a Promise that is resolved when the
> >  // event listener is called.
> >  function await_event(aElement, aEventName) {
> >    return new Promise(aResolve => {
> >      function listener(aEvent) {
> >        aElement.removeEventListener(aEventName, listener);
> > -      aResolve();
> > +      requestAnimationFrame(aResolve);
> >      }
> >      aElement.addEventListener(aEventName, listener);
> >    });
> >  }
> 
> This doesn't seem like the right fix. A function called await_event should
> just wait for an event.
> 
> In the example from the commit message, we should just flush styles (or wait
> two frames if need be).

Ok, so adding a wrapper that wait for an event and wait for requestAnimationFrame just like I did in https://reviewboard.mozilla.org/r/197886/diff/1#index_header works for you?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8926224 [details]
> Bug 1415399 - Wait for a frame instead of waiting Animation.ready Promise.
> 
> https://reviewboard.mozilla.org/r/197482/#review203072
> 
> ::: dom/animation/test/chrome/test_animation_observers_async.html:973
> (Diff revision 3)
> >      // Wait to ensure no change is dispatched
> > -    yield animations[0].ready;
> > +    // Note: We have to wait for a frame instead of Animation.ready since we
> > +    // modify style for the target element and wait for a frame below.
> > +    yield waitForFrame();
> >      assert_records([], "records after redundant pause()");
> >  
> >      // Cancel the animation.
> >      e.style = "";
> >  
> >      // Wait for the single removal notification.
> >      yield waitForFrame();
> 
> This doesn't seem like the right fix here either. We should just flush
> styles (and if that ever proves problematic, we should wait two frames and
> add a comment saying "We need to wait two frames because this restyle won't
> be processed before requestAnimationFrames are next run").
> 
> The same problem would occur if we were to use regular events without
> wrapping them in Promises so we shouldn't wait on something different (a
> frame) just because Promises previously worked differently.

OK, I will take another approach in comment 7, i.e. use waitForPaints() after the style change there.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Ok, so adding a wrapper that wait for an event and wait for
> requestAnimationFrame just like I did in
> https://reviewboard.mozilla.org/r/197886/diff/1#index_header works for you?

As I commented there, that doesn't seem like the right approach.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926222 [details]
Bug 1415399 - Use arrow function in test_animation_observers_{async,sync}.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197478/diff/1-2/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197482/diff/3-4/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> > Ok, so adding a wrapper that wait for an event and wait for
> > requestAnimationFrame just like I did in
> > https://reviewboard.mozilla.org/r/197886/diff/1#index_header works for you?
> 
> As I commented there, that doesn't seem like the right approach.

I did use waitForPaints() after style changes.  I am worrying that it might cause timeout on Android, but it happens, I will split this test into more smaller pieces.

Comment 20

[Brian Birtles (:birtles)]

What's wrong with flushing styles?

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #20)
> What's wrong with flushing styles?

I think in some places we also need time to deliver animation records to the observer.

Comment 22

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> (In reply to Brian Birtles (:birtles) from comment #20)
> > What's wrong with flushing styles?
> 
> I think in some places we also need time to deliver animation records to the
> observer.

Can you please check? Thank you!

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #22)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> > (In reply to Brian Birtles (:birtles) from comment #20)
> > > What's wrong with flushing styles?
> > 
> > I think in some places we also need time to deliver animation records to the
> > observer.
> 
> Can you please check? Thank you!

All failure cases with the conformant Promise handling.   Attachment 8926224 [details] has actually overkills but I'd like to do replace all the usage for consistency.

Comment 24

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Brian Birtles (:birtles) from comment #22)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> > > (In reply to Brian Birtles (:birtles) from comment #20)
> > > > What's wrong with flushing styles?
> > > 
> > > I think in some places we also need time to deliver animation records to the
> > > observer.
> > 
> > Can you please check? Thank you!
> 
> All failure cases with the conformant Promise handling.

What does that mean? That even if you flush style they all fail? Can you explain the exact timing of the mutation observer dispatch in this case?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > (In reply to Brian Birtles (:birtles) from comment #22)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> > > > (In reply to Brian Birtles (:birtles) from comment #20)
> > > > > What's wrong with flushing styles?
> > > > 
> > > > I think in some places we also need time to deliver animation records to the
> > > > observer.
> > > 
> > > Can you please check? Thank you!
> > 
> > All failure cases with the conformant Promise handling.
> 
> What does that mean? That even if you flush style they all fail? Can you
> explain the exact timing of the mutation observer dispatch in this case?

That's because there is no chance that the callback for the mutation observer is called.  If we do use takeRecords() there, it should work.  But introducing takeRecords() in async test cases in this file will bring us lots of complexity.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

After some more thought, we could move such test cases in test_animation_observers_sync.html, once they have moved, we no longer need to wait for a frame there.

Comment 27

[Brian Birtles (:birtles)]

Sorry for the delay here. I've been having lots of trouble building on Windows (my Linux machine is still at the repair shop) and it seems I need to build it in order to get a proper answer to comment 24.

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

https://reviewboard.mozilla.org/r/197482/#review203610

Clearing review for now since I think there will be changes required regardless of which approach we opt for.

::: dom/animation/test/chrome/test_animation_observers_async.html:250
(Diff revision 4)
>      yield waitForFrame();
>      assert_records([{ added: animations, changed: [], removed: [] }],
>                     "records after transition start");
>  
>      // Cancel the transition by setting transition-property.
>      e.style.transitionProperty = "none";
>  
>      // Wait for the single MutationRecord for the Animation removal to
>      // be delivered.
> -    yield waitForFrame();
> +    yield waitForPaints();
>      assert_records([{ added: [], changed: [], removed: animations }],
>                     "records after transition end");

(See comments below but I'd rather we don't change this code if we don't need to. It seems to pass for me without changes.)

::: dom/animation/test/chrome/test_animation_observers_async.html:598
(Diff revision 4)
>      animations[0].currentTime = 99900;
>      yield animationEnd;
>  
>      // The only MutationRecord at this point should be the change from
>      // seeking the Animation.
>      assert_records([{ added: [], changed: animations, removed: [] }],
>                     "records after animation starts filling");
>  
>      // Cancel the animation by setting animation-fill-mode.
>      e.style.animationFillMode = "none";
>  
>      // Wait for the single MutationRecord for the Animation removal to
>      // be delivered.
> -    yield waitForFrame();
> +    yield waitForPaints();
>      assert_records([{ added: [], changed: [], removed: animations }],
>                     "records after animation end");

As far as I can tell, this is the first test that actually fails.

Waiting for paints feels like the wrong thing here: we know there are problem with paints firing from overlapping transaction IDs, from synthetic events, etc. etc. Also, if the animation doesn't trigger a paint we might timeout.

So, I'd rather avoid waiting for paints where possible. (Waiting for paints will also mean we can't ever export this to web-platform-tests if Animation MutationObservers gets standardized.)

In this case we simply want to:

1. change style
2. let that restyle happen
3. allow mutation records from that restyle to be dispatched

In most cases waiting for a frame works since we are processing requestAnimationFrame callbacks and any *new* calls to requestAnimationFrame will be run during the *next* frame.

In this test, however, we are in the animationend event callback so a call to requestAnimationFrame will land us in *this* animation frame and before restyling.

I think we have two options:

1. Just flush style here, i.e. call

    getComputedStyle(e).animationFillMode;

   after we update the animationFillMode.
   I've tested this locally and it appears to work (I think the confusion in the previous discussion was that we dropped the call to waitForFrame?).

2. After the call to `animationEnd`, call `await waitForFrame` before continuing, that way ensuring we are always in requestAnimationFrames.
It seems like the simplest thing here is just to flush style in (2) (but keep the waitForFrame).

(2) might be simpler since it isolates the complexity to the few places where we are waiting on events.

What do you think?

Comment 29

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #28)
> It seems like the simplest thing here is just to flush style in (2) (but
> keep the waitForFrame).

(Sorry, ignore this sentence. It comes from a previous draft of this message.)

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #28)
> So, I'd rather avoid waiting for paints where possible. (Waiting for paints
> will also mean we can't ever export this to web-platform-tests if Animation
> MutationObservers gets standardized.)
> 
> In this case we simply want to:
> 
> 1. change style
> 2. let that restyle happen
> 3. allow mutation records from that restyle to be dispatched

To clarify, do you want to put style flush all over the places what we do change styles?

> In most cases waiting for a frame works since we are processing
> requestAnimationFrame callbacks and any *new* calls to requestAnimationFrame
> will be run during the *next* frame.
> 
> In this test, however, we are in the animationend event callback so a call
> to requestAnimationFrame will land us in *this* animation frame and before
> restyling.
> 
> I think we have two options:
> 
> 1. Just flush style here, i.e. call
> 
>     getComputedStyle(e).animationFillMode;
> 
>    after we update the animationFillMode.
>    I've tested this locally and it appears to work (I think the confusion in
> the previous discussion was that we dropped the call to waitForFrame?).

Right.  I dropped the waitForFrame.

> 2. After the call to `animationEnd`, call `await waitForFrame` before
> continuing, that way ensuring we are always in requestAnimationFrames.
> It seems like the simplest thing here is just to flush style in (2) (but
> keep the waitForFrame).
> 
> (2) might be simpler since it isolates the complexity to the few places
> where we are waiting on events.
> 
> What do you think?

Yeah, actually I prefer 2), and I did such thing in the bug for test_restyles.html.

Comment 31

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> (In reply to Brian Birtles (:birtles) from comment #28)
> > So, I'd rather avoid waiting for paints where possible. (Waiting for paints
> > will also mean we can't ever export this to web-platform-tests if Animation
> > MutationObservers gets standardized.)
> > 
> > In this case we simply want to:
> > 
> > 1. change style
> > 2. let that restyle happen
> > 3. allow mutation records from that restyle to be dispatched
> 
> To clarify, do you want to put style flush all over the places what we do
> change styles?

No, that's just one option that we could use where needed as discussed below.

...
> > 2. After the call to `animationEnd`, call `await waitForFrame` before
> > continuing, that way ensuring we are always in requestAnimationFrames.
> > It seems like the simplest thing here is just to flush style in (2) (but
> > keep the waitForFrame).
> > 
> > (2) might be simpler since it isolates the complexity to the few places
> > where we are waiting on events.
> > 
> > What do you think?
> 
> Yeah, actually I prefer 2), and I did such thing in the bug for
> test_restyles.html.

Ok, let's do that.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

After more debugging into detail.  It's more complicated,  actually it's confusing.  Yeah, I had been tracking down animation events case initially so what I knew is in below case;

  yield animationEnd;
  
  // do something 

In this case, after the yield, subsequent script is processed between animation events callbacks and requestAnimationFrame callbacks handling.  This is *NOT* confusing.

After I knew this fact, I checked that where Animation.ready Promise is processed in C++ code.  I found and checked it's being called from Animation::Tick() which is called from WillRefresh() in nsRefreshDriver::Tick(), also the WillRefresh is called before DispatchAnimationEvents().   This is *NOT* confusing so far.  After I knew this fact, I made a big mistake.  I did not check where subsequent script is processed in case of Animation.ready.  I thought it's processed between WillRefresh() and DispatchAnimationEvents() as well as animation events case, but actually it's NOT!

Script following 'yield animation.ready' is processed after nsRefreshDriver::Tick() is finished, the place is that UI events callbacks are also processed (IIUC).

I don't know this is the behavior what the conformant Promise handling supposed.

Comment 33

[Hiroyuki Ikezoe (:hiro)]

As for the Animation.ready behavior, waitForNextFrame() does not help unfortunately.

Comment 34

[Brian Birtles (:birtles)]

This behavior is currently completely unspecified. I need to write some spec text in Web Animations that covers:

- ticking animations
  (and subsequently resolving animation promises)
- dispatching AnimationPlayback events
- dispatching CSS transition events
- dispatching CSS animation events

then link to it from HTML.

It's probably worth checking what Chrome currently does.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Thanks!  It's more intuitive.  Also bug 135450 noticed me that 'finish' and 'cancel' events are processed after tick, so it's the same behavior as Animation.ready for now, IIUC.  Anyway as for this particular bug, I am going to put flushComputedStyle() where we need it.  The test cases that need flushComputedStyle() for Animation.ready can be moved in test_animation_observers_sync.html in future since AFAIK there is nothing that needs to do asynchronously. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=764b619a57d39d4b775e03c687bd931a6ed5089b

Comment 36

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> Thanks!  It's more intuitive.  Also bug 135450 noticed me that 'finish' and
> 'cancel' events are processed after tick, so it's the same behavior as

bug 1354501.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926222 [details]
Bug 1415399 - Use arrow function in test_animation_observers_{async,sync}.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197478/diff/2-3/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197482/diff/4-5/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> Thanks!  It's more intuitive.  Also bug 135450 noticed me that 'finish' and
> 'cancel' events are processed after tick, so it's the same behavior as
> Animation.ready for now, IIUC.  Anyway as for this particular bug, I am
> going to put flushComputedStyle() where we need it.  The test cases that
> need flushComputedStyle() for Animation.ready can be moved in
> test_animation_observers_sync.html in future since AFAIK there is nothing
> that needs to do asynchronously. 

Filed bug 1416693 for moving such test cases.

Comment 40

[Brian Birtles (:birtles)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

https://reviewboard.mozilla.org/r/197482/#review204290

::: dom/animation/test/chrome/test_animation_observers_async.html:1030
(Diff revision 5)
>      assert_records([{ added: [], changed: animations, removed: [] }],
>                      "records after pause()");
>  
>      // Cancel the animation.
>      e.style = "";
> +    // Explicitly flush style as the above test.

Nit: 'as with' (or 'as in')

::: dom/animation/test/chrome/test_animation_observers_async.html:1072
(Diff revision 5)
>                      { added: [], changed: animations, removed: [] }],
>                      "records after aborting a pause()");
>  
>      // Cancel the animation.
>      e.style = "";
> +    // Explicitly flush style as the above test.

Nit: 'as with' (or 'as in')

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926222 [details]
Bug 1415399 - Use arrow function in test_animation_observers_{async,sync}.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197478/diff/3-4/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8926224 [details]
Bug 1415399 - Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197482/diff/5-6/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #34)
> This behavior is currently completely unspecified. I need to write some spec
> text in Web Animations that covers:
> 
> - ticking animations
>   (and subsequently resolving animation promises)
> - dispatching AnimationPlayback events
> - dispatching CSS transition events
> - dispatching CSS animation events
> 
> then link to it from HTML.
> 
> It's probably worth checking what Chrome currently does.

I tried to check it with a script but it did not go well since they don't implement timeline.currentTime yet.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example to check animation events and Animation.ready promise handling

FWIW, this is the file what I used for checking the chrome behavior.  It's actually hard to read for now. :/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1cc8eaa72fc
Use arrow function in test_animation_observers_{async,sync}.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6098597ee637
Explicitly flush styles to make sure style changes happen in the case where we are in the state just before requestAnimationFrame is handled. r=birtles

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #43)
> (In reply to Brian Birtles (:birtles) from comment #34)
> > This behavior is currently completely unspecified. I need to write some spec
> > text in Web Animations that covers:
> > 
> > - ticking animations
> >   (and subsequently resolving animation promises)
> > - dispatching AnimationPlayback events
> > - dispatching CSS transition events
> > - dispatching CSS animation events
> > 
> > then link to it from HTML.
> > 
> > It's probably worth checking what Chrome currently does.
> 
> I tried to check it with a script but it did not go well since they don't
> implement timeline.currentTime yet.

Chrome does not implement Animation.ready either..

Comment 47

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> Chrome does not implement Animation.ready either..

Even on canary with experimental web features enabled?

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #47)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> > Chrome does not implement Animation.ready either..
> 
> Even on canary with experimental web features enabled?

Thanks! I totally forget about the flag.  I used to do it before, but my shell history on new laptop did not include it unfortunately.

For my own record, that is " --enable-blink-features=WebAnimationsAPI".

Anyway, they have a micro task check point for Animation.ready before handling CSS animation events handling.  So it seems to match #comment 34.

Comment 49

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d1cc8eaa72fc
https://hg.mozilla.org/mozilla-central/rev/6098597ee637