Bug 18340 - Elements with display="none" in a <clipPath> still contribute to clipping path
Summary: Elements with display="none" in a <clipPath> still contribute to clipping path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://mcc.id.au/temp/2008/clipPath-d...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-07 04:21 PDT by Cameron McCormack
Modified: 2008-04-11 00:10 PDT (History)
0 users

See Also:


Attachments
First attempt (26.95 KB, patch)
2008-04-09 12:25 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Now with improved testcase (26.97 KB, patch)
2008-04-10 11:12 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack 2008-04-07 04:21:56 PDT
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>.
Comment 1 Cameron McCormack 2008-04-07 04:27:43 PDT
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.
Comment 2 Rob Buis 2008-04-09 12:25:14 PDT
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.
Comment 3 Cameron McCormack 2008-04-09 22:26:07 PDT
Filed bug 18398 with test case for the related bug.
Comment 4 Eric Seidel (no email) 2008-04-09 23:56:03 PDT
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.
Comment 5 Cameron McCormack 2008-04-10 00:06:16 PDT
(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.
Comment 6 Rob Buis 2008-04-10 11:12:49 PDT
Created attachment 20456 [details]
Now with improved testcase

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

Rob.
Comment 7 Eric Seidel (no email) 2008-04-10 23:27:18 PDT
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.
Comment 8 Rob Buis 2008-04-11 00:10:29 PDT
Landed in r31805.