Bug 78613 – Paddings and borders on root SVG element with viewbox causes child SVG elements to be rendered with the incorrect size

• Component: SVG
• Status: RESOLVED FIXED

[Max Vujovic]

Created attachment 126989 [details]
Reproduction

Please see the attached reproduction for an example. Note that Opera renders the reproduction correctly.

[Max Vujovic]

Created attachment 126990 [details]
Screenshot

Added screenshot of reproduction.

[Max Vujovic]

Created attachment 127679 [details]
Initial patch for EWS bots. Do not review.

Trying out a fix. I needed to update the svg/custom/circle-move-invalidation.svg test, so I want to run the EWS bots.

[WebKit Review Bot]

Comment on attachment 127679 [details]
Initial patch for EWS bots. Do not review.

Attachment 127679 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542594

New failing tests:
svg/custom/circle-move-invalidation.svg

[Max Vujovic]

Created attachment 127846 [details]
Patch for EWS bots. Do not review.

[WebKit Review Bot]

Comment on attachment 127846 [details]
Patch for EWS bots. Do not review.

Attachment 127846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11546562

New failing tests:
svg/custom/circle-move-invalidation.svg

[Max Vujovic]

Created attachment 127927 [details]
Patch for EWS bots. Do not review.

[Max Vujovic]

Created attachment 128006 [details]
Patch for review.

Created a patch for review.

Here is the description of the fix, taken from the ChangeLog:
"""
        When computing its localToBorderBoxTransform, RenderSVGRoot was using its width and height
        as the dimensions of the SVG viewport. However, width and height include CSS borders and
        paddings, which are not part of the SVG viewport area. Now, RenderSVGRoot uses its
        contentWidth and contentHeight, which correspond to the SVG viewport area.
"""

[WebKit Review Bot]

Comment on attachment 128006 [details]
Patch for review.

Attachment 128006 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11561251

New failing tests:
svg/custom/circle-move-invalidation.svg

[Max Vujovic]

Created attachment 128054 [details]
Patch for review.

Updated the chromium test_expectations.txt file properly to pass cr-linux.

[WebKit Review Bot]

Comment on attachment 128054 [details]
Patch for review.

Attachment 128054 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11520335

New failing tests:
svg/custom/circle-move-invalidation.svg

[Nikolas Zimmermann]

Comment on attachment 128054 [details]
Patch for review.

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

Great catch, almost r+, still one issue:

> LayoutTests/platform/chromium/test_expectations.txt:933
> +BUGWK78613 : svg/custom/circle-move-invalidation-expected.svg = IMAGE IMAGE+TEXT

Not sure how this works for reftest, but probably oyu need to mark "circle-move-invaidation.svg" as failing, not the -expected.svg file.
cr-linux is still red, and I think this is the reason.

[Max Vujovic]

Created attachment 128243 [details]
Patch for review after bots cycle.

