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+

Description Simon Fraser (smfr) 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?
Comment 1 Simon Fraser (smfr) 2020-02-06 16:09:58 PST
Created attachment 390020 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2020-02-06 16:10:09 PST
This is web-detectable (see test case).
Comment 3 Simon Fraser (smfr) 2020-02-06 16:23:36 PST
Issue about this: https://github.com/w3c/csswg-drafts/issues/4553
Comment 4 Simon Fraser (smfr) 2020-02-06 20:11:24 PST
Created attachment 390051 [details]
WIP patch
Comment 5 Antoine Quint 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.
Comment 6 Antoine Quint 2020-02-12 06:52:58 PST
<rdar://problem/59370413>
Comment 7 Antoine Quint 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.
Comment 8 Antoine Quint 2020-02-12 07:26:47 PST
Renaming this Radar to make it clear what work it entails.
Comment 9 Antoine Quint 2020-02-14 03:34:43 PST
Created attachment 390752 [details]
WIP patch
Comment 10 Antoine Quint 2020-02-14 07:00:41 PST
Created attachment 390760 [details]
Patch
Comment 11 Antoine Quint 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.
Comment 12 Simon Fraser (smfr) 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.
Comment 13 Antoine Quint 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.
Comment 14 Antoine Quint 2020-02-14 09:52:18 PST
Committed r256619: <https://trac.webkit.org/changeset/256619>
Comment 15 Alexey Proskuryakov 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.