Closed
Bug 1416966
Opened 7 years ago
Closed 7 years ago
Perform micro task check point when Animation.ready or Animation.finished is fulfilled
Categories
(Core :: DOM: Animation, enhancement, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
()
Details
Attachments
(9 files)
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
bevis
:
review+
|
Details |
(deleted),
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
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
Comment 4•7 years ago
|
||
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.)
Comment 5•7 years ago
|
||
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)
?
Comment 6•7 years ago
|
||
(Not to mention fixing bug 1354501 too.)
Assignee | ||
Comment 7•7 years ago
|
||
(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
Comment 8•7 years ago
|
||
For what it's worth, this is the test file I've been using to compare Firefox and Chrome's behavior.
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 13•7 years ago
|
||
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.
Assignee | ||
Comment 14•7 years ago
|
||
Bevis told me on IRC that we need to still use Promise::PerformMicroTaskCheckpoint() before bug 1193394 and he will replace it in bug 1193394.
Assignee | ||
Comment 15•7 years ago
|
||
A new try, it might be new failures in other test cases.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85a3c2b63664eed5f7c3cbcf8bc282a919a6b060
Assignee | ||
Comment 16•7 years ago
|
||
(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
Assignee | ||
Comment 17•7 years ago
|
||
It seems to be better to make those test cases synchronous rather than make these tests pass in this intermediate situations.
Depends on: 1416693
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
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.
Assignee | ||
Comment 20•7 years ago
|
||
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 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 29•7 years ago
|
||
mozreview-review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment 30•7 years ago
|
||
mozreview-review |
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 31•7 years ago
|
||
mozreview-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 32•7 years ago
|
||
mozreview-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 33•7 years ago
|
||
mozreview-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 34•7 years ago
|
||
mozreview-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 35•7 years ago
|
||
mozreview-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 36•7 years ago
|
||
mozreview-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 37•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 38•7 years ago
|
||
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 47•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P3
Comment 48•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4ae4883c829e
https://hg.mozilla.org/mozilla-central/rev/75e481ca63e6
https://hg.mozilla.org/mozilla-central/rev/7b37c0b27f0b
https://hg.mozilla.org/mozilla-central/rev/0f834f7fbe90
https://hg.mozilla.org/mozilla-central/rev/3e1a082b6ff0
https://hg.mozilla.org/mozilla-central/rev/f9a466c592f2
https://hg.mozilla.org/mozilla-central/rev/39850d2b42f6
https://hg.mozilla.org/mozilla-central/rev/7ad629927e45
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•