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
: WebKit
Layout and Rendering
: 420+
: Macintosh Mac OS X 10.3
: P2 Normal
Assigned To:
:
:
:
: 4911
  Show dependency treegraph
 
Reported: 2005-11-21 10:25 PST by
Modified: 2011-04-26 16:04 PST (History)


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 PST, David Carson
no flags Details
updated test case (202 bytes, text/html)
2008-03-14 05:27 PST, Robert Blaut
no flags Details
test case with data URL (ok) (2.56 KB, text/html)
2008-03-24 12:02 PST, Robert Blaut
no flags Details
Proposed path. (996.54 KB, application/zip)
2010-10-27 05:21 PST, Julie Jeongeun Kim
ddkilzer: review-
Details
Proposed patch without source code. (5.13 KB, patch)
2010-10-27 07:11 PST, Julie Jeongeun Kim
ddkilzer: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch with accepting reviewer's comment. (5.34 KB, patch)
2010-10-27 10:22 PST, Julie Jeongeun Kim
ddkilzer: review-
Review Patch | 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 PST, Jeff Schiller
no flags Details
Patch for Object and SVG size issue. (9.29 KB, patch)
2010-11-01 04:56 PST, Julie Jeongeun Kim
eric: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch-with code refined (9.23 KB, patch)
2010-12-05 20:18 PST, Julie Jeongeun Kim
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 2005-11-22 13:03:17 PST -------
Confirmed, adding testcase based on code above in a few secs.
------- Comment #2 From 2005-11-22 13:04:30 PST -------
Created an attachment (id=4769) [details]
Testcase

minimal testcase.
------- Comment #3 From 2005-12-28 00:58:40 PST -------
This should be an easy fix.
------- Comment #4 From 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 From 2006-02-15 02:24:27 PST -------
Adding keyword per Eric's remark.
------- Comment #6 From 2006-05-22 20:57:46 PST -------
Created an attachment (id=8472) [details]
Updated test case

Updated the test case as the image used in the test case are no longer there.
------- Comment #7 From 2006-05-23 03:38:05 PST -------
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 From 2008-03-14 05:27:03 PST -------
Created an attachment (id=19760) [details]
updated test case
------- Comment #9 From 2008-03-14 05:28:29 PST -------
This bug affects also Acid2 (no data) test suite. See bug 4911 for more details.
------- Comment #10 From 2008-03-24 12:02:18 PST -------
Created an attachment (id=20012) [details]
test case with data URL (ok)
------- Comment #11 From 2010-09-17 16:29:32 PST -------
Looks like this bug has been fixed for rasters.  Can someone point me to a revision that fixed it?
------- Comment #12 From 2010-09-19 20:32:04 PST -------
(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 From 2010-10-27 05:21:51 PST -------
Created an attachment (id=72016) [details]
Proposed path.
------- Comment #14 From 2010-10-27 05:25:15 PST -------
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 From 2010-10-27 06:46:52 PST -------
(In reply to comment #13)
> Created an attachment (id=72016) [details] [details]
> Proposed path.

This is not a plain text patch.  Is it a zipped archive or a gzipped archive?
------- Comment #16 From 2010-10-27 06:49:59 PST -------
(From update of attachment 72016 [details])
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 From 2010-10-27 07:11:56 PST -------
Created an attachment (id=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 From 2010-10-27 07:42:31 PST -------
(From update of attachment 72031 [details])
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 From 2010-10-27 08:07:26 PST -------
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 From 2010-10-27 10:22:22 PST -------
Created an attachment (id=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 From 2010-10-27 10:52:28 PST -------
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 From 2010-10-27 10:53:19 PST -------
Created an attachment (id=72056) [details]
Test case showing three SVG images referenced in object tags that do not have explicit widths/heights.
------- Comment #23 From 2010-10-27 12:43:37 PST -------
(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 From 2010-10-27 12:51:07 PST -------
(From update of attachment 72053 [details])
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 From 2010-10-27 14:03:16 PST -------
@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 From 2010-10-27 21:43:32 PST -------
See also: bug 11976.
------- Comment #27 From 2010-10-29 10:46:12 PST -------
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 From 2010-10-29 10:59:57 PST -------
(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 From 2010-10-29 11:04:55 PST -------
Um, isn't that what I attached here: https://bugs.webkit.org/attachment.cgi?id=72056 way back in Comment #22 ?
------- Comment #30 From 2010-10-29 11:09:38 PST -------
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 From 2010-11-01 04:56:34 PST -------
Created an attachment (id=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 From 2010-12-03 10:36:22 PST -------
(From update of attachment 72495 [details])
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 From 2010-12-05 20:18:48 PST -------
Created an attachment (id=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 From 2011-04-26 15:42:13 PST -------
(From update of attachment 75642 [details])
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 From 2011-04-26 15:42:36 PST -------
This bug was fixed long ago.
------- Comment #36 From 2011-04-26 16:04:06 PST -------
We likely have a bug for SVG already (maybe bug 12095?)