Bug 92908

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

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.

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.

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.

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

(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.

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.

Created attachment 156428 [details] Patch for landing

Comment on attachment 156428 [details] Patch for landing Clearing flags on attachment: 156428 Committed r124671: <http://trac.webkit.org/changeset/124671>

All reviewed patches have been landed. Closing bug.

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.

(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.