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 Rendering

Assignee:

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:

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

Julien Chaffraix

Reported 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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Julien Chaffraix

Comment 1 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.

Tony Chang

Comment 2 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

Julien Chaffraix

Comment 3 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.

Julien Chaffraix

Comment 4 2013-03-08 17:22:56 PST

Created attachment 192315 [details] Patch for landing

WebKit Review Bot

Comment 5 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>

WebKit Review Bot

Comment 6 2013-03-08 17:50:31 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.