Bug 204204 – Changes to clip-path and filter SVG elements referenced by CSS don't trigger repaints

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Attachment shows a bug where a dynamic change to a polygon used in -webkit-clip-path doesn't trigger a repaint.

[Simon Fraser (smfr)]

Created attachment 383576 [details]
Test

[Simon Fraser (smfr)]

Dirk, how is this supposed to work?

[bdc]

Thanks Simon!

Just to clarify: this bug happens no matter if it's a polygon, a path, etc. used as a clip-path but, I'm sure you figured that out :)

[Nikolas Zimmermann]

Simon, there is no mechanism that could handle the update.
Support for -webkit-clip-path referencing SVG elements is really a snapshot.
The referenced object knows nothing about a CSS Clip-path that makes use of it.
For SVG clipPath elements these updates would be handled via the SVGResources mechanism...

We should investigate to re-use this...

[Simon Fraser (smfr)]

Created attachment 436842 [details]
For EWS

[Simon Fraser (smfr)]

Created attachment 436905 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 436960 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 436960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436960&action=review

> Source/WebCore/rendering/RenderElement.cpp:2300
> +// This needs to run when the entire render tree has been constructed, so can't be called from styleDidChange.

This comment is wrong (the code just looks for elements now).

[Antti Koivisto]

Comment on attachment 436960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436960&action=review

r=me with stable test

> Source/WebCore/rendering/ReferencedSVGResources.cpp:39
> +class CSSSVGResourceElementClient final : public SVGResourceElementClient {

Why subclassing? Do we expect other SVGResourceElementClients?

> Source/WebCore/rendering/ReferencedSVGResources.cpp:50
> +    RenderElement& m_clientRenderer;

Ryosuke would tell you to use WeakPtr

> Source/WebCore/rendering/ReferencedSVGResources.cpp:92
> +HashMap<AtomString, QualifiedName> ReferencedSVGResources::referencedSVGResourceIDs(const RenderStyle& style)
> +{
> +    HashMap<AtomString, QualifiedName> referencedResourceIDToTagNameMap;
> +    if (is<ReferenceClipPathOperation>(style.clipPath())) {
> +        auto& clipPath = downcast<ReferenceClipPathOperation>(*style.clipPath());
> +        if (!clipPath.fragment().isEmpty())
> +            referencedResourceIDToTagNameMap.add(clipPath.fragment(), SVGNames::clipPathTag);
> +    }

Wouldn't HashSet<std::pair<AtomString, QualifiedName>> be a more natural type for this? Or just Vector<std::pair<>> as they are going to be stuffed into a HashMap later anyway.

I think with HashMap referencing a wrong type with a fragment breaks any further correct use of the the same fragment.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:123
> +        m_elementClients.ensure(targetID, [&] {
> +            auto client = WTF::makeUnique<CSSSVGResourceElementClient>(m_renderer);
> +            element->addReferencingCSSClient(*client);
> +            return client;
> +        });

For symmetry it might be nice to have helper for add case too (to pair with removeClientForTarget)

> Source/WebCore/rendering/ReferencedSVGResources.cpp:133
> +// SVG code uses getRenderSVGResourceById<>, but that works in terms of renderers. We need to find resources
> +// before the render tree is fully constructed, so this works on Elements.

It seems unfortunate to have two mechanisms for this. Like how do we even know they produce the same results? Can we have just one?

> Source/WebCore/rendering/ReferencedSVGResources.cpp:147
> +    if (referenceFilter.fragment().isEmpty())
> +        return nullptr;
> +    AtomString targetID = referenceFilter.fragment();

could either move the local to the beginning and test for it, or just get rid of it entirely.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:156
> +    if (clipPath.fragment().isEmpty())
> +        return nullptr;
> +    AtomString targetID = clipPath.fragment();

Same here.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:158
> +    // For some reason, SVG stores a cache of id -> renderer, rather than just using getElementById() and renderer().
> +    return getRenderSVGResourceById<RenderSVGResourceClipper>(document, targetID);

y tho

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #9)
> Comment on attachment 436960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436960&action=review
> 
> r=me with stable test
> 
> > Source/WebCore/rendering/ReferencedSVGResources.cpp:39
> > +class CSSSVGResourceElementClient final : public SVGResourceElementClient {
> 
> Why subclassing? Do we expect other SVGResourceElementClients?

I wanted to avoid adding more Renderer references to Element code (for LFC).

> > Source/WebCore/rendering/ReferencedSVGResources.cpp:50
> > +    RenderElement& m_clientRenderer;
> 
> Ryosuke would tell you to use WeakPtr

I could, though the lifetime of this object is strongly tied to the Renderer.


> > Source/WebCore/rendering/ReferencedSVGResources.cpp:92
> > +HashMap<AtomString, QualifiedName> ReferencedSVGResources::referencedSVGResourceIDs(const RenderStyle& style)
> > +{
> > +    HashMap<AtomString, QualifiedName> referencedResourceIDToTagNameMap;
> > +    if (is<ReferenceClipPathOperation>(style.clipPath())) {
> > +        auto& clipPath = downcast<ReferenceClipPathOperation>(*style.clipPath());
> > +        if (!clipPath.fragment().isEmpty())
> > +            referencedResourceIDToTagNameMap.add(clipPath.fragment(), SVGNames::clipPathTag);
> > +    }
> 
> Wouldn't HashSet<std::pair<AtomString, QualifiedName>> be a more natural
> type for this? Or just Vector<std::pair<>> as they are going to be stuffed
> into a HashMap later anyway.
> 
> I think with HashMap referencing a wrong type with a fragment breaks any
> further correct use of the the same fragment.

Good point, I'll make a test for that and fix this.

> > Source/WebCore/rendering/ReferencedSVGResources.cpp:133
> > +// SVG code uses getRenderSVGResourceById<>, but that works in terms of renderers. We need to find resources
> > +// before the render tree is fully constructed, so this works on Elements.
> 
> It seems unfortunate to have two mechanisms for this. Like how do we even
> know they produce the same results? Can we have just one?

I wish I knew. This is historical SVG resource cruft.

> > Source/WebCore/rendering/ReferencedSVGResources.cpp:147
> > +    if (referenceFilter.fragment().isEmpty())
> > +        return nullptr;
> > +    AtomString targetID = referenceFilter.fragment();
> 
> could either move the local to the beginning and test for it, or just get
> rid of it entirely.

Right.

> > Source/WebCore/rendering/ReferencedSVGResources.cpp:158
> > +    // For some reason, SVG stores a cache of id -> renderer, rather than just using getElementById() and renderer().
> > +    return getRenderSVGResourceById<RenderSVGResourceClipper>(document, targetID);
> 
> y tho

Ditto.

[Simon Fraser (smfr)]

Created attachment 437177 [details]
Patch

[EWS]

Committed r281967 (241274@main): <https://commits.webkit.org/241274@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/82699249>