Bug 76282 – ForeignObject content is zoomed two times

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

Created attachment 122441 [details]
FO content zoomed twice

The zoom factor is applied twice to FO-embedded elements. This affects both SVG and HTML content, but for different reasons:

* When embedding foreign SVG content, we end up with two RenderSVGRoot nodes on the render path:
  RenderSVGRoot {svg}
    RenderSVGForeignObject {foreignObject}
      RenderSVGRoot {svg}
        RenderSVGRect {rect}
        ...

Each RenderSVGRoot::paint() applies the zoom factor to its current canvas context - hence the duplicate zooming: http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp#L310 , http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp#L355

* When embedding foreign HTML content, the CSS properties appear to be pre-scaled by the zoom factor (http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleApplyProperty.cpp#L334), so the zoom introduced by RenderSVGRoot up the tree is compounding the effect.


See the attached test for an example; try zooming in/out a few times and observe how the FO content is scaled twice.
The test works correctly in FF8+.

[Florin Malita]

Created attachment 133107 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 133107 [details]
Patch

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

Good catch, I'd like to r+ this, though the test should be extended to cover text as well, or a new test should be added. Its possible that theres still a bug related to non-shapes drawing in fO trees.

> LayoutTests/svg/zoom/page/zoom-foreign-content.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="runRepaintTest()">

For the record: in theory this could use a reftest, if we'd zoom the subtrees by specifying zoom CSS properties, though for testing zooming the preferred way is to use the DRT functionality, as that uses the same code path, as if you'd zoom in eg. Safari. So using a pixel+repaint test is just right for this case.

> LayoutTests/svg/zoom/page/zoom-foreign-content.svg:7
> +            <rect width="99" height="99" fill="red"/>

I'd like to see another test that includes text in the SVG subtree (and maybe also in the HTML subtree). As <text> depends on the accumulated transforms up to RenderSVGRoot, we should verify this works as well, including zooming.

[Florin Malita]

(In reply to comment #2)
> (From update of attachment 133107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133107&action=review
> 
> Good catch, I'd like to r+ this, though the test should be extended to cover text as well, or a new test should be added. Its possible that theres still a bug related to non-shapes drawing in fO trees.

There's already a test covering zooming of FO text: svg/zoom/{page,text}/zoom-foreignObject.svg

Should I go ahead and add more coverage to it (SVG text maybe)? I was hoping to avoid having to deal with a gazillion platform rebaselines :)

[Florin Malita]

Created attachment 133535 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 133535 [details]
Patch

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

Looks great, r=me, with one comment:

> LayoutTests/platform/mac/Skipped:566
> +# Rebaseline needed after https://bugs.webkit.org/show_bug.cgi?id=76282
> +svg/zoom/page/zoom-foreignObject.svg
> +svg/zoom/text/zoom-foreignObject.svg

You can use platform/mac/test_expectations.txt as well btw, so there's no need to skip those tests!

[Florin Malita]

Created attachment 133635 [details]
Patch

[Florin Malita]

(In reply to comment #5)
> You can use platform/mac/test_expectations.txt as well btw, so there's no need to skip those tests!

Good to know! Still figuring out which platforms observe the expectations files :)

[WebKit Review Bot]

Comment on attachment 133635 [details]
Patch

Clearing flags on attachment: 133635

Committed r112022: <http://trac.webkit.org/changeset/112022>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.