Bug 62769 – REGRESSION (r88913): <object> has wrong computed height

• Component: Layout and Rendering
• Status: RESOLVED DUPLICATE

[Lei Zhang]

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.

[Lei Zhang]

Created attachment 97389 [details]
screenshot of the problem

[Dimitri Glazkov (Google)]

Sounds pretty bad. Niko, how soon can you fix this?

[Nikolas Zimmermann]

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?

[Nikolas Zimmermann]

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

[Adam M-W]

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.

[Lei Zhang]

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.

[Nikolas Zimmermann]

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

[Mihai Parparita]

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)

[Stephen White]

See also http://code.google.com/p/chromium/issues/detail?id=87260

[Simon Fraser (smfr)]

Would be useful if the title said *when* the computed height is wrong.

[Julien Wajsberg]

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.

[Nikolas Zimmermann]

(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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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.

[Lei Zhang]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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