Skip to content

Commit 83bf26f

Browse files
nullhooknullhook
committed
Fix SVG clipPath bounding box transform order with objectBoundingBox units
https://bugs.webkit.org/show_bug.cgi?id=304836 rdar://167417135 Reviewed by Simon Fraser. When a clipPath has both clipPathUnits="objectBoundingBox" and a local transform, the bounding box calculation applied transforms in the wrong order. The local transform was applied before the objectBoundingBox transform, causing it to be scaled incorrectly. The correct order is: OBB transform first (to realize 0-1 coordinates to pixels), then local transform (in pixel space). Tests: svg/clip-path/hittest-transformed-clip-objectboundingbox.html svg/clip-path/transformed-clip-expected.svg svg/clip-path/transformed-clip.svg * LayoutTests/svg/clip-path/hittest-transformed-clip-objectboundingbox-expected.txt: Added. * LayoutTests/svg/clip-path/hittest-transformed-clip-objectboundingbox.html: Added. * LayoutTests/svg/clip-path/transformed-clip-expected.svg: Added. * LayoutTests/svg/clip-path/transformed-clip.svg: Added. * Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceClipper.cpp: (WebCore::LegacyRenderSVGResourceClipper::calculateClipContentRepaintRect): (WebCore::LegacyRenderSVGResourceClipper::hitTestClipContent): (WebCore::LegacyRenderSVGResourceClipper::resourceBoundingBox): Canonical link: https://commits.webkit.org/305078@main
1 parent adac9d9 commit 83bf26f

File tree

5 files changed

+87
-6
lines changed

5 files changed

+87
-6
lines changed

‎LayoutTests/svg/clip-path/hittest-transformed-clip-objectboundingbox-expected.txt‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
Test that hit-testing correctly handles clipPath with objectBoundingBox units and a local transform. The clip should be at (50, 50) with size 100x100.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS document.elementFromPoint(75, 75).id is "target"
7+
PASS document.elementFromPoint(100, 100).id is "target"
8+
PASS document.elementFromPoint(140, 140).id is "target"
9+
PASS document.elementFromPoint(25, 25).id is ''
10+
PASS document.elementFromPoint(175, 175).id is ''
11+
PASS document.elementFromPoint(10, 100).id is ''
12+
PASS document.elementFromPoint(100, 10).id is ''
13+
PASS document.elementFromPoint(51, 51).id is "target"
14+
PASS document.elementFromPoint(149, 149).id is "target"
15+
PASS document.elementFromPoint(49, 49).id is ''
16+
PASS successfullyParsed is true
17+
18+
TEST COMPLETE
19+

‎LayoutTests/svg/clip-path/hittest-transformed-clip-objectboundingbox.html‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>Hit testing with transformed clipPath using objectBoundingBox units</title>
5+
<script src="../../resources/js-test.js"></script>
6+
<style>
7+
svg {
8+
position: absolute;
9+
top: 0;
10+
left: 0;
11+
}
12+
</style>
13+
</head>
14+
<body>
15+
<svg width="400" height="400">
16+
<defs>
17+
<clipPath id="clip" clipPathUnits="objectBoundingBox" transform="translate(50, 50)">
18+
<rect width="0.5" height="0.5"/>
19+
</clipPath>
20+
</defs>
21+
22+
<rect id="target" width="200" height="200" fill="green" clip-path="url(#clip)"/>
23+
</svg>
24+
25+
<script>
26+
description('Test that hit-testing correctly handles clipPath with objectBoundingBox units and a local transform. The clip should be at (50, 50) with size 100x100.');
27+
28+
shouldBeEqualToString("document.elementFromPoint(75, 75).id", "target");
29+
shouldBeEqualToString("document.elementFromPoint(100, 100).id", "target");
30+
shouldBeEqualToString("document.elementFromPoint(140, 140).id", "target");
31+
32+
// Points outside the clipped region but inside the original rect bounds
33+
// These should NOT hit the target because they're clipped away
34+
shouldBe("document.elementFromPoint(25, 25).id", "''");
35+
shouldBe("document.elementFromPoint(175, 175).id", "''");
36+
shouldBe("document.elementFromPoint(10, 100).id", "''");
37+
shouldBe("document.elementFromPoint(100, 10).id", "''");
38+
39+
// Edge cases - just inside the clip boundary
40+
shouldBeEqualToString("document.elementFromPoint(51, 51).id", "target");
41+
shouldBeEqualToString("document.elementFromPoint(149, 149).id", "target");
42+
43+
// Edge cases - just outside the clip boundary
44+
shouldBe("document.elementFromPoint(49, 49).id", "''");
45+
</script>
46+
</body>
47+
</html>

