Bug 69762 – Improper handling of foreignobjects nested in svg groups

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

[Adam Bender]

Created attachment 110362 [details]
A simple test case for an issue in the processing of foreignobjects

The best explanation for this issues is actually the attached test case but the gist is as follows: 

1) Embed a foreignobject tag inside a standard svg document. 
2) Wrap the foreignobject inside a single 'g' or group tag
3) Wrap all of that in another group tag

The foreign object will fail to render.

Note that I have verified this both in Chrome Nightlies as well as Safari 3.2. For comparison this works fine in Firefox 5, 6 and 7.

[Florin Malita]

Created attachment 113670 [details]
The green box positioned at 400px doesn't get painted if the browser window is resized to less than 800px wide.

This appears to be a problem with the repaint rectangle calculation in SVGRenderSupport::computeContainerBoundingBoxes: RenderSVGForeignObject does not use local coordinates for its objectBoundingBox, strokeBoundingBox & repaintRectInLocalCoordinates but simply returns the viewport rectangle (http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGForeignObject.h?rev=95040#L50). Then the local transform is applied to these non-local rectangles with an effective result of doubling the values.

For example, if the FO is positioned at 100,100 the calculated repaintBoundingBox will be positioned at 200,200.

So if the FO is positioned in the second half of the browser window, the calculated repaint rectangle for its parent group will appear to be off screen and painting will not occur. This can be tested with the attached example: resize/shrink the browser window such that green box would be located in the second half of the window -> the box disappears.

I have a patch that changes RenderSVGForeignObject::{objectBoundingBox,strokeBoundingBox,repaintRectInLocalCoordinates} to return local coordinates coming up shortly.

[Florin Malita]

Created attachment 113671 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 113671 [details]
Patch

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

> LayoutTests/platform/qt/svg/custom/use-on-non-svg-namespaced-element-expected.txt:5
> +  RenderSVGRoot {svg} at (54,54) size 746x546
>      RenderSVGForeignObject {foreignObject} at (10,10) size 480x360

This change seems reasonable, but these values still seem a little strange.   Is the FO really bigger than the SVG Root?

[Florin Malita]

Comment on attachment 113671 [details]
Patch

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

>> LayoutTests/platform/qt/svg/custom/use-on-non-svg-namespaced-element-expected.txt:5

> 
> This change seems reasonable, but these values still seem a little strange.   Is the FO really bigger than the SVG Root?

I'm not sure I follow your question, the FO does have a 5x scale transformation applied: transform="scale(5) skewY(5) skewX(5)".

There might be something more going on, but I think this is pushing it closer to correct values.

[Eric Seidel (no email)]

Comment on attachment 113671 [details]
Patch

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

Thank you for the fix.

>>> LayoutTests/platform/qt/svg/custom/use-on-non-svg-namespaced-element-expected.txt:5
>>>      RenderSVGForeignObject {foreignObject} at (10,10) size 480x360
>> 
>> This change seems reasonable, but these values still seem a little strange.   Is the FO really bigger than the SVG Root?
> 
> 

It just looks like the FO is being clipped by the svg root, given those origin values.  Or is the FO's origin relative to the SVG (actually, come to think of it, I think it is).

[Florin Malita]

(In reply to comment #5)
> (From update of attachment 113671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113671&action=review
> 
> Thank you for the fix.

Thanks for reviewing this.


> It just looks like the FO is being clipped by the svg root, given those origin values.  Or is the FO's origin relative to the SVG (actually, come to think of it, I think it is).

Oh, I see what you mean. But yes, that was my assumption too (relative FO origin).

[WebKit Review Bot]

Comment on attachment 113671 [details]
Patch

Rejecting attachment 113671 [details] from commit-queue.

New failing tests:
svg/custom/use-on-disallowed-foreign-object-3.svg
svg/foreignObject/multiple-foreign-objects.html
Full output: http://queues.webkit.org/results/10987560

[Nikolas Zimmermann]

Doesn't this affect pixel test results?
If not you should definately create a test which shows the visible impact of this change!

[Florin Malita]

(In reply to comment #8)
> Doesn't this affect pixel test results?

Kind of surprising, but it doesn't seem to.

> If not you should definately create a test which shows the visible impact of this change!

OK, added a reftest. I'm trying to figure out why the cq tests failed before pushing the update though.

[Florin Malita]

(In reply to comment #7)
> (From update of attachment 113671 [details])
> Rejecting attachment 113671 [details] from commit-queue.
> 
> New failing tests:
> svg/custom/use-on-disallowed-foreign-object-3.svg
> svg/foreignObject/multiple-foreign-objects.html
> Full output: http://queues.webkit.org/results/10987560

Gah, after scratching my head for a while (these are passing for me), I just realized that I haven't updated the patch after adding rebaselines for those two tests in my branch.

Update coming soon.

[Florin Malita]

Created attachment 120182 [details]
Patch

Added reftest and rebaselined use-on-disallowed-foreign-object-3 & multiple-foreign-objects expectations.

[Nikolas Zimmermann]

Comment on attachment 120182 [details]
Patch

Good idea with the reftest, I should get used to it, r=me.

[WebKit Review Bot]

Comment on attachment 120182 [details]
Patch

Clearing flags on attachment: 120182

Committed r103465: <http://trac.webkit.org/changeset/103465>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.