Bug 92908 - Implement computePreferredLogicalWidths on RenderGrid
Summary: Implement computePreferredLogicalWidths on RenderGrid
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: 60731
  Show dependency treegraph
 
Reported: 2012-08-01 14:16 PDT by Julien Chaffraix
Modified: 2012-08-06 10:19 PDT (History)
4 users (show)

See Also:


Attachments
Proposed change. Added an early implementation + fixed a test. (9.72 KB, patch)
2012-08-01 16:04 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Better change. Don't mess up with rows / columns. (6.90 KB, patch)
2012-08-02 11:14 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (6.92 KB, patch)
2012-08-03 12:02 PDT, 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 2012-08-01 14:16:05 PDT
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.
Comment 1 Julien Chaffraix 2012-08-01 16:04:37 PDT
Created attachment 155913 [details]
Proposed change. Added an early implementation + fixed a test.
Comment 2 Ojan Vafai 2012-08-01 17:34:42 PDT
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 3 Ojan Vafai 2012-08-01 18:20:13 PDT
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 4 Julien Chaffraix 2012-08-02 11:01:57 PDT
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.
Comment 5 Julien Chaffraix 2012-08-02 11:14:07 PDT
Created attachment 156127 [details]
Better change. Don't mess up with rows / columns.
Comment 6 Ojan Vafai 2012-08-02 11:15:20 PDT
(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 7 Ojan Vafai 2012-08-02 11:20:30 PDT
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.
Comment 8 Julien Chaffraix 2012-08-03 12:02:56 PDT
Created attachment 156428 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-08-03 16:36:21 PDT
Comment on attachment 156428 [details]
Patch for landing

Clearing flags on attachment: 156428

Committed r124671: <http://trac.webkit.org/changeset/124671>
Comment 10 WebKit Review Bot 2012-08-03 16:36:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Tony Chang 2012-08-05 20:31:34 PDT
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.
Comment 12 Ojan Vafai 2012-08-06 10:19:45 PDT
(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.