Bug 5793 - HTML OBJECT without width/height attributes doesn't honor the size of the image
: HTML OBJECT without width/height attributes doesn't honor the size of the image
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 420+
: Macintosh Mac OS X 10.3
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 4911
  Show dependency treegraph
 
Reported: 2005-11-21 10:25 PST by Simon Pieters
Modified: 2011-04-26 16:04 PDT (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pieters 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
Comment 1 Joost de Valk (AlthA) 2005-11-22 13:03:17 PST
Confirmed, adding testcase based on code above in a few secs.
Comment 2 Joost de Valk (AlthA) 2005-11-22 13:04:30 PST
Created attachment 4769 [details]
Testcase

minimal testcase.
Comment 3 Eric Seidel 2005-12-28 00:58:40 PST
This should be an easy fix.
Comment 4 Eric Seidel 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.
Comment 5 Joost de Valk (AlthA) 2006-02-15 02:24:27 PST
Adding keyword per Eric's remark.
Comment 6 David Carson 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.
Comment 7 Alexey Proskuryakov 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.
Comment 8 Robert Blaut 2008-03-14 05:27:03 PDT
Created attachment 19760 [details]
updated test case
Comment 9 Robert Blaut 2008-03-14 05:28:29 PDT
This bug affects also Acid2 (no data) test suite. See bug 4911 for more details.
Comment 10 Robert Blaut 2008-03-24 12:02:18 PDT
Created attachment 20012 [details]
test case with data URL (ok)
Comment 11 Jeff Schiller 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?
Comment 12 David Kilzer (:ddkilzer) 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.
Comment 13 Julie Jeongeun Kim 2010-10-27 05:21:51 PDT
Created attachment 72016 [details]
Proposed path.
Comment 14 WebKit Review Bot 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.
Comment 15 David Kilzer (:ddkilzer) 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?
Comment 16 David Kilzer (:ddkilzer) 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.
Comment 17 Julie Jeongeun Kim 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.
Comment 18 David Kilzer (:ddkilzer) 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))
Comment 19 Julie Jeongeun Kim 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
Comment 20 Julie Jeongeun Kim 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,
Comment 21 Jeff Schiller 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.
Comment 22 Jeff Schiller 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.
Comment 23 David Kilzer (:ddkilzer) 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.
Comment 24 David Kilzer (:ddkilzer) 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();
Comment 25 Jeff Schiller 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.
Comment 26 Alexey Proskuryakov 2010-10-27 21:43:32 PDT
See also: bug 11976.
Comment 27 Julie Jeongeun Kim 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)
Comment 28 Robert Blaut 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 ?
Comment 29 Jeff Schiller 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 ?
Comment 30 Julie Jeongeun Kim 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 ?
Comment 31 Julie Jeongeun Kim 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.
Comment 32 Eric Seidel 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.
Comment 33 Julie Jeongeun Kim 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.
Comment 34 Adam Barth 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.)
Comment 35 Adam Barth 2011-04-26 15:42:36 PDT
This bug was fixed long ago.
Comment 36 Alexey Proskuryakov 2011-04-26 16:04:06 PDT
We likely have a bug for SVG already (maybe bug 12095?)