Bug 209370 – REGRESSION: CSS animations inside an embedded SVG image do not animate

• Component: Animations
• Status: RESOLVED FIXED

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.

[Radar WebKit Bug Importer]

<rdar://problem/60733068>

[Antoine Quint]

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.

[Antoine Quint]

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.

[TG]

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.

[Antoine Quint]

This is a regression due to Web Animations introduced in Safari 13.1. Looking for a fix now.

[Antoine Quint]

The DocumentTimeline schedules an initial update but its current time never advances.

[Antoine Quint]

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.

[Antoine Quint]

The legacy animation engine uses a different codepath presumably, since DisplayRefreshMonitorMac::requestRefreshCallback() is never called.

[Antoine Quint]

The displayID passed to CVDisplayLinkCreateWithCGDisplay is 0, presumably that's an invalid value. Maybe this is because this is the Page for an SVGDocument?

[Antoine Quint]

When I open the SVG file by itself, the code never goes through DisplayRefreshMonitorMac::requestRefreshCallback() in Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp

[Antoine Quint]

This also affects <img> elements pointing to SVG documents.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 395379 [details]
Test

[Said Abou-Hallawa]

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?

[Antoine Quint]

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

[Simon Fraser (smfr)]

I think SVGImageChromeClient should implement something that allows the ImageObserver to implement schduleRenderingUpdate.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 395420 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

Created attachment 395432 [details]
Patch

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 395659 [details]
Patch

[Said Abou-Hallawa]

Created attachment 395662 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 395723 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 395750 [details]
Patch

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 395859 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 395860 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 395860 [details]
Patch

This patch does not fix the animation in https://css-tricks.com/having-fun-with-link-hover-effects/.

[Said Abou-Hallawa]

Created attachment 395907 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 395986 [details]
Patch

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 395988 [details]
Patch

[EWS]

Committed r259830: <https://trac.webkit.org/changeset/259830>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395988 [details].

[Said Abou-Hallawa]

*** Bug 211922 has been marked as a duplicate of this bug. ***