Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web-animations] clean up the keyframe recomputation code on style change #9591

Conversation

graouts
Copy link
Contributor

@graouts graouts commented Feb 3, 2023

7a34ce6

[web-animations] clean up the keyframe recomputation code on style change
https://bugs.webkit.org/show_bug.cgi?id=251674

Reviewed by Antti Koivisto.

We landed a fair few changes recently to recompute keyframes when style changes while animations are
active. The ever-increasing list of parameters passed to Style::Resolver::keyframeStylesForAnimation()
as well as the ever-growing list of change checks made in KeyframeEffectStack::applyKeyframeEffects()
could do with some cleaning up.

We add a new KeyframeList::updatePropertiesMetadata() method which takes in a StyleProperties
and gathers information on the rule's StyleProperties relevant to keyframe recomputation: whether
"inherit" or "currentcolor" values are set, whether CSS variable values are used, whether a relative
font-weight value is set. This replaces duplicated logic found in Style::Resolver (in the CSS Animations
case) and KeyframeEffect (in the JS-originated case) when computing keyframes.

This method is called in Style::Resolver::keyframeStylesForAnimation() and KeyframeEffect::updateBlendingKeyframes().
This allows us to remove the matching information previously stored on KeyframeEffect.

We move all the logic from KeyframeEffectStack::applyKeyframeEffects() related to checking
whether some properties change to a new KeyframeEffect::recomputeKeyframesIfNecessary() which
will query the KeyframeList.

The code is now clearer and any future change to recompute keyframes will no longer require code
duplication.

Doing this refactor also uncovered a small error in the animation wrapper for text-indent which would
not check for the RenderStyle::textIndentLine() and RenderStyle::textIndentType() bits to determine
whether two text-indent were equal. This was likely due to change in the order of functions we use to check
whether a recomputation is required in the new KeyframeEffect::recomputeKeyframesIfNecessary().

* Source/WebCore/animation/CSSPropertyAnimation.cpp:
* Source/WebCore/animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::updateBlendingKeyframes):
(WebCore::KeyframeEffect::computeCSSAnimationBlendingKeyframes):
(WebCore::KeyframeEffect::recomputeKeyframesIfNecessary):
(WebCore::KeyframeEffect::animatesDirectionAwareProperty const): Deleted.
(WebCore::KeyframeEffect::propertyAffectingKeyframeResolutionDidChange): Deleted.
(WebCore::KeyframeEffect::hasPropertySetToCurrentColor const): Deleted.
(WebCore::KeyframeEffect::hasColorSetToCurrentColor const): Deleted.
* Source/WebCore/animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::inheritedProperties const): Deleted.
(WebCore::KeyframeEffect::containsCSSVariableReferences const): Deleted.
(WebCore::KeyframeEffect::hasExplicitlyInheritedKeyframeProperty const): Deleted.
(WebCore::KeyframeEffect::hasRelativeFontWeight const): Deleted.
* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::applyKeyframeEffects):
* Source/WebCore/css/CSSKeyframeRule.cpp:
(WebCore::StyleRuleKeyframe::containsCSSVariableReferences const): Deleted.
* Source/WebCore/css/CSSKeyframeRule.h:
* Source/WebCore/rendering/style/KeyframeList.cpp:
(WebCore::KeyframeList::clear):
(WebCore::KeyframeList::usesRelativeFontWeight const):
(WebCore::KeyframeList::hasCSSVariableReferences const):
(WebCore::KeyframeList::hasColorSetToCurrentColor const):
(WebCore::KeyframeList::hasPropertySetToCurrentColor const):
(WebCore::KeyframeList::propertiesSetToInherit const):
(WebCore::KeyframeList::updatePropertiesMetadata):
* Source/WebCore/rendering/style/KeyframeList.h:
* Source/WebCore/style/StyleResolver.cpp:
(WebCore::Style::Resolver::keyframeStylesForAnimation):
* Source/WebCore/style/StyleResolver.h:

Canonical link: https://commits.webkit.org/259838@main

540dab9

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl ⏳ πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
⏳ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim
βœ… πŸ›  πŸ§ͺ unsafe-merge

@graouts graouts self-assigned this Feb 3, 2023
@graouts graouts added the Animations Bugs related to CSS + SVG animations and transitions label Feb 3, 2023
@graouts graouts changed the title [web-animations] [web-animations] clean up the keyframe recomputation code on style change Feb 3, 2023
@graouts graouts force-pushed the recompute-keyframes-refactor branch from b4f881d to d945260 Compare February 3, 2023 14:07
@graouts graouts marked this pull request as ready for review February 3, 2023 14:09
@graouts graouts requested a review from anttijk February 3, 2023 14:09
Source/WebCore/rendering/style/KeyframeList.cpp Outdated Show resolved Hide resolved
Source/WebCore/animation/KeyframeEffect.h Outdated Show resolved Hide resolved
auto unanimatedStyle = RenderStyle::clone(targetStyle);

