Bug 96308

Summary: percentage widths rendered wrong in vertical writing mode with orthogonal parent
Product: WebKit Reporter: John Mellor <johnme>
Component: Layout and RenderingAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dglazkov, eric, hyatt, jchaffraix, jochen, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch
none
Patch for landing none

John Mellor
Reported 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.
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
DetailsFormatted DiffDiff
Patch for landing (18.82 KB, patch)
2012-09-12 15:01 PDT, Ojan Vafai 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 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)?
John Mellor
Comment 2 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...
Ojan Vafai
Comment 3 2012-09-11 15:41:09 PDT
Created attachment 163461 [details] Patch
WebKit Review Bot
Comment 4 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
Tony Chang
Comment 5 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?
Ojan Vafai
Comment 6 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.
Ojan Vafai
Comment 7 2012-09-12 15:01:41 PDT
Created attachment 163707 [details] Patch for landing
Ojan Vafai
Comment 8 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.
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2012-09-12 15:54:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.