[Web Animations] Retargeted transitions targeting accelerated properties do not stop...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Nov 2019 14:25:21 +0000 (14:25 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Nov 2019 14:25:21 +0000 (14:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204116
<rdar://problem/57116961>

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/css-transition-in-flight-reversal-accelerated.html

There were two problems with the reversal of in-flight transitions targeting accelerated properties. The first issue
was that the animated value for the accelerated property would be missing from the before-change style since animating
acceelerated properties do not update RenderStyle while running. As such, we would not detect the need to reverse a
transition, but rather simply cancel the initial transition with no new transition to reverse its effect, since the
value in the before-change and after-change styles were the same. We now fix this by figuring out whether the running
transition for that property is accelerated in AnimationTimeline::updateCSSTransitionsForElementAndProperty() and
applying the animated value to the before-change style.

The second issue was that canceling an accelerated transition had no visible effect since the accelerated animation
was not stopped. We now have a new AnimationEffect::animationWasCanceled() virtual method which KeyframeEffect overrides
to add AcceleratedAction::Stop to the list of pending acceleration actions for this effect. We also ensure that the document
timeline knows to run DocumentTimeline::updateAnimationsAndSendEvents() if there are pending accelerated actions, even if
there aren't any animations waiting to be resolved, since a single canceled transition would prevent this method from
completing.

* animation/AnimationEffect.h:
* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::removeAnimation):
(WebCore::DocumentTimeline::scheduleAnimationResolution):
(WebCore::DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState const):
(WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
(WebCore::DocumentTimeline::updateListOfElementsWithRunningAcceleratedAnimationsForElement):
* animation/DocumentTimeline.h:
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::animationWasCanceled):
* animation/KeyframeEffect.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::cancel):

LayoutTests:

Add a new test that checks that reversing an in-flight transition for "opacity" and "transform" correctly reverses the transition.

* platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt: Added.
* webanimations/css-transition-in-flight-reversal-accelerated-expected.txt: Added.
* webanimations/css-transition-in-flight-reversal-accelerated.html: Added.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationEffect.h
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/DocumentTimeline.h
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/KeyframeEffect.h
Source/WebCore/animation/WebAnimation.cpp

index d777195..a66024f 100644 (file)
@@ -1,3 +1,17 @@
+2019-11-14  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Retargeted transitions targeting accelerated properties do not stop the original transition
+        https://bugs.webkit.org/show_bug.cgi?id=204116
+        <rdar://problem/57116961>
+
+        Reviewed by Dean Jackson.
+
+        Add a new test that checks that reversing an in-flight transition for "opacity" and "transform" correctly reverses the transition.
+
+        * platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt: Added.
+        * webanimations/css-transition-in-flight-reversal-accelerated-expected.txt: Added.
+        * webanimations/css-transition-in-flight-reversal-accelerated.html: Added.
+
 2019-11-13  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [Clipboard API] Add support for Clipboard.write()
