Bug 12571 – <clipPath> doesn't correctly handle <text> elements

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

[Eric Seidel (no email)]

WebKit fails SVG text-clip test

Notice that the bottom text-clip is missing.

[Eric Seidel (no email)]

Bleh.  Our current clipping code assumes that the contents of <clipPath> will only be path elements.  It needs to be smarter than that.  It needs to perhaps have a "fast path" where it builds a single combined path element and clips to that, but has fallback code to handle mask-based clipping when necessary.

Another option is to not build a "clip path" at all until it's actually used.  Still for speed it would be best to either create a single path to use for clipping, or to create a mask for clipping.

Either way, <clipPath> needs to be re-written to allow for clipping to text.  We also need to add tests to test clipping more complicated situations than the W3C suite currently handles.

[Nikolas Zimmermann]

Ok, we discussed this.  Eric will fix.  Eric will remove all of the clipping pre-caching code, and just walk each of the clipPath children each time a clip is applied.  This will allow <text> elements to do something differenet at clip-time instead of trying to convert text into a path (which CG doesn't support).

[Eric Seidel (no email)]

Perhaps Rob would like to tackle this after fixing the <clipPath> <use> issue. :)

[Dirk Schulze]

(In reply to comment #1)
> Another option is to not build a "clip path" at all until it's actually used. 
> Still for speed it would be best to either create a single path to use for
> clipping, or to create a mask for clipping.

Thats the way I would prefer. Never the less. Our current implementation even fails on very simple cases (only paths, one clip-rule - I'll add one of this cases afterwards). 

So there might be no other way to allways use masking. I think even ff masks all the time, but they just have began to support animation. Maybe they also run into speed problems.
 I'll investigate more on it and hopefully find a way to hold the speed on "normal" cases: path+clip-rule:nonzero

[Dirk Schulze]

Created attachment 48714 [details]
failing simple clipPath example

This is a simple implementation, where we fail. There are just paths and the same clipping rule for every child of clipPath.

[Dirk Schulze]

Created attachment 48715 [details]
clipper on clipPath

Also clipPaths can be clipped. We don't support it yet. But I can imagine a testcase, like this one, but with 360 clippings (one per degree) to get performance data as a reference to e.g. opera. Or one clipping per degree and a slopy scale factor.

[Dirk Schulze]

Created attachment 51191 [details]
patch

This adds support for texts on clipPath, clipping on ClipPath and heterogenous clip rules. We now pass another test on the current W3C test suite and 4 addional tests on the upcoming test suite for SVG1.1SE.

Tests moved into another patch to shrink size.

[Dirk Schulze]

Created attachment 51193 [details]
tests

Tests for clip patch.

[Nikolas Zimmermann]

Comment on attachment 51191 [details]
patch

Hi Dirk,

some random notes:

1) can you comment why  RenderSVGResourceClipper::invalidate now only takes() the value out of the HashMap instead of actually deleting it?
Who's responsible to delete it? I see it's reused in calculateClipper() (which needs a rename to sth. more meaningful btw.. :-) - though what happens if you are invalidating a resource, that is never reused again? Won't your new code just leave the ClipperData object undeleted?

2) In RenderSVGResourceClipper::fastClipper (which should be renamed to sth. like pathOnlyClipping, instead of fast..) can you introduce local variable for RenderSVGSTyle & co instead of callling style()->svgStyle() several times...

3) I am slightly worried about the approach you've chosen to implement the non-fast-clipping mode, especially the RenderSttyle modifications... how about creating a copy of the RenderSTyle that can be safely mutated and stored inside the RenderSVGResourceClipper?

4) ClipperData doesn't need an empty default ctor

The general approach is _great_ though, glad you finally tried to fix that issue!
r- until all issues are resolved.

[Eric Seidel (no email)]

Comment on attachment 51193 [details]
tests

Please do the RenderSVGShadowTreeRootContainer rename change in a separate patch, first.  Before doing any of the rest of this.  That would make these changes much smaller (and then people could actually look at your new tests and decide if they are good or not).

[Dirk Schulze]

Created attachment 52510 [details]
patch

Corrected issues of Niko as far as possible. Checked that the dtor's of ImageBuffer get called, works. Tests are in the following patch.

[Dirk Schulze]

Created attachment 52511 [details]
tests

Following Erics suggestion and removed the ShadowTree name for DRT results. Only clip-path gets tested.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ERROR: File does not exist: LayoutTests/svg/custom/clip-path-display-none-child.svg


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

