Scrolling away from and back to an individually playing animation wil…

…l cause it to be incorrectly paused https://bugs.webkit.org/show_bug.cgi?id=251759 rdar://105059884 Reviewed by Andres Gonzalez. Given this sequence: 1. Pause All Animations on the page 2. Individually play an animation, notice it is animating correctly. 3. Scroll away from the animation such that it's off-screen 4. Scroll back to the animation Notice the animation is paused when it should be playing (because it's respecting the page-wide pause state over its own play-state). This happened because RenderElement::repaintForPausedImageAnimationsIfNeeded referenced the Page::imageAnimationEnabled state rather than RenderElement::allowsAnimation. The former does not respect an individual animation's play-state (i.e. it could be playing individually while the rest of the animations on the page are forced to be paused.) * LayoutTests/fast/images/individual-animation-toggle-expected.txt: * LayoutTests/fast/images/individual-animation-toggle.html: Add a testcase verifying this bug doesn't happen again. * Source/WebCore/rendering/RenderElement.h: * Source/WebCore/rendering/RenderElement.cpp: (WebCore::RenderElement::allowsAnimation const): (WebCore::RenderElement::repaintForPausedImageAnimationsIfNeeded): * Source/WebCore/rendering/RenderImage.cpp: (WebCore::RenderImage::allowsAnimation const): Deleted -- the logic is moved into RenderElement::allowsAnimation. * Source/WebCore/rendering/RenderImage.h: Canonical link: https://commits.webkit.org/259910@main
WebKit · Feb 6, 2023 · 9d10e5a · 9d10e5a
1 parent f2ad1fd
commit 9d10e5a
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 31 deletions.
diff --git a/LayoutTests/fast/images/individual-animation-toggle-expected.txt b/LayoutTests/fast/images/individual-animation-toggle-expected.txt
@@ -3,9 +3,18 @@ This test ensures that when image animation is disabled page-wide, individual im
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS internals.isImageAnimating(image) became false
-PASS internals.isImageAnimating(image) became true
-PASS internals.isImageAnimating(image) became false
+Pausing animations page-wide.
+PASS internals.isImageAnimating(gif) became false
+Resuming animation.
+PASS internals.isImageAnimating(gif) became false
+Pausing animation.
+PASS internals.isImageAnimating(gif) became false
+Resuming animation.
+PASS internals.isImageAnimating(gif) became false
+Scrolling animation off-screen. This should result in it becoming paused.
+PASS internals.isImageAnimating(gif) became false
+Scrolling to make animation visible again.
+PASS internals.isImageAnimating(gif) became true
 PASS successfullyParsed is true
 
 TEST COMPLETE

diff --git a/LayoutTests/fast/images/individual-animation-toggle.html b/LayoutTests/fast/images/individual-animation-toggle.html
@@ -3,31 +3,55 @@
 <head>
 <script src="../../resources/js-test.js"></script>
 </head>
