REGRESSION (r268483): Map jumps around while zooming on windy.com, strava.com

https://bugs.webkit.org/show_bug.cgi?id=217987
<rdar://problem/70418548>

Reviewed by Simon Fraser.

Source/WebCore:

When several animations targetting the same property and the same layer are overlapping, we used to
always override the previous animations. With r268483 we started maintaining all active animations
and let them run, potentially with additivity if the animation could be broken into several animations
each targeting a given transform operation.

On top of that, with r268615 and the support for accelerated animation of individual CSS transform
properties (translate, scale and rotate), all transform-related animations were made additive.

This meant that we would always run active animations targeting "transform" in a way where they would be
additive rather than being replaced.

Any animation targeting "transform" will yield one or several accelerated animations, and the first of this
animation set will always have a 0 index. So now, when we compile a list of transform animations in
GraphicsLayerCA::updateAnimations(), we reset that list any time we encounted an animation with a 0 index,
ensuring only the top-most transform animation is applied.

We also fix an issue where we didn't account for the possibility that a single KeyframeEffect could yield
several transform animations with the same name in pauseAnimation() and removeAnimation(). We now pause or
remove all animations with the provided name.

Test: webanimations/accelerated-overlapping-transform-animations.html

* platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::pauseAnimation):
(WebCore::GraphicsLayerCA::removeAnimation):
(WebCore::GraphicsLayerCA::updateAnimations):

LayoutTests:

Add a new test that checks that only the last of two overlapping "transform" animations is applied.

* platform/mac-wk1/TestExpectations:
* webanimations/accelerated-overlapping-transform-animations-expected.html: Added.
* webanimations/accelerated-overlapping-transform-animations.html: Added.

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

Author: [email protected]

LayoutTests/ChangeLog

@@ -1,3 +1,17 @@
+2020-10-20  Antoine Quint  <[email protected]>
+
+        REGRESSION (r268483): Map jumps around while zooming on windy.com, strava.com
+        https://bugs.webkit.org/show_bug.cgi?id=217987
+        <rdar://problem/70418548>
+
+        Reviewed by Simon Fraser.
+
+        Add a new test that checks that only the last of two overlapping "transform" animations is applied. 
+
+        * platform/mac-wk1/TestExpectations:
+        * webanimations/accelerated-overlapping-transform-animations-expected.html: Added.
+        * webanimations/accelerated-overlapping-transform-animations.html: Added.
+
 2020-10-20  Truitt Savell  <[email protected]>
 
         imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audioworklet-interface/audioworkletnode-output-channel-count.https.html is a flaky failure

LayoutTests/platform/mac-wk1/TestExpectations

@@ -1199,3 +1199,4 @@ webkit.org/b/217761 [ Mojave+ Debug ] webgl/2.0.0/conformance/extensions/webgl-c
 
 webkit.org/b/217934 inspector/debugger/breakpoint-resolve-when-script-added.html [ Skip ]
 
+webkit.org/b/217997 webanimations/accelerated-overlapping-transform-animations.html [ Pass Failure ]

LayoutTests/webanimations/accelerated-overlapping-transform-animations-expected.html

@@ -0,0 +1,14 @@
+<style>
+
+    #target {
+        position: absolute;
+        top: 0;
+        left: 0;
+        width: 100px;
+        height: 100px;
+        background-color: black;
+        transform: translateX(200px);
+    }
+
+</style>
+<div id="target"></div>

LayoutTests/webanimations/accelerated-overlapping-transform-animations.html

@@ -0,0 +1,44 @@
+<style>
+
+    #target {
+        position: absolute;
+        top: 0;
+        left: 0;
+        width: 100px;
+        height: 100px;
+        background-color: black;
+    }
+
+</style>
+<div id="target"></div>
+<script src="../resources/ui-helper.js"></script>
+<script>
+
+(async () => {
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    const target = document.getElementById("target");
+
+    // Make the animations last a whole minute so they don't end while the test is running. 
+    const duration = 60 * 1000;
+
+    // Start a first stationary "transform" animation.
+    const firstAnimation = target.animate({ transform: ["translateX(100px)", "translateX(100px)"] }, duration);
+
+    // Wait until this first animation has been applied.
+    await firstAnimation.ready;
+    await UIHelper.ensureStablePresentationUpdate();
+
+    // Now start a second stationary "transform" animation, which should replace the previous animation.
+    const secondAnimation = target.animate({ transform: ["translateX(200px)", "translateX(200px)"] }, duration);
+
+    // Wait until this second animation has been applied.
+    await secondAnimation.ready;
+    await UIHelper.ensureStablePresentationUpdate();
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+})();
+
+</script>

Source/WebCore/ChangeLog

