Implement computePreferredLogicalWidths on RenderGrid

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently RenderGrid doesn't implement the function which is the cause for the failures in fast/css-grid-layout/place-cell-by-index.html due to vertical writing-modes.

The result of the test in vertical writing modes doesn't seem totally right as the rows / columns are in logical coordinates.

[Julien Chaffraix]

Created attachment 155913 [details]
Proposed change. Added an early implementation + fixed a test.

[Ojan Vafai]

Comment on attachment 155913 [details]
Proposed change. Added an early implementation + fixed a test.

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

> Source/WebCore/rendering/RenderGrid.cpp:142
> +    return styleToUse->isHorizontalWritingMode() ? styleToUse->gridRows() : styleToUse->gridColumns();

Use isHorizontalWritingMode() (i.e. off the RenderGrid, not the RenderStyle). That caches the value. Then you won't need the styleToUse local variable either.

[Ojan Vafai]

Comment on attachment 155913 [details]
Proposed change. Added an early implementation + fixed a test.

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

Took a closer look at this patch. Sorry, was a little rushed before. I don't think we ever need to check isHorizontalWritingMode. grid-columns *always* refers to the logicalWidth. So, the old code in computedUsedBreadthOfGridTracks was correct and computePreferredLogicalWidths just needs to look at the grid-columns trackStyle regardless of writing mode.

> LayoutTests/fast/css-grid-layout/place-cell-by-index.html:72
> +<div class="grid" style="-webkit-writing-mode: vertical-rl; margin-bottom: 60px;" data-expected-width="110" data-expected-height="50">

I think the old values here and below were correct. Since row and column are swapped, grid-rows (20+30) now sets the width and grid-columns (50+60) now sets the height.

Also, now that the heights should be correct, I think you can get rid of the margin-bottom. I think that was just there to make the grids not overlap.

[Julien Chaffraix]

Comment on attachment 155913 [details]
Proposed change. Added an early implementation + fixed a test.

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

> Sorry, was a little rushed before. 

No problem.

> I don't think we ever need to check isHorizontalWritingMode. grid-columns *always* refers to the logicalWidth. So, the old code in computedUsedBreadthOfGridTracks was correct and computePreferredLogicalWidths just needs to look at the grid-columns trackStyle regardless of writing mode.

That's right I got confused and forgot we were handling logical coordinates so we don't need to flip.

>> LayoutTests/fast/css-grid-layout/place-cell-by-index.html:72
>> +<div class="grid" style="-webkit-writing-mode: vertical-rl; margin-bottom: 60px;" data-expected-width="110" data-expected-height="50">
> 
> I think the old values here and below were correct. Since row and column are swapped, grid-rows (20+30) now sets the width and grid-columns (50+60) now sets the height.
> 
> Also, now that the heights should be correct, I think you can get rid of the margin-bottom. I think that was just there to make the grids not overlap.

The width / height are fine - it was just me being confused - but I still think the offsets are wrong. The default for grid-{column|row}-align is 'stretch' which means that the grid items match their grid track sizes. The current offsets assumes that the properties resolve to 'start' but never defines that. It's orthogonal and I think this test should just assumes stretching. We will add the required testing once we support it anyway.

[Julien Chaffraix]

Created attachment 156127 [details]
Better change. Don't mess up with rows / columns.

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 155913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155913&action=review
> 
> The width / height are fine - it was just me being confused - but I still think the offsets are wrong. The default for grid-{column|row}-align is 'stretch' which means that the grid items match their grid track sizes. The current offsets assumes that the properties resolve to 'start' but never defines that. It's orthogonal and I think this test should just assumes stretching. We will add the required testing once we support it anyway.

Interesting. With flexbox, stretch only does something if the width/height is auto. The grid spec doesn't say that, but I think it probably should.

[Ojan Vafai]

Comment on attachment 156127 [details]
Better change. Don't mess up with rows / columns.

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

> LayoutTests/fast/css-grid-layout/place-cell-by-index.html:53
> +     them once we properly implement stretching / alingment. -->

s/alingment/alignment.

[Julien Chaffraix]

Created attachment 156428 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 156428 [details]
Patch for landing

Clearing flags on attachment: 156428

Committed r124671: <http://trac.webkit.org/changeset/124671>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Thanks for implementing this!

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 155913 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155913&action=review
> > 
> > The width / height are fine - it was just me being confused - but I still think the offsets are wrong. The default for grid-{column|row}-align is 'stretch' which means that the grid items match their grid track sizes. The current offsets assumes that the properties resolve to 'start' but never defines that. It's orthogonal and I think this test should just assumes stretching. We will add the required testing once we support it anyway.
> 
> Interesting. With flexbox, stretch only does something if the width/height is auto. The grid spec doesn't say that, but I think it probably should.

It looks like grid-{column|row}-align has been replaced by justify-self and align-self and now references the align spec: http://dev.w3.org/csswg/css3-align/

They now default to auto, which means stretch for grid items.  For a grid item with a fixed size, I think that means we align/justify to start (the logical top left).  I think this means that the offsets should be the same as start, but I could be wrong.

Anyway, yes, this is something we should fix when implementing {justify,align}-self properties.

[Ojan Vafai]

(In reply to comment #11)
> It looks like grid-{column|row}-align has been replaced by justify-self and align-self and now references the align spec: http://dev.w3.org/csswg/css3-align/
> 
> They now default to auto, which means stretch for grid items.  For a grid item with a fixed size, I think that means we align/justify to start (the logical top left).  I think this means that the offsets should be the same as start, but I could be wrong.
> 
> Anyway, yes, this is something we should fix when implementing {justify,align}-self properties.

I agree that this is how it should behave, but I don't see anything in the grid spec or the alignment spec that says to do this. I only see this in the flexbox spec at the moment.