Bug 251738

Created attachment 464834 [details] Patch

Created attachment 464835 [details] Patch

Created attachment 464836 [details] Patch

Created attachment 464838 [details] Patch

Created attachment 464839 [details] Patch

(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.

> 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.

Created attachment 464884 [details] Patch

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].

<rdar://problem/105138982>