- Revision
- 289048
- Author
- [email protected]
- Date
- 2022-02-03 06:02:26 -0800 (Thu, 03 Feb 2022)
Log Message
Incorrect KeyframesEffect generated for background
https://bugs.webkit.org/show_bug.cgi?id=229398
<rdar://problem/82516118>
Reviewed by Darin Adler.
LayoutTests/imported/w3c:
Mark WPT progressions.
* web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt:
* web-platform-tests/web-animations/interfaces/KeyframeEffect/constructor-expected.txt:
* web-platform-tests/web-animations/interfaces/KeyframeEffect/setKeyframes-expected.txt:
Source/WebCore:
Our keyframe merging code when dealing with input from the Web Animations JS API was incorrect.
First, we would iterate over proprties on a keyframe based on the MutableStyleProperties object we
use instead of the HashMap<CSSPropertyID, String> map we use to keep track of the properties set
on the keyframe and its original string value (which we use to return the exact same string when
getKeyframes() is called).
This was incorrect because calling MutableStyleProperties::setProperty() expands shorthands into
longhands, so our property count wouldn't accurate.
Second, honestly I have no idea what I was thinking when I wrote this code as it only ever worked
with a single property on the kefyrame to merge. We now correctly merge all properties from the
keyframe-to-merge into the previous keyframe by using MutableStyleProperties::mergeAndOverrideOnConflict()
and then iterate over all known properties in the HashMap<CSSPropertyID, String> to merge the
properties and strings input.
* animation/KeyframeEffect.cpp:
(WebCore::processPropertyIndexedKeyframes):
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (289047 => 289048)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-02-03 14:02:26 UTC (rev 289048)
@@ -1,3 +1,17 @@
+2022-02-03 Antoine Quint <[email protected]>
+
+ Incorrect KeyframesEffect generated for background
+ https://bugs.webkit.org/show_bug.cgi?id=229398
+ <rdar://problem/82516118>
+
+ Reviewed by Darin Adler.
+
+ Mark WPT progressions.
+
+ * web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt:
+ * web-platform-tests/web-animations/interfaces/KeyframeEffect/constructor-expected.txt:
+ * web-platform-tests/web-animations/interfaces/KeyframeEffect/setKeyframes-expected.txt:
+
2022-02-03 Martin Robinson <[email protected]>
Transform interpolation should blend between shared transform function primitives
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt (289047 => 289048)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-expected.txt 2022-02-03 14:02:26 UTC (rev 289048)
@@ -9,7 +9,7 @@
PASS Element.animate() accepts a one property two value property-indexed keyframes specification
PASS Element.animate() accepts a one shorthand property two value property-indexed keyframes specification
PASS Element.animate() accepts a two property (one shorthand and one of its longhand components) two value property-indexed keyframes specification
-FAIL Element.animate() accepts a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification assert_equals: properties on ComputedKeyframe #0 should match expected "border,borderColor,composite,computedOffset,easing,offset" but got "border,borderTopColor,composite,computedOffset,easing,offset"
+PASS Element.animate() accepts a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification
PASS Element.animate() accepts a two property two value property-indexed keyframes specification
PASS Element.animate() accepts a two property property-indexed keyframes specification with different numbers of values
PASS Element.animate() accepts a property-indexed keyframes specification with an invalid value
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/constructor-expected.txt (289047 => 289048)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/constructor-expected.txt 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/constructor-expected.txt 2022-02-03 14:02:26 UTC (rev 289048)
@@ -11,7 +11,7 @@
PASS A KeyframeEffect constructed with a one shorthand property two value property-indexed keyframes specification roundtrips
PASS A KeyframeEffect can be constructed with a two property (one shorthand and one of its longhand components) two value property-indexed keyframes specification
PASS A KeyframeEffect constructed with a two property (one shorthand and one of its longhand components) two value property-indexed keyframes specification roundtrips
-FAIL A KeyframeEffect can be constructed with a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification assert_equals: properties on ComputedKeyframe #0 should match expected "border,borderColor,composite,computedOffset,easing,offset" but got "border,borderTopColor,composite,computedOffset,easing,offset"
+PASS A KeyframeEffect can be constructed with a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification
PASS A KeyframeEffect constructed with a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification roundtrips
PASS A KeyframeEffect can be constructed with a two property two value property-indexed keyframes specification
PASS A KeyframeEffect constructed with a two property two value property-indexed keyframes specification roundtrips
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/setKeyframes-expected.txt (289047 => 289048)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/setKeyframes-expected.txt 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/setKeyframes-expected.txt 2022-02-03 14:02:26 UTC (rev 289048)
@@ -3,7 +3,7 @@
PASS Keyframes can be replaced with a one property two value property-indexed keyframes specification
PASS Keyframes can be replaced with a one shorthand property two value property-indexed keyframes specification
PASS Keyframes can be replaced with a two property (one shorthand and one of its longhand components) two value property-indexed keyframes specification
-FAIL Keyframes can be replaced with a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification assert_equals: properties on ComputedKeyframe #0 should match expected "border,borderColor,composite,computedOffset,easing,offset" but got "border,borderTopColor,composite,computedOffset,easing,offset"
+PASS Keyframes can be replaced with a two property (one shorthand and one of its shorthand components) two value property-indexed keyframes specification
PASS Keyframes can be replaced with a two property two value property-indexed keyframes specification
PASS Keyframes can be replaced with a two property property-indexed keyframes specification with different numbers of values
PASS Keyframes can be replaced with a property-indexed keyframes specification with an invalid value
Modified: trunk/Source/WebCore/ChangeLog (289047 => 289048)
--- trunk/Source/WebCore/ChangeLog 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/Source/WebCore/ChangeLog 2022-02-03 14:02:26 UTC (rev 289048)
@@ -1,3 +1,30 @@
+2022-02-03 Antoine Quint <[email protected]>
+
+ Incorrect KeyframesEffect generated for background
+ https://bugs.webkit.org/show_bug.cgi?id=229398
+ <rdar://problem/82516118>
+
+ Reviewed by Darin Adler.
+
+ Our keyframe merging code when dealing with input from the Web Animations JS API was incorrect.
+
+ First, we would iterate over proprties on a keyframe based on the MutableStyleProperties object we
+ use instead of the HashMap<CSSPropertyID, String> map we use to keep track of the properties set
+ on the keyframe and its original string value (which we use to return the exact same string when
+ getKeyframes() is called).
+
+ This was incorrect because calling MutableStyleProperties::setProperty() expands shorthands into
+ longhands, so our property count wouldn't accurate.
+
+ Second, honestly I have no idea what I was thinking when I wrote this code as it only ever worked
+ with a single property on the kefyrame to merge. We now correctly merge all properties from the
+ keyframe-to-merge into the previous keyframe by using MutableStyleProperties::mergeAndOverrideOnConflict()
+ and then iterate over all known properties in the HashMap<CSSPropertyID, String> to merge the
+ properties and strings input.
+
+ * animation/KeyframeEffect.cpp:
+ (WebCore::processPropertyIndexedKeyframes):
+
2022-02-03 Carlos Garcia Campos <[email protected]>
[GTK][a11y] Test /webkit/WebKitAccessibility/accessible/children-changed times out
Modified: trunk/Source/WebCore/animation/KeyframeEffect.cpp (289047 => 289048)
--- trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-02-03 13:45:11 UTC (rev 289047)
+++ trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-02-03 14:02:26 UTC (rev 289048)
@@ -398,10 +398,10 @@
// property in cssPropertiesAndValues, so just set this on the previous keyframe.
// In case an invalid or null value was originally provided, then the property
// was not set and the property count is 0, in which case there is nothing to merge.
- if (keyframe.style->propertyCount()) {
- auto property = keyframe.style->propertyAt(0);
- previousKeyframe.style->setProperty(property.id(), property.value());
- previousKeyframe.styleStrings.set(property.id(), keyframe.styleStrings.get(property.id()));
+ if (keyframe.styleStrings.size()) {
+ previousKeyframe.style->mergeAndOverrideOnConflict(keyframe.style);
+ for (auto& [property, value] : keyframe.styleStrings)
+ previousKeyframe.styleStrings.set(property, value);
}
// Since we've processed this keyframe, we can remove it and keep i the same
// so that we process the next keyframe in the next loop iteration.
