Bug 96426 - percentage heights in quirks mode with auto-sized body are computed incorrectly
Summary: percentage heights in quirks mode with auto-sized body are computed incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-11 14:35 PDT by Ojan Vafai
Modified: 2012-09-13 15:55 PDT (History)
9 users (show)

See Also:


Attachments
testcase (267 bytes, text/html)
2012-09-11 14:35 PDT, Ojan Vafai
no flags Details
Patch (4.58 KB, patch)
2012-09-11 16:48 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (456.61 KB, patch)
2012-09-13 11:05 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (456.64 KB, patch)
2012-09-13 14:56 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-09-11 14:35:25 PDT
Created attachment 163439 [details]
testcase

We compute them to the size of the content of the body element. Opera and Firefox walk up to the html element to find the height of the html element or use the window's height if the html element doesn't have a fixed height.
Comment 1 Ojan Vafai 2012-09-11 16:00:08 PDT
IE9 also matches Opera/Firefox.
Comment 2 Ojan Vafai 2012-09-11 16:48:52 PDT
Created attachment 163480 [details]
Patch
Comment 3 WebKit Review Bot 2012-09-11 22:36:46 PDT
Comment on attachment 163480 [details]
Patch

Attachment 163480 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13818660

New failing tests:
tables/mozilla/core/cell_heights.html
fast/table/height-percent-test.html
tables/mozilla_expected_failures/bugs/bug19526.html
fast/block/basic/quirk-height.html
tables/mozilla_expected_failures/bugs/bug85016.html
tables/mozilla/core/table_heights.html
Comment 4 WebKit Review Bot 2012-09-11 23:18:26 PDT
Comment on attachment 163480 [details]
Patch

Attachment 163480 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13818667

New failing tests:
tables/mozilla/core/cell_heights.html
fast/table/height-percent-test.html
tables/mozilla_expected_failures/bugs/bug19526.html
fast/block/basic/quirk-height.html
tables/mozilla_expected_failures/bugs/bug85016.html
tables/mozilla/core/table_heights.html
Comment 5 Ojan Vafai 2012-09-12 15:17:55 PDT
Comment on attachment 163480 [details]
Patch

nm. This is causing genuine test failures.
Comment 6 Ojan Vafai 2012-09-13 11:05:30 PDT
Created attachment 163912 [details]
Patch
Comment 7 Tony Chang 2012-09-13 14:17:13 PDT
Comment on attachment 163912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163912&action=review

> Source/WebCore/rendering/RenderBox.cpp:2136
> +    while (!cb->isRenderView() && !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode()) {
>          if (!document()->inQuirksMode() && !cb->isAnonymousBlock())
>              break;

In quirks mode, it looks like it's possible for cb to be NULL now. How do we exit this loop?

> Source/WebCore/rendering/RenderBox.cpp:2138
> +            rootMarginBorderPaddingHeight += cb->marginBefore() + cb->marginAfter() + cb->borderAndPaddingLogicalHeight();

Why do we subtract the margin?  It shouldn't be in cb's height.  Can you add a test where you set the margin?

> LayoutTests/fast/css/percentage-height-auto-sized-body-quirks.html:8
> +<body onload="checkLayout('#item')">

What about a percentage height on body when in standards mode?  It should still check the <html> element, right?  Can you add a test for that?
Comment 8 Ojan Vafai 2012-09-13 14:56:19 PDT
Created attachment 163972 [details]
Patch
Comment 9 Ojan Vafai 2012-09-13 14:56:30 PDT
Comment on attachment 163912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163912&action=review

> Source/WebCore/rendering/RenderBox.cpp:2136
>              break;

I think we always hit the !cb->isRenderView() case. Do we ever have an root renderbox (i.e. html element) that is not contained in a RenderView?

>> Source/WebCore/rendering/RenderBox.cpp:2138
>> +            rootMarginBorderPaddingHeight += cb->marginBefore() + cb->marginAfter() + cb->borderAndPaddingLogicalHeight();
> 
> Why do we subtract the margin?  It shouldn't be in cb's height.  Can you add a test where you set the margin?

The root and body elements are special because we skip over them and grab the height of the renderview, which doesn't correctly subtract the margins on the body/html elements. This matches other browsers.

The body element by default has a margin, so that is tested already in the new test. I thought I added a test with margin on the HTML element, but apparently not. Added it to the existing test.

>> LayoutTests/fast/css/percentage-height-auto-sized-body-quirks.html:8
>> +<body onload="checkLayout('#item')">
> 
> What about a percentage height on body when in standards mode?  It should still check the <html> element, right?  Can you add a test for that?

I could, but this code literally has no effect on standards mode since the first thing the for loop does is break if we're in quirks mode. It's on my TODO list to merge that into the for-loop condition to make that more clear.

Anyways, I'm pretty sure we have test coverage of this already.
Comment 10 Tony Chang 2012-09-13 15:26:02 PDT
Comment on attachment 163912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163912&action=review

>>> Source/WebCore/rendering/RenderBox.cpp:2136
>>>              break;
>> 
>> In quirks mode, it looks like it's possible for cb to be NULL now. How do we exit this loop?
> 
> I think we always hit the !cb->isRenderView() case. Do we ever have an root renderbox (i.e. html element) that is not contained in a RenderView?

What happens in a DocumentFragment?
Comment 11 Tony Chang 2012-09-13 15:46:46 PDT
Comment on attachment 163972 [details]
Patch

(In reply to comment #10)
> (From update of attachment 163912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163912&action=review
> 
> >>> Source/WebCore/rendering/RenderBox.cpp:2136
> >>>              break;
> >> 
> >> In quirks mode, it looks like it's possible for cb to be NULL now. How do we exit this loop?
> > 
> > I think we always hit the !cb->isRenderView() case. Do we ever have an root renderbox (i.e. html element) that is not contained in a RenderView?
> 
> What happens in a DocumentFragment?

Ojan tells me that DocumentFragment's don't have a render tree, so they don't go through this code path.
Comment 12 Ojan Vafai 2012-09-13 15:55:41 PDT
Committed r128517: <http://trac.webkit.org/changeset/128517>