diff --git a/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt b/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt
new file mode 100644 (file)
index 0000000..67c33e0
--- /dev/null
@@ -0,0 +1,13 @@
+
+Harness Error (TIMEOUT), message = null
+
+PASS Can dispatch untrusted 'click' Events at disabled HTML elements. 
+PASS Can dispatch untrusted Events at disabled HTML elements. 
+PASS Can dispatch CustomEvents at disabled HTML elements. 
+PASS Calling click() on disabled elements must not dispatch events. 
+PASS CSS Transitions transitionrun, transitionstart, transitionend events fire on disabled form elements 
+TIMEOUT CSS Transitions transitioncancel event fires on disabled form elements Test timed out
+NOTRUN CSS Animation animationstart, animationiteration, animationend fire on disabled form elements 
+NOTRUN CSS Animation's animationcancel event fires on disabled form elements 
+NOTRUN Real clicks on disabled elements must not dispatch events. 
+
diff --git a/LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated-expected.txt b/LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated-expected.txt
new file mode 100644 (file)
index 0000000..cb4f160
--- /dev/null
@@ -0,0 +1,4 @@
+
+PASS A CSS transition targeting opacity can be reversed in-flight. 
+PASS A CSS transition targeting transform can be reversed in-flight. 
+
diff --git a/LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated.html b/LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated.html
new file mode 100644 (file)
index 0000000..6da5bfa
--- /dev/null
@@ -0,0 +1,72 @@
+<!DOCTYPE html><!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] -->
+<meta charset="utf-8">
+<title>Reversal of accelerated in-flight CSS Transitions</title>
+<style type="text/css" media="screen">
+
+.target.opacity {
+    transition: opacity 2s linear;
+}
+
+.target.opacity.in-flight {
+    opacity: 0;
+}
+
+.target.transform {
+    transition: transform 2s linear;
+}
+
+.target.transform.in-flight {
+    transform: scale(0);
+}
+
+</style>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<script>
+
+'use strict';
+
+function targetTest(propertyName)
+{
+    async_test(test => {
+        const target = document.body.appendChild(document.createElement("div"));
+        target.classList.add("target");
+        target.classList.add(propertyName);
+
+        let initialTransition;
+        let reversedTransition;
+
+        // Start the initial transition.
+        requestAnimationFrame(() => {
+            target.classList.add("in-flight");
+            const animations = target.getAnimations();
+            assert_equals(animations.length, 1, "There is one animation applied to the target after starting the initial transition.");
+
+            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.
+            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.");
+
+                    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();
+                });
+            });
+        });
+    }, `A CSS transition targeting ${propertyName} can be reversed in-flight.`);
+}
+
+targetTest("opacity");
+targetTest("transform");
+
+</script>
+</body>
\ No newline at end of file
index a53651f..c9707f8 100644 (file)
@@ -1,3 +1,44 @@
+2019-11-14  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Retargeted transitions targeting accelerated properties do not stop the original transition
+        https://bugs.webkit.org/show_bug.cgi?id=204116
+        <rdar://problem/57116961>
+
+        Reviewed by Dean Jackson.
+
+        Test: webanimations/css-transition-in-flight-reversal-accelerated.html
+
+        There were two problems with the reversal of in-flight transitions targeting accelerated properties. The first issue
+        was that the animated value for the accelerated property would be missing from the before-change style since animating
+        acceelerated properties do not update RenderStyle while running. As such, we would not detect the need to reverse a
+        transition, but rather simply cancel the initial transition with no new transition to reverse its effect, since the
+        value in the before-change and after-change styles were the same. We now fix this by figuring out whether the running
+        transition for that property is accelerated in AnimationTimeline::updateCSSTransitionsForElementAndProperty() and
+        applying the animated value to the before-change style.
+
+        The second issue was that canceling an accelerated transition had no visible effect since the accelerated animation
+        was not stopped. We now have a new AnimationEffect::animationWasCanceled() virtual method which KeyframeEffect overrides
+        to add AcceleratedAction::Stop to the list of pending acceleration actions for this effect. We also ensure that the document
+        timeline knows to run DocumentTimeline::updateAnimationsAndSendEvents() if there are pending accelerated actions, even if
+        there aren't any animations waiting to be resolved, since a single canceled transition would prevent this method from
+        completing.
+
+        * animation/AnimationEffect.h:
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::removeAnimation):
+        (WebCore::DocumentTimeline::scheduleAnimationResolution):
+        (WebCore::DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState const):
+        (WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
+        (WebCore::DocumentTimeline::updateListOfElementsWithRunningAcceleratedAnimationsForElement):
+        * animation/DocumentTimeline.h:
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::animationWasCanceled):
+        * animation/KeyframeEffect.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::cancel):
+
 2019-11-13  Nikolas Zimmermann  <nzimmermann@igalia.com>
 
         Fix GTK/WPE builds after enabling POINTER_EVENTS support
index e1eced4..8ebb44c 100644 (file)
@@ -61,6 +61,7 @@ public:
     virtual void apply(RenderStyle&) = 0;
     virtual void invalidate() = 0;
     virtual void animationDidSeek() = 0;
+    virtual void animationWasCanceled() = 0;
     virtual void animationSuspensionStateDidChange(bool) = 0;
     virtual void animationTimelineDidChange(AnimationTimeline*) = 0;
 
