Elements with display="none" in a <clipPath> still contribute to clipping path

• Component: SVG
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

Elements inside a <clipPath> that have display="none" should not contribute to the clipping path, but they currently do.

I thought perhaps that simply replacing SVGStyledTransformableElement::toClipPath() with:

  virtual Path toClipPath() const { return renderer() ? toPathData() : Path(); }

would work (on the assumption that there'd be no renderer for elements with display="none"), but in fact there is no renderer for any element in the <clipPath>.

[Cameron McCormack (:heycam)]

Relatedly, when computing the clipping path from a given <clipPath>, each child element of it needs to have its clipping path (i.e., what's specified by the 'clip-path' property) applied before being unioned into the resulting clipping path.

[Rob Buis]

Created attachment 20440 [details]
First attempt

This should do it. I am not sure whether I did the test stuff right, as in that the png is in the right dir...
The second remark from Cameron handles I think a different bug, and IMHO we need
a new bug and testcase to proof that we have this problem.
Cheers,

Rob.

[Cameron McCormack (:heycam)]

Filed bug 18398 with test case for the related bug.

[Eric Seidel (no email)]

Comment on attachment 20440 [details]
First attempt

Ok, so two things:

1. style error:
if ( pathStyle->display() != NONE )
(no spaces inside the ( ))

2.  You should test visibility: hidden as well.  What's the expected behavior there?

Also:
// FIXME: How do we know the element has done a layout?
We could pretty easily test the not-yet-layout case, but I'm not sure that layout even matters here, since we're calling a DOM method toClipPath(), however I'm not sure the DOM can really get the right proportions... how does it resolve lengths correctly?

We at least need a visibility: hidden test, IMO.

[Cameron McCormack (:heycam)]

(In reply to comment #4)
> 2.  You should test visibility: hidden as well.  What's the expected behavior
> there?

visibility='hidden' (but display!='inline') elements still contribute to the clip path.

[Rob Buis]

Created attachment 20456 [details]
Now with improved testcase

Fix the style issue and make the testcase check for visibility=hidden too.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 20456 [details]
Now with improved testcase

If you update the testcase to mention the purpose of visibility="hidden" (that we're testing to make sure that it does not affect clipping) then r=me.

I don't need to see the patch again before you land.

[Rob Buis]

Landed in r31805.