Changeset 252455 in webkit

Timestamp:

Nov 14, 2019 6:25:21 AM (4 years ago)

Author:

graouts@webkit.org

Message:

[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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom (added)
• LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events (added)
• LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt (added)
• LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated-expected.txt (added)
• LayoutTests/webanimations/css-transition-in-flight-reversal-accelerated.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/AnimationEffect.h (modified) (1 diff)
• Source/WebCore/animation/AnimationTimeline.cpp (modified) (2 diffs)
• Source/WebCore/animation/DocumentTimeline.cpp (modified) (5 diffs)
• Source/WebCore/animation/DocumentTimeline.h (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.h (modified) (1 diff)
• Source/WebCore/animation/WebAnimation.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/animation/AnimationEffect.h

@@ -62 +62 @@
     virtual void invalidate() = 0;
     virtual void animationDidSeek() = 0;
+    virtual void animationWasCanceled() = 0;
     virtual void animationSuspensionStateDidChange(bool) = 0;
     virtual void animationTimelineDidChange(AnimationTimeline*) = 0;

trunk/Source/WebCore/animation/AnimationTimeline.cpp

@@ -400 +400 @@
     // 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;
-
-    if (!runningTransitionsByProperty.contains(property)
+    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 (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)
@@ -436 +455 @@
     }
 
-    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

trunk/Source/WebCore/animation/DocumentTimeline.cpp

@@ -308 +308 @@
     AnimationTimeline::removeAnimation(animation);
 
-    if (m_animations.isEmpty())
+    if (!shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         unscheduleAnimationResolution();
 }
@@ -314 +314 @@
 void DocumentTimeline::scheduleAnimationResolution()
 {
-    if (m_isSuspended || m_animations.isEmpty() || m_animationResolutionScheduled)
+    if (m_isSuspended || m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
 
@@ -330 +330 @@
 }
 
+bool DocumentTimeline::shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() const
+{
+    return !m_animations.isEmpty() || !m_acceleratedAnimationsPendingRunningStateChange.isEmpty();
+}
+
 void DocumentTimeline::updateAnimationsAndSendEvents(DOMHighResTimeStamp timestamp)
 {
@@ -338 +343 @@
         cacheCurrentTime(timestamp);
 
-    if (m_isSuspended || m_animations.isEmpty() || !m_animationResolutionScheduled)
+    if (m_isSuspended || !m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
 
@@ -655 +660 @@
 
     m_elementsWithRunningAcceleratedAnimations.add(&element);
+
+    if (shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
+        scheduleAnimationResolution();
+    else
+        unscheduleAnimationResolution();
 }
 

trunk/Source/WebCore/animation/DocumentTimeline.h

@@ -100 +100 @@
     void removeReplacedAnimations();
     bool animationCanBeRemoved(WebAnimation&);
+    bool shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState() const;
 
     Timer m_tickScheduleTimer;

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -1341 +1341 @@
 }
 
+void KeyframeEffect::animationWasCanceled()
+{
+    if (m_shouldRunAccelerated && isRunningAccelerated())
+        addPendingAcceleratedAction(AcceleratedAction::Stop);
+}
+
 void KeyframeEffect::animationSuspensionStateDidChange(bool animationIsSuspended)
 {

trunk/Source/WebCore/animation/KeyframeEffect.h

@@ -114 +114 @@
     void invalidate() override;
     void animationDidSeek() final;
+    void animationWasCanceled() final;
     void animationSuspensionStateDidChange(bool) final;
     void animationTimelineDidChange(AnimationTimeline*) final;

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -615 +615 @@
 
     invalidateEffect();
+
+    if (m_effect)
+        m_effect->animationWasCanceled();
 }