Bug 180633 – Wrong position for orthogonal positioned element with writing-mode: vertical-rl

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Created attachment 328935 [details]
Test case to reproduce the bug

Check the attached example it has a positioned item with "writing-mode: vertical-rl" and border.
The element appears out of the grid, which is wrong.
Without the border the issue is not reproducible.

[Javier Fernandez]

Created attachment 328936 [details]
Expected

[Carlos Alberto Lopez Perez]

This causes failures on the subtests cases with vertical-rl wirting-mode from WPT tests:

css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-010.html css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-009.html
css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-011.html css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-012.html

Created attachment 427133 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 427138 [details]
Patch

[Javier Fernandez]

Comment on attachment 427138 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:4454
> +        logicalLeftPos = containerLogicalWidth - computedValues.m_extent - logicalLeftPos;

Why in this case we don't need to consider the border ?

(In reply to Javier Fernandez from comment #6)
> Comment on attachment 427138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427138&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:4454
> > +        logicalLeftPos = containerLogicalWidth - computedValues.m_extent - logicalLeftPos;
> 
> Why in this case we don't need to consider the border ?

For replaced, we have computedValues.m_extent = computeReplacedLogicalHeight() + borderAndPaddingLogicalHeight();/computedValues.m_extent = computeReplacedLogicalWidth() + borderAndPaddingLogicalWidth();

So I assume that borders and paddings have already been included in computedValues.m_extent. In later calculation, they are taken out together as part of the logicalHeightValue/logicalWidthValue which is the corresponding value of computedValues.m_extent.

[Javier Fernandez]

Comment on attachment 427138 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:4586
> +    if ((this->style().isFlippedBlocksWritingMode() && this->isHorizontalWritingMode() != containerBlock.isHorizontalWritingMode())

I'm not convinced of the approach we are following in this patch. I understand that the problem is that we are not considering the border when computing the logical offset for vertical modes. However, after this change, the computeLogicalLeftPositionedOffset just adds the logical border, and the logic to deal with vertical modes were moved outside; it's now replicated in the classes of the different layout models. 

Additionally, I think this condition to detect cases that need to compute the rtl offset is quite complex and difficult to read. Would it be possible to define a function ? I guess the original idea of the current code was to insert this complex condition inside a computeLogicalLeftPositionedOffset, so that at least it was transparent for the pure layout logic.

Created attachment 427985 [details]
Patch

[Oriol Brufau]

Comment on attachment 427985 [details]
Patch

Overall it looks good to me, but I'm not a WebKit reviewer.

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

> Source/WebCore/rendering/RenderBox.cpp:3788
> +static void computeLogicalLeftPositionedOffset(LayoutUnit& logicalLeftPos, const RenderBox* child, LayoutUnit logicalWidthValue, const RenderBoxModelObject& containerBlock, LayoutUnit containerLogicalWidth, LayoutUnit bordersPlusPadding)

Nit: rather than adding a new parameter, maybe just add bordersPlusPadding into logicalWidthValue in the callers.
RenderBox::logicalWidth() and RenderBox::logicalHeight() seem to include borders and padding, so this logicalWidthValue should too?

> Source/WebCore/rendering/RenderBox.cpp:4123
> +static void computeLogicalTopPositionedOffset(LayoutUnit& logicalTopPos, const RenderBox* child, LayoutUnit logicalHeightValue, const RenderBoxModelObject& containerBlock, LayoutUnit containerLogicalHeight, LayoutUnit bordersPlusPadding)

Ditto.

Created attachment 428365 [details]
Patch

[Javier Fernandez]

Comment on attachment 428365 [details]
Patch

r=me

[EWS]

Committed r277391 (237647@main): <https://commits.webkit.org/237647@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428365 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77932469>