[Web Animations] Support multiple CSS Animations with the same name in animation...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Jan 2020 17:35:45 +0000 (17:35 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Jan 2020 17:35:45 +0000 (17:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206688

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Mark some new WPT progressions.

* web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt:
* web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt:
* web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt:

Source/WebCore:

AnimationTimeline would keep track of registered CSS Animations by name for a given element in m_elementToCSSAnimationByName which would map one CSSAnimation
per String (the animation-name) for a given Element. However, within the same animation-name property, the name of a given @keyframes rules may appear more
than once, and the CSS Animations specification explains how to handle this scenario.

We now correctly handle this by replacing m_elementToCSSAnimationByName with the new m_elementToCSSAnimationsCreatedByMarkupMap which simply maps an Element
to a ListHashSet of CSSAnimation objects. Removing the string that appeared in animation-name to create this animation requires us to keep the AnimationList
used for the last style update for sorting purposes, since having multiple instances of the same string would not allow disambiguation when sorting the
KeyframeEffectStack.

So we also replace m_cssAnimationNames, a Vector<String>, with m_cssAnimationList, a RefPtr<const AnimationList>, and use this to compare Animation objects
stored in the AnimationList against the backing animation of each CSSAnimation.

Storing the AnimationList on the KeyframeEffectStack also has the benefit of allowing us to use this as the previous state when updating CSS Animations in
AnimationTimeline::updateCSSAnimationsForElement(). We used to rely on the previous RenderStyle provided to that function, but it's possible that this style
is null and we would unnecessarily create additional CSSAnimation objects for animations that actually were retained since the last time CSS Animations were
invalidated. We now use the stored AnimationList on the invalidated element's KeyframeEffectStack and create a new animation list that will replace the old
list stored in the m_elementToCSSAnimationsCreatedByMarkupMap map for that element. We can also compare the old list with the new list to find out which
animations are no longer current.

Finally, we refactor things a bit to have some new aliases AnimationCollection and CSSAnimationCollection instead of using ListHashSet<> in our types.

* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::animationWasAddedToElement): Use the new AnimationCollection alias.
(WebCore::AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement): We no longer need to do any work for CSSAnimation here since the
m_elementToCSSAnimationByName map is no more and the m_elementToCSSAnimationsCreatedByMarkupMap that replaces it is updated in updateCSSAnimationsForElement()
and elementWasRemoved().
(WebCore::AnimationTimeline::animationsForElement const): Since animations are correctly sorted accounting for their composite order in KeyframeEffectStack,
call KeyframeEffectStack::sortedEffects() when we're called with Ordering::Sorted.
(WebCore::AnimationTimeline::removeCSSAnimationCreatedByMarkup): New method called by elementWasRemoved() to ensure that when an element is removed, we remove
its CSSAnimation objects from the new m_elementToCSSAnimationsCreatedByMarkupMap and also update the AnimationList on the relevant KeyframeEffectStack.
(WebCore::AnimationTimeline::elementWasRemoved): Call the new removeCSSAnimationCreatedByMarkup() method before canceling a CSSAnimation.
(WebCore::AnimationTimeline::cancelDeclarativeAnimationsForElement): Call the new removeCSSAnimationCreatedByMarkup() method before canceling a CSSAnimation.
(WebCore::AnimationTimeline::updateCSSAnimationsForElement): Use the AnimationList recoreded on the relevant KeyframeEffectStack to determine which CSSAnimation
objects to create, cancel or merely update depending on the AnimationList in the current style.
* animation/AnimationTimeline.h:
* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::getAnimations const): Use compareAnimationsByCompositeOrder() to correctly sort CSS Animations since they are no longer guaranteed
to be stored in the relevant map in the expected order.
* animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::ensureEffectsAreSorted): Use the new m_cssAnimationList instead of the old m_cssAnimationNames when sorting effects.
(WebCore::KeyframeEffectStack::setCSSAnimationList):
(WebCore::KeyframeEffectStack::setCSSAnimationNames): Deleted.
* animation/KeyframeEffectStack.h:
(WebCore::KeyframeEffectStack::cssAnimationList const):
(WebCore::KeyframeEffectStack::cssAnimationNames const): Deleted.
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::commitStyles): Use the new KeyframeEffectStack::cssAnimationList() instead of the old KeyframeEffectStack::cssAnimationNames().
* animation/WebAnimationUtilities.cpp:
(WebCore::compareAnimationsByCompositeOrder): Update the composite order comparison utility to use an AnimationList rather than a list of animation names.
* animation/WebAnimationUtilities.h:
* platform/animation/AnimationList.h:
(WebCore::AnimationList::copy const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@255076 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt
LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt
LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/AnimationTimeline.h
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/KeyframeEffectStack.cpp
Source/WebCore/animation/KeyframeEffectStack.h
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimationUtilities.cpp
Source/WebCore/animation/WebAnimationUtilities.h
Source/WebCore/platform/animation/AnimationList.h

index e24dcb1..cefe68d 100644 (file)
@@ -1,3 +1,16 @@
+2020-01-23  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Support multiple CSS Animations with the same name in animation-name
+        https://bugs.webkit.org/show_bug.cgi?id=206688
+
+        Reviewed by Dean Jackson.
+
+        Mark some new WPT progressions.
+
+        * web-platform-tests/css/css-animations/Element-getAnimations-dynamic-changes.tentative-expected.txt:
+        * web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt:
+        * web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt:
+
 2020-01-24  Rossana Monteriso  <rmonteriso@igalia.com>
 
         Import 2 sets of grid-align-tests from WPT
index 9419924..3476924 100644 (file)
@@ -1,7 +1,7 @@
 
 PASS Animations preserve their startTime when changed 
 PASS Updated Animations maintain their order in the list 
-FAIL Only the startTimes of existing animations are preserved assert_true: New Animation is prepended to start of list expected true got false
+PASS Only the startTimes of existing animations are preserved 
 FAIL Animations are removed from the start of the list while preserving the state of existing Animations assert_equals: List of Animations was trimmed expected 1 but got 2
-FAIL Animation state is preserved when interleaving animations in list assert_not_equals: New Animations are added to start of the list got disallowed value object "[object CSSAnimation]"
+FAIL Animation state is preserved when interleaving animations in list assert_equals: Second Animation remains in same position after interleaving expected object "[object CSSAnimation]" but got object "[object CSSAnimation]"
 
index 0587078..fcf752b 100644 (file)
@@ -15,7 +15,7 @@ PASS getAnimations for CSS animations in delay phase
 PASS getAnimations for zero-duration CSS Animations 
 PASS getAnimations returns objects with the same identity 
 PASS getAnimations for CSS Animations that are canceled 
-FAIL getAnimations for CSS Animations follows animation-name order assert_equals: animation order after prepending to list expected "anim1" but got "anim2"
+PASS getAnimations for CSS Animations follows animation-name order 
 PASS { subtree: false } on a leaf element returns the element's animations and ignore pseudo-elements 
 FAIL { subtree: true } on a leaf element returns the element's animations and its pseudo-elements' animations assert_equals: getAnimations({ subtree: true }) should return animations on pseudo-elements expected 3 but got 1
 PASS { subtree: false } on an element with a child returns only the element's animations 
index f1b486d..d748b7c 100644 (file)
@@ -11,7 +11,7 @@ PASS Removes an animation after updating the fill mode of another animation
 PASS Removes an animation after updating its fill mode 
 PASS Removes an animation after updating another animation's effect to one with different timing 
 PASS Removes an animation after updating its effect to one with different timing 
-FAIL Removes an animation after updating another animation's timeline assert_equals: expected "removed" but got "active"
+PASS Removes an animation after updating another animation's timeline 
 PASS Removes an animation after updating its timeline 
 PASS Removes an animation after updating another animation's effect's properties 
 PASS Removes an animation after updating its effect's properties 
index acb5503..9dff8c8 100644 (file)
@@ -1,3 +1,63 @@
+2020-01-23  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Support multiple CSS Animations with the same name in animation-name
+        https://bugs.webkit.org/show_bug.cgi?id=206688
+
+        Reviewed by Dean Jackson.
+
+        AnimationTimeline would keep track of registered CSS Animations by name for a given element in m_elementToCSSAnimationByName which would map one CSSAnimation
+        per String (the animation-name) for a given Element. However, within the same animation-name property, the name of a given @keyframes rules may appear more
+        than once, and the CSS Animations specification explains how to handle this scenario.
+
+        We now correctly handle this by replacing m_elementToCSSAnimationByName with the new m_elementToCSSAnimationsCreatedByMarkupMap which simply maps an Element
+        to a ListHashSet of CSSAnimation objects. Removing the string that appeared in animation-name to create this animation requires us to keep the AnimationList
+        used for the last style update for sorting purposes, since having multiple instances of the same string would not allow disambiguation when sorting the
+        KeyframeEffectStack.
+
+        So we also replace m_cssAnimationNames, a Vector<String>, with m_cssAnimationList, a RefPtr<const AnimationList>, and use this to compare Animation objects
+        stored in the AnimationList against the backing animation of each CSSAnimation.
+
+        Storing the AnimationList on the KeyframeEffectStack also has the benefit of allowing us to use this as the previous state when updating CSS Animations in
+        AnimationTimeline::updateCSSAnimationsForElement(). We used to rely on the previous RenderStyle provided to that function, but it's possible that this style
+        is null and we would unnecessarily create additional CSSAnimation objects for animations that actually were retained since the last time CSS Animations were
+        invalidated. We now use the stored AnimationList on the invalidated element's KeyframeEffectStack and create a new animation list that will replace the old
+        list stored in the m_elementToCSSAnimationsCreatedByMarkupMap map for that element. We can also compare the old list with the new list to find out which
+        animations are no longer current.
+
+        Finally, we refactor things a bit to have some new aliases AnimationCollection and CSSAnimationCollection instead of using ListHashSet<> in our types.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::animationWasAddedToElement): Use the new AnimationCollection alias.
+        (WebCore::AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement): We no longer need to do any work for CSSAnimation here since the
+        m_elementToCSSAnimationByName map is no more and the m_elementToCSSAnimationsCreatedByMarkupMap that replaces it is updated in updateCSSAnimationsForElement()
+        and elementWasRemoved().
+        (WebCore::AnimationTimeline::animationsForElement const): Since animations are correctly sorted accounting for their composite order in KeyframeEffectStack,
+        call KeyframeEffectStack::sortedEffects() when we're called with Ordering::Sorted. 
+        (WebCore::AnimationTimeline::removeCSSAnimationCreatedByMarkup): New method called by elementWasRemoved() to ensure that when an element is removed, we remove
+        its CSSAnimation objects from the new m_elementToCSSAnimationsCreatedByMarkupMap and also update the AnimationList on the relevant KeyframeEffectStack.
+        (WebCore::AnimationTimeline::elementWasRemoved): Call the new removeCSSAnimationCreatedByMarkup() method before canceling a CSSAnimation.
+        (WebCore::AnimationTimeline::cancelDeclarativeAnimationsForElement): Call the new removeCSSAnimationCreatedByMarkup() method before canceling a CSSAnimation.
+        (WebCore::AnimationTimeline::updateCSSAnimationsForElement): Use the AnimationList recoreded on the relevant KeyframeEffectStack to determine which CSSAnimation
+        objects to create, cancel or merely update depending on the AnimationList in the current style. 
+        * animation/AnimationTimeline.h:
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::getAnimations const): Use compareAnimationsByCompositeOrder() to correctly sort CSS Animations since they are no longer guaranteed
+        to be stored in the relevant map in the expected order.
+        * animation/KeyframeEffectStack.cpp:
+        (WebCore::KeyframeEffectStack::ensureEffectsAreSorted): Use the new m_cssAnimationList instead of the old m_cssAnimationNames when sorting effects.
+        (WebCore::KeyframeEffectStack::setCSSAnimationList):
+        (WebCore::KeyframeEffectStack::setCSSAnimationNames): Deleted.
+        * animation/KeyframeEffectStack.h:
+        (WebCore::KeyframeEffectStack::cssAnimationList const):
+        (WebCore::KeyframeEffectStack::cssAnimationNames const): Deleted.
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::commitStyles): Use the new KeyframeEffectStack::cssAnimationList() instead of the old KeyframeEffectStack::cssAnimationNames().
+        * animation/WebAnimationUtilities.cpp:
+        (WebCore::compareAnimationsByCompositeOrder): Update the composite order comparison utility to use an AnimationList rather than a list of animation names.
+        * animation/WebAnimationUtilities.h:
+        * platform/animation/AnimationList.h:
+        (WebCore::AnimationList::copy const):
+
 2020-01-24  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][Painting] Add Display::Run cleanup to TextPainter::clearGlyphDisplayLists
