Web Animations CSS: Unfreeze AnimationClock if sampling timelines doe…

…s not trigger style recalc When sampling the animations and transitions timelines, we set the AnimationClock's time and freeze it so that subsequent accesses of the clock's time during style recalc receive the same value. The clock is unfrozen when style recalc completes. However, if a timeline is sampled after its last Player is removed, it will not trigger style recalc *. This can occur if a transition is cancelled prematurely. In this case, we will fail to unfreeze the clock, so transitions started subsequently will see a stale clock time. This change modifies DocumentTimeline::serviceAnimations() to unfreeze the AnimationClock if it did not trigger a style recalc. It also adds a test to verify this behavior. * In non-CSS uses of the Web Animations model, a timeline can also fail to trigger style recalc if its animations have no effect or no target. BUG=303430,248938 R=timloh@chromium.org Review URL: https://codereview.chromium.org/46043014 git-svn-id: svn://svn.chromium.org/blink/trunk@161134 bbb929c8-8fbe-4397-9dbb-9b2b20218538
marcosholgado · Nov 1, 2013 · 9940aa1 · 9940aa1
1 parent 875384f
commit 9940aa1
Show file tree

Hide file tree

Showing 16 changed files with 181 additions and 42 deletions.
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
@@ -249,6 +249,7 @@ crbug.com/248938 virtual/threaded/animations/animation-direction-normal.html [ P
 crbug.com/248938 virtual/threaded/animations/combo-transform-translate+scale.html [ Pass Failure ]
 crbug.com/248938 virtual/threaded/animations/change-one-anim.html [ Pass Failure ]
 crbug.com/248938 virtual/threaded/animations/delay-start-event.html [ Pass Failure ]
+crbug.com/248938 virtual/threaded/transitions/cancel-and-start-new.html [ Failure ]
 crbug.com/248938 virtual/threaded/transitions/transition-end-event-nested.html [ Pass Failure ]
 crbug.com/248938 virtual/threaded/transitions/interrupted-all-transition.html [ Pass Failure ]
 crbug.com/248938 virtual/threaded/transitions/transition-end-event-multiple-03.html [ Pass Failure ]

diff --git a/third_party/WebKit/LayoutTests/transitions/cancel-and-start-new-expected.txt b/third_party/WebKit/LayoutTests/transitions/cancel-and-start-new-expected.txt
@@ -0,0 +1,3 @@
+Tests that having stopped a transition before it completes, a subsequent transition starts correctly.
+
+PASS
diff --git a/third_party/WebKit/LayoutTests/transitions/cancel-and-start-new.html b/third_party/WebKit/LayoutTests/transitions/cancel-and-start-new.html
@@ -0,0 +1,76 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        #target {
+            position: relative;
+            background-color: #933;
+            width: 50px;
+            height: 50px;
+            top: 0px;
+            left: 0px;
+        }
+        #target.transition-top {
+            top: 400px;
+            -webkit-transition: top 100ms linear;
+            transition: top 100ms linear;
+        }
+        #target.transition-left {
+            left: 400px;
+            -webkit-transition: left 100ms linear;
+            transition: left 100ms linear;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function isEqual(actual, desired, tolerance)
+        {
+            var diff = Math.abs(actual - desired);
+            return diff < tolerance;
+        }
+
+        function cancelTransition()
+        {
+            document.getElementById("target").classList.remove('transition-top');
+        }
+
+        function startNewTransition()
+        {
+            document.getElementById("target").classList.add('transition-left');
+            setTimeout(check, 50);
+        }
+
+        function check()
+        {
+            var left = parseFloat(window.getComputedStyle(document.getElementById('target')).left);
+            if (isEqual(left, 200, 50))
+                var result = "<span style='color:green'>PASS</span>";
+            else
+                var result = "<span style='color:red'>FAIL(was: " + left + ", expected: 200)</span>";
+            document.getElementById('result').innerHTML = result;
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        function start()
+        {
+            document.getElementById("target").classList.add('transition-top');
+            setTimeout("cancelTransition()", 50);
+            setTimeout("startNewTransition()", 100);
+        }
+    </script>
+</head>
+<body onload="start()">
+<p>
+    Tests that having stopped a transition before it completes, a subsequent
+    transition starts correctly.
+</p>
+<div id="target"></div>
+<div id="result"></div>
+</body>
+</html>
diff --git a/third_party/WebKit/Source/core/animation/Animation.cpp b/third_party/WebKit/Source/core/animation/Animation.cpp
@@ -62,11 +62,11 @@ static AnimationStack& ensureAnimationStack(Element* element)
     return element->ensureActiveAnimations()->defaultStack();
 }
 
