Bug 110244 - [CSS Grid Layout] Refactor the code in preparation of auto placement support
Summary: [CSS Grid Layout] Refactor the code in preparation of auto placement support
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: 103316
  Show dependency treegraph
 
Reported: 2013-02-19 11:40 PST by Julien Chaffraix
Modified: 2013-02-19 15:36 PST (History)
6 users (show)

See Also:


Attachments
Proposed refactoring: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm. (11.42 KB, patch)
2013-02-19 11:48 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed refactoring 2: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm. (11.56 KB, patch)
2013-02-19 11:58 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.87 KB, patch)
2013-02-19 13:33 PST, 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 2013-02-19 11:40:33 PST
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.
Comment 1 Julien Chaffraix 2013-02-19 11:48:16 PST
Created attachment 189127 [details]
Proposed refactoring: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.
Comment 2 Julien Chaffraix 2013-02-19 11:58:06 PST
Created attachment 189130 [details]
Proposed refactoring 2: Including some simple tests for grid-auto-flow: none, more testing with the real algorithm.
Comment 3 Ojan Vafai 2013-02-19 12:07:07 PST
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?
Comment 4 Julien Chaffraix 2013-02-19 12:14:05 PST
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.
Comment 5 Ojan Vafai 2013-02-19 12:15:39 PST
I was just suggesting to add both. There's no harm to making sure both bits happen correctly.
Comment 6 Julien Chaffraix 2013-02-19 13:28:42 PST
(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.
Comment 7 Julien Chaffraix 2013-02-19 13:33:13 PST
Created attachment 189154 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-02-19 15:36:15 PST
Comment on attachment 189154 [details]
Patch for landing

Clearing flags on attachment: 189154

Committed r143397: <http://trac.webkit.org/changeset/143397>
Comment 9 WebKit Review Bot 2013-02-19 15:36:19 PST
All reviewed patches have been landed.  Closing bug.