Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
CSS animations should participate in the cascade
https://bugs.webkit.org/show_bug.cgi?id=210963
rdar://62301534

Reviewed by Antoine Quint.

Property animation can affect other properties, for example via em units when animating font-size or var() when animating custom properties.
Animated values can also be overridden by !important properties.

With this patch we start applying the cascade after computing animated style. Only the properties that may be affected are recomputed.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-base-response-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-base-response-003-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-base-response-004-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-important-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/keyframes-unrelated-custom-property-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/animation/custom-property-animation-non-inherited-used-by-standard-property-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/animation/custom-property-transition-non-inherited-used-by-standard-property-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/transition-base-response-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/transition-base-response-003-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-animation-from-to-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-animation-over-transition-expected.txt:

A pile of progressions.

* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::applyKeyframeEffects):

Collected animated properties.

* Source/WebCore/animation/KeyframeEffectStack.h:
* Source/WebCore/animation/WebAnimationTypes.h:
* Source/WebCore/dom/Element.h:
* Source/WebCore/html/shadow/TextControlInnerElements.cpp:
(WebCore::TextControlInnerElement::resolveCustomStyle):
(WebCore::TextControlInnerTextElement::resolveCustomStyle):
* Source/WebCore/rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::customPropertiesEqual const):
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/style/ElementRuleCollector.cpp:
(WebCore::Style::ElementRuleCollector::ElementRuleCollector):

Switch to std::unique_ptr<MatchResult> for easier shuffling around.

(WebCore::Style::ElementRuleCollector::matchResult const):
(WebCore::Style::ElementRuleCollector::releaseMatchResult):
(WebCore::Style::ElementRuleCollector::addElementStyleProperties):
(WebCore::Style::ElementRuleCollector::declarationsForOrigin):
(WebCore::Style::ElementRuleCollector::transferMatchedRules):
(WebCore::Style::ElementRuleCollector::addMatchedProperties):
(WebCore::Style::ElementRuleCollector::addAuthorKeyframeRules):
* Source/WebCore/style/ElementRuleCollector.h:
* Source/WebCore/style/MatchResult.h:
* Source/WebCore/style/PropertyCascade.cpp:
(WebCore::Style::PropertyCascade::PropertyCascade):
(WebCore::Style::PropertyCascade::AnimationLayer::AnimationLayer):

Animations behave like a cascade layer, that's where the name of this helper struct comes from.

(WebCore::Style::PropertyCascade::set):
(WebCore::Style::PropertyCascade::setDeferred):
(WebCore::Style::PropertyCascade::addMatch):
(WebCore::Style::PropertyCascade::shouldApplyAfterAnimation):

Build a cascade that only contains properties that require re-resolving after animation.

(WebCore::Style::initializeCSSValue): Deleted.
* Source/WebCore/style/PropertyCascade.h:
(WebCore::Style::PropertyCascade::isEmpty const):

In normal cases there is nothing to apply after animations so we can bail out.

* Source/WebCore/style/StyleAdjuster.h:
* Source/WebCore/style/StyleBuilder.cpp:
(WebCore::Style::Builder::Builder):
(WebCore::Style::Builder::applyAllProperties):
* Source/WebCore/style/StyleBuilder.h:
* Source/WebCore/style/StyleResolver.cpp:
(WebCore::Style::Resolver::styleForElement):

Return the MatchResult along with the style. This allows us to reapply the cascade without matching the selectors again.

* Source/WebCore/style/StyleResolver.h:
(WebCore::Style::ElementStyle::ElementStyle): Deleted.
* Source/WebCore/style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::styleForStyleable):
(WebCore::Style::TreeResolver::resolveElement):
(WebCore::Style::TreeResolver::resolvePseudoElement):
(WebCore::Style::TreeResolver::resolveAncestorPseudoElement):
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):
(WebCore::Style::TreeResolver::applyCascadeAfterAnimation):

Build and apply the cascade.

* Source/WebCore/style/StyleTreeResolver.h:
* Source/WebCore/style/Styleable.h:
(WebCore::Styleable::applyKeyframeEffects const):

Canonical link: https://commits.webkit.org/258514@main
  • Loading branch information
anttijk committed Jan 6, 2023
1 parent 6a81017 commit eb9e694
Show file tree
Hide file tree
Showing 37 changed files with 215 additions and 119 deletions.
4 changes: 0 additions & 4 deletions LayoutTests/TestExpectations
Expand Up @@ -2309,9 +2309,6 @@ webkit.org/b/148026 [ Debug ] animations/restart-after-scroll.html [ Skip ]

