[Web Animations] Changing the delay of an accelerated animation doesn't seek the...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jan 2020 13:47:58 +0000 (13:47 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jan 2020 13:47:58 +0000 (13:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206990
<rdar://problem/58675608>

Reviewed by Antti Koivisto.

Source/WebCore:

Test: webanimations/seeking-by-changing-delay-accelerated.html

In order to seek an accelerated animation, we need to update the animation on the target element's backing GraphicsLayer. We do this by enqueuing an
AcceleratedAction:Seek command which is done by calling KeyframeEffect::animationDidSeek(), which we would only call from WebAnimation::setCurrentTime().
However, seeking can be performed by modifying the animation's effect's timing.

We now call WebAnimation::effectTimingDidChange() with an optional ComputedEffectTiming for call sites that want to provide timing data prior to
modifying timing properties. This allows WebAnimation::effectTimingDidChange() to compare the previous progress with the new progress to determine if the
animation was seeked, so KeyframeEffect::animationDidSeek() may be called.

There are two places where we now call WebAnimation::effectTimingDidChange() with the previous timing data. First, when updateTiming() is called
through the JavaScript API (AnimationEffect::updateTiming) and when a CSS Animation's timing has been modified by changing some of the animation CSS
properties (CSSAnimation::syncPropertiesWithBackingAnimation).

* animation/AnimationEffect.cpp:
(WebCore::AnimationEffect::updateTiming): Compute the previous timing data and provide it to WebAnimation::effectTimingDidChange().
* animation/CSSAnimation.cpp:
(WebCore::CSSAnimation::syncPropertiesWithBackingAnimation): Compute the previous timing data and provide it to WebAnimation::effectTimingDidChange().
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::computeAcceleratedPropertiesState): Drive-by fix for faulty logic introduced in a recent patch (r255383).
(WebCore::KeyframeEffect::applyPendingAcceleratedActions): We need to reset the m_isRunningAccelerated flag when an animation was supposed to be stopped but
couldn't be because the target's layer backing was removed prior to the accelerated action being committed.
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::effectTimingDidChange): If previous timing data was provided, check whether its progress differs from the current timing data and
call KeyframeEffect::animationDidSeek().
* animation/WebAnimation.h:

LayoutTests:

Add a new test which would fail prior to this patch where we pause an animation after it has started playing accelerated and
change its delay to check that it correctly seeks the animation.

* webanimations/seeking-by-changing-delay-accelerated-expected.html: Added.
* webanimations/seeking-by-changing-delay-accelerated.html: Added.
* platform/win/TestExpectations: Mark the new test as failing.

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

LayoutTests/ChangeLog
LayoutTests/platform/win/TestExpectations
LayoutTests/webanimations/seeking-by-changing-delay-accelerated-expected.html [new file with mode: 0644]
LayoutTests/webanimations/seeking-by-changing-delay-accelerated.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationEffect.cpp
Source/WebCore/animation/CSSAnimation.cpp
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h

index 075a256..929d05e 100644 (file)
@@ -1,3 +1,18 @@
+2020-01-30  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Changing the delay of an accelerated animation doesn't seek the animation
+        https://bugs.webkit.org/show_bug.cgi?id=206990
+        <rdar://problem/58675608>
+
+        Reviewed by Antti Koivisto.
+
+        Add a new test which would fail prior to this patch where we pause an animation after it has started playing accelerated and
+        change its delay to check that it correctly seeks the animation.
+
+        * webanimations/seeking-by-changing-delay-accelerated-expected.html: Added.
+        * webanimations/seeking-by-changing-delay-accelerated.html: Added.
+        * platform/win/TestExpectations: Mark the new test as failing.
+
 2020-01-30  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         REGRESSION(r253636): [GTK] Mouse cursor changes using onMouseXYZ are erratic
