Bug 62769

Summary: REGRESSION (r88913): <object> has wrong computed height
Product: WebKit Reporter: Lei Zhang <thestig>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: adman.com, dglazkov, felash, hyatt, jamesr, mihaip, senorblanco, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
screenshot of the problem none

Description Lei Zhang 2011-06-15 19:29:46 PDT
Created attachment 97388 [details]
test case

After r88913, I'm seeing a regression where an <object> with height=100% no longer having a height of 100%. Instead, the height is "100%", but it's computed to be 150 px.

I tested with the attached html file. You may also need a PDF viewer and a dummy test.pdf file to actually display the object.

In the attached screenshot, the left copy of Chromium is with webkit r88913. The one on the right is 13.x with an older version of webkit.
Comment 1 Lei Zhang 2011-06-15 19:30:04 PDT
Created attachment 97389 [details]
screenshot of the problem
Comment 2 Dimitri Glazkov (Google) 2011-06-17 15:50:30 PDT
Sounds pretty bad. Niko, how soon can you fix this?
Comment 3 Nikolas Zimmermann 2011-06-17 23:47:05 PDT
Excellent catch! I feared we had no layout test coverage for object+PDF. How can that be possible _at all_? Who landed PDFDocument w/o proper test...

Enough of the moaning, I'll take care to fix this, and add decent test coverage for it, ok?
Comment 4 Nikolas Zimmermann 2011-06-17 23:52:04 PDT
Some background, for those who wonder what has changed:



int RenderReplaced::computeReplacedLogicalHeight() const
{
    // 10.5 Content height: the 'height' property: http://www.w3.org/TR/CSS21/visudet.html#propdef-height
    // If the height of the containing block is not specified explicitly (i.e., it depends on
    // content height), and this element is not absolutely positioned, the value computes to 'auto'.
    bool heightIsAuto = style()->logicalHeight().isAuto();
    if (!document()->inQuirksMode() && !isPositioned() && style()->logicalHeight().isPercent()) {
        if (RenderObject* containingBlock = this->containingBlock()) {
            while (containingBlock->isAnonymous())
                containingBlock = containingBlock->containingBlock();
            heightIsAuto = !containingBlock->style()->logicalHeight().isSpecified();
        }
    }

...

This behavior is correct according to 2.1. Though as you can see above I had to disable this logic for two cases:
1) document is in quirks mode: don't apply the height="xx" -> height="auto" computation.
2) containingBlock is anonymous, the spec doesn't say anything about this, and in order to not break table/mozilla/ it had to be done this way.

Hm, do we need more exceptions for PDF? I'm not sure. Maybe Dave can comment as well?
I'll also CC Simon Fraser, maybe he's got an opinion.

