- Revision
- 289454
- Author
- [email protected]
- Date
- 2022-02-08 22:25:57 -0800 (Tue, 08 Feb 2022)
Log 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):
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (289453 => 289454)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-02-09 06:22:01 UTC (rev 289453)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-02-09 06:25:57 UTC (rev 289454)
@@ -1,5 +1,17 @@
2022-02-08 Antoine Quint <[email protected]>
+ [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 <[email protected]>
+
[web-animations] Animation.commitStyles() should use the non-animated style
https://bugs.webkit.org/show_bug.cgi?id=236315
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt (289453 => 289454)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt 2022-02-09 06:22:01 UTC (rev 289453)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition-expected.txt 2022-02-09 06:25:57 UTC (rev 289454)
@@ -1,11 +1,15 @@
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
PASS unspecified composite mode on a keyframe is overriden by setting accumulate of the effect
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
PASS unspecified composite mode on a keyframe is overriden by setting add of the effect
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html (289453 => 289454)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html 2022-02-09 06:22:01 UTC (rev 289453)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/combining-effects/effect-composition.html 2022-02-09 06:25:57 UTC (rev 289454)
@@ -41,6 +41,34 @@
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 => {
+ const div = createDiv(t);
div.style.marginLeft = '10px';
const anim =
div.animate([{ marginLeft: '10px', composite },
Modified: trunk/Source/WebCore/ChangeLog (289453 => 289454)
--- trunk/Source/WebCore/ChangeLog 2022-02-09 06:22:01 UTC (rev 289453)
+++ trunk/Source/WebCore/ChangeLog 2022-02-09 06:25:57 UTC (rev 289454)
@@ -1,5 +1,22 @@
2022-02-08 Antoine Quint <[email protected]>
+ [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 <[email protected]>
+
[web-animations] Animation.commitStyles() should use the non-animated style
https://bugs.webkit.org/show_bug.cgi?id=236315
Modified: trunk/Source/WebCore/animation/KeyframeEffect.cpp (289453 => 289454)
--- trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-02-09 06:22:01 UTC (rev 289453)
+++ trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-02-09 06:25:57 UTC (rev 289454)
@@ -1468,10 +1468,13 @@
if (propertySpecificKeyframes.isEmpty())
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;
}
@@ -1479,7 +1482,7 @@
// 9. Similarly, if there is no keyframe in property-specific keyframes with a computed keyframe offset of 1, create a new keyframe with a computed
// 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;
}
@@ -1540,13 +1543,23 @@
// (Va) and value to combine (Vb) using the procedure for the composite operation to use corresponding to the
// 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);
+ // 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);
+ }
- 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 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);
+ }
}
// 13. If there is only one keyframe in interval endpoints return the property value of target property on that keyframe.
