Skip to content

Commit ccba346

Browse files
Ahmad-S792Ahmad Saleem
Ahmad-S792
authored and
Ahmad Saleem
committed
Fix clipping when <use> element in <clipPath> has visibility:hidden but references visible content
https://bugs.webkit.org/show_bug.cgi?id=304896 rdar://167491519 Reviewed by Nikolas Zimmermann and Brent Fulgham. This patch aligns WebKit with Gecko / Firefox and Blink / Chromium. Additionally, it fixes the bug in both LegacySVG and Layer Based SVG engine (LBSE). Merge: https://chromium.googlesource.com/chromium/src/+/53cbed151cea16f4d27cc887a272d9e672c41b75 When a <use> element inside a <clipPath> has visibility:hidden, but the element it references has visibility:visible, the clipping should still be applied based on the referenced element's visibility, not the <use> element's visibility. Previously, WebKit incorrectly checked the <use> element's own visibility state, causing valid clips to be ignored. Now we correctly look through to the referenced element to determine if clipping should be applied And remove reportError() to print error info for <clipPath> usage on SVGUseElement. * Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceClipper.cpp: (WebCore::LegacyRenderSVGResourceClipper::pathOnlyClipping): (WebCore::LegacyRenderSVGResourceClipper::drawContentIntoMaskImage): (WebCore::LegacyRenderSVGResourceClipper::calculateClipContentRepaintRect): * Source/WebCore/svg/SVGClipPathElement.cpp: (WebCore::SVGClipPathElement::shouldApplyPathClipping const): (WebCore::SVGClipPathElement::calculateClipContentRepaintRect): * Source/WebCore/svg/SVGUseElement.cpp: (WebCore::SVGUseElement::visibleTargetGraphicsElementForClipping const): (WebCore::SVGUseElement::toClipPath): * Source/WebCore/svg/SVGUseElement.h: * LayoutTests/svg/clip-path/visible-clip-path-as-hidden-use-element-expected.html: Added. * LayoutTests/svg/clip-path/visible-clip-path-as-hidden-use-element.html: Added. * LayoutTests/svg/clip-path/visible-nested-clip-path-as-hidden-use-element-expected.html: Added. * LayoutTests/svg/clip-path/visible-nested-clip-path-as-hidden-use-element.html: Added. Canonical link: https://commits.webkit.org/305374@main
1 parent 6aad1a4 commit ccba346

8 files changed

+93
-24
lines changed

‎LayoutTests/svg/clip-path/visible-clip-path-as-hidden-use-element-expected.html‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<!DOCTYPE html>
2+
<div style="width: 100px; height: 100px; background-color: green"></div>

‎LayoutTests/svg/clip-path/visible-clip-path-as-hidden-use-element.html‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<!DOCTYPE html>
2+
<svg>
3+
<defs>
4+
<rect id="rect" width="100" height="100" visibility="visible"></rect>
5+
<clipPath id="clip" visibility="hidden">
6+
<use xlink:href="#rect"/>
7+
</clipPath>
8+
</defs>
9+
<rect width="400" height="400" fill="green" clip-path="url(#clip)"></rect>
10+
</svg>

‎LayoutTests/svg/clip-path/visible-nested-clip-path-as-hidden-use-element-expected.html‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<!DOCTYPE html>
2+
<div style="width: 100px; height: 100px; background-color: green"></div>

