Bug 111885 - [CSS Grid Layout] Resolve grid-{end|after} integer against the end|after edge
Summary: [CSS Grid Layout] Resolve grid-{end|after} integer against the end|after edge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 103314
  Show dependency treegraph
 
Reported: 2013-03-08 13:12 PST by Julien Chaffraix
Modified: 2013-03-08 17:50 PST (History)
10 users (show)

See Also:


Attachments
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid. (17.25 KB, patch)
2013-03-08 14:23 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (13.09 KB, patch)
2013-03-08 17:22 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.