Changeset 287827 in webkit

Timestamp:

Jan 9, 2022 1:19:45 PM (6 months ago)

Author:

graouts@webkit.org

Message:

Implicit keyframe for a CSS Animation should always use the underlying style
​https://bugs.webkit.org/show_bug.cgi?id=235014

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Add a new WPT which changes the underlying style for a property that is being
animated with an @keyframes rule that doesn't explicitly specify this value
on all keyframes.

• web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt: Added.
• web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html: Added.

Source/WebCore:

When resolving keyframes for a CSS Animation in Style::Resolver::keyframeStylesForAnimation(),
we would fill implicit keyframes based on the style at the time the animation was created.
However, the underlying style may change at any point while the CSS Animation is in flight,
so we should not be filling those values.

The code that actually resolves animated styles already knows how to handle this case, which
we already supported correctly for JS-originated animations. The only thing we need to do is
to ensure we fill implicit keyframes when calling getKeyframes().

This surfaced an issue with KeyframeList::copyKeyframes() where we only copied the styles
from one set of keyframes to another, but not the timing function, which would cause a
getKeyframes() test to regress. We should also make sure to copy the composite operation
for when we support the animation-composition property (see bug 232086).

Test: imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html

• animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::getKeyframes):

• rendering/style/KeyframeList.cpp:

(WebCore::KeyframeList::copyKeyframes):

• style/StyleResolver.cpp:

(WebCore::Style::Resolver::keyframeStylesForAnimation):

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt (added)
• LayoutTests/imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (6 diffs)
• Source/WebCore/rendering/style/KeyframeList.cpp (modified) (1 diff)
• Source/WebCore/style/StyleResolver.cpp (modified) (1 diff)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-01-09  Antoine Quint  <graouts@webkit.org>
+
+        Implicit keyframe for a CSS Animation should always use the underlying style
+        https://bugs.webkit.org/show_bug.cgi?id=235014
+
+        Reviewed by Antti Koivisto.
+
+        Add a new WPT which changes the underlying style for a property that is being
+        animated with an @keyframes rule that doesn't explicitly specify this value
+        on all keyframes.
+
+        * web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight-expected.txt: Added.
+        * web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html: Added.
+
 2022-01-09  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-09  Antoine Quint  <graouts@webkit.org>
+
+        Implicit keyframe for a CSS Animation should always use the underlying style
+        https://bugs.webkit.org/show_bug.cgi?id=235014
+
+        Reviewed by Antti Koivisto.
+
+        When resolving keyframes for a CSS Animation in Style::Resolver::keyframeStylesForAnimation(),
+        we would fill implicit keyframes based on the style at the time the animation was created.
+        However, the underlying style may change at any point while the CSS Animation is in flight,
+        so we should not be filling those values.
+
+        The code that actually resolves animated styles already knows how to handle this case, which
+        we already supported correctly for JS-originated animations. The only thing we need to do is
+        to ensure we fill implicit keyframes when calling getKeyframes().
+
+        This surfaced an issue with KeyframeList::copyKeyframes() where we only copied the styles
+        from one set of keyframes to another, but not the timing function, which would cause a
+        getKeyframes() test to regress. We should also make sure to copy the composite operation
+        for when we support the animation-composition property (see bug 232086).
+
+        Test: imported/w3c/web-platform-tests/css/css-animations/animation-change-underlying-value-changed-in-flight.html
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::getKeyframes):
+        * rendering/style/KeyframeList.cpp:
+        (WebCore::KeyframeList::copyKeyframes):
+        * style/StyleResolver.cpp:
+        (WebCore::Style::Resolver::keyframeStylesForAnimation):
+
 2022-01-09  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -640 +640 @@
         auto computedStyleExtractor = ComputedStyleExtractor(target, false, m_pseudoId);
 
+        KeyframeList computedKeyframes(m_blendingKeyframes.animationName());
+        computedKeyframes.copyKeyframes(m_blendingKeyframes);
+        computedKeyframes.fillImplicitKeyframes(*m_target, m_target->styleResolver(), lastStyleChangeEventStyle, nullptr);
+
         auto keyframeRules = [&]() -> const Vector<Ref<StyleRuleKeyframe>> {
             if (!is<CSSAnimation>(animation()))
@@ -649 +653 @@
                 return { };
 
-            return styleScope->resolver().keyframeRulesForName(m_blendingKeyframes.animationName());
+            return styleScope->resolver().keyframeRulesForName(computedKeyframes.animationName());
         }();
 
@@ -663 +667 @@
 
         // We need to establish which properties are implicit for 0% and 100%.
-        HashSet<CSSPropertyID> zeroKeyframeProperties = m_blendingKeyframes.properties();
-        HashSet<CSSPropertyID> oneKeyframeProperties = m_blendingKeyframes.properties();
+        HashSet<CSSPropertyID> zeroKeyframeProperties = computedKeyframes.properties();
+        HashSet<CSSPropertyID> oneKeyframeProperties = computedKeyframes.properties();
         zeroKeyframeProperties.remove(CSSPropertyCustom);
         oneKeyframeProperties.remove(CSSPropertyCustom);
-        for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
-            auto& keyframe = m_blendingKeyframes[i];
+        for (size_t i = 0; i < computedKeyframes.size(); ++i) {
+            auto& keyframe = computedKeyframes[i];
             if (!keyframe.key()) {
                 for (auto cssPropertyId : keyframe.properties())
@@ -678 +682 @@
         }
 
-        for (size_t i = 0; i < m_blendingKeyframes.size(); ++i) {
+        for (size_t i = 0; i < computedKeyframes.size(); ++i) {
             // 1. Initialize a dictionary object, output keyframe, using the following definition:
             //
@@ -688 +692 @@
             // };
 
-            auto& keyframe = m_blendingKeyframes[i];
+            auto& keyframe = computedKeyframes[i];
 
             // 2. Set offset, computedOffset, easing members of output keyframe to the respective values keyframe offset, computed keyframe offset,
@@ -696 +700 @@
             computedKeyframe.computedOffset = keyframe.key();
             // For CSS transitions, all keyframes should return "linear" since the effect's global timing function applies.
-            computedKeyframe.easing = is<CSSTransition>(animation()) ? "linear" : timingFunctionForKeyframeAtIndex(i)->cssText();
+            computedKeyframe.easing = is<CSSTransition>(animation()) ? "linear" : timingFunctionForBlendingKeyframe(keyframe)->cssText();
 
             auto outputKeyframe = convertDictionaryToJS(lexicalGlobalObject, *jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject), computedKeyframe);

trunk/Source/WebCore/rendering/style/KeyframeList.cpp

@@ -89 +89 @@
         for (auto propertyId : keyframe.properties())
             keyframeValue.addProperty(propertyId);
+        keyframeValue.setTimingFunction(keyframe.timingFunction());
+        keyframeValue.setCompositeOperation(keyframe.compositeOperation());
         insert(WTFMove(keyframeValue));
     }

trunk/Source/WebCore/style/StyleResolver.cpp

@@ -400 +400 @@
         }
     }
-
-    list.fillImplicitKeyframes(element, *this, elementStyle, context.parentStyle);
 }