‎LayoutTests/svg/clip-path/visible-nested-clip-path-as-hidden-use-element.html‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<!DOCTYPE html>
2+
<svg>
3+
<defs>
4+
<rect id="rect" width="100" height="100" visibility="visible"/>
5+
<clipPath id="clipClip">
6+
<rect width="100" height="100"/>
7+
</clipPath>
8+
<clipPath id="clip" clip-path="url(#clipClip)">
9+
<use visibility="hidden" xlink:href="#rect"/>
10+
</clipPath>
11+
</defs>
12+
<rect height="400" width="400" fill="green" clip-path="url(#clip)"/>
13+
</svg>

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

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,16 @@ auto LegacyRenderSVGResourceClipper::pathOnlyClipping(GraphicsContext& context,
121121
CheckedPtr renderer = graphicsElement->renderer();
122122
if (!renderer)
123123
continue;
124-
if (rendererRequiresMaskClipping(*renderer))
125-
return { };
126124

127-
// For <use> elements, delegate the decision whether to use mask clipping or not to the referenced element.
125+
// For <use> elements, check visibility of the target element and skip if no visible target.
128126
if (auto* useElement = dynamicDowncast<SVGUseElement>(graphicsElement.get())) {
129127
auto* clipChildRenderer = useElement->rendererClipChild();
130-
if (clipChildRenderer && rendererRequiresMaskClipping(*clipChildRenderer))
128+
if (!clipChildRenderer)
129+
continue;
130+
if (rendererRequiresMaskClipping(*clipChildRenderer))
131+
return { };
132+
} else {
133+
if (rendererRequiresMaskClipping(*renderer))
131134
return { };
132135
}
133136

@@ -265,7 +268,7 @@ bool LegacyRenderSVGResourceClipper::drawContentIntoMaskImage(ImageBuffer& maskI
265268
return false;
266269
}
267270
const RenderStyle& style = renderer->style();
268-
if (style.display() == DisplayType::None || style.usedVisibility() != Visibility::Visible)
271+
if (style.display() == DisplayType::None || (style.usedVisibility() != Visibility::Visible && !is<SVGUseElement>(child)))
269272
continue;
270273

271274
WindRule newClipRule = style.clipRule();
@@ -304,8 +307,14 @@ void LegacyRenderSVGResourceClipper::calculateClipContentRepaintRect(RepaintRect
304307
if (!renderer->isRenderOrLegacyRenderSVGShape() && !renderer->isRenderSVGText() && !childNode->hasTagName(SVGNames::useTag))
305308
continue;
306309
const RenderStyle& style = renderer->style();
307-
if (style.display() == DisplayType::None || style.usedVisibility() != Visibility::Visible)
308-
continue;
310+
if (style.display() == DisplayType::None || (style.usedVisibility() != Visibility::Visible && !childNode->hasTagName(SVGNames::useTag)))
311+
continue;
312+
313+
// For <use> elements, check if the clipping target is visible.
314+
if (auto* useElement = dynamicDowncast<SVGUseElement>(childNode)) {
315+
if (!useElement->visibleTargetGraphicsElement())
316+
continue;
317+
}
309318
m_clipBoundaries[repaintRectCalculation].unite(renderer->localToParentTransform().mapRect(renderer->repaintRectInLocalCoordinates(repaintRectCalculation)));
310319
}
311320
}

‎Source/WebCore/svg/SVGClipPathElement.cpp‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -146,19 +146,24 @@ RefPtr<SVGGraphicsElement> SVGClipPathElement::shouldApplyPathClipping() const
146146
CheckedPtr renderer = graphicsElement->renderer();
147147
if (!renderer)
148148
continue;
149-
if (rendererRequiresMaskClipping(*renderer))
150-
return nullptr;
151-
// Fallback to masking, if there is more than one clipping path.
152-
if (useGraphicsElement)
153-
return nullptr;
154149

155-
// For <use> elements, delegate the decision whether to use mask clipping or not to the referenced element.
150+
// For <use> elements, check visibility of the target element and skip if no visible target.
156151
if (auto* useElement = dynamicDowncast<SVGUseElement>(*graphicsElement)) {
157152
CheckedPtr clipChildRenderer = useElement->rendererClipChild();
158-
if (clipChildRenderer && rendererRequiresMaskClipping(*clipChildRenderer))
153+
if (!clipChildRenderer)
154+
continue;
155+
if (rendererRequiresMaskClipping(*clipChildRenderer))
156+
return nullptr;
157+
} else {
158+
// For non-<use> elements, check normally.
159+
if (rendererRequiresMaskClipping(*renderer))
159160
return nullptr;
160161
}
161162

163+
// Fallback to masking, if there is more than one clipping path.
164+
if (useGraphicsElement)
165+
return nullptr;
166+
162167
useGraphicsElement = WTF::move(graphicsElement);
163168
}
164169