Side note: removing that heightIsAuto correction completely, will break some of the new CSS 2.1 testcases that I imported... *sigh*.
Comment 5 Adam M-W 2011-06-20 16:43:03 PDT
Hi,
Looking at the test with Chrome's Developer Tools, object is actually 100% of the height of it's parent tag, body. I'm not sure, but isn't this the expected behaviour? body defaults to auto height, which is computed to be about 155px.
A quick fix is either to:
1. Use position:absolute to force to be 100% of window height, not body height
2. Use height:100% on html,body
in the actual html file rather than depending that body will always be the size of the window.
Comment 6 Lei Zhang 2011-06-20 17:38:45 PDT
Yes, it is possible to modify the html / css to do display correctly. The question is, what is the preferred behavior for the test case as is? I have no idea.
Comment 7 Nikolas Zimmermann 2011-06-21 13:06:09 PDT
(In reply to comment #5)
> Hi,
> Looking at the test with Chrome's Developer Tools, object is actually 100% of the height of it's parent tag, body. I'm not sure, but isn't this the expected behaviour? body defaults to auto height, which is computed to be about 155px.
> A quick fix is either to:
> 1. Use position:absolute to force to be 100% of window height, not body height
> 2. Use height:100% on html,body
> in the actual html file rather than depending that body will always be the size of the window.

We should compare our behaviour for that testcase with other browsers.
Did anyone look at IE9/Opera/FF and whether they pass the testcase?

As I said before, the 100% -> 150px is correct here, according to CSS 2.1.
Comment 8 Mihai Parparita 2011-06-23 12:44:01 PDT
This seems to affect iframes with height 100% too (test case at http://beta.everlong.org/iframe2/, reported on the Chromium side as http://crbug.com/86489)
Comment 9 Stephen White 2011-06-23 14:57:26 PDT
See also http://code.google.com/p/chromium/issues/detail?id=87260
Comment 10 Simon Fraser (smfr) 2011-06-23 14:59:13 PDT
Would be useful if the title said *when* the computed height is wrong.
Comment 11 Julien Wajsberg 2011-07-07 07:17:52 PDT
For the IFrame problem, I can tell it gets the wrong computed height even if its parent is not body.

In the aforementioned testcase (http://beta.everlong.org/iframe2/), we have the following DOM tree :

html
 body
  div.iframe
   iframe

div.iframe is absoluted positioned, with top = right = bottom = left = 0px.
In my setup it gets computed height = 728px.

iframe has height:100% and gets height = 150px, which is completely wrong.

As I said in my original Chromium bug report, the only browsers doing this are IE7 and older. That's painful.
Comment 12 Nikolas Zimmermann 2011-07-08 04:43:54 PDT
(In reply to comment #11)
> For the IFrame problem, I can tell it gets the wrong computed height even if its parent is not body.
> 
> In the aforementioned testcase (http://beta.everlong.org/iframe2/), we have the following DOM tree :
> 
> html
>  body
>   div.iframe
>    iframe
> 
> div.iframe is absoluted positioned, with top = right = bottom = left = 0px.
> In my setup it gets computed height = 728px.
> 
> iframe has height:100% and gets height = 150px, which is completely wrong.
> 
> As I said in my original Chromium bug report, the only browsers doing this are IE7 and older. That's painful.


Hi Julien,

sorry I forgot to mention here that in bug 64059 I've discussed the problem, and how to fix it. I'll come to it soon!

Cheers,
Niko
Comment 13 Nikolas Zimmermann 2011-07-11 09:11:35 PDT
(In reply to comment #0)
> Created an attachment (id=97388) [details]
> test case
> 
> After r88913, I'm seeing a regression where an <object> with height=100% no longer having a height of 100%. Instead, the height is "100%", but it's computed to be 150 px.
> 
> I tested with the attached html file. You may also need a PDF viewer and a dummy test.pdf file to actually display the object.
> 
> In the attached screenshot, the left copy of Chromium is with webkit r88913. The one on the right is 13.x with an older version of webkit.

Sorry, I can't get your testcase running. Whatever pdf I use, it doesn't show up -- doesn't Safari support pdf-in-<object> ?? Is it chromium specific? If so, please reduce to a testcase that works in Safari.
Comment 14 Nikolas Zimmermann 2011-07-11 12:55:58 PDT
I can't open the original testcase with Safari, it doesn't seem to be able to embed pdf this way, I didn't investigate further. Though the problem is identical to bug 64059, and should be fixed with it as well.
Comment 15 Lei Zhang 2011-07-11 13:07:05 PDT
(In reply to comment #13)
> Sorry, I can't get your testcase running. Whatever pdf I use, it doesn't show up -- doesn't Safari support pdf-in-<object> ?? Is it chromium specific? If so, please reduce to a testcase that works in Safari.

The pdf plugin is Chromium specific. I think you can get the same result if you replace the <object> with:

<embed src="test.swf" type="application/x-shockwave-flash" width="100%" height="100%"></embed>
Comment 16 Nikolas Zimmermann 2011-07-12 02:06:53 PDT
Thanks a lot! My feeling was right: the underlying bug is the same. Marking as duplicate of bug 64059, whose patch fixes this.(In reply to comment #15)
> (In reply to comment #13)
> > Sorry, I can't get your testcase running. Whatever pdf I use, it doesn't show up -- doesn't Safari support pdf-in-<object> ?? Is it chromium specific? If so, please reduce to a testcase that works in Safari.
> 
> The pdf plugin is Chromium specific. I think you can get the same result if you replace the <object> with:
> 
> <embed src="test.swf" type="application/x-shockwave-flash" width="100%" height="100%"></embed>

Thank you, I can confirm the underlying issue is the same as 64059. Marking as dup

*** This bug has been marked as a duplicate of bug 64059 ***
Comment 17 Nikolas Zimmermann 2011-07-19 01:14:29 PDT
(In reply to comment #16)
The fix landed in r91242. Please test and comment whether the original bug with the PDF in Chromium is gone as well.