index deefcf8..e2c7f83 100644 (file)
@@ -4528,3 +4528,4 @@ webkit.org/b/205856 storage/indexeddb/IDBTransaction-page-cache.html [ Pass Time
 
 webkit.org/b/206165 fast/dom/Range/getBoundingClientRect.html [ Failure ]
 
+webkit.org/b/206995 webanimations/seeking-by-changing-delay-accelerated.html [ Failure ]
diff --git a/LayoutTests/webanimations/seeking-by-changing-delay-accelerated-expected.html b/LayoutTests/webanimations/seeking-by-changing-delay-accelerated-expected.html
new file mode 100644 (file)
index 0000000..4ec430d
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+
+#target {
+    position: absolute;
+    left: 0;
+    top: 0;
+    width: 100px;
+    height: 100px;
+    background-color: black;
+    transform: translateX(50px);
+}
+
+</style>
+</head>
+<body>
+<div id="target"></div>
+</body>
+</html>
diff --git a/LayoutTests/webanimations/seeking-by-changing-delay-accelerated.html b/LayoutTests/webanimations/seeking-by-changing-delay-accelerated.html
new file mode 100644 (file)
index 0000000..03e3803
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+
+#target {
+    position: absolute;
+    left: 0;
+    top: 0;
+    width: 100px;
+    height: 100px;
+    background-color: black;
+}
+
+</style>
+</head>
+<body>
+<div id="target"></div>
+<script>
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+const animation = document.getElementById("target").animate({ transform: ["translateX(0)", "translateX(100px)"] }, 1000);
+
+animation.ready.then(() => {
+    // Ensure accelerated animations have been commmitted by waiting for the next frame.
+    requestAnimationFrame(() => {
+        // We pause the animation right away.
+        animation.pause();
+
+        // Then on the next frame, we change its delay to seek it to its mid-point.
+        requestAnimationFrame(() => {
+            const effect = animation.effect;
+            const duration = effect.getTiming().duration;
+            const delay = animation.currentTime - 0.5 * duration;
+            animation.effect.updateTiming({ delay });
+
+            // Then wait another frame to ensure the seek was committed.
+            if (window.testRunner)
+                requestAnimationFrame(() => testRunner.notifyDone());
+        });
+    });
+});
+
+</script>
+</body>
+</html>
index 458a2e4..fdcfe02 100644 (file)
@@ -1,3 +1,38 @@
+2020-01-30  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Changing the delay of an accelerated animation doesn't seek the animation
+        https://bugs.webkit.org/show_bug.cgi?id=206990
+        <rdar://problem/58675608>
+
+        Reviewed by Antti Koivisto.
+
+        Test: webanimations/seeking-by-changing-delay-accelerated.html
+
+        In order to seek an accelerated animation, we need to update the animation on the target element's backing GraphicsLayer. We do this by enqueuing an
+        AcceleratedAction:Seek command which is done by calling KeyframeEffect::animationDidSeek(), which we would only call from WebAnimation::setCurrentTime().
+        However, seeking can be performed by modifying the animation's effect's timing.
+
+        We now call WebAnimation::effectTimingDidChange() with an optional ComputedEffectTiming for call sites that want to provide timing data prior to
+        modifying timing properties. This allows WebAnimation::effectTimingDidChange() to compare the previous progress with the new progress to determine if the
+        animation was seeked, so KeyframeEffect::animationDidSeek() may be called.
+
+        There are two places where we now call WebAnimation::effectTimingDidChange() with the previous timing data. First, when updateTiming() is called
+        through the JavaScript API (AnimationEffect::updateTiming) and when a CSS Animation's timing has been modified by changing some of the animation CSS
+        properties (CSSAnimation::syncPropertiesWithBackingAnimation).
+
+        * animation/AnimationEffect.cpp:
+        (WebCore::AnimationEffect::updateTiming): Compute the previous timing data and provide it to WebAnimation::effectTimingDidChange().
+        * animation/CSSAnimation.cpp:
+        (WebCore::CSSAnimation::syncPropertiesWithBackingAnimation): Compute the previous timing data and provide it to WebAnimation::effectTimingDidChange().
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::computeAcceleratedPropertiesState): Drive-by fix for faulty logic introduced in a recent patch (r255383).
+        (WebCore::KeyframeEffect::applyPendingAcceleratedActions): We need to reset the m_isRunningAccelerated flag when an animation was supposed to be stopped but
+        couldn't be because the target's layer backing was removed prior to the accelerated action being committed.
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::effectTimingDidChange): If previous timing data was provided, check whether its progress differs from the current timing data and
+        call KeyframeEffect::animationDidSeek().
+        * animation/WebAnimation.h:
+
 2020-01-30  Noam Rosenthal  <noam@webkit.org>
 
         REGRESSION (r254406): Gmail.com star/favorite icons are not rendering