-void Animation::applyEffects(bool previouslyInEffect)
+bool Animation::applyEffects(bool previouslyInEffect)
 {
     ASSERT(player());
     if (!m_target || !m_effect)
-        return;
+        return false;
 
     if (!previouslyInEffect) {
         ensureAnimationStack(m_target.get()).add(this);
@@ -75,6 +75,7 @@ void Animation::applyEffects(bool previouslyInEffect)
 
     m_compositableValues = m_effect->sample(currentIteration(), timeFraction());
     m_target->setNeedsStyleRecalc(LocalStyleChange, StyleChangeFromRenderer);
+    return true;
 }
 
 void Animation::clearEffects()
@@ -87,15 +88,19 @@ void Animation::clearEffects()
     m_target->setNeedsStyleRecalc(LocalStyleChange, StyleChangeFromRenderer);
 }
 
-void Animation::updateChildrenAndEffects() const
+bool Animation::updateChildrenAndEffects() const
 {
     if (!m_effect)
-        return;
+        return false;
 
     if (isInEffect())
-        const_cast<Animation*>(this)->applyEffects(m_activeInAnimationStack);
-    else if (m_activeInAnimationStack)
+        return const_cast<Animation*>(this)->applyEffects(m_activeInAnimationStack);
+
+    if (m_activeInAnimationStack) {
         const_cast<Animation*>(this)->clearEffects();
+        return true;
+    }
+    return false;
 }
 
 double Animation::calculateTimeToEffectChange(double inheritedTime, double activeTime, Phase phase) const

diff --git a/third_party/WebKit/Source/core/animation/Animation.h b/third_party/WebKit/Source/core/animation/Animation.h
@@ -56,9 +56,10 @@ class Animation FINAL : public TimedItem {
     Priority priority() const { return m_priority; }
 
 protected:
-    virtual void applyEffects(bool previouslyInEffect);
+    // Returns whether style recalc was triggered.
+    virtual bool applyEffects(bool previouslyInEffect);
     virtual void clearEffects();
-    virtual void updateChildrenAndEffects() const OVERRIDE FINAL;
+    virtual bool updateChildrenAndEffects() const OVERRIDE FINAL;
     virtual void willDetach() OVERRIDE FINAL;
     virtual double calculateTimeToEffectChange(double inheritedTime, double activeTime, Phase) const OVERRIDE FINAL;
 

diff --git a/third_party/WebKit/Source/core/animation/DocumentTimeline.cpp b/third_party/WebKit/Source/core/animation/DocumentTimeline.cpp
@@ -77,19 +77,20 @@ void DocumentTimeline::wake()
     m_timing->serviceOnNextFrame();
 }
 
-void DocumentTimeline::serviceAnimations(double monotonicAnimationStartTime)
+bool DocumentTimeline::serviceAnimations()
 {
     TRACE_EVENT0("webkit", "DocumentTimeline::serviceAnimations");
 
     m_timing->cancelWake();
 
-    m_document->animationClock().updateTime(monotonicAnimationStartTime);
-
     double timeToNextEffect = std::numeric_limits<double>::infinity();
-    double playerNextEffect;
+    bool didTriggerStyleRecalc = false;
     for (int i = m_players.size() - 1; i >= 0; --i) {
-        if (!m_players[i]->update(&playerNextEffect))
+        double playerNextEffect;
+        bool playerDidTriggerStyleRecalc;
+        if (!m_players[i]->update(&playerNextEffect, &playerDidTriggerStyleRecalc))
             m_players.remove(i);
+        didTriggerStyleRecalc |= playerDidTriggerStyleRecalc;
         if (playerNextEffect < timeToNextEffect)
             timeToNextEffect = playerNextEffect;
     }
@@ -103,6 +104,8 @@ void DocumentTimeline::serviceAnimations(double monotonicAnimationStartTime)
 
     if (m_document->view() && !m_players.isEmpty())
         m_document->view()->scheduleAnimation();
+
+    return didTriggerStyleRecalc;
 }
 
 void DocumentTimeline::setZeroTime(double zeroTime)

diff --git a/third_party/WebKit/Source/core/animation/DocumentTimeline.h b/third_party/WebKit/Source/core/animation/DocumentTimeline.h
@@ -60,7 +60,8 @@ class DocumentTimeline : public RefCounted<DocumentTimeline> {
     };
 
     static PassRefPtr<DocumentTimeline> create(Document*, PassOwnPtr<PlatformTiming> = nullptr);
-    void serviceAnimations(double);
+    // Returns whether style recalc was triggered.
+    bool serviceAnimations();
     PassRefPtr<Player> play(TimedItem*);
     // Called from setReadyState() in Document.cpp to set m_zeroTime to
     // performance.timing.domInteractive

