Skip to content

Commit b61039e

Browse files
nullhooknullhook
committed
Fix clip-path reference breaking when removing SVG element with duplicate ID
https://bugs.webkit.org/show_bug.cgi?id=305001 rdar://147015037 Reviewed by Simon Fraser. When multiple SVG resource elements share the same ID, removing any one breaks references for all elements using that ID, even though other definitions remain in the DOM. To fix this, we now verify the resource being removed matches the one registered before removing. Fixes the appearance of user avatar clipping in Slack i.e the green activity ring. Slack uses multiple SVG clipPath definitions with the same ID, and when elements are added/removed during virtual scrolling, clip-path references would break. Test: svg/clip-path-duplicate-id-removal.html * LayoutTests/svg/clip-path-duplicate-id-removal-expected.html: Added. * LayoutTests/svg/clip-path-duplicate-id-removal.html: Added. * Source/WebCore/dom/TreeScope.cpp: (WebCore::TreeScope::removeSVGResource): * Source/WebCore/dom/TreeScope.h: * Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceContainer.cpp: (WebCore::LegacyRenderSVGResourceContainer::willBeDestroyed): (WebCore::LegacyRenderSVGResourceContainer::idChanged): Canonical link: https://commits.webkit.org/305197@main
1 parent 5ae3c80 commit b61039e

File tree

5 files changed

+76
-5
lines changed

5 files changed

+76
-5
lines changed

‎LayoutTests/svg/clip-path-duplicate-id-removal-expected.html‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<style>
5+
body {
6+
background: white;
7+
}
8+
.target {
9+
width: 50px;
10+
height: 50px;
11+
background: green;
12+
}
13+
</style>
14+
</head>
15+
<body>
16+
<div class="target"></div>
17+
</body>
18+
</html>

‎LayoutTests/svg/clip-path-duplicate-id-removal.html‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>clip-path with duplicate IDs should work after removing non-registered element</title>
5+
<link rel="match" href="clip-path-duplicate-id-removal-expected.html">
6+
<style>
7+
.target {
8+
clip-path: url(#clipper);
9+
width: 100px;
10+
height: 100px;
11+
background: green;
12+
}
13+
</style>
14+
</head>
15+
<body>
16+
<div class="target"></div>
17+
18+
<svg height="0" width="0">
19+
<clipPath id="clipper" clipPathUnits="objectBoundingBox">
20+
<rect width="0.5" height="0.5"/>
21+
</clipPath>
22+
</svg>
23+
<svg height="0" width="0">
24+
<clipPath id="clipper" clipPathUnits="objectBoundingBox">
25+
<rect width="0.5" height="0.5"/>
26+
</clipPath>
27+
</svg>
28+
<svg height="0" width="0">
29+
<clipPath id="clipper" clipPathUnits="objectBoundingBox">
30+
<rect width="0.5" height="0.5"/>
31+
</clipPath>
32+
</svg>
33+
34+
<script>
35+
if (window.testRunner)
36+
testRunner.waitUntilDone();
37+
38+
document.querySelector('svg').remove();
39+
40+
// Force repaint. A direct script removal doesn't force the repaint
41+
// needed to re-resolve the clip-path reference and expose the bug.
42+
document.body.style.background = 'white';
43+
44+
requestAnimationFrame(() => {
45+
if (window.testRunner)
46+
testRunner.notifyDone();
47+
});
48+
</script>
49+
</body>
50+
</html>

‎Source/WebCore/dom/TreeScope.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -656,12 +656,15 @@ void TreeScope::addSVGResource(const AtomString& id, LegacyRenderSVGResourceCont
656656
svgResourcesMap().legacyResources.set(id, &resource);
657657
}
658658

659-
void TreeScope::removeSVGResource(const AtomString& id)
659+
void TreeScope::removeSVGResource(const AtomString& id, LegacyRenderSVGResourceContainer& resource)
660660
{
661661
if (id.isEmpty())
662662
return;
663663

664-
svgResourcesMap().legacyResources.remove(id);
664+
auto& resources = svgResourcesMap().legacyResources;
665+
auto it = resources.find(id);
666+
if (it != resources.end() && it->value == &resource)
667+
resources.remove(it);
665668
}
666669

667670
LegacyRenderSVGResourceContainer* TreeScope::lookupLegacySVGResoureById(const AtomString& id) const

‎Source/WebCore/dom/TreeScope.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ class TreeScope : public NoVirtualDestructorBase {
146146
ExceptionOr<void> setAdoptedStyleSheets(Vector<Ref<CSSStyleSheet>>&&);
147147

148148
void addSVGResource(const AtomString& id, LegacyRenderSVGResourceContainer&);
149-
void removeSVGResource(const AtomString& id);
149+
void removeSVGResource(const AtomString& id, LegacyRenderSVGResourceContainer&);
150150
LegacyRenderSVGResourceContainer* lookupLegacySVGResoureById(const AtomString& id) const;
151151

152152
void addPendingSVGResource(const AtomString& id, SVGElement&);

‎Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceContainer.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ void LegacyRenderSVGResourceContainer::willBeDestroyed()
6262
SVGResourcesCache::resourceDestroyed(*this);
6363

6464
if (m_registered) {
65-
treeScopeForSVGReferences().removeSVGResource(m_id);
65+
treeScopeForSVGReferences().removeSVGResource(m_id, *this);
6666
m_registered = false;
6767
}
6868

@@ -85,7 +85,7 @@ void LegacyRenderSVGResourceContainer::idChanged()
8585
removeAllClientsFromCacheAndMarkForInvalidation();
8686

8787
// Remove old id, that is guaranteed to be present in cache.
88-
treeScopeForSVGReferences().removeSVGResource(m_id);
88+
treeScopeForSVGReferences().removeSVGResource(m_id, *this);
8989
m_id = element().getIdAttribute();
9090

9191
registerResource();

0 commit comments

Comments
 (0)