-
Notifications
You must be signed in to change notification settings - Fork 666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web-animations-1] Ordering of promises and events, and where events are fired #5582
Comments
Right, I believe that's because in that case we'll run the promise callback microtask before we run the rendering part of the event loop again.
The resolving happens synchronously, but the callbacks for the promise run as a separate microtask as per the resolve steps.
The animation spec only queues tasks to dispatch events when there is no document for timing (e.g. no timeline, or timeline not associated with a document). In all other cases it uses the pending animation event queue that is processed in the rendering part of the event loop. (As for why it creates a separate task to dispatch events when there is no document for timing, it's because in the past we've had a lot of security bugs arise from running event dispatch synchronously. Since the finish notification steps can be run synchronously, we'd have to be very careful not to assume anything about the state of the world after running them if they also did synchronous event dispatch. In the update animations and send events procedure we ensure the event dispatch is the very last thing to happen for that reason.)
To ensure that, I think we'd need to somehow resolve the finished promises in the "update animations and send events" procedure. That sounds a little tricky to me and I wonder if it would be Web-compatible, but if someone wants to try, please do! |
Ah, I see. I saw the task queuing and thought the algorithm was running off the main thread, and in turn I misinterpreted everything else. Sorry!
The animation spec is already having to hack around the ordering by creating a microtask checkpoint in step 3. This is why I think the order is incorrect, as every spec that wants to follow this ordering will have to add a microtask checkpoint before event dispatch. Spec wise, it would involve adding the promise, value, and resolve/reject along with the event info in the event queue. I don't know if this would break existing code (the finished promise is relatively new in Chrome), but it'd be sad if web animation becomes an outlier in terms of event & promise ordering. |
Ah, I see. I don't recall exactly the thinking there except I believe I thought Promise callbacks should happen before events, but it sounds like it should be the other way around.
Yes, that sounds right.
I'm all for trying it. It would be nice if Chrome could go first with trying to ship it. |
There seems to be some confusion around the ordering of promises and events in the spec.
https://github.com/web-platform-tests/wpt/blob/master/web-animations/interfaces/Animation/onfinish.html#L30 - the first test assumes the promise callbacks happen before the equivalent event (other tests may too).
https://drafts.csswg.org/web-animations-1/#updating-the-finished-state - the promise is 'resolved' before a task is queued to dispatch the event, which could be read as "promise resolves first", but the timing is unclear as no task is queued to perform the resolving.
The timing is important here, since it's expected to synchronise with rendering.
The animation spec probably shouldn't queue tasks to dispatch events at all. Since they're related to rendering, they should be part of animation events, which has a particular step in the rendering part of the event loop (11.10). This would also clarify the ordering of CSS animations/transitions vs web animations.
The event and promise should happen at the 'same time' from the developer's perspective. Since event dispatch is synchronous, and promise callbacks are asynchronous, this means all event listeners should be called before any 'same' promise callbacks. I've asked for some clarification on this in the promises guide w3ctag/promises-guide#71.
The text was updated successfully, but these errors were encountered: