Changeset 289454 in webkit

Timestamp:

Feb 8, 2022 10:25:57 PM (5 months ago)

Author:

graouts@webkit.org

Message:

[web-animations] additive and accumulation interpolation does not work correctly with implicit 0% and 100% keyframes
​https://bugs.webkit.org/show_bug.cgi?id=236314

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Add some new WPT tests and mark our PASS results for the "add" case (we still fail for "accumulate").

• web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt:
• web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html:

Source/WebCore:

We incorrectly handled implicit keyframes for the additive and accumulate cases.

The spec says that for implicit 0% and 100% keyframes, a keyframe should be generated with a "neutral" keyframe which,
when added or accumulated with another keyframe, would yield the same style as the keyframe it's composed with. We sort
of did the right thing by cloning the underlying style for those keyframes, but then we would blame them anyway in the
composition case, whereas we should just use the underlying style as-is.

• animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::setAnimatedPropertiesInStyle):

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt (modified) (2 diffs)
• LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (3 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-02-08  Antoine Quint  <graouts@webkit.org>
+
+        [web-animations] additive and accumulation interpolation does not work correctly with implicit 0% and 100% keyframes
+        https://bugs.webkit.org/show_bug.cgi?id=236314
+
+        Reviewed by Dean Jackson.
+
+        Add some new WPT tests and mark our PASS results for the "add" case (we still fail for "accumulate").
+
+        * web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt:
+        * web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html:
+
 2022-02-08  Antoine Quint  <graouts@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt

@@ -2 +2 @@
 PASS accumulate onto the base value
 PASS accumulate onto an underlying animation value
+FAIL accumulate onto an underlying animation value with implicit from values assert_equals: Animated style at 50% expected "matrix(1, 0, 0, 1, 50, 50)" but got "matrix(1, 0, 0, 1, 25, 50)"
+FAIL accumulate onto an underlying animation value with implicit to values assert_equals: Animated style at 50% expected "matrix(1, 0, 0, 1, 50, 50)" but got "matrix(1, 0, 0, 1, 25, 50)"
 PASS Composite when mixing accumulate and replace
 PASS accumulate specified on a keyframe overrides the composite mode of the effect
@@ -7 +9 @@
 PASS add onto the base value
 PASS add onto an underlying animation value
+PASS add onto an underlying animation value with implicit from values
+PASS add onto an underlying animation value with implicit to values
 PASS Composite when mixing add and replace
 PASS add specified on a keyframe overrides the composite mode of the effect

trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html

@@ -39 +39 @@
       'Animated style at 50%');
   }, `${composite} onto an underlying animation value`);