‎LayoutTests/svg/clip-path/transformed-clip-expected.svg‎

Lines changed: 3 additions & 0 deletions
Loading

‎LayoutTests/svg/clip-path/transformed-clip.svg‎

Lines changed: 9 additions & 0 deletions
Loading

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,6 @@ void LegacyRenderSVGResourceClipper::calculateClipContentRepaintRect(RepaintRect
308308
continue;
309309
m_clipBoundaries[repaintRectCalculation].unite(renderer->localToParentTransform().mapRect(renderer->repaintRectInLocalCoordinates(repaintRectCalculation)));
310310
}
311-
m_clipBoundaries[repaintRectCalculation] = clipPathElement().animatedLocalTransform().mapRect(m_clipBoundaries[repaintRectCalculation]);
312311
}
313312

314313
bool LegacyRenderSVGResourceClipper::hitTestClipContent(const FloatRect& objectBoundingBox, const FloatPoint& nodeAtPoint)
@@ -325,15 +324,17 @@ bool LegacyRenderSVGResourceClipper::hitTestClipContent(const FloatRect& objectB
325324
if (!SVGRenderSupport::pointInClippingArea(*this, point))
326325
return false;
327326

327+
// The forward transform order is: OBB first, then local transform.
328+
// So the inverse order is: inverse local first, then inverse OBB.
329+
point = valueOrDefault(clipPathElement().animatedLocalTransform().inverse()).mapPoint(point);
330+
328331
if (clipPathElement().clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
329332
AffineTransform transform;
330333
transform.translate(objectBoundingBox.location());
331334
transform.scale(objectBoundingBox.size());
332335
point = valueOrDefault(transform.inverse()).mapPoint(point);
333336
}
334337

335-
point = valueOrDefault(clipPathElement().animatedLocalTransform().inverse()).mapPoint(point);
336-
337338
for (Node* childNode = clipPathElement().firstChild(); childNode; childNode = childNode->nextSibling()) {
338339
RenderObject* renderer = childNode->renderer();
339340
if (!childNode->isSVGElement() || !renderer)
@@ -360,19 +361,21 @@ FloatRect LegacyRenderSVGResourceClipper::resourceBoundingBox(const RenderObject
360361
});
361362
return object.objectBoundingBox();
362363
}
363-
364+
364365
if (m_clipBoundaries[repaintRectCalculation].isEmpty())
365366
calculateClipContentRepaintRect(repaintRectCalculation);
366367

368+
auto clipBoundaries = m_clipBoundaries[repaintRectCalculation];
369+
367370
if (clipPathElement().clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
368371
FloatRect objectBoundingBox = object.objectBoundingBox();
369372
AffineTransform transform;
370373
transform.translate(objectBoundingBox.location());
371374
transform.scale(objectBoundingBox.size());
372-
return transform.mapRect(m_clipBoundaries[repaintRectCalculation]);
375+
clipBoundaries = transform.mapRect(clipBoundaries);
373376
}
374377

375-
return m_clipBoundaries[repaintRectCalculation];
378+
return clipPathElement().animatedLocalTransform().mapRect(clipBoundaries);
376379
}
377380

378381
}

0 commit comments

Comments
 (0)