RESOLVED FIXED 80431
RenderImage ignores its percent width/height when setContainerSizeForRenderer 
https://bugs.webkit.org/show_bug.cgi?id=80431
Summary RenderImage ignores its percent width/height when setContainerSizeForRenderer
Yong Li
Reported 2012-03-06 10:57:36 PST
<img width=50% height=50% src=sample.svg/> The percent width/height is ignored and it ends up with using the width/height defined in the svg source to allocate image buffers.
Attachments
the proposed patch (1.81 KB, patch)
2012-03-06 11:52 PST, Yong Li 		no flags
DetailsFormatted DiffDiff
Test case (3.12 KB, patch)
2012-03-08 08:38 PST, Yong Li 		no flags
DetailsFormatted DiffDiff
Test case again (9.02 KB, patch)
2012-03-08 08:48 PST, Yong Li 		zimmermann: review-
DetailsFormatted DiffDiff
Test case (2.85 KB, patch)
2012-03-09 10:33 PST, Yong Li 		zimmermann: review+
DetailsFormatted DiffDiff
test case to commit (2.79 KB, patch)
2012-04-16 12:20 PDT, Yong Li 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Yong Li
Comment 1 2012-03-06 11:52:26 PST
Created attachment 130424 [details] the proposed patch
Nikolas Zimmermann
Comment 2 2012-03-07 00:47:20 PST
Comment on attachment 130424 [details] the proposed patch Hm, its kinda weird that this doesn't affect tests - I would have expected that you could construct a testcase where this matters. How about using a svg file with small intrinsic dimensions <svg width="1" height="1"> and embedding that in an <img width="50%"... which is wrapped in a div with eg. 400px. Can you try to come up with a test?
George Staikos
Comment 3 2012-03-07 06:44:13 PST
Comment on attachment 130424 [details] the proposed patch Looks like the right thing even as per the original comment.
Yong Li
Comment 4 2012-03-07 11:59:22 PST
(In reply to comment #2) > (From update of attachment 130424 [details]) > Hm, its kinda weird that this doesn't affect tests - I would have expected that you could construct a testcase where this matters. > How about using a svg file with small intrinsic dimensions <svg width="1" height="1"> and embedding that in an <img width="50%"... which is wrapped in a div with eg. 400px. Can you try to come up with a test? I'll try to create a test case with this approach.
WebKit Review Bot
Comment 5 2012-03-07 13:02:59 PST
Comment on attachment 130424 [details] the proposed patch Clearing flags on attachment: 130424 Committed r110091: <http://trac.webkit.org/changeset/110091>
WebKit Review Bot
Comment 6 2012-03-07 13:03:03 PST
All reviewed patches have been landed. Closing bug.
Nikolas Zimmermann
Comment 7 2012-03-08 00:49:20 PST
(In reply to comment #3) > (From update of attachment 130424 [details]) > Looks like the right thing even as per the original comment. This shouldn't have gone in w/o a new test though - it's just likely to break again :( Reopening until a test is created & landed.
Yong Li
Comment 8 2012-03-08 08:38:58 PST
Created attachment 130827 [details] Test case
Yong Li
Comment 9 2012-03-08 08:39:52 PST
(In reply to comment #7) > (In reply to comment #3) > > (From update of attachment 130424 [details] [details]) > > Looks like the right thing even as per the original comment. > This shouldn't have gone in w/o a new test though - it's just likely to break again :( > Reopening until a test is created & landed. Sorry for that. It took me some time to get the pixel result for the first time.
Yong Li
Comment 10 2012-03-08 08:45:21 PST
Comment on attachment 130827 [details] Test case seems I should use git diff --binary
Yong Li
Comment 11 2012-03-08 08:48:58 PST
Created attachment 130829 [details] Test case again I verified with latest Qt port. Without the code change, it doesn't show the circle but just fill the area with fuzzy color. Google Chrome also shows the issue. With the code change, it can show the circle as what FireFox does
Nikolas Zimmermann
Comment 12 2012-03-09 02:50:32 PST
Comment on attachment 130829 [details] Test case again View in context: https://bugs.webkit.org/attachment.cgi?id=130829&action=review Thanks Yong! Needs another iteration: > LayoutTests/svg/as-image/resources/circle-1x1.svg:5 > + <circle cx="50%" cy="50%" r="0.3" style="fill:blue;" /> Please make it green, we don't land non-green circles/rect if not necessary :-) > LayoutTests/svg/as-image/svg-as-image-with-relative-size.html:9 > + <div width=400px height=300px> This doesn't work for <divs>. I've corrected your testcase and included some borders to make it easier to see: <html> <head> <title>svg-as-image-with-relative-size</title> <style> body {margin: 0px;} </style> </head> <body> <div style="width: 400px; height: 300px; border: 1px solid black;"> <img width=50% height=50% style="border: 1px solid green;" src='resources/circle-1x1.svg' /> </body> </html> Note that your testcase is _perfectly_ suited for a reftest. Please create a svg-as-image-with-reliave-size-expected.html, that contains a standalone <svg> embedded in a <html> modelled to look like the expected image - this will also avoid platform specific results. > LayoutTests/svg/as-image/svg-as-image-with-relative-size.html:11 > + <!-- The following svg has a width of 1px and a height of 1px and draw a circle inside. > + The circle should still be painted on the page (but the shape is actually oval). --> The shape is oval? For me it looks like a regular circle.
Yong Li
Comment 13 2012-03-09 08:41:26 PST
(In reply to comment #12) > (From update of attachment 130829 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130829&action=review > > Thanks Yong! Needs another iteration: > > > LayoutTests/svg/as-image/resources/circle-1x1.svg:5 > > + <circle cx="50%" cy="50%" r="0.3" style="fill:blue;" /> > > Please make it green, we don't land non-green circles/rect if not necessary :-) I copied it from circle.svg. Will change to green rect as I think probably circle is probably not always painted in the same way by different graphics engines. > > > LayoutTests/svg/as-image/svg-as-image-with-relative-size.html:9 > > + <div width=400px height=300px> > > This doesn't work for <divs>. I've corrected your testcase and included some borders to make it easier to see: > > <html> > <head> > <title>svg-as-image-with-relative-size</title> > <style> > body {margin: 0px;} > </style> > </head> > <body> > <div style="width: 400px; height: 300px; border: 1px solid black;"> > <img width=50% height=50% style="border: 1px solid green;" src='resources/circle-1x1.svg' /> > </body> > </html> Thanks! > > Note that your testcase is _perfectly_ suited for a reftest. Please create a svg-as-image-with-reliave-size-expected.html, that contains a standalone <svg> embedded in a <html> modelled to look like the expected image - this will also avoid platform specific results. Good to know "reftest" :) > > > LayoutTests/svg/as-image/svg-as-image-with-relative-size.html:11 > > + <!-- The following svg has a width of 1px and a height of 1px and draw a circle inside. > > + The circle should still be painted on the page (but the shape is actually oval). --> > > The shape is oval? For me it looks like a regular circle. Probably because the width/height in div doesn't work
Yong Li
Comment 14 2012-03-09 10:33:28 PST
Created attachment 131059 [details] Test case
Rob Buis
Comment 15 2012-04-05 13:26:28 PDT
Comment on attachment 131059 [details] Test case View in context: https://bugs.webkit.org/attachment.cgi?id=131059&action=review This looks good to me. But I think WildFox should have the final verdict since he commented a lot :) > LayoutTests/svg/as-image/resources/rect-1x1.svg:3 > + "http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd"> I don't think the DOCTYPE line is needed at all.
Nikolas Zimmermann
Comment 16 2012-04-07 01:25:01 PDT
Comment on attachment 131059 [details] Test case View in context: https://bugs.webkit.org/attachment.cgi?id=131059&action=review r=me. I forgot to write down here that the bug wasn't really fixed with Yongs patch. It turns out it was much more complex, to get it right, see bug 81631. This combined with Yong patch fixes the problem for real, still his test is good and should be landed. > LayoutTests/svg/as-image/svg-as-image-with-relative-size-expected.html:10 > + <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" height="100%" width="100%"> All attributes except xmlns are unnecessary here, please remove them. > LayoutTests/svg/as-image/svg-as-image-with-relative-size.html:10 > + <!-- The following svg has a width of 1px and a height of 1px and draw a rect inside. s/draw/draws/
Yong Li
Comment 17 2012-04-16 12:20:24 PDT
Created attachment 137374 [details] test case to commit
WebKit Review Bot
Comment 18 2012-04-16 13:08:22 PDT
Comment on attachment 137374 [details] test case to commit Clearing flags on attachment: 137374 Committed r114290: <http://trac.webkit.org/changeset/114290>
Note You need to log in before you can comment on or make changes to this bug.