Changeset 287769 in webkit

Timestamp:

Jan 7, 2022 11:22:52 AM (6 months ago)

Author:

graouts@webkit.org

Message:

Inserting a new @keyframes rule does not start animations that already used this name
​https://bugs.webkit.org/show_bug.cgi?id=234955

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Mark WPT progression.

• web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt:

Source/WebCore:

In bug 234895, we added logic to handle the case where an existing @keyframes rule was manipulated
via the CSSOM API and ensured this would update the keyframes of any existing CSSAnimation currently
referencing this @keyframes rule.

Another WPT tests the case where a @keyframes rule is referenced by an animation prior to existing
in the stylesheet, adding that named @keyframes rule, and then testing a CSSAnimation was created.

To handle this case, we need to track which animation names were ignored during style resolution
due not matching an existing @keyframes rule. We now manage a list of invalid CSS animation names
stored on the KeyframeEffectStack (where we also keep track of the AnimationList last seen during
style resolution) and use that to determine when, even though the previous and current AnimationList
contain the same data, a previously-ignored animation should now be processed due to its referenced
@keyframes rule now existing.

• animation/KeyframeEffectStack.cpp:

(WebCore::KeyframeEffectStack::clearInvalidCSSAnimationNames):
(WebCore::KeyframeEffectStack::hasInvalidCSSAnimationNames const):
(WebCore::KeyframeEffectStack::containsInvalidCSSAnimationName const):
(WebCore::KeyframeEffectStack::addInvalidCSSAnimationName):

• animation/KeyframeEffectStack.h:
• style/Styleable.cpp:

(WebCore::keyframesRuleExistsForAnimation):
(WebCore::Styleable::animationListContainsNewlyValidAnimation const):
(WebCore::Styleable::updateCSSAnimations const):
(WebCore::shouldConsiderAnimation): Deleted.

• style/Styleable.h:

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffectStack.cpp (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffectStack.h (modified) (1 diff)
• Source/WebCore/style/Styleable.cpp (modified) (3 diffs)
• Source/WebCore/style/Styleable.h (modified) (1 diff)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-01-07  Antoine Quint  <graouts@webkit.org>
+
+        Inserting a new @keyframes rule does not start animations that already used this name
+        https://bugs.webkit.org/show_bug.cgi?id=234955
+
+        Reviewed by Antti Koivisto.
+
+        Mark WPT progression.
+
+        * web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt:
+
 2022-01-07  Antoine Quint  <graouts@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt

@@ -9 +9 @@
 PASS getAnimations for CSS Animations with animation-name: none
 PASS getAnimations for CSS Animations with animation-name: missing
-FAIL getAnimations for CSS Animations where the @keyframes rule is added later assert_equals: getAnimations includes Animation when @keyframes rule is added later expected 2 but got 1
+PASS getAnimations for CSS Animations where the @keyframes rule is added later
 PASS getAnimations for CSS Animations with duplicated animation-name
 PASS getAnimations for CSS Animations with empty keyframes rule

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-07  Antoine Quint  <graouts@webkit.org>
+
+        Inserting a new @keyframes rule does not start animations that already used this name
+        https://bugs.webkit.org/show_bug.cgi?id=234955
+
+        Reviewed by Antti Koivisto.
+
+        In bug 234895, we added logic to handle the case where an existing @keyframes rule was manipulated
+        via the CSSOM API and ensured this would update the keyframes of any existing CSSAnimation currently
+        referencing this @keyframes rule.
+
+        Another WPT tests the case where a @keyframes rule is referenced by an animation prior to existing
+        in the stylesheet, adding that named @keyframes rule, and then testing a CSSAnimation was created.
+
+        To handle this case, we need to track which animation names were ignored during style resolution
+        due not matching an existing @keyframes rule. We now manage a list of invalid CSS animation names
+        stored on the KeyframeEffectStack (where we also keep track of the AnimationList last seen during
+        style resolution) and use that to determine when, even though the previous and current AnimationList
+        contain the same data, a previously-ignored animation should now be processed due to its referenced
+        @keyframes rule now existing.
+
+        * animation/KeyframeEffectStack.cpp:
+        (WebCore::KeyframeEffectStack::clearInvalidCSSAnimationNames):
+        (WebCore::KeyframeEffectStack::hasInvalidCSSAnimationNames const):
+        (WebCore::KeyframeEffectStack::containsInvalidCSSAnimationName const):
+        (WebCore::KeyframeEffectStack::addInvalidCSSAnimationName):
+        * animation/KeyframeEffectStack.h:
+        * style/Styleable.cpp:
+        (WebCore::keyframesRuleExistsForAnimation):
+        (WebCore::Styleable::animationListContainsNewlyValidAnimation const):
+        (WebCore::Styleable::updateCSSAnimations const):
+        (WebCore::shouldConsiderAnimation): Deleted.
+        * style/Styleable.h:
+
 2022-01-07  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/animation/KeyframeEffectStack.cpp

@@ -157 +157 @@
 }
 
