[Web Animations] Correctly handle timing functions specified by CSS Animations and...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2018 19:08:01 +0000 (19:08 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2018 19:08:01 +0000 (19:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183935

Reviewed by Dean Jackson.

LayoutTests/imported/mozilla:

Record progressions of Mozilla tests.

* css-animations/test_animation-computed-timing-expected.txt:
* css-transitions/test_animation-computed-timing-expected.txt:
* css-transitions/test_keyframeeffect-getkeyframes-expected.txt:

Source/WebCore:

We were incorrectly reflecting the animation-timing-function and transition-timing-function values on the generated
DeclarativeAnimation effect timing "easing" property. In fact, those values should only be represented on the keyframes.

In the case of a CSS Animation, the animation-timing-function property set on the element's style serves as the default
value used for all keyframes, and individual keyframes can specify an overriding animation-timing-function. For a CSS
Transition, the transition-timing-function property set on the element's style serves as the timing function of the
from keyframe.

To correctly reflect this, we provide a new timingFunctionForKeyframeAtIndex() function on KeyframeEffectReadOnly
which will return the right TimingFunction object at a given index, regardless of the animation type. In the case
of getKeyframes(), we manually return "linear" for the "to" keyframe since timingFunctionForKeyframeAtIndex()
would otherwise return the same timing function as the "from" keyframe. This avoids creating an extra
LinearTimingFunction object.

As a result, a number of Mozilla imported tests progress since we have correct information on the "easing" property
of objects returned by getKeyframes() and the "progress" reported by getComputedTiming() now always uses a linear
timing function.

* animation/DeclarativeAnimation.cpp:
(WebCore::DeclarativeAnimation::syncPropertiesWithBackingAnimation): The timing function of the backing Animation should
not be reflected on the effect's timing object.
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::getKeyframes): Return the correct timing function for a keyframe, and use a "linear"
value for the "to" keyframe of a CSS Transition.
(WebCore::KeyframeEffectReadOnly::setAnimatedPropertiesInStyle):
(WebCore::KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex):
* animation/KeyframeEffectReadOnly.h:

LayoutTests:

We now pass 2 additional Mozilla tests completely, so they no longer need to be marked as flaky failures or timeouts.
We also update tests that we wrote ourselves and which incorrectly assumed that the effect's timing would reflect
the timing function set by CSS.

* TestExpectations:
* webanimations/css-animations-expected.txt:
* webanimations/css-animations.html:
* webanimations/css-transitions-expected.txt:
* webanimations/css-transitions.html:

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/imported/mozilla/ChangeLog
LayoutTests/imported/mozilla/css-animations/test_animation-computed-timing-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_animation-computed-timing-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_keyframeeffect-getkeyframes-expected.txt
LayoutTests/webanimations/css-animations-expected.txt
LayoutTests/webanimations/css-animations.html
LayoutTests/webanimations/css-transitions-expected.txt
LayoutTests/webanimations/css-transitions.html
Source/WebCore/ChangeLog
Source/WebCore/animation/DeclarativeAnimation.cpp
Source/WebCore/animation/KeyframeEffectReadOnly.cpp
Source/WebCore/animation/KeyframeEffectReadOnly.h

index 51269a0..25be0fe 100644 (file)
@@ -1,3 +1,20 @@
+2018-03-23  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Correctly handle timing functions specified by CSS Animations and CSS Transitions
+        https://bugs.webkit.org/show_bug.cgi?id=183935
+
+        Reviewed by Dean Jackson.
+
+        We now pass 2 additional Mozilla tests completely, so they no longer need to be marked as flaky failures or timeouts.
+        We also update tests that we wrote ourselves and which incorrectly assumed that the effect's timing would reflect
+        the timing function set by CSS.
+
+        * TestExpectations:
+        * webanimations/css-animations-expected.txt:
+        * webanimations/css-animations.html:
+        * webanimations/css-transitions-expected.txt:
+        * webanimations/css-transitions.html:
+
 2018-03-26  Ryan Haddad  <ryanhaddad@apple.com>
 
         Mark imported/mozilla/css-animations/test_animation-cancel.html as flaky.
index aa8ee26..c5e7b11 100644 (file)
@@ -1708,8 +1708,6 @@ webkit.org/b/181122 imported/w3c/web-platform-tests/web-animations/interfaces/An
 webkit.org/b/181123 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finish.html [ Pass Failure ]
 webkit.org/b/181888 imported/w3c/web-platform-tests/web-animations/timing-model/animation-effects/current-iteration.html [ Pass Failure ]
 
-webkit.org/b/183816 imported/mozilla/css-transitions/test_keyframeeffect-getkeyframes.html [ Pass Failure Timeout ]
-webkit.org/b/183817 imported/mozilla/css-animations/test_animation-computed-timing.html [ Pass Failure Timeout ]
 webkit.org/b/183818 imported/mozilla/css-animations/test_pseudoElement-get-animations.html [ Pass Failure Timeout ]
 webkit.org/b/183819 imported/mozilla/css-animations/test_animation-currenttime.html [ Pass Failure Timeout ]
 webkit.org/b/184011 imported/mozilla/css-animations/test_animation-cancel.html [ Pass Failure ]
index 5650267..e8687ed 100644 (file)
@@ -1,5 +1,18 @@
 2018-03-23  Antoine Quint  <graouts@apple.com>
 
+        [Web Animations] Correctly handle timing functions specified by CSS Animations and CSS Transitions
+        https://bugs.webkit.org/show_bug.cgi?id=183935
+
+        Reviewed by Dean Jackson.
+
+        Record progressions of Mozilla tests.
+
+        * css-animations/test_animation-computed-timing-expected.txt:
+        * css-transitions/test_animation-computed-timing-expected.txt:
+        * css-transitions/test_keyframeeffect-getkeyframes-expected.txt:
+
+2018-03-23  Antoine Quint  <graouts@apple.com>
+
         [Web Animations] infinite repeat counts aren't reflected for CSS Animations
         https://bugs.webkit.org/show_bug.cgi?id=183932
 
index 274ca26..6a0917b 100644 (file)
@@ -10,7 +10,7 @@ PASS iterations of a finitely repeating animation
 PASS iterations of an infinitely repeating animation 
 PASS duration of a new animation 
 PASS direction of a new animation 
-FAIL easing of a new animation assert_equals: Initial value of easing expected "linear" but got "ease"
+PASS easing of a new animation 
 PASS endTime of an new animation 
 PASS endTime of an animation with a negative delay 
 PASS endTime of an infinitely repeating animation 
@@ -25,13 +25,13 @@ PASS localTime of an animation is always equal to currentTime
 PASS localTime reflects playbackRate immediately 
 PASS localTime of an AnimationEffect without an Animation 
 PASS progress of an animation with different fill modes 
-FAIL progress of an integral repeating animation with normal direction assert_equals: Value of progress expected 0.25 but got 0.4085665049012694
+PASS progress of an integral repeating animation with normal direction 
 PASS progress of an infinitely repeating zero-duration animation 
 PASS progress of a finitely repeating zero-duration animation 
 PASS progress of a non-integral repeating zero-duration animation 
 PASS Progress of a non-integral repeating zero-duration animation with reversing direction 
-FAIL progress of a non-integral repeating animation with alternate direction assert_equals: Value of progress expected 0.25 but got 0.4085665049012694
-FAIL progress of a non-integral repeating animation with alternate-reversing direction assert_equals: Value of progress expected 0.75 but got 0.9604590729525889
+PASS progress of a non-integral repeating animation with alternate direction 
+PASS progress of a non-integral repeating animation with alternate-reversing direction 
 PASS progress of a non-integral repeating zero-duration animation with alternate direction 
 PASS progress of a non-integral repeating zero-duration animation with alternate-reverse direction 
 PASS currentIteration of a new animation with no backwards fill is unresolved in before phase 
index 8e496f7..4a49871 100644 (file)
@@ -8,7 +8,7 @@ PASS iterationStart of a new transition
 PASS iterations of a new transition 
 PASS duration of a new transition 
 PASS direction of a new transition 
-FAIL easing of a new transition assert_equals: Initial value of easing expected "linear" but got "ease"
+PASS easing of a new transition 
 PASS endTime of a new transition 
 PASS activeDuration of a new transition 
 PASS localTime of a new transition 
index d717ad5..79aaf09 100644 (file)
@@ -1,5 +1,5 @@
 
-FAIL KeyframeEffectReadOnly.getKeyframes() returns expected frames for a simple transition assert_equals: value for 'easing' on ComputedKeyframe #0 expected "ease" but got "linear"
-FAIL KeyframeEffectReadOnly.getKeyframes() returns expected frames for a simple transition with a non-default easing function assert_equals: value for 'easing' on ComputedKeyframe #0 expected "steps(2)" but got "linear"
-FAIL KeyframeEffectReadOnly.getKeyframes() returns expected frames for a transition with a CSS variable endpoint assert_equals: value for 'easing' on ComputedKeyframe #0 expected "ease" but got "linear"
+PASS KeyframeEffectReadOnly.getKeyframes() returns expected frames for a simple transition 
+PASS KeyframeEffectReadOnly.getKeyframes() returns expected frames for a simple transition with a non-default easing function 
+PASS KeyframeEffectReadOnly.getKeyframes() returns expected frames for a transition with a CSS variable endpoint 
 
index 782354f..daa98a6 100644 (file)
@@ -7,7 +7,7 @@ PASS Web Animations should reflect the animation-fill-mode property.
 PASS Web Animations should reflect the animation-iteration-count property. 
 PASS Web Animations should reflect the animation-name property. 
 PASS Web Animations should reflect the animation-play-state property. 
-PASS Web Animations should reflect the animation-timing-function property
+PASS Web Animations should not reflect the animation-timing-function property on the effect's timing
 PASS Calling finish() on the animation no longer lists the animation after it has been running. 
 PASS Seeking the animation to its end time no longer lists the animation after it has been running. 
 PASS Setting the target's animation-name to none no longer lists the animation after it has been running. 
index 479772e..949b2ce 100644 (file)
@@ -59,7 +59,6 @@ targetTest(target => {
     assert_equals(timing.duration, 2000, "The animation's duration property matches the animation-duration property.");
     assert_equals(timing.iterations, 2, "The animation's iterations property matches the animation-iteration-count property.");
     assert_equals(timing.direction, "alternate", "The animation's direction property matches the animation-direction property.");
-    assert_equals(timing.easing, "linear", "The animation's easing property matches the animation-timing-function property.");
 
     const computedTiming = animation.effect.getComputedTiming();
     assert_equals(computedTiming.activeDuration, 4000, "The animations's computed timing activeDuration property matches the properties set by CSS");
@@ -196,14 +195,14 @@ targetTest(target => {
     target.style.animationTimingFunction = "ease-out";
     target.style.animationName = "animation";
 
-    assert_equals(target.getAnimations()[0].effect.timing.easing, "ease-out", "The animation's easing matches the initial animation-timing-function property.");
+    assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the initial animation-timing-function property.");
 
     target.style.animationTimingFunction = "ease-in";
-    assert_equals(target.getAnimations()[0].effect.timing.easing, "ease-in", "The animation's easing matches the updated animation-timing-function property.");
+    assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the updated animation-timing-function property.");
 
     target.style.removeProperty("animation-timing-function");
-    assert_equals(target.getAnimations()[0].effect.timing.easing, "ease", "The animation's easing matches the default animation-timing-function value when the property is not set.");
-}, "Web Animations should reflect the animation-timing-function property.");
+    assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the default animation-timing-function value when the property is not set.");
+}, "Web Animations should not reflect the animation-timing-function property on the effect's timing.");
 
 function runAnimationCompletionTest(finalAssertionCallback, description)
 {
index 8586e7a..2cbae40 100644 (file)
@@ -3,7 +3,7 @@ PASS A CSS Transition should be reflected entirely as a CSSTransition object on
 PASS Web Animations should reflect the transition-delay property. 
 PASS Web Animations should reflect the transition-duration property. 
 PASS Web Animations should reflect the transition-property property. 
-PASS Web Animations should reflect the transition-timing-function property
+PASS Web Animations should not reflect the transition-timing-function property on the effect's timing
 PASS Calling finish() on the animation no longer lists the animation after it has been running. 
 PASS Seeking the animation to its end time no longer lists the animation after it has been running. 
 PASS Setting the target's transition-property to none no longer lists the animation after it has been running. 
index 7be8607..3c452aa 100644 (file)
@@ -52,7 +52,6 @@ targetTest(target => {
     const timing = animation.effect.timing;
     assert_equals(timing.delay, -1000, "The animation's delay property matches the transition-delay property.");
     assert_equals(timing.duration, 2000, "The animation's duration property matches the transition-duration property.");
-    assert_equals(timing.easing, "linear", "The animation's easing property matches the transition-timing-function property.");
 
     const computedTiming = animation.effect.getComputedTiming();
     assert_equals(computedTiming.activeDuration, 2000, "The animations's computed timing activeDuration property matches the properties set by CSS");
@@ -153,19 +152,16 @@ targetTest(target => {
 
 transitionPropertyUpdateTest(target => {
     const initialAnimation = target.getAnimations()[0];
-    assert_equals(initialAnimation.effect.timing.easing, "ease", "The animation's easing matches the default transition-timing-function property.");
+    assert_equals(initialAnimation.effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function property.");
 
     target.style.transitionTimingFunction = "ease-in";
     const updatedAnimation = target.getAnimations()[0];
-    assert_equals(updatedAnimation.effect.timing.easing, "ease-in", "The animation's easing matches the updated transition-timing-function property.");
+    assert_equals(updatedAnimation.effect.timing.easing, "linear", "The animation's easing does not match the updated transition-timing-function property.");
 
     target.style.removeProperty("transition-timing-function");
     const finalAnimation = target.getAnimations()[0];
-    assert_equals(target.getAnimations()[0].effect.timing.easing, "ease", "The animation's easing matches the default transition-timing-function value when the property is not set.");
-
-    assert_not_equals(initialAnimation, updatedAnimation, "The animations before and after updating the transition-timing-function property differ.");
-    assert_not_equals(updatedAnimation, finalAnimation, "The animations before and after updating the transition-timing-function property differ.");
-}, "Web Animations should reflect the transition-timing-function property.");
+    assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function value when the property is not set.");
+}, "Web Animations should not reflect the transition-timing-function property on the effect's timing.");
 
 function runAnimationCompletionTest(finalAssertionCallback, description)
 {
index 1524dee..765866c 100644 (file)
@@ -1,3 +1,38 @@
+2018-03-23  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Correctly handle timing functions specified by CSS Animations and CSS Transitions
+        https://bugs.webkit.org/show_bug.cgi?id=183935
+
+        Reviewed by Dean Jackson.
+
+        We were incorrectly reflecting the animation-timing-function and transition-timing-function values on the generated
+        DeclarativeAnimation effect timing "easing" property. In fact, those values should only be represented on the keyframes.
+
+        In the case of a CSS Animation, the animation-timing-function property set on the element's style serves as the default
+        value used for all keyframes, and individual keyframes can specify an overriding animation-timing-function. For a CSS
+        Transition, the transition-timing-function property set on the element's style serves as the timing function of the
+        from keyframe.
+
+        To correctly reflect this, we provide a new timingFunctionForKeyframeAtIndex() function on KeyframeEffectReadOnly
+        which will return the right TimingFunction object at a given index, regardless of the animation type. In the case
+        of getKeyframes(), we manually return "linear" for the "to" keyframe since timingFunctionForKeyframeAtIndex()
+        would otherwise return the same timing function as the "from" keyframe. This avoids creating an extra
+        LinearTimingFunction object.
+
+        As a result, a number of Mozilla imported tests progress since we have correct information on the "easing" property
+        of objects returned by getKeyframes() and the "progress" reported by getComputedTiming() now always uses a linear
+        timing function.
+
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::syncPropertiesWithBackingAnimation): The timing function of the backing Animation should
+        not be reflected on the effect's timing object.
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::getKeyframes): Return the correct timing function for a keyframe, and use a "linear"
+        value for the "to" keyframe of a CSS Transition.
+        (WebCore::KeyframeEffectReadOnly::setAnimatedPropertiesInStyle):
+        (WebCore::KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex):
+        * animation/KeyframeEffectReadOnly.h:
+
 2018-03-26  Chris Dumez  <cdumez@apple.com>
 
         Use SecurityOriginData more consistently in Service Worker code
index db4a968..3b93378 100644 (file)
@@ -81,7 +81,6 @@ void DeclarativeAnimation::syncPropertiesWithBackingAnimation()
     auto* timing = effect()->timing();
     timing->setDelay(Seconds(m_backingAnimation->delay()));
     timing->setIterationDuration(Seconds(m_backingAnimation->duration()));
-    timing->setTimingFunction(m_backingAnimation->timingFunction());
 
     unsuspendEffectInvalidation();
 }
index 4ab4c54..372062e 100644 (file)
@@ -524,6 +524,9 @@ Vector<Strong<JSObject>> KeyframeEffectReadOnly::getKeyframes(ExecState& state)
             BaseComputedKeyframe computedKeyframe;
             computedKeyframe.offset = keyframe.key();
             computedKeyframe.computedOffset = keyframe.key();
+            // For CSS transitions, there are only two keyframes and the second keyframe should always report "linear". In practice, this value
+            // has no bearing since, as the last keyframe, its value will never be used.
+            computedKeyframe.easing = is<CSSTransition>(animation()) && i == 1 ? "linear" : timingFunctionForKeyframeAtIndex(0)->cssText();
 
             auto outputKeyframe = convertDictionaryToJS(state, *jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject()), computedKeyframe);
 
@@ -545,7 +548,7 @@ Vector<Strong<JSObject>> KeyframeEffectReadOnly::getKeyframes(ExecState& state)
             result.append(JSC::Strong<JSC::JSObject> { state.vm(), outputKeyframe });
         }
     } else {
-        for (auto& parsedKeyframe : m_parsedKeyframes) {
+        for (size_t i = 0; i < m_parsedKeyframes.size(); ++i) {
             // 1. Initialize a dictionary object, output keyframe, using the following definition:
             //
             // dictionary BaseComputedKeyframe {
@@ -555,12 +558,14 @@ Vector<Strong<JSObject>> KeyframeEffectReadOnly::getKeyframes(ExecState& state)
             //      CompositeOperation? composite = null;
             // };
 
+            auto& parsedKeyframe = m_parsedKeyframes[i];
+
             // 2. Set offset, computedOffset, easing, composite members of output keyframe to the respective values keyframe offset, computed keyframe
             // offset, keyframe-specific timing function and keyframe-specific composite operation of keyframe.
             BaseComputedKeyframe computedKeyframe;
             computedKeyframe.offset = parsedKeyframe.offset;
             computedKeyframe.computedOffset = parsedKeyframe.computedOffset;
-            computedKeyframe.easing = parsedKeyframe.timingFunction->cssText();
+            computedKeyframe.easing = timingFunctionForKeyframeAtIndex(i)->cssText();
             computedKeyframe.composite = parsedKeyframe.composite;
 
             auto outputKeyframe = convertDictionaryToJS(state, *jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject()), computedKeyframe);
@@ -977,11 +982,10 @@ void KeyframeEffectReadOnly::setAnimatedPropertiesInStyle(RenderStyle& targetSty
         // 17. Let transformed distance be the result of evaluating the timing function associated with the first keyframe in interval endpoints
         //     passing interval distance as the input progress.
         auto transformedDistance = intervalDistance;
-        // In case we're backing a CSSAnimation or CSSTransition we won't actually have parsed keyframes.
-        if (startKeyframeIndex && startKeyframeIndex.value() + 1 <= m_parsedKeyframes.size()) {
+        if (startKeyframeIndex) {
             if (auto iterationDuration = timing()->iterationDuration()) {
                 auto rangeDuration = (endOffset - startOffset) * iterationDuration.seconds();
-                transformedDistance = m_parsedKeyframes[startKeyframeIndex.value()].timingFunction->transformTime(intervalDistance, rangeDuration);
+                transformedDistance = timingFunctionForKeyframeAtIndex(startKeyframeIndex.value())->transformTime(intervalDistance, rangeDuration);
             }
         }
 
@@ -994,6 +998,27 @@ void KeyframeEffectReadOnly::setAnimatedPropertiesInStyle(RenderStyle& targetSty
     }
 }
 
+TimingFunction* KeyframeEffectReadOnly::timingFunctionForKeyframeAtIndex(size_t index)
+{
+    if (!m_parsedKeyframes.isEmpty())
+        return m_parsedKeyframes[index].timingFunction.get();
+
+    auto effectAnimation = animation();
+
+    // If we didn't have parsed keyframes, we must be dealing with a declarative animation.
+    ASSERT(is<DeclarativeAnimation>(effectAnimation));
+
+    // If we're dealing with a CSS Animation, the timing function is specified either on the keyframe itself,
+    // or failing that on the backing Animation object which defines the default for all keyframes.
+    if (is<CSSAnimation>(effectAnimation)) {
+        if (auto* timingFunction = m_blendingKeyframes[index].timingFunction(downcast<CSSAnimation>(effectAnimation)->animationName()))
+            return timingFunction;
+    }
+
+    // Failing that, or for a CSS Transition, the timing function is inherited from the backing Animation object. 
+    return downcast<DeclarativeAnimation>(effectAnimation)->backingAnimation().timingFunction();
+}
+
 void KeyframeEffectReadOnly::startOrStopAccelerated()
 {
     auto* renderer = this->renderer();
index a2f421e..9ffa82b 100644 (file)
@@ -126,6 +126,7 @@ protected:
 
 private:
     void setAnimatedPropertiesInStyle(RenderStyle&, double);
+    TimingFunction* timingFunctionForKeyframeAtIndex(size_t);
     void computeStackingContextImpact();
     void updateBlendingKeyframes();
     bool shouldRunAccelerated();