webkit.org/b/148650 fast/repaint/add-table-overpaint.html [ Pass Failure ]

imported/w3c/web-platform-tests/css/css-cascade/important-prop.html [ ImageOnlyFailure ]
webkit.org/b/192334 imported/w3c/web-platform-tests/css/css-cascade/revert-layer-011.html [ ImageOnlyFailure ]

# Initial failures on the import of css-content

imported/w3c/web-platform-tests/css/css-content/element-replacement-on-replaced-element.tentative.html [ ImageOnlyFailure ]
Expand Down Expand Up @@ -4134,7 +4131,6 @@ webkit.org/b/206579 [ Debug ] imported/w3c/web-platform-tests/css/css-background

webkit.org/b/207262 imported/w3c/web-platform-tests/web-animations/timing-model/animations/sync-start-times.html [ Pass ImageOnlyFailure ]

webkit.org/b/210963 imported/w3c/web-platform-tests/css/css-animations/animation-important-002.html [ ImageOnlyFailure ]
webkit.org/b/235110 imported/w3c/web-platform-tests/css/css-animations/flip-running-animation-via-variable.html [ ImageOnlyFailure ]

webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-transformed-tr-percent-height-child.html [ ImageOnlyFailure ]
Expand Down
@@ -1,5 +1,5 @@

FAIL em units respond to font-size animation assert_equals: expected "15px" but got "1px"
FAIL ex units respond to font-size animation assert_equals: expected "6.71875px" but got "0.4375px"
FAIL var() references respond to custom property animation assert_equals: expected "20px" but got "0px"
PASS em units respond to font-size animation
PASS ex units respond to font-size animation
PASS var() references respond to custom property animation

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

FAIL Identical elements are all responsive to font-size animation assert_equals: expected "15px" but got "1px"
PASS Identical elements are all responsive to font-size animation

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

FAIL Base is responsive to font-affecting appearing via setKeyframes assert_equals: expected "15px" but got "10px"
PASS Base is responsive to font-affecting appearing via setKeyframes

@@ -1,9 +1,9 @@

PASS Interpolated value is observable
FAIL Important rules override animations assert_equals: expected "rgb(0, 128, 0)" but got "rgb(15, 15, 15)"
FAIL Non-overriden interpolations are observable assert_equals: expected "rgb(0, 128, 0)" but got "rgb(15, 15, 15)"
PASS Important rules override animations
PASS Non-overriden interpolations are observable
FAIL Important rules override animations (::before) assert_equals: expected "rgb(0, 128, 0)" but got "rgb(15, 15, 15)"
PASS Important rules do not override animations on :visited as seen from JS
FAIL Standard property animations appearing via setKeyframes do not override important declarations assert_equals: expected "rgb(255, 255, 255)" but got "rgb(15, 15, 15)"
FAIL Custom property animations appearing via setKeyframes do not override important declarations assert_equals: expected "10px" but got "150px"
PASS Standard property animations appearing via setKeyframes do not override important declarations
PASS Custom property animations appearing via setKeyframes do not override important declarations

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

FAIL Unrelated custom properties do not conflict with each other assert_equals: expected "rgb(0, 128, 0)" but got "rgb(255, 0, 0)"
PASS Unrelated custom properties do not conflict with each other

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

FAIL Animating a non-inherited CSS variable is reflected on a standard property using that variable as a value assert_equals: expected "25px" but got "0px"
PASS Animating a non-inherited CSS variable is reflected on a standard property using that variable as a value

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

FAIL Running a transition a non-inherited CSS variable is reflected on a standard property using that variable as a value assert_equals: expected "150px" but got "200px"
PASS Running a transition a non-inherited CSS variable is reflected on a standard property using that variable as a value

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

FAIL em units respond to font-size transition assert_equals: expected "15px" but got "20px"
FAIL ex units respond to font-size transition assert_equals: expected "6.71875px" but got "8.96875px"
PASS em units respond to font-size transition
PASS ex units respond to font-size transition
PASS var() references respond to custom property transition

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

FAIL Identical elements are all responsive to font-size transition assert_equals: expected "15px" but got "20px"
PASS Identical elements are all responsive to font-size transition

@@ -1,7 +1,7 @@
This text should animate from blue to green over a period of 1 second.

