Bug 180632 – [css-grid] Implement alignment for absolute positioned grid items

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Javier Fernandez]

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)."

[Javier Fernandez]

Created attachment 328955 [details]
Patch

[Manuel Rego Casasnovas]

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?

[Javier Fernandez]

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

[Manuel Rego Casasnovas]

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.

[Javier Fernandez]

Created attachment 329139 [details]
Patch

Applied suggested changed.

[Javier Fernandez]

Created attachment 329141 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 329141 [details]
Patch

Thanks for the changes. LGTM!

[WebKit Commit Bot]

Comment on attachment 329141 [details]
Patch

Clearing flags on attachment: 329141

Committed r225805: <https://trac.webkit.org/changeset/225805>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36004392>