- Revision
- 287769
- Author
- grao...@webkit.org
- Date
- 2022-01-07 11:22:52 -0800 (Fri, 07 Jan 2022)
Log 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:
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (287768 => 287769)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2022-01-07 19:22:52 UTC (rev 287769)
@@ -1,5 +1,16 @@
2022-01-07 Antoine Quint <grao...@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 <grao...@webkit.org>
+
Transitions without an explicit property-name should not be considered
https://bugs.webkit.org/show_bug.cgi?id=234960
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt (287768 => 287769)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/Element-getAnimations.tentative-expected.txt 2022-01-07 19:22:52 UTC (rev 287769)
@@ -8,7 +8,7 @@
PASS getAnimations for CSS Animations that have finished but are forwards filling
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
PASS getAnimations for CSS animations in delay phase
Modified: trunk/Source/WebCore/ChangeLog (287768 => 287769)
--- trunk/Source/WebCore/ChangeLog 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/Source/WebCore/ChangeLog 2022-01-07 19:22:52 UTC (rev 287769)
@@ -1,5 +1,39 @@
2022-01-07 Antoine Quint <grao...@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 <grao...@webkit.org>
+
Transitions without an explicit property-name should not be considered
https://bugs.webkit.org/show_bug.cgi?id=234960
Modified: trunk/Source/WebCore/animation/KeyframeEffectStack.cpp (287768 => 287769)
--- trunk/Source/WebCore/animation/KeyframeEffectStack.cpp 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/Source/WebCore/animation/KeyframeEffectStack.cpp 2022-01-07 19:22:52 UTC (rev 287769)
@@ -156,4 +156,24 @@
effect->stopAcceleratingTransformRelatedProperties(useAcceleratedAction);
}
+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
Modified: trunk/Source/WebCore/animation/KeyframeEffectStack.h (287768 => 287769)
--- trunk/Source/WebCore/animation/KeyframeEffectStack.h 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/Source/WebCore/animation/KeyframeEffectStack.h 2022-01-07 19:22:52 UTC (rev 287769)
@@ -59,13 +59,18 @@
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 };
-
};
} // namespace WebCore
Modified: trunk/Source/WebCore/style/Styleable.cpp (287768 => 287769)
--- trunk/Source/WebCore/style/Styleable.cpp 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/Source/WebCore/style/Styleable.cpp 2022-01-07 19:22:52 UTC (rev 287769)
@@ -201,17 +201,23 @@
}
}
-static bool shouldConsiderAnimation(Element& element, const Animation& animation)
+static bool keyframesRuleExistsForAnimation(Element& element, const Animation& animation, const String& animationName)
{
- if (!animation.isValidAnimation())
- return false;
+ auto* styleScope = Style::Scope::forOrdinal(element, animation.nameStyleScopeOrdinal());
+ return styleScope && styleScope->resolver().isAnimationNameValid(animationName);
+}
- auto& name = animation.name().string;
- if (name == "none" || name.isEmpty())
+bool Styleable::animationListContainsNewlyValidAnimation(const AnimationList& animations) const
+{
+ auto& keyframeEffectStack = ensureKeyframeEffectStack();
+ if (!keyframeEffectStack.hasInvalidCSSAnimationNames())
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;
}
@@ -230,12 +236,14 @@
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
// 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
@@ -247,12 +255,21 @@
// first item in the list.
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.
previousAnimation->setBackingAnimation(currentAnimation.get());
Modified: trunk/Source/WebCore/style/Styleable.h (287768 => 287769)
--- trunk/Source/WebCore/style/Styleable.h 2022-01-07 19:21:02 UTC (rev 287768)
+++ trunk/Source/WebCore/style/Styleable.h 2022-01-07 19:22:52 UTC (rev 287769)
@@ -150,6 +150,8 @@
element.keyframesRuleDidChange(pseudoId);
}
+ bool animationListContainsNewlyValidAnimation(const AnimationList&) const;
+
void elementWasRemoved() const;
void willChangeRenderer() const;
