[web-animations] keyframes should be recomputed if used CSS variable …

…changes https://bugs.webkit.org/show_bug.cgi?id=248145 Reviewed by Antti Koivisto. When the value for a CSS variable changes, we must ensure that any set of keyframes that use that CSS variable are recomputed, whether the animation is a CSS Animation of a script-originated animation. This does not apply to CSS Transitions which would operate on resolved values in RenderStyle. To do this we add a StyleRuleKeyframe::containsCSSVariableReferences() method which indicates whether a keyframe rule contains CSS variables. Then, we add a similar method on KeyframeEffect returning the boolean flag computed resolving keyframes in KeyframeEffect::computeCSSAnimationBlendingKeyframes(), for the CSS Animations case, and KeyframeEffect::updateBlendingKeyframes(), for the script-originated animation case. Then in KeyframeEffectStack::applyKeyframeEffects(), much like we do for detecting changes made to font-size, we check whether any CSS variable (or custom property in WebCore parlance) has changed and recompute keyframes if that is the case. We now pass the final two subtests in web-animations/animation-model/keyframe-effects/effect-value-context-filling.html and since those tests only test the script-originated animation case, we also add a new test in css/css-animations to test the CSS Animations case. * LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted.html: Added. * LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/keyframe-effects/effect-value-context-filling-expected.txt: * Source/WebCore/animation/KeyframeEffect.cpp: (WebCore::KeyframeEffect::updateBlendingKeyframes): (WebCore::KeyframeEffect::computeCSSAnimationBlendingKeyframes): * Source/WebCore/animation/KeyframeEffect.h: (WebCore::KeyframeEffect::containsCSSVariableReferences const): * Source/WebCore/animation/KeyframeEffectStack.cpp: (WebCore::KeyframeEffectStack::applyKeyframeEffects): * Source/WebCore/css/CSSKeyframeRule.cpp: (WebCore::StyleRuleKeyframe::containsCSSVariableReferences const): * Source/WebCore/css/CSSKeyframeRule.h: * Source/WebCore/style/StyleResolver.cpp: (WebCore::Style::Resolver::keyframeStylesForAnimation): * Source/WebCore/style/StyleResolver.h: Canonical link: https://commits.webkit.org/256893@main
WebKit · Nov 20, 2022 · 5c0b3cb4b2450bcc60e21f512306aaaa56e3e9b3 · 5c0b3cb
1 parent 8090392
commit 5c0b3cb4b2450bcc60e21f512306aaaa56e3e9b3
Show file tree

Hide file tree

Showing 10 changed files with 114 additions and 6 deletions.
diff --git a/...latform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted-expected.txt b/...latform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted-expected.txt
@@ -0,0 +1,4 @@
+
+PASS Animations reflect changes to variables on element
+PASS Animations reflect changes to variables on parent element
+
diff --git a/...3c/web-platform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted.html b/...3c/web-platform-tests/css/css-animations/animation-css-variable-in-keyframe-adjusted.html
@@ -0,0 +1,68 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Animations: adjust value of CSS variable used in keyframes</title>
+<link rel="help" href="https://drafts.csswg.org/css-animations/">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="support/testcommon.js"></script>
+<style>
+
+@keyframes anim {
+    from { margin-left: var(--margin-left) }
+    to   { margin-left: calc(var(--margin-left) * 2) }
+}
+
+</style>
+<div id="log"></div>
+<script>
+
+test(t => {
+  const div = addDiv(t);
+  div.style.setProperty('--margin-left', '100px');
+
+  div.style.animation = 'anim 1s linear';
+  const animation = div.getAnimations()[0];
+  animation.currentTime = 500;
+
+  assert_equals(
+    getComputedStyle(div).marginLeft,
+    '150px',
+    'Animated value before updating variable'
+  );
+
+  div.style.setProperty('--margin-left', '200px');
+
+  assert_equals(
+    getComputedStyle(div).marginLeft,
+    '300px',
+    'Animated value after updating variable'
+  );
+}, 'Animations reflect changes to variables on element');
+
+test(t => {
+  const parentDiv = addDiv(t);
+  const div = addDiv(t);
+  parentDiv.appendChild(div);
+  parentDiv.style.setProperty('--margin-left', '100px');
+
+  div.style.animation = 'anim 1s linear';
+  const animation = div.getAnimations()[0];
+  animation.currentTime = 500;
+
+  assert_equals(
+    getComputedStyle(div).marginLeft,
+    '150px',
+    'Animated value before updating variable'
+  );
+
+  parentDiv.style.setProperty('--margin-left', '200px');
+
+  assert_equals(
+    getComputedStyle(div).marginLeft,
+    '300px',
+    'Animated value after updating variable'
+  );
+}, 'Animations reflect changes to variables on parent element');
+
+
+</script>
diff --git a/...web-animations/animation-model/keyframe-effects/effect-value-context-filling-expected.txt b/...web-animations/animation-model/keyframe-effects/effect-value-context-filling-expected.txt
@@ -1,8 +1,8 @@
 
 PASS Filling effect values reflect changes to font-size on element
 PASS Filling effect values reflect changes to font-size on parent element
