Bug 111885 – [CSS Grid Layout] Resolve grid-{end|after} integer against the end|after edge

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

[Julien Chaffraix]

The specification mandates that all grid logical properties be resolved against the matching side.

This means that in the following code #gridItem is in the 3nd row and 3nd column:

<div style="display: grid; grid-definition-rows: 10px 10px 10px; grid-definition-columns: 10px 10px 10px;">
   <div id="gridItem" style="grid-row: auto / 1; grid-column: auto / 1"></div>
</div>

You can also check the examples on the specification: http://dev.w3.org/csswg/css3-grid-layout/#grid-start

[Julien Chaffraix]

Created attachment 192282 [details]
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.

[Tony Chang]

Comment on attachment 192282 [details]
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.

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

> Source/WebCore/rendering/RenderGrid.cpp:341
> +        const GridPosition& initialPosition = (direction == ForColumns) ? child->style()->gridItemStart() : child->style()->gridItemBefore();
> +        const GridPosition& finalPosition = (direction == ForColumns) ? child->style()->gridItemEnd() : child->style()->gridItemAfter();

I think it's a bit weird that we add the side information to GridPosition because when we read the values here, it's clear what side we're talking about.  

Another way to do this is to make another class (e.g., GridPositionWithSide) and construct it here to pass around.  This would avoid using the extra memory in RenderStyle and having to construct the grid position with side information.

Anyway, the current code is OK, but it seems redundant to store the direction information in 2 different ways and trying to make sure it stays in sync.

> Source/WebCore/rendering/style/GridPosition.h:51
> +    GridPosition(GridPositionSide side)

explicit

[Julien Chaffraix]

Comment on attachment 192282 [details]
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.

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

>> Source/WebCore/rendering/RenderGrid.cpp:341
>> +        const GridPosition& finalPosition = (direction == ForColumns) ? child->style()->gridItemEnd() : child->style()->gridItemAfter();
> 
> I think it's a bit weird that we add the side information to GridPosition because when we read the values here, it's clear what side we're talking about.  
> 
> Another way to do this is to make another class (e.g., GridPositionWithSide) and construct it here to pass around.  This would avoid using the extra memory in RenderStyle and having to construct the grid position with side information.
> 
> Anyway, the current code is OK, but it seems redundant to store the direction information in 2 different ways and trying to make sure it stays in sync.

You have a point, I thought that the side would be required elsewhere but only resolveGridPositionsFromStyle requires it. We are probably better off just passing it to the function explicitly for now. I can always revive something along the lines you said or this patch if the need shows up.

[Julien Chaffraix]

Created attachment 192315 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 192315 [details]
Patch for landing

Clearing flags on attachment: 192315

Committed r145297: <http://trac.webkit.org/changeset/145297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.