Skip to content

Commit

Permalink
[Web Animations] Make WPT test at animation-model/keyframe-effects/ef…
Browse files Browse the repository at this point in the history
…fect-value-context.html pass reliably

https://bugs.webkit.org/show_bug.cgi?id=186490
rdar://41000137

Reviewed by Antti Koivisto.

We added support for recomputing keyframes when the computed font-size changes in bug 237357, allowing
any keyframe values depending on font-size to produce the expected values. However, we have a bug when
font-size changes on a parent element where the logic under ComputedStyleExtractor::propertyValue() will
not invalidate style because the code does not think the style needs updating.

So we update hasValidStyleForProperty() to account for animations affecting the property for which
the computed style is requested.

* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/keyframe-effects/effect-value-context-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/keyframe-effects/effect-value-context-filling-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/boxShadow-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/shapeOutside-expected.txt:
* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::containsProperty const):
* Source/WebCore/animation/KeyframeEffectStack.h:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::hasValidStyleForProperty):

Canonical link: https://commits.webkit.org/256889@main
  • Loading branch information
graouts committed Nov 20, 2022
1 parent 3caac1b commit 6f8d19c
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 6 deletions.
@@ -1,6 +1,6 @@

PASS Effect values reflect changes to font-size on element
FAIL Effect values reflect changes to font-size on parent element assert_equals: Effect value after updating font-size on parent element expected "300px" but got "150px"
PASS Effect values reflect changes to font-size on parent element
PASS Effect values reflect changes to font-size when computed style is not immediately flushed
PASS Effect values reflect changes to font-size from reparenting
PASS Effect values reflect changes to target element
Expand Down
@@ -1,6 +1,6 @@

PASS Filling effect values reflect changes to font-size on element
FAIL Filling effect values reflect changes to font-size on parent element assert_equals: Effect value after updating font-size on parent element expected "400px" but got "200px"
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 the the animation's keyframes
Expand All @@ -9,8 +9,8 @@ PASS Filling effect values reflect changes to the the animation's iteration comp
PASS Filling effect values reflect changes to the base value when using additive animation
PASS Filling effect values reflect changes to the base value when using additive animation on a single keyframe
PASS Filling effect values reflect changes to the base value when using the fill value is an implicit keyframe
FAIL Filling effect values reflect changes to the base value via a parent element assert_equals: Effect value after updating font-size on parent expected "400px" but got "300px"
PASS Filling effect values reflect changes to the base value via a parent element
PASS Filling effect values reflect changes to underlying animations
FAIL Filling effect values reflect changes to underlying animations via a a parent element assert_equals: Effect value after updating parent font-size expected "400px" but got "300px"
PASS Filling effect values reflect changes to underlying animations via a a parent element
PASS Filling effect values reflect changes to underlying animations made by directly changing the keyframes

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

FAIL boxShadow responsive to style changes assert_not_equals: got disallowed value "rgb(255, 0, 0) 100px 100px 0px 0px"
PASS boxShadow responsive to style changes

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

FAIL shapeOutside responsive to style changes assert_not_equals: got disallowed value "circle(100px at 50% 50%)"
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%)"

7 changes: 7 additions & 0 deletions Source/WebCore/animation/KeyframeEffectStack.cpp
Expand Up @@ -77,6 +77,13 @@ bool KeyframeEffectStack::hasMatchingEffect(const Function<bool(const KeyframeEf
return false;
}

bool KeyframeEffectStack::containsProperty(CSSPropertyID property) const
{
return hasMatchingEffect([property] (const KeyframeEffect& effect) {
return effect.animatesProperty(property);
});
}

bool KeyframeEffectStack::requiresPseudoElement() const
{
return hasMatchingEffect([] (const KeyframeEffect& effect) {
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/animation/KeyframeEffectStack.h
Expand Up @@ -52,6 +52,7 @@ class KeyframeEffectStack {
Vector<WeakPtr<KeyframeEffect>> sortedEffects();
const AnimationList* cssAnimationList() const { return m_cssAnimationList.get(); }
void setCSSAnimationList(RefPtr<const AnimationList>&&);
bool containsProperty(CSSPropertyID) const;
bool isCurrentlyAffectingProperty(CSSPropertyID) const;
bool requiresPseudoElement() const;
OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle* previousLastStyleChangeEventStyle, const Style::ResolutionContext&);
Expand Down
5 changes: 5 additions & 0 deletions Source/WebCore/css/ComputedStyleExtractor.cpp
Expand Up @@ -2484,6 +2484,11 @@ static inline bool hasValidStyleForProperty(Element& element, CSSPropertyID prop
if (!element.document().childNeedsStyleRecalc())
return true;

if (auto* keyframeEffectStack = Styleable(element, PseudoId::None).keyframeEffectStack()) {
if (keyframeEffectStack->containsProperty(propertyID))
return false;
}

auto isQueryContainer = [&](Element& element) {
auto* style = element.renderStyle();
return style && style->containerType() != ContainerType::Normal;
Expand Down

0 comments on commit 6f8d19c

Please sign in to comment.