-FAIL Filling effect values reflect changes to variables on element assert_equals: Effect value after updating variable expected "200px" but got "100px"
-FAIL Filling effect values reflect changes to variables on parent element assert_equals: Effect value after updating variable expected "400px" but got "200px"
+PASS Filling effect values reflect changes to variables on element
+PASS Filling effect values reflect changes to variables on parent element
 PASS Filling effect values reflect changes to the the animation's keyframes
 PASS Filling effect values reflect changes to the the animation's composite mode
 PASS Filling effect values reflect changes to the the animation's iteration composite mode

diff --git a/Source/WebCore/animation/KeyframeEffect.cpp b/Source/WebCore/animation/KeyframeEffect.cpp
@@ -873,6 +873,8 @@ void KeyframeEffect::updateBlendingKeyframes(RenderStyle& elementStyle, const St
     KeyframeList keyframeList(m_keyframesName);
     auto& styleResolver = m_target->styleResolver();
 
+    m_containsCSSVariableReferences = false;
+
     for (auto& keyframe : m_parsedKeyframes) {
         KeyframeValue keyframeValue(keyframe.computedOffset, nullptr);
         keyframeValue.setTimingFunction(keyframe.timingFunction->clone());
@@ -892,6 +894,8 @@ void KeyframeEffect::updateBlendingKeyframes(RenderStyle& elementStyle, const St
         }
 
         auto keyframeRule = StyleRuleKeyframe::create(keyframe.style->immutableCopyIfNeeded());
+        if (!m_containsCSSVariableReferences)
+            m_containsCSSVariableReferences = keyframeRule->containsCSSVariableReferences();
         keyframeValue.setStyle(styleResolver.styleForKeyframe(*m_target, elementStyle, resolutionContext, keyframeRule.get(), keyframeValue));
         keyframeList.insert(WTFMove(keyframeValue));
     }
@@ -1101,7 +1105,7 @@ void KeyframeEffect::computeCSSAnimationBlendingKeyframes(const RenderStyle& una
 
     KeyframeList keyframeList(AtomString { backingAnimation.name().string });
     if (auto* styleScope = Style::Scope::forOrdinal(*m_target, backingAnimation.nameStyleScopeOrdinal()))
-        styleScope->resolver().keyframeStylesForAnimation(*m_target, unanimatedStyle, resolutionContext, keyframeList);
+        styleScope->resolver().keyframeStylesForAnimation(*m_target, unanimatedStyle, resolutionContext, keyframeList, m_containsCSSVariableReferences);
 
     // Ensure resource loads for all the frames.
     for (auto& keyframe : keyframeList) {

diff --git a/Source/WebCore/animation/KeyframeEffect.h b/Source/WebCore/animation/KeyframeEffect.h
@@ -189,6 +189,8 @@ class KeyframeEffect : public AnimationEffect
 
     static String CSSPropertyIDToIDLAttributeName(CSSPropertyID);
 
+    bool containsCSSVariableReferences() const { return m_containsCSSVariableReferences; }
+
 private:
     KeyframeEffect(Element*, PseudoId);
 
@@ -282,6 +284,7 @@ class KeyframeEffect : public AnimationEffect
     bool m_someKeyframesUseStepsTimingFunction { false };
     bool m_hasImplicitKeyframeForAcceleratedProperty { false };
     bool m_hasKeyframeComposingAcceleratedProperty { false };
+    bool m_containsCSSVariableReferences { false };
 };
 
 } // namespace WebCore

diff --git a/Source/WebCore/animation/KeyframeEffectStack.cpp b/Source/WebCore/animation/KeyframeEffectStack.cpp
@@ -171,8 +171,21 @@ OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle
             return false;
         };
 
