Bug 207364

Summary: [Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, dbates, dino, dstockwell, esprehn+autocc, ews-watchlist, graouts, graouts, kangil.han, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=207361
https://bugs.webkit.org/show_bug.cgi?id=207629
Bug Depends on: 207629    
Bug Blocks: 207760    
Attachments:
Description Flags
Testcase
none
WIP patch
none
WIP patch
none
Patch simon.fraser: review+

Simon Fraser (smfr)
Reported 2020-02-06 15:41:51 PST
transitionstart/transitionend events are dispatched via enqueueDOMEvent() which adds them to the DeclarativeAnimation's m_eventQueue, which is a MainThreadGenericEventQueue which dispatches the events via a zero-delay timer. Conversely, DocumentTimeline::internalUpdateAnimationsAndSendEvents() explicitly dispatches the Web Animations related events (like "finish") immediately, as would be expected from https://drafts.csswg.org/web-animations/#update-animations-and-send-events Is this difference deliberate?
Attachments
Testcase (1.99 KB, text/html)
2020-02-06 16:09 PST, Simon Fraser (smfr)
no flags
WIP patch (8.47 KB, patch)
2020-02-06 20:11 PST, Simon Fraser (smfr)
no flags
WIP patch (61.81 KB, patch)
2020-02-14 03:34 PST, Antoine Quint
no flags
Patch (41.38 KB, patch)
2020-02-14 07:00 PST, Antoine Quint
simon.fraser: review+
Simon Fraser (smfr)
Comment 1 2020-02-06 16:09:58 PST
Created attachment 390020 [details] Testcase
Simon Fraser (smfr)
Comment 2 2020-02-06 16:10:09 PST
This is web-detectable (see test case).
Simon Fraser (smfr)
Comment 3 2020-02-06 16:23:36 PST
Simon Fraser (smfr)
Comment 4 2020-02-06 20:11:24 PST
Created attachment 390051 [details] WIP patch
Antoine Quint
Comment 5 2020-02-07 07:16:39 PST
(In reply to Simon Fraser (smfr) from comment #0) > transitionstart/transitionend events are dispatched via enqueueDOMEvent() > which adds them to the DeclarativeAnimation's m_eventQueue, which is a > MainThreadGenericEventQueue which dispatches the events via a zero-delay > timer. > > Conversely, DocumentTimeline::internalUpdateAnimationsAndSendEvents() > explicitly dispatches the Web Animations related events (like "finish") > immediately, as would be expected from > https://drafts.csswg.org/web-animations/#update-animations-and-send-events > > Is this difference deliberate? No, it's a bug. We should dispatch all events in DocumentTimeline::internalUpdateAnimationsAndSendEvents(). It's been on my to-do list but didn't seem like a pressing issue.
Antoine Quint
Comment 6 2020-02-12 06:52:58 PST
Antoine Quint
Comment 7 2020-02-12 07:25:18 PST
Bug 207629 has a patch up for some required refactoring for all animation event types to share a single interface. This will allow them all to be queued, sorted and dispatched by DocumentTimeline.
Antoine Quint
Comment 8 2020-02-12 07:26:47 PST
Renaming this Radar to make it clear what work it entails.
Antoine Quint
Comment 9 2020-02-14 03:34:43 PST
Created attachment 390752 [details] WIP patch
Antoine Quint
Comment 10 2020-02-14 07:00:41 PST
Antoine Quint
Comment 11 2020-02-14 07:07:19 PST
Note that the patch for review is _not_ expected to build since it depends on the one for https://bugs.webkit.org/show_bug.cgi?id=207629. We can retry it once it lands, but the "WIP Patch" contained the dependency as well and showed positive bot results.
Simon Fraser (smfr)
Comment 12 2020-02-14 08:39:04 PST
Comment on attachment 390760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390760&action=review > Source/WebCore/animation/DocumentTimeline.cpp:394 > + // enqueueAnimationEvent() calls scheduleAnimationResolution() to ensure that the "update animations and send events" > + // procedure is run and enqueued events are dispatched in the next frame. However, events that are enqueued while > + // this procedure is running should not schedule animation resolution until the event queue has been cleared. > + m_shouldScheduleAnimationResolutionForNewPendingEvents = false; This is a bit confusing because I'd expect scheduleAnimationResolution() to queue up something that is going to happen later, which makes me wonder why calling it in this block is bad. What's the timing difference between calling scheduleAnimationResolution() here, and calling it later via scheduleNextTick()? > Source/WebCore/animation/DocumentTimeline.cpp:728 > + String eventType = event.type().string(); eventType is unused > Source/WebCore/animation/WebAnimation.cpp:1206 > + return pending() || playState() == PlayState::Running || m_hasScheduledEventsDuringTick; Why does the WebAnimation have to stick around until the event is dispatched? Does the event need to reference back to the WebAnimation? Or do we need to be able to unschedule the event sometimes? > LayoutTests/imported/w3c/ChangeLog:16 > + However, in order to not regress our WPT score, the issue of "style change" events will be addressed in a follow-up patch. We should file GitHub issues on tests we think aren't cross-browser compatible.
Antoine Quint
Comment 13 2020-02-14 09:48:51 PST
(In reply to Simon Fraser (smfr) from comment #12) > Comment on attachment 390760 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390760&action=review > > > Source/WebCore/animation/DocumentTimeline.cpp:394 > > + // enqueueAnimationEvent() calls scheduleAnimationResolution() to ensure that the "update animations and send events" > > + // procedure is run and enqueued events are dispatched in the next frame. However, events that are enqueued while > > + // this procedure is running should not schedule animation resolution until the event queue has been cleared. > > + m_shouldScheduleAnimationResolutionForNewPendingEvents = false; > > This is a bit confusing because I'd expect scheduleAnimationResolution() to > queue up something that is going to happen later, which makes me wonder why > calling it in this block is bad. What's the timing difference between > calling scheduleAnimationResolution() here, and calling it later via > scheduleNextTick()? Here's what happens: - enqueueAnimationEvent() schedules an animation right away when an event is added (except when the flag is set. This is because events are dispatched during the "update animations and send events" procedure. - but events can be enqueued while the "update animations and send events" procedure is running, for instance when WebAnimation::tick() is called. - if that happens prior to when events are dispatched, we don't want to for animation resolution to be scheduled because that means we'll have a rendering update that is not necessary. If more events are added _after_ events have been dispatched, or rather after the event queue has been cleared prior to dispatch, then animation resolution will be scheduled for the next frame. > > Source/WebCore/animation/DocumentTimeline.cpp:728 > > + String eventType = event.type().string(); > > eventType is unused Nice catch. Leftovers from some earlier debugging code. Wonder why the compiler didn't catch that! > > Source/WebCore/animation/WebAnimation.cpp:1206 > > + return pending() || playState() == PlayState::Running || m_hasScheduledEventsDuringTick; > > Why does the WebAnimation have to stick around until the event is > dispatched? Does the event need to reference back to the WebAnimation? Or do > we need to be able to unschedule the event sometimes? The events don't need to be unscheduled. I think once they're queued, they're expected to be dispatched. I'm not certain that they actually need to stick around for event dispatch, it might actually be because we want them to be around for one more frame after the animation has reverted back to its idle state or gone info its finished state (for fill-forwards animations) once they're no longer active. This would happen automatically before this change because the events were queued and dispatched asynchronously, and the Animation needed to remain alive for that. We might want to change the terminology sometimes going forwards, but I'd rather keep it that way for now to avoid regressions. > > LayoutTests/imported/w3c/ChangeLog:16 > > + However, in order to not regress our WPT score, the issue of "style change" events will be addressed in a follow-up patch. > > We should file GitHub issues on tests we think aren't cross-browser > compatible. Yes, I'll make a PR to change the tests in question so that they would have caught that issue before this change.
Antoine Quint
Comment 14 2020-02-14 09:52:18 PST
Alexey Proskuryakov
Comment 15 2020-02-14 22:29:12 PST
Comment on attachment 390760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390760&action=review > LayoutTests/ChangeLog:11 > + * TestExpectations: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is no longer flaky. It is still flaky or failing on most queues, see bug 207790.
Note You need to log in before you can comment on or make changes to this bug.