+
+  test(t => {
+    const div = createDiv(t);
+    const anims = [];
+    anims.push(div.animate({ transform: 'translateX(100px)' }, { duration: 100, composite: 'replace' }));
+    anims.push(div.animate({ transform: 'translateY(100px)' }, { duration: 100, composite }));
+
+    for (const anim of anims) {
+      anim.currentTime = 50;
+    }
+
+    assert_equals(getComputedStyle(div).transform, 'matrix(1, 0, 0, 1, 50, 50)',
+      'Animated style at 50%');
+  }, `${composite} onto an underlying animation value with implicit from values`);
+
+  test(t => {
+    const div = createDiv(t);
+    const anims = [];
+    anims.push(div.animate([{ offset: 1, transform: 'translateX(100px)' }], { duration: 100, composite: 'replace' }));
+    anims.push(div.animate([{ offset: 1, transform: 'translateY(100px)' }], { duration: 100, composite }));
+
+    for (const anim of anims) {
+      anim.currentTime = 50;
+    }
+
+    assert_equals(getComputedStyle(div).transform, 'matrix(1, 0, 0, 1, 50, 50)',
+      'Animated style at 50%');
+  }, `${composite} onto an underlying animation value with implicit to values`);
 
   test(t => {

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-02-08  Antoine Quint  <graouts@webkit.org>
+
+        [web-animations] additive and accumulation interpolation does not work correctly with implicit 0% and 100% keyframes
+        https://bugs.webkit.org/show_bug.cgi?id=236314
+
+        Reviewed by Dean Jackson.
+
+        We incorrectly handled implicit keyframes for the additive and accumulate cases.
+
+        The spec says that for implicit 0% and 100% keyframes, a keyframe should be generated with a "neutral" keyframe which,
+        when added or accumulated with another keyframe, would yield the same style as the keyframe it's composed with. We sort
+        of did the right thing by cloning the underlying style for those keyframes, but then we would blame them anyway in the
+        composition case, whereas we should just use the underlying style as-is.
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::setAnimatedPropertiesInStyle):
+
 2022-02-08  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -1469 +1469 @@
             continue;
 
+        auto hasImplicitZeroKeyframe = !numberOfKeyframesWithZeroOffset;
+        auto hasImplicitOneKeyframe = !numberOfKeyframesWithOneOffset;
+
         // 8. If there is no keyframe in property-specific keyframes with a computed keyframe offset of 0, create a new keyframe with a computed keyframe
         //    offset of 0, a property value set to the neutral value for composition, and a composite operation of add, and prepend it to the beginning of
         //    property-specific keyframes.
-        if (!numberOfKeyframesWithZeroOffset) {
+        if (hasImplicitZeroKeyframe) {
             propertySpecificKeyframes.insert(0, &propertySpecificKeyframeWithZeroOffset);
             numberOfKeyframesWithZeroOffset = 1;
@@ -1480 +1483 @@
         //    keyframe offset of 1, a property value set to the neutral value for composition, and a composite operation of add, and append it to the end of
         //    property-specific keyframes.
-        if (!numberOfKeyframesWithOneOffset) {
+        if (hasImplicitOneKeyframe) {
             propertySpecificKeyframes.append(&propertySpecificKeyframeWithOneOffset);
             numberOfKeyframesWithOneOffset = 1;
@@ -1541 +1544 @@
         //            target property’s animation type.
         if (CSSPropertyAnimation::isPropertyAdditiveOrCumulative(cssPropertyId)) {
-            auto startKeyframeCompositeOperation = startKeyframe.compositeOperation().value_or(m_compositeOperation);
-            if (startKeyframeCompositeOperation != CompositeOperation::Replace)
-                CSSPropertyAnimation::blendProperties(this, cssPropertyId, startKeyframeStyle, targetStyle, *startKeyframe.style(), 1, startKeyframeCompositeOperation);
-
-            auto endKeyframeCompositeOperation = endKeyframe.compositeOperation().value_or(m_compositeOperation);
-            if (endKeyframeCompositeOperation != CompositeOperation::Replace)
-                CSSPropertyAnimation::blendProperties(this, cssPropertyId, endKeyframeStyle, targetStyle, *endKeyframe.style(), 1, endKeyframeCompositeOperation);
+            // Only do this for the 0 keyframe if it was provided explicitly, since otherwise we want to use the "neutral value
+            // for composition" which really means we don't want to do anything but rather just use the underlying style which
+            // is already set on startKeyframe.
+            if (!startKeyframe.key() && !hasImplicitZeroKeyframe) {
+                auto startKeyframeCompositeOperation = startKeyframe.compositeOperation().value_or(m_compositeOperation);
+                if (startKeyframeCompositeOperation != CompositeOperation::Replace)
+                    CSSPropertyAnimation::blendProperties(this, cssPropertyId, startKeyframeStyle, targetStyle, *startKeyframe.style(), 1, startKeyframeCompositeOperation);
+            }
+
+            // Only do this for the 1 keyframe if it was provided explicitly, since otherwise we want to use the "neutral value
+            // for composition" which really means we don't want to do anything but rather just use the underlying style which
+            // is already set on endKeyframe.
+            if (endKeyframe.key() == 1 && !hasImplicitOneKeyframe) {
+                auto endKeyframeCompositeOperation = endKeyframe.compositeOperation().value_or(m_compositeOperation);
+                if (endKeyframeCompositeOperation != CompositeOperation::Replace)
+                    CSSPropertyAnimation::blendProperties(this, cssPropertyId, endKeyframeStyle, targetStyle, *endKeyframe.style(), 1, endKeyframeCompositeOperation);
+            }
         }