+        auto cssVariableChanged = [&]() {
+            auto customPropertyValueMapDidChange = [](const CustomPropertyValueMap& a, const CustomPropertyValueMap& b) {
+                return &a != &b && a != b;
+            };
+
+            if (previousLastStyleChangeEventStyle && effect->containsCSSVariableReferences()) {
+                if (customPropertyValueMapDidChange(previousLastStyleChangeEventStyle->inheritedCustomProperties(), targetStyle.inheritedCustomProperties())
+                    || customPropertyValueMapDidChange(previousLastStyleChangeEventStyle->nonInheritedCustomProperties(), targetStyle.nonInheritedCustomProperties()))
+                    return true;
+            }
+            return false;
+        };
+
         auto logicalPropertyDidChange = propertyAffectingLogicalPropertiesChanged && effect->animatesDirectionAwareProperty();
-        if (logicalPropertyDidChange || fontSizeChanged || inheritedPropertyChanged())
+        if (logicalPropertyDidChange || fontSizeChanged || inheritedPropertyChanged() || cssVariableChanged())
             effect->propertyAffectingKeyframeResolutionDidChange(unanimatedStyle, resolutionContext);
 
         animation->resolve(targetStyle, resolutionContext);

diff --git a/Source/WebCore/css/CSSKeyframeRule.cpp b/Source/WebCore/css/CSSKeyframeRule.cpp
@@ -94,6 +94,17 @@ String StyleRuleKeyframe::cssText() const
     return makeString(keyText(), " { }");
 }
 
+bool StyleRuleKeyframe::containsCSSVariableReferences() const
+{
+    for (unsigned i = 0; i < m_properties->propertyCount(); ++i) {
+        if (auto* cssValue = m_properties->propertyAt(i).value()) {
+            if (cssValue->hasVariableReferences())
+                return true;
+        }
+    }
+    return false;
+}
+
 CSSKeyframeRule::CSSKeyframeRule(StyleRuleKeyframe& keyframe, CSSKeyframesRule* parent)
     : CSSRule(nullptr)
     , m_keyframe(keyframe)

diff --git a/Source/WebCore/css/CSSKeyframeRule.h b/Source/WebCore/css/CSSKeyframeRule.h
@@ -59,6 +59,8 @@ class StyleRuleKeyframe final : public StyleRuleBase {
 
     String cssText() const;
 
+    bool containsCSSVariableReferences() const;
+
 private:
     explicit StyleRuleKeyframe(Ref<StyleProperties>&&);
     StyleRuleKeyframe(Vector<double>&&, Ref<StyleProperties>&&);

diff --git a/Source/WebCore/style/StyleResolver.cpp b/Source/WebCore/style/StyleResolver.cpp
@@ -412,21 +412,24 @@ Vector<Ref<StyleRuleKeyframe>> Resolver::keyframeRulesForName(const AtomString&
     return deduplicatedKeyframes;
 }
 
-void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle& elementStyle, const ResolutionContext& context, KeyframeList& list)
+void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle& elementStyle, const ResolutionContext& context, KeyframeList& list, bool& containsCSSVariableReferences)
 {
     list.clear();
 
     auto keyframeRules = keyframeRulesForName(list.animationName());
     if (keyframeRules.isEmpty())
         return;
 
+    containsCSSVariableReferences = false;
     // Construct and populate the style for each keyframe.
     for (auto& keyframeRule : keyframeRules) {
         // Add this keyframe style to all the indicated key times
         for (auto key : keyframeRule->keys()) {
             KeyframeValue keyframeValue(0, nullptr);
             keyframeValue.setStyle(styleForKeyframe(element, elementStyle, context, keyframeRule.get(), keyframeValue));
             keyframeValue.setKey(key);
+            if (!containsCSSVariableReferences)
+                containsCSSVariableReferences = keyframeRule->containsCSSVariableReferences();
             if (auto timingFunctionCSSValue = keyframeRule->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction))
                 keyframeValue.setTimingFunction(TimingFunction::createFromCSSValue(*timingFunctionCSSValue.get()));
             if (auto compositeOperationCSSValue = keyframeRule->properties().getPropertyCSSValue(CSSPropertyAnimationComposition)) {

diff --git a/Source/WebCore/style/StyleResolver.h b/Source/WebCore/style/StyleResolver.h
@@ -98,7 +98,7 @@ class Resolver : public RefCounted<Resolver> {
 
     ElementStyle styleForElement(const Element&, const ResolutionContext&, RuleMatchingBehavior = RuleMatchingBehavior::MatchAllRules);
 
-    void keyframeStylesForAnimation(const Element&, const RenderStyle& elementStyle, const ResolutionContext&, KeyframeList&);
+    void keyframeStylesForAnimation(const Element&, const RenderStyle& elementStyle, const ResolutionContext&, KeyframeList&, bool& containsCSSVariableReferences);
 
     WEBCORE_EXPORT std::unique_ptr<RenderStyle> pseudoStyleForElement(const Element&, const PseudoElementRequest&, const ResolutionContext&);