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: | SVG | Assignee: | 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: |
|
Created attachment 126990 [details]
Screenshot
Added screenshot of reproduction.
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 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 Created attachment 127846 [details]
Patch for EWS bots. Do not review.
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 Created attachment 127927 [details]
Patch for EWS bots. Do not review.
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 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 Created attachment 128054 [details]
Patch for review.
Updated the chromium test_expectations.txt file properly to pass cr-linux.
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 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. 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 on attachment 128243 [details]
Patch for review after bots cycle.
Passed cr-linux. Setting r?, cq?.
Comment on attachment 128243 [details]
Patch for review after bots cycle.
r=me.
Comment on attachment 128243 [details] Patch for review after bots cycle. Clearing flags on attachment: 128243 Committed r108559: <http://trac.webkit.org/changeset/108559> All reviewed patches have been landed. Closing bug. 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. (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. 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.. (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. (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. (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. Closing this bug again, will move further discussion into the bug which is guilty for the failure. (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) Oops, ignore my last comment, was a left-over. It's indeed the aforementioned <table> refactorization, which broke the test, not this bug. Committed r114415: <http://trac.webkit.org/changeset/114415> |
Created attachment 126989 [details] Reproduction Please see the attached reproduction for an example. Note that Opera renders the reproduction correctly.