Closed Bug 1416966 Opened 7 years ago Closed 6 years ago

Perform micro task check point when Animation.ready or Animation.finished is fulfilled

Categories

(Core :: DOM: Animation, enhancement, P3)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(
URL
)

Details

Attachments

(9 files)

Bug 1416966 - Make sure the next frame happen in the case where we are in the callback for Animation.ready Promise.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1416966 - Add todo for checking the final restyle after Animation.finished.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Test for task handling
2.62 KB, text/html
Details
Bug 1416966 - Add todo for checking one restyle after Animation.pause().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1416966 - Count a remaining animation restyle request when animating element was detached from the document.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1416966 - Add an annotation for a test case that will fail once we perform a micro task checkpoint between Animation::Tick and requestAnimationFrame callbacks.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1416966 - Test that there is a micro task checkpoint before requestAnimationFrame callbacks.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1416966 - Perform a micro task checkpoint in DocumentTimeline::WillRefresh.
59 bytes, text/x-review-board-request
birtles
: review+
bevis
: review+
Details
Bug 1416966 - Drop the check for the new micro task checkpoint for animations.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
From bug 1415399 comment 34;

(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.

I am not super familiar with micro task, so wording in this bug tile might be not accurate.
Uploaded patches work fine mostly, there is one failure in web-platform tests.  That is;

https://hg.mozilla.org/mozilla-central/file/e1d7427787f7/testing/web-platform/tests/web-animations/timing-model/animations/updating-the-finished-state.html#l180

The test case assumes that waiting a requestAnimationFrame in Animation.ready.then changes animation.currentTime, but yeah, with these patches it's still in the same tick.

Chrome has a bug for it; https://bugs.chromium.org/p/chromium/issues/detail?id=772060
I'm looking into this problem now and while I agree the tests are probably broken, I'm entirely not sure if using nsAutoMicroTask is the right thing here.

Conceptually, at least, I think we just want to run a single microtask checkpoint after ticking all the timelines. MaybeResolve/MaybeReject should already be enqueueing a microtask for each resolved/rejected promise so we just need to iterate through them after updating all Animation objects (i.e. after ticking the relevant timelines).

I need to write the spec text for this but so far I think it's going to be along the lines of:

  When asked to _update_animations_and_report_changes_ for a Document /doc/, run these steps:

  1. _Sample_ all document timelines associated with /doc/.
  2. Perform a microtask checkpoint.
     (This is to actually run any promises resolved/rejected in 1)
  3. Collect all pending animation playback events, CSS transition events, and CSS animation events.
  4. Sort the events by their relative wallclock time (e.g. relative to navigationStart)
     (This will need a rigorous definition including something for scroll timelines)
  5. For events that have equal wallclock time sort by:
     a. composite order of the corresponding animations
     b. For events coming from the same Animation object sort animation playback
        events before CSS transitions or CSS animation events.
  6. Dispatch pending events in using the order established above such that older events are dispatched
     first.

(Note that dispatching events automatically runs a microtask checkpoint each time we finish calling into script.)
Oh, and this is probably not the right bug for this, but if the above spec text makes sense, then maybe we can implement it and fix bug 1415780 at the same time by making some sort of AnimationEventRegistry that:

1. Collects all the different types of events (this bit is hard because of the weird event subclassing that happens)
2. Sorts them
3. Dispatches them
4. Registers as a refresh driver observer when it has pending events simply to keep it ticking (but actually does nothing in WillRefresh)

?
(Not to mention fixing bug 1354501 too.)
(In reply to Brian Birtles (:birtles) from comment #4)
> I'm looking into this problem now and while I agree the tests are probably
> broken, I'm entirely not sure if using nsAutoMicroTask is the right thing
> here.
> 
> Conceptually, at least, I think we just want to run a single microtask
> checkpoint after ticking all the timelines. MaybeResolve/MaybeReject should
> already be enqueueing a microtask for each resolved/rejected promise so we
> just need to iterate through them after updating all Animation objects (i.e.
> after ticking the relevant timelines).

Great!  This is what I was wondering.

> I need to write the spec text for this but so far I think it's going to be
> along the lines of:
> 
>   When asked to _update_animations_and_report_changes_ for a Document /doc/,
> run these steps:
> 
>   1. _Sample_ all document timelines associated with /doc/.
>   2. Perform a microtask checkpoint.
>      (This is to actually run any promises resolved/rejected in 1)
>   3. Collect all pending animation playback events, CSS transition events,
> and CSS animation events.
>   4. Sort the events by their relative wallclock time (e.g. relative to
> navigationStart)
>      (This will need a rigorous definition including something for scroll
> timelines)
>   5. For events that have equal wallclock time sort by:
>      a. composite order of the corresponding animations
>      b. For events coming from the same Animation object sort animation
> playback
>         events before CSS transitions or CSS animation events.
>   6. Dispatch pending events in using the order established above such that
> older events are dispatched
>      first.

Two questions;

1) Where do we do these whole steps?  After 7-7 in the spec [1] I guess?
2) Does 2 mean that "6. Microtasks: Perform a microtask checkpoint" in the spec will be moved?