(In reply to comment #11)
> (From update of attachment 128054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128054&action=review
> 
> Great catch, almost r+, still one issue:
> 
> > LayoutTests/platform/chromium/test_expectations.txt:933
> > +BUGWK78613 : svg/custom/circle-move-invalidation-expected.svg = IMAGE IMAGE+TEXT
> 
> Not sure how this works for reftest, but probably oyu need to mark "circle-move-invaidation.svg" as failing, not the -expected.svg file.
> cr-linux is still red, and I think this is the reason.

Oops, you're right. Thanks for the review! I'm reuploading the patch, and I changed the line to:

+BUGWK78613 : svg/custom/circle-move-invalidation.svg = IMAGE IMAGE+TEXT

circle-move-invalidation.svg is a DRT + pixel test that I had to update. I added my own reftest called svg-root-padding-border-margin.html.

I'll wait for the cr-linux to cycle before setting r?, cq?.

[Max Vujovic]

Comment on attachment 128243 [details]
Patch for review after bots cycle.

Passed cr-linux. Setting r?, cq?.

[Nikolas Zimmermann]

Comment on attachment 128243 [details]
Patch for review after bots cycle.

r=me.

[WebKit Review Bot]

Comment on attachment 128243 [details]
Patch for review after bots cycle.

Clearing flags on attachment: 128243

Committed r108559: <http://trac.webkit.org/changeset/108559>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

Reopening bug. This seems to make svg/zoom/page/zoom-replaced-intrinsic-ratio.* fail :(
It already got marked as flky in DEBUG builds in platform/chromium, for me it fails locally in DRT 100%, but works in Safari. Weird.

[Nikolas Zimmermann]

(In reply to comment #17)
> Reopening bug. This seems to make svg/zoom/page/zoom-replaced-intrinsic-ratio.* fail :(
> It already got marked as flky in DEBUG builds in platform/chromium, for me it fails locally in DRT 100%, but works in Safari. Weird.
Okay, its flaky as well in Safari, reloading it moves the rectangles third rectangle down sometimes.

[Nikolas Zimmermann]

I've looked at the testcase:
    <object id="img1" type="image/svg+xml" data="resources/intrinsic-ratio.svg">This test won't work because you do not have images enabled.</object></p>
  <p><object id="img2" type="image/svg+xml" data="resources/intrinsic-ratio.svg">This test won't work because you do not have images enabled.</object></p>
  <p id="t4"><object id="img4" type="image/svg+xml" data="resources/intrinsic-ratio.svg">This test won't work because you do not have images enabled.</object></p>
  <table id="t5"><tr><td><object id="img5" type="image/svg+xml" data="resources/intrinsic-ratio.svg">This test won't work because you do not have images enabled.</object></td></tr></table>
  <p id="p3"><object id="img3" type="image/svg+xml" data="resources/intrinsic-ratio.svg">This test won't work because you do not have images enabled.</object></p>

The element <p id="t4"> is guilty. When you zoom out several times, it doesn't change its size, unless you reload. t4 contains following style:    #t4 { width: 15em; display: table-cell; }

When removing the display: table-cell the tests also zooms as expected. Must be a problem with preferred width/height calculation... Anyhow I reverted Max patch and the problem is still there!

Something else must have caused this..

[Nikolas Zimmermann]

(In reply to comment #19)
> Something else must have caused this..

commit 2668db931dd9dc7b8bb595e4b43c18e334c3ee05
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Wed Feb 22 23:46:48 2012 +0000

Still broken.

commit dffe42dac6795ee8f8154e789b41bbe127176a9b
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Tue Feb 21 23:30:52 2012 +0000

Works again w/o flakiness. I can look up corresponding SVN revisions, so we can find the culprit in that range.

[Nikolas Zimmermann]

(In reply to comment #20)
> 
> Works again w/o flakiness. I can look up corresponding SVN revisions, so we can find the culprit in that range.
Regression appeared between 108407 (works) .. 108568 (broken), will now trace it further.

[Nikolas Zimmermann]

(In reply to comment #21)
Tracked it down to: 108555 (works) ... 108568 (broken)

Only table related changes are in: http://trac.webkit.org/changeset/108557
I'll revert that now and retry.

[Nikolas Zimmermann]

Closing this bug again, will move further discussion into the bug which is guilty for the failure.

[Nikolas Zimmermann]

(In reply to comment #22)
> (In reply to comment #21)
> Tracked it down to: 108555 (works) ... 108568 (broken)
> 
> Only table related changes are in: http://trac.webkit.org/changeset/108557
> I'll revert that now and retry.

108556??? (works) ... 108557 (broken)

[Nikolas Zimmermann]

Oops, ignore my last comment, was a left-over. It's indeed the aforementioned <table> refactorization, which broke the test, not this bug.

[Stephen Chenney]

Committed r114415: <http://trac.webkit.org/changeset/114415>