Bug 110244 – [CSS Grid Layout] Refactor the code in preparation of auto placement support

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

We are up to a point where placeItemsOnGrid is called once and the cached coordinates are reused so implementing auto placement is possible.

In order to support auto placement, we need to start iterating several times over our grid items with auto row / column but only if grid-auto-flow is not none.

This refactor will prepare the multiple iteration as well as draw a better distinction with the grid-auto-flow: none case.

[Julien Chaffraix]

Created attachment 189127 [details]
Proposed refactoring: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.

[Julien Chaffraix]

Created attachment 189130 [details]
Proposed refactoring 2: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.

[Ojan Vafai]

Comment on attachment 189130 [details]
Proposed refactoring 2: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.

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

Please fix the test issue before submitting. Otherwise, this looks good.

> LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html:21
> +        <div class="sizedToGridArea autoRowAutoColumn" data-expected-width="50" data-expected-height="50">XXXXX XXXXX XXXXX</div>

Shouldn't this be checking x/y offsets if the thing we're checking is the positioning?

[Julien Chaffraix]

Comment on attachment 189130 [details]
Proposed refactoring 2: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.

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

>> LayoutTests/fast/css-grid-layout/grid-auto-flow-resolution.html:21
>> +        <div class="sizedToGridArea autoRowAutoColumn" data-expected-width="50" data-expected-height="50">XXXXX XXXXX XXXXX</div>
> 
> Shouldn't this be checking x/y offsets if the thing we're checking is the positioning?

It would work too but here the sizing is based on which column / row you are in (see the smallGrid example).

AFAICT both proposals have the same limitation with respect to implicit row / column as they would be zero-sized without any grid items. That means I don't have a strong preference over which one is better.

[Ojan Vafai]

I was just suggesting to add both. There's no harm to making sure both bits happen correctly.

[Julien Chaffraix]

(In reply to comment #5)
> I was just suggesting to add both. There's no harm to making sure both bits happen correctly.

Sounds good, I thought you were confused by the indirect position measurement through height / width.

[Julien Chaffraix]

Created attachment 189154 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189154 [details]
Patch for landing

Clearing flags on attachment: 189154

Committed r143397: <http://trac.webkit.org/changeset/143397>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.