@@ -191,8 +196,16 @@ FloatRect SVGClipPathElement::calculateClipContentRepaintRect(RepaintRectCalcula
191196
if (!renderer->isRenderSVGShape() && !renderer->isRenderSVGText() && !childNode->hasTagName(SVGNames::useTag))
192197
continue;
193198
auto& style = renderer->style();
194-
if (style.display() == DisplayType::None || style.usedVisibility() != Visibility::Visible)
199+
// For <use> elements, skip visibility check on the <use> itself, check target instead.
200+
if (style.display() == DisplayType::None || (style.usedVisibility() != Visibility::Visible && !childNode->hasTagName(SVGNames::useTag)))
195201
continue;
202+
203+
// For <use> elements, verify the target is visible and valid
204+
if (auto* useElement = dynamicDowncast<SVGUseElement>(childNode)) {
205+
if (!useElement->rendererClipChild())
206+
continue;
207+
}
208+
196209
auto r = renderer->repaintRectInLocalCoordinates(repaintRectCalculation);
197210
if (auto transform = transformationMatrixFromChild(downcast<RenderLayerModelObject>(*renderer)))
198211
r = transform->mapRect(r);

‎Source/WebCore/svg/SVGUseElement.cpp‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -342,21 +342,39 @@ static bool isDirectReference(const SVGElement& element)
342342
|| element.hasTagName(textTag);
343343
}
344344

345+
SVGGraphicsElement* SVGUseElement::visibleTargetGraphicsElement() const
346+
{
347+
RefPtr clone = this->targetClone();
348+
auto* targetElement = dynamicDowncast<SVGGraphicsElement>(clone.get());
349+
if (!targetElement)
350+
return nullptr;
351+
352+
CheckedPtr renderer = targetElement->renderer();
353+
if (!renderer)
354+
return nullptr;
355+
356+
auto& style = renderer->style();
357+
if (style.display() == DisplayType::None || style.usedVisibility() != Visibility::Visible)
358+
return nullptr;
359+
360+
// Spec: "If a <use> element is a child of a clipPath element, it must directly
361+
// reference <path>, <text> or basic shapes elements. Indirect references are an
362+
// error and the clipPath element must be ignored."
363+
if (!isDirectReference(*targetElement))
364+
return nullptr;
365+
366+
return targetElement;
367+
}
368+
345369
Path SVGUseElement::toClipPath()
346370
{
347371
RELEASE_ASSERT(!document().settings().layerBasedSVGEngineEnabled());
348372

349-
RefPtr targetClone = dynamicDowncast<SVGGraphicsElement>(this->targetClone());
350-
if (!targetClone)
373+
RefPtr element = visibleTargetGraphicsElement();
374+
if (!element)
351375
return { };
352376

353-
if (!isDirectReference(*targetClone)) {
354-
// Spec: Indirect references are an error (14.3.5)
355-
protectedDocument()->checkedSVGExtensions()->reportError("Not allowed to use indirect reference in <clip-path>"_s);
356-
return { };
357-
}
358-
359-
Path path = targetClone->toClipPath();
377+
Path path = element->toClipPath();
360378
SVGLengthContext lengthContext(this);
361379
// FIXME: Find a way to do this without manual resolution of x/y here. It's potentially incorrect.
362380
path.translate(FloatSize(x().value(lengthContext), y().value(lengthContext)));

‎Source/WebCore/svg/SVGUseElement.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ class SVGUseElement final : public SVGGraphicsElement, public SVGURIReference, p
4949
RefPtr<SVGElement> clipChild() const;
5050
RenderElement* rendererClipChild() const;
5151

52+
SVGGraphicsElement* visibleTargetGraphicsElement() const;
53+
5254
const SVGLengthValue& x() const { return m_x->currentValue(); }
5355
const SVGLengthValue& y() const { return m_y->currentValue(); }
5456
const SVGLengthValue& width() const { return m_width->currentValue(); }

0 commit comments

Comments
 (0)