PASS Verify CSS variable value before animation
FAIL Verify substituted color value before animation assert_equals: color is blue before animation runs expected "rgb(0, 0, 255)" but got "rgb(255, 0, 0)"
PASS Verify substituted color value before animation
PASS Verify CSS variable value after animation
FAIL Verify substituted color value after animation assert_equals: color is green after animation runs expected "rgb(0, 128, 0)" but got "rgb(255, 0, 0)"
PASS Verify substituted color value after animation

@@ -1,7 +1,7 @@
This text should animate from blue to green over a period of 1 second. The transition is not visible because the animation overrides it.

PASS Verify CSS variable value before animation
FAIL Verify substituted color value before animation assert_equals: color is blue before animation runs expected "rgb(0, 0, 255)" but got "rgb(255, 0, 0)"
PASS Verify substituted color value before animation
PASS Verify CSS variable value after animation
FAIL Verify substituted color value after animation assert_equals: color is green after animation runs expected "rgb(0, 128, 0)" but got "rgb(0, 0, 0)"
PASS Verify substituted color value after animation

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

11 changes: 4 additions & 7 deletions Source/WebCore/animation/KeyframeEffectStack.cpp
Expand Up @@ -140,7 +140,7 @@ void KeyframeEffectStack::setCSSAnimationList(RefPtr<const AnimationList>&& cssA
m_isSorted = false;
}

OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle* previousLastStyleChangeEventStyle, const Style::ResolutionContext& resolutionContext)
OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle& targetStyle, HashSet<AnimatableProperty>& affectedProperties, const RenderStyle* previousLastStyleChangeEventStyle, const Style::ResolutionContext& resolutionContext)
{
OptionSet<AnimationImpact> impact;

Expand Down Expand Up @@ -174,13 +174,8 @@ OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle
};

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()))
if (!previousLastStyleChangeEventStyle->customPropertiesEqual(targetStyle))
return true;
}
return false;
Expand All @@ -206,6 +201,8 @@ OptionSet<AnimationImpact> KeyframeEffectStack::applyKeyframeEffects(RenderStyle
ASSERT(animation->timeline());
animation->timeline()->animationTimingDidChange(*animation);
}

affectedProperties.formUnion(effect->animatedProperties());
}

return impact;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/animation/KeyframeEffectStack.h
Expand Up @@ -55,7 +55,7 @@ class KeyframeEffectStack {
bool containsProperty(CSSPropertyID) const;
bool isCurrentlyAffectingProperty(CSSPropertyID) const;
bool requiresPseudoElement() const;
OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& targetStyle, const RenderStyle* previousLastStyleChangeEventStyle, const Style::ResolutionContext&);
OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle& targetStyle, HashSet<AnimatableProperty>& affectedProperties, const RenderStyle* previousLastStyleChangeEventStyle, const Style::ResolutionContext&);
bool hasEffectWithImplicitKeyframes() const;

void effectAbilityToBeAcceleratedDidChange(const KeyframeEffect&);
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/animation/WebAnimationTypes.h
Expand Up @@ -53,7 +53,7 @@ struct WebAnimationsMarkableDoubleTraits {
}
};

enum class AnimationImpact {
enum class AnimationImpact : uint8_t {
RequiresRecomposite = 1 << 0,
ForcesStackingContext = 1 << 1
};
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/dom/Element.h
Expand Up @@ -81,7 +81,7 @@ class Text;
class UniqueElementData;
class WebAnimation;

enum class AnimationImpact;
enum class AnimationImpact : uint8_t;
enum class EventHandling : uint8_t;
enum class EventProcessing : uint8_t;
enum class FullscreenNavigationUI : uint8_t;
Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/html/shadow/TextControlInnerElements.cpp
Expand Up @@ -137,7 +137,7 @@ std::optional<Style::ElementStyle> TextControlInnerElement::resolveCustomStyle(c
newStyle->setFlexBasis(Length { pixels, LengthType::Fixed });
}

return { WTFMove(newStyle) };
return Style::ElementStyle { WTFMove(newStyle) };
}

// MARK: TextControlInnerTextElement
Expand Down Expand Up @@ -198,7 +198,7 @@ RenderTextControlInnerBlock* TextControlInnerTextElement::renderer() const
std::optional<Style::ElementStyle> TextControlInnerTextElement::resolveCustomStyle(const Style::ResolutionContext&, const RenderStyle* shadowHostStyle)
{
auto style = downcast<HTMLTextFormControlElement>(*shadowHost()).createInnerTextStyle(*shadowHostStyle);
return { makeUnique<RenderStyle>(WTFMove(style)) };
return Style::ElementStyle { makeUnique<RenderStyle>(WTFMove(style)) };
}

