Bug 69762

Summary: Improper handling of foreignobjects nested in svg groups
Product: WebKit Reporter: Adam Bender <adambender>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adambender, eric, fmalita, pdr, rwlbuis, simon.fraser, webkit.review.bot, zimmermann
Priority: P2    
Version: 525.x (Safari 3.2)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
A simple test case for an issue in the processing of foreignobjects
none
The green box positioned at 400px doesn't get painted if the browser window is resized to less than 800px wide.
none
Patch
none
Patch none

Description Adam Bender 2011-10-10 09:26:06 PDT
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.
Comment 1 Florin Malita 2011-11-04 09:46:23 PDT
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.
Comment 2 Florin Malita 2011-11-04 09:49:55 PDT
Created attachment 113671 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-12-19 14:07:52 PST
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?
Comment 4 Florin Malita 2011-12-20 10:50:42 PST
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.
Comment 5 Eric Seidel (no email) 2011-12-20 13:17:56 PST
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).
Comment 6 Florin Malita 2011-12-20 13:28:08 PST
(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).
Comment 7 WebKit Review Bot 2011-12-20 16:16:30 PST
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
Comment 8 Nikolas Zimmermann 2011-12-21 04:53:29 PST
Doesn't this affect pixel test results?
If not you should definately create a test which shows the visible impact of this change!
Comment 9 Florin Malita 2011-12-21 08:04:44 PST
(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.
Comment 10 Florin Malita 2011-12-21 08:52:05 PST
(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.
Comment 11 Florin Malita 2011-12-21 09:02:24 PST
Created attachment 120182 [details]
Patch

Added reftest and rebaselined use-on-disallowed-foreign-object-3 & multiple-foreign-objects expectations.
Comment 12 Nikolas Zimmermann 2011-12-21 12:36:28 PST
Comment on attachment 120182 [details]
Patch

Good idea with the reftest, I should get used to it, r=me.
Comment 13 WebKit Review Bot 2011-12-21 15:26:21 PST
Comment on attachment 120182 [details]
Patch

Clearing flags on attachment: 120182

Committed r103465: <http://trac.webkit.org/changeset/103465>
Comment 14 WebKit Review Bot 2011-12-21 15:26:27 PST
All reviewed patches have been landed.  Closing bug.