According to the css-align spec (https://drafts.csswg.org/css-align/#justify-abspos), it should be possible to align positioned items too: "Alignment Container The box’s containing block, as modified by the offset properties (top/right/bottom/left)."
Created attachment 328955 [details] Patch
Comment on attachment 328955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328955&action=review The patch looks good, just some minor comments and a question about the result of some tests. > Source/WebCore/ChangeLog:25 > + Updated web-platform-tests so that all test cases pass now. Nit: I'd say "Updated web-platform-tests results" or "expected results". Also not all the tests pass now as stated in the previous paragraph, so I'd reword this. > Source/WebCore/rendering/RenderGrid.cpp:1423 > + return (position.isAuto() || (position.isNamedGridArea() && !NamedLineCollection::isValidNamedLineOrArea(position.namedGridLine(), style(), GridPositionsResolver::initialPositionSide(direction)))); Nit: I guess the external parenthesis are not needed. > Source/WebCore/rendering/RenderGrid.cpp:1493 > + // These vectors store line positions including gaps, but we shouldn't > + // consider them for the edges of the grid. Nit: Don't need to wrap this comment line in WebKit. > Source/WebCore/rendering/RenderGrid.cpp:1502 > + // TODO (lajava): Is expectable that in some cases 'end' is smaller than > + // 'start' ? I believe this is expected in some situations, so we can remove the TODO. > Source/WebCore/rendering/RenderGrid.cpp:1532 > + if (std::optional<size_t> line = lineOfPositionedItem.get(&child)) { Nit: You can use "auto" here. > Source/WebCore/rendering/RenderGrid.h:185 > + OutOfFlowPositionsMap m_columnOfPositionedItem; Nit: I don't like a lot the name "OutOfFlow..." when the variable is "...PositionedItem". > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-009-expected.txt:41 > +offsetLeft expected 80 but got 110 This is a RTL case, not a vertical one. Why is this failing?
Comment on attachment 328955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328955&action=review >> Source/WebCore/ChangeLog:25 >> + Updated web-platform-tests so that all test cases pass now. > > Nit: I'd say "Updated web-platform-tests results" or "expected results". > Also not all the tests pass now as stated in the previous paragraph, so I'd reword this. Done. >> Source/WebCore/rendering/RenderGrid.cpp:1423 >> + return (position.isAuto() || (position.isNamedGridArea() && !NamedLineCollection::isValidNamedLineOrArea(position.namedGridLine(), style(), GridPositionsResolver::initialPositionSide(direction)))); > > Nit: I guess the external parenthesis are not needed. Done >> Source/WebCore/rendering/RenderGrid.cpp:1493 >> + // consider them for the edges of the grid. > > Nit: Don't need to wrap this comment line in WebKit. Done >> Source/WebCore/rendering/RenderGrid.cpp:1502 >> + // 'start' ? > > I believe this is expected in some situations, so we can remove the TODO. Done >> Source/WebCore/rendering/RenderGrid.cpp:1532 >> + if (std::optional<size_t> line = lineOfPositionedItem.get(&child)) { > > Nit: You can use "auto" here. Done >> Source/WebCore/rendering/RenderGrid.h:185 >> + OutOfFlowPositionsMap m_columnOfPositionedItem; > > Nit: I don't like a lot the name "OutOfFlow..." when the variable is "...PositionedItem". Well, positioned items are indeed out-of-flow elements, according to this statement: https://www.w3.org/TR/CSS22/visuren.html#positioning-scheme Do you mean that we should rename, either the variable or the type, to use the same term ? >> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-009-expected.txt:41 >> +offsetLeft expected 80 but got 110 > > This is a RTL case, not a vertical one. Why is this failing? The item is also verticalRL
Comment on attachment 328955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328955&action=review >>> Source/WebCore/ChangeLog:25 >>> + Updated web-platform-tests so that all test cases pass now. >> >> Nit: I'd say "Updated web-platform-tests results" or "expected results". >> Also not all the tests pass now as stated in the previous paragraph, so I'd reword this. > > Done. Mmmm, I don't see the new patch yet. :-) >>> Source/WebCore/rendering/RenderGrid.h:185 >>> + OutOfFlowPositionsMap m_columnOfPositionedItem; >> >> Nit: I don't like a lot the name "OutOfFlow..." when the variable is "...PositionedItem". > > Well, positioned items are indeed out-of-flow elements, according to this statement: > > https://www.w3.org/TR/CSS22/visuren.html#positioning-scheme > > Do you mean that we should rename, either the variable or the type, to use the same term ? Yeah I know, I meant that is strange to use one term in the type and another in the variable. I won't modify the variable name, so I'd use a different name for the type. Anyway it's not really important, it's just a nit. >>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-self-alignment-non-static-positioned-items-009-expected.txt:41 >>> +offsetLeft expected 80 but got 110 >> >> This is a RTL case, not a vertical one. Why is this failing? > > The item is also verticalRL Oops I didn't realize.
Created attachment 329139 [details] Patch Applied suggested changed.
Created attachment 329141 [details] Patch
Comment on attachment 329141 [details] Patch Thanks for the changes. LGTM!
Comment on attachment 329141 [details] Patch Clearing flags on attachment: 329141 Committed r225805: <https://trac.webkit.org/changeset/225805>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36004392>