Bug 251738 - Individually paused / playing animations are not effected by Play All Animations and Pause All Animations
Summary: Individually paused / playing animations are not effected by Play All Animati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-02-04 11:20 PST by Tyler Wilcock
Modified: 2023-02-07 12:00 PST (History)
12 users (show)

See Also:


Attachments
Patch (9.76 KB, patch)
2023-02-04 11:32 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2023-02-04 11:39 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2023-02-04 12:10 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.35 KB, patch)
2023-02-04 12:46 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2023-02-04 12:56 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (15.66 KB, patch)
2023-02-07 00:24 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2023-02-04 11:20:00 PST
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.
Comment 1 Tyler Wilcock 2023-02-04 11:32:20 PST
Created attachment 464834 [details]
Patch
Comment 2 Tyler Wilcock 2023-02-04 11:39:32 PST
Created attachment 464835 [details]
Patch
Comment 3 Tyler Wilcock 2023-02-04 12:10:16 PST
Created attachment 464836 [details]
Patch
Comment 4 Tyler Wilcock 2023-02-04 12:46:20 PST
Created attachment 464838 [details]
Patch
Comment 5 Tyler Wilcock 2023-02-04 12:56:10 PST
Created attachment 464839 [details]
Patch
Comment 6 Andres Gonzalez 2023-02-06 11:20:03 PST
(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.
Comment 7 Tyler Wilcock 2023-02-06 14:02:16 PST
> 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.
Comment 8 Tyler Wilcock 2023-02-07 00:24:29 PST
Created attachment 464884 [details]
Patch
Comment 9 EWS 2023-02-07 11:59:52 PST
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].
Comment 10 Radar WebKit Bug Importer 2023-02-07 12:00:20 PST
<rdar://problem/105138982>