Bug 111653 - [CSS Grid Layout] Handle 2 positions with one 'auto' properly
Summary: [CSS Grid Layout] Handle 2 positions with one 'auto' properly
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: 103314
  Show dependency treegraph
 
Reported: 2013-03-06 17:51 PST by Julien Chaffraix
Modified: 2013-03-08 10:27 PST (History)
7 users (show)

See Also:


Attachments
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment). (23.78 KB, patch)
2013-03-06 19:12 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (28.17 KB, patch)
2013-03-08 10:01 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-03-06 17:51:31 PST
After bug 111372 and bug 110777, it is possible to set the grid positions independently. However the layout code make the assumption that we have only one position. It also wrongly assumes that 'auto' / 1 should be autoplaced.
Comment 1 Julien Chaffraix 2013-03-06 19:12:20 PST
Created attachment 191893 [details]
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment).
Comment 2 Julien Chaffraix 2013-03-07 10:35:05 PST
Comment on attachment 191893 [details]
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment).

Passing the tests => good to go!
Comment 3 Julien Chaffraix 2013-03-07 17:21:14 PST
Comment on attachment 191893 [details]
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment).

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

> LayoutTests/fast/css-grid-layout/grid-item-spanning-resolution.html:106
> +    <div class="sizedToGridArea autoSecondRowAutoFirstColumn" data-expected-x="0" data-expected-y="90" data-expected-width="160" data-expected-height="210"></div>

The data-expected-{x|y} are wrong, they should be data-offset-{x|y}.

Also the resolution assumes that we resolve grid-{end|after} like grid-{start|before}, which is wrong as we need to take into account the direction from which it was made. I will see if keeping this test as part of this change still makes sense without further refactorings.
Comment 4 Tony Chang 2013-03-07 17:59:57 PST
Comment on attachment 191893 [details]
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment).

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

> Source/WebCore/rendering/RenderGrid.cpp:340
> +        // No |positions| means that the item's position will need to be resolved by the auto placement algoritm

Nit: algoritm -> algorithm

> Source/WebCore/rendering/RenderGrid.h:121
> +    // This is the function that should be used for any position resolution as we *have* to
> +    // resolve both directions at the same time (e.g. auto / 1, span 2 / 4, ...).
> +    PassOwnPtr<GridSpan> resolveGridPositionsFromStyle(const RenderBox*, TrackSizingDirection) const;

Nit: The wording of this comment is awkward.  Maybe the function name should somehow suggest that this is for resolving both directions at the same time.
Comment 5 Julien Chaffraix 2013-03-08 09:45:54 PST
Comment on attachment 191893 [details]
Proposed change 1: Added a new function to resolve both opposite positions. Naming could probably be improved (welcome any comment).

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

>> Source/WebCore/rendering/RenderGrid.h:121
>> +    PassOwnPtr<GridSpan> resolveGridPositionsFromStyle(const RenderBox*, TrackSizingDirection) const;
> 
> Nit: The wording of this comment is awkward.  Maybe the function name should somehow suggest that this is for resolving both directions at the same time.

Good point, I also discovered that some places (grid size estimations e.g.) won't be able to reuse the function. I will just remove the comment for now.
Comment 6 Julien Chaffraix 2013-03-08 10:01:24 PST
Created attachment 192245 [details]
Patch for landing
Comment 7 WebKit Review Bot 2013-03-08 10:27:41 PST
Comment on attachment 192245 [details]
Patch for landing

Clearing flags on attachment: 192245

Committed r145240: <http://trac.webkit.org/changeset/145240>
Comment 8 WebKit Review Bot 2013-03-08 10:27:45 PST
All reviewed patches have been landed.  Closing bug.