SVG animations should respect Page::imageAnimationEnabled
rdar://100143723
Created attachment 462456 [details] Patch
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.
Created attachment 462479 [details] Patch
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.
<rdar://problem/100430906>
Created attachment 462749 [details] Patch
Created attachment 462755 [details] Patch
Created attachment 462757 [details] Patch
Created attachment 462760 [details] Patch
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()));
Created attachment 462801 [details] Patch
(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()));
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().
Created attachment 462858 [details] Patch
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.
Created attachment 462977 [details] Patch
(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.
Tyler, this seems to be looking pretty good. @Said, any other comments? I'm inclined to approve
Comment on attachment 462977 [details] Patch r+ @said jump in if you think we need more changes
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].