Bug 96308 - percentage widths rendered wrong in vertical writing mode with orthogonal parent
Summary: percentage widths rendered wrong in vertical writing mode with orthogonal parent
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-10 13:22 PDT by John Mellor
Modified: 2012-09-12 15:54 PDT (History)
9 users (show)

See Also:


Attachments
Testcase (1.19 KB, text/html)
2012-09-10 13:22 PDT, John Mellor
no flags Details
Patch (14.26 KB, patch)
2012-09-11 15:41 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch for landing (18.82 KB, patch)
2012-09-12 15:01 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Mellor 2012-09-10 13:22:38 PDT
Created attachment 163192 [details]
Testcase

See the attached testcase. The two red divs have vertical writing modes and width:100%. But instead of stretching the full width of the body (their parent), their width equals the height of the body (i.e. if you resize the window, to make it taller/shorter, the width of the red divs will increase/decrease!). This seems wrong, and I suspect is due to an error at the boundary between the orthogonal writing modes.

It's possible this is related to bug 47238.
Comment 1 Ojan Vafai 2012-09-10 18:59:11 PDT
RenderBox::computePercentageLogicalHeight is just wrong. All the places where it calls things like cb->logicalHeight(), it needs to be calling something like logicalHeightForContainingBlock(cb). Really, this->logicalHeightForChild(cb) would do the right thing, but the containingBlock is not this's child. It seems a shame to add a new set of methods here when we have a set that does the right thing but is named wrong.

We could rename these logicalHeightFor(RenderBox* other)?
Comment 2 John Mellor 2012-09-11 09:52:55 PDT
(In reply to comment #1)
> We could rename these logicalHeightFor(RenderBox* other)?

Alternatively, doing it the other way round, it might be useful to expose getters that take a writing mode, instead of using their own, something like:
LayoutUnit logicalHeightIn(EWritingMode)
{
    return EWritingMode == HORIZONTAL_TB ? height() : width();
}

Though I guess we can't produce a ternary value from RenderObject's m_bitfields.horizontalWritingMode(), and there's already a WritingMode enum in platform/text/WritingMode.h, so this probably isn't the best choice of enum...
Comment 3 Ojan Vafai 2012-09-11 15:41:09 PDT
Created attachment 163461 [details]
Patch
Comment 4 WebKit Review Bot 2012-09-11 17:02:45 PDT
Comment on attachment 163461 [details]
Patch

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

New failing tests:
fast/table/height-percent-test-vertical.html
Comment 5 Tony Chang 2012-09-11 17:03:17 PDT
Comment on attachment 163461 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2130
> +    while (!cb->isRenderView() && !cb->isBody() && !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode()) {

Nit: I would make a bool |hasPerpendicularContainingBlock|.

> Source/WebCore/rendering/RenderBox.cpp:2149
> +        availableHeight = cb->logicalWidth() - cb->borderAndPaddingLogicalWidth();

Nit: cb->contentLogicalWidth() which also subtracts the scrollbar.

> LayoutTests/fast/writing-mode/percentage-height-orthogonal-writing-modes-quirks.html:25
> +<body onload="checkLayout('.container')">
> +

Nit: Can we dump document.compatMode in the output to make sure we're in quirks mode?
Comment 6 Ojan Vafai 2012-09-12 14:49:21 PDT
Comment on attachment 163461 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2130
>> +    while (!cb->isRenderView() && !cb->isBody() && !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode()) {
> 
> Nit: I would make a bool |hasPerpendicularContainingBlock|.

I could, but cb changes each iteration through the loop. We'll need to set it each iteration through the loop. I'm not sure that's actually better.

>> Source/WebCore/rendering/RenderBox.cpp:2149
>> +        availableHeight = cb->logicalWidth() - cb->borderAndPaddingLogicalWidth();
> 
> Nit: cb->contentLogicalWidth() which also subtracts the scrollbar.

Good call.

>> LayoutTests/fast/writing-mode/percentage-height-orthogonal-writing-modes-quirks.html:25
>> +
> 
> Nit: Can we dump document.compatMode in the output to make sure we're in quirks mode?

I suppose. I think it's standard enough that no doctype == quirks mode that we don't need to do this for every quirks test, but...no harm. I'll add it.
Comment 7 Ojan Vafai 2012-09-12 15:01:41 PDT
Created attachment 163707 [details]
Patch for landing
Comment 8 Ojan Vafai 2012-09-12 15:02:16 PDT
Added a rebaseline for fast/table/height-percent-test-vertical.html since that test was testing this case as well.
Comment 9 WebKit Review Bot 2012-09-12 15:54:34 PDT
Comment on attachment 163707 [details]
Patch for landing

Clearing flags on attachment: 163707

Committed r128375: <http://trac.webkit.org/changeset/128375>
Comment 10 WebKit Review Bot 2012-09-12 15:54:38 PDT
All reviewed patches have been landed.  Closing bug.