Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(193)

Issue 1228983003: [CSS Grid Layout] Do not stretch always grid items with auto width (Closed)

Created:
5 years, 5 months ago by jfernandez
Modified:
5 years, 4 months ago
CC:
blink-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, blink-reviews-rendering, jchaffraix+rendering, eae+blinkwatch
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

[CSS Grid Layout] Do not stretch always grid items with auto width We assumed that any grid item with 'auto' width will be stretched to fill all the available space in its grid area. We assumed this because grid area acts as item's container. However, Grid Layout specification states on its Alignment section that items will be stretched by default, unless either justify-self or align-self compute to a value other than stretch or margins are auto. In those cases, grid items will auto-size to fit their contents. BUG=249451 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199819

Patch Set 1 #

Total comments: 26

Patch Set 2 : Applied suggested changes. #

Patch Set 3 : Renaming some fuctions to describe the axis direction. #

Patch Set 4 : Avoid overflow, since width is auto. #

Total comments: 7

Patch Set 5 : Respecting minMax constrains and let min-width logic decide if shrink is possible. #

Total comments: 4

Patch Set 6 : Removed stretching logic optimization, clearing override values by default. #

Total comments: 6

Patch Set 7 : Applied suggested changes. #

Patch Set 8 : Forgot to update the expectations file. #

Total comments: 4

Patch Set 9 : Applied additional suggested changes. #

Total comments: 28

Patch Set 10 : Applied suggested changes. #

Total comments: 7

Patch Set 11 : Applied suggested changes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -86 lines) Patch
M LayoutTests/fast/css-grid-layout/grid-align-justify-stretch.html View 1 2 3 4 5 3 chunks +6 lines, -6 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-auto-margins-and-stretch.html View 6 chunks +12 lines, -12 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html View 1 2 3 4 5 6 1 chunk +0 lines, -37 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change-expected.txt View 1 2 3 4 5 6 1 chunk +0 lines, -4 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-width-or-margin-change.html View 1 2 3 4 5 6 1 chunk +46 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-width-or-margin-change-expected.txt View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/min-width-height-auto.html View 1 2 3 4 5 6 7 8 9 2 chunks +48 lines, -0 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/min-width-height-auto-expected.txt View 1 2 3 4 5 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-align-items-changed.html View 1 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-align-items-changed-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-align-self-changed.html View 1 1 chunk +39 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-align-self-changed-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-justify-items-changed.html View 1 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-justify-items-changed-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-justify-self-changed.html View 1 1 chunk +39 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/relayout-justify-self-changed-expected.txt View 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/layout/LayoutBox.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M Source/core/layout/LayoutGrid.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/layout/LayoutGrid.cpp View 1 2 3 4 5 6 7 8 9 10 3 chunks +36 lines, -24 lines 0 comments Download

Messages

