From 36c3b023dd016e4890e542db6181758d874cab47 Mon Sep 17 00:00:00 2001 From: Antoine Quint Date: Fri, 3 Feb 2023 01:39:54 -0800 Subject: [PATCH] Cherry-pick b1a5811b3017. rdar://problem/104997994 [web-animations] keyframes should be recomputed when the "currentcolor" value is used on a custom property https://bugs.webkit.org/show_bug.cgi?id=251591 Reviewed by Antti Koivisto. We keep a set of CSS properties set to "currentcolor" in a KeyframeEffect but until now only considered "standard" CSS properties. We now also consider custom properties by changing the set's type from HashSet to HashSet. * LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation.html: * Source/WebCore/animation/KeyframeEffect.cpp: (WebCore::KeyframeEffect::updateBlendingKeyframes): * Source/WebCore/animation/KeyframeEffect.h: * Source/WebCore/css/CSSCustomPropertyValue.cpp: (WebCore::CSSCustomPropertyValue::isCurrentColor const): * Source/WebCore/css/CSSCustomPropertyValue.h: * Source/WebCore/style/StyleResolver.cpp: (WebCore::Style::Resolver::keyframeStylesForAnimation): * Source/WebCore/style/StyleResolver.h: Canonical link: https://commits.webkit.org/259808@main Canonical link: https://commits.webkit.org/259548.124@safari-7615-branch --- .../at-property-animation-expected.txt | 2 + .../at-property-animation.html | 55 +++++++++++++++++++ Source/WebCore/animation/KeyframeEffect.cpp | 4 ++ Source/WebCore/animation/KeyframeEffect.h | 2 +- Source/WebCore/css/CSSCustomPropertyValue.cpp | 8 +++ Source/WebCore/css/CSSCustomPropertyValue.h | 1 + Source/WebCore/style/StyleResolver.cpp | 6 +- Source/WebCore/style/StyleResolver.h | 2 +- 8 files changed, 77 insertions(+), 3 deletions(-) diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt index b065ddd63398..ab7c252ec09e 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt +++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt @@ -13,4 +13,6 @@ PASS No transition when changing types PASS No transition when removing @property rule PASS Unregistered properties referencing animated properties update correctly. PASS Registered properties referencing animated properties update correctly. +PASS CSS animation setting "currentColor" for a custom property on a keyframe is responsive to changing "color" on the parent. +PASS JS-originated animation setting "currentColor" for a custom property on a keyframe is responsive to changing "color" on the parent. diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation.html index 6d467ab2b992..e891802bbec4 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation.html +++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation.html @@ -338,4 +338,59 @@ }); }, 'Registered properties referencing animated properties update correctly.'); +test_with_at_property({ + syntax: '""', + inherits: false, + initialValue: 'black' +}, (name) => { + with_style_node(` + @keyframes test { + from { ${name}: currentcolor; } + to { ${name}: rgb(200, 200, 200); } + } + #outer { + color: rgb(100, 100, 100); + } + #div { + animation: test 100s -50s linear paused; + } + `, (style) => { + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(150, 150, 150)'); + + outer.style.color = 'rgb(50, 50, 50)'; + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(125, 125, 125)'); + + outer.style.color = 'rgb(150, 150, 150)'; + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(175, 175, 175)'); + + outer.style.removeProperty("color"); + }); +}, 'CSS animation setting "currentColor" for a custom property on a keyframe is responsive to changing "color" on the parent.'); + +test_with_at_property({ + syntax: '""', + inherits: false, + initialValue: 'black' +}, (name) => { + with_style_node(` + #outer { + color: rgb(100, 100, 100); + } + `, () => { + const animation = div.animate({ [name]: ["currentcolor", "rgb(200, 200, 200)"]}, 1000); + animation.currentTime = 500; + animation.pause(); + + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(150, 150, 150)'); + + outer.style.color = 'rgb(50, 50, 50)'; + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(125, 125, 125)'); + + outer.style.color = 'rgb(150, 150, 150)'; + assert_equals(getComputedStyle(div).getPropertyValue(name), 'rgb(175, 175, 175)'); + + outer.style.removeProperty("color"); + }); +}, 'JS-originated animation setting "currentColor" for a custom property on a keyframe is responsive to changing "color" on the parent.'); + diff --git a/Source/WebCore/animation/KeyframeEffect.cpp b/Source/WebCore/animation/KeyframeEffect.cpp index 530c9308780c..00ee9c100a20 100644 --- a/Source/WebCore/animation/KeyframeEffect.cpp +++ b/Source/WebCore/animation/KeyframeEffect.cpp @@ -28,6 +28,7 @@ #include "Animation.h" #include "CSSAnimation.h" +#include "CSSCustomPropertyValue.h" #include "CSSKeyframeRule.h" #include "CSSPrimitiveValue.h" #include "CSSPropertyAnimation.h" @@ -919,6 +920,9 @@ void KeyframeEffect::updateBlendingKeyframes(RenderStyle& elementStyle, const St m_currentColorProperties.add(property.id()); else if (property.id() == CSSPropertyFontWeight && (valueId == CSSValueBolder || valueId == CSSValueLighter)) m_hasRelativeFontWeight = true; + } else if (auto* customPropertyValue = dynamicDowncast(cssValue)) { + if (customPropertyValue->isCurrentColor()) + m_currentColorProperties.add(customPropertyValue->name()); } } } diff --git a/Source/WebCore/animation/KeyframeEffect.h b/Source/WebCore/animation/KeyframeEffect.h index cc20f5ed1506..ec0317c1323a 100644 --- a/Source/WebCore/animation/KeyframeEffect.h +++ b/Source/WebCore/animation/KeyframeEffect.h @@ -246,7 +246,7 @@ class KeyframeEffect : public AnimationEffect, public CSSPropertyBlendingClient KeyframeList m_blendingKeyframes { emptyAtom() }; HashSet m_animatedProperties; HashSet m_inheritedProperties; - HashSet m_currentColorProperties; + HashSet m_currentColorProperties; Vector m_parsedKeyframes; Vector m_pendingAcceleratedActions; RefPtr m_target; diff --git a/Source/WebCore/css/CSSCustomPropertyValue.cpp b/Source/WebCore/css/CSSCustomPropertyValue.cpp index 7d6e3c6ffea9..8f5adaedc34a 100644 --- a/Source/WebCore/css/CSSCustomPropertyValue.cpp +++ b/Source/WebCore/css/CSSCustomPropertyValue.cpp @@ -152,4 +152,12 @@ Ref CSSCustomPropertyValue::asVariableData() const }); } +bool CSSCustomPropertyValue::isCurrentColor() const +{ + // FIXME: it's surprising that setting a custom property to "currentcolor" + // results in m_value being a CSSVariableReferenceValue rather than a + // CSSValueID set to CSSValueCurrentcolor or a StyleValue. + return std::holds_alternative>(m_value) && std::get>(m_value)->customCSSText() == "currentcolor"_s; +} + } diff --git a/Source/WebCore/css/CSSCustomPropertyValue.h b/Source/WebCore/css/CSSCustomPropertyValue.h index 63aea9a690c0..8fa28a1004b7 100644 --- a/Source/WebCore/css/CSSCustomPropertyValue.h +++ b/Source/WebCore/css/CSSCustomPropertyValue.h @@ -107,6 +107,7 @@ class CSSCustomPropertyValue final : public CSSValue { bool isResolved() const { return !std::holds_alternative>(m_value); } bool isUnset() const { return std::holds_alternative(m_value) && std::get(m_value) == CSSValueUnset; } bool isInvalid() const { return std::holds_alternative(m_value) && std::get(m_value) == CSSValueInvalid; } + bool isCurrentColor() const; bool containsCSSWideKeyword() const; const VariantValue& value() const { return m_value; } diff --git a/Source/WebCore/style/StyleResolver.cpp b/Source/WebCore/style/StyleResolver.cpp index 5c807633e92e..0e83443116f5 100644 --- a/Source/WebCore/style/StyleResolver.cpp +++ b/Source/WebCore/style/StyleResolver.cpp @@ -30,6 +30,7 @@ #include "config.h" #include "StyleResolver.h" +#include "CSSCustomPropertyValue.h" #include "CSSFontSelector.h" #include "CSSKeyframeRule.h" #include "CSSKeyframesRule.h" @@ -430,7 +431,7 @@ Vector> Resolver::keyframeRulesForName(const AtomString& return deduplicatedKeyframes; } -void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle& elementStyle, const ResolutionContext& context, KeyframeList& list, bool& containsCSSVariableReferences, bool& hasRelativeFontWeight, HashSet& inheritedProperties, HashSet& currentColorProperties) +void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle& elementStyle, const ResolutionContext& context, KeyframeList& list, bool& containsCSSVariableReferences, bool& hasRelativeFontWeight, HashSet& inheritedProperties, HashSet& currentColorProperties) { inheritedProperties.clear(); currentColorProperties.clear(); @@ -468,6 +469,9 @@ void Resolver::keyframeStylesForAnimation(const Element& element, const RenderSt currentColorProperties.add(property.id()); else if (property.id() == CSSPropertyFontWeight && (valueId == CSSValueBolder || valueId == CSSValueLighter)) hasRelativeFontWeight = true; + } else if (auto* customPropertyValue = dynamicDowncast(cssValue)) { + if (customPropertyValue->isCurrentColor()) + currentColorProperties.add(customPropertyValue->name()); } } } diff --git a/Source/WebCore/style/StyleResolver.h b/Source/WebCore/style/StyleResolver.h index 0384275cb74b..27b96eafc9ab 100644 --- a/Source/WebCore/style/StyleResolver.h +++ b/Source/WebCore/style/StyleResolver.h @@ -95,7 +95,7 @@ class Resolver : public RefCounted { ResolvedStyle styleForElement(const Element&, const ResolutionContext&, RuleMatchingBehavior = RuleMatchingBehavior::MatchAllRules); - void keyframeStylesForAnimation(const Element&, const RenderStyle& elementStyle, const ResolutionContext&, KeyframeList&, bool& containsCSSVariableReferences, bool& hasRelativeFontWeight, HashSet& inheritedProperties, HashSet& currentColorProperties); + void keyframeStylesForAnimation(const Element&, const RenderStyle& elementStyle, const ResolutionContext&, KeyframeList&, bool& containsCSSVariableReferences, bool& hasRelativeFontWeight, HashSet& inheritedProperties, HashSet& currentColorProperties); WEBCORE_EXPORT std::optional styleForPseudoElement(const Element&, const PseudoElementRequest&, const ResolutionContext&);