Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[web-animations] keyframes should be recomputed when a parent element…
… changes value for a non-inherited property set to "inherit"

https://bugs.webkit.org/show_bug.cgi?id=248152

Reviewed by Antti Koivisto.

In the case where a non-inherited property is set to "inherit" on a keyframe, we now update keyframes
each time animations are updated in case the parent style changed value. While this is not optimal, this
is bound to be a pretty rare scenario which we can improve on later if we deem it necessary.

* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/responsive/column-rule-color-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/responsive/column-width-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/at-property-animation-expected.txt
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/baselineShift-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/clip-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/columnCount-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/columnGap-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/offsetRotate-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/opacity-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/perspective-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/rowGap-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/shapeOutside-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/to-color-change-expected.txt:
* Source/WebCore/animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::setBlendingKeyframes):
(WebCore::KeyframeEffect::setAnimatedPropertiesInStyle):
(WebCore::KeyframeEffect::computeHasExplicitlyInheritedKeyframeProperty):
* Source/WebCore/animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::hasExplicitlyInheritedKeyframeProperty const):
* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::applyKeyframeEffects):

Canonical link: https://commits.webkit.org/259645@main
  • Loading branch information
graouts committed Jan 31, 2023
1 parent c7d795d commit 25ead8e
Show file tree
Hide file tree
Showing 16 changed files with 47 additions and 17 deletions.
@@ -1,4 +1,4 @@

FAIL column-rule-color responds to currentColor changes assert_equals: expected "rgb(0, 30, 30)" but got "rgb(30, 30, 0)"
FAIL column-rule-color responds to inherited changes assert_equals: expected "rgb(0, 40, 40)" but got "rgb(40, 0, 40)"
PASS column-rule-color responds to inherited changes

@@ -1,5 +1,5 @@

PASS column-width responds to font-size changes
FAIL column-width clamps to 0px assert_equals: expected "0px" but got "20px"
FAIL column-width responds to inherited changes assert_equals: expected "auto" but got "30px"
PASS column-width responds to inherited changes

Expand Up @@ -4,7 +4,7 @@ PASS @keyframes picks up the latest @property in the document
FAIL Ongoing animation picks up redeclared custom property assert_equals: expected "rgb(150, 150, 150)" but got "0px"
PASS Ongoing animation matches new keyframes against the current registration
FAIL Ongoing animation picks up redeclared intial value assert_equals: expected "250px" but got "200px"
FAIL Ongoing animation picks up redeclared inherits flag assert_equals: expected "250px" but got "200px"
PASS Ongoing animation picks up redeclared inherits flag
FAIL Ongoing animation picks up redeclared meaning of 'unset' assert_equals: expected "250px" but got "200px"
PASS Transitioning from initial value
PASS Transitioning from specified value
Expand Down
@@ -1,4 +1,4 @@

PASS baselineShift responsive to style changes
FAIL baselineShift responsive to inherited changes assert_equals: expected "super" but got "sub"
FAIL baselineShift responsive to inherited changes assert_equals: expected "80px" but got "20px"

@@ -1,4 +1,4 @@

FAIL clip responsive to inherited clip changes assert_equals: expected "rect(10px, 20px, 330px, 340px)" but got "rect(10px, 20px, 30px, 40px)"
FAIL clip responsive to inherited clip changes from auto assert_equals: expected "rect(310px, 320px, 30px, auto)" but got "auto"
PASS clip responsive to inherited clip changes
PASS clip responsive to inherited clip changes from auto

@@ -1,3 +1,3 @@

FAIL column-count responsive to inherited column-count changes assert_equals: expected "3" but got "auto"
PASS column-count responsive to inherited column-count changes

@@ -1,5 +1,5 @@

PASS column-gap responsive to style changes
PASS column-gap clamped to 0px on keyframes
FAIL column-gap responsive to inherited changes assert_equals: expected "80px" but got "normal"
PASS column-gap responsive to inherited changes

@@ -1,3 +1,3 @@

FAIL offsetRotate responsive to inherited offsetRotate changes assert_equals: expected "auto 200deg" but got "auto 150deg"
PASS offsetRotate responsive to inherited offsetRotate changes

@@ -1,8 +1,8 @@

PASS fillOpacity responsive to inherited changes
FAIL floodOpacity responsive to inherited changes assert_equals: expected "0.375" but got "0.75"
FAIL opacity responsive to inherited changes assert_equals: expected "0.375" but got "0.75"
FAIL shapeImageThreshold responsive to inherited changes assert_equals: expected "0.375" but got "0.75"
FAIL stopOpacity responsive to inherited changes assert_equals: expected "0.375" but got "0.75"
PASS floodOpacity responsive to inherited changes
PASS opacity responsive to inherited changes
PASS shapeImageThreshold responsive to inherited changes
PASS stopOpacity responsive to inherited changes
PASS strokeOpacity responsive to inherited changes

@@ -1,5 +1,5 @@

PASS perspective responsive to style changes
PASS perspective clamped to 0px on keyframes
FAIL perspective responsive to inherited changes assert_equals: expected "80px" but got "none"
PASS perspective responsive to inherited changes

@@ -1,5 +1,5 @@