[1] https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-8
Attached file Test for task handling — 
For what it's worth, this is the test file I've been using to compare Firefox and Chrome's behavior.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Two questions;
> 
> 1) Where do we do these whole steps?  After 7-7 in the spec [1] I guess?

Yeah, once I add this hook, Domenic will update the HTML spec to change:

  8. For each fully active Document in docs, run CSS animations and send events for that Document, passing in now as the timestamp. [CSSANIMATIONS]

to:

  8. For each fully active Document in docs, update animations for that Document. [WEBANIMATIONS]

> 2) Does 2 mean that "6. Microtasks: Perform a microtask checkpoint" in the
> spec will be moved?

No, that stays. There are actually lots of places where microtask checkpoints take place.

Take the following step for example:

  6. For each fully active Document in docs, run the scroll steps for that Document, passing in now as the timestamp. [CSSOMVIEW]

* "run the scroll steps" (https://drafts.csswg.org/cssom-view/#run-the-resize-steps) calls "fire an event"
* "fire an event" (https://dom.spec.whatwg.org/#concept-event-fire) calls "dispatching"
* "dispatch" (https://dom.spec.whatwg.org/#concept-event-dispatch) calls "invoke"
* "invoke" (https://dom.spec.whatwg.org/#concept-event-listener-invoke) calls "inner invoke"
* "inner invoke" (https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke) calls "Call a user object’s operation"
* "Call a user object’s operation" (https://heycam.github.io/webidl/#call-a-user-objects-operation) calls "clean up after running script"
* "clean up after running script" (https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script) calls "perform a microtask checkpoint"

So basically anything that calls into script ends up running a microtask checkpoint.
(In reply to Brian Birtles (:birtles) from comment #9)
> > 2) Does 2 mean that "6. Microtasks: Perform a microtask checkpoint" in the
> > spec will be moved?
> 
> No, that stays. There are actually lots of places where microtask
> checkpoints take place.
> 
> Take the following step for example:
> 
>   6. For each fully active Document in docs, run the scroll steps for that
> Document, passing in now as the timestamp. [CSSOMVIEW]
> 
> * "run the scroll steps"
> (https://drafts.csswg.org/cssom-view/#run-the-resize-steps) calls "fire an
> event"
> * "fire an event" (https://dom.spec.whatwg.org/#concept-event-fire) calls
> "dispatching"
> * "dispatch" (https://dom.spec.whatwg.org/#concept-event-dispatch) calls
> "invoke"
> * "invoke" (https://dom.spec.whatwg.org/#concept-event-listener-invoke)
> calls "inner invoke"
> * "inner invoke"
> (https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke) calls
> "Call a user object’s operation"
> * "Call a user object’s operation"
> (https://heycam.github.io/webidl/#call-a-user-objects-operation) calls
> "clean up after running script"
> * "clean up after running script"
> (https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-
> running-script) calls "perform a microtask checkpoint"
> 
> So basically anything that calls into script ends up running a microtask
> checkpoint.

Thanks.  That's good to know.  The uploaded patches should be revised then.
Depends on: 1418268
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7ec298c663eb3340e150e3c75ef7eb36b83937d

Here is a try including a patch to make the incoming spec change happens.  I am not yet sure using nsAutoMicroTask is a right thing to do.
Note that two failures below we can see in a Bevis' try [1] will be fixed by this bug.

 * Script animations on "display: none" element should not update styles
 * Opacity script animations on "display:none" element should not run on the compositor

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a005fb8aae235b850857ce9954c81f2f6e4ffc41&selectedJob=149021721
Hmm putting nsAutoMicroTask performs a micro task checkpoint if the patch for bug 1193394 is applied, but without the patch it does not perform the check point there.
Bevis told me on IRC that we need to still use Promise::PerformMicroTaskCheckpoint() before bug 1193394 and he will replace it in bug 1193394.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> A new try, it might be new failures in other test cases.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=85a3c2b63664eed5f7c3cbcf8bc282a919a6b060

This try noticed me that making test_animation_observers_async.html pass with this new micro task checkpoint but without the conformant Promise handling looks hard.  I will take a look it tomorrow.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85a3c2b63664eed5f7c3cbcf8bc282a919a6b060&selectedJob=150093874
It seems to be better to make those test cases synchronous rather than make these tests pass in this intermediate situations.
Depends on: 1416693
I couldn't make a failure test in test_animation_observers_async.html synchronous in bug 1416693, the test name is 'finish_from_pause'.  What I don't quite understand is the failure test passes with the new micro task checkpoint for animation (this bug) and the conformant Promise handling (bug 1193394).  I was hoping that rewriting the failure with testharness.js kicks out the failure reason somehow, but actually it's not.
After more debugging, finally, I noticed what the problem for the failure is.  I id put a PerformMicroTaskCheckpoint call after each animation tick calls [1], that's the problem.   Before the each animation tick calls, there is an nsAutoAnimationMutationBatch, so yeah, we need to call PerformMicroTaskCheckpoint after the mutation batch is destroyed.  Otherwise we will not see some mutation records there. 

Here is an update try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbac3d3cf844f33f34823bccbe6c81e2831a3ebe
Comment on attachment 8935681 [details]
Bug 1416966 - Perform a micro task checkpoint in DocumentTimeline::WillRefresh.

https://reviewboard.mozilla.org/r/206584/#review212236

LGTM, thanks!
Attachment #8935681 - Flags: review?(btseng) → review+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8928077 [details]
Bug 1416966 - Make sure the next frame happen in the case where we are in the callback for Animation.ready Promise.

https://reviewboard.mozilla.org/r/199304/#review212742
Attachment #8928077 - Flags: review?(bbirtles) → review+
Comment on attachment 8928078 [details]
Bug 1416966 - Add todo for checking the final restyle after Animation.finished.

https://reviewboard.mozilla.org/r/199306/#review212744
Attachment #8928078 - Flags: review?(bbirtles) → review+
Comment on attachment 8935677 [details]
Bug 1416966 - Add todo for checking one restyle after Animation.pause().

https://reviewboard.mozilla.org/r/206576/#review212746
Attachment #8935677 - Flags: review?(bbirtles) → review+
Comment on attachment 8935678 [details]
Bug 1416966 - Count a remaining animation restyle request when animating element was detached from the document.

https://reviewboard.mozilla.org/r/206578/#review212748

::: dom/animation/test/mozilla/file_restyles.html:886
(Diff revision 1)
> -      is(markers.length, 1,
> +      //
> +      // Note that there is another request that has been remaining when the
> +      // animation element was removed (bug 1417354).
> +      todo_is(markers.length, 2,

Bug 1417354: There will be an additional superfluous restyle request which results when we detach an element from the document between the Animation tick and styling, and then re-attach it.
Attachment #8935678 - Flags: review?(bbirtles) → review+
Comment on attachment 8935679 [details]
Bug 1416966 - Add an annotation for a test case that will fail once we perform a micro task checkpoint between Animation::Tick and requestAnimationFrame callbacks.

https://reviewboard.mozilla.org/r/206580/#review212754
Attachment #8935679 - Flags: review?(bbirtles) → review+
Comment on attachment 8935680 [details]
Bug 1416966 - Test that there is a micro task checkpoint before requestAnimationFrame callbacks.

https://reviewboard.mozilla.org/r/206582/#review212866

::: testing/web-platform/tests/web-animations/timing-model/timelines/timelines.html:82
(Diff revision 1)
> +
> +  animation.ready.then(t.step_func(() => {
> +    const readyTimelineTime = document.timeline.currentTime;
> +    requestAnimationFrame(t.step_func_done(() => {
> +      assert_equals(readyTimelineTime, document.timeline.currentTime,
> +                    'There should be a micro task checkpoint');

Nit: s/micro task/microtask/

::: testing/web-platform/tests/web-animations/timing-model/timelines/timelines.html:85
(Diff revision 1)
> +    requestAnimationFrame(t.step_func_done(() => {
> +      assert_equals(readyTimelineTime, document.timeline.currentTime,
> +                    'There should be a micro task checkpoint');
> +    }));
> +  }));
> +}, 'Performs a micro task checkpoint before requestAnimation callbacks');

nit: s/micro task/microtask/

Also, since we're testing the "update animations and send events" procedure, it might make more sense to say,

"Performs a microtask checkpoint after updating timelines"

(since that procedure doesn't specifically refer to requestAnimationFrame callbacks.)
Attachment #8935680 - Flags: review?(bbirtles) → review+
Comment on attachment 8935681 [details]
Bug 1416966 - Perform a micro task checkpoint in DocumentTimeline::WillRefresh.

https://reviewboard.mozilla.org/r/206584/#review212880

::: dom/animation/DocumentTimeline.cpp:161
(Diff revision 1)
>               "Should be able to reach refresh driver from within WillRefresh");
>  
>    bool needsTicks = false;
>    nsTArray<Animation*> animationsToRemove(mAnimations.Count());
>  
> +  // https://drafts.csswg.org/web-animations-1/#ref-for-perform-a-microtask-checkpoint

I'm not sure we should link to these bikeshed ref-* type links since they're not stable. If we introduce another reference to HTML's "perform a microtask checkpoint" definition earlier in the spec, then this will point to that instead (the subsequent link will get a ① appended onto it).

So we should probably just link to "https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events, step 2" (which, is still not completely stable, but good enough).
Attachment #8935681 - Flags: review?(bbirtles) → review+
Comment on attachment 8935682 [details]
Bug 1416966 - Drop the check for the new micro task checkpoint for animations.

https://reviewboard.mozilla.org/r/206586/#review212882

r=me with the comment updated

::: dom/animation/test/mozilla/file_restyles.html:129
(Diff revision 1)
>      // 1193394.  However, we won't observe that initial restyling unless BOTH of
>      // the following two conditions hold:
>      //
>      // 1. We are running *before* restyling happens.  This only happens if we
>      //    perform a micro task checkpoint after resolving the 'ready' promise
>      //    above (bug 1416966).
>      // 2. The animation actually needs a restyle because it started prior to
>      //    this frame.  Even if (1) is true, in some cases due to aligning with
>      //    the refresh driver, the animation fame in which the ready promise is
>      //    resolved happens to coincide perfectly with the start time of the
>      //    animation.  In this case no restyling is needed so we won't observe
>      //    an additional restyle.

I think we need to update this comment now, right?
Attachment #8935682 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ae4883c829e
Make sure the next frame happen in the case where we are in the callback for Animation.ready Promise. r=birtles
https://hg.mozilla.org/integration/autoland/rev/75e481ca63e6
Add todo for checking the final restyle after Animation.finished. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7b37c0b27f0b
Add todo for checking one restyle after Animation.pause(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/0f834f7fbe90
Count a remaining animation restyle request when animating element was detached from the document. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3e1a082b6ff0
Add an annotation for a test case that will fail once we perform a micro task checkpoint between Animation::Tick and requestAnimationFrame callbacks. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f9a466c592f2
Test that there is a micro task checkpoint before requestAnimationFrame callbacks. r=birtles
https://hg.mozilla.org/integration/autoland/rev/39850d2b42f6
Perform a micro task checkpoint in DocumentTimeline::WillRefresh. r=bevis,birtles
https://hg.mozilla.org/integration/autoland/rev/7ad629927e45
Drop the check for the new micro task checkpoint for animations. r=birtles
Depends on: 1424948
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: