REGRESSION(r252455): imported/w3c/web-platform-tests/dom/events/Event-dispatch-on...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Nov 2019 19:38:43 +0000 (19:38 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Nov 2019 19:38:43 +0000 (19:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204272
<rdar://problem/57253742>

Reviewed by Dean Jackson.

Source/WebCore:

Events for declarative animations are dispatched using a MainThreadGenericEventQueue which dispatches enqueued events asynchronously. When a declarative
animation would be canceled, AnimationTimeline::cancelDeclarativeAnimation() would be called and would enqueue a "transitioncancel" or "animationcancel"
event (depending on the animation type) by virtue of calling DeclarativeAnimation::cancelFromStyle(), and would also call AnimationTimeline::removeAnimation()
right after. However, calling AnimationTimeline::removeAnimation() could have the side effect of removing the last reference to the DeclarativeAnimation
object, which would destroy its attached MainThreadGenericEventQueue before it had the time to dispatch the queued "transitioncancel" or "animationcancel"
event.

The call to AnimationTimeline::removeAnimation() in AnimationTimeline::cancelDeclarativeAnimation() is actually unnecessary. Simply canceling the animation
via DeclarativeAnimation::cancelFromStyle() will end up calling AnimationTimeline::removeAnimation() the next time animations are updated, which will leave
time for the cancel events to be dispatched. So all we need to do is remove AnimationTimeline::cancelDeclarativeAnimation() and replace its call sites with
simple calls to DeclarativeAnimation::cancelFromStyle().

Making this change broke a test however: imported/w3c/web-platform-tests/css/css-animations/Document-getAnimations.tentative.html. We actually passed that
test by chance without implementing the feature required to make it work. We now implement the correct way to track a global position for an animation by
only setting one for declarative animations once they are disassociated with their owning element and have a non-idle play state.

And a few other tests broke: animations/animation-shorthand-name-order.html, imported/w3c/web-platform-tests/css/css-animations/animationevent-types.html
and webanimations/css-animations.html. The reason for those tests being broken was that not calling AnimationTimeline::removeAnimation() instantly as CSS
Animations were canceled also meant that the KeyframeEffectStack for the targeted element wasn't updated. To solve this, we added the animationTimingDidChange()
method on KeyframeEffect which is called whenever timing on the owning Animation changes, so for instance when cancel() is called as we cancel a CSS
Animation. That way we ensure we add and remove KeyframeEffect instances from the KeyframeEffectStack as the animation becomes relevant, which is now
an added condition checked by KeyframeEffectStack::addEffect().

Finally, this revealed an issue in KeyframeEffectStack::ensureEffectsAreSorted() where we would consider CSSTransition and CSSAnimation objects to be
representative of a CSS Transition or CSS Animation even after the relationship with their owning element had been severed. We now correctly check that
relationship is intact and otherwise consider those animations just like any other animation.

* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::animationTimingDidChange):
(WebCore::AnimationTimeline::updateGlobalPosition):
(WebCore::AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement):
(WebCore::AnimationTimeline::updateCSSAnimationsForElement):
(WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
(WebCore::AnimationTimeline::updateCSSTransitionsForElement):
(WebCore::AnimationTimeline::cancelDeclarativeAnimation): Deleted.
* animation/AnimationTimeline.h:
* animation/CSSAnimation.cpp:
(WebCore::CSSAnimation::syncPropertiesWithBackingAnimation):
* animation/CSSTransition.cpp:
(WebCore::CSSTransition::setTimingProperties):
* animation/DeclarativeAnimation.cpp:
(WebCore::DeclarativeAnimation::canHaveGlobalPosition):
* animation/DeclarativeAnimation.h:
* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::getAnimations const):
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::animationTimelineDidChange):
(WebCore::KeyframeEffect::animationTimingDidChange):
(WebCore::KeyframeEffect::updateEffectStackMembership):
(WebCore::KeyframeEffect::setAnimation):
(WebCore::KeyframeEffect::setTarget):
* animation/KeyframeEffect.h:
* animation/KeyframeEffectStack.cpp:
(WebCore::KeyframeEffectStack::addEffect):
(WebCore::KeyframeEffectStack::ensureEffectsAreSorted):
* animation/KeyframeEffectStack.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::timingDidChange):
* animation/WebAnimation.h:
(WebCore::WebAnimation::canHaveGlobalPosition):

LayoutTests:

Removing this specific expectation for WK1 since it now behaves just like the other configurations.

* platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt: Removed.

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

16 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt [deleted file]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/AnimationTimeline.h
Source/WebCore/animation/CSSAnimation.cpp
Source/WebCore/animation/CSSTransition.cpp
Source/WebCore/animation/DeclarativeAnimation.cpp
Source/WebCore/animation/DeclarativeAnimation.h
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/KeyframeEffect.h
Source/WebCore/animation/KeyframeEffectStack.cpp
Source/WebCore/animation/KeyframeEffectStack.h
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h

index d438a22..74f2df0 100644 (file)
@@ -1,3 +1,15 @@
+2019-11-27  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION(r252455): imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements.html fails on iOS and WK1
+        https://bugs.webkit.org/show_bug.cgi?id=204272
+        <rdar://problem/57253742>
+
+        Reviewed by Dean Jackson.
+
+        Removing this specific expectation for WK1 since it now behaves just like the other configurations.
+
+        * platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt: Removed.
+
 2019-11-27  Jonathan Bedard  <jbedard@apple.com>
 
         [WebGL] Garden dedicated queue (Part 7)
diff --git a/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt b/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements-expected.txt
deleted file mode 100644 (file)
index 67c33e0..0000000
+++ /dev/null
@@ -1,13 +0,0 @@
-
-Harness Error (TIMEOUT), message = null
-
-PASS Can dispatch untrusted 'click' Events at disabled HTML elements. 
-PASS Can dispatch untrusted Events at disabled HTML elements. 
-PASS Can dispatch CustomEvents at disabled HTML elements. 
-PASS Calling click() on disabled elements must not dispatch events. 
-PASS CSS Transitions transitionrun, transitionstart, transitionend events fire on disabled form elements 
-TIMEOUT CSS Transitions transitioncancel event fires on disabled form elements Test timed out
-NOTRUN CSS Animation animationstart, animationiteration, animationend fire on disabled form elements 
-NOTRUN CSS Animation's animationcancel event fires on disabled form elements 
-NOTRUN Real clicks on disabled elements must not dispatch events. 
-
index d471d31..ae96258 100644 (file)
@@ -1,3 +1,72 @@
+2019-11-27  Antoine Quint  <graouts@apple.com>
+
+        REGRESSION(r252455): imported/w3c/web-platform-tests/dom/events/Event-dispatch-on-disabled-elements.html fails on iOS and WK1
+        https://bugs.webkit.org/show_bug.cgi?id=204272
+        <rdar://problem/57253742>
+
+        Reviewed by Dean Jackson.
+
+        Events for declarative animations are dispatched using a MainThreadGenericEventQueue which dispatches enqueued events asynchronously. When a declarative
+        animation would be canceled, AnimationTimeline::cancelDeclarativeAnimation() would be called and would enqueue a "transitioncancel" or "animationcancel"
+        event (depending on the animation type) by virtue of calling DeclarativeAnimation::cancelFromStyle(), and would also call AnimationTimeline::removeAnimation()
+        right after. However, calling AnimationTimeline::removeAnimation() could have the side effect of removing the last reference to the DeclarativeAnimation
+        object, which would destroy its attached MainThreadGenericEventQueue before it had the time to dispatch the queued "transitioncancel" or "animationcancel"
+        event.
+
+        The call to AnimationTimeline::removeAnimation() in AnimationTimeline::cancelDeclarativeAnimation() is actually unnecessary. Simply canceling the animation
+        via DeclarativeAnimation::cancelFromStyle() will end up calling AnimationTimeline::removeAnimation() the next time animations are updated, which will leave
+        time for the cancel events to be dispatched. So all we need to do is remove AnimationTimeline::cancelDeclarativeAnimation() and replace its call sites with
+        simple calls to DeclarativeAnimation::cancelFromStyle().
+
+        Making this change broke a test however: imported/w3c/web-platform-tests/css/css-animations/Document-getAnimations.tentative.html. We actually passed that
+        test by chance without implementing the feature required to make it work. We now implement the correct way to track a global position for an animation by
+        only setting one for declarative animations once they are disassociated with their owning element and have a non-idle play state.
+
+        And a few other tests broke: animations/animation-shorthand-name-order.html, imported/w3c/web-platform-tests/css/css-animations/animationevent-types.html
+        and webanimations/css-animations.html. The reason for those tests being broken was that not calling AnimationTimeline::removeAnimation() instantly as CSS
+        Animations were canceled also meant that the KeyframeEffectStack for the targeted element wasn't updated. To solve this, we added the animationTimingDidChange()
+        method on KeyframeEffect which is called whenever timing on the owning Animation changes, so for instance when cancel() is called as we cancel a CSS
+        Animation. That way we ensure we add and remove KeyframeEffect instances from the KeyframeEffectStack as the animation becomes relevant, which is now
+        an added condition checked by KeyframeEffectStack::addEffect().
+
+        Finally, this revealed an issue in KeyframeEffectStack::ensureEffectsAreSorted() where we would consider CSSTransition and CSSAnimation objects to be
+        representative of a CSS Transition or CSS Animation even after the relationship with their owning element had been severed. We now correctly check that
+        relationship is intact and otherwise consider those animations just like any other animation.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::AnimationTimeline::animationTimingDidChange):
+        (WebCore::AnimationTimeline::updateGlobalPosition):
+        (WebCore::AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement):
+        (WebCore::AnimationTimeline::updateCSSAnimationsForElement):
+        (WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
+        (WebCore::AnimationTimeline::updateCSSTransitionsForElement):
+        (WebCore::AnimationTimeline::cancelDeclarativeAnimation): Deleted.
+        * animation/AnimationTimeline.h:
+        * animation/CSSAnimation.cpp:
+        (WebCore::CSSAnimation::syncPropertiesWithBackingAnimation):
+        * animation/CSSTransition.cpp:
+        (WebCore::CSSTransition::setTimingProperties):
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::canHaveGlobalPosition):
+        * animation/DeclarativeAnimation.h:
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::getAnimations const):
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::animationTimelineDidChange):
+        (WebCore::KeyframeEffect::animationTimingDidChange):
+        (WebCore::KeyframeEffect::updateEffectStackMembership):
+        (WebCore::KeyframeEffect::setAnimation):
+        (WebCore::KeyframeEffect::setTarget):
+        * animation/KeyframeEffect.h:
+        * animation/KeyframeEffectStack.cpp:
+        (WebCore::KeyframeEffectStack::addEffect):
+        (WebCore::KeyframeEffectStack::ensureEffectsAreSorted):
+        * animation/KeyframeEffectStack.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::timingDidChange):
+        * animation/WebAnimation.h:
+        (WebCore::WebAnimation::canHaveGlobalPosition):
+
 2019-11-27  James Darpinian  <jdarpinian@chromium.org>
 
         Enable GPU switching with ANGLE
index 3a92a93..a7ed689 100644 (file)
@@ -62,8 +62,9 @@ void AnimationTimeline::forgetAnimation(WebAnimation* animation)
 
 void AnimationTimeline::animationTimingDidChange(WebAnimation& animation)
 {
+    updateGlobalPosition(animation);
+
     if (m_animations.add(&animation)) {
-        animation.setGlobalPosition(m_allAnimations.size());
         m_allAnimations.append(makeWeakPtr(&animation));
         auto* timeline = animation.timeline();
         if (timeline && timeline != this)
@@ -71,6 +72,13 @@ void AnimationTimeline::animationTimingDidChange(WebAnimation& animation)
     }
 }
 
+void AnimationTimeline::updateGlobalPosition(WebAnimation& animation)
+{
+    static uint64_t s_globalPosition = 0;
+    if (!animation.globalPosition() && animation.canHaveGlobalPosition())
+        animation.setGlobalPosition(++s_globalPosition);
+}
+
 void AnimationTimeline::removeAnimation(WebAnimation& animation)
 {
     ASSERT(!animation.timeline() || animation.timeline() == this);
@@ -156,9 +164,11 @@ void AnimationTimeline::removeDeclarativeAnimationFromListsForOwningElement(WebA
         if (iterator != m_elementToCSSAnimationByName.end()) {
             auto& cssAnimationsByName = iterator->value;
             auto& name = downcast<CSSAnimation>(animation).animationName();
-            cssAnimationsByName.remove(name);
-            if (cssAnimationsByName.isEmpty())
-                m_elementToCSSAnimationByName.remove(&element);
+            if (cssAnimationsByName.get(name) == &animation) {
+                cssAnimationsByName.remove(name);
+                if (cssAnimationsByName.isEmpty())
+                    m_elementToCSSAnimationByName.remove(&element);
+            }
         }
     } else if (is<CSSTransition>(animation)) {
         auto& transition = downcast<CSSTransition>(animation);
@@ -252,7 +262,7 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
     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))
-                cancelDeclarativeAnimation(*cssAnimationsByNameMapItem.value);
+                cssAnimationsByNameMapItem.value->cancelFromStyle();
         }
         element.ensureKeyframeEffectStack().setCSSAnimationNames(WTFMove(animationNames));
         return;
@@ -281,16 +291,17 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
         for (size_t i = 0; i < currentAnimations->size(); ++i) {
             auto& currentAnimation = currentAnimations->animation(i);
             auto& name = currentAnimation.name();
-            animationNames.append(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);
             }
             // Remove the name of this animation from our list since it's now known to be current.
             namesOfPreviousAnimations.remove(name);
@@ -301,7 +312,7 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
     // remove the CSSAnimation object created for them.
     for (const auto& nameOfAnimationToRemove : namesOfPreviousAnimations) {
         if (auto animation = cssAnimationsByName.take(nameOfAnimationToRemove))
-            cancelDeclarativeAnimation(*animation);
+            animation->cancelFromStyle();
     }
 
     element.ensureKeyframeEffectStack().setCSSAnimationNames(WTFMove(animationNames));
@@ -472,15 +483,15 @@ void AnimationTimeline::updateCSSTransitionsForElementAndProperty(Element& eleme
         if (CSSPropertyAnimation::propertiesEqual(property, &previouslyRunningTransitionCurrentStyle, &afterChangeStyle) || !CSSPropertyAnimation::canPropertyBeInterpolated(property, &currentStyle, &afterChangeStyle)) {
             // 1. If the current value of the property in the running transition is equal to the value of the property in the after-change style,
             //    or if these two values cannot be interpolated, then implementations must cancel the running transition.
-            cancelDeclarativeAnimation(*previouslyRunningTransition);
+            previouslyRunningTransition->cancelFromStyle();
         } else if (transitionCombinedDuration(matchingBackingAnimation) <= 0.0 || !CSSPropertyAnimation::canPropertyBeInterpolated(property, &previouslyRunningTransitionCurrentStyle, &afterChangeStyle)) {
             // 2. Otherwise, if the combined duration is less than or equal to 0s, or if the current value of the property in the running transition
             //    cannot be interpolated with the value of the property in the after-change style, then implementations must cancel the running transition.
-            cancelDeclarativeAnimation(*previouslyRunningTransition);
+            previouslyRunningTransition->cancelFromStyle();
         } else if (CSSPropertyAnimation::propertiesEqual(property, &previouslyRunningTransition->reversingAdjustedStartStyle(), &afterChangeStyle)) {
             // 3. Otherwise, if the reversing-adjusted start value of the running transition is the same as the value of the property in the after-change
             //    style (see the section on reversing of transitions for why these case exists), implementations must cancel the running transition
-            cancelDeclarativeAnimation(*previouslyRunningTransition);
+            previouslyRunningTransition->cancelFromStyle();
 
             // and start a new transition whose:
             //   - reversing-adjusted start value is the end value of the running transition,
@@ -506,7 +517,7 @@ void AnimationTimeline::updateCSSTransitionsForElementAndProperty(Element& eleme
             ensureRunningTransitionsByProperty(element).set(property, CSSTransition::create(element, property, generationTime, *matchingBackingAnimation, &previouslyRunningTransitionCurrentStyle, afterChangeStyle, delay, duration, reversingAdjustedStartStyle, reversingShorteningFactor));
         } else {
             // 4. Otherwise, implementations must cancel the running transition
-            cancelDeclarativeAnimation(*previouslyRunningTransition);
+            previouslyRunningTransition->cancelFromStyle();
 
             // and start a new transition whose:
             //   - start time is the time of the style change event plus the matching transition delay,
@@ -530,7 +541,7 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
     if (currentStyle.hasTransitions() && currentStyle.display() != DisplayType::None && afterChangeStyle.display() == DisplayType::None) {
         if (m_elementToRunningCSSTransitionByCSSPropertyID.contains(&element)) {
             for (const auto& cssTransitionsByCSSPropertyIDMapItem : m_elementToRunningCSSTransitionByCSSPropertyID.take(&element))
-                cancelDeclarativeAnimation(*cssTransitionsByCSSPropertyIDMapItem.value);
+                cssTransitionsByCSSPropertyIDMapItem.value->cancelFromStyle();
         }
         return;
     }
@@ -568,11 +579,4 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
         updateCSSTransitionsForElementAndProperty(element, property, currentStyle, afterChangeStyle, runningTransitionsByProperty, completedTransitionsByProperty, generationTime);
 }
 
-void AnimationTimeline::cancelDeclarativeAnimation(DeclarativeAnimation& animation)
-{
-    animation.cancelFromStyle();
-    removeAnimation(animation);
-    m_allAnimations.removeFirst(&animation);
-}
-
 } // namespace WebCore
index f08b339..3141cf5 100644 (file)
@@ -82,10 +82,10 @@ protected:
     HashMap<Element*, PropertyToTransitionMap> m_elementToCompletedCSSTransitionByCSSPropertyID;
 
 private:
+    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 cancelDeclarativeAnimation(DeclarativeAnimation&);
 
     ElementToAnimationsMap m_elementToAnimationsMap;
     ElementToAnimationsMap m_elementToCSSAnimationsMap;
index 4ecbf03..7c72f62 100644 (file)
@@ -97,6 +97,7 @@ void CSSAnimation::syncPropertiesWithBackingAnimation()
     animationEffect->setDelay(Seconds(animation.delay()));
     animationEffect->setIterationDuration(Seconds(animation.duration()));
     animationEffect->updateStaticTimingProperties();
+    effectTimingDidChange();
 
     // Synchronize the play state
     if (animation.playState() == AnimationPlayState::Playing && playState() == WebAnimation::PlayState::Paused) {
index 9c7f690..dff5fb1 100644 (file)
@@ -74,6 +74,7 @@ void CSSTransition::setTimingProperties(Seconds delay, Seconds duration)
     animationEffect->setIterationDuration(duration);
     animationEffect->setTimingFunction(backingAnimation().timingFunction());
     animationEffect->updateStaticTimingProperties();
+    effectTimingDidChange();
 
     unsuspendEffectInvalidation();
 }
index 2c6770d..bbe4c37 100644 (file)
@@ -71,6 +71,17 @@ void DeclarativeAnimation::tick()
     }
 }
 
+bool DeclarativeAnimation::canHaveGlobalPosition()
+{
+    // https://drafts.csswg.org/css-animations-2/#animation-composite-order
+    // https://drafts.csswg.org/css-transitions-2/#animation-composite-order
+    // CSS Animations and CSS Transitions generated using the markup defined in this specification are not added
+    // to the global animation list when they are created. Instead, these animations are appended to the global
+    // animation list at the first moment when they transition out of the idle play state after being disassociated
+    // from their owning element.
+    return !m_owningElement && playState() != WebAnimation::PlayState::Idle;
+}
+
 void DeclarativeAnimation::disassociateFromOwningElement()
 {
     if (!m_owningElement)
index c966e58..363e98c 100644 (file)
@@ -67,6 +67,8 @@ public:
     bool needsTick() const override;
     void tick() override;
 
+    bool canHaveGlobalPosition() final;
+
 protected:
     DeclarativeAnimation(Element&, const Animation&);
 
index aea558d..20a07ee 100644 (file)
@@ -182,6 +182,10 @@ Vector<RefPtr<WebAnimation>> DocumentTimeline::getAnimations() const
         return false;
     });
 
+    std::sort(webAnimations.begin(), webAnimations.end(), [](auto& lhs, auto& rhs) {
+        return lhs->globalPosition() < rhs->globalPosition();
+    });
+
     // Finally, we can concatenate the sorted CSS Transitions, CSS Animations and Web Animations in their relative composite order.
     Vector<RefPtr<WebAnimation>> animations;
     animations.appendRange(cssTransitions.begin(), cssTransitions.end());
index 98b6168..af30ddc 100644 (file)
@@ -996,21 +996,43 @@ void KeyframeEffect::animationTimelineDidChange(AnimationTimeline* timeline)
         return;
 
     if (timeline)
-        m_target->ensureKeyframeEffectStack().addEffect(*this);
-    else
+        m_inTargetEffectStack = m_target->ensureKeyframeEffectStack().addEffect(*this);
+    else {
+        m_target->ensureKeyframeEffectStack().removeEffect(*this);
+        m_inTargetEffectStack = false;
+    }
+}
+
+void KeyframeEffect::animationTimingDidChange()
+{
+    updateEffectStackMembership();
+}
+
+void KeyframeEffect::updateEffectStackMembership()
+{
+    if (!m_target)
+        return;
+
+    bool isRelevant = animation() && animation()->isRelevant();
+    if (isRelevant && !m_inTargetEffectStack)
+        m_inTargetEffectStack = m_target->ensureKeyframeEffectStack().addEffect(*this);
+    else if (!isRelevant && m_inTargetEffectStack) {
         m_target->ensureKeyframeEffectStack().removeEffect(*this);
+        m_inTargetEffectStack = false;
+    }
 }
 
 void KeyframeEffect::setAnimation(WebAnimation* animation)
 {
     bool animationChanged = animation != this->animation();
     AnimationEffect::setAnimation(animation);
-    if (m_target && animationChanged) {
-        if (animation)
-            m_target->ensureKeyframeEffectStack().addEffect(*this);
-        else
-            m_target->ensureKeyframeEffectStack().removeEffect(*this);
-    }
+
+    if (!animationChanged)
+        return;
+
+    if (animation)
+        animation->updateRelevance();
+    updateEffectStackMembership();
 }
 
 void KeyframeEffect::setTarget(RefPtr<Element>&& newTarget)
@@ -1033,10 +1055,12 @@ void KeyframeEffect::setTarget(RefPtr<Element>&& newTarget)
     // any animated styles are removed immediately.
     invalidateElement(previousTarget.get());
 
-    if (previousTarget)
+    if (previousTarget) {
         previousTarget->ensureKeyframeEffectStack().removeEffect(*this);
+        m_inTargetEffectStack = false;
+    }
     if (m_target)