diff --git a/third_party/WebKit/Source/core/animation/DocumentTimelineTest.cpp b/third_party/WebKit/Source/core/animation/DocumentTimelineTest.cpp
@@ -99,6 +99,12 @@ class CoreAnimationDocumentTimelineTest : public ::testing::Test {
         element.release();
     }
 
+    void updateClockAndService(double time)
+    {
+        document->animationClock().updateTime(time);
+        timeline->serviceAnimations();
+    }
+
     RefPtr<Document> document;
     RefPtr<Element> element;
     RefPtr<DocumentTimeline> timeline;
@@ -114,7 +120,6 @@ class CoreAnimationDocumentTimelineTest : public ::testing::Test {
     {
         return DocumentTimeline::s_minimumDelay;
     }
-
 };
 
 TEST_F(CoreAnimationDocumentTimelineTest, EmptyKeyframeAnimation)
@@ -125,31 +130,65 @@ TEST_F(CoreAnimationDocumentTimelineTest, EmptyKeyframeAnimation)
     timeline->play(anim.get());
 
     platformTiming->expectNoMoreActions();
-    timeline->serviceAnimations(0);
+    updateClockAndService(0);
     EXPECT_FLOAT_EQ(0, timeline->currentTime());
     EXPECT_TRUE(anim->compositableValues()->isEmpty());
 
     platformTiming->expectNoMoreActions();
-    timeline->serviceAnimations(100);
+    updateClockAndService(100);
     EXPECT_FLOAT_EQ(100, timeline->currentTime());
 }
 
-TEST_F(CoreAnimationDocumentTimelineTest, ZeroTimeAsPerfTime)
+TEST_F(CoreAnimationDocumentTimelineTest, EmptyTimelineDoesNotTriggerStyleRecalc)
+{
+    document->animationClock().updateTime(100);
+    EXPECT_FALSE(timeline->serviceAnimations());
+}
+
+TEST_F(CoreAnimationDocumentTimelineTest, EmptyPlayerDoesNotTriggerStyleRecalc)
+{
+    timeline->play(0);
+    document->animationClock().updateTime(100);
+    EXPECT_FALSE(timeline->serviceAnimations());
+}
+
+TEST_F(CoreAnimationDocumentTimelineTest, EmptyTargetDoesNotTriggerStyleRecalc)
+{
+    timing.iterationDuration = 200;
+    timeline->play(Animation::create(0, KeyframeAnimationEffect::create(KeyframeAnimationEffect::KeyframeVector()), timing).get());
+    document->animationClock().updateTime(100);
+    EXPECT_FALSE(timeline->serviceAnimations());
+}
+
+TEST_F(CoreAnimationDocumentTimelineTest, EmptyEffectDoesNotTriggerStyleRecalc)
+{
+    timeline->play(Animation::create(element.get(), 0, timing).get());
+    document->animationClock().updateTime(100);
+    EXPECT_FALSE(timeline->serviceAnimations());
+}
+
+TEST_F(CoreAnimationDocumentTimelineTest, TriggerStyleRecalc)
+{
+    timeline->play(Animation::create(element.get(), KeyframeAnimationEffect::create(KeyframeAnimationEffect::KeyframeVector()), timing).get());
+    document->animationClock().updateTime(100);
+    EXPECT_TRUE(timeline->serviceAnimations());
+}
+
+TEST_F(CoreAnimationDocumentTimelineTest, ZeroTime)
 {
     timeline = DocumentTimeline::create(document.get());
 
-    timeline->serviceAnimations(100);
+    document->animationClock().updateTime(100);
     EXPECT_TRUE(isNull(timeline->currentTime()));
 
-    timeline->serviceAnimations(200);
+    document->animationClock().updateTime(200);
     EXPECT_TRUE(isNull(timeline->currentTime()));
 
     timeline->setZeroTime(300);
     document->animationClock().updateTime(300);
-    timeline->serviceAnimations(300);
     EXPECT_EQ(0, timeline->currentTime());
 
-    timeline->serviceAnimations(400);
+    document->animationClock().updateTime(400);
     EXPECT_EQ(100, timeline->currentTime());
 }
 
@@ -200,22 +239,21 @@ TEST_F(CoreAnimationDocumentTimelineTest, NumberOfActiveAnimations)
     RefPtr<Player> player4 = timeline->play(anim4.get());
 
     platformTiming->expectNextFrameAction();
-    timeline->serviceAnimations(0);
+    updateClockAndService(0);
     EXPECT_EQ(4U, timeline->numberOfActiveAnimationsForTesting());
     platformTiming->expectNextFrameAction();
