- Revision
- 287524
- Author
- [email protected]
- Date
- 2022-01-02 15:42:27 -0800 (Sun, 02 Jan 2022)
Log Message
LayoutTests/imported/w3c:
[Web Animations] getKeyframes() should handle multiple 0% and 100% keyframes
https://bugs.webkit.org/show_bug.cgi?id=234799
Reviewed by Dean Jackson.
Mark a WPT progression.
* web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:
Source/WebCore:
[Web Animations] getKeyframes() should handle multiple keyframes for the same offset
https://bugs.webkit.org/show_bug.cgi?id=234799
Reviewed by Dean Jackson.
A @keyframes rule may specify multiple keyframes for the same offset. We currently have support
for merging such keyframes into consolidated keyframes in Style::Resolver::keyframeStylesForAnimation()
but WPT shows that it's lacking since it doesn't account for animation-timing-function. Indeed, we
need to be careful not to merge keyframes with the same offset but a different timing function.
So we now use a std::pair<> using an offset and a timing function to compute de-duplicated keyframes.
But this showed an issue with StepsTimingFunction::operator== where we would not return true for
steps(1) and steps(1, end) since we only checked for equality for the optional keyword and did not
account for the default "end" value should the keyword not be explicit.
This ensured we correctly de-duplicated keyframes accounting for not only the offset but also
the timing function. But this then highlighted a different issue which was that our getKeyframes()
only ever expected one keyframe respectively for the 0% or 100% offsets when filling in implicit
values, which we'd just added support for in bug 234795.
So in KeyframeEffect::getKeyframes() we now compile a list of properties not explicitly specified
on any of the 0% or 100% keyframes and, as we process the first of a 0% or 100% keyframe, we set
the implicit properties.
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::getKeyframes):
* platform/animation/TimingFunction.h:
* style/StyleResolver.cpp:
(WebCore::Style::Resolver::keyframeStylesForAnimation):
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (287523 => 287524)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-02 23:42:27 UTC (rev 287524)
@@ -1,5 +1,16 @@
2022-01-02 Antoine Quint <[email protected]>
+ [Web Animations] getKeyframes() should handle multiple 0% and 100% keyframes
+ https://bugs.webkit.org/show_bug.cgi?id=234799
+
+ Reviewed by Dean Jackson.
+
+ Mark a WPT progression.
+
+ * web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:
+
+2022-01-02 Antoine Quint <[email protected]>
+
[Web Animations] getKeyframes() should ensure that all properties are present on 0% and 100% keyframes
https://bugs.webkit.org/show_bug.cgi?id=234795
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt (287523 => 287524)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt 2022-01-02 23:42:27 UTC (rev 287524)
@@ -12,7 +12,7 @@
PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with different properties on different keyframes, all with the same easing function
PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with different properties on different keyframes, with a different easing function on each
PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time, and all with the same easing function
-FAIL KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different easing functions assert_equals: Number of keyframes should match expected 3 but got 2
+PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different easing functions
PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different but equivalent easing functions
PASS KeyframeEffect.getKeyframes() returns expected frames for overlapping keyframes
FAIL KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes assert_equals: value for 'filter' on Keyframe #1 should match expected "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6) saturate(0.3)"
Modified: trunk/Source/WebCore/ChangeLog (287523 => 287524)
--- trunk/Source/WebCore/ChangeLog 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/Source/WebCore/ChangeLog 2022-01-02 23:42:27 UTC (rev 287524)
@@ -1,3 +1,35 @@
+2022-01-02 Antoine Quint <[email protected]>
+
+ [Web Animations] getKeyframes() should handle multiple keyframes for the same offset
+ https://bugs.webkit.org/show_bug.cgi?id=234799
+
+ Reviewed by Dean Jackson.
+
+ A @keyframes rule may specify multiple keyframes for the same offset. We currently have support
+ for merging such keyframes into consolidated keyframes in Style::Resolver::keyframeStylesForAnimation()
+ but WPT shows that it's lacking since it doesn't account for animation-timing-function. Indeed, we
+ need to be careful not to merge keyframes with the same offset but a different timing function.
+
+ So we now use a std::pair<> using an offset and a timing function to compute de-duplicated keyframes.
+ But this showed an issue with StepsTimingFunction::operator== where we would not return true for
+ steps(1) and steps(1, end) since we only checked for equality for the optional keyword and did not
+ account for the default "end" value should the keyword not be explicit.
+
+ This ensured we correctly de-duplicated keyframes accounting for not only the offset but also
+ the timing function. But this then highlighted a different issue which was that our getKeyframes()
+ only ever expected one keyframe respectively for the 0% or 100% offsets when filling in implicit
+ values, which we'd just added support for in bug 234795.
+
+ So in KeyframeEffect::getKeyframes() we now compile a list of properties not explicitly specified
+ on any of the 0% or 100% keyframes and, as we process the first of a 0% or 100% keyframe, we set
+ the implicit properties.
+
+ * animation/KeyframeEffect.cpp:
+ (WebCore::KeyframeEffect::getKeyframes):
+ * platform/animation/TimingFunction.h:
+ * style/StyleResolver.cpp:
+ (WebCore::Style::Resolver::keyframeStylesForAnimation):
+
2022-01-02 Wenson Hsieh <[email protected]>
Followup to r287494
Modified: trunk/Source/WebCore/animation/KeyframeEffect.cpp (287523 => 287524)
--- trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-02 23:42:27 UTC (rev 287524)
@@ -639,7 +639,23 @@
auto computedStyleExtractor = ComputedStyleExtractor(target, false, m_pseudoId);
+ // We need to establish which properties are implicit for 0% and 100%.
+ HashSet<CSSPropertyID> zeroKeyframeProperties = m_blendingKeyframes.properties();
+ HashSet<CSSPropertyID> _oneKeyframeProperties_ = m_blendingKeyframes.properties();
+ zeroKeyframeProperties.remove(CSSPropertyCustom);
+ oneKeyframeProperties.remove(CSSPropertyCustom);
for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
+ auto& keyframe = m_blendingKeyframes[i];
+ if (!keyframe.key()) {
+ for (auto cssPropertyId : keyframe.properties())
+ zeroKeyframeProperties.remove(cssPropertyId);
+ } else if (keyframe.key() == 1) {
+ for (auto cssPropertyId : keyframe.properties())
+ oneKeyframeProperties.remove(cssPropertyId);
+ }
+ }
+
+ for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
// 1. Initialize a dictionary object, output keyframe, using the following definition:
//
// dictionary BaseComputedKeyframe {
@@ -661,29 +677,45 @@
auto outputKeyframe = convertDictionaryToJS(lexicalGlobalObject, *jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject), computedKeyframe);
- // 3. For each animation property-value pair specified on keyframe, declaration, perform the following steps:
- auto isFirstOrLastKeyframe = !keyframe.key() || keyframe.key() == 1;
- auto& properties = isFirstOrLastKeyframe ? m_blendingKeyframes.properties() : keyframe.properties();
- auto& style = *keyframe.style();
- for (auto cssPropertyId : properties) {
- if (cssPropertyId == CSSPropertyCustom)
- continue;
+ auto addPropertyToKeyframe = [&](CSSPropertyID cssPropertyId, String idlValue) {
// 1. Let property name be the result of applying the animation property name to IDL attribute name algorithm to the property name of declaration.
auto propertyName = CSSPropertyIDToIDLAttributeName(cssPropertyId);
// 2. Let IDL value be the result of serializing the property value of declaration by passing declaration to the algorithm to serialize a CSS value.
- String idlValue = "";
- if (isFirstOrLastKeyframe && !keyframe.properties().contains(cssPropertyId) && lastStyleChangeEventStyle) {
- if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(*lastStyleChangeEventStyle, cssPropertyId, renderer))
- idlValue = cssValue->cssText();
- } else if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
- idlValue = cssValue->cssText();
// 3. Let value be the result of converting IDL value to an ECMAScript String value.
auto value = toJS<IDLDOMString>(lexicalGlobalObject, idlValue);
// 4. Call the [[DefineOwnProperty]] internal method on output keyframe with property name property name,
// Property Descriptor { [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true, [[Value]]: value } and Boolean flag false.
JSObject::defineOwnProperty(outputKeyframe, &lexicalGlobalObject, AtomString(propertyName).impl(), PropertyDescriptor(value, 0), false);
+ };
+
+ // 3. For each animation property-value pair specified on keyframe, declaration, perform the following steps:
+ auto& style = *keyframe.style();
+ for (auto cssPropertyId : keyframe.properties()) {
+ if (cssPropertyId == CSSPropertyCustom)
+ continue;
+ String idlValue = "";
+ if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
+ idlValue = cssValue->cssText();
+ addPropertyToKeyframe(cssPropertyId, idlValue);
}
+ // Now add the implicit properties in case there are any and we're dealing with a 0% or 100% keyframe.
+ if (lastStyleChangeEventStyle) {
+ if (!keyframe.key()) {
+ for (auto cssPropertyId : zeroKeyframeProperties) {
+ if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(*lastStyleChangeEventStyle, cssPropertyId, renderer))
+ addPropertyToKeyframe(cssPropertyId, cssValue->cssText());
+ }
+ zeroKeyframeProperties.clear();
+ } else if (keyframe.key() == 1) {
+ for (auto cssPropertyId : oneKeyframeProperties) {
+ if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(*lastStyleChangeEventStyle, cssPropertyId, renderer))
+ addPropertyToKeyframe(cssPropertyId, cssValue->cssText());
+ }
+ oneKeyframeProperties.clear();
+ }
+ }
+
// 5. Append output keyframe to result.
result.append(JSC::Strong<JSC::JSObject> { lexicalGlobalObject.vm(), outputKeyframe });
}
Modified: trunk/Source/WebCore/platform/animation/TimingFunction.h (287523 => 287524)
--- trunk/Source/WebCore/platform/animation/TimingFunction.h 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/Source/WebCore/platform/animation/TimingFunction.h 2022-01-02 23:42:27 UTC (rev 287524)
@@ -222,7 +222,20 @@
if (!is<StepsTimingFunction>(other))
return false;
auto& otherSteps = downcast<StepsTimingFunction>(other);
- return m_steps == otherSteps.m_steps && m_stepPosition == otherSteps.m_stepPosition;
+
+ if (m_steps != otherSteps.m_steps)
+ return false;
+
+ if (m_stepPosition == otherSteps.m_stepPosition)
+ return true;
+
+ if (!m_stepPosition && *otherSteps.m_stepPosition == StepPosition::End)
+ return true;
+
+ if (*m_stepPosition == StepPosition::End && !otherSteps.m_stepPosition)
+ return true;
+
+ return false;
}
int numberOfSteps() const { return m_steps; }
Modified: trunk/Source/WebCore/style/StyleResolver.cpp (287523 => 287524)
--- trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-02 21:37:07 UTC (rev 287523)
+++ trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-02 23:42:27 UTC (rev 287524)
@@ -321,46 +321,61 @@
if (it == m_keyframesRuleMap.end())
return;
+ auto timingFunctionForKeyframe = [](Ref<StyleRuleKeyframe> keyframe) -> RefPtr<const TimingFunction> {
+ if (auto timingFunctionCSSValue = keyframe->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction)) {
+ if (auto timingFunction = TimingFunction::createFromCSSValue(*timingFunctionCSSValue))
+ return timingFunction;
+ }
+ return &CubicBezierTimingFunction::defaultTimingFunction();
+ };
+
+ HashSet<RefPtr<const TimingFunction>> timingFunctions;
+ auto uniqueTimingFunctionForKeyframe = [&](Ref<StyleRuleKeyframe> keyframe) -> RefPtr<const TimingFunction> {
+ auto timingFunction = timingFunctionForKeyframe(keyframe);
+ for (auto existingTimingFunction : timingFunctions) {
+ if (arePointingToEqualData(timingFunction, existingTimingFunction))
+ return existingTimingFunction;
+ }
+ timingFunctions.add(timingFunction);
+ return timingFunction;
+ };
+
const StyleRuleKeyframes* keyframesRule = it->value.get();
-
auto* keyframes = &keyframesRule->keyframes();
- Vector<Ref<StyleRuleKeyframe>> newKeyframesIfNecessary;
- bool hasDuplicateKeys = false;
- HashSet<double> keyframeKeys;
- for (auto& keyframe : *keyframes) {
- for (auto key : keyframe->keys()) {
- if (!keyframeKeys.add(key)) {
- hasDuplicateKeys = true;
- break;
+ using KeyframeUniqueKey = std::pair<double, RefPtr<const TimingFunction>>;
+ auto hasDuplicateKeys = [&]() -> bool {
+ HashSet<KeyframeUniqueKey> uniqueKeyframeKeys;
+ for (auto& keyframe : *keyframes) {
+ auto timingFunction = uniqueTimingFunctionForKeyframe(keyframe);
+ for (auto key : keyframe->keys()) {
+ if (!uniqueKeyframeKeys.add({ key, timingFunction }))
+ return true;
}
}
- if (hasDuplicateKeys)
- break;
- }
+ return false;
+ }();
// FIXME: If HashMaps could have Ref<> as value types, we wouldn't need
// to copy the HashMap into a Vector.
+ Vector<Ref<StyleRuleKeyframe>> newKeyframesIfNecessary;
if (hasDuplicateKeys) {
- // Merge duplicate key times.
- HashMap<double, RefPtr<StyleRuleKeyframe>> keyframesMap;
-
- for (auto& originalKeyframe : keyframesRule->keyframes()) {
+ // Merge keyframes with a similar offset and timing function.
+ HashMap<KeyframeUniqueKey, RefPtr<StyleRuleKeyframe>> keyframesMap;
+ for (auto& originalKeyframe : *keyframes) {
+ auto timingFunction = uniqueTimingFunctionForKeyframe(originalKeyframe);
for (auto key : originalKeyframe->keys()) {
- if (auto keyframe = keyframesMap.get(key))
+ if (auto keyframe = keyframesMap.get({ key, timingFunction }))
keyframe->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
else {
- auto StyleRuleKeyframe = StyleRuleKeyframe::create(MutableStyleProperties::create());
- StyleRuleKeyframe.ptr()->setKey(key);
- StyleRuleKeyframe.ptr()->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
- keyframesMap.set(key, StyleRuleKeyframe.ptr());
+ auto styleRuleKeyframe = StyleRuleKeyframe::create(MutableStyleProperties::create());
+ styleRuleKeyframe.ptr()->setKey(key);
+ styleRuleKeyframe.ptr()->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
+ keyframesMap.set({ key, timingFunction }, styleRuleKeyframe.ptr());
+ newKeyframesIfNecessary.append(styleRuleKeyframe);
}
}
}
-
- for (auto& keyframe : keyframesMap.values())
- newKeyframesIfNecessary.append(*keyframe.get());
-
keyframes = &newKeyframesIfNecessary;
}
