RESOLVED FIXED 78613
Paddings and borders on root SVG element with viewbox causes child SVG elements to be rendered with the incorrect size
https://bugs.webkit.org/show_bug.cgi?id=78613
Summary Paddings and borders on root SVG element with viewbox causes child SVG elemen...
Max Vujovic
Reported 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.
Attachments
Reproduction (1.22 KB, text/html)
2012-02-14 09:46 PST, Max Vujovic
no flags
Screenshot (22.84 KB, image/png)
2012-02-14 09:48 PST, Max Vujovic
no flags
Initial patch for EWS bots. Do not review. (27.10 KB, patch)
2012-02-17 17:19 PST, Max Vujovic
mvujovic: commit-queue-
Patch for EWS bots. Do not review. (27.10 KB, patch)
2012-02-20 13:50 PST, Max Vujovic
mvujovic: commit-queue-
Patch for EWS bots. Do not review. (27.11 KB, patch)
2012-02-20 23:46 PST, Max Vujovic
mvujovic: commit-queue-
Patch for review. (29.42 KB, patch)
2012-02-21 11:20 PST, Max Vujovic
webkit.review.bot: commit-queue-
Patch for review. (29.43 KB, patch)
2012-02-21 15:17 PST, Max Vujovic
zimmermann: review-
webkit.review.bot: commit-queue-
Patch for review after bots cycle. (29.42 KB, patch)
2012-02-22 10:37 PST, Max Vujovic
no flags
Max Vujovic
Comment 1 2012-02-14 09:48:04 PST
Created attachment 126990 [details] Screenshot Added screenshot of reproduction.
Max Vujovic
Comment 2 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.
WebKit Review Bot
Comment 3 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
Max Vujovic
Comment 4 2012-02-20 13:50:06 PST
Created attachment 127846 [details] Patch for EWS bots. Do not review.
WebKit Review Bot
Comment 5 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
Max Vujovic
Comment 6 2012-02-20 23:46:17 PST
Created attachment 127927 [details] Patch for EWS bots. Do not review.
Max Vujovic
Comment 7 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. """
WebKit Review Bot
Comment 8 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
Max Vujovic
Comment 9 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.
WebKit Review Bot
Comment 10 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
Nikolas Zimmermann
Comment 11 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.
Max Vujovic
Comment 12 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?.
Max Vujovic
Comment 13 2012-02-22 13:36:21 PST
Comment on attachment 128243 [details] Patch for review after bots cycle. Passed cr-linux. Setting r?, cq?.
Nikolas Zimmermann
Comment 14 2012-02-22 14:10:23 PST
Comment on attachment 128243 [details] Patch for review after bots cycle. r=me.
WebKit Review Bot
Comment 15 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>
WebKit Review Bot
Comment 16 2012-02-22 15:17:38 PST
All reviewed patches have been landed. Closing bug.
Nikolas Zimmermann
Comment 17 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.
Nikolas Zimmermann
Comment 18 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.
Nikolas Zimmermann
Comment 19 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..
Nikolas Zimmermann
Comment 20 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.
Nikolas Zimmermann
Comment 21 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.
Nikolas Zimmermann
Comment 22 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.
Nikolas Zimmermann
Comment 23 2012-02-24 06:56:33 PST
Closing this bug again, will move further discussion into the bug which is guilty for the failure.
Nikolas Zimmermann
Comment 24 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)
Nikolas Zimmermann
Comment 25 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.
Stephen Chenney
Comment 26 2012-04-17 12:32:05 PDT
Note You need to log in before you can comment on or make changes to this bug.