| Summary: | HTML OBJECT without width/height attributes doesn't honor the size of the image | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Pieters (:zcorpan) <zcorpan> | ||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, aestes, ap, dacarson, ddkilzer, eric, ian, jeffschiller, jiyuluna, rwlbuis, sam, webkit, webkit.review.bot, zimmermann | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 420+ | ||||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||||
| OS: | OS X 10.3 | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 4911 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Simon Pieters (:zcorpan)
2005-11-21 10:25:39 PST
Confirmed, adding testcase based on code above in a few secs. Created attachment 4769 [details]
Testcase
minimal testcase.
This should be an easy fix. The code would be in HTMLObjectElementImpl. Somehow it must be setting the wrong width/height on the created RenderImage object. A quick comparsison of HTMLImageElementImpl and HTMLObjectElmentImpl or setting a breakpoint on setWidth setHeight on RenderImage should make the bug clear. Adding keyword per Eric's remark. Created attachment 8472 [details]
Updated test case
Updated the test case as the image used in the test case are no longer there.
From an IRC discussion with Rob Buis: HTMLObjectElmentImpl treats its content as either an image or a part based on type attribute (so, the workaround is to add a type="image/png" attribute). Reworking this doesn't sound like a particularly easy fix; I'm removing the keyword. See also: bug 8950. Created attachment 19760 [details]
updated test case
This bug affects also Acid2 (no data) test suite. See bug 4911 for more details. Created attachment 20012 [details]
test case with data URL (ok)
Looks like this bug has been fixed for rasters. Can someone point me to a revision that fixed it? (In reply to comment #11) > Looks like this bug has been fixed for rasters. Can someone point me to a revision that fixed it? Attachment 19760 [details] doesn't work, but Attachment 20012 [details] does. This should not be closed until both work. Created attachment 72016 [details]
Proposed path.
Attachment 72016 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #13) > Created an attachment (id=72016) [details] > Proposed path. This is not a plain text patch. Is it a zipped archive or a gzipped archive? Comment on attachment 72016 [details]
Proposed path.
Please use WebKitTools/Scripts/svn-create-patch to create a patch, then attach it to the bug.
Uploading whole source files with changes in a zip archive is not an acceptable format.
Created attachment 72031 [details]
Proposed patch without source code.
Sorry that I make you confused.
I added new patch with some correction.
Please check the '5793objectSize_2nd.txt'.
If I have wrong, please let me know.
Thanks.
Comment on attachment 72031 [details] Proposed patch without source code. View in context: https://bugs.webkit.org/attachment.cgi?id=72031&action=review Overall, this patch looks good, but r- to run layout tests and/or write a new layout test, remove debugging code, and fix the ChangeLog patch. > WebCore/ChangeLog:1 > -2010-10-27 Adam Roben <aroben@apple.com> > +2010-10-27 Julie-Jeongeun-Kim <jiyuluna@gmail.com> Please don't delete existing ChangeLog entries. Your new changelog entry should simply appear before the other ones. For the most part, your new changelog entry looks good. Just a couple nits below. > WebCore/ChangeLog:10 > + It should let its owner know the real size so that OBJECT is re-layouted. Let's use "is laid out again." instead of "is re-layouted." > WebCore/ChangeLog:12 > + No new tests. (OOPS!) Did you try to write a test case for this change? Did you run layout tests to see if any existing layout test results changed? > WebCore/html/ImageDocument.cpp:46 > +#include <stdio.h> This is probably left over from debugging. Please remove it. > WebCore/html/ImageDocument.cpp:301 > + IntSize windowSize = IntSize(view->width(), view->height()); Is there a FrameView::size() method that returns an IntSize? > WebCore/html/ImageDocument.cpp:310 > + RenderEmbeddedObject* rendererEmbedded = toRenderEmbeddedObject(renderer); > + > + if (rendererEmbedded) These lines can be combined: if (RenderEmbeddedObject* rendererEmbedded = toRenderEmbeddedObject(renderer)) Thank you for your time for review. I have some questions. (In reply to comment #18) .... > > WebCore/ChangeLog:12 > > + No new tests. (OOPS!) > > Did you try to write a test case for this change? If I need to create new test case for it, I think I can use this attached test case on this issue. Is it acceptable? > Did you run layout tests to see if any existing layout test results changed? I run 'run-webkit-tests' script with this patch. There was no problem. Is it not enough? Do I need any other test? ... > > WebCore/html/ImageDocument.cpp:301 > > + IntSize windowSize = IntSize(view->width(), view->height()); > > Is there a FrameView::size() method that returns an IntSize? I followed 'ImageDocument::imageFitsInWindow()'. It uses 'IntSize windowSize = IntSize(view->width(), view->height());'. Do I need to change it to 'FrameView::size()'? Please check. Thanks, Julie Created attachment 72053 [details]
Proposed patch with accepting reviewer's comment.
I changed code additionally.
1.Removed code for debugging.
2.Kept 'IntSize windowSize = IntSize(view->width(), view->height());' but I added null check, as other APIs in ImageDocument did.
3.condition and declaration were combined.
As to new test case,
I think the test case attached here is suitable.
Please let me know how to apply it.
* I've done other tests with 'run-webkit-tests'.
Thanks,
I applied your patch and tried the attached test case. It failed to properly render either of the three SVG images. In the first case (blue circle), since the SVG has no specified width/height, they default to 100%/100%. What other browser do (Firefox, Opera) is set the width to the full width of the browser window and the height to 150px. I'm not sure if this is correct by CSS sizing rules, but it seems consistent across those two browsers. In the second case (green circle), the SVG has a specific size of width="500" and height="500". What other browsers do is set the object frame to be the size of the image (500px by 500px). In the third case, (purple circle), the SVG has a specific size of width="500px" and height="500px". All browsers treat this case identically to the second case. You can right-click on the object frames and view source of each one to see the base64-decoded SVG doc (I made the data: URIs so that I can just attach one test case and everyone can see it without downloading files). I think fixing this bug should at least fix the second and third cases, since the SVG has an explicit size. Created attachment 72056 [details]
Test case showing three SVG images referenced in object tags that do not have explicit widths/heights.
(In reply to comment #19) > Thank you for your time for review. > I have some questions. > > (In reply to comment #18) > .... > > > WebCore/ChangeLog:12 > > > + No new tests. (OOPS!) > > > > Did you try to write a test case for this change? > If I need to create new test case for it, I think I can use this attached test case on this issue. > Is it acceptable? The attached test case needs to be turned into a layout test (under the LayoutTests directory structure) and the test and its results need to be part of the patch. > > Did you run layout tests to see if any existing layout test results changed? > I run 'run-webkit-tests' script with this patch. There was no problem. > Is it not enough? Do I need any other test? Nope, that's good. I didn't see anywhere that you had mentioned doing it, which is why I asked. > > > WebCore/html/ImageDocument.cpp:301 > > > + IntSize windowSize = IntSize(view->width(), view->height()); > > > > Is there a FrameView::size() method that returns an IntSize? > I followed 'ImageDocument::imageFitsInWindow()'. It uses 'IntSize windowSize = IntSize(view->width(), view->height());'. > Do I need to change it to 'FrameView::size()'? Using view->size() would be slightly less work. I would change it. Comment on attachment 72053 [details] Proposed patch with accepting reviewer's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=72053&action=review Making progress! r- to fix the ChangeLog patch, to use view->size(), and to include a layout test. This page (under the "Regression tests" section) talks about how to make a layout test: <http://webkit.org/coding/contributing.html> It's probably just as easy to look at existing layout tests to figure out how to write one. Note that I haven't reviewed this patch for correctness of behavior yet--just getting through some of the basic issues first. Comment #21 concerns me, though, in that these changes don't fix the test cases attached to the bug. > WebCore/ChangeLog:-106 > -2010-10-27 Adam Roben <aroben@apple.com> The ChangeLog patch is still deleting old ChangeLog entries. There should only be the new ChangeLog added to the top of the file. > WebCore/html/ImageDocument.cpp:300 > + IntSize windowSize = IntSize(view->width(), view->height()); Please change to: IntSize windowSize = view->size(); @ddkilzer: I should be clear that the test I refer to in Comment #21 was attached in Comment #22, it does not refer to earlier test cases that were attached prior to my comment. If the SVG issues need to be fixed outside this bug, that's fine, but in case they can be fixed here I'd prefer it. See also: bug 11976. 1.I tried to make a test case for this issue without network request. But this issue doesn't happen if I make it with local object file. So, it's hard to make the test case for this issue. If you have any idea for it, please let me know. 2.I'm trying to fix SVG problem as well. I'll attach the new patch soon. (In reply to comment #24) (In reply to comment #27) > 1.I tried to make a test case for this issue without network request. > But this issue doesn't happen if I make it with local object file. > So, it's hard to make the test case for this issue. > If you have any idea for it, please let me know. What about data URL: http://software.hixie.ch/utilities/cgi/data/data ? Um, isn't that what I attached here: https://bugs.webkit.org/attachment.cgi?id=72056 way back in Comment #22 ? Sorry that I made you confused. I mean the test case for 'run-webkit-tests' script related to this patch. I'm using your attached test case to fix SVG problem now. Thanks. (In reply to comment #29) > Um, isn't that what I attached here: https://bugs.webkit.org/attachment.cgi?id=72056 way back in Comment #22 ? Created attachment 72495 [details]
Patch for Object and SVG size issue.
It includes previous patch and new patch for SVG issue.
The test case for the first issue, OBJECT size issue, generated with local file, data attribute or localhost can't reproduce this issue.
I just added a new test case for SVG.
If you have any problem with this patch, please let me know.
Thanks.
Comment on attachment 72495 [details] Patch for Object and SVG size issue. View in context: https://bugs.webkit.org/attachment.cgi?id=72495&action=review I'm glad you're taking a look at this super-old bug. I think we need another round here. > WebCore/html/ImageDocument.cpp:312 > + if (FrameView* view = frame()->view()) { > + IntSize windowSize = IntSize(view->width(), view->height()); > > + if (imageSize != windowSize) { > + if (HTMLFrameOwnerElement* ownerElement = frame()->ownerElement()) { > + RenderObject* renderer = ownerElement->renderer(); > + > + if (renderer && renderer->isEmbeddedObject()) { > + if (RenderEmbeddedObject* r = toRenderEmbeddedObject(renderer)) > + r->updateIntrinsicSize(imageSize); > + } > + } > + } > + } Seems this block may make sense as a helper function instead of inlined here. (for easier readability and easy use of early-return). > WebCore/rendering/RenderSVGRoot.cpp:335 > +void RenderSVGRoot::updateOwnerSize() > +{ > + if (HTMLFrameOwnerElement* ownerElement = node()->document()->ownerElement()) { > + RenderObject* render = ownerElement->renderer(); > + > + if (render && render->isEmbeddedObject()) { > + if (RenderEmbeddedObject* r = toRenderEmbeddedObject(render)) { > + IntSize size = IntSize(width(), height()); > + r->updateIntrinsicSize(size); > + } > + } > + } Generally we prefer early return to long indented if blocks. I'm not sure why teh "size" local is useful. > LayoutTests/svg/custom/svg-without-size-attr.html:36 > + <object id="svg1" type='image/svg+xml' data='data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiA/Pjxzdmcgd2lkdGg9IjUwMCIgaGVpZ2h0PSI1MDAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PGNpcmNsZSBjeD0iMjUwIiBjeT0iMjUwIiByPSIyMDAiIGZpbGw9ImdyZWVuIiAvPjwvc3ZnPg=='></object> Seems a little odd to use the same data url twice. I guess I would have just added a resources/test-name.svg file isntead. Created attachment 75642 [details]
Proposed patch-with code refined
I changed code a little according to reviewer's comment.
As to new test case,
I added one object tag to html file.
The issue just happens with SVG images referenced in object tags that do not have explicit widths/heights.
I can't replace html file with SVG file.
Please look into the updated patch.
Thanks.
Comment on attachment 75642 [details]
Proposed patch-with code refined
This patch is plausible, but has been filed against the wrong bug. The original bug reported here has been fixed. The only bug remaining appears to be for SVGImages. I'm going to close this bug, but please feel free to file a new bug and attack this patch to that bug. (Sorry if that feels like busywork.)
This bug was fixed long ago. We likely have a bug for SVG already (maybe bug 12095?) |