-<body>
-<img src="resources/animation.gif" id="testImage" />
+<body height="2000px">
+<img src="resources/animation.gif" id="gif" />
 <!-- To make this test able to pass in WK1, use this <div style="..."> to setAutodisplay:YES (rdar://42625657 has more details). -->
 <div style="will-change: transform"></div>
 
 <script>
-    description("This test ensures that when image animation is disabled page-wide, individual image animations can still be toggled on and off individually.\n\n");
-    jsTestIsAsync = true;
+window.jsTestIsAsync = true;
+description("This test ensures that when image animation is disabled page-wide, individual image animations can still be toggled on and off individually.\n\n");
+
+async function toggleAnimationPlayState(shouldAnimate) {
+    if (shouldAnimate) {
+        debug("Resuming animation.");
+        internals.resumeImageAnimation(gif);
+        await shouldBecomeEqual("internals.isImageAnimating(gif)", "false");
+    } else {
+        debug("Pausing animation.");
+        internals.pauseImageAnimation(gif);
+        await shouldBecomeEqual("internals.isImageAnimating(gif)", "false");
+    }
+}
+
+var gif = document.getElementById("gif");
+window.onload = async function() {
+    if (!window.internals)
+        return;
     internals.settings.setImageAnimationControlEnabled(true);
 
+    debug("Pausing animations page-wide.");
     internals.setImageAnimationEnabled(false);
-    var image = document.getElementById("testImage");
-    window.onload = function() {
-        shouldBecomeEqual("internals.isImageAnimating(image)", "false", function() {
-            internals.resumeImageAnimation(image);
-            shouldBecomeEqual("internals.isImageAnimating(image)", "true", function() {
-                internals.pauseImageAnimation(image);
-                shouldBecomeEqual("internals.isImageAnimating(image)", "false", function() {
-                    internals.setImageAnimationEnabled(true);
-                    internals.clearMemoryCache();
-                    finishJSTest();
-                });
-            });
-        });
-    };
+    await shouldBecomeEqual("internals.isImageAnimating(gif)", "false");
+
+    await toggleAnimationPlayState(true);
+    await toggleAnimationPlayState(false);
+    await toggleAnimationPlayState(true);
+
+    debug("Scrolling animation off-screen. This should result in it becoming paused.");
+    window.scrollBy(0, 2000);
+    await shouldBecomeEqual("internals.isImageAnimating(gif)", "false");
+
+    debug("Scrolling to make animation visible again.");
+    window.scroll(0, 0);
+    await shouldBecomeEqual("internals.isImageAnimating(gif)", "true");
+
+    internals.setImageAnimationEnabled(true);
+    internals.settings.setImageAnimationControlEnabled(false);
+
+    internals.clearMemoryCache();
+    finishJSTest();
+};
 </script>
 
 </body>

diff --git a/Source/WebCore/rendering/RenderElement.cpp b/Source/WebCore/rendering/RenderElement.cpp
@@ -1461,6 +1461,8 @@ void RenderElement::notifyFinished(CachedResource& resource, const NetworkLoadMe
 
 bool RenderElement::allowsAnimation() const
 {
+    if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element()))
+        return imageElement->allowsAnimation();
     return page().imageAnimationEnabled();
 }
 
@@ -1479,7 +1481,7 @@ void RenderElement::scheduleRenderingUpdateForImage(CachedImage&)
 bool RenderElement::repaintForPausedImageAnimationsIfNeeded(const IntRect& visibleRect, CachedImage& cachedImage)
 {
     ASSERT(m_hasPausedImageAnimations);
-    if (!page().imageAnimationEnabled() || !isVisibleInDocumentRect(visibleRect))
+    if (!allowsAnimation() || !isVisibleInDocumentRect(visibleRect))
         return false;
 
     repaint();

diff --git a/Source/WebCore/rendering/RenderElement.h b/Source/WebCore/rendering/RenderElement.h
@@ -242,7 +242,7 @@ class RenderElement : public RenderObject {
     bool didContibuteToVisuallyNonEmptyPixelCount() const { return m_didContributeToVisuallyNonEmptyPixelCount; }
     void setDidContibuteToVisuallyNonEmptyPixelCount() { m_didContributeToVisuallyNonEmptyPixelCount = true; }
 
-    bool allowsAnimation() const override;
+    bool allowsAnimation() const final;
     bool repaintForPausedImageAnimationsIfNeeded(const IntRect& visibleRect, CachedImage&);
     bool hasPausedImageAnimations() const { return m_hasPausedImageAnimations; }
     void setHasPausedImageAnimations(bool b) { m_hasPausedImageAnimations = b; }

diff --git a/Source/WebCore/rendering/RenderImage.cpp b/Source/WebCore/rendering/RenderImage.cpp
@@ -897,11 +897,4 @@ RenderBox* RenderImage::embeddedContentBox() const
     return nullptr;
 }
 
-bool RenderImage::allowsAnimation() const
-{
-    if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element()))
-        return imageElement->allowsAnimation();
-    return RenderReplaced::allowsAnimation();
-}
-
 } // namespace WebCore
diff --git a/Source/WebCore/rendering/RenderImage.h b/Source/WebCore/rendering/RenderImage.h
@@ -82,7 +82,6 @@ class RenderImage : public RenderReplaced {
     String accessibilityDescription() const { return imageResource().image()->accessibilityDescription(); }
 
     bool hasAnimatedImage() const;
-    bool allowsAnimation() const final;
 
 protected:
     void willBeDestroyed() override;