REGRESSION (r263506): timing of CSS Animation on https://animate.style …

…is incorrect https://bugs.webkit.org/show_bug.cgi?id=215807 <rdar://problem/66770136> Reviewed by Simon Fraser. Source/WebCore: Test: webanimations/accelerated-css-animation-with-easing.html In r263506, we added a way for accelerated animations to adhere to both a timing function set on the animation, affecting the timing of the entire animation, as well as a timing function set on individual keyframes, affecting the timing of the animation in a given interval. Alas, this change broke handling of timing functions with implicit animation-timing-function on keyframes. That code change assumed that the Animation object ultimately passed to GraphicsLayerCA::setupAnimation() would return the animation-wide timing function through Animation::timingFunction(). This was correct for JS-originated animations, but not for CSS Animations, since that value was set based on the computed CSS property animation-timing-function for the target element. However, that CSS property does not specify the animation-wide timing function, but rather the default timing function to use for keyframes should they fail to specify an explicit timing function. To fix this, first, we ensure that the animation-wide timing function is set on the Animation object, changing KeyframeEffect::backingAnimationForCompositedRenderer() to always generate an Animation object based on the effect, divorcing itself from the Animation object created through CSS. Then, we add a new member to Animation to specify the default timing function for keyframes, allowing GraphicsLayerCA to use it to determine the timing function when building keyframes. * animation/KeyframeEffect.cpp: (WebCore::KeyframeEffect::backingAnimationForCompositedRenderer const): Always generate an Animation object based on the effect's properties, also setting the new defaultTimingFunctionForKeyframes in the case of a CSS Animation object. * platform/animation/Animation.h: (WebCore::Animation::defaultTimingFunctionForKeyframes const): (WebCore::Animation::setDefaultTimingFunctionForKeyframes): * platform/graphics/ca/GraphicsLayerCA.cpp: (WebCore::animationHasStepsTimingFunction): Use the optional defaultTimingFunctionForKeyframes to determine whether a keyframe uses a steps timing function. (WebCore::GraphicsLayerCA::timingFunctionForAnimationValue): Use the optional defaultTimingFunctionForKeyframes to determine the timing function for a keyframe. LayoutTests: Add a new test that checks that a CSS Animation with implicit and explicit animation-timing-function values behave the same when running accelerated. * webanimations/accelerated-css-animation-with-easing-expected.html: Added. * webanimations/accelerated-css-animation-with-easing.html: Added. Canonical link: https://commits.webkit.org/228695@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266241 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Aug 27, 2020 · a65a423 · a65a423
1 parent 371b0a2
commit a65a423
Show file tree

Hide file tree

Showing 7 changed files with 134 additions and 4 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2020-08-27  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION (r263506): timing of CSS Animation on https://animate.style is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=215807
+        <rdar://problem/66770136>
+
+        Reviewed by Simon Fraser.
+
+        Add a new test that checks that a CSS Animation with implicit and explicit animation-timing-function
+        values behave the same when running accelerated.
+
+        * webanimations/accelerated-css-animation-with-easing-expected.html: Added.
+        * webanimations/accelerated-css-animation-with-easing.html: Added.
+
 2020-08-27  Jer Noble  <jer.noble@apple.com>
 
         REGRESSION (r258215): Title preview movie isn't displayed at www.thismmalife.com

diff --git a/LayoutTests/webanimations/accelerated-css-animation-with-easing-expected.html b/LayoutTests/webanimations/accelerated-css-animation-with-easing-expected.html
@@ -0,0 +1,2 @@
+<body style="background-color: green">
+</body>
diff --git a/LayoutTests/webanimations/accelerated-css-animation-with-easing.html b/LayoutTests/webanimations/accelerated-css-animation-with-easing.html
@@ -0,0 +1,61 @@
+<body>
+<style>
+
+    body {
+        background-color: green;
+    }
+
+    div {
+        position: absolute;
+        top: 0;
+        height: 100px;
+        animation-duration: 1s;
+    }
+
+    #implicit {
+        left: 0;
+        width: 100px;
+        background-color: red;
+        animation-name: implicit;
+    }
+
+    /* Allow for a 1px error on either side */
+    #explicit {
+        left: -1px;
+        width: 102px;
+        background-color: green;
+        animation-name: explicit;
+        animation-timing-function: ease;
+    }
+
+    @keyframes implicit {
+        0.001% { transform: translate(400px) }
+    }
+
+    @keyframes explicit {
+        0.001% {
+            transform: translate(400px);
+            animation-timing-function: ease;
+        }
+    }
+
+</style>
+<div id="implicit"></div>
+<div id="explicit"></div>
+<script>
+
+(async function() {
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    await Promise.all(document.getAnimations().map(animation => animation.ready));
+    await new Promise(requestAnimationFrame);
+    await new Promise(requestAnimationFrame);
+    await new Promise(requestAnimationFrame);
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+})();
+
+</script>
+</body>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,45 @@
+2020-08-27  Antoine Quint  <graouts@webkit.org>
+
+        REGRESSION (r263506): timing of CSS Animation on https://animate.style is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=215807
+        <rdar://problem/66770136>
+
+        Reviewed by Simon Fraser.
+
+        Test: webanimations/accelerated-css-animation-with-easing.html
+
+        In r263506, we added a way for accelerated animations to adhere to both a timing function set on the
+        animation, affecting the timing of the entire animation, as well as a timing function set on individual
+        keyframes, affecting the timing of the animation in a given interval. Alas, this change broke handling
+        of timing functions with implicit animation-timing-function on keyframes.
+
+        That code change assumed that the Animation object ultimately passed to GraphicsLayerCA::setupAnimation()
+        would return the animation-wide timing function through Animation::timingFunction(). This was correct for
+        JS-originated animations, but not for CSS Animations, since that value was set based on the computed CSS
+        property animation-timing-function for the target element. However, that CSS property does not specify
+        the animation-wide timing function, but rather the default timing function to use for keyframes should
+        they fail to specify an explicit timing function.
+
+        To fix this, first, we ensure that the animation-wide timing function is set on the Animation object,
+        changing KeyframeEffect::backingAnimationForCompositedRenderer() to always generate an Animation object
+        based on the effect, divorcing itself from the Animation object created through CSS.
+
+        Then, we add a new member to Animation to specify the default timing function for keyframes, allowing
+        GraphicsLayerCA to use it to determine the timing function when building keyframes.
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::backingAnimationForCompositedRenderer const): Always generate an Animation
+        object based on the effect's properties, also setting the new defaultTimingFunctionForKeyframes in the
+        case of a CSS Animation object.
+        * platform/animation/Animation.h:
+        (WebCore::Animation::defaultTimingFunctionForKeyframes const):
+        (WebCore::Animation::setDefaultTimingFunctionForKeyframes):
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::animationHasStepsTimingFunction): Use the optional defaultTimingFunctionForKeyframes to determine
+        whether a keyframe uses a steps timing function.
+        (WebCore::GraphicsLayerCA::timingFunctionForAnimationValue): Use the optional defaultTimingFunctionForKeyframes
+        to determine the timing function for a keyframe.
+
 2020-08-27  Jer Noble  <jer.noble@apple.com>
 
         REGRESSION (r258215): Title preview movie isn't displayed at www.thismmalife.com

diff --git a/Source/WebCore/animation/KeyframeEffect.cpp b/Source/WebCore/animation/KeyframeEffect.cpp
@@ -1662,8 +1662,6 @@ void KeyframeEffect::applyPendingAcceleratedActions()
 Ref<const Animation> KeyframeEffect::backingAnimationForCompositedRenderer() const
 {
     auto effectAnimation = animation();
-    if (is<DeclarativeAnimation>(effectAnimation))
-        return downcast<DeclarativeAnimation>(effectAnimation)->backingAnimation();
 
     // FIXME: The iterationStart and endDelay AnimationEffectTiming properties do not have
     // corresponding Animation properties.
@@ -1705,6 +1703,12 @@ Ref<const Animation> KeyframeEffect::backingAnimationForCompositedRenderer() con
         break;
     }
 
+    // In the case of CSS Animations, we must set the default timing function for keyframes to match
+    // the current value set for animation-timing-function on the target element which affects only
+    // keyframes and not the animation-wide timing.
+    if (is<CSSAnimation>(effectAnimation))
+        animation->setDefaultTimingFunctionForKeyframes(downcast<CSSAnimation>(effectAnimation)->backingAnimation().timingFunction());
+
     return animation;
 }
 

diff --git a/Source/WebCore/platform/animation/Animation.h b/Source/WebCore/platform/animation/Animation.h
@@ -127,6 +127,7 @@ class Animation : public RefCounted<Animation> {
     TransitionProperty property() const { return m_property; }
     const String& unknownProperty() const { return m_unknownProperty; }
     TimingFunction* timingFunction() const { return m_timingFunction.get(); }
+    TimingFunction* defaultTimingFunctionForKeyframes() const { return m_defaultTimingFunctionForKeyframes.get(); }
 
     void setDelay(double c) { m_delay = c; m_delaySet = true; }
     void setDirection(AnimationDirection d) { m_direction = d; m_directionSet = true; }
@@ -144,6 +145,7 @@ class Animation : public RefCounted<Animation> {
     void setProperty(TransitionProperty t) { m_property = t; m_propertySet = true; }
     void setUnknownProperty(const String& property) { m_unknownProperty = property; }
     void setTimingFunction(RefPtr<TimingFunction>&& function) { m_timingFunction = WTFMove(function); m_timingFunctionSet = true; }
+    void setDefaultTimingFunctionForKeyframes(RefPtr<TimingFunction>&& function) { m_defaultTimingFunctionForKeyframes = WTFMove(function); }
 
     void setIsNoneAnimation(bool n) { m_isNone = n; }
 
@@ -173,6 +175,7 @@ class Animation : public RefCounted<Animation> {
     double m_duration;
     double m_playbackRate { 1 };
     RefPtr<TimingFunction> m_timingFunction;
+    RefPtr<TimingFunction> m_defaultTimingFunctionForKeyframes;
 
     Style::ScopeOrdinal m_nameStyleScopeOrdinal { Style::ScopeOrdinal::Element };
 

diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
@@ -279,12 +279,14 @@ static bool animationHasStepsTimingFunction(const KeyframeValueList& valueList,
 {
     if (is<StepsTimingFunction>(anim->timingFunction()))
         return true;
-
+
+    bool hasStepsDefaultTimingFunctionForKeyframes = is<StepsTimingFunction>(anim->defaultTimingFunctionForKeyframes());
     for (unsigned i = 0; i < valueList.size(); ++i) {
         if (const TimingFunction* timingFunction = valueList.at(i).timingFunction()) {
             if (is<StepsTimingFunction>(timingFunction))
                 return true;
-        }
+        } else if (hasStepsDefaultTimingFunctionForKeyframes)
+            return true;
     }
 
     return false;
@@ -3338,6 +3340,8 @@ const TimingFunction& GraphicsLayerCA::timingFunctionForAnimationValue(const Ani
     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
         if (animValue.timingFunction())
             return *animValue.timingFunction();
+        else if (anim.defaultTimingFunctionForKeyframes())
+            return *anim.defaultTimingFunctionForKeyframes();
         return LinearTimingFunction::sharedLinearTimingFunction();
     }