Bug 96381 – -webkit-clip-path should parse IRIs

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

-webkit-clip-path should parse IRIs

[Raul Hudea]

Created attachment 165258 [details]
patch

[WebKit Review Bot]

Attachment 165258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/masking/clip-path-referen..." exit_code: 1
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2431:  Extra space between return and CSSPrimitiveValue  [whitespace/declaration] [3]
Source/WebCore/rendering/RenderLayer.cpp:85:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 165258 [details]
patch

Attachment 165258 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13953971

[Build Bot]

Comment on attachment 165258 [details]
patch

Attachment 165258 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13987001

New failing tests:
fast/masking/parsing-clip-path-iri.html

[Dirk Schulze]

Comment on attachment 165258 [details]
patch

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

Please use phrases on FIXME's. There is a build warning that you never run into a code path in styleresolver.

> LayoutTests/css3/masking/clip-path-reference.html:21
> +<svg height="0">
> +    <clipPath id="c1" clipPathUnits="objectBoundingBox">

Please add a test with userSpaceOnUse as well. Note that you need to specify a with and height for the SVG element then:
<svg height="200" width="200">
<clipPath id="c1" clipPathUnits="userSpaceOnUse">
    <rect x="20" y="20" width="160" height="160"/>
</clipPath>

> Source/WebCore/css/StyleBuilder.cpp:-1690
> -template <ClipPathOperation* (RenderStyle::*getterFunction)() const, void (RenderStyle::*setterFunction)(PassRefPtr<ClipPathOperation>), ClipPathOperation* (*initialFunction)()>
> -class ApplyPropertyClipPath {
> -public:

I was told to use this template, since it is the new way to use StyleBuilder. Like discussed offline, the code from StyleResolver::createPathOperation should be moved here.

> Source/WebCore/css/StyleBuilder.cpp:-2055
> -    setPropertyHandler(CSSPropertyWebkitClipPath, ApplyPropertyClipPath<&RenderStyle::clipPath, &RenderStyle::setClipPath, &RenderStyle::initialClipPath>::createHandler());

Needs to be added again.

> Source/WebCore/css/StyleResolver.cpp:5171
> +    // FIXME: needs handling for load pending SVG documents (similar to what CSS Filters)

Add the reference to the bug report https://bugs.webkit.org/show_bug.cgi?id=90405.

> Source/WebCore/rendering/RenderLayer.cpp:3149
> +            // FIXME: doesn't work if we have an external reference to a SVG document or if the SVG element is after the HTML one

The external referencing doesn't work in SVG either. We have a bug report about that. The one with SVG after HTML should reference to the bug report: https://bugs.webkit.org/show_bug.cgi?id=90405

> Source/WebCore/rendering/RenderLayer.cpp:3151
> +
> +

One empty line s enough.

> Source/WebCore/rendering/RenderLayer.cpp:3155
> +                RenderSVGResourceClipper* clipper = static_cast<RenderSVGResourceClipper*>(clipPath->renderer());
> +                if (clipper)

If you don't have more checks, move the initialisation into the if function: 
if (RenderSVGResourceClipper* clipper = static_cast<RenderSVGResourceClipper*>(clipPath->renderer()))

> Source/WebCore/rendering/RenderLayer.cpp:3156
> +                    clipper->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, transparencyLayerContext);

should be context, not transparencyLayerContext.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:58
> +    // clipPath can be clipped too, but don't have a boundingBox or repaintRect. So we can't call
> +    // applyResource directly and use the rects from the object, since they are empty for RenderSVGResources
> +    // This function is public because we need to apply clipping on HTML element
> +    bool applyClippingToContext(RenderObject*, const FloatRect&, const FloatRect&, GraphicsContext*);

They should have a bounding box. Seems to be a bug.

[Dirk Schulze]

Forgot to say: This is really awesome code! Great work! :)

[Raul Hudea]

Created attachment 165340 [details]
Updated patch

[Raul Hudea]

Created attachment 165342 [details]
removed extra include

[Build Bot]

Comment on attachment 165342 [details]
removed extra include

Attachment 165342 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13985655

New failing tests:
fast/masking/parsing-clip-path-iri.html

[WebKit Review Bot]

Comment on attachment 165342 [details]
removed extra include

Attachment 165342 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13989641

New failing tests:
fast/masking/parsing-clip-path-iri.html
css3/masking/clip-path-reference-userSpaceOnUse.html

[WebKit Review Bot]

Comment on attachment 165342 [details]
removed extra include

Attachment 165342 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13996664

New failing tests:
fast/masking/parsing-clip-path-iri.html
css3/masking/clip-path-reference-userSpaceOnUse.html

[Dirk Schulze]

Do you have the time to investigate in the test failure?

[Nikolas Zimmermann]

Comment on attachment 165342 [details]
removed extra include

Looks sane to me. I'll leave the final review for DIrk though as he published comments earlier.

[Dirk Schulze]

Created attachment 167456 [details]
Patch

Just updated the style issues and added on ref test for chromium to TextExpectations. Seems to be a Chromium specific bug. Opened a separate bug report.

[Dirk Schulze]

Dan asked to make one more change on IRC: cssUrlValue => cssURLValue

Will do that before landing.

[Dirk Schulze]

Committed r130592: <http://trac.webkit.org/changeset/130592>

[Simon Fraser (smfr)]

Comment on attachment 167456 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3150
> +                static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context);

Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct.

[Dirk Schulze]

(In reply to comment #17)
> (From update of attachment 167456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167456&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:3150
> > +                static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context);
> 
> Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct.

I'll check.

[Dirk Schulze]

(In reply to comment #17)
> (From update of attachment 167456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167456&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:3150
> > +                static_cast<RenderSVGResourceClipper*>(clipPath->renderer())->applyClippingToContext(renderer(), calculateLayerBounds(this, rootLayer, 0), paintDirtyRect, context);
> 
> Why doesn't this code set hasClipPath to true? Don't we need to restore the context at the end of the function? In fact, applyClippingToContext() clips the context without saving state, so I can't see how this is correct.

Looking at the source, we indeed do not restore the context on SVG clipping. I was more worried if we call save without restore, but this isn't the case.

The most important part is that we do not have a security issue here and we haven't. I can not say if the painted result is correct under all circumstances. I guess it just needs testing. However, saving and restoring for SVG clipping shouldn't cause rendering issues either.

If you have a test where the rendering fails, we can add save and restore guards.