PASS row-gap responsive to style changes
PASS row-gap clamped to 0px on keyframes
FAIL row-gap responsive to inherited changes assert_equals: expected "80px" but got "normal"
PASS row-gap responsive to inherited changes

@@ -1,4 +1,4 @@

PASS shapeOutside responsive to style changes
FAIL shapeOutside responsive to inherited shapeOutside changes assert_equals: expected "circle(200px at 50% 50%)" but got "circle(150px at 50% 50%)"
PASS shapeOutside responsive to inherited shapeOutside changes

Expand Up @@ -11,5 +11,5 @@ FAIL Color responsive to parent currentColor changes assert_equals: expected "rg
FAIL Color responsive to repeated parent currentColor changes assert_equals: expected "rgb(102, 17, 51)" but got "rgb(0, 17, 153)"
PASS Color animations do not expose visited status
PASS Color animations do not expose parent visited status
FAIL Color animations respond to inherited changes assert_equals: expected "rgb(70, 120, 120)" but got "rgb(120, 70, 120)"
PASS Color animations respond to inherited changes

21 changes: 21 additions & 0 deletions Source/WebCore/animation/KeyframeEffect.cpp
Expand Up @@ -1013,6 +1013,7 @@ void KeyframeEffect::setBlendingKeyframes(KeyframeList& blendingKeyframes)
computeSomeKeyframesUseStepsTimingFunction();
computeHasImplicitKeyframeForAcceleratedProperty();
computeHasKeyframeComposingAcceleratedProperty();
computeHasExplicitlyInheritedKeyframeProperty();

checkForMatchingTransformFunctionLists();
}
Expand Down Expand Up @@ -1600,6 +1601,12 @@ void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, doub

for (auto property : properties)
blendProperty(property);

// In case one of the animated properties has its value set to "inherit" in one of the keyframes,
// let's mark the resulting animated style as having an explicitly inherited property such that
// a future style update accounts for this in a future call to TreeResolver::determineResolutionType().
if (m_hasExplicitlyInheritedKeyframeProperty)
targetStyle.setHasExplicitlyInheritedProperties();
}

TimingFunction* KeyframeEffect::timingFunctionForBlendingKeyframe(const KeyframeValue& keyframe) const
Expand Down Expand Up @@ -2318,6 +2325,20 @@ void KeyframeEffect::computeHasKeyframeComposingAcceleratedProperty()
}();
}

void KeyframeEffect::computeHasExplicitlyInheritedKeyframeProperty()
{
m_hasExplicitlyInheritedKeyframeProperty = false;

for (auto& keyframe : m_blendingKeyframes) {
if (auto* keyframeStyle = keyframe.style()) {
if (keyframeStyle->hasExplicitlyInheritedProperties()) {
m_hasExplicitlyInheritedKeyframeProperty = true;
return;
}
}
}
}

void KeyframeEffect::effectStackNoLongerPreventsAcceleration()
{
if (m_runningAccelerated == RunningAccelerated::Failed)
Expand Down
3 changes: 3 additions & 0 deletions Source/WebCore/animation/KeyframeEffect.h
Expand Up @@ -179,6 +179,7 @@ class KeyframeEffect : public AnimationEffect, public CSSPropertyBlendingClient
static String CSSPropertyIDToIDLAttributeName(CSSPropertyID);

bool containsCSSVariableReferences() const { return m_containsCSSVariableReferences; }
bool hasExplicitlyInheritedKeyframeProperty() const { return m_hasExplicitlyInheritedKeyframeProperty; }

private:
KeyframeEffect(Element*, PseudoId);
Expand Down Expand Up @@ -223,6 +224,7 @@ class KeyframeEffect : public AnimationEffect, public CSSPropertyBlendingClient
void checkForMatchingTransformFunctionLists();
void computeHasImplicitKeyframeForAcceleratedProperty();
void computeHasKeyframeComposingAcceleratedProperty();
void computeHasExplicitlyInheritedKeyframeProperty();
void abilityToBeAcceleratedDidChange();

// AnimationEffect
Expand Down Expand Up @@ -261,6 +263,7 @@ class KeyframeEffect : public AnimationEffect, public CSSPropertyBlendingClient
bool m_hasImplicitKeyframeForAcceleratedProperty { false };
bool m_hasKeyframeComposingAcceleratedProperty { false };
bool m_containsCSSVariableReferences { false };
bool m_hasExplicitlyInheritedKeyframeProperty { false };
};

} // namespace WebCore
Expand Down
6 changes: 6 additions & 0 deletions Source/WebCore/animation/KeyframeEffectStack.cpp
Expand Up @@ -163,6 +163,12 @@ OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle
auto* animation = effect->animation();

auto inheritedPropertyChanged = [&]() {
// In the rare case where a non-inherted property was set to "inherit" on a keyframe,
// we consider that a property set to "inherit" changed without trying to work out whether
// the computed value changed.
if (effect->hasExplicitlyInheritedKeyframeProperty())
return true;

if (previousLastStyleChangeEventStyle) {
for (auto property : effect->inheritedProperties()) {
ASSERT(effect->target());
Expand Down

0 comments on commit 25ead8e

Please sign in to comment.