Bug 5793

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:

Version:

420+

Hardware:

Mac

OS:

OS X 10.3

Bug Depends on:

Bug Blocks:

4911

Attachments:

Description	Flags
Testcase	none
Updated test case	none
updated test case	none
test case with data URL (ok)	none
Proposed path.	ddkilzer: review-
Proposed patch without source code.	ddkilzer: review-
Proposed patch with accepting reviewer's comment.	ddkilzer: review-
Test case showing three SVG images referenced in object tags that do not have explicit widths/heights.	none
Patch for Object and SVG size issue.	eric: review-
Proposed patch-with code refined	none

Simon Pieters (:zcorpan)

Reported 2005-11-21 10:25:39 PST

If a raster image is referenced by an OBJECT element, and the width or height attributes are not set, then the OBJECT should honor the actual width and height of the image (just like the IMG element type). Steps to reproduce: 1. Open data:text/html,<!DOCTYPE html><p>The following two images should be identical:<p><img alt="" src="http://www.opendarwin.org/images/hexley-32-trans.png"><p><object data="http://www.opendarwin.org/images/hexley-32-trans.png"></object> 2. Compare the images 3. Expected results: The images should look the same. Actual results: The OBJECT doesn't honor the size of the image

Attachments
Testcase (231 bytes, text/html) 2005-11-22 13:04 PST, Joost de Valk (AlthA)	no flags	Details
Updated test case (217 bytes, text/html) 2006-05-22 20:57 PDT, David Carson	no flags	Details
updated test case (202 bytes, text/html) 2008-03-14 05:27 PDT, Robert Blaut	no flags	Details
test case with data URL (ok) (2.56 KB, text/html) 2008-03-24 12:02 PDT, Robert Blaut	no flags	Details
Proposed path. (996.54 KB, application/zip) 2010-10-27 05:21 PDT, Julie Jeongeun Kim	ddkilzer: review-	Details
Proposed patch without source code. (5.13 KB, patch) 2010-10-27 07:11 PDT, Julie Jeongeun Kim	ddkilzer: review-	Details Formatted Diff Diff
Proposed patch with accepting reviewer's comment. (5.34 KB, patch) 2010-10-27 10:22 PDT, Julie Jeongeun Kim	ddkilzer: review-	Details Formatted Diff Diff
Test case showing three SVG images referenced in object tags that do not have explicit widths/heights. (1.04 KB, text/html) 2010-10-27 10:53 PDT, Jeff Schiller	no flags	Details
Patch for Object and SVG size issue. (9.29 KB, patch) 2010-11-01 04:56 PDT, Julie Jeongeun Kim	eric: review-	Details Formatted Diff Diff
Proposed patch-with code refined (9.23 KB, patch) 2010-12-05 20:18 PST, Julie Jeongeun Kim	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Joost de Valk (AlthA)

Comment 1 2005-11-22 13:03:17 PST

Confirmed, adding testcase based on code above in a few secs.

Joost de Valk (AlthA)

Comment 2 2005-11-22 13:04:30 PST

Created attachment 4769 [details] Testcase minimal testcase.

Eric Seidel (no email)

Comment 3 2005-12-28 00:58:40 PST

This should be an easy fix.

Eric Seidel (no email)

Comment 4 2005-12-28 01:02:14 PST

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.

Joost de Valk (AlthA)

Comment 5 2006-02-15 02:24:27 PST

Adding keyword per Eric's remark.

David Carson

Comment 6 2006-05-22 20:57:46 PDT

Created attachment 8472 [details] Updated test case Updated the test case as the image used in the test case are no longer there.

Alexey Proskuryakov

Comment 7 2006-05-23 03:38:05 PDT

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.

Robert Blaut

Comment 8 2008-03-14 05:27:03 PDT

Created attachment 19760 [details] updated test case

Robert Blaut

Comment 9 2008-03-14 05:28:29 PDT

This bug affects also Acid2 (no data) test suite. See bug 4911 for more details.

Robert Blaut

Comment 10 2008-03-24 12:02:18 PDT

Created attachment 20012 [details] test case with data URL (ok)

Jeff Schiller

Comment 11 2010-09-17 16:29:32 PDT

Looks like this bug has been fixed for rasters. Can someone point me to a revision that fixed it?

David Kilzer (:ddkilzer)

Comment 12 2010-09-19 20:32:04 PDT

(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.

Julie Jeongeun Kim

Comment 13 2010-10-27 05:21:51 PDT

Created attachment 72016 [details] Proposed path.

WebKit Review Bot

Comment 14 2010-10-27 05:25:15 PDT

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.

David Kilzer (:ddkilzer)

Comment 15 2010-10-27 06:46:52 PDT

(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?

David Kilzer (:ddkilzer)

Comment 16 2010-10-27 06:49:59 PDT

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.

Julie Jeongeun Kim

Comment 17 2010-10-27 07:11:56 PDT

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.

David Kilzer (:ddkilzer)

Comment 18 2010-10-27 07:42:31 PDT

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))

Julie Jeongeun Kim

Comment 19 2010-10-27 08:07:26 PDT

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

Julie Jeongeun Kim

Comment 20 2010-10-27 10:22:22 PDT

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,

Jeff Schiller

Comment 21 2010-10-27 10:52:28 PDT

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.

Jeff Schiller

Comment 22 2010-10-27 10:53:19 PDT

Created attachment 72056 [details] Test case showing three SVG images referenced in object tags that do not have explicit widths/heights.

David Kilzer (:ddkilzer)

Comment 23 2010-10-27 12:43:37 PDT

(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.

David Kilzer (:ddkilzer)

Comment 24 2010-10-27 12:51:07 PDT

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();

Jeff Schiller

Comment 25 2010-10-27 14:03:16 PDT

@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.

Alexey Proskuryakov

Comment 26 2010-10-27 21:43:32 PDT

See also: bug 11976.

Julie Jeongeun Kim

Comment 27 2010-10-29 10:46:12 PDT

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)

Robert Blaut

Comment 28 2010-10-29 10:59:57 PDT

(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 ?

Jeff Schiller

Comment 29 2010-10-29 11:04:55 PDT

Um, isn't that what I attached here: https://bugs.webkit.org/attachment.cgi?id=72056 way back in Comment #22 ?

Julie Jeongeun Kim

Comment 30 2010-10-29 11:09:38 PDT

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 ?

Julie Jeongeun Kim

Comment 31 2010-11-01 04:56:34 PDT

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.

Eric Seidel (no email)

Comment 32 2010-12-03 10:36:22 PST

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.

Julie Jeongeun Kim

Comment 33 2010-12-05 20:18:48 PST

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.

Adam Barth

Comment 34 2011-04-26 15:42:13 PDT

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.)

Adam Barth

Comment 35 2011-04-26 15:42:36 PDT

This bug was fixed long ago.

Alexey Proskuryakov

Comment 36 2011-04-26 16:04:06 PDT

We likely have a bug for SVG already (maybe bug 12095?)

Note You need to log in before you can comment on or make changes to this bug.