Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mask clipping determination insufficient for <use> elements #15026

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Mask clipping determination insufficient for <use> elements #15026

merged 1 commit into from
Jun 16, 2023

Conversation

nikolaszimmermann
Copy link
Contributor

@nikolaszimmermann nikolaszimmermann commented Jun 15, 2023 •
edited by webkit-early-warning-system

df14838

Mask clipping determination insufficient for <use> elements
https://bugs.webkit.org/show_bug.cgi?id=258168

Reviewed by Rob Buis.

To determine if we need mask clipping or can use path clipping,
the renderer is queried for certain information. This is not
sufficient for <use> renderers -- the logic needs to extend to
the referenced renderer as well.

Previously when referencing e.g. a <text> element that is itself
clipped, from an <use> element (that is a child of <clipPath>)
we determined that path clipping is possible, leading to a
fully clipped object (net result: nothing visible).

Fix that issue, and handle <use> elements in the clip mask determination.

Covered by two new tests, exercising the <use> clipping peculiarities.

* LayoutTests/svg/clip-path/clip-path-use-referencing-clipped-text-expected.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-clipped-text.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-text-expected.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-text.html: Added.
Imported from Blink.

* Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:
(WebCore::RenderSVGResourceClipper::pathOnlyClipping):

Refactor the mask clipping determination logic into a helper lambda.
Re-use that lambda to check if the <use> referenced clip renderer
is required to use mask clipping, or if path clipping is possible.

Canonical link: https://commits.webkit.org/265238@main

e6d920f

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@nikolaszimmermann nikolaszimmermann self-assigned this Jun 15, 2023
@nikolaszimmermann nikolaszimmermann added the SVG For bugs in the SVG implementation. label Jun 15, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash e6d920f)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

rwlbuis
rwlbuis reviewed Jun 16, 2023
View reviewed changes
LayoutTests/svg/clip-path/clip-path-use-referencing-clipped-text-expected.html
@@ -0,0 +1,2 @@
<!DOCTYPE html>
<div style="width: 100px; height: 100px; background-color: green"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may have an existing html file featuring a green 100x100 rect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time I checked there was no mechanism to re-use files named differently, other than symlinks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SVG WPT tests use "<link rel="match" ...", is it possible to use that?

LayoutTests/svg/clip-path/clip-path-use-referencing-text-expected.html
@@ -0,0 +1,2 @@
<!DOCTYPE html>
<div style="width: 100px; height: 100px; background-color: green"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 16, 2023
@nikolaszimmermann nikolaszimmermann added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Jun 16, 2023
@nikolaszimmermann
Mask clipping determination insufficient for <use> elements
df14838 
https://bugs.webkit.org/show_bug.cgi?id=258168

Reviewed by Rob Buis.

To determine if we need mask clipping or can use path clipping,
the renderer is queried for certain information. This is not
sufficient for <use> renderers -- the logic needs to extend to
the referenced renderer as well.

Previously when referencing e.g. a <text> element that is itself
clipped, from an <use> element (that is a child of <clipPath>)
we determined that path clipping is possible, leading to a
fully clipped object (net result: nothing visible).

Fix that issue, and handle <use> elements in the clip mask determination.

Covered by two new tests, exercising the <use> clipping peculiarities.

* LayoutTests/svg/clip-path/clip-path-use-referencing-clipped-text-expected.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-clipped-text.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-text-expected.html: Added.
* LayoutTests/svg/clip-path/clip-path-use-referencing-text.html: Added.
Imported from Blink.

* Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:
(WebCore::RenderSVGResourceClipper::pathOnlyClipping):

Refactor the mask clipping determination logic into a helper lambda.
Re-use that lambda to check if the <use> referenced clip renderer
is required to use mask clipping, or if path clipping is possible.

Canonical link: https://commits.webkit.org/265238@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Mask-clipping-determination-insufficient-for-use-elements branch from e6d920f to df14838 Compare June 16, 2023 11:46
@webkit-commit-queue
Copy link
Collaborator

Committed 265238@main (df14838): https://commits.webkit.org/265238@main

Reviewed commits have been landed. Closing PR #15026 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit df14838 into WebKit:main Jun 16, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 16, 2023
@nikolaszimmermann nikolaszimmermann deleted the eng/Mask-clipping-determination-insufficient-for-use-elements branch June 16, 2023 11:52
@nikolaszimmermann
Copy link
Contributor Author

Thanks a lot @rwlbuis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwlbuis rwlbuis rwlbuis approved these changes

@shallawa shallawa Awaiting requested review from shallawa

Assignees

@nikolaszimmermann nikolaszimmermann

Labels
SVG For bugs in the SVG implementation.
Projects
None yet
Milestone
No milestone
5 participants
@nikolaszimmermann @webkit-early-warning-system @webkit-commit-queue @rwlbuis @webkit-ews-buildbot