Bug 78613

Summary: Paddings and borders on root SVG element with viewbox causes child SVG elements to be rendered with the incorrect size
Product: WebKit Reporter: Max Vujovic <mvujovic>
Component: SVGAssignee: Max Vujovic <mvujovic>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 78037    
Attachments:
Description Flags
Reproduction
none
Screenshot
none
Initial patch for EWS bots. Do not review.
mvujovic: commit-queue-
Patch for EWS bots. Do not review.
mvujovic: commit-queue-
Patch for EWS bots. Do not review.
mvujovic: commit-queue-
Patch for review.
webkit.review.bot: commit-queue-
Patch for review.
zimmermann: review-, webkit.review.bot: commit-queue-
Patch for review after bots cycle. none

Description Max Vujovic 2012-02-14 09:46:08 PST
Created attachment 126989 [details]
Reproduction

Please see the attached reproduction for an example. Note that Opera renders the reproduction correctly.
Comment 1 Max Vujovic 2012-02-14 09:48:04 PST
Created attachment 126990 [details]
Screenshot

Added screenshot of reproduction.
Comment 2 Max Vujovic 2012-02-17 17:19:05 PST
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.
Comment 3 WebKit Review Bot 2012-02-17 20:56:50 PST
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
Comment 4 Max Vujovic 2012-02-20 13:50:06 PST
Created attachment 127846 [details]
Patch for EWS bots. Do not review.
Comment 5 WebKit Review Bot 2012-02-20 14:26:01 PST
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
Comment 6 Max Vujovic 2012-02-20 23:46:17 PST
Created attachment 127927 [details]
Patch for EWS bots. Do not review.
Comment 7 Max Vujovic 2012-02-21 11:20:27 PST
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.
"""
Comment 8 WebKit Review Bot 2012-02-21 15:10:56 PST
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
Comment 9 Max Vujovic 2012-02-21 15:17:38 PST
Created attachment 128054 [details]
Patch for review.

Updated the chromium test_expectations.txt file properly to pass cr-linux.
Comment 10 WebKit Review Bot 2012-02-21 19:11:40 PST
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
Comment 11 Nikolas Zimmermann 2012-02-22 00:40:14 PST
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.
Comment 12 Max Vujovic 2012-02-22 10:37:29 PST
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?.
Comment 13 Max Vujovic 2012-02-22 13:36:21 PST
Comment on attachment 128243 [details]
Patch for review after bots cycle.

Passed cr-linux. Setting r?, cq?.
Comment 14 Nikolas Zimmermann 2012-02-22 14:10:23 PST
Comment on attachment 128243 [details]
Patch for review after bots cycle.

r=me.
Comment 15 WebKit Review Bot 2012-02-22 15:17:31 PST
Comment on attachment 128243 [details]
Patch for review after bots cycle.

Clearing flags on attachment: 128243

Committed r108559: <http://trac.webkit.org/changeset/108559>
Comment 16 WebKit Review Bot 2012-02-22 15:17:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Nikolas Zimmermann 2012-02-24 01:59:32 PST
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.
Comment 18 Nikolas Zimmermann 2012-02-24 02:01:31 PST
(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.
Comment 19 Nikolas Zimmermann 2012-02-24 02:23:49 PST
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..
Comment 20 Nikolas Zimmermann 2012-02-24 04:45:27 PST
(In reply to comment #19)
> Something else must have caused this..

commit 2668db931dd9dc7b8bb595e4b43c18e334c3ee05
Author: barraclough@apple.com <barraclough@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Wed Feb 22 23:46:48 2012 +0000

Still broken.

commit dffe42dac6795ee8f8154e789b41bbe127176a9b
Author: noam.rosenthal@nokia.com <noam.rosenthal@nokia.com@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.
Comment 21 Nikolas Zimmermann 2012-02-24 04:53:27 PST
(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.
Comment 22 Nikolas Zimmermann 2012-02-24 06:55:36 PST
(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.
Comment 23 Nikolas Zimmermann 2012-02-24 06:56:33 PST
Closing this bug again, will move further discussion into the bug which is guilty for the failure.
Comment 24 Nikolas Zimmermann 2012-02-25 01:47:25 PST
(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)
Comment 25 Nikolas Zimmermann 2012-02-25 01:48:46 PST
Oops, ignore my last comment, was a left-over. It's indeed the aforementioned <table> refactorization, which broke the test, not this bug.
Comment 26 Stephen Chenney 2012-04-17 12:32:05 PDT
Committed r114415: <http://trac.webkit.org/changeset/114415>