Skip to content
Permalink
Browse files
[web-animations] Animation.commitStyles() triggers a mutation even wh…
…en the styles are unchanged

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

Reviewed by Antti Koivisto.

We would always set the "style" attribute on the target element in Animation.commitStyles() even
if the styles committed were redundant with the existing styles. So now we use a MustableStyleProperties
to set the new styles which allows us to use the bool value returned by setProperty() and
setCustomProperty() to determine whether a change in value occurred.

Applying that change surfaced another issue caught by a separate WPT test (custom-elements/reactions/Animation.html)
that indicated that we failed to commit the longhand values when a shorthand is used as an animated
property. This was actually called out by the spec, so we correctly implement that to avoid a regression.

* LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/commitStyles-expected.txt:
* Source/WebCore/animation/WebAnimation.cpp:
(WebCore::WebAnimation::commitStyles):

Canonical link: https://commits.webkit.org/255129@main
  • Loading branch information
graouts committed Oct 4, 2022
1 parent 4406377 commit 8687381bc34b4a4d1ce04492f9569ddaaa310cf2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 15 deletions.
@@ -16,7 +16,7 @@ PASS Commits "none" transform
PASS Commits the intermediate value of an animation in the middle of stack
PASS Commit composites on top of the underlying value
PASS Triggers mutation observers when updating style
FAIL Does NOT trigger mutation observers when the change to style is redundant assert_equals: Should have no mutation records expected 0 but got 1
PASS Does NOT trigger mutation observers when the change to style is redundant
PASS Throws if the target element is a pseudo element
PASS Throws if the target element is not something with a style attribute
PASS Throws if the target effect is display:none
@@ -47,6 +47,7 @@
#include "KeyframeEffectStack.h"
#include "Logging.h"
#include "RenderElement.h"
#include "StylePropertyShorthand.h"
#include "StyleResolver.h"
#include "StyledElement.h"
#include "WebAnimationUtilities.h"
@@ -1461,9 +1462,6 @@ ExceptionOr<void> WebAnimation::commitStyles()
// 2.3 Let inline style be the result of getting the CSS declaration block corresponding to target’s style attribute. If target does not have a style
// attribute, let inline style be a new empty CSS declaration block with the readonly flag unset and owner node set to target.

// 2.4 Let targeted properties be the set of physical longhand properties that are a target property for at least one animation effect associated with
// animation whose effect target is target.

auto unanimatedStyle = [&]() {
if (auto styleable = Styleable::fromRenderer(*renderer)) {
if (auto* lastStyleChangeEventStyle = styleable->lastStyleChangeEventStyle())
@@ -1475,8 +1473,14 @@ ExceptionOr<void> WebAnimation::commitStyles()
}();

auto computedStyleExtractor = ComputedStyleExtractor(&styledElement);
auto inlineStyle = styledElement.document().createCSSStyleDeclaration();
inlineStyle->setCssText(styledElement.getAttribute(HTMLNames::styleAttr));

auto inlineStyle = [&]() {
if (auto existinInlineStyle = styledElement.inlineStyle())
return existinInlineStyle->mutableCopy();
auto styleDeclaration = styledElement.document().createCSSStyleDeclaration();
styleDeclaration->setCssText(styledElement.getAttribute(HTMLNames::styleAttr));
return styleDeclaration->copyProperties();
}();

auto& keyframeStack = styledElement.ensureKeyframeEffectStack(PseudoId::None);

@@ -1510,30 +1514,40 @@ ExceptionOr<void> WebAnimation::commitStyles()
}
if (m_replaceState == ReplaceState::Removed)
effect->animation()->resolve(*animatedStyle, { nullptr });
WTF::switchOn(property,
return WTF::switchOn(property,
[&] (CSSPropertyID propertyId) {
if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(*animatedStyle, propertyId, nullptr))
inlineStyle->setPropertyInternal(propertyId, cssValue->cssText(), false);
return inlineStyle->setProperty(propertyId, cssValue->cssText(), false);
return false;
},
[&] (AtomString customProperty) {
if (auto cssValue = computedStyleExtractor.customPropertyValue(customProperty))
inlineStyle->setProperty(customProperty, cssValue->cssText(), emptyString());
return inlineStyle->setCustomProperty(&styledElement.document(), customProperty, cssValue->cssText(), false, { styledElement.document() });
return false;
}
);
};

// During iteration resolve() could clear the underlying properties so we use a copy
auto properties = effect->animatedProperties();
// 2.4 Let targeted properties be the set of physical longhand properties that are a target property for at least one
// animation effect associated with animation whose effect target is target.
HashSet<CSSPropertyID> targetedProperties;
for (auto property : effect->animatedProperties()) {
for (auto longhand : shorthandForProperty(property))
targetedProperties.add(longhand);
targetedProperties.add(property);
}
// 2.5 For each property, property, in targeted properties:
for (auto property : properties) {
auto didMutate = false;
for (auto property : targetedProperties) {
if (property != CSSPropertyCustom)
commitProperty(property);
didMutate = commitProperty(property) || didMutate;
}
auto customProperties = effect->animatedCustomProperties();
for (auto customProperty : customProperties)
commitProperty(customProperty);
didMutate = commitProperty(customProperty) || didMutate;

styledElement.setAttribute(HTMLNames::styleAttr, AtomString { inlineStyle->cssText() });
if (didMutate)
styledElement.setAttribute(HTMLNames::styleAttr, inlineStyle->asTextAtom());

return { };
}

0 comments on commit 8687381

Please sign in to comment.