[Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Feb 2020 17:52:07 +0000 (17:52 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Feb 2020 17:52:07 +0000 (17:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207364
<rdar://problem/59370413>

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

There are some progressions but also some "regressions". The progressions are real, showing the delivery of all animation events at the correct
time. However, the regressions are misleading. The fact that the "style change" tests would work was due to a design issue in the test which would
only wait one frame to detect whether a CSS Transition was started after a change made through the Web Animations API. These would work because
events were queued in the next frame, but delivered later due to the dedicated per-animation queue used, which meant the test was fooled into
thinking the CSS Transition did not start, as expected. Changing those test to use more than one frame to test for the lack of a CSS Transition
would have shown the FAIL results.

However, in order to not regress our WPT score, the issue of "style change" events will be addressed in a follow-up patch.

* web-platform-tests/css/css-transitions/CSSTransition-startTime.tentative-expected.txt:
* web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/style-change-events-expected.txt:
* web-platform-tests/web-animations/interfaces/KeyframeEffect/style-change-events-expected.txt:
* web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-expected.txt:

Source/WebCore:

Until now, AnimationPlaybackEvent events, which are new events introduced by the Web Animations spec, were enqueued in a shared queue on the DocumentTimeline
and dispatched during the "update animations and send events" procedure. However, AnimationEvent and TransitionEvent events, dispatched by CSS Animations
and CSS Transitions, were dispatched via a dedicated per-animation queue, which meant typically that those events were dispathed one runloop after the
AnimationPlaybackEvent events.

We now remove the dedicated per-animation queue and enqueue all events in the shared DocumentTimeline queue for dispatch during the "update animations and send
events" procedure. To do this correctly, we need to do a couple of other things that ensure we don't regress tests.

First, we update the DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() to account for whether there are pending animation events,
guaranteeing that an animation update is scheduled should there be any.

Second, when animation events are enqueued in DocumentTimeline::enqueueAnimationEvent() we schedule an animation update if needed, since we know we now
have pending events that will need to be delivered in an upcoming update. We also maintain a flag between the start of the "update animations and send events"
procedure and the moment when the pending animation events queue is cleared prior to dispatching events so that events enqueued in the meantime do not
prematurely schedule animation resolution. The need for a new animation resolution will be checked at the end of the procedure.

Finally, declarative animations used to have a special suclass of WebAnimation::needsTick() that would check whether they had any pending events, ensuring
they would not be removed prematurely. We now reset a flag to false as WebAnimation::tick() is called (as part of the "update animations and send events"
procedure) and set it to true in case an animation is enqueued. This flag is then used in needsTick() to guarantee the animation is not removed before
the DocumentTimeline has had a chance to dispatch the enqueued event.

Note also that, for clarity, the DocumentTimeline::unscheduleAnimationResolution() was renamed to DocumentTimeline::clearTickScheduleTimer() since it wouldn't
actually cancel a previous animation resolution schedule.

* animation/CSSTransition.h: Fix a newly found build error due to the missing wtf/MonotonicTime.h header.
* animation/DeclarativeAnimation.cpp: Remove all code related to the dedicated per-animation queue and instead call the new WebAnimation::enqueueAnimationEvent()
method to enqueue events on the DocumentTimeline.
(WebCore::DeclarativeAnimation::DeclarativeAnimation):
(WebCore::DeclarativeAnimation::tick):
(WebCore::DeclarativeAnimation::enqueueDOMEvent):
* animation/DeclarativeAnimation.h:
* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::detachFromDocument): Ensure the pending events queue is cleared when the timeline is detached from a document, ensuring that there no
longer events that would cause a ref-cycle (DocumentTimeline -> AnimationPlaybackEvent -> WebAnimation -> DocumentTimeline).
(WebCore::DocumentTimeline::suspendAnimations):
(WebCore::DocumentTimeline::removeAnimation):
(WebCore::DocumentTimeline::scheduleAnimationResolution):
(WebCore::DocumentTimeline::clearTickScheduleTimer):
(WebCore::DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState const):
(WebCore::DocumentTimeline::updateCurrentTime):
(WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
(WebCore::DocumentTimeline::internalUpdateAnimationsAndSendEvents):
(WebCore::DocumentTimeline::scheduleNextTick):
(WebCore::DocumentTimeline::animationAcceleratedRunningStateDidChange):
(WebCore::DocumentTimeline::enqueueAnimationEvent):
* animation/DocumentTimeline.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
(WebCore::WebAnimation::enqueueAnimationEvent):
(WebCore::WebAnimation::needsTick const):
(WebCore::WebAnimation::tick):
* animation/WebAnimation.h:

LayoutTests:

Fix a couple of tests that made some incorrect assumptions.

* TestExpectations: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is no longer flaky.
* compositing/backing/animate-into-view.html: Because the "animationstart" event is now dispatched during the "update animations and send events" procedure, which happens
during page rendering _before_ rAF callbacks are serviced, we must remove the rAF callback used prior to adding the "animationstart" event listener or else we would never
get it and the test would time out.
* webanimations/css-transition-in-flight-reversal-accelerated.html: We must wait for the initial transition to start and then two frames before reversing the transition,
to be certain that the animation did start. Indeed, the "transitionstart" event will be fired right before the next rAF callback is called, as the animation starts in that
very same frame, and so progress will be 0 and the transition wouldn't be reversable until the next frame when the animation has progress > 0.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@256619 268f45cc-cd09-0410-ab3c-d52691b4dbfc

18 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/compositing/backing/animate-into-view.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/CSSTransition-startTime.tentative-expected.txt
LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt
LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/style-change-events-expected.txt
LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/style-change-events-expected.txt
LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-expected.txt
LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated.html
Source/WebCore/ChangeLog
Source/WebCore/animation/CSSTransition.h
Source/WebCore/animation/DeclarativeAnimation.cpp
Source/WebCore/animation/DeclarativeAnimation.h
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/DocumentTimeline.h
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h

index 470faa1..7d9e3c1 100644 (file)
@@ -1,3 +1,21 @@
+2020-02-14  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline
+        https://bugs.webkit.org/show_bug.cgi?id=207364
+        <rdar://problem/59370413>
+
+        Reviewed by Simon Fraser.
+
+        Fix a couple of tests that made some incorrect assumptions.
+
+        * TestExpectations: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html is no longer flaky.
+        * compositing/backing/animate-into-view.html: Because the "animationstart" event is now dispatched during the "update animations and send events" procedure, which happens
+        during page rendering _before_ rAF callbacks are serviced, we must remove the rAF callback used prior to adding the "animationstart" event listener or else we would never
+        get it and the test would time out.
+        * webanimations/css-transition-in-flight-reversal-accelerated.html: We must wait for the initial transition to start and then two frames before reversing the transition,
+        to be certain that the animation did start. Indeed, the "transitionstart" event will be fired right before the next rAF callback is called, as the animation starts in that
+        very same frame, and so progress will be 0 and the transition wouldn't be reversable until the next frame when the animation has progress > 0.
+
 2020-02-14  Jacob Uphoff  <jacob_uphoff@apple.com>
 
         N[ iOS ] imported/w3c/IndexedDB-private-browsing/idbcursor_advance_index3.html is flaky timing out
index 53b5492..e1393bb 100644 (file)
@@ -2656,7 +2656,6 @@ webkit.org/b/179069 imported/w3c/web-platform-tests/html/semantics/embedded-cont
 
 webkit.org/b/202107 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/style-change-events.html [ Pass Failure ]
 webkit.org/b/202108 imported/w3c/web-platform-tests/web-animations/interfaces/DocumentTimeline/style-change-events.html [ Pass Failure ]
-webkit.org/b/202109 imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html [ Pass Failure ]
 
 webkit.org/b/157068 [ Debug ] imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html [ Pass Crash ]
 webkit.org/b/157068 [ Release ] imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html [ Pass Failure ]
index 2b669a1..c8adfc8 100644 (file)
         }
 
         window.addEventListener('load', () => {
-            requestAnimationFrame(() => {
-                let animator = document.getElementById('target');
-                animator.addEventListener('animationstart', () => {
-                    requestAnimationFrame(() => {
-                        dumpLayers();
-                        if (window.testRunner)
-                            testRunner.notifyDone();
-                    });
+            let animator = document.getElementById('target');
+            animator.addEventListener('animationstart', () => {
+                requestAnimationFrame(() => {
+                    dumpLayers();
+                    if (window.testRunner)
+                        testRunner.notifyDone();
                 });
-                animator.classList.add('animating');
             });
+            animator.classList.add('animating');
         }, false);
     </script>
 </head>
index 9530db1..fa3f081 100644 (file)
@@ -1,3 +1,26 @@
+2020-02-14  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline
+        https://bugs.webkit.org/show_bug.cgi?id=207364
+        <rdar://problem/59370413>
+
+        Reviewed by Simon Fraser.
+
+        There are some progressions but also some "regressions". The progressions are real, showing the delivery of all animation events at the correct
+        time. However, the regressions are misleading. The fact that the "style change" tests would work was due to a design issue in the test which would
+        only wait one frame to detect whether a CSS Transition was started after a change made through the Web Animations API. These would work because
+        events were queued in the next frame, but delivered later due to the dedicated per-animation queue used, which meant the test was fooled into
+        thinking the CSS Transition did not start, as expected. Changing those test to use more than one frame to test for the lack of a CSS Transition
+        would have shown the FAIL results.
+
+        However, in order to not regress our WPT score, the issue of "style change" events will be addressed in a follow-up patch.
+
+        * web-platform-tests/css/css-transitions/CSSTransition-startTime.tentative-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/style-change-events-expected.txt:
+        * web-platform-tests/web-animations/interfaces/KeyframeEffect/style-change-events-expected.txt:
+        * web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-expected.txt:
+
 2020-02-12  Rossana Monteriso  <rmonteriso@igalia.com>
 
         [css-grid] Move grid-item-auto-margins-alignment tests to WPT folder
index fadea6f..ab9c2ae 100644 (file)
@@ -3,5 +3,5 @@ PASS The start time of a newly-created transition is unresolved
 PASS The start time of transitions is based on when they are generated 
 PASS The start time of a transition can be set 
 PASS The start time can be set to seek a transition 
-FAIL Seeking a transition using start time dispatches transition events promise_test: Unhandled rejection with value: object "Error: Timed out waiting for Promise to resolve: transitionstart"
+PASS Seeking a transition using start time dispatches transition events 
 
index 7514426..16ec853 100644 (file)
@@ -132,7 +132,7 @@ PASS Element.animate() correctly sets the id attribute
 PASS Element.animate() correctly sets the Animation's timeline 
 PASS Element.animate() correctly sets the Animation's timeline when triggered on an element in a different document 
 PASS Element.animate() calls play on the Animation 
-PASS Element.animate() does NOT trigger a style change event 
+FAIL Element.animate() does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
 PASS animate() with pseudoElement parameter creates an Animation object 
 PASS animate() with pseudoElement parameter without content creates an Animation object 
 PASS animate() with pseudoElement parameter  creates an Animation object for ::marker 
index 6eea874..8ce8356 100644 (file)
@@ -20,7 +20,7 @@ FAIL Animation.play produces expected style change events assert_false: A transi
 FAIL Animation.pause produces expected style change events assert_false: A transition should NOT have been triggered expected false got true
 FAIL Animation.updatePlaybackRate produces expected style change events assert_false: A transition should NOT have been triggered expected false got true
 FAIL Animation.reverse produces expected style change events assert_false: A transition should NOT have been triggered expected false got true
-PASS Animation.persist produces expected style change events 
+FAIL Animation.persist produces expected style change events assert_false: A transition should NOT have been triggered expected false got true
 FAIL Animation.commitStyles produces expected style change events assert_true: A transition should have been triggered expected true got false
 FAIL Animation.Animation constructor produces expected style change events assert_false: A transition should NOT have been triggered expected false got true
 
index d309a47..3935548 100644 (file)
@@ -1,13 +1,13 @@
 
 FAIL All property keys are recognized assert_in_array: Test property 'pseudoElement' should be one of the properties on  KeyframeEffect value "pseudoElement" not in array ["getTiming", "getComputedTiming", "updateTiming", "target", "iterationComposite", "composite", "getKeyframes", "setKeyframes", "KeyframeEffect constructor", "KeyframeEffect copy constructor"]
-PASS KeyframeEffect.getTiming does NOT trigger a style change event 
-PASS KeyframeEffect.getComputedTiming does NOT trigger a style change event 
-PASS KeyframeEffect.updateTiming does NOT trigger a style change event 
-PASS KeyframeEffect.target does NOT trigger a style change event 
-PASS KeyframeEffect.iterationComposite does NOT trigger a style change event 
-PASS KeyframeEffect.composite does NOT trigger a style change event 
-PASS KeyframeEffect.getKeyframes does NOT trigger a style change event 
-PASS KeyframeEffect.setKeyframes does NOT trigger a style change event 
-PASS KeyframeEffect.KeyframeEffect constructor does NOT trigger a style change event 
-PASS KeyframeEffect.KeyframeEffect copy constructor does NOT trigger a style change event 
+FAIL KeyframeEffect.getTiming does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.getComputedTiming does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.updateTiming does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.target does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.iterationComposite does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.composite does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.getKeyframes does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.setKeyframes does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.KeyframeEffect constructor does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
+FAIL KeyframeEffect.KeyframeEffect copy constructor does NOT trigger a style change event assert_false: A transition should NOT have been triggered expected false got true
 
index 1a6ae3f..44e0668 100644 (file)
@@ -2,9 +2,9 @@
 PASS Fires cancel event before requestAnimationFrame 
 PASS Fires finish event before requestAnimationFrame 
 FAIL Sorts finish events by composite order assert_array_equals: finish events for various animation type should be sorted by composite order property 0, expected "CSSTransition:finish" but got "ScriptAnimation:finish"
-FAIL Sorts cancel events by composite order assert_array_equals: cancel events should be sorted by composite order lengths differ, expected 5 got 3
-FAIL Queues a cancel event in transitionstart event callback assert_approx_equals: A rAF callback should happen in the same frame expected 112 +/- 0.001 but got 96
-FAIL Sorts events for the same transition assert_array_equals: Playback and CSS events for the same transition should be sorted by schedule event time and composite order lengths differ, expected 2 got 1
+FAIL Sorts cancel events by composite order assert_array_equals: cancel events should be sorted by composite order property 0, expected "CSSTransition:cancel" but got "ScriptAnimation:cancel"
+PASS Queues a cancel event in transitionstart event callback 
+PASS Sorts events for the same transition 
 PASS Playback events with the same timeline retain the order in which they arequeued 
 FAIL All timelines are updated before running microtasks promise_test: Unhandled rejection with value: object "AbortError: The operation was aborted."
 
index 6da5bfa..30783a8 100644 (file)
@@ -46,19 +46,23 @@ function targetTest(propertyName)
             initialTransition = animations[0];
             assert_true(initialTransition instanceof CSSTransition, "There is one animation applied to the target after starting the initial transition.");
 
-            // Wait for the initial transition to start and then another frame before reversing the transition.
+            // Wait for the initial transition to start and then two frames before reversing the transition, to be certain that the animation did start.
+            // Indeed, the "transitionstart" event will be fired right before the next rAF callback is called, as the animation starts in that very same
+            // frame, and so progress will be 0 and the transition wouldn't be reversable until the next frame when the animation has progress > 0.
             target.addEventListener("transitionstart", event => {
                 requestAnimationFrame(() => {
-                    target.classList.remove("in-flight");
-                    const animations = target.getAnimations();
-                    assert_equals(animations.length, 1, "There is one animation applied to the target after reversing the initial transition.");
+                    requestAnimationFrame(() => {
+                        target.classList.remove("in-flight");
+                        const animations = target.getAnimations();
+                        assert_equals(animations.length, 1, "There is one animation applied to the target after reversing the initial transition.");
 
-                    reversedTransition = animations[0];
-                    assert_true(reversedTransition instanceof CSSTransition, "There is one animation applied to the target after reversing the initial transition.");
-                    assert_not_equals(initialTransition, reversedTransition, "The animation applied to the target after reversing the initial transition is different than the original transition.");
+                        reversedTransition = animations[0];
+                        assert_true(reversedTransition instanceof CSSTransition, "There is one animation applied to the target after reversing the initial transition.");
+                        assert_not_equals(initialTransition, reversedTransition, "The animation applied to the target after reversing the initial transition is different than the original transition.");
 
-                    target.remove();
-                    test.done();
+                        target.remove();
+                        test.done();
+                    });
                 });
             });
         });
index 9934ebf..1a0bb1b 100644 (file)
@@ -1,5 +1,122 @@
 2020-02-14  Antoine Quint  <graouts@webkit.org>
 
+        [Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline
+        https://bugs.webkit.org/show_bug.cgi?id=207364
+        <rdar://problem/59370413>
+
+        Reviewed by Simon Fraser.
+
+        Until now, AnimationPlaybackEvent events, which are new events introduced by the Web Animations spec, were enqueued in a shared queue on the DocumentTimeline
+        and dispatched during the "update animations and send events" procedure. However, AnimationEvent and TransitionEvent events, dispatched by CSS Animations
+        and CSS Transitions, were dispatched via a dedicated per-animation queue, which meant typically that those events were dispathed one runloop after the
+        AnimationPlaybackEvent events.
+
+        We now remove the dedicated per-animation queue and enqueue all events in the shared DocumentTimeline queue for dispatch during the "update animations and send
+        events" procedure. To do this correctly, we need to do a couple of other things that ensure we don't regress tests.
+
+        First, we update the DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() to account for whether there are pending animation events,
+        guaranteeing that an animation update is scheduled should there be any.
+
+        Second, when animation events are enqueued in DocumentTimeline::enqueueAnimationEvent() we schedule an animation update if needed, since we know we now
+        have pending events that will need to be delivered in an upcoming update. We also maintain a flag between the start of the "update animations and send events"
+        procedure and the moment when the pending animation events queue is cleared prior to dispatching events so that events enqueued in the meantime do not
+        prematurely schedule animation resolution. The need for a new animation resolution will be checked at the end of the procedure.
+
+        Finally, declarative animations used to have a special suclass of WebAnimation::needsTick() that would check whether they had any pending events, ensuring
+        they would not be removed prematurely. We now reset a flag to false as WebAnimation::tick() is called (as part of the "update animations and send events"
+        procedure) and set it to true in case an animation is enqueued. This flag is then used in needsTick() to guarantee the animation is not removed before
+        the DocumentTimeline has had a chance to dispatch the enqueued event.
+
+        Note also that, for clarity, the DocumentTimeline::unscheduleAnimationResolution() was renamed to DocumentTimeline::clearTickScheduleTimer() since it wouldn't
+        actually cancel a previous animation resolution schedule.
+
+        * animation/CSSTransition.h: Fix a newly found build error due to the missing wtf/MonotonicTime.h header.
+        * animation/DeclarativeAnimation.cpp: Remove all code related to the dedicated per-animation queue and instead call the new WebAnimation::enqueueAnimationEvent()
+        method to enqueue events on the DocumentTimeline.
+        (WebCore::DeclarativeAnimation::DeclarativeAnimation):
+        (WebCore::DeclarativeAnimation::tick):
+        (WebCore::DeclarativeAnimation::enqueueDOMEvent):
+        * animation/DeclarativeAnimation.h:
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::detachFromDocument): Ensure the pending events queue is cleared when the timeline is detached from a document, ensuring that there no
+        longer events that would cause a ref-cycle (DocumentTimeline -> AnimationPlaybackEvent -> WebAnimation -> DocumentTimeline).
+        (WebCore::DocumentTimeline::suspendAnimations):
+        (WebCore::DocumentTimeline::removeAnimation):
+        (WebCore::DocumentTimeline::scheduleAnimationResolution):
+        (WebCore::DocumentTimeline::clearTickScheduleTimer):
+        (WebCore::DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState const):
+        (WebCore::DocumentTimeline::updateCurrentTime):
+        (WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
+        (WebCore::DocumentTimeline::internalUpdateAnimationsAndSendEvents):
+        (WebCore::DocumentTimeline::scheduleNextTick):
+        (WebCore::DocumentTimeline::animationAcceleratedRunningStateDidChange):
+        (WebCore::DocumentTimeline::enqueueAnimationEvent):
+        * animation/DocumentTimeline.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
+        (WebCore::WebAnimation::enqueueAnimationEvent):
+        (WebCore::WebAnimation::needsTick const):
+        (WebCore::WebAnimation::tick):
+        * animation/WebAnimation.h:
+
+2020-02-14  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] Make all animation event types inherit from the same base class
+        https://bugs.webkit.org/show_bug.cgi?id=207629
+
+        Reviewed by Simon Fraser.
+
+        Currently we dispatch events CSS Transitions and CSS Animations events using a dedicated event queue on DeclarativeAnimation, while the events
+        added by the Web Animations specification (of type AnimationPlaybackEvent) are dispatched using a shared queue on the DocumentTimeline that is
+        processed during the "update animations and send events procedure". The Web Animations specification dictates that all events should be dispatched
+        during that procedure, which includes sorting of such events based on their timeline time and associated animation relative composite order.
+
+        In this patch, we prepare the work towards spec compliance for animation events dispatch by making all event types (AnimationPlaybackEvent,
+        TransitionEvent and AnimationEvent) inherit from a single AnimationEventBase interface. This will allow DocumentTimeline to enqueue, sort and
+        dispatch all such events with a single queue in a future patch.
+
+        Due to CSSAnimationController, we must make the "timeline time" and "animation" parameters optional. When we drop support for CSSAnimationController
+        we'll be able to enforce stronger requirements for these.
+
+        No new test since this should not introduce any behavior change.
+
+        * Sources.txt:
+        * WebCore.xcodeproj/project.pbxproj:
+        * animation/AnimationEventBase.cpp: Added.
+        (WebCore::AnimationEventBase::AnimationEventBase):
+        * animation/AnimationEventBase.h: Added.
+        (WebCore::AnimationEventBase::create):
+        (WebCore::AnimationEventBase::isAnimationPlaybackEvent const):
+        (WebCore::AnimationEventBase::isAnimationEvent const):
+        (WebCore::AnimationEventBase::isTransitionEvent const):
+        (WebCore::AnimationEventBase::timelineTime const):
+        (WebCore::AnimationEventBase::animation const):
+        * animation/AnimationPlaybackEvent.cpp:
+        (WebCore::AnimationPlaybackEvent::AnimationPlaybackEvent):
+        (WebCore::AnimationPlaybackEvent::bindingsTimelineTime const):
+        * animation/AnimationPlaybackEvent.h:
+        * animation/CSSAnimation.cpp:
+        (WebCore::CSSAnimation::createEvent):
+        * animation/CSSAnimation.h:
+        * animation/CSSTransition.cpp:
+        (WebCore::CSSTransition::createEvent):
+        * animation/CSSTransition.h:
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::enqueueDOMEvent):
+        * animation/DeclarativeAnimation.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
+        * dom/AnimationEvent.cpp:
+        (WebCore::AnimationEvent::AnimationEvent):
+        * dom/AnimationEvent.h:
+        * dom/TransitionEvent.cpp:
+        (WebCore::TransitionEvent::TransitionEvent):
+        * dom/TransitionEvent.h:
+        * page/animation/CSSAnimationController.cpp:
+        (WebCore::CSSAnimationControllerPrivate::fireEventsAndUpdateStyle):
+
+2020-02-14  Antoine Quint  <graouts@webkit.org>
+
         [Web Animations] Make all animation event types inherit from the same base class
         https://bugs.webkit.org/show_bug.cgi?id=207629
 
index 23317ed..d18285c 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "CSSPropertyNames.h"
 #include "DeclarativeAnimation.h"
+#include <wtf/MonotonicTime.h>
 #include <wtf/Ref.h>
 
 namespace WebCore {
index e31d2f7..626b80e 100644 (file)
@@ -42,7 +42,6 @@ WTF_MAKE_ISO_ALLOCATED_IMPL(DeclarativeAnimation);
 
 DeclarativeAnimation::DeclarativeAnimation(Element& owningElement, const Animation& backingAnimation)
     : WebAnimation(owningElement.document())
-    , m_eventQueue(MainThreadGenericEventQueue::create(owningElement))
     , m_owningElement(&owningElement)
     , m_backingAnimation(const_cast<Animation&>(backingAnimation))
 {
@@ -63,10 +62,8 @@ void DeclarativeAnimation::tick()
     // canceled using the Web Animations API and it should be disassociated from its owner element.
     // From this point on, this animation is like any other animation and should not appear in the
     // maps containing running CSS Transitions and CSS Animations for a given element.
-    if (wasRelevant && playState() == WebAnimation::PlayState::Idle) {
+    if (wasRelevant && playState() == WebAnimation::PlayState::Idle)
         disassociateFromOwningElement();
-        m_eventQueue->close();
-    }
 }
 
 bool DeclarativeAnimation::canHaveGlobalPosition()
@@ -90,17 +87,6 @@ void DeclarativeAnimation::disassociateFromOwningElement()
     m_owningElement = nullptr;
 }
 
-bool DeclarativeAnimation::needsTick() const
-{
-    return WebAnimation::needsTick() || m_eventQueue->hasPendingEvents();
-}
-
-void DeclarativeAnimation::remove()
-{
-    m_eventQueue->close();
-    WebAnimation::remove();
-}
-
 void DeclarativeAnimation::setBackingAnimation(const Animation& backingAnimation)
 {
     m_backingAnimation = const_cast<Animation&>(backingAnimation);
@@ -351,7 +337,9 @@ void DeclarativeAnimation::enqueueDOMEvent(const AtomString& eventType, Seconds
     auto time = secondsToWebAnimationsAPITime(elapsedTime) / 1000;
     const auto& pseudoId = PseudoElement::pseudoElementNameForEvents(m_owningElement->pseudoId());
     auto timelineTime = timeline() ? timeline()->currentTime() : WTF::nullopt;
-    m_eventQueue->enqueueEvent(createEvent(eventType, time, pseudoId, timelineTime));
+    auto event = createEvent(eventType, time, pseudoId, timelineTime);
+    event->setTarget(m_owningElement);
+    enqueueAnimationEvent(WTFMove(event));
 }
 
 } // namespace WebCore
index 27e52d9..03e84a3 100644 (file)
@@ -27,7 +27,6 @@
 
 #include "AnimationEffect.h"
 #include "AnimationEffectPhase.h"
-#include "GenericEventQueue.h"
 #include "WebAnimation.h"
 #include <wtf/Ref.h>
 
@@ -65,7 +64,6 @@ public:
     void setTimeline(RefPtr<AnimationTimeline>&&) final;
     void cancel() final;
 
-    bool needsTick() const override;
     void tick() override;
 
     bool canHaveGlobalPosition() final;
@@ -85,13 +83,10 @@ private:
     void flushPendingStyleChanges() const;
     AnimationEffectPhase phaseWithoutEffect() const;
     void enqueueDOMEvent(const AtomString&, Seconds);
-    void remove() final;
 
     bool m_wasPending { false };
     AnimationEffectPhase m_previousPhase { AnimationEffectPhase::Idle };
 
-    UniqueRef<MainThreadGenericEventQueue> m_eventQueue;
-
     Element* m_owningElement;
     Ref<Animation> m_backingAnimation;
     double m_previousIteration;
index bf82db4..8f36a4e 100644 (file)
@@ -26,7 +26,7 @@
 #include "config.h"
 #include "DocumentTimeline.h"
 
-#include "AnimationPlaybackEvent.h"
+#include "AnimationEventBase.h"
 #include "CSSAnimation.h"
 #include "CSSTransition.h"
 #include "DOMWindow.h"
@@ -88,6 +88,7 @@ void DocumentTimeline::detachFromDocument()
     if (m_document)
         m_document->removeTimeline(*this);
 
+    m_pendingAnimationEvents.clear();
     m_currentTimeClearingTaskQueue.close();
     m_elementsWithRunningAcceleratedAnimations.clear();
 
@@ -95,7 +96,7 @@ void DocumentTimeline::detachFromDocument()
     while (!animationsToRemove.isEmpty())
         animationsToRemove.first()->remove();
 
-    unscheduleAnimationResolution();
+    clearTickScheduleTimer();
     m_document = nullptr;
 }
 
@@ -234,7 +235,7 @@ void DocumentTimeline::suspendAnimations()
 
     applyPendingAcceleratedAnimations();
 
-    unscheduleAnimationResolution();
+    clearTickScheduleTimer();
 }
 
 void DocumentTimeline::resumeAnimations()
@@ -327,31 +328,31 @@ void DocumentTimeline::removeAnimation(WebAnimation& animation)
 {
     AnimationTimeline::removeAnimation(animation);
 
-    if (!shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
-        unscheduleAnimationResolution();
+    if (m_animations.isEmpty())
+        clearTickScheduleTimer();
 }
 
 void DocumentTimeline::scheduleAnimationResolution()
 {
-    if (m_isSuspended || m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
+    if (m_isSuspended || m_animationResolutionScheduled || !m_document || !m_document->page())
         return;
 
-    if (!m_document || !m_document->page())
+    // We need some relevant animations or pending events to proceed.
+    if (!shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
-    
+
     m_document->page()->renderingUpdateScheduler().scheduleTimedRenderingUpdate();
     m_animationResolutionScheduled = true;
 }
 
-void DocumentTimeline::unscheduleAnimationResolution()
+void DocumentTimeline::clearTickScheduleTimer()
 {
     m_tickScheduleTimer.stop();
-    m_animationResolutionScheduled = false;
 }
 
 bool DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() const
 {
-    return !m_animations.isEmpty() || !m_acceleratedAnimationsPendingRunningStateChange.isEmpty();
+    return !m_animations.isEmpty() || !m_pendingAnimationEvents.isEmpty() || !m_acceleratedAnimationsPendingRunningStateChange.isEmpty();
 }
 
 void DocumentTimeline::updateCurrentTime(DOMHighResTimeStamp timestamp)
@@ -359,19 +360,23 @@ void DocumentTimeline::updateCurrentTime(DOMHighResTimeStamp timestamp)
     // We need to freeze the current time even if no animation is running.
     // document.timeline.currentTime may be called from a rAF callback and
     // it has to match the rAF timestamp.
-    if (!m_isSuspended)
+    if (!m_isSuspended || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         cacheCurrentTime(timestamp);
 }
 
 void DocumentTimeline::updateAnimationsAndSendEvents()
 {
-    if (m_isSuspended || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
-        return;
 
     // Updating animations and sending events may invalidate the timing of some animations, so we must set the m_animationResolutionScheduled
     // flag to false prior to running that procedure to allow animation with timing model updates to schedule updates.
     m_animationResolutionScheduled = false;
 
+    if (m_isSuspended)
+        return;
+
+    if (!shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
+        return;
+
     internalUpdateAnimationsAndSendEvents();
     applyPendingAcceleratedAnimations();
 
@@ -383,6 +388,11 @@ void DocumentTimeline::internalUpdateAnimationsAndSendEvents()
 {
     m_numberOfAnimationTimelineInvalidationsForTesting++;
 
+    // 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;
+
     // https://drafts.csswg.org/web-animations/#update-animations-and-send-events
 
     // 1. Update the current time of all timelines associated with doc passing now as the timestamp.
@@ -421,9 +431,10 @@ void DocumentTimeline::internalUpdateAnimationsAndSendEvents()
     // 4. Let events to dispatch be a copy of doc's pending animation event queue.
     // 5. Clear doc's pending animation event queue.
     auto pendingAnimationEvents = WTFMove(m_pendingAnimationEvents);
+    m_shouldScheduleAnimationResolutionForNewPendingEvents = true;
 
     // 6. Perform a stable sort of the animation events in events to dispatch as follows.
-    std::stable_sort(pendingAnimationEvents.begin(), pendingAnimationEvents.end(), [] (const Ref<AnimationPlaybackEvent>& lhs, const Ref<AnimationPlaybackEvent>& rhs) {
+    std::stable_sort(pendingAnimationEvents.begin(), pendingAnimationEvents.end(), [] (const Ref<AnimationEventBase>& lhs, const Ref<AnimationEventBase>& rhs) {
         // 1. Sort the events by their scheduled event time such that events that were scheduled to occur earlier, sort before events scheduled to occur later
         // and events whose scheduled event time is unresolved sort before events with a resolved scheduled event time.
         // 2. Within events with equal scheduled event times, sort by their composite order. FIXME: We don't do this.
@@ -437,8 +448,8 @@ void DocumentTimeline::internalUpdateAnimationsAndSendEvents()
     });
 
     // 7. Dispatch each of the events in events to dispatch at their corresponding target using the order established in the previous step.
-    for (auto& pendingEvent : pendingAnimationEvents)
-        pendingEvent->target()->dispatchEvent(pendingEvent);
+    for (auto& pendingAnimationEvent : pendingAnimationEvents)
+        pendingAnimationEvent->target()->dispatchEvent(pendingAnimationEvent);
 
     // This will cancel any scheduled invalidation if we end up removing all animations.
     for (auto& animation : animationsToRemove) {
@@ -547,6 +558,10 @@ void DocumentTimeline::transitionDidComplete(RefPtr<CSSTransition> transition)
 
 void DocumentTimeline::scheduleNextTick()
 {
+    // If we have pending animation events, we need to schedule an update right away.
+    if (!m_pendingAnimationEvents.isEmpty())
+        scheduleAnimationResolution();
+
     // There is no tick to schedule if we don't have any relevant animations.
     if (m_animations.isEmpty())
         return;
@@ -665,7 +680,7 @@ void DocumentTimeline::animationAcceleratedRunningStateDidChange(WebAnimation& a
     if (shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         scheduleAnimationResolution();
     else
-        unscheduleAnimationResolution();
+        clearTickScheduleTimer();
 }
 
 void DocumentTimeline::updateListOfElementsWithRunningAcceleratedAnimationsForElement(Element& element)
@@ -708,9 +723,11 @@ bool DocumentTimeline::runningAnimationsForElementAreAllAccelerated(Element& ele
     return m_elementsWithRunningAcceleratedAnimations.contains(&element);
 }
 
-void DocumentTimeline::enqueueAnimationPlaybackEvent(AnimationPlaybackEvent& event)
+void DocumentTimeline::enqueueAnimationEvent(AnimationEventBase& event)
 {
     m_pendingAnimationEvents.append(event);
+    if (m_shouldScheduleAnimationResolutionForNewPendingEvents)
+        scheduleAnimationResolution();
 }
 
 Vector<std::pair<String, double>> DocumentTimeline::acceleratedAnimationsForElement(Element& element) const
index 1fcb806..592a9d5 100644 (file)
@@ -35,7 +35,7 @@
 
 namespace WebCore {
 
-class AnimationPlaybackEvent;
+class AnimationEventBase;
 class RenderElement;
 
 class DocumentTimeline final : public AnimationTimeline, public CanMakeWeakPtr<DocumentTimeline>
@@ -70,7 +70,7 @@ public:
     bool runningAnimationsForElementAreAllAccelerated(Element&) const;
     void detachFromDocument();
 
-    void enqueueAnimationPlaybackEvent(AnimationPlaybackEvent&);
+    void enqueueAnimationEvent(AnimationEventBase&);
     
     bool scheduledUpdate() const { return m_animationResolutionScheduled; }
     void updateCurrentTime(DOMHighResTimeStamp timestamp);
@@ -92,11 +92,9 @@ private:
     void cacheCurrentTime(DOMHighResTimeStamp);
     void maybeClearCachedCurrentTime();
     void scheduleInvalidationTaskIfNeeded();
-    void performInvalidationTask();
     void scheduleAnimationResolution();
-    void unscheduleAnimationResolution();
+    void clearTickScheduleTimer();
     void internalUpdateAnimationsAndSendEvents();
-    void performEventDispatchTask();
     void updateListOfElementsWithRunningAcceleratedAnimationsForElement(Element&);
     void transitionDidComplete(RefPtr<CSSTransition>);
     void scheduleNextTick();
@@ -108,7 +106,7 @@ private:
     GenericTaskQueue<Timer> m_currentTimeClearingTaskQueue;
     HashSet<RefPtr<WebAnimation>> m_acceleratedAnimationsPendingRunningStateChange;
     HashSet<Element*> m_elementsWithRunningAcceleratedAnimations;
-    Vector<Ref<AnimationPlaybackEvent>> m_pendingAnimationEvents;
+    Vector<Ref<AnimationEventBase>> m_pendingAnimationEvents;
     RefPtr<Document> m_document;
     Markable<Seconds, Seconds::MarkableTraits> m_cachedCurrentTime;
     Seconds m_originTime;
@@ -116,6 +114,7 @@ private:
     bool m_isSuspended { false };
     bool m_waitingOnVMIdle { false };
     bool m_animationResolutionScheduled { false };
+    bool m_shouldScheduleAnimationResolutionForNewPendingEvents { true };
 };
 
 } // namespace WebCore
index d0896fb..45e247f 100644 (file)
@@ -685,13 +685,18 @@ void WebAnimation::enqueueAnimationPlaybackEvent(const AtomString& type, Optiona
 {
     auto event = AnimationPlaybackEvent::create(type, currentTime, timelineTime, this);
     event->setTarget(this);
+    enqueueAnimationEvent(WTFMove(event));
+}
 
+void WebAnimation::enqueueAnimationEvent(Ref<AnimationEventBase>&& event)
+{
     if (is<DocumentTimeline>(m_timeline)) {
         // If animation has a document for timing, then append event to its document for timing's pending animation event queue along
         // with its target, animation. If animation is associated with an active timeline that defines a procedure to convert timeline times
         // to origin-relative time, let the scheduled event time be the result of applying that procedure to timeline time. Otherwise, the
         // scheduled event time is an unresolved time value.
-        downcast<DocumentTimeline>(*m_timeline).enqueueAnimationPlaybackEvent(WTFMove(event));
+        m_hasScheduledEventsDuringTick = true;
+        downcast<DocumentTimeline>(*m_timeline).enqueueAnimationEvent(WTFMove(event));
     } else {
         // Otherwise, queue a task to dispatch event at animation. The task source for this task is the DOM manipulation task source.
         queueTaskToDispatchEvent(*this, TaskSource::DOMManipulation, WTFMove(event));
@@ -1198,11 +1203,12 @@ bool WebAnimation::isCompletelyAccelerated() const
 
 bool WebAnimation::needsTick() const
 {
-    return pending() || playState() == PlayState::Running;
+    return pending() || playState() == PlayState::Running || m_hasScheduledEventsDuringTick;
 }
 
 void WebAnimation::tick()
 {
+    m_hasScheduledEventsDuringTick = false;
     updateFinishedState(DidSeek::No, SynchronouslyNotify::Yes);
     m_shouldSkipUpdatingFinishedStateWhenResolving = true;
 
index 3032f28..b02ef9f 100644 (file)
@@ -42,7 +42,7 @@
 namespace WebCore {
 
 class AnimationEffect;
-class AnimationPlaybackEvent;
+class AnimationEventBase;
 class AnimationTimeline;
 class Document;
 class Element;
@@ -116,7 +116,7 @@ public:
     virtual ExceptionOr<void> bindingsPlay() { return play(); }
     virtual ExceptionOr<void> bindingsPause() { return pause(); }
 
-    virtual bool needsTick() const;
+    bool needsTick() const;
     virtual void tick();
     Seconds timeToNextTick() const;
     virtual void resolve(RenderStyle&);
@@ -135,7 +135,7 @@ public:
     void setSuspended(bool);
     bool isSuspended() const { return m_isSuspended; }
     bool isReplaceable() const;
-    virtual void remove();
+    void remove();
     void enqueueAnimationPlaybackEvent(const AtomString&, Optional<Seconds>, Optional<Seconds>);
 
     uint64_t globalPosition() const { return m_globalPosition; }
@@ -155,6 +155,8 @@ public:
 protected:
     explicit WebAnimation(Document&);
 
+    void enqueueAnimationEvent(Ref<AnimationEventBase>&&);
+
 private:
     enum class DidSeek : uint8_t { Yes, No };
     enum class SynchronouslyNotify : uint8_t { Yes, No };
@@ -201,6 +203,7 @@ private:
     bool m_finishNotificationStepsMicrotaskPending;
     bool m_isRelevant;
     bool m_shouldSkipUpdatingFinishedStateWhenResolving;
+    bool m_hasScheduledEventsDuringTick { false };
     TimeToRunPendingTask m_timeToRunPendingPlayTask { TimeToRunPendingTask::NotScheduled };
     TimeToRunPendingTask m_timeToRunPendingPauseTask { TimeToRunPendingTask::NotScheduled };
     ReplaceState m_replaceState { ReplaceState::Active };