+void KeyframeEffectStack::clearInvalidCSSAnimationNames()
+{
+    m_invalidCSSAnimationNames.clear();
+}
+
+bool KeyframeEffectStack::hasInvalidCSSAnimationNames() const
+{
+    return !m_invalidCSSAnimationNames.isEmpty();
+}
+
+bool KeyframeEffectStack::containsInvalidCSSAnimationName(const String& name) const
+{
+    return m_invalidCSSAnimationNames.contains(name);
+}
+
+void KeyframeEffectStack::addInvalidCSSAnimationName(const String& name)
+{
+    m_invalidCSSAnimationNames.add(name);
+}
+
 } // namespace WebCore

trunk/Source/WebCore/animation/KeyframeEffectStack.h

@@ -60 +60 @@
     void stopAcceleratingTransformRelatedProperties(UseAcceleratedAction);
 
+    void clearInvalidCSSAnimationNames();
+    bool hasInvalidCSSAnimationNames() const;
+    bool containsInvalidCSSAnimationName(const String&) const;
+    void addInvalidCSSAnimationName(const String&);
+
 private:
     void ensureEffectsAreSorted();
 
     Vector<WeakPtr<KeyframeEffect>> m_effects;
+    HashSet<String> m_invalidCSSAnimationNames;
     RefPtr<const AnimationList> m_cssAnimationList;
     bool m_isSorted { true };
-
 };
 

trunk/Source/WebCore/style/Styleable.cpp

@@ -202 +202 @@
 }
 
-static bool shouldConsiderAnimation(Element& element, const Animation& animation)
-{
-    if (!animation.isValidAnimation())
+static bool keyframesRuleExistsForAnimation(Element& element, const Animation& animation, const String& animationName)
+{
+    auto* styleScope = Style::Scope::forOrdinal(element, animation.nameStyleScopeOrdinal());
+    return styleScope && styleScope->resolver().isAnimationNameValid(animationName);
+}
+
+bool Styleable::animationListContainsNewlyValidAnimation(const AnimationList& animations) const
+{
+    auto& keyframeEffectStack = ensureKeyframeEffectStack();
+    if (!keyframeEffectStack.hasInvalidCSSAnimationNames())
         return false;
 
-    auto& name = animation.name().string;
-    if (name == "none" || name.isEmpty())
-        return false;
-
-    if (auto* styleScope = Style::Scope::forOrdinal(element, animation.nameStyleScopeOrdinal()))
-        return styleScope->resolver().isAnimationNameValid(name);
+    for (auto& animation : animations) {
+        auto& name = animation->name().string;
+        if (name != "none" && !name.isEmpty() && keyframeEffectStack.containsInvalidCSSAnimationName(name) && keyframesRuleExistsForAnimation(element, animation.get(), name))
+            return true;
+    }
 
     return false;
@@ -231 +237 @@
     auto* currentAnimationList = newStyle.animations();
     auto* previousAnimationList = keyframeEffectStack.cssAnimationList();
-    if (!element.hasPendingKeyframesUpdate(pseudoId) && previousAnimationList && !previousAnimationList->isEmpty() && newStyle.hasAnimations() && *(previousAnimationList) == *(newStyle.animations()))
+    if (!element.hasPendingKeyframesUpdate(pseudoId) && previousAnimationList && !previousAnimationList->isEmpty() && newStyle.hasAnimations() && *(previousAnimationList) == *(newStyle.animations()) && !animationListContainsNewlyValidAnimation(*newStyle.animations()))
         return;
 
     CSSAnimationCollection newAnimations;
     auto& previousAnimations = animationsCreatedByMarkup();
+
+    keyframeEffectStack.clearInvalidCSSAnimationNames();
 
     // https://www.w3.org/TR/css-animations-1/#animations
@@ -248 +256 @@
     if (currentAnimationList) {
         for (auto& currentAnimation : makeReversedRange(*currentAnimationList)) {
-            if (!shouldConsiderAnimation(this->element, currentAnimation.get()))
+            if (!currentAnimation->isValidAnimation())
                 continue;
+
+            auto& animationName = currentAnimation->name().string;
+            if (animationName == "none" || animationName.isEmpty())
+                continue;
+
+            if (!keyframesRuleExistsForAnimation(element, currentAnimation.get(), animationName)) {
+                keyframeEffectStack.addInvalidCSSAnimationName(animationName);
+                continue;
+            }
 
             bool foundMatchingAnimation = false;
             for (auto& previousAnimation : previousAnimations) {
-                if (previousAnimation->animationName() == currentAnimation->name().string) {
+                if (previousAnimation->animationName() == animationName) {
                     // Timing properties or play state may have changed so we need to update the backing animation with
                     // the Animation found in the current style.

trunk/Source/WebCore/style/Styleable.h

@@ -151 +151 @@
     }
 
+    bool animationListContainsNewlyValidAnimation(const AnimationList&) const;
+
     void elementWasRemoved() const;