Bug 204204 - Changes to clip-path and filter SVG elements referenced by CSS don't trigger repaints
Summary: Changes to clip-path and filter SVG elements referenced by CSS don't trigger ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks: 126207
  Show dependency treegraph
 
Reported: 2019-11-14 15:15 PST by Simon Fraser (smfr)
Modified: 2021-09-02 16:55 PDT (History)
20 users (show)

See Also:


Attachments
Test (565 bytes, text/html)
2019-11-14 15:15 PST, Simon Fraser (smfr)
no flags Details
For EWS (34.93 KB, patch)
2021-08-30 20:57 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (33.97 KB, patch)
2021-08-31 10:30 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (43.31 KB, patch)
2021-08-31 16:11 PDT, Simon Fraser (smfr)
koivisto: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (44.23 KB, patch)
2021-09-02 11:59 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-11-14 15:15:38 PST
Attachment shows a bug where a dynamic change to a polygon used in -webkit-clip-path doesn't trigger a repaint.
Comment 1 Simon Fraser (smfr) 2019-11-14 15:15:55 PST
Created attachment 383576 [details]
Test
Comment 2 Simon Fraser (smfr) 2019-11-14 15:24:03 PST
Dirk, how is this supposed to work?
Comment 3 bdc 2019-11-14 15:53:08 PST
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 :)
Comment 4 Nikolas Zimmermann 2019-11-14 23:30:00 PST
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...
Comment 5 Simon Fraser (smfr) 2021-08-30 20:57:41 PDT
Created attachment 436842 [details]
For EWS
Comment 6 Simon Fraser (smfr) 2021-08-31 10:30:19 PDT
Created attachment 436905 [details]
Patch
Comment 7 Simon Fraser (smfr) 2021-08-31 16:11:07 PDT
Created attachment 436960 [details]
Patch
Comment 8 Simon Fraser (smfr) 2021-08-31 16:21:11 PDT
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).
Comment 9 Antti Koivisto 2021-09-01 02:03:07 PDT
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
Comment 10 Simon Fraser (smfr) 2021-09-02 11:21:00 PDT
(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.
Comment 11 Simon Fraser (smfr) 2021-09-02 11:59:36 PDT
Created attachment 437177 [details]
Patch
Comment 12 EWS 2021-09-02 16:54:37 PDT
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].
Comment 13 Radar WebKit Bug Importer 2021-09-02 16:55:22 PDT
<rdar://problem/82699249>