Summary: | percentage widths rendered wrong in vertical writing mode with orthogonal parent | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Mellor <johnme> | ||||||||
Component: | Layout and Rendering | Assignee: | 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
John Mellor
2012-09-10 13:22:38 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)? (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... Created attachment 163461 [details]
Patch
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 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 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. Created attachment 163707 [details]
Patch for landing
Added a rebaseline for fast/table/height-percent-test-vertical.html since that test was testing this case as well. Comment on attachment 163707 [details] Patch for landing Clearing flags on attachment: 163707 Committed r128375: <http://trac.webkit.org/changeset/128375> All reviewed patches have been landed. Closing bug. |