Bug 245399

Summary:

SVG animations should respect Page::imageAnimationEnabled

Product:

WebKit

Reporter:

Tyler Wilcock <tyler_w>

Component:

SVG

Assignee:

Tyler Wilcock <tyler_w>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cfleizach, changseok, cmarcelo, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, sabouhallawa, schenney, sergio, simon.fraser, tyler_w, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

244128

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Tyler Wilcock

Reported 2022-09-19 15:26:34 PDT

SVG animations should respect Page::imageAnimationEnabled

Attachments
Patch (7.70 KB, patch) 2022-09-19 15:36 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (7.65 KB, patch) 2022-09-20 16:28 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (22.61 KB, patch) 2022-10-01 13:16 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (22.91 KB, patch) 2022-10-01 16:29 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (22.71 KB, patch) 2022-10-01 18:09 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (22.93 KB, patch) 2022-10-01 23:22 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (18.97 KB, patch) 2022-10-04 17:58 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (17.58 KB, patch) 2022-10-06 20:31 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Patch (35.65 KB, patch) 2022-10-13 23:01 PDT, Tyler Wilcock	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Tyler Wilcock

Comment 1 2022-09-19 15:32:40 PDT

rdar://100143723

Tyler Wilcock

Comment 2 2022-09-19 15:36:25 PDT

Created attachment 462456 [details] Patch

Darin Adler

Comment 3 2022-09-20 15:53:18 PDT

Comment on attachment 462456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462456&action=review > Source/WebCore/svg/SVGAnimateElementBase.cpp:159 > + if (auto* page = document().page()) { > + if (!page->imageAnimationEnabled()) > + return; > + } This can be written less nested: if (auto* page = document().page(); page && !page->imageAnimationEnabled()) return; > Source/WebCore/svg/animation/SVGSMILElement.cpp:1091 > + if (auto* page = document().page()) { > + if (!page->imageAnimationEnabled()) { Ditto.

Tyler Wilcock

Comment 4 2022-09-20 16:28:32 PDT

Created attachment 462479 [details] Patch

Said Abou-Hallawa

Comment 5 2022-09-20 16:35:44 PDT

Comment on attachment 462456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462456&action=review > Source/WebCore/svg/animation/SVGSMILElement.cpp:1096 > + } I do not think this is the right level of pausing the SVG animation. I think this should be done by calling SVGSVGElement::pauseAnimations() at a higher level. > LayoutTests/fast/images/disable-animations-svg.html:8 > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" style="background-color: rgb(232, 232, 237);" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="200" viewBox="-4 -4 8 8"> Two extra cases we need to test: the embedded SVG image and the background image: <style> .box { width: 300px; height: 200px; background: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><rect id="green" width="100" height="100" fill="green"><animate attributeName="x" from="0" to="200" dur="3s" repeatCount="indefinite"/></rect></svg>'); } </style> <body> <div class="box"></div> <img src='data:image/svg+xml;utf8, <svg xmlns="http://www.w3.org/2000/svg"> <rect id="green" width="100" height="100" fill="green"> <animate attributeName="x" from="0" to="200" dur="3s" repeatCount="indefinite"/> </rect> </svg>'> </body> > LayoutTests/fast/images/disable-animations-svg.html:32 > + shouldBeFalse("internals.isSVGAnimating(svgElement)"); I do not think this is enough testing. We should be testing the animated property ("transform" in this case) did not changed after sometime (e.g. 50ms). Search for "svg.setCurrentTime()" and "svg.pauseAnimations()" under LayoutTests/svg for examples on how to do that.

Radar WebKit Bug Importer

Comment 6 2022-09-26 15:27:18 PDT

<rdar://problem/100430906>

Tyler Wilcock

Comment 7 2022-10-01 13:16:34 PDT

Created attachment 462749 [details] Patch

Tyler Wilcock

Comment 8 2022-10-01 16:29:07 PDT

Created attachment 462755 [details] Patch

Tyler Wilcock

Comment 9 2022-10-01 18:09:21 PDT

Created attachment 462757 [details] Patch

Tyler Wilcock

Comment 10 2022-10-01 23:22:36 PDT

Created attachment 462760 [details] Patch

Said Abou-Hallawa

Comment 11 2022-10-03 14:27:40 PDT

Comment on attachment 462760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462760&action=review > Source/WebCore/rendering/RenderView.cpp:879 > +void RenderView::repaintImageAnimationsIfNeeded(const IntRect& visibleRect, bool animationEnabled) I think the patch will look smaller if we access page().imageAnimationEnabled() instead of passing 'animationEnabled' from Page::setImageAnimationEnabled() all the way through here. bool animationEnabled = page().imageAnimationEnabled(); > Source/WebCore/rendering/RenderView.cpp:899 > + if (auto* cachedImage = layer->image() ? layer->image()->cachedImage() : nullptr) { > + if (auto* svgImage = dynamicDowncast<SVGImage>(cachedImage->image())) > + updateAnimations(*svgImage); > + else if (auto* image = cachedImage->image(); image && image->isAnimated()) > + needsRepaint = true; > + } This code is repeated for the RenderImage. Can we make a lambda which takes care of: 1. Checking cachedImage != nullptr 2. Checking is<SVGImage>(cachedImage->image()) and updates the animation according to animationEnabled. 3. Checking cachedImage->image() && cachedImage->image()->isAnimated() and sets needsRepaint = true. > Source/WebCore/svg/SVGDocumentExtensions.cpp:118 > + // If animations are paused at the document level, don't allow `this` to be unpaused. > + if (animationsPausedForDocument(m_document)) > + return; I do not understand why we need this check here. Why do not we allow overriding the global page setting for pausing/unpausing the animation on the document level? We already allow overriding it for a single image. > Source/WebCore/svg/graphics/SVGImage.h:61 > + void resumeAnimation(); I think the names are getting out of control for animation: startAnimation() // Maybe the same resumeAnimation() unpauseAnimation() // Maybe the same stopAnimation() pauseAnimation() resetAnimation() But I think this can be addressed later. > Source/WebCore/testing/Internals.cpp:786 > +Vector<Ref<SVGSVGElement>> Internals::allSVGSVGElements() const I wonder if an equivalent JS code for this function can be written. Maybe we cannot get the CSS background SVGImages? > Source/WebCore/testing/Internals.cpp:796 > + document->accessSVGExtensions().timeContainersForTesting().forEach([&elements] (WeakPtr<SVGSVGElement, WeakPtrImplWithEventTargetData> element) { > + if (element) > + elements.append(*element); > + }); Maybe this code can can be implemented as a function in SVGDocumentExtensions: elements.appendVector(WTFMove(document->accessSVGExtensions().allSVGSVGElements()));

Tyler Wilcock

Comment 12 2022-10-04 17:58:42 PDT

Created attachment 462801 [details] Patch

Tyler Wilcock

Comment 13 2022-10-04 18:23:19 PDT

(In reply to Said Abou-Hallawa from comment #11) > Comment on attachment 462760 [details] > > Source/WebCore/svg/SVGDocumentExtensions.cpp:118 > > + // If animations are paused at the document level, don't allow `this` to be unpaused. > > + if (animationsPausedForDocument(m_document)) > > + return; > > I do not understand why we need this check here. Why do not we allow > overriding the global page setting for pausing/unpausing the animation on > the document level? We already allow overriding it for a single image. Josh had trouble creating a good UI for individually pausing / resuming SVG animations (e.g. via context menu or user agent shadow DOM button). Until I can experiment with this myself, I think we should try to move forward without the ability to individually control SVG animations. The global play / pause control over SVG animations will still cover the main use case we eventually hope to achieve (animations can be made to pause by default).   The reason this check is in this specific place is because in this scenario:  1. Open a page with animations 2. Pause animations (e.g. via some future UI — I've hacked something together locally to test for now) 3. Minimize Safari 4. Re-open Safari We call SVGDocumentExtensions::unpauseAnimations(). Without this new check, we would unpause animations despite Page::m_imageAnimationEnabled being false. > > Source/WebCore/testing/Internals.cpp:786 > > +Vector<Ref<SVGSVGElement>> Internals::allSVGSVGElements() const > > I wonder if an equivalent JS code for this function can be written. Maybe we > cannot get the CSS background SVGImages? I did experiment with this for a while but couldn't find a way to use JS to get at the SVG elements backing CSS background images, or those embedded in img elements. > > Source/WebCore/testing/Internals.cpp:796 > > + document->accessSVGExtensions().timeContainersForTesting().forEach([&elements] (WeakPtr<SVGSVGElement, WeakPtrImplWithEventTargetData> element) { > > + if (element) > > + elements.append(*element); > > + }); > > Maybe this code can can be implemented as a function in > SVGDocumentExtensions: > > elements.appendVector(WTFMove(document->accessSVGExtensions(). > allSVGSVGElements())); Done, minus the WTFMove — seems like it's unnecessary: Source/WebCore/testing/Internals.cpp:793:31: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] elements.appendVector(WTFMove(document->accessSVGExtensions().allSVGSVGElements()));

Said Abou-Hallawa

Comment 14 2022-10-06 12:20:09 PDT

Comment on attachment 462801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462801&action=review > Source/WTF/wtf/WeakHashSet.h:192 > +template<typename T, typename WeakMapImpl> > +inline Vector<Ref<T>> copyToRefVector(const WeakHashSet<T, WeakMapImpl>& weakHashSet) > +{ > + Vector<Ref<T>> result; > + result.reserveInitialCapacity(weakHashSet.computeSize()); > + weakHashSet.forEach([&] (WeakPtr<T, WeakMapImpl> item) { > + // Unconditionally dereferencing `item` is safe here because `forEach` will only execute our callback for non-null WeakPtrs. > + result.uncheckedAppend(*item); > + }); > + return result; > +} I do not think this is needed. The caller can just say: copyToVectorOf<Ref<SVGSVGElement>>(m_timeContainers); See SVGDocumentExtensions::dispatchLoadEventToOutermostSVGElements().

Tyler Wilcock

Comment 15 2022-10-06 20:31:12 PDT

Created attachment 462858 [details] Patch

Darin Adler

Comment 16 2022-10-06 20:49:41 PDT

Comment on attachment 462858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462858&action=review > Source/WebCore/rendering/RenderView.cpp:881 > void RenderView::repaintImageAnimationsIfNeeded(const IntRect& visibleRect) I think this function’s name should be updated, since it now does other things. Unless you think of calling stop/resumeAnimation on an SVGImage as "repainting". > Source/WebCore/rendering/RenderView.cpp:898 > + if (auto* svgImage = dynamicDowncast<SVGImage>(image)) { > + if (animationEnabled) > + svgImage->resumeAnimation(); > + else > + svgImage->stopAnimation(); I understand for "repainting" why we would only do work for visible elements. But what about for an SVGImage? Is there code that stops and starts these as the elements with them are scrolled into and out of the visible rectangle? Or some other reason it is OK to neither stop nor start ones that are outside the visible rectangle? > Source/WebCore/svg/SVGAnimateElementBase.cpp:28 > +#include "Document.h" > +#include "Page.h" What change in this patch motivates adding these includes here? > Source/WebCore/svg/SVGDocumentExtensions.cpp:72 > } > > + > +Vector<Ref<SVGSVGElement>> SVGDocumentExtensions::allSVGSVGElements() const Extra blank line here.

Tyler Wilcock

Comment 17 2022-10-13 23:01:45 PDT

Created attachment 462977 [details] Patch

Tyler Wilcock

Comment 18 2022-10-14 16:22:55 PDT

(In reply to Darin Adler from comment #16) > Comment on attachment 462858 [details] > Patch > > Source/WebCore/rendering/RenderView.cpp:898 > > + if (auto* svgImage = dynamicDowncast<SVGImage>(image)) { > > + if (animationEnabled) > > + svgImage->resumeAnimation(); > > + else > > + svgImage->stopAnimation(); > > I understand for "repainting" why we would only do work for visible > elements. But what about for an SVGImage? Is there code that stops and > starts these as the elements with them are scrolled into and out of the > visible rectangle? Or some other reason it is OK to neither stop nor start > ones that are outside the visible rectangle?  This was a bug — thanks! Fixed in the latest patch with a layout test verifying it.

chris fleizach

Comment 19 2022-10-21 15:52:25 PDT

Tyler, this seems to be looking pretty good. @Said, any other comments? I'm inclined to approve

chris fleizach

Comment 20 2022-10-25 12:03:21 PDT

Comment on attachment 462977 [details] Patch r+ @said jump in if you think we need more changes

EWS

Comment 21 2022-10-28 17:18:20 PDT

Committed 256122@main (a2cbeb35dc51): <https://commits.webkit.org/256122@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 462977 [details].

Note You need to log in before you can comment on or make changes to this bug.