- Revision
- 287820
- Author
- grao...@webkit.org
- Date
- 2022-01-09 07:35:10 -0800 (Sun, 09 Jan 2022)
Log Message
[Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
https://bugs.webkit.org/show_bug.cgi?id=235008
Reviewed by Antti Koivisto.
LayoutTests/imported/w3c:
Mark WPT progression.
* web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:
Source/WebCore:
Until now, when calling getKeyframes() on a keyframe effect associated with a CSS Animation, we would
serialize for each keyframe the computed style for the animated properties. However, the Web Aniations
spec says (https://drafts.csswg.org/web-animations-1/#dom-keyframeeffect-getkeyframes) that the value
to use should "be the result of serializing the property value of declaration by passing declaration
to the algorithm to serialize a CSS value [CSSOM]."
One way we can get to the value as specified on the @keyframes rule rather than the computed style
is to access the StyleRuleKeyframe object directly as stored by the Style::Resolver. This keeps the
raw CSSValues as parsed.
To do this, we first refactor Style::Resolver::keyframeStylesForAnimation() such that all the code
that creates a Vector<Ref<StyleRuleKeyframe>> with deduplicated keyframe rules is in the new dedicated
method Style::Resolver::keyframeRulesForName().
Then, in KeyframeEffect::getKeyframes(), we can call into this function to obtain the set of keyframe
rules for the associated CSS Animation's name and read the style from there. Note that these values
may contain unsubstituted --var() values, so in that case for now we still use the computed style.
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::getKeyframes):
* style/StyleResolver.cpp:
(WebCore::Style::Resolver::keyframeRulesForName):
(WebCore::Style::Resolver::keyframeStylesForAnimation):
* style/StyleResolver.h:
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (287819 => 287820)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-09 15:35:10 UTC (rev 287820)
@@ -1,3 +1,14 @@
+2022-01-08 Antoine Quint <grao...@webkit.org>
+
+ [Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
+ https://bugs.webkit.org/show_bug.cgi?id=235008
+
+ Reviewed by Antti Koivisto.
+
+ Mark WPT progression.
+
+ * web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:
+
2022-01-08 Simon Fraser <simon.fra...@apple.com>
If the drop-shadow filter has no color, it should use the value of the color property
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt (287819 => 287820)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt 2022-01-09 15:35:10 UTC (rev 287820)
@@ -15,13 +15,13 @@
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)"
+PASS KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes
PASS KeyframeEffect.getKeyframes() returns expected values for animation with drop-shadow of filter property
FAIL KeyframeEffect.getKeyframes() returns expected values for animations with text-shadow properties and missing keyframes assert_equals: value for 'textShadow' on Keyframe #0 should match expected "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0, 0, 255) 0px 0px 3.2px" but got "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0, 0, 255) 0px 0px 3.200000047683716px"
-FAIL KeyframeEffect.getKeyframes() returns expected values for animations with background-size properties and missing keyframes assert_equals: value for 'backgroundSize' on ComputedKeyframe #1 should match expected "50%, 6px, contain" but got "50%"
+FAIL KeyframeEffect.getKeyframes() returns expected values for animations with background-size properties and missing keyframes assert_equals: value for 'backgroundSize' on ComputedKeyframe #0 after updating current style should match expected "30px, 40%, auto" but got "30px"
FAIL KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values assert_equals: value for 'transform' on Keyframe #1 should match expected "translate(100px)" but got "matrix(1, 0, 0, 1, 100, 0)"
PASS KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values in a shorthand property
PASS KeyframeEffect.getKeyframes() returns expected values for animations with a CSS variable which is overriden by the value in keyframe
-FAIL KeyframeEffect.getKeyframes() returns expected values for animations with only custom property in a keyframe assert_equals: value for 'transform' on Keyframe #0 should match expected "translate(100px)" but got "matrix(1, 0, 0, 1, 100, 0)"
+FAIL KeyframeEffect.getKeyframes() returns expected values for animations with only custom property in a keyframe assert_equals: value for 'transform' on Keyframe #0 should match expected "translate(100px)" but got "translate(100px, 0px)"
PASS KeyframeEffect.getKeyframes() reflects changes to @keyframes rules
Modified: trunk/Source/WebCore/ChangeLog (287819 => 287820)
--- trunk/Source/WebCore/ChangeLog 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/Source/WebCore/ChangeLog 2022-01-09 15:35:10 UTC (rev 287820)
@@ -1,3 +1,35 @@
+2022-01-08 Antoine Quint <grao...@webkit.org>
+
+ [Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
+ https://bugs.webkit.org/show_bug.cgi?id=235008
+
+ Reviewed by Antti Koivisto.
+
+ Until now, when calling getKeyframes() on a keyframe effect associated with a CSS Animation, we would
+ serialize for each keyframe the computed style for the animated properties. However, the Web Aniations
+ spec says (https://drafts.csswg.org/web-animations-1/#dom-keyframeeffect-getkeyframes) that the value
+ to use should "be the result of serializing the property value of declaration by passing declaration
+ to the algorithm to serialize a CSS value [CSSOM]."
+
+ One way we can get to the value as specified on the @keyframes rule rather than the computed style
+ is to access the StyleRuleKeyframe object directly as stored by the Style::Resolver. This keeps the
+ raw CSSValues as parsed.
+
+ To do this, we first refactor Style::Resolver::keyframeStylesForAnimation() such that all the code
+ that creates a Vector<Ref<StyleRuleKeyframe>> with deduplicated keyframe rules is in the new dedicated
+ method Style::Resolver::keyframeRulesForName().
+
+ Then, in KeyframeEffect::getKeyframes(), we can call into this function to obtain the set of keyframe
+ rules for the associated CSS Animation's name and read the style from there. Note that these values
+ may contain unsubstituted --var() values, so in that case for now we still use the computed style.
+
+ * animation/KeyframeEffect.cpp:
+ (WebCore::KeyframeEffect::getKeyframes):
+ * style/StyleResolver.cpp:
+ (WebCore::Style::Resolver::keyframeRulesForName):
+ (WebCore::Style::Resolver::keyframeStylesForAnimation):
+ * style/StyleResolver.h:
+
2022-01-09 Alan Bujtas <za...@apple.com>
[LFC][IFC] Line::Run needs access to FontCascade
Modified: trunk/Source/WebCore/animation/KeyframeEffect.cpp (287819 => 287820)
--- trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/Source/WebCore/animation/KeyframeEffect.cpp 2022-01-09 15:35:10 UTC (rev 287820)
@@ -639,6 +639,28 @@
auto computedStyleExtractor = ComputedStyleExtractor(target, false, m_pseudoId);
+ auto keyframeRules = [&]() -> const Vector<Ref<StyleRuleKeyframe>> {
+ if (!is<CSSAnimation>(animation()))
+ return { };
+
+ auto& backingAnimation = downcast<CSSAnimation>(*animation()).backingAnimation();
+ auto* styleScope = Style::Scope::forOrdinal(*m_target, backingAnimation.nameStyleScopeOrdinal());
+ if (!styleScope)
+ return { };
+
+ return styleScope->resolver().keyframeRulesForName(m_blendingKeyframes.animationName());
+ }();
+
+ auto keyframeRuleForKey = [&](double key) -> StyleRuleKeyframe* {
+ for (auto& keyframeRule : keyframeRules) {
+ for (auto keyframeRuleKey : keyframeRule->keys()) {
+ if (keyframeRuleKey == key)
+ return keyframeRule.ptr();
+ }
+ }
+ return nullptr;
+ };
+
// We need to establish which properties are implicit for 0% and 100%.
HashSet<CSSPropertyID> zeroKeyframeProperties = m_blendingKeyframes.properties();
HashSet<CSSPropertyID> _oneKeyframeProperties_ = m_blendingKeyframes.properties();
@@ -690,12 +712,21 @@
// 3. For each animation property-value pair specified on keyframe, declaration, perform the following steps:
auto& style = *keyframe.style();
+ auto* keyframeRule = keyframeRuleForKey(keyframe.key());
for (auto cssPropertyId : keyframe.properties()) {
if (cssPropertyId == CSSPropertyCustom)
continue;
String idlValue = "";
- if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
- idlValue = cssValue->cssText();
+ if (keyframeRule) {
+ if (auto cssValue = keyframeRule->properties().getPropertyCSSValue(cssPropertyId)) {
+ if (!cssValue->hasVariableReferences())
+ idlValue = keyframeRule->properties().getPropertyValue(cssPropertyId);
+ }
+ }
+ if (idlValue.isEmpty()) {
+ if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
+ idlValue = cssValue->cssText();
+ }
addPropertyToKeyframe(cssPropertyId, idlValue);
}
Modified: trunk/Source/WebCore/style/StyleResolver.cpp (287819 => 287820)
--- trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/Source/WebCore/style/StyleResolver.cpp 2022-01-09 15:35:10 UTC (rev 287820)
@@ -307,19 +307,16 @@
return m_keyframesRuleMap.find(AtomString(name).impl()) != m_keyframesRuleMap.end();
}
-void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, const ResolutionContext& context, KeyframeList& list)
+const Vector<Ref<StyleRuleKeyframe>> Resolver::keyframeRulesForName(const AtomString& animationName)
{
- list.clear();
+ if (animationName.isEmpty())
+ return { };
- // Get the keyframesRule for this name.
- if (list.animationName().isEmpty())
- return;
-
m_keyframesRuleMap.checkConsistency();
- KeyframesRuleMap::iterator it = m_keyframesRuleMap.find(list.animationName().impl());
+ auto it = m_keyframesRuleMap.find(animationName.impl());
if (it == m_keyframesRuleMap.end())
- return;
+ return { };
auto timingFunctionForKeyframe = [](Ref<StyleRuleKeyframe> keyframe) -> RefPtr<const TimingFunction> {
if (auto timingFunctionCSSValue = keyframe->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction)) {
@@ -340,7 +337,7 @@
return timingFunction;
};
- const StyleRuleKeyframes* keyframesRule = it->value.get();
+ auto* keyframesRule = it->value.get();
auto* keyframes = &keyframesRule->keyframes();
using KeyframeUniqueKey = std::pair<double, RefPtr<const TimingFunction>>;
@@ -356,37 +353,48 @@
return false;
}();
+ if (!hasDuplicateKeys)
+ return *keyframes;
+
// 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 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, 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, timingFunction }, styleRuleKeyframe.ptr());
- newKeyframesIfNecessary.append(styleRuleKeyframe);
- }
+ Vector<Ref<StyleRuleKeyframe>> deduplicatedKeyframes;
+ // 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, 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, timingFunction }, styleRuleKeyframe.ptr());
+ deduplicatedKeyframes.append(styleRuleKeyframe);
}
}
- keyframes = &newKeyframesIfNecessary;
}
+ return deduplicatedKeyframes;
+}
+
+void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, const ResolutionContext& context, KeyframeList& list)
+{
+ list.clear();
+
+ auto keyframeRules = keyframeRulesForName(list.animationName());
+ if (keyframeRules.isEmpty())
+ return;
+
// Construct and populate the style for each keyframe.
- for (auto& keyframe : *keyframes) {
+ for (auto& keyframeRule : keyframeRules) {
// Add this keyframe style to all the indicated key times
- for (auto key : keyframe->keys()) {
+ for (auto key : keyframeRule->keys()) {
KeyframeValue keyframeValue(0, nullptr);
- keyframeValue.setStyle(styleForKeyframe(element, elementStyle, context, keyframe.ptr(), keyframeValue));
+ keyframeValue.setStyle(styleForKeyframe(element, elementStyle, context, keyframeRule.ptr(), keyframeValue));
keyframeValue.setKey(key);
- if (auto timingFunctionCSSValue = keyframe->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction))
+ if (auto timingFunctionCSSValue = keyframeRule->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction))
keyframeValue.setTimingFunction(TimingFunction::createFromCSSValue(*timingFunctionCSSValue.get()));
list.insert(WTFMove(keyframeValue));
}
Modified: trunk/Source/WebCore/style/StyleResolver.h (287819 => 287820)
--- trunk/Source/WebCore/style/StyleResolver.h 2022-01-09 13:50:27 UTC (rev 287819)
+++ trunk/Source/WebCore/style/StyleResolver.h 2022-01-09 15:35:10 UTC (rev 287820)
@@ -139,6 +139,7 @@
std::optional<DynamicMediaQueryEvaluationChanges> evaluateDynamicMediaQueries();
void addKeyframeStyle(Ref<StyleRuleKeyframes>&&);
+ const Vector<Ref<StyleRuleKeyframe>> keyframeRulesForName(const AtomString&);
bool usesFirstLineRules() const { return m_ruleSets.features().usesFirstLineRules; }
bool usesFirstLetterRules() const { return m_ruleSets.features().usesFirstLetterRules; }