-        m_target->ensureKeyframeEffectStack().addEffect(*this);
+        m_inTargetEffectStack = m_target->ensureKeyframeEffectStack().addEffect(*this);
 }
 
 void KeyframeEffect::apply(RenderStyle& targetStyle)
index b808314..7a1aeb7 100644 (file)
@@ -116,6 +116,7 @@ public:
     void animationWasCanceled() final;
     void animationSuspensionStateDidChange(bool) final;
     void animationTimelineDidChange(AnimationTimeline*) final;
+    void animationTimingDidChange();
     void applyPendingAcceleratedActions();
     bool isRunningAccelerated() const { return m_lastRecordedAcceleratedAction != AcceleratedAction::Stop; }
     bool hasPendingAcceleratedAction() const { return !m_pendingAcceleratedActions.isEmpty() && isRunningAccelerated(); }
@@ -147,6 +148,7 @@ private:
     enum class AcceleratedAction : uint8_t { Play, Pause, Seek, Stop };
     enum class BlendingKeyframesSource : uint8_t { CSSAnimation, CSSTransition, WebAnimation };
 
+    void updateEffectStackMembership();
     void copyPropertiesFromSource(Ref<KeyframeEffect>&&);
     ExceptionOr<void> processKeyframes(JSC::JSGlobalObject&, JSC::Strong<JSC::JSObject>&&);
     void addPendingAcceleratedAction(AcceleratedAction);
@@ -188,6 +190,7 @@ private:
     bool m_backdropFilterFunctionListsMatch { false };
 #endif
     bool m_colorFilterFunctionListsMatch { false };
+    bool m_inTargetEffectStack { false };
 };
 
 } // namespace WebCore
index 747464e..129b4aa 100644 (file)
@@ -42,15 +42,16 @@ KeyframeEffectStack::~KeyframeEffectStack()
     ASSERT(m_effects.isEmpty());
 }
 
-void KeyframeEffectStack::addEffect(KeyframeEffect& effect)
+bool KeyframeEffectStack::addEffect(KeyframeEffect& effect)
 {
-    // To qualify for membership in an effect stack, an effect must have a target, an animation and a timeline.
+    // To qualify for membership in an effect stack, an effect must have a target, an animation, a timeline and be relevant.
     // This method will be called in WebAnimation and KeyframeEffect as those properties change.
-    if (!effect.target() || !effect.animation() || !effect.animation()->timeline())
-        return;
+    if (!effect.target() || !effect.animation() || !effect.animation()->timeline() || !effect.animation()->isRelevant())
+        return false;
 
     m_effects.append(makeWeakPtr(&effect));
     m_isSorted = false;
+    return true;
 }
 
 void KeyframeEffectStack::removeEffect(KeyframeEffect& effect)
@@ -76,9 +77,12 @@ void KeyframeEffectStack::ensureEffectsAreSorted()
         ASSERT(lhsAnimation);
         ASSERT(rhsAnimation);
 
+        bool lhsHasOwningElement = is<DeclarativeAnimation>(lhsAnimation) && downcast<DeclarativeAnimation>(lhsAnimation)->owningElement();
+        bool rhsHasOwningElement = is<DeclarativeAnimation>(rhsAnimation) && downcast<DeclarativeAnimation>(rhsAnimation)->owningElement();
+
         // CSS Transitions sort first.
