[Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline

• Component: Animations
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

Created attachment 390020 [details]
Testcase

[Simon Fraser (smfr)]

This is web-detectable (see test case).

[Simon Fraser (smfr)]

Issue about this: https://github.com/w3c/csswg-drafts/issues/4553

[Simon Fraser (smfr)]

Created attachment 390051 [details]
WIP patch

[Antoine Quint]

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

<rdar://problem/59370413>

[Antoine Quint]

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]

Renaming this Radar to make it clear what work it entails.

[Antoine Quint]

Created attachment 390752 [details]
WIP patch

[Antoine Quint]

Created attachment 390760 [details]
Patch

[Antoine Quint]

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 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]

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

Committed r256619: <https://trac.webkit.org/changeset/256619>

[Alexey Proskuryakov]

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.