index c4dea3c..3a92a93 100644 (file)
@@ -399,10 +399,29 @@ void AnimationTimeline::updateCSSTransitionsForElementAndProperty(Element& eleme
     // https://drafts.csswg.org/css-transitions-1/#before-change-style
     // Define the before-change style as the computed values of all properties on the element as of the previous style change event, except with
     // any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time.
-    auto existingAnimation = cssAnimationForElementAndProperty(element, property);
-    const auto& beforeChangeStyle = existingAnimation ? downcast<CSSAnimation>(existingAnimation.get())->unanimatedStyle() : currentStyle;
+    bool hasRunningTransition = runningTransitionsByProperty.contains(property);
+    auto& beforeChangeStyle = [&]() -> const RenderStyle& {
+        if (hasRunningTransition && CSSPropertyAnimation::animationOfPropertyIsAccelerated(property)) {
+            // In case we have an accelerated transition running for this element, we need to get its computed style as the before-change style
+            // since otherwise the animated value for that property won't be visible.
+            auto* runningTransition = runningTransitionsByProperty.get(property);
+            if (is<KeyframeEffect>(runningTransition->effect())) {
+                auto& keyframeEffect = *downcast<KeyframeEffect>(runningTransition->effect());
+                if (keyframeEffect.isAccelerated()) {
+                    auto animatedStyle = RenderStyle::clonePtr(currentStyle);
+                    runningTransition->resolve(*animatedStyle);
+                    return *animatedStyle;
+                }
+            }
+        }
 
-    if (!runningTransitionsByProperty.contains(property)
+        if (auto existingAnimation = cssAnimationForElementAndProperty(element, property))
+            return downcast<CSSAnimation>(existingAnimation.get())->unanimatedStyle();
+
+        return currentStyle;
+    }();
+
+    if (!hasRunningTransition
         && !CSSPropertyAnimation::propertiesEqual(property, &beforeChangeStyle, &afterChangeStyle)
         && CSSPropertyAnimation::canPropertyBeInterpolated(property, &beforeChangeStyle, &afterChangeStyle)
         && !propertyInStyleMatchesValueForTransitionInMap(property, afterChangeStyle, completedTransitionsByProperty)
@@ -435,7 +454,7 @@ void AnimationTimeline::updateCSSTransitionsForElementAndProperty(Element& eleme
         completedTransitionsByProperty.remove(property);
     }
 
-    bool hasRunningTransition = runningTransitionsByProperty.contains(property);
+    hasRunningTransition = runningTransitionsByProperty.contains(property);
     if ((hasRunningTransition || completedTransitionsByProperty.contains(property)) && !matchingBackingAnimation) {
         // 3. If the element has a running transition or completed transition for the property, and there is not a matching transition-property
         //    value, then implementations must cancel the running transition or remove the completed transition from the set of completed transitions.
index b0c481e..7a78358 100644 (file)
@@ -307,13 +307,13 @@ void DocumentTimeline::removeAnimation(WebAnimation& animation)
 {
     AnimationTimeline::removeAnimation(animation);
 
-    if (m_animations.isEmpty())
+    if (!shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         unscheduleAnimationResolution();
 }
 
 void DocumentTimeline::scheduleAnimationResolution()
 {
-    if (m_isSuspended || m_animations.isEmpty() || m_animationResolutionScheduled)
+    if (m_isSuspended || m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
 
     if (!m_document || !m_document->page())
@@ -329,6 +329,11 @@ void DocumentTimeline::unscheduleAnimationResolution()
     m_animationResolutionScheduled = false;
 }
 
+bool DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() const
+{
+    return !m_animations.isEmpty() || !m_acceleratedAnimationsPendingRunningStateChange.isEmpty();
+}
+
 void DocumentTimeline::updateAnimationsAndSendEvents(DOMHighResTimeStamp timestamp)
 {
     // We need to freeze the current time even if no animation is running.
@@ -337,7 +342,7 @@ void DocumentTimeline::updateAnimationsAndSendEvents(DOMHighResTimeStamp timesta
     if (!m_isSuspended)
         cacheCurrentTime(timestamp);
 
-    if (m_isSuspended || m_animations.isEmpty() || !m_animationResolutionScheduled)
+    if (m_isSuspended || !m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
 
     internalUpdateAnimationsAndSendEvents();
@@ -654,6 +659,11 @@ void DocumentTimeline::updateListOfElementsWithRunningAcceleratedAnimationsForEl
     }
 
     m_elementsWithRunningAcceleratedAnimations.add(&element);
+
+    if (shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
+        scheduleAnimationResolution();
+    else
+        unscheduleAnimationResolution();
 }
 
 void DocumentTimeline::applyPendingAcceleratedAnimations()
index 7c481f2..2c35d24 100644 (file)
@@ -99,6 +99,7 @@ private:
     void scheduleNextTick();
     void removeReplacedAnimations();
     bool animationCanBeRemoved(WebAnimation&);
+    bool shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() const;
 
     Timer m_tickScheduleTimer;
     GenericTaskQueue<Timer> m_currentTimeClearingTaskQueue;
index fd89aed..ebd328a 100644 (file)
@@ -1340,6 +1340,12 @@ void KeyframeEffect::animationDidSeek()
         addPendingAcceleratedAction(AcceleratedAction::Seek);
 }
 
+void KeyframeEffect::animationWasCanceled()
+{
+    if (m_shouldRunAccelerated && isRunningAccelerated())
+        addPendingAcceleratedAction(AcceleratedAction::Stop);
+}
+
 void KeyframeEffect::animationSuspensionStateDidChange(bool animationIsSuspended)
 {
     if (m_shouldRunAccelerated)
index 3cd4879..444d23f 100644 (file)
@@ -113,6 +113,7 @@ public:
     void apply(RenderStyle&) override;
     void invalidate() override;
     void animationDidSeek() final;
+    void animationWasCanceled() final;
     void animationSuspensionStateDidChange(bool) final;
     void animationTimelineDidChange(AnimationTimeline*) final;
     void applyPendingAcceleratedActions();
index b91184e..b3f746d 100644 (file)
@@ -614,6 +614,9 @@ void WebAnimation::cancel(Silently silently)
     timingDidChange(DidSeek::No, SynchronouslyNotify::No);
 
     invalidateEffect();
+
+    if (m_effect)
+        m_effect->animationWasCanceled();
 }
 
 void WebAnimation::enqueueAnimationPlaybackEvent(const AtomString& type, Optional<Seconds> currentTime, Optional<Seconds> timelineTime)