Changeset 268771 in webkit

Timestamp:

Oct 20, 2020, 3:58:48 PM (6 years ago)

Author:

graouts@webkit.org

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac-wk1/TestExpectations (modified) (1 diff)
• LayoutTests/webanimations/accelerated-overlapping-transform-animations-expected.html (added)
• LayoutTests/webanimations/accelerated-overlapping-transform-animations.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-10-20  Antoine Quint  <graouts@webkit.org>
+
+        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  <tsavell@apple.com>
 

trunk/LayoutTests/platform/mac-wk1/TestExpectations

@@ -1200 +1200 @@
 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 ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-10-20  Antoine Quint  <graouts@webkit.org>
+
+        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  <jer.noble@apple.com>
 

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

@@ -1080 +1080 @@
     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 };
-
-    noteLayerPropertyChanged(AnimationChanged);
+    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);
+        }
+    }
 }
 
@@ -1098 +1096 @@
     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);
+        }
+    }
 }
 
@@ -2961 +2957 @@
             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;