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+.
Created attachment 133107 [details] Patch
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.
(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 :)
Created attachment 133535 [details] Patch
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!
Created attachment 133635 [details] Patch
(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 :)
Comment on attachment 133635 [details] Patch Clearing flags on attachment: 133635 Committed r112022: <http://trac.webkit.org/changeset/112022>
All reviewed patches have been landed. Closing bug.