Summary: | Wrong position for orthogonal positioned element with writing-mode: vertical-rl | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Javier Fernandez <jfernandez> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, changseok, clopez, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, obrufau, pdr, rego, simon.fraser, webkit-bug-importer, youennf, zalan, zsun | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: |
https://bugs.chromium.org/p/chromium/issues/detail?id=779105 https://bugs.chromium.org/p/chromium/issues/detail?id=808758 https://bugs.webkit.org/show_bug.cgi?id=209650 |
||||||||||||||||
Attachments: |
|
Created attachment 328936 [details]
Expected
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
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
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. 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
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
Comment on attachment 428365 [details]
Patch
r=me
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]. |
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.