index 0ac880f..2908520 100644 (file)
@@ -108,7 +108,7 @@ void AnimationTimeline::animationWasAddedToElement(WebAnimation& animation, Elem
             return m_elementToCSSAnimationsMap;
         return m_elementToAnimationsMap;
     }().ensure(&element, [] {
-        return ListHashSet<RefPtr<WebAnimation>> { };
+        return AnimationCollection { };
     }).iterator->value.add(&animation);
 }
 
@@ -161,18 +161,7 @@ void AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement(WebA
 {
     ASSERT(is<DeclarativeAnimation>(animation));
 
-    if (is<CSSAnimation>(animation)) {
-        auto iterator = m_elementToCSSAnimationByName.find(&element);
-        if (iterator != m_elementToCSSAnimationByName.end()) {
-            auto& cssAnimationsByName = iterator->value;
-            auto& name = downcast<CSSAnimation>(animation).animationName();
-            if (cssAnimationsByName.get(name) == &animation) {
-                cssAnimationsByName.remove(name);
-                if (cssAnimationsByName.isEmpty())
-                    m_elementToCSSAnimationByName.remove(&element);
-            }
-        }
-    } else if (is<CSSTransition>(animation)) {
+    if (is<CSSTransition>(animation)) {
         auto& transition = downcast<CSSTransition>(animation);
         if (!removeCSSTransitionFromMap(transition, element, m_elementToRunningCSSTransitionByCSSPropertyID))
             removeCSSTransitionFromMap(transition, element, m_elementToCompletedCSSTransitionByCSSPropertyID);
@@ -182,47 +171,68 @@ void AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement(WebA
 Vector<RefPtr<WebAnimation>> AnimationTimeline::animationsForElement(Element& element, Ordering ordering) const
 {
     Vector<RefPtr<WebAnimation>> animations;
-    if (m_elementToCSSTransitionsMap.contains(&element)) {
-        const auto& cssTransitions = m_elementToCSSTransitionsMap.get(&element);
-        if (ordering == Ordering::Sorted) {
-            auto sortedCSSTransitions = copyToVector(cssTransitions);
-            std::sort(sortedCSSTransitions.begin(), sortedCSSTransitions.end(), [](auto& lhs, auto& rhs) {
-                // Sort transitions first by their generation time, and then by transition-property.
-                // https://drafts.csswg.org/css-transitions-2/#animation-composite-order
-                auto* lhsTransition = downcast<CSSTransition>(lhs.get());
-                auto* rhsTransition = downcast<CSSTransition>(rhs.get());
-                if (lhsTransition->generationTime() != rhsTransition->generationTime())
-                    return lhsTransition->generationTime() < rhsTransition->generationTime();
-                return lhsTransition->transitionProperty().utf8() < rhsTransition->transitionProperty().utf8();
-            });
-            animations.appendVector(sortedCSSTransitions);
-        } else
+
+    if (ordering == Ordering::Sorted) {
+        if (element.hasKeyframeEffects()) {
+            for (auto& effect : element.ensureKeyframeEffectStack().sortedEffects())
+                animations.append(effect->animation());
+        }
+    } else {
+        if (m_elementToCSSTransitionsMap.contains(&element)) {
+            const auto& cssTransitions = m_elementToCSSTransitionsMap.get(&element);
             animations.appendRange(cssTransitions.begin(), cssTransitions.end());
-    }
-    if (m_elementToCSSAnimationsMap.contains(&element)) {
-        const auto& cssAnimations = m_elementToCSSAnimationsMap.get(&element);
-        animations.appendRange(cssAnimations.begin(), cssAnimations.end());
-    }
-    if (m_elementToAnimationsMap.contains(&element)) {
-        const auto& webAnimations = m_elementToAnimationsMap.get(&element);
-        if (ordering == Ordering::Sorted) {
-            auto sortedWebAnimations = copyToVector(webAnimations);
-            std::sort(sortedWebAnimations.begin(), sortedWebAnimations.end(), [](auto& lha, auto& rha) {
-                return lha->globalPosition() < rha->globalPosition();
-            });
-            animations.appendVector(sortedWebAnimations);
-        } else
+        }
+        if (m_elementToCSSAnimationsMap.contains(&element)) {
+            const auto& cssAnimations = m_elementToCSSAnimationsMap.get(&element);
+            animations.appendRange(cssAnimations.begin(), cssAnimations.end());
+        }
+        if (m_elementToAnimationsMap.contains(&element)) {
+            const auto& webAnimations = m_elementToAnimationsMap.get(&element);
             animations.appendRange(webAnimations.begin(), webAnimations.end());
+        }
     }
+
     return animations;
 }
 
+void AnimationTimeline::removeCSSAnimationCreatedByMarkup(Element& element, CSSAnimation& cssAnimation)
+{
+    auto iterator = m_elementToCSSAnimationsCreatedByMarkupMap.find(&element);
+    if (iterator != m_elementToCSSAnimationsCreatedByMarkupMap.end()) {
+        auto& cssAnimations = iterator->value;
+        cssAnimations.remove(&cssAnimation);
+        if (!cssAnimations.size())
+            m_elementToCSSAnimationsCreatedByMarkupMap.remove(iterator);
+    }
+
+    if (!element.hasKeyframeEffects())
+        return;
+
+    auto& keyframeEffectStack = element.ensureKeyframeEffectStack();
+    auto* cssAnimationList = keyframeEffectStack.cssAnimationList();
+    if (!cssAnimationList || cssAnimationList->isEmpty())
+        return;
+
+    auto& backingAnimation = cssAnimation.backingAnimation();
+    for (size_t i = 0; i < cssAnimationList->size(); ++i) {
+        if (cssAnimationList->animation(i) == backingAnimation) {
+            auto newAnimationList = cssAnimationList->copy();
+            newAnimationList->remove(i);
+            keyframeEffectStack.setCSSAnimationList(WTFMove(newAnimationList));
+            return;
+        }
+    }
+}
+
 void AnimationTimeline::elementWasRemoved(Element& element)
 {
     for (auto& cssTransition : m_elementToCSSTransitionsMap.get(&element))
         cssTransition->cancel(WebAnimation::Silently::Yes);
-    for (auto& cssAnimation : m_elementToCSSAnimationsMap.get(&element))
+    for (auto& cssAnimation : m_elementToCSSAnimationsMap.get(&element)) {
+        if (is<CSSAnimation>(cssAnimation))
+            removeCSSAnimationCreatedByMarkup(element, downcast<CSSAnimation>(*cssAnimation));
         cssAnimation->cancel(WebAnimation::Silently::Yes);
+    }
 }
 
 void AnimationTimeline::removeAnimationsForElement(Element& element)
@@ -235,8 +245,11 @@ void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
 {
     for (auto& cssTransition : m_elementToCSSTransitionsMap.get(&element))
         cssTransition->cancel();
-    for (auto& cssAnimation : m_elementToCSSAnimationsMap.get(&element))
+    for (auto& cssAnimation : m_elementToCSSAnimationsMap.get(&element)) {
+        if (is<CSSAnimation>(cssAnimation))
+            removeCSSAnimationCreatedByMarkup(element, downcast<CSSAnimation>(*cssAnimation));
         cssAnimation->cancel();
+    }
 }
 
 static bool shouldConsiderAnimation(Element& element, const Animation& animation)
@@ -258,66 +271,75 @@ static bool shouldConsiderAnimation(Element& element, const Animation& animation
 
 void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const RenderStyle* currentStyle, const RenderStyle& afterChangeStyle)
 {
-    Vector<String> animationNames;
+    auto& keyframeEffectStack = element.ensureKeyframeEffectStack();
 
     // In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
-    if (currentStyle && currentStyle->hasAnimations() && currentStyle->display() != DisplayType::None && afterChangeStyle.display() == DisplayType::None) {
-        if (m_elementToCSSAnimationByName.contains(&element)) {
-            for (const auto& cssAnimationsByNameMapItem : m_elementToCSSAnimationByName.take(&element))
-                cssAnimationsByNameMapItem.value->cancelFromStyle();
+    if (currentStyle && currentStyle->display() != DisplayType::None && afterChangeStyle.display() == DisplayType::None) {
+        auto iterator = m_elementToCSSAnimationsCreatedByMarkupMap.find(&element);
+        if (iterator != m_elementToCSSAnimationsCreatedByMarkupMap.end()) {
+            auto& cssAnimations = iterator->value;
+            for (auto& cssAnimation : cssAnimations)
+                cssAnimation->cancelFromStyle();
+            m_elementToCSSAnimationsCreatedByMarkupMap.remove(iterator);
         }
-        element.ensureKeyframeEffectStack().setCSSAnimationNames(WTFMove(animationNames));
+        keyframeEffectStack.setCSSAnimationList(nullptr);
         return;
     }
 
-    if (currentStyle && currentStyle->hasAnimations() && afterChangeStyle.hasAnimations() && *(currentStyle->animations()) == *(afterChangeStyle.animations()))
+    auto* currentAnimationList = afterChangeStyle.animations();
+    auto* previousAnimationList = keyframeEffectStack.cssAnimationList();
+    if (previousAnimationList && !previousAnimationList->isEmpty() && afterChangeStyle.hasAnimations() && *(previousAnimationList) == *(afterChangeStyle.animations()))
         return;
 
-    // First, compile the list of animation names that were applied to this element up to this point.
-    HashSet<String> namesOfPreviousAnimations;
-    if (currentStyle && currentStyle->hasAnimations()) {
-        auto* previousAnimations = currentStyle->animations();
-        for (size_t i = 0; i < previousAnimations->size(); ++i) {
-            auto& previousAnimation = previousAnimations->animation(i);
-            if (shouldConsiderAnimation(element, previousAnimation))
-                namesOfPreviousAnimations.add(previousAnimation.name());
-        }
-    }
-
-    // Create or get the CSSAnimations by animation name map for this element.
-    auto& cssAnimationsByName = m_elementToCSSAnimationByName.ensure(&element, [] {
-        return HashMap<String, RefPtr<CSSAnimation>> { };
+    CSSAnimationCollection newAnimations;
+    auto& previousAnimations = m_elementToCSSAnimationsCreatedByMarkupMap.ensure(&element, [] {
+        return CSSAnimationCollection { };
     }).iterator->value;
 
-    if (auto* currentAnimations = afterChangeStyle.animations()) {
-        for (size_t i = 0; i < currentAnimations->size(); ++i) {
-            auto& currentAnimation = currentAnimations->animation(i);
-            auto& name = currentAnimation.name();
-            if (namesOfPreviousAnimations.contains(name)) {
-                // We've found the name of this animation in our list of previous animations, this means we've already
-                // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current
-                // animation object for this animation name.
-                if (auto cssAnimation = cssAnimationsByName.get(name))
-                    cssAnimation->setBackingAnimation(currentAnimation);
-                animationNames.append(name);
-            } else if (shouldConsiderAnimation(element, currentAnimation)) {
-                // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it.
-                cssAnimationsByName.set(name, CSSAnimation::create(element, currentAnimation, currentStyle, afterChangeStyle));
-                animationNames.append(name);
+    // https://www.w3.org/TR/css-animations-1/#animations
+    // The same @keyframes rule name may be repeated within an animation-name. Changes to the animation-name update existing
+    // animations by iterating over the new list of animations from last to first, and, for each animation, finding the last
+    // matching animation in the list of existing animations. If a match is found, the existing animation is updated using the
+    // animation properties corresponding to its position in the new list of animations, whilst maintaining its current playback
+    // time as described above. The matching animation is removed from the existing list of animations such that it will not match
+    // twice. If a match is not found, a new animation is created. As a result, updating animation-name from ‘a’ to ‘a, a’ will
+    // cause the existing animation for ‘a’ to become the second animation in the list and a new animation will be created for the
+    // first item in the list.
+    if (currentAnimationList) {
+        for (size_t i = currentAnimationList->size(); i > 0; --i) {
+            auto& currentAnimation = currentAnimationList->animation(i - 1);
+            if (!shouldConsiderAnimation(element, currentAnimation))
+                continue;
+
+            bool foundMatchingAnimation = false;
+            for (auto& previousAnimation : previousAnimations) {
+                if (previousAnimation->animationName() == currentAnimation.name()) {
+                    // Timing properties or play state may have changed so we need to update the backing animation with
+                    // the Animation found in the current style.
+                    previousAnimation->setBackingAnimation(currentAnimation);
+                    newAnimations.add(previousAnimation);
+                    // Remove the matched animation from the list of previous animations so we may not match it again.
+                    previousAnimations.remove(previousAnimation);
+                    foundMatchingAnimation = true;
+                    break;
+                }
             }
-            // Remove the name of this animation from our list since it's now known to be current.
-            namesOfPreviousAnimations.remove(name);
+
+            if (!foundMatchingAnimation)
+                newAnimations.add(CSSAnimation::create(element, currentAnimation, currentStyle, afterChangeStyle));
         }
     }
 
-    // The animations names left in namesOfPreviousAnimations are now known to no longer apply so we need to
-    // remove the CSSAnimation object created for them.
-    for (const auto& nameOfAnimationToRemove : namesOfPreviousAnimations) {
-        if (auto animation = cssAnimationsByName.take(nameOfAnimationToRemove))
-            animation->cancelFromStyle();
+    // Any animation found in previousAnimations but not found in newAnimations is not longer current and should be canceled.
+    for (auto& previousAnimation : previousAnimations) {
+        if (!newAnimations.contains(previousAnimation)) {
+            if (previousAnimation->owningElement())
+                previousAnimation->cancelFromStyle();
+        }
     }
 
-    element.ensureKeyframeEffectStack().setCSSAnimationNames(WTFMove(animationNames));
+    m_elementToCSSAnimationsCreatedByMarkupMap.set(&element, WTFMove(newAnimations));
+    keyframeEffectStack.setCSSAnimationList(currentAnimationList);
 }
 
 RefPtr<WebAnimation> AnimationTimeline::cssAnimationForElementAndProperty(Element& element, CSSPropertyID property)
index 3141cf5..9837d47 100644 (file)
@@ -69,7 +69,8 @@ public:
     void updateCSSAnimationsForElement(Element&, const RenderStyle* currentStyle, const RenderStyle& afterChangeStyle);
     void updateCSSTransitionsForElement(Element&, const RenderStyle& currentStyle, const RenderStyle& afterChangeStyle);
 
-    using ElementToAnimationsMap = HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>;
+    using AnimationCollection = ListHashSet<RefPtr<WebAnimation>>;
+    using ElementToAnimationsMap = HashMap<Element*, AnimationCollection>;
     using PropertyToTransitionMap = HashMap<CSSPropertyID, RefPtr<CSSTransition>>;
 
     virtual ~AnimationTimeline();
@@ -78,19 +79,23 @@ protected:
     explicit AnimationTimeline();
 
     Vector<WeakPtr<WebAnimation>> m_allAnimations;
-    ListHashSet<RefPtr<WebAnimation>> m_animations;
+    AnimationCollection m_animations;
     HashMap<Element*, PropertyToTransitionMap> m_elementToCompletedCSSTransitionByCSSPropertyID;
 
 private:
+    using CSSAnimationCollection = ListHashSet<RefPtr<CSSAnimation>>;
+    using ElementToCSSAnimationsMap = HashMap<Element*, CSSAnimationCollection>;
+
     void updateGlobalPosition(WebAnimation&);
     RefPtr<WebAnimation> cssAnimationForElementAndProperty(Element&, CSSPropertyID);
     PropertyToTransitionMap& ensureRunningTransitionsByProperty(Element&);
     void updateCSSTransitionsForElementAndProperty(Element&, CSSPropertyID, const RenderStyle& currentStyle, const RenderStyle& afterChangeStyle, PropertyToTransitionMap&, PropertyToTransitionMap&, const MonotonicTime);
+    void removeCSSAnimationCreatedByMarkup(Element&, CSSAnimation&);
 
     ElementToAnimationsMap m_elementToAnimationsMap;
     ElementToAnimationsMap m_elementToCSSAnimationsMap;
     ElementToAnimationsMap m_elementToCSSTransitionsMap;
-    HashMap<Element*, HashMap<String, RefPtr<CSSAnimation>>> m_elementToCSSAnimationByName;
+    ElementToCSSAnimationsMap m_elementToCSSAnimationsCreatedByMarkupMap;
     HashMap<Element*, PropertyToTransitionMap> m_elementToRunningCSSTransitionByCSSPropertyID;
 
     Markable<Seconds, Seconds::MarkableTraits> m_currentTime;
index 4fe9980..18db091 100644 (file)
@@ -177,8 +177,7 @@ Vector<RefPtr<WebAnimation>> DocumentTimeline::getAnimations() const
             return compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder(lhsOwningElement, rhsOwningElement);
 
         // Otherwise, sort A and B based on their position in the computed value of the animation-name property of the (common) owning element.
-        // In our case, this matches the time at which the animations were created and thus their relative position in m_allAnimations.
-        return false;
+        return compareAnimationsByCompositeOrder(*lhs, *rhs, lhsOwningElement->ensureKeyframeEffectStack().cssAnimationList());
     });
 
     std::sort(webAnimations.begin(), webAnimations.end(), [](auto& lhs, auto& rhs) {
index ede32a3..4d623b2 100644 (file)
@@ -78,15 +78,15 @@ void KeyframeEffectStack::ensureEffectsAreSorted()
         ASSERT(lhsAnimation);
         ASSERT(rhsAnimation);
 
-        return compareAnimationsByCompositeOrder(*lhsAnimation, *rhsAnimation, m_cssAnimationNames);
+        return compareAnimationsByCompositeOrder(*lhsAnimation, *rhsAnimation, m_cssAnimationList.get());
     });
 
     m_isSorted = true;
 }
 
-void KeyframeEffectStack::setCSSAnimationNames(Vector<String>&& animationNames)
+void KeyframeEffectStack::setCSSAnimationList(RefPtr<const AnimationList>&& cssAnimationList)
 {
-    m_cssAnimationNames = WTFMove(animationNames);
+    m_cssAnimationList = WTFMove(cssAnimationList);
     // Since the list of animation names has changed, the sorting order of the animation effects may have changed as well.
     m_isSorted = false;
 }
index 051199a..ae7f5c6 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "AnimationList.h"
 #include <wtf/Vector.h>
 #include <wtf/WeakPtr.h>
 
@@ -42,14 +43,14 @@ public:
     void removeEffect(KeyframeEffect&);
     bool hasEffects() const { return !m_effects.isEmpty(); }
     Vector<WeakPtr<KeyframeEffect>> sortedEffects();
-    Vector<String> cssAnimationNames() const { return m_cssAnimationNames; }
-    void setCSSAnimationNames(Vector<String>&&);
+    const AnimationList* cssAnimationList() const { return m_cssAnimationList.get(); }
+    void setCSSAnimationList(RefPtr<const AnimationList>&&);
 
 private:
     void ensureEffectsAreSorted();
 
     Vector<WeakPtr<KeyframeEffect>> m_effects;
-    Vector<String> m_cssAnimationNames;
+    RefPtr<const AnimationList> m_cssAnimationList;
     bool m_isSorted { true };
 
 };
index c9e0756..3b8790a 100644 (file)
@@ -1339,7 +1339,7 @@ ExceptionOr<void> WebAnimation::commitStyles()
     inlineStyle->setCssText(styledElement.getAttribute("style"));
 
     auto& keyframeStack = styledElement.ensureKeyframeEffectStack();
-    auto cssAnimationNames = keyframeStack.cssAnimationNames();
+    auto* cssAnimationList = keyframeStack.cssAnimationList();
 
     // 2.5 For each property, property, in targeted properties:
     for (auto property : effect->animatedProperties()) {
@@ -1356,7 +1356,7 @@ ExceptionOr<void> WebAnimation::commitStyles()
         // effect stack and stop when we've found this animation's effect or when we've found an effect associated with an animation with a higher composite order.
         auto animatedStyle = RenderStyle::clonePtr(style);
         for (const auto& effectInStack : keyframeStack.sortedEffects()) {
-            if (effectInStack->animation() != this && !compareAnimationsByCompositeOrder(*effectInStack->animation(), *this, cssAnimationNames))
+            if (effectInStack->animation() != this && !compareAnimationsByCompositeOrder(*effectInStack->animation(), *this, cssAnimationList))
                 break;
             if (effectInStack->animatedProperties().contains(property))
                 effectInStack->animation()->resolve(*animatedStyle);
index bcd1fc1..2177f20 100644 (file)
@@ -27,6 +27,7 @@
 #include "WebAnimationUtilities.h"
 
 #include "Animation.h"
+#include "AnimationList.h"
 #include "CSSAnimation.h"
 #include "CSSTransition.h"
 #include "DeclarativeAnimation.h"
@@ -34,7 +35,7 @@
 
 namespace WebCore {
 
-bool compareAnimationsByCompositeOrder(WebAnimation& lhsAnimation, WebAnimation& rhsAnimation, Vector<String>& cssAnimationNames)
+bool compareAnimationsByCompositeOrder(WebAnimation& lhsAnimation, WebAnimation& rhsAnimation, const AnimationList* cssAnimationList)
 {
     bool lhsHasOwningElement = is<DeclarativeAnimation>(lhsAnimation) && downcast<DeclarativeAnimation>(lhsAnimation).owningElement();
     bool rhsHasOwningElement = is<DeclarativeAnimation>(rhsAnimation) && downcast<DeclarativeAnimation>(rhsAnimation).owningElement();
@@ -60,17 +61,23 @@ bool compareAnimationsByCompositeOrder(WebAnimation& lhsAnimation, WebAnimation&
     bool rhsIsCSSAnimation = rhsHasOwningElement && is<CSSAnimation>(rhsAnimation);
     if (lhsIsCSSAnimation || rhsIsCSSAnimation) {
         if (lhsIsCSSAnimation == rhsIsCSSAnimation) {
+            // We must have a list of CSS Animations if we have CSS Animations to sort through.
+            ASSERT(cssAnimationList);
+            ASSERT(!cssAnimationList->isEmpty());
+
             // https://drafts.csswg.org/css-animations-2/#animation-composite-order
             // Sort A and B based on their position in the computed value of the animation-name property of the (common) owning element.
-            auto& lhsCSSAnimationName = downcast<CSSAnimation>(lhsAnimation).backingAnimation().name();
-            auto& rhsCSSAnimationName = downcast<CSSAnimation>(rhsAnimation).backingAnimation().name();
+            auto& lhsBackingAnimation = downcast<CSSAnimation>(lhsAnimation).backingAnimation();
+            auto& rhsBackingAnimation = downcast<CSSAnimation>(rhsAnimation).backingAnimation();
 
-            for (auto& animationName : cssAnimationNames) {
-                if (animationName == lhsCSSAnimationName)
+            for (size_t i = 0; i < cssAnimationList->size(); ++i) {
+                auto& animation = cssAnimationList->animation(i);
+                if (animation == lhsBackingAnimation)
                     return true;
-                if (animationName == rhsCSSAnimationName)
+                if (animation == rhsBackingAnimation)
                     return false;
             }
+
             // We should have found either of those CSS animations in the CSS animations list.
             ASSERT_NOT_REACHED();
         }
index 82d6ba5..2b4b3e3 100644 (file)
@@ -31,6 +31,7 @@
 
 namespace WebCore {
 
+class AnimationList;
 class WebAnimation;
 
 inline double secondsToWebAnimationsAPITime(const Seconds time)
@@ -63,7 +64,7 @@ struct WebAnimationsMarkableDoubleTraits {
 
 using MarkableDouble = Markable<double, WebAnimationsMarkableDoubleTraits>;
 
-bool compareAnimationsByCompositeOrder(WebAnimation&, WebAnimation&, Vector<String>& cssAnimationNames);
+bool compareAnimationsByCompositeOrder(WebAnimation&, WebAnimation&, const AnimationList*);
 
 } // namespace WebCore
 
index bf70fff..b41b6c7 100644 (file)
@@ -35,7 +35,7 @@ class AnimationList : public RefCounted<AnimationList> {
 public:
     static Ref<AnimationList> create() { return adoptRef(*new AnimationList); }
 
-    Ref<AnimationList> copy() { return adoptRef(*new AnimationList(*this)); }
+    Ref<AnimationList> copy() const { return adoptRef(*new AnimationList(*this)); }
 
     void fillUnsetProperties();
     bool operator==(const AnimationList&) const;