index b184973..da8a61c 100644 (file)
@@ -345,6 +345,10 @@ ExceptionOr<void> AnimationEffect::updateTiming(Optional<OptionalEffectTiming> t
     if (!timing)
         return { };
 
+    Optional<ComputedEffectTiming> previousTiming;
+    if (m_animation)
+        previousTiming = getComputedTiming();
+
     // 1. If the iterationStart member of input is present and less than zero, throw a TypeError and abort this procedure.
     if (timing->iterationStart) {
         if (timing->iterationStart.value() < 0)
@@ -413,7 +417,7 @@ ExceptionOr<void> AnimationEffect::updateTiming(Optional<OptionalEffectTiming> t
     updateStaticTimingProperties();
 
     if (m_animation)
-        m_animation->effectTimingDidChange();
+        m_animation->effectTimingDidChange(previousTiming);
 
     return { };
 }
index fd3eb9d..58ada03 100644 (file)
@@ -65,6 +65,8 @@ void CSSAnimation::syncPropertiesWithBackingAnimation()
     auto& animation = backingAnimation();
     auto* animationEffect = effect();
 
+    auto previousTiming = animationEffect->getComputedTiming();
+
     switch (animation.fillMode()) {
     case AnimationFillMode::None:
         animationEffect->setFill(FillMode::None);
@@ -101,7 +103,7 @@ void CSSAnimation::syncPropertiesWithBackingAnimation()
     animationEffect->setDelay(Seconds(animation.delay()));
     animationEffect->setIterationDuration(Seconds(animation.duration()));
     animationEffect->updateStaticTimingProperties();
-    effectTimingDidChange();
+    effectTimingDidChange(previousTiming);
 
     // Synchronize the play state
     if (animation.playState() == AnimationPlayState::Playing && playState() == WebAnimation::PlayState::Paused) {
index 72cba67..1674f0a 100644 (file)
@@ -1127,7 +1127,7 @@ void KeyframeEffect::computeAcceleratedPropertiesState()
             break;
     }
 
-    if (!hasSomeAcceleratedProperties && !hasSomeUnacceleratedProperties)
+    if (!hasSomeAcceleratedProperties)
         m_acceleratedPropertiesState = AcceleratedProperties::None;
     else if (hasSomeUnacceleratedProperties)
         m_acceleratedPropertiesState = AcceleratedProperties::Some;
@@ -1410,8 +1410,10 @@ void KeyframeEffect::applyPendingAcceleratedActions()
     if (!renderer || !renderer->isComposited()) {
         // The renderer may no longer be composited because the accelerated animation ended before we had a chance to update it,
         // in which case if we asked for the animation to stop, we can discard the current set of accelerated actions.
-        if (m_lastRecordedAcceleratedAction == AcceleratedAction::Stop)
+        if (m_lastRecordedAcceleratedAction == AcceleratedAction::Stop) {
             m_pendingAcceleratedActions.clear();
+            m_isRunningAccelerated = false;
+        }
         return;
     }
 
index a8b48c2..bf77edb 100644 (file)
@@ -144,11 +144,19 @@ void WebAnimation::unsuspendEffectInvalidation()
     --m_suspendCount;
 }
 
-void WebAnimation::effectTimingDidChange()
+void WebAnimation::effectTimingDidChange(Optional<ComputedEffectTiming> previousTiming)
 {
     timingDidChange(DidSeek::No, SynchronouslyNotify::Yes);
 
     InspectorInstrumentation::didChangeWebAnimationEffectTiming(*this);
+
+    if (!previousTiming)
+        return;
+
+    auto* effect = this->effect();
+    ASSERT(effect);
+    if (previousTiming->progress != effect->getComputedTiming().progress)
+        effect->animationDidSeek();
 }
 
 void WebAnimation::setEffect(RefPtr<AnimationEffect>&& newEffect)
index 0e037fc..d3b1b98 100644 (file)
 #pragma once
 
 #include "ActiveDOMObject.h"
+#include "ComputedEffectTiming.h"
 #include "EventTarget.h"
 #include "ExceptionOr.h"
 #include "IDLTypes.h"
 #include "WebAnimationUtilities.h"
 #include <wtf/Forward.h>
 #include <wtf/Markable.h>
+#include <wtf/Optional.h>
 #include <wtf/RefCounted.h>
 #include <wtf/Seconds.h>
 #include <wtf/UniqueRef.h>
@@ -126,7 +128,7 @@ public:
     bool isCompletelyAccelerated() const;
     bool isRelevant() const { return m_isRelevant; }
     void updateRelevance();
-    void effectTimingDidChange();
+    void effectTimingDidChange(Optional<ComputedEffectTiming> = WTF::nullopt);
     void suspendEffectInvalidation();
     void unsuspendEffectInvalidation();
     void setSuspended(bool);