for (const auto& effect : sortedEffects()) {
auto keyframeRecomputationReason = effect->recomputeKeyframesIfNecessary(previousLastStyleChangeEventStyle, unanimatedStyle, resolutionContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup

@graouts graouts force-pushed the recompute-keyframes-refactor branch from d945260 to 85db0eb Compare February 3, 2023 14:39
@graouts graouts force-pushed the recompute-keyframes-refactor branch from 85db0eb to 540dab9 Compare February 3, 2023 14:41
@graouts graouts added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 3, 2023
…ange

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

Reviewed by Antti Koivisto.

We landed a fair few changes recently to recompute keyframes when style changes while animations are
active. The ever-increasing list of parameters passed to Style::Resolver::keyframeStylesForAnimation()
as well as the ever-growing list of change checks made in KeyframeEffectStack::applyKeyframeEffects()
could do with some cleaning up.

We add a new KeyframeList::updatePropertiesMetadata() method which takes in a StyleProperties
and gathers information on the rule's StyleProperties relevant to keyframe recomputation: whether
"inherit" or "currentcolor" values are set, whether CSS variable values are used, whether a relative
font-weight value is set. This replaces duplicated logic found in Style::Resolver (in the CSS Animations
case) and KeyframeEffect (in the JS-originated case) when computing keyframes.

This method is called in Style::Resolver::keyframeStylesForAnimation() and KeyframeEffect::updateBlendingKeyframes().
This allows us to remove the matching information previously stored on KeyframeEffect.

We move all the logic from KeyframeEffectStack::applyKeyframeEffects() related to checking
whether some properties change to a new KeyframeEffect::recomputeKeyframesIfNecessary() which
will query the KeyframeList.

The code is now clearer and any future change to recompute keyframes will no longer require code
duplication.

Doing this refactor also uncovered a small error in the animation wrapper for text-indent which would
not check for the RenderStyle::textIndentLine() and RenderStyle::textIndentType() bits to determine
whether two text-indent were equal. This was likely due to change in the order of functions we use to check
whether a recomputation is required in the new KeyframeEffect::recomputeKeyframesIfNecessary().

* Source/WebCore/animation/CSSPropertyAnimation.cpp:
* Source/WebCore/animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::updateBlendingKeyframes):
(WebCore::KeyframeEffect::computeCSSAnimationBlendingKeyframes):
(WebCore::KeyframeEffect::recomputeKeyframesIfNecessary):
(WebCore::KeyframeEffect::animatesDirectionAwareProperty const): Deleted.
(WebCore::KeyframeEffect::propertyAffectingKeyframeResolutionDidChange): Deleted.
(WebCore::KeyframeEffect::hasPropertySetToCurrentColor const): Deleted.
(WebCore::KeyframeEffect::hasColorSetToCurrentColor const): Deleted.
* Source/WebCore/animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::inheritedProperties const): Deleted.
(WebCore::KeyframeEffect::containsCSSVariableReferences const): Deleted.
(WebCore::KeyframeEffect::hasExplicitlyInheritedKeyframeProperty const): Deleted.
(WebCore::KeyframeEffect::hasRelativeFontWeight const): Deleted.
* Source/WebCore/animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::applyKeyframeEffects):
* Source/WebCore/css/CSSKeyframeRule.cpp:
(WebCore::StyleRuleKeyframe::containsCSSVariableReferences const): Deleted.
* Source/WebCore/css/CSSKeyframeRule.h:
* Source/WebCore/rendering/style/KeyframeList.cpp:
(WebCore::KeyframeList::clear):
(WebCore::KeyframeList::usesRelativeFontWeight const):
(WebCore::KeyframeList::hasCSSVariableReferences const):
(WebCore::KeyframeList::hasColorSetToCurrentColor const):
(WebCore::KeyframeList::hasPropertySetToCurrentColor const):
(WebCore::KeyframeList::propertiesSetToInherit const):
(WebCore::KeyframeList::updatePropertiesMetadata):
* Source/WebCore/rendering/style/KeyframeList.h:
* Source/WebCore/style/StyleResolver.cpp:
(WebCore::Style::Resolver::keyframeStylesForAnimation):
* Source/WebCore/style/StyleResolver.h:

Canonical link: https://commits.webkit.org/259838@main
@webkit-commit-queue
Copy link
Collaborator

Committed 259838@main (7a34ce6): https://commits.webkit.org/259838@main

Reviewed commits have been landed. Closing PR #9591 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 7a34ce6 into WebKit:main Feb 3, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 3, 2023
@graouts graouts deleted the recompute-keyframes-refactor branch February 3, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Animations Bugs related to CSS + SVG animations and transitions
Projects
None yet
4 participants