Bug 180633 - Wrong position for orthogonal positioned element with writing-mode: vertical-rl
Summary: Wrong position for orthogonal positioned element with writing-mode: vertical-rl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-10 08:32 PST by Javier Fernandez
Modified: 2021-05-12 14:44 PDT (History)
16 users (show)

See Also:


Attachments
Test case to reproduce the bug (388 bytes, text/html)
2017-12-10 08:32 PST, Javier Fernandez
no flags Details
Expected (7.13 KB, image/png)
2017-12-10 08:35 PST, Javier Fernandez
no flags Details
Patch (21.70 KB, patch)
2021-04-27 03:34 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (32.93 KB, patch)
2021-04-27 05:47 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (31.29 KB, patch)
2021-05-07 03:10 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (27.62 KB, patch)
2021-05-12 03:15 PDT, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-12-10 08:32:41 PST
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.
Comment 1 Javier Fernandez 2017-12-10 08:35:54 PST
Created attachment 328936 [details]
Expected
Comment 2 Carlos Alberto Lopez Perez 2020-03-23 19:47:30 PDT
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
Comment 3 zsun 2021-04-27 03:34:21 PDT
Created attachment 427133 [details]
Patch
Comment 4 EWS Watchlist 2021-04-27 03:36:08 PDT
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
Comment 5 zsun 2021-04-27 05:47:23 PDT
Created attachment 427138 [details]
Patch
Comment 6 Javier Fernandez 2021-04-28 04:03:49 PDT
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 ?
Comment 7 zsun 2021-04-29 03:08:36 PDT
(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.
Comment 8 Javier Fernandez 2021-05-06 15:52:33 PDT
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.
Comment 9 zsun 2021-05-07 03:10:39 PDT
Created attachment 427985 [details]
Patch
Comment 10 Oriol Brufau 2021-05-11 07:50:09 PDT
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.
Comment 11 zsun 2021-05-12 03:15:28 PDT
Created attachment 428365 [details]
Patch
Comment 12 Javier Fernandez 2021-05-12 05:45:12 PDT
Comment on attachment 428365 [details]
Patch

r=me
Comment 13 EWS 2021-05-12 14:43:39 PDT
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].
Comment 14 Radar WebKit Bug Importer 2021-05-12 14:44:19 PDT
<rdar://problem/77932469>