// MARK: TextControlPlaceholderElement
Expand Down
6 changes: 6 additions & 0 deletions Source/WebCore/rendering/style/RenderStyle.cpp
Expand Up @@ -2669,6 +2669,12 @@ void RenderStyle::setNonInheritedCustomPropertyValue(const AtomString& name, Ref
m_rareNonInheritedData.access().customProperties.access().setCustomPropertyValue(name, WTFMove(value));
}

bool RenderStyle::customPropertiesEqual(const RenderStyle& other) const
{
return m_rareNonInheritedData->customProperties == other.m_rareNonInheritedData->customProperties
&& m_rareInheritedData->customProperties == other.m_rareInheritedData->customProperties;
}

const LengthBox& RenderStyle::scrollMargin() const
{
return m_rareNonInheritedData->scrollMargin;
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/rendering/style/RenderStyle.h
Expand Up @@ -198,6 +198,7 @@ class RenderStyle {
const CSSCustomPropertyValue* getCustomProperty(const AtomString&) const;
void setInheritedCustomPropertyValue(const AtomString& name, Ref<CSSCustomPropertyValue>&&);
void setNonInheritedCustomPropertyValue(const AtomString& name, Ref<CSSCustomPropertyValue>&&);
bool customPropertiesEqual(const RenderStyle&) const;

void setUsesViewportUnits() { m_nonInheritedFlags.usesViewportUnits = true; }
bool usesViewportUnits() const { return m_nonInheritedFlags.usesViewportUnits; }
Expand Down
33 changes: 20 additions & 13 deletions Source/WebCore/style/ElementRuleCollector.cpp
Expand Up @@ -92,6 +92,7 @@ ElementRuleCollector::ElementRuleCollector(const Element& element, const ScopeRu
, m_userStyle(ruleSets.userStyle())
, m_userAgentMediaQueryStyle(ruleSets.userAgentMediaQueryStyle())
, m_selectorMatchingState(selectorMatchingState)
, m_result(makeUnique<MatchResult>())
{
ASSERT(!m_selectorMatchingState || m_selectorMatchingState->selectorFilter.parentStackIsConsistent(element.parentNode()));
}
Expand All @@ -100,14 +101,20 @@ ElementRuleCollector::ElementRuleCollector(const Element& element, const RuleSet
: m_element(element)
, m_authorStyle(authorStyle)
, m_selectorMatchingState(selectorMatchingState)
, m_result(makeUnique<MatchResult>())
{
ASSERT(!m_selectorMatchingState || m_selectorMatchingState->selectorFilter.parentStackIsConsistent(element.parentNode()));
}

const MatchResult& ElementRuleCollector::matchResult() const
{
ASSERT(m_mode == SelectorChecker::Mode::ResolvingStyle);
return m_result;
return *m_result;
}

std::unique_ptr<MatchResult> ElementRuleCollector::releaseMatchResult()
{
return WTFMove(m_result);
}

const Vector<RefPtr<const StyleRule>>& ElementRuleCollector::matchedRuleList() const
Expand All @@ -134,7 +141,7 @@ inline void ElementRuleCollector::addElementStyleProperties(const StylePropertie
return;

if (!isCacheable)
m_result.isCacheable = false;
m_result->isCacheable = false;

auto matchedProperty = MatchedProperties { propertySet };
matchedProperty.cascadeLayerPriority = priority;
Expand Down Expand Up @@ -214,15 +221,15 @@ void ElementRuleCollector::collectMatchingRules(const MatchRequest& matchRequest
}


Vector<MatchedProperties>& ElementRuleCollector::declarationsForOrigin(MatchResult& matchResult, DeclarationOrigin declarationOrigin)
Vector<MatchedProperties>& ElementRuleCollector::declarationsForOrigin(DeclarationOrigin declarationOrigin)
{
switch (declarationOrigin) {
case DeclarationOrigin::UserAgent: return matchResult.userAgentDeclarations;
case DeclarationOrigin::User: return matchResult.userDeclarations;
case DeclarationOrigin::Author: return matchResult.authorDeclarations;
case DeclarationOrigin::UserAgent: return m_result->userAgentDeclarations;
case DeclarationOrigin::User: return m_result->userDeclarations;
case DeclarationOrigin::Author: return m_result->authorDeclarations;
}
ASSERT_NOT_REACHED();
return matchResult.authorDeclarations;
return m_result->authorDeclarations;
}

void ElementRuleCollector::sortAndTransferMatchedRules(DeclarationOrigin declarationOrigin)
Expand All @@ -238,7 +245,7 @@ void ElementRuleCollector::sortAndTransferMatchedRules(DeclarationOrigin declara
void ElementRuleCollector::transferMatchedRules(DeclarationOrigin declarationOrigin, std::optional<ScopeOrdinal> fromScope)
{
if (m_mode != SelectorChecker::Mode::CollectingRules)
declarationsForOrigin(m_result, declarationOrigin).reserveCapacity(m_matchedRules.size());
declarationsForOrigin(declarationOrigin).reserveCapacity(m_matchedRules.size());

for (; m_matchedRuleTransferIndex < m_matchedRules.size(); ++m_matchedRuleTransferIndex) {
auto& matchedRule = m_matchedRules[m_matchedRuleTransferIndex];
Expand Down Expand Up @@ -649,7 +656,7 @@ void ElementRuleCollector::addMatchedProperties(MatchedProperties&& matchedPrope
{
// FIXME: This should be moved to the matched properties cache code.
auto computeIsCacheable = [&] {
if (!m_result.isCacheable)
if (!m_result->isCacheable)
return false;

if (matchedProperties.styleScopeOrdinal != ScopeOrdinal::Element)
Expand Down Expand Up @@ -683,15 +690,15 @@ void ElementRuleCollector::addMatchedProperties(MatchedProperties&& matchedPrope
return true;
};

m_result.isCacheable = computeIsCacheable();
m_result->isCacheable = computeIsCacheable();

declarationsForOrigin(m_result, declarationOrigin).append(WTFMove(matchedProperties));
declarationsForOrigin(declarationOrigin).append(WTFMove(matchedProperties));
}

void ElementRuleCollector::addAuthorKeyframeRules(const StyleRuleKeyframe& keyframe)
{
ASSERT(m_result.authorDeclarations.isEmpty());
m_result.authorDeclarations.append({ &keyframe.properties(), SelectorChecker::MatchAll, propertyAllowlistForPseudoId(m_pseudoElementRequest.pseudoId) });
ASSERT(m_result->authorDeclarations.isEmpty());
m_result->authorDeclarations.append({ &keyframe.properties(), SelectorChecker::MatchAll, propertyAllowlistForPseudoId(m_pseudoElementRequest.pseudoId) });
}

}
Expand Down
6 changes: 4 additions & 2 deletions Source/WebCore/style/ElementRuleCollector.h
Expand Up @@ -86,6 +86,8 @@ class ElementRuleCollector {
bool hasAnyMatchingRules(const RuleSet&);

const MatchResult& matchResult() const;
std::unique_ptr<MatchResult> releaseMatchResult();

const Vector<RefPtr<const StyleRule>>& matchedRuleList() const;

void clearMatchedRules();
Expand Down Expand Up @@ -120,7 +122,7 @@ class ElementRuleCollector {
void sortMatchedRules();

enum class DeclarationOrigin { UserAgent, User, Author };
static Vector<MatchedProperties>& declarationsForOrigin(MatchResult&, DeclarationOrigin);
Vector<MatchedProperties>& declarationsForOrigin(DeclarationOrigin);
void sortAndTransferMatchedRules(DeclarationOrigin);
void transferMatchedRules(DeclarationOrigin, std::optional<ScopeOrdinal> forScope = { });

Expand All @@ -146,7 +148,7 @@ class ElementRuleCollector {
// Output.
Vector<RefPtr<const StyleRule>> m_matchedRuleList;
bool m_didMatchUncommonAttributeSelector { false };
MatchResult m_result;
std::unique_ptr<MatchResult> m_result;
Relations m_styleRelations;
PseudoIdSet m_matchedPseudoElementIds;
};
Expand Down
2 changes: 2 additions & 0 deletions Source/WebCore/style/MatchResult.h
Expand Up @@ -45,6 +45,8 @@ struct MatchedProperties {
};

struct MatchResult {
WTF_MAKE_STRUCT_FAST_ALLOCATED;

bool isCacheable { true };
Vector<MatchedProperties> userAgentDeclarations;
Vector<MatchedProperties> userDeclarations;
Expand Down

0 comments on commit eb9e694

Please sign in to comment.