[CSS Grid Layout] Implementation of the align-self property in grid.

[CSS Grid Layout]  Implementation of the align-self and align-items
properties in grid.

BUG=376823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178006

[Julien - ping for review, 2014-05-21 14:41:37]

Ojan: you should double-check this change as it's mostly mine.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1123: LayoutUnit rowPosition = startOfRow +
marginBeforeForChild(child);
We should repeat the comment about shifting the grid item into the grid
container's border box.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1131: switch (childAlignSelf) {
No need for the |childAlignSelf| variable.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1133: return startOfRowForChild(child);
I think this needs a bit more logic:
- orthogonal flow are also disallowed in this case too
(http://dev.w3.org/csswg/css-align/#valuedef-self-start).
- if both alignment container and alignment subject have parallel writing-modes
but different block-flow direction, this will be wrong as the rows are measured
in the alignment container's (aka grid container) block-flow direction.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1135: return endOfRowForChild(child);
Same comment.

[jfernandez, 2014-05-23 14:06:48]

Applied suggested changes and adapted the layout test accordingly.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1123: LayoutUnit rowPosition = startOfRow +
marginBeforeForChild(child);
On 2014/05/21 14:41:38, Julien Chaffraix - CET wrote:
> We should repeat the comment about shifting the grid item into the grid
> container's border box.

Done.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1131: switch (childAlignSelf) {
On 2014/05/21 14:41:38, Julien Chaffraix - CET wrote:
> No need for the |childAlignSelf| variable.

Done.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1133: return startOfRowForChild(child);
On 2014/05/21 14:41:38, Julien Chaffraix - CET wrote:
> I think this needs a bit more logic:
> - orthogonal flow are also disallowed in this case too
> (http://dev.w3.org/csswg/css-align/#valuedef-self-start).

right, although such change is still only in the editor's draft. I've added it
to the implementation, though, with a FIXME for properly implementing the
orthogonal modes. I've been playing with them a bit and I think they don't work
very well now in grid layout. I guess it's because it's not properly described
the behavior in the specs, as you told me some days ago in the IRC.

> - if both alignment container and alignment subject have parallel
writing-modes
> but different block-flow direction, this will be wrong as the rows are
measured
> in the alignment container's (aka grid container) block-flow direction.

That's good point, thanks. I've implemented it and it should be working fine.
I've added new test cases to cover this situation.

https://codereview.chromium.org/297483005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:1135: return endOfRowForChild(child);
On 2014/05/21 14:41:38, Julien Chaffraix - CET wrote:
> Same comment.

Done.

[jfernandez, 2014-06-16 09:27:40]

Could someone review the last patch, please ?

[cbiesinger, 2014-06-17 22:18:31]

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1126: return startOfRow +
std::max<LayoutUnit>(0, endOfRow - startOfRow - child->logicalHeight()) / 2;
Shouldn't this also have a FIXME comment for overflow-position?

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1144: // FIXME: Properly support orthogonal
writing mode.
Isn't this the right way to handle it? Quoting
http://dev.w3.org/csswg/css-align-3/#valuedef-self-start:

| If the writing modes of the alignment subject and the alignment container are
orthogonal, this value computes to start.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1156: // Only used in flex layout, for
other layout, it's equivalent to 'start'.
I think it'd be clearer to add these two cases to ItemPositionStart, so it's
easier to see that the code is identical.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1165: return 0;
Is 0 really correct here?

http://dev.w3.org/csswg/css-align-3/#valuedef-left sounds like handling this
like "start" is the right thing.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1176: case ItemPositionAuto:
This seems really easy to implement, but OK to do this separately.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1177: case ItemPositionStretch:
http://dev.w3.org/csswg/css-align-3/#valuedef-stretch

| (For layout modes other than flex layout, flex-start is identical to start.)

[jfernandez, 2014-06-20 10:46:00]

Applied suggested changes.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1126: return startOfRow +
std::max<LayoutUnit>(0, endOfRow - startOfRow - child->logicalHeight()) / 2;
On 2014/06/17 22:18:31, cbiesinger wrote:
> Shouldn't this also have a FIXME comment for overflow-position?

Done.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1144: // FIXME: Properly support orthogonal
writing mode.
On 2014/06/17 22:18:31, cbiesinger wrote:
> Isn't this the right way to handle it? Quoting
> http://dev.w3.org/csswg/css-align-3/#valuedef-self-start:
> 
> | If the writing modes of the alignment subject and the alignment container
are
> orthogonal, this value computes to start.

Yes, but the FIXME is about the lack of orthogonal writing-modes support of
RenderGrid. I've implemented the behavior described in the css-align-3
specification, though.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1156: // Only used in flex layout, for
other layout, it's equivalent to 'start'.
On 2014/06/17 22:18:31, cbiesinger wrote:
> I think it'd be clearer to add these two cases to ItemPositionStart, so it's
> easier to see that the code is identical.

Done.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1165: return 0;
On 2014/06/17 22:18:31, cbiesinger wrote:
> Is 0 really correct here?
> 
> http://dev.w3.org/csswg/css-align-3/#valuedef-left sounds like handling this
> like "start" is the right thing.

Yes, done that, even though the previously commented FIXME still applies here.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1176: case ItemPositionAuto:
On 2014/06/17 22:18:31, cbiesinger wrote:
> This seems really easy to implement, but OK to do this separately.

Finally implemented in the new patch, It was quite easy, as you said.

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1177: case ItemPositionStretch:
On 2014/06/17 22:18:31, cbiesinger wrote:
> http://dev.w3.org/csswg/css-align-3/#valuedef-stretch
> 
> | (For layout modes other than flex layout, flex-start is identical to start.)

Well, it seems "stretch" requires more than such logic, at least for grids. It
involves changes in the track sizing algorithm in case of auto-sizing items.

[jfernandez, 2014-06-20 20:41:06]

Applied suggested changes.

[cbiesinger, 2014-06-21 00:52:36]

lgtm

thanks!

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1177: case ItemPositionStretch:
On 2014/06/20 10:46:00, jfernandez wrote:
> On 2014/06/17 22:18:31, cbiesinger wrote:
> > http://dev.w3.org/csswg/css-align-3/#valuedef-stretch
> > 
> > | (For layout modes other than flex layout, flex-start is identical to
start.)
> 
> Well, it seems "stretch" requires more than such logic, at least for grids. It
> involves changes in the track sizing algorithm in case of auto-sizing items.

Indeed. I misread the spec.

[Julien - ping for review, 2014-07-10 22:59:59]

lgtm

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1216: }
I assume this code is temporary as we talked about moving this to style
resolution / adjustment.

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1230: // self-start is based on the child's
direction. That's why we need to check against the grid container's direction.
s/direction/block axis direction/

direction has a different meaning (inline 'direction'), that's no coherent with
your use.

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1241: // self-end is based on the child's
direction. That's why we need to check against the grid container's direction.
Same comment.

[jfernandez, 2014-07-12 22:29:48]

Applied suggested changes.

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1216: }
On 2014/07/10 22:59:59, Julien Chaffraix - PST wrote:
> I assume this code is temporary as we talked about moving this to style
> resolution / adjustment.

Yes, still have to reach an agreement on issue 363133003, though. 
I've added a FIXME about it.

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1230: // self-start is based on the child's
direction. That's why we need to check against the grid container's direction.
On 2014/07/10 22:59:59, Julien Chaffraix - PST wrote:
> s/direction/block axis direction/
> 
> direction has a different meaning (inline 'direction'), that's no coherent
with
> your use.

Done.

https://codereview.chromium.org/297483005/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1241: // self-end is based on the child's
direction. That's why we need to check against the grid container's direction.
On 2014/07/10 22:59:59, Julien Chaffraix - PST wrote:
> Same comment.

Done.

[jfernandez, 2014-07-12 22:30:00]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-12 22:30:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/297483005/100001

[jfernandez, 2014-07-12 23:02:24]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2014-07-12 23:02:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jfernandez@igalia.com/297483005/120001

[commit-bot: I haz the power, 2014-07-13 00:07:10]

Change committed as 178006