Bug 96426

Summary:

percentage heights in quirks mode with auto-sized body are computed incorrectly

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

Layout and Rendering

Assignee:

Ojan Vafai <ojan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, esprehn, gyuyoung.kim, hyatt, jchaffraix, rakuco, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
testcase	none
Patch	none
Patch	none
Patch	tony: review+

Ojan Vafai

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 2012-09-11 16:00:08 PDT

IE9 also matches Opera/Firefox.

Ojan Vafai

Comment 2 2012-09-11 16:48:52 PDT

Created attachment 163480 [details] Patch

WebKit Review Bot

Comment 3 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

WebKit Review Bot

Comment 4 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

Ojan Vafai

Comment 5 2012-09-12 15:17:55 PDT

Comment on attachment 163480 [details] Patch nm. This is causing genuine test failures.

Ojan Vafai

Comment 6 2012-09-13 11:05:30 PDT

Created attachment 163912 [details] Patch

Tony Chang

Comment 7 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?

Ojan Vafai

Comment 8 2012-09-13 14:56:19 PDT

Created attachment 163972 [details] Patch

Ojan Vafai

Comment 9 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.

Tony Chang

Comment 10 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?

Tony Chang

Comment 11 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.

Ojan Vafai

Comment 12 2012-09-13 15:55:41 PDT

Committed r128517: <http://trac.webkit.org/changeset/128517>

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