Total messages: 23 (3 generated)
jfernandez
5 years, 5 months ago (2015-07-09 21:32:32 UTC) #2
svillar
Some comments about the tests https://codereview.chromium.org/1228983003/diff/1/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html File LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html (right): https://codereview.chromium.org/1228983003/diff/1/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html#newcode10 LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html:10: #fromFixedWidth { width: 100px; ...
5 years, 5 months ago (2015-07-10 14:00:53 UTC) #3
jfernandez
https://codereview.chromium.org/1228983003/diff/1/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html File LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html (right): https://codereview.chromium.org/1228983003/diff/1/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html#newcode10 LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html:10: #fromFixedWidth { width: 100px; } On 2015/07/10 14:00:52, svillar ...
5 years, 5 months ago (2015-07-10 15:07:25 UTC) #4
Manuel Rego
Nits about the tittle: Start with captial letter and remove final dot. Some comments inline. ...
5 years, 5 months ago (2015-07-13 12:40:07 UTC) #5
jfernandez
Thanks for the review @rego. See my replies inline to some of your comments. I've ...
5 years, 5 months ago (2015-07-14 13:36:08 UTC) #6
Manuel Rego
It's looking better but I'd add some extra tests in fast/css-grid-layout/min-width-height-auto.html and ast/css-grid-layout/min-width-height-auto-overflow.html: 1) Replicating ...
5 years, 5 months ago (2015-07-14 15:39:42 UTC) #7
jfernandez
Added the additional test cases, as requested. https://codereview.chromium.org/1228983003/diff/80001/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1228983003/diff/80001/Source/core/layout/LayoutGrid.cpp#newcode1457 Source/core/layout/LayoutGrid.cpp:1457: { On ...
5 years, 5 months ago (2015-07-15 11:05:33 UTC) #8
Manuel Rego
Non-owner LGTM. We still need a better solution and code for min-width|height, but we should ...
5 years, 5 months ago (2015-07-16 10:45:05 UTC) #9
jfernandez
Applied suggested changes. https://codereview.chromium.org/1228983003/diff/100001/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html File LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html (right): https://codereview.chromium.org/1228983003/diff/100001/LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html#newcode14 LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html:14: <p>The grids below had initially 'stretched' ...
5 years, 5 months ago (2015-07-16 11:06:03 UTC) #10
jfernandez
This still needs an OWNER lgtm, so perhaps @svillar can do something about it.
5 years, 5 months ago (2015-07-16 11:07:00 UTC) #11
Manuel Rego
Sorry, but I was wrong before because of I was comparing behavior with flexbox and ...
5 years, 5 months ago (2015-07-23 09:41:26 UTC) #12
jfernandez
https://codereview.chromium.org/1228983003/diff/140001/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1228983003/diff/140001/Source/core/layout/LayoutGrid.cpp#newcode1463 Source/core/layout/LayoutGrid.cpp:1463: return isHorizontalWritingMode() ? style()->minWidth().isAuto() : child.style()->minHeight().isAuto(); On 2015/07/23 09:41:26, ...
5 years, 5 months ago (2015-07-23 11:01:04 UTC) #13
svillar
Feeling that we're almost there (I specially love the big number of new tests). I'd ...
5 years, 4 months ago (2015-07-28 15:14:08 UTC) #14
svillar
https://codereview.chromium.org/1228983003/diff/160001/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1228983003/diff/160001/Source/core/layout/LayoutGrid.cpp#newcode1688 Source/core/layout/LayoutGrid.cpp:1688: if (!hasOrthogonalWritingMode) { Now that I think about this ...
5 years, 4 months ago (2015-07-28 15:34:14 UTC) #15
jfernandez
I think I've applied all the suggested changes. https://codereview.chromium.org/1228983003/diff/160001/LayoutTests/fast/css-grid-layout/min-width-height-auto.html File LayoutTests/fast/css-grid-layout/min-width-height-auto.html (right): https://codereview.chromium.org/1228983003/diff/160001/LayoutTests/fast/css-grid-layout/min-width-height-auto.html#newcode78 LayoutTests/fast/css-grid-layout/min-width-height-auto.html:78: <!-- ...
5 years, 4 months ago (2015-07-28 21:31:51 UTC) #16
svillar
https://codereview.chromium.org/1228983003/diff/160001/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1228983003/diff/160001/Source/core/layout/LayoutGrid.cpp#newcode1674 Source/core/layout/LayoutGrid.cpp:1674: LayoutUnit childWidthToFit = std::max(std::min(child.maxPreferredLogicalWidth(), child.overrideContainingBlockContentLogicalWidth() - child.marginLogicalWidth()), child.minPreferredLogicalWidth()); On ...
5 years, 4 months ago (2015-07-29 08:18:05 UTC) #17
svillar
lgtm https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/LayoutGrid.cpp File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/LayoutGrid.cpp#newcode1638 Source/core/layout/LayoutGrid.cpp:1638: // We clear botth width and height override ...
5 years, 4 months ago (2015-07-30 10:11:27 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1228983003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1228983003/200001
5 years, 4 months ago (2015-07-31 10:53:04 UTC) #21
commit-bot: I haz the power
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199819
5 years, 4 months ago (2015-07-31 12:08:38 UTC) #22
jfernandez
5 years, 4 months ago (2015-07-31 22:25:42 UTC) #23
Message was sent while issue was closed.
https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/Lay...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:1638: // We clear botth width and height
override values because we will decide now whether they
On 2015/07/30 10:11:27, svillar wrote:
> Nit botth -> both

Done.

https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:1648: bool canShrinkToFitInRowAxisForChild =
!hasAutoMinSizeInRowAxis || child.minPreferredLogicalWidth() <=
child.overrideContainingBlockContentLogicalWidth();
On 2015/07/30 10:11:27, svillar wrote:
> As a general comment, perhaps we should use styleRef() to deal with references
> instead of pointers. Actually we use it so many times that perhaps we should
> consider
> 
> auto childStyle = child.styleRef()
> 
> and use it all along the method

Good idea.

https://codereview.chromium.org/1228983003/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:1666: return;
On 2015/07/30 10:11:27, svillar wrote:
> Sorry for the back and forth but I agree that perhaps it's better to keep it
as
> it was before without the early return which indeed looks weird here.

Acknowledged.

Powered by Google App Engine
This is Rietveld 408576698