| 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: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Yong Li
2012-03-06 10:57:36 PST
Created attachment 130424 [details]
the proposed patch
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?
Comment on attachment 130424 [details]
the proposed patch
Looks like the right thing even as per the original comment.
(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. Comment on attachment 130424 [details] the proposed patch Clearing flags on attachment: 130424 Committed r110091: <http://trac.webkit.org/changeset/110091> All reviewed patches have been landed. Closing bug. (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. Created attachment 130827 [details]
Test case
(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. Comment on attachment 130827 [details]
Test case
seems I should use git diff --binary
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
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. (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 Created attachment 131059 [details]
Test case
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. 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/ Created attachment 137374 [details]
test case to commit
Comment on attachment 137374 [details] test case to commit Clearing flags on attachment: 137374 Committed r114290: <http://trac.webkit.org/changeset/114290> |