-        bool lhsIsCSSTransition = is<CSSTransition>(lhsAnimation);
-        bool rhsIsCSSTransition = is<CSSTransition>(rhsAnimation);
+        bool lhsIsCSSTransition = lhsHasOwningElement && is<CSSTransition>(lhsAnimation);
+        bool rhsIsCSSTransition = rhsHasOwningElement && is<CSSTransition>(rhsAnimation);
         if (lhsIsCSSTransition || rhsIsCSSTransition) {
             if (lhsIsCSSTransition == rhsIsCSSTransition) {
                 // Sort transitions first by their generation time, and then by transition-property.
@@ -93,8 +97,8 @@ void KeyframeEffectStack::ensureEffectsAreSorted()
         }
 
         // CSS Animations sort next.
-        bool lhsIsCSSAnimation = is<CSSAnimation>(lhsAnimation);
-        bool rhsIsCSSAnimation = is<CSSAnimation>(rhsAnimation);
+        bool lhsIsCSSAnimation = lhsHasOwningElement && is<CSSAnimation>(lhsAnimation);
+        bool rhsIsCSSAnimation = rhsHasOwningElement && is<CSSAnimation>(rhsAnimation);
         if (lhsIsCSSAnimation || rhsIsCSSAnimation) {
             if (lhsIsCSSAnimation == rhsIsCSSAnimation) {
                 // https://drafts.csswg.org/css-animations-2/#animation-composite-order
index e656828..79a4e97 100644 (file)
@@ -38,7 +38,7 @@ public:
     explicit KeyframeEffectStack();
     ~KeyframeEffectStack();
 
-    void addEffect(KeyframeEffect&);
+    bool addEffect(KeyframeEffect&);
     void removeEffect(KeyframeEffect&);
     bool hasEffects() const { return !m_effects.isEmpty(); }
     Vector<WeakPtr<KeyframeEffect>> sortedEffects();
index baf820c..737da75 100644 (file)
@@ -721,6 +721,12 @@ void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchron
 {
     m_shouldSkipUpdatingFinishedStateWhenResolving = false;
     updateFinishedState(didSeek, synchronouslyNotify);
+
+    if (is<KeyframeEffect>(m_effect)) {
+        updateRelevance();
+        downcast<KeyframeEffect>(*m_effect).animationTimingDidChange();
+    }
+
     if (m_timeline)
         m_timeline->animationTimingDidChange(*this);
 };
index ce0c7e9..15fad61 100644 (file)
@@ -120,6 +120,7 @@ public:
 
     bool isRunningAccelerated() const;
     bool isRelevant() const { return m_isRelevant; }
+    void updateRelevance();
     void effectTimingDidChange();
     void suspendEffectInvalidation();
     void unsuspendEffectInvalidation();
@@ -129,11 +130,13 @@ public:
     virtual void remove();
     void enqueueAnimationPlaybackEvent(const AtomString&, Optional<Seconds>, Optional<Seconds>);
 
-    unsigned globalPosition() const { return m_globalPosition; }
-    void setGlobalPosition(unsigned globalPosition) { m_globalPosition = globalPosition; }
+    uint64_t globalPosition() const { return m_globalPosition; }
+    void setGlobalPosition(uint64_t globalPosition) { m_globalPosition = globalPosition; }
 
     bool hasPendingActivity() const final;
 
+    virtual bool canHaveGlobalPosition() { return true; }
+
     // ContextDestructionObserver.
     ScriptExecutionContext* scriptExecutionContext() const final { return ActiveDOMObject::scriptExecutionContext(); }
     void contextDestroyed() final;
@@ -169,7 +172,6 @@ private:
     void setTimelineInternal(RefPtr<AnimationTimeline>&&);
     bool isEffectInvalidationSuspended() { return m_suspendCount; }
     bool computeRelevance();
-    void updateRelevance();
     void invalidateEffect();
     double effectivePlaybackRate() const;
     void applyPendingPlaybackRate();
@@ -194,7 +196,7 @@ private:
     TimeToRunPendingTask m_timeToRunPendingPlayTask { TimeToRunPendingTask::NotScheduled };
     TimeToRunPendingTask m_timeToRunPendingPauseTask { TimeToRunPendingTask::NotScheduled };
     ReplaceState m_replaceState { ReplaceState::Active };
-    unsigned m_globalPosition;
+    uint64_t m_globalPosition { 0 };
 
     // ActiveDOMObject.
     const char* activeDOMObjectName() const final;