Bug 185829 – [css-masking] Fully support -webkit-clip-path on SVG elements

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Currently there is just some rudimentary support for -webkit-clip-path on SVG elements. E.g:
* -webkit-clip-path does not contribute to hit-testing.
* References to <clipPath> do not work.
* <shape-box> only values do not work.

[Dirk Schulze]

Created attachment 343839 [details]
Patch

[EWS Watchlist]

Comment on attachment 343839 [details]
Patch

Attachment 343839 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8373725

New failing tests:
svg/dynamic-updates/SVGClipPathElement-prefixed-transform-influences-hitTesting.html
svg/dynamic-updates/SVGClipPath-prefixed-path-influences-hitTesting.html
svg/dynamic-updates/SVGClipPathElement-prefixed-css-transform-influences-hitTesting.html
svg/dynamic-updates/SVGClipPath-prefixed-influences-hitTesting.html

[EWS Watchlist]

Created attachment 343864 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 343839 [details]
Patch

Attachment 343839 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8375317

New failing tests:
svg/dynamic-updates/SVGClipPathElement-prefixed-transform-influences-hitTesting.html
svg/dynamic-updates/SVGClipPath-prefixed-path-influences-hitTesting.html
svg/dynamic-updates/SVGClipPathElement-prefixed-css-transform-influences-hitTesting.html
svg/dynamic-updates/SVGClipPath-prefixed-influences-hitTesting.html

[EWS Watchlist]

Created attachment 343875 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 343839 [details]
Patch

Attachment 343839 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8378703

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 343895 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dirk Schulze]

Created attachment 343897 [details]
Patch

[EWS Watchlist]

Comment on attachment 343897 [details]
Patch

Attachment 343897 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8382738

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 343912 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dirk Schulze]

The failing win tests are unrelated.

[Simon Fraser (smfr)]

Comment on attachment 343897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343897&action=review

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:408
> +inline FloatRect clipPathReferenceBox(const RenderElement& renderer, const CSSBoxType& boxType)

boxType can be passed by value. It's just an enum.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:410
> +    // Depends on https://bugs.webkit.org/show_bug.cgi?id=185797

This comment can be removed.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:411
> +    FloatRect referenceBox { };

No need for { }

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:413
> +    if (boxType == CSSBoxType::StrokeBox || boxType == CSSBoxType::BorderBox
> +        || boxType == CSSBoxType::MarginBox)

Don't wrap the lines this narrow.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:417
> +    } else if (boxType == CSSBoxType::ViewBox && renderer.element()) {

Maybe this would be better as a switch, so the compiler tells you if you forgot a value.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:421
> +        referenceBox.setWidth(viewportSize.width());
> +        referenceBox.setHeight(viewportSize.height());

Can this be setSize(viewportSize.width.size())?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:432
> +        FloatRect referenceBox {clipPathReferenceBox(renderer, clipPath.referenceBox())};

Would be simpler as FloatRect referenceBox = clipPathReferenceBox(renderer, clipPath.referenceBox());

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:439
> +        FloatRect referenceBox {clipPathReferenceBox(renderer, clipPath.referenceBox())};

Same.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:442
> +        return clipPath.pathForReferenceRect(FloatRoundedRect {referenceBox}).contains(point);

Is the explicit FloatRoundedRect necessary?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:453
> +        FloatRect referenceBox {clipPathReferenceBox(renderer, clipPath.referenceBox())};

Ditto.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:459
> +        FloatRect referenceBox {clipPathReferenceBox(renderer, clipPath.referenceBox())};
> +        context.clipPath(clipPath.pathForReferenceRect(FloatRoundedRect {referenceBox}));

Ditto.

[Dirk Schulze]

Created attachment 345032 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345032 [details]
Patch for landing

Clearing flags on attachment: 345032

Committed r233835: <https://trac.webkit.org/changeset/233835>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42841841>