@@ -1,3 +1,38 @@
+2020-10-20  Antoine Quint  <[email protected]>
+
+        REGRESSION (r268483): Map jumps around while zooming on windy.com, strava.com
+        https://bugs.webkit.org/show_bug.cgi?id=217987
+        <rdar://problem/70418548>
+
+        Reviewed by Simon Fraser.
+
+        When several animations targetting the same property and the same layer are overlapping, we used to
+        always override the previous animations. With r268483 we started maintaining all active animations
+        and let them run, potentially with additivity if the animation could be broken into several animations
+        each targeting a given transform operation.
+
+        On top of that, with r268615 and the support for accelerated animation of individual CSS transform
+        properties (translate, scale and rotate), all transform-related animations were made additive.
+
+        This meant that we would always run active animations targeting "transform" in a way where they would be
+        additive rather than being replaced.
+
+        Any animation targeting "transform" will yield one or several accelerated animations, and the first of this
+        animation set will always have a 0 index. So now, when we compile a list of transform animations in
+        GraphicsLayerCA::updateAnimations(), we reset that list any time we encounted an animation with a 0 index,
+        ensuring only the top-most transform animation is applied.
+
+        We also fix an issue where we didn't account for the possibility that a single KeyframeEffect could yield
+        several transform animations with the same name in pauseAnimation() and removeAnimation(). We now pause or
+        remove all animations with the provided name.
+
+        Test: webanimations/accelerated-overlapping-transform-animations.html
+
+        * platform/graphics/ca/GraphicsLayerCA.cpp:
+        (WebCore::GraphicsLayerCA::pauseAnimation):
+        (WebCore::GraphicsLayerCA::removeAnimation):
+        (WebCore::GraphicsLayerCA::updateAnimations):
+
 2020-10-20  Jer Noble  <[email protected]>
 
         [media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler()

Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

@@ -1079,34 +1079,30 @@ void GraphicsLayerCA::pauseAnimation(const String& animationName, double timeOff
 {
     LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " pauseAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
 
-    auto index = m_animations.findMatching([&](LayerPropertyAnimation animation) {
-        return animation.m_name == animationName && !animation.m_pendingRemoval;
-    });
-
-    if (index == notFound)
-        return;
-
-    auto& animation = m_animations[index];
-    animation.m_playState = PlayState::PausePending;
-    animation.m_timeOffset = Seconds { timeOffset };
+    for (auto& animation : m_animations) {
+        // There may be several animations with the same name in the case of transform animations
+        // animating multiple components as individual animations.
+        if (animation.m_name == animationName && !animation.m_pendingRemoval) {
+            animation.m_playState = PlayState::PausePending;
+            animation.m_timeOffset = Seconds { timeOffset };
 
-    noteLayerPropertyChanged(AnimationChanged);
+            noteLayerPropertyChanged(AnimationChanged);
+        }
+    }
 }
 
 void GraphicsLayerCA::removeAnimation(const String& animationName)
 {
     LOG_WITH_STREAM(Animations, stream << "GraphicsLayerCA " << this << " id " << primaryLayerID() << " removeAnimation " << animationName << " (is running " << animationIsRunning(animationName) << ")");
 
-    auto index = m_animations.findMatching([&](LayerPropertyAnimation animation) {
-        return animation.m_name == animationName && !animation.m_pendingRemoval;
-    });
-
-    if (index == notFound)
-        return;
-
-    m_animations[index].m_pendingRemoval = true;
-
-    noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
+    for (auto& animation : m_animations) {
+        // There may be several animations with the same name in the case of transform animations
+        // animating multiple components as individual animations.
+        if (animation.m_name == animationName && !animation.m_pendingRemoval) {
+            animation.m_pendingRemoval = true;
+            noteLayerPropertyChanged(AnimationChanged | CoverageRectChanged);
+        }
+    }
 }
 
 void GraphicsLayerCA::platformCALayerAnimationStarted(const String& animationKey, MonotonicTime startTime)
@@ -2960,6 +2956,14 @@ void GraphicsLayerCA::updateAnimations()
             rotateAnimation = &animation;
             break;
         case AnimatedPropertyTransform:
+            // In the case of animations targeting the "transform" CSS property, there may be several
+            // animations created for a single KeyframeEffect, one for each transform component. In that
+            // case the animation index starts at 0 and increases for each component. If we encounter an
+            // index of 0 this means this animation establishes a new group of animation belonging to a
+            // single KeyframeEffect. As such, since the top-most KeyframeEffect replaces the previous
+            // ones, we can remove all the previously-added "transform" animations.
+            if (!animation.m_index)
+                transformAnimations.clear();
             transformAnimations.append(&animation);
             break;
         case AnimatedPropertyOpacity: