On this site, https://css-tricks.com/having-fun-with-link-hover-effects/, the wavy line a:hover effect is supposed to animate but it doesn't. The rest of the examples provided all seem to animate correctly otherwise. This page mentions its inspiration as coming from https://theoutline.com/post/5970/unconventional-wisdom-emotional-readiness-is-bullshit. The hover effects here don't work, either. I thought that this issue might only apply to link states, but none of the decorative lines animate either. They do in Firefox. This happens in the WebKit Nightly as well as on iPadOS 13.4 beta 6. I tried adding the -webkit prefix but the results are the same.
(In reply to tym46383 from comment #0) > On this site, https://css-tricks.com/having-fun-with-link-hover-effects/, > the wavy line a:hover effect is supposed to animate but it doesn't. The rest > of the examples provided all seem to animate correctly otherwise. > > This page mentions its inspiration as coming from > https://theoutline.com/post/5970/unconventional-wisdom-emotional-readiness- > is-bullshit. The hover effects here don't work, either. I thought that this > issue might only apply to link states, but none of the decorative lines > animate either. They do in Firefox. > > This happens in the WebKit Nightly as well as on iPadOS 13.4 beta 6. I tried > adding the -webkit prefix but the results are the same. Actually, I just noticed that the horizontal "Border to Background Effect" (https://codepen.io/geoffgraham/pen/qMbVGv) fades from orange to bluish-gray in WebKit before fading to white. In Firefox it goes from orange to white. But this and the rest of the examples aren't SVG animations so it's not especially relevant to this bug I suppose.
<rdar://problem/60733068>
https://theoutline.com/post/5970/unconventional-wisdom-emotional-readiness-is-bullshit shows the issue without hovering links right at the top of the page where I can see the dividing wavy line animate in Firefox but not in STP 102. This is not new with the Web Animations implementation.
The animation is performed by a CSS animation in an SVG document, https://theoutline.com/svg/sq/v?c=ffffff&a=4&f=5&sw=1&anim=true. You can change the store color by modifying the URL to https://theoutline.com/svg/sq/v?c=000000&a=4&f=5&sw=1&anim=true, and this shows the animation working as expected. This is probably an issue where SVG images in background-image don't run animations.
Confirming this bug in Safari 13.1 (15609.1.20.111.8) and STP Release 103. SVG keyframe animations on CSS background-image attributes aren't rendering properly, but the animations will render if the SVG is opened/displayed directly in a browser window.
This is a regression due to Web Animations introduced in Safari 13.1. Looking for a fix now.
The DocumentTimeline schedules an initial update but its current time never advances.
In DisplayRefreshMonitorMac::requestRefreshCallback(): CVReturn error = CVDisplayLinkCreateWithCGDisplay(displayID(), &m_displayLink); if (error) return false; We get an error and bail early, never registering for an actual callback.
The legacy animation engine uses a different codepath presumably, since DisplayRefreshMonitorMac::requestRefreshCallback() is never called.
The displayID passed to CVDisplayLinkCreateWithCGDisplay is 0, presumably that's an invalid value. Maybe this is because this is the Page for an SVGDocument?
When I open the SVG file by itself, the code never goes through DisplayRefreshMonitorMac::requestRefreshCallback() in Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp
This also affects <img> elements pointing to SVG documents.
A simple reduction: <img src='data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg"><style>@keyframes animate { to { opacity: 0 } } rect { animation: animate 0.5s infinite alternate }</style><rect width="100%" height="100%"></rect></svg>'>
Created attachment 395379 [details] Test
The CSS animation DocumentTimeline is added to the SVGDocument of the embedded SVGImage. This SVGDocument is not enumerate-able by Page::forEachDocument(), so we never call updateAnimationsAndSendEvents() for it. I am not sure what the right solution is: 1. Should we add the DocumentTimeline to the topDocument? 2. Should we make Page::forEachDocument() enumerate through the SVGDocuments of the embedded SVGImages also?
(In reply to Said Abou-Hallawa from comment #15) > The CSS animation DocumentTimeline is added to the SVGDocument of the > embedded SVGImage. This SVGDocument is not enumerate-able by > Page::forEachDocument(), so we never call updateAnimationsAndSendEvents() > for it. > > I am not sure what the right solution is: > 1. Should we add the DocumentTimeline to the topDocument? > 2. Should we make Page::forEachDocument() enumerate through the SVGDocuments > of the embedded SVGImages also? I've been talking about this with Simon and he reckons what we need to do is to make Chrome::displayID() return the hosting page's displayID such that the call to CVDisplayLinkCreateWithCGDisplay doesn't fail.
I think SVGImageChromeClient should implement something that allows the ImageObserver to implement schduleRenderingUpdate.
(In reply to Antoine Quint from comment #16) > (In reply to Said Abou-Hallawa from comment #15) > > The CSS animation DocumentTimeline is added to the SVGDocument of the > > embedded SVGImage. This SVGDocument is not enumerate-able by > > Page::forEachDocument(), so we never call updateAnimationsAndSendEvents() > > for it. > > > > I am not sure what the right solution is: > > 1. Should we add the DocumentTimeline to the topDocument? > > 2. Should we make Page::forEachDocument() enumerate through the SVGDocuments > > of the embedded SVGImages also? > > I've been talking about this with Simon and he reckons what we need to do is > to make Chrome::displayID() return the hosting page's displayID such that > the call to CVDisplayLinkCreateWithCGDisplay doesn't fail. DisplayRefreshMonitorMac is only used by WK1 and the bug is happening in WK2 as well.
Created attachment 395420 [details] Patch
Comment on attachment 395420 [details] Patch Something is still missing in this patch. WebAnimation should be enabled for top-level SVGDocument but not for the embedded ones.
Comment on attachment 395420 [details] Patch This doesn't seem like the right approach (and we also want to eliminate the old animation code eventually).
Created attachment 395432 [details] Patch
(In reply to Said Abou-Hallawa from comment #22) > Created attachment 395432 [details] > Patch This patch is not the right approach. But it is fixes bug and it does not break the top-level case. Besides it has layout tests. Just ignore it for now.
Created attachment 395659 [details] Patch
Created attachment 395662 [details] Patch
Comment on attachment 395662 [details] Patch Two problems with this patch: 1. It fixes the case where the SVG image is embedded in an <img> element. But it does not fix the case where the SVG image is the background of an element. 2. It has to enumerate the RendererImages of every document in each Page::updateRendering() regardless they have SVG images or not.
Created attachment 395723 [details] Patch
Comment on attachment 395723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395723&action=review > Source/WebCore/page/Page.cpp:1312 > + if (chrome().client().isSVGImageChromeClient()) > + chrome().client().scheduleRenderingUpdate(); This is weird, because ChromeClient is a virtual interface. Maybe Chrome::scheduleRenderingUpdate() could just return a bool - true if it handled it. > Source/WebCore/page/Page.cpp:1374 > + for (auto& renderer : descendantsOfType<RenderElement>(*renderView)) > + renderer.updateRendering(); This is a massive tree walk! I think we need to avoid this by storing a list of SVGImages somewhere.
Created attachment 395750 [details] Patch
Comment on attachment 395750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395750&action=review > Source/WebCore/dom/Document.cpp:6401 > + for (auto& cachedResourceHandle : m_cachedResourceLoader->allCachedResources().values()) { > + auto* cachedResource = cachedResourceHandle.get(); > + if (!cachedResource || !cachedResource->isImage()) > + continue; > + > + auto* image = downcast<CachedImage>(cachedResource)->image(); > + if (!image || !is<SVGImage>(image)) > + continue; > + > + if (auto* page = downcast<SVGImage>(image)->page()) > + page->updateRendering(); > + } Please measure how long this takes on some resource-heavy pages (throw in a TimingScope in your local release build). It might be too expensive to do every frame. > Source/WebCore/page/Page.cpp:1311 > + if (chrome().client().isSVGImageChromeClient()) > + chrome().client().scheduleRenderingUpdate(); Can we do this through virtual functions, as I suggested before?
Created attachment 395859 [details] Patch
Comment on attachment 395750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395750&action=review >> Source/WebCore/dom/Document.cpp:6401 >> + } > > Please measure how long this takes on some resource-heavy pages (throw in a TimingScope in your local release build). It might be too expensive to do every frame. I changed that by building a HashSet<String> which contains the URLs of the cached SVGImages when the documentDidFinishLoadEvent fires. This HashSet<> will be used to get the list of the SVGImages in a document instead. So building HashSet<String> happens only once and enumerating this HashSet is cheap. If the HashSet is empty, which is the common case, there should not be any noticeable overhead in the Page::updateRendering(). >> Source/WebCore/page/Page.cpp:1311 >> + chrome().client().scheduleRenderingUpdate(); > > Can we do this through virtual functions, as I suggested before? Done.
Created attachment 395860 [details] Patch
Comment on attachment 395860 [details] Patch This patch does not fix the animation in https://css-tricks.com/having-fun-with-link-hover-effects/.
Created attachment 395907 [details] Patch
Comment on attachment 395907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395907&action=review > Source/WebCore/loader/cache/CachedResourceLoader.h:121 > + Vector<SVGImage*> allCachedSVGImages() const; I think this should return Vector<Ref<SVGImage>> > Source/WebCore/page/ChromeClient.h:319 > + virtual bool scheduleTimedRenderingUpdate() { return false; } Why do we need to expose the "timed" version here? > Source/WebCore/page/Page.cpp:1370 > + for (auto* image : document.cachedResourceLoader().allCachedSVGImages()) { This becomes for (auto image : ... > Source/WebCore/svg/graphics/SVGImage.h:72 > + Page* page() { return m_page.get(); } Because this is so special, I would call it internalPage() or something. It's not the Page that the image lives in.
Created attachment 395986 [details] Patch
Comment on attachment 395907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395907&action=review >> Source/WebCore/loader/cache/CachedResourceLoader.h:121 >> + Vector<SVGImage*> allCachedSVGImages() const; > > I think this should return Vector<Ref<SVGImage>> Done. >> Source/WebCore/page/ChromeClient.h:319 >> + virtual bool scheduleTimedRenderingUpdate() { return false; } > > Why do we need to expose the "timed" version here? Because the immediate scheduleRenderingUpdate() is used by RenderLayerCompositor. All the callers including the WebAnimation use scheduleTimedRenderingUpdate(). The immediate scheduling was implemented to fix many layout test flakiness where waiting for the next frame was producing incorrect results. >> Source/WebCore/page/Page.cpp:1370 >> + for (auto* image : document.cachedResourceLoader().allCachedSVGImages()) { > > This becomes for (auto image : ... But I had to make it for (auto& image : ... because Ref<SVGImage> can't be copied. >> Source/WebCore/svg/graphics/SVGImage.h:72 >> + Page* page() { return m_page.get(); } > > Because this is so special, I would call it internalPage() or something. It's not the Page that the image lives in. Done: page() => internalPage().
Created attachment 395988 [details] Patch
Committed r259830: <https://trac.webkit.org/changeset/259830> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395988 [details].
*** Bug 211922 has been marked as a duplicate of this bug. ***