Bug 80431

Summary:

RenderImage ignores its percent width/height when setContainerSizeForRenderer

Product:

WebKit

Reporter:

Yong Li <yong.li.webkit>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
the proposed patch	none
Test case	none
Test case again	zimmermann: review-
Test case	zimmermann: review+
test case to commit	none

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	Details Formatted Diff Diff
Test case (3.12 KB, patch) 2012-03-08 08:38 PST, Yong Li	no flags	Details Formatted Diff Diff
Test case again (9.02 KB, patch) 2012-03-08 08:48 PST, Yong Li	zimmermann: review-	Details Formatted Diff Diff
Test case (2.85 KB, patch) 2012-03-09 10:33 PST, Yong Li	zimmermann: review+	Details Formatted Diff Diff
test case to commit (2.79 KB, patch) 2012-04-16 12:20 PDT, Yong Li	no flags	Details Formatted Diff Diff
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 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 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.