Bug 251738 – Individually paused / playing animations are not effected by Play All Animations and Pause All Animations

• Component: Animations
• Status: RESOLVED FIXED

[Tyler Wilcock]

Given this sequence:

  1. Load a page with a GIF animation
  2. Individually pause that animation
  3. Perform Play All Animations

The animation does not start playing again.

[Tyler Wilcock]

Created attachment 464834 [details]
Patch

[Tyler Wilcock]

Created attachment 464835 [details]
Patch

[Tyler Wilcock]

Created attachment 464836 [details]
Patch

[Tyler Wilcock]

Created attachment 464838 [details]
Patch

[Tyler Wilcock]

Created attachment 464839 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> Created attachment 464839 [details]
> Patch

--- a/Source/WebCore/page/Page.cpp
+++ b/Source/WebCore/page/Page.cpp
@@ -2003,15 +2003,12 @@ void Page::setLoadSchedulingMode(LoadSchedulingMode mode)
 #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
 void Page::setImageAnimationEnabled(bool enabled)
 {
-    if (m_imageAnimationEnabled == enabled || !settings().imageAnimationControlEnabled())
+    if (!settings().imageAnimationControlEnabled())

If I"m following the logic here, you remove the check m_imageAnimationEnabled == enabled to override the animation state of all images, not only the global state, which seems to be the cause of the bug. Would it be worth to spell out that in a comment here?

--- a/Source/WebCore/rendering/RenderView.cpp
+++ b/Source/WebCore/rendering/RenderView.cpp
@@ -967,6 +969,12 @@ void RenderView::updatePlayStateForAllAnimations(const IntRect& visibleRect)
                     addRendererWithPausedImageAnimations(renderElement, *cachedImage);
                 }
             } else if (image && image->isAnimated()) {
+                // Override any individual animation play state that may have been set.
+                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(renderElement.element()))
+                    imageElement->setAllowsAnimation(std::nullopt);
+                else
+                    image->setAllowsAnimation(std::nullopt);

Would you help me understand why it is different to setAllowsAnimation on image and imageElement? it Is somewhat counterintuitive to me since image and imageElement should be both associated with the same underlying entity.

[Tyler Wilcock]

> If I'm following the logic here, you remove the check
> m_imageAnimationEnabled == enabled to override the animation state of all
> images, not only the global state, which seems to be the cause of the bug.
> Would it be worth to spell out that in a comment here?
Yeah, a comment would be useful here. Will change.

> --- a/Source/WebCore/rendering/RenderView.cpp
> +++ b/Source/WebCore/rendering/RenderView.cpp
> @@ -967,6 +969,12 @@ void RenderView::updatePlayStateForAllAnimations(const
> IntRect& visibleRect)
>                      addRendererWithPausedImageAnimations(renderElement,
> *cachedImage);
>                  }
>              } else if (image && image->isAnimated()) {
> +                // Override any individual animation play state that may
> have been set.
> +                if (auto* imageElement =
> dynamicDowncast<HTMLImageElement>(renderElement.element()))
> +                    imageElement->setAllowsAnimation(std::nullopt);
> +                else
> +                    image->setAllowsAnimation(std::nullopt);
> 
> Would you help me understand why it is different to setAllowsAnimation on
> image and imageElement? it Is somewhat counterintuitive to me since image
> and imageElement should be both associated with the same underlying entity.
Your intuition is not wrong. HTMLImageElement::setAllowsAnimation calls into Image::setAllowsAnimation(), but importantly does other bookkeeping too. HTMLImageElement is the only element I expect to see with a directly associated Image* (this loop iterates over RenderElements), but I decided to add the else fallback to make the code more robust in case my assumption was wrong.

If / when we decide to make this feature support dynamic pause / play of CSS animations (which can be associated with any type of element), we'll need to modify this method again.

[Tyler Wilcock]

Created attachment 464884 [details]
Patch

[EWS]

Committed 259971@main (b722c6bcb96b): <https://commits.webkit.org/259971@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 464884 [details].

[Radar WebKit Bug Importer]

<rdar://problem/105138982>