Changeset 274165 in webkit

Timestamp:

Mar 9, 2021 12:09:49 PM (17 months ago)

Author:

graouts@webkit.org

Message:

[Web Animations] setKeyframes does not preserve animation's current offset
​https://bugs.webkit.org/show_bug.cgi?id=222939
<rdar://problem/75207793>

Reviewed by Dean Jackson.

Source/WebCore:

Test: webanimations/set-keyframes-after-animation-completion.html

When setKeyframes() is called on a KeyframeEffect, it clears the existing blending keyframes
created for the style system which will be re-populated on the next style update.

When an animation completes, it becomes eligible for being removed unless its effect is the
top-most effect in the target element's effect stack affecting one of the CSS properties
animated by effects in the stack. To determine what properties an effect animates, the method
KeyframeEffect::animatedProperties() is used.

Until now, KeyframeEffect::animatedProperties() simply returned the properties from the
blending keyframes. As such, if blending keyframes are empty, that method returns an empty
set of properties.

Since blending keyframes are cleared by a call to setKeyframes(), calling this method on
the effect of an animation that just finished will remove that animation and the effect
will no longer be applied when updating styles.

To fix this, we add a new instance variable on KeyframeEffect to store the list of properties
affected by the effect from the keyframes parsed by setKeyframes() until we generate the
blending keyframes during the next style update.

As soon as we generate the blending keyframes and setBlendingKeyframes() is called, we clear
that new property. This guarantees that no matter how keyframes are specified – CSS Transitions,
CSS Animations or Web Animations JS API – the animatedProperties() method returns the set
of affected properties.

• animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::animatedProperties):
(WebCore::KeyframeEffect::setBlendingKeyframes):

• animation/KeyframeEffect.h:

(WebCore::KeyframeEffect::animatedProperties const): Deleted.

LayoutTests:

Add a new test that checks that updating keyframes after an animation has completed
correctly updates styles accounting for the new keyframes.

• webanimations/set-keyframes-after-animation-completion-expected.html: Added.
• webanimations/set-keyframes-after-animation-completion.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/webanimations/set-keyframes-after-animation-completion-expected.html (added)
• LayoutTests/webanimations/set-keyframes-after-animation-completion.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (2 diffs)
• Source/WebCore/animation/KeyframeEffect.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-03-09  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] setKeyframes does not preserve animation's current offset
+        https://bugs.webkit.org/show_bug.cgi?id=222939
+        <rdar://problem/75207793>
+
+        Reviewed by Dean Jackson.
+
+        Add a new test that checks that updating keyframes after an animation has completed
+        correctly updates styles accounting for the new keyframes.
+
+        * webanimations/set-keyframes-after-animation-completion-expected.html: Added.
+        * webanimations/set-keyframes-after-animation-completion.html: Added.
+
 2021-03-09  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-03-09  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] setKeyframes does not preserve animation's current offset
+        https://bugs.webkit.org/show_bug.cgi?id=222939
+        <rdar://problem/75207793>
+
+        Reviewed by Dean Jackson.
+
+        Test: webanimations/set-keyframes-after-animation-completion.html
+
+        When setKeyframes() is called on a KeyframeEffect, it clears the existing blending keyframes
+        created for the style system which will be re-populated on the next style update. 
+
+        When an animation completes, it becomes eligible for being removed unless its effect is the
+        top-most effect in the target element's effect stack affecting one of the CSS properties
+        animated by effects in the stack. To determine what properties an effect animates, the method
+        KeyframeEffect::animatedProperties() is used.
+        
+        Until now, KeyframeEffect::animatedProperties() simply returned the properties from the
+        blending keyframes. As such, if blending keyframes are empty, that method returns an empty
+        set of properties.
+
+        Since blending keyframes are cleared by a call to setKeyframes(), calling this method on
+        the effect of an animation that just finished will remove that animation and the effect
+        will no longer be applied when updating styles.
+
+        To fix this, we add a new instance variable on KeyframeEffect to store the list of properties
+        affected by the effect from the keyframes parsed by setKeyframes() until we generate the
+        blending keyframes during the next style update.
+
+        As soon as we generate the blending keyframes and setBlendingKeyframes() is called, we clear
+        that new property. This guarantees that no matter how keyframes are specified – CSS Transitions,
+        CSS Animations or Web Animations JS API – the animatedProperties() method returns the set
+        of affected properties.
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::animatedProperties):
+        (WebCore::KeyframeEffect::setBlendingKeyframes):
+        * animation/KeyframeEffect.h:
+        (WebCore::KeyframeEffect::animatedProperties const): Deleted.
+
 2021-03-09  Said Abou-Hallawa  <said@apple.com>
 

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -835 +835 @@
 }
 
+const HashSet<CSSPropertyID>& KeyframeEffect::animatedProperties()
+{
+    if (!m_blendingKeyframes.isEmpty())
+        return m_blendingKeyframes.properties();
+
+    if (m_animatedProperties.isEmpty()) {
+        for (auto& keyframe : m_parsedKeyframes) {
+            for (auto keyframeProperty : keyframe.unparsedStyle.keys())
+                m_animatedProperties.add(keyframeProperty);
+        }
+    }
+
+    return m_animatedProperties;
+}
+
 bool KeyframeEffect::animatesProperty(CSSPropertyID property) const
 {
@@ -877 +892 @@
 {
     m_blendingKeyframes = WTFMove(blendingKeyframes);
+    m_animatedProperties.clear();
 
     computedNeedsForcedLayout();

trunk/Source/WebCore/animation/KeyframeEffect.h

@@ -157 +157 @@
     void computeDeclarativeAnimationBlendingKeyframes(const RenderStyle* oldStyle, const RenderStyle& newStyle, const RenderStyle* parentElementStyle);
     const KeyframeList& blendingKeyframes() const { return m_blendingKeyframes; }
-    const HashSet<CSSPropertyID>& animatedProperties() const { return m_blendingKeyframes.properties(); }
+    const HashSet<CSSPropertyID>& animatedProperties();
     bool animatesProperty(CSSPropertyID) const;
 
@@ -216 +216 @@
 
     KeyframeList m_blendingKeyframes { emptyString() };
+    HashSet<CSSPropertyID> m_animatedProperties;
     Vector<ParsedKeyframe> m_parsedKeyframes;
     Vector<AcceleratedAction> m_pendingAcceleratedActions;