Bug 111885

Summary: [CSS Grid Layout] Resolve grid-{end|after} integer against the end|after edge
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, eric, esprehn+autocc, macpherson, menard, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 103314    
Attachments:
Description Flags
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.
none
Patch for landing none

Description Julien Chaffraix 2013-03-08 13:12:43 PST
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
Comment 1 Julien Chaffraix 2013-03-08 14:23:49 PST
Created attachment 192282 [details]
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.
Comment 2 Tony Chang 2013-03-08 15:12:41 PST
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
Comment 3 Julien Chaffraix 2013-03-08 17:15:17 PST
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.
Comment 4 Julien Chaffraix 2013-03-08 17:22:56 PST
Created attachment 192315 [details]
Patch for landing
Comment 5 WebKit Review Bot 2013-03-08 17:50:27 PST
Comment on attachment 192315 [details]
Patch for landing

Clearing flags on attachment: 192315

Committed r145297: <http://trac.webkit.org/changeset/145297>
Comment 6 WebKit Review Bot 2013-03-08 17:50:31 PST
All reviewed patches have been landed.  Closing bug.