-    timeline->serviceAnimations(0.5);
+    updateClockAndService(0.5);
     EXPECT_EQ(4U, timeline->numberOfActiveAnimationsForTesting());
     platformTiming->expectNextFrameAction();
-    timeline->serviceAnimations(1.5);
+    updateClockAndService(1.5);
     EXPECT_EQ(4U, timeline->numberOfActiveAnimationsForTesting());
     platformTiming->expectNoMoreActions();
-    timeline->serviceAnimations(3);
+    updateClockAndService(3);
     EXPECT_EQ(1U, timeline->numberOfActiveAnimationsForTesting());
 }
 
 TEST_F(CoreAnimationDocumentTimelineTest, DelayBeforeAnimationStart)
 {
-
     timing.hasIterationDuration = true;
     timing.iterationDuration = 2;
     timing.startDelay = 5;
@@ -226,16 +264,16 @@ TEST_F(CoreAnimationDocumentTimelineTest, DelayBeforeAnimationStart)
 
     // TODO: Put the player startTime in the future when we add the capability to change player startTime
     platformTiming->expectDelayedAction(timing.startDelay - minimumDelay());
-    timeline->serviceAnimations(0);
+    updateClockAndService(0);
 
     platformTiming->expectDelayedAction(timing.startDelay - minimumDelay() - 1.5);
-    timeline->serviceAnimations(1.5);
+    updateClockAndService(1.5);
 
     EXPECT_CALL(*platformTiming, serviceOnNextFrame());
     wake();
 
     platformTiming->expectNextFrameAction();
-    timeline->serviceAnimations(4.98);
+    updateClockAndService(4.98);
 }
 
 }
diff --git a/third_party/WebKit/Source/core/animation/InertAnimation.h b/third_party/WebKit/Source/core/animation/InertAnimation.h
@@ -46,7 +46,7 @@ class InertAnimation FINAL : public TimedItem {
     bool paused() const { return m_paused; }
 
 protected:
-    virtual void updateChildrenAndEffects() const OVERRIDE { };
+    virtual bool updateChildrenAndEffects() const OVERRIDE { return false; };
     virtual void willDetach() OVERRIDE { };
     virtual double calculateTimeToEffectChange(double inheritedTime, double activeTime, Phase) const OVERRIDE FINAL;
 

diff --git a/third_party/WebKit/Source/core/animation/Player.cpp b/third_party/WebKit/Source/core/animation/Player.cpp
@@ -90,18 +90,22 @@ double Player::currentTime() const
     return currentTimeBeforeDrift() - timeDrift();
 }
 
-bool Player::update(double* timeToEffectChange)
+bool Player::update(double* timeToEffectChange, bool* didTriggerStyleRecalc)
 {
     if (!m_content) {
         if (timeToEffectChange)
             *timeToEffectChange = std::numeric_limits<double>::infinity();
+        if (didTriggerStyleRecalc)
+            *didTriggerStyleRecalc = false;
         return false;
     }
 
     double newTime = isNull(m_timeline.currentTime()) ? nullValue() : currentTime();
-    m_content->updateInheritedTime(newTime);
+    bool didTriggerStyleRecalcLocal = m_content->updateInheritedTime(newTime);
     if (timeToEffectChange)
         *timeToEffectChange = m_content->timeToEffectChange();
+    if (didTriggerStyleRecalc)
+        *didTriggerStyleRecalc = didTriggerStyleRecalcLocal;
     return m_content->isCurrent() || m_content->isInEffect();
 }
 

diff --git a/third_party/WebKit/Source/core/animation/Player.h b/third_party/WebKit/Source/core/animation/Player.h
@@ -49,7 +49,7 @@ class Player FINAL : public RefCounted<Player> {
     //  infinity  - if this player is no longer in effect
     //  0         - if this player requires an update on the next frame
     //  n         - if this player requires an update after 'n' units of time
-    bool update(double* timeToEffectChange = 0);
+    bool update(double* timeToEffectChange = 0, bool* didTriggerStyleRecalc = 0);
 
     void cancel();
     double currentTime() const;

diff --git a/third_party/WebKit/Source/core/animation/PlayerTest.cpp b/third_party/WebKit/Source/core/animation/PlayerTest.cpp
@@ -56,7 +56,7 @@ class CoreAnimationPlayerTest : public ::testing::Test {
 
     bool updateTimeline(double time, double* timeToEffectChange = 0)
     {
-        timeline->serviceAnimations(time);
+        document->animationClock().updateTime(time);
         // The timeline does not know about our player, so we have to explicitly call update().
         return player->update(timeToEffectChange);
     }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		Tests that having stopped a transition before it completes, a subsequent transition starts correctly.

		PASS