Summary: | Improper handling of foreignobjects nested in svg groups | ||
---|---|---|---|
Product: | WebKit | Reporter: | Adam Bender <adambender> |
Component: | SVG | Assignee: | 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: |
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. Created attachment 113671 [details]
Patch
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 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 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). (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 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 Doesn't this affect pixel test results? If not you should definately create a test which shows the visible impact of this change! (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. (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. Created attachment 120182 [details]
Patch
Added reftest and rebaselined use-on-disallowed-foreign-object-3 & multiple-foreign-objects expectations.
Comment on attachment 120182 [details]
Patch
Good idea with the reftest, I should get used to it, r=me.
Comment on attachment 120182 [details] Patch Clearing flags on attachment: 120182 Committed r103465: <http://trac.webkit.org/changeset/103465> All reviewed patches have been landed. Closing bug. |
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.