[Chris Jerdonek]

(In reply to comment #13)
> Attachment 52511 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> ERROR: File does not exist:
> LayoutTests/svg/custom/clip-path-display-none-child.svg

FYI, this was a bug in check-webkit-style that was fixed after this occurred:

http://trac.webkit.org/changeset/57119

(Eric just CC'ed me on this report.)

[Nikolas Zimmermann]

Comment on attachment 52510 [details]
patch

Moin Dirk,

I still have some questions, sorry for the number of iteration with this patch, but it's quite involved :-)

> +    // ClipPath gets clipped.
"If a clipPath gets clipped, we need to use image-based clipping" would be easier to understand.

Ok I see the approach below, pathOnlyClipping() should check wheter we can use path only clipping, or return false if not.
So you optimized for the fast-case, where we only have paths to clip. If there are more complex objects to clip, you'd waste time calling toClipPath() on the shape objects though. I think it's still reasonable to do that. If we ever see this is a bottleneck, we may want to introduce a seperated function which checks wheter path-only clipping is possible _in advance_. (I just remember Beth identifying toPathData() as a bottle-neck in a certain situation...) Hm, feel free to keep this approach for now, or introduce a new function "isPathOnlyClippingPossible".


> +    Path clipPath = Path();
> +    for (Node* childNode = node()->firstChild(); childNode; childNode = childNode->nextSibling()) {
...
> +        if (!svgStyle->clipPath().isEmpty())
> +            return false;
How about adding a comment that clip-path on children inside a <clipPath> disallow path-only clipping?

> +        if (clipPath.isEmpty()) {
> +            clipPath = styled->toClipPath();
> +            clipRule = svgStyle->clipRule();
> +        } else
> +            return false;

Can you explain that approach? You're initially setting clipPath to Path(), then assign a toClipPath() of the first path-only clipping child.
So when there are two <rect> as children of a <clipPath> you will fall-back to image-based clipping? Even when both share the same clip-rule?
Am I overlooking something?


> +    }
> +    if (static_cast<SVGClipPathElement*>(node())->clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> +        AffineTransform obbTransform;
> +        obbTransform.translate(objectBoundingBox.x(), objectBoundingBox.y());
> +        obbTransform.scaleNonUniform(objectBoundingBox.width(), objectBoundingBox.height());
> +        clipPath.transform(obbTransform);
Please use "transform" as name, obbTransform looks awkward, and we shouldn't use abbrevations, and "objectBoundingBoxTransform" is way too long ;-)

> +    if (clipPath.isEmpty())
> +        clipPath.addRect(FloatRect());
I know why you need this, but a comment would help for others reading the code.

> +bool RenderSVGResourceClipper::applyClippingToContext(RenderObject* object, const FloatRect& objectBoundingBox,
> +                                                const FloatRect& repaintRect, GraphicsContext* context)
Can you align the "const' with the "RenderObject", looks slightly better.


...
> +        // Save the old RenderStyle of the current object for restoring after drawing
> +        // it to the MaskImage. The new intermediate RenderStyle needs to inherit from
> +        // the old one.
> +        RefPtr<RenderStyle> oldRenderStyle = RenderStyle::clone(renderer->style());
> +        RefPtr<RenderStyle> newRenderStyle = RenderStyle::clone(renderer->style());
That puzzles me. Why do you need two clones? Can't you just grab the old renderer->style() and stash it into a RefPtr<RenderStyle>? I guess it's a copy/paste error.
Can you check wheter a cloned RenderStyle is automatically "unique" (There's a setUnique() method on RenderStyle, preventing this style to be shared across different renderers, we have to be sure that happens, otherwhise we'll get in trouble)

> +
> +        if (isUseElement)
> +            renderer = childNode->renderer();
A comment regarding the special <use> handling would help here as well.

Other than that, looks great to me. Hope we can resolve the remaining issues quickly...

Cheers,
Niko

[Dirk Schulze]

Created attachment 53238 [details]
patch

Added some more comments, so that the code should be more understandable. Fixed the clone issue of RenderStyle and updated the patch to trunk.

[Nikolas Zimmermann]

Comment on attachment 52511 [details]
tests

Repeat after me: 33 tests. W o w.

[Nikolas Zimmermann]

Comment on attachment 53238 [details]
patch

Patch is nice, comments contains some typos though and could be clearer. Talked to Dirk who will those issues before landing, r=me.

[Dirk Schulze]

Landed in http://trac.webkit.org/changeset/57511.