Chromium Code Reviews
Help | Chromium Project | Sign in
(7)

Issue 2333583002: [css-grid] Update intrinsic size for the extra sizing alg iteration.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by jfernandez
Modified:
4 months, 3 weeks ago
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Update intrinsic size for the extra sizing alg iteration. The spec states that in case any grid item has changed its 'min content contribution' we must repeat the track sizing algorithm. We were doing so without updating the grid contaner's intrinsic size. Hence, when the grid container was sized under min-content/max-content constraints, we were using an incorrect available space for sizing the column and row tracks. This patchs ensures that any orthogonal flows item is laid out before evaluating its 'min size contribution'. Also, if any item has a new size contribution, we assume the grid container's intrinsic size has changed, so it's recomputed. BUG=628565

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -38 lines) Patch
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-spanning-and-orthogonal-flows.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html View 3 chunks +10 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 2 chunks +3 lines, -2 lines 1 comment Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 5 chunks +41 lines, -23 lines 4 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 21 (4 generated)
jfernandez
5 months, 1 week ago (2016-09-12 07:49:45 UTC) #3
svillar
https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode438 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:438: if (!minContentContributionChanged) { I don't get this. For example ...
5 months, 1 week ago (2016-09-13 13:44:18 UTC) #5
jfernandez
@cbiesigner, what do you think ? https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2333583002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode438 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:438: if (!minContentContributionChanged) { ...
5 months, 1 week ago (2016-09-13 14:57:00 UTC) #6
jfernandez
@mstensho any idea of how to force a relayout of the grid container, so we ...
5 months ago (2016-09-16 12:28:09 UTC) #8
mstensho
On 2016/09/16 12:28:09, jfernandez wrote: > @mstensho any idea of how to force a relayout ...
5 months ago (2016-09-20 07:37:51 UTC) #9
jfernandez
On 2016/09/20 07:37:51, mstensho wrote: > On 2016/09/16 12:28:09, jfernandez wrote: > > @mstensho any ...
5 months ago (2016-09-20 08:10:04 UTC) #10
mstensho
On 2016/09/20 08:10:04, jfernandez wrote: > On 2016/09/20 07:37:51, mstensho wrote: > > On 2016/09/16 ...
5 months ago (2016-09-20 11:38:41 UTC) #11
jfernandez
On 2016/09/20 11:38:41, mstensho wrote: > On 2016/09/20 08:10:04, jfernandez wrote: > > On 2016/09/20 ...
5 months ago (2016-09-20 13:21:56 UTC) #12
cbiesinger
This code may be correct-ish due to https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp?q=preferredLogicalWidthsBecameDirty&sq=package:chromium&l=486 However, I want to advise you against ...
5 months ago (2016-09-20 21:07:37 UTC) #13
mstensho
I think I need a (simple) test case. I'm very unfamiliar with grid layout. The ...
5 months ago (2016-09-20 21:30:29 UTC) #14
jfernandez
On 2016/09/20 21:07:37, cbiesinger wrote: > This code may be correct-ish due to > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp?q=preferredLogicalWidthsBecameDirty&sq=package:chromium&l=486 ...
5 months ago (2016-09-21 10:32:55 UTC) #15
mstensho
On 2016/09/21 10:32:55, jfernandez wrote: > The idea solution, as we internally have discussed, would ...
5 months ago (2016-09-21 10:47:49 UTC) #16
jfernandez
On 2016/09/20 21:30:29, mstensho wrote: > I think I need a (simple) test case. I'm ...
5 months ago (2016-09-21 11:04:11 UTC) #17
jfernandez
On 2016/09/21 10:47:49, mstensho wrote: > On 2016/09/21 10:32:55, jfernandez wrote: > > The idea ...
5 months ago (2016-09-21 11:05:37 UTC) #18
mstensho
On 2016/09/21 11:04:11, jfernandez wrote: > On 2016/09/20 21:30:29, mstensho wrote: > > I think ...
5 months ago (2016-09-21 12:13:40 UTC) #19
jfernandez
On 2016/09/21 12:13:40, mstensho wrote: > On 2016/09/21 11:04:11, jfernandez wrote: > > On 2016/09/20 ...
5 months ago (2016-09-21 12:30:12 UTC) #20
cbiesinger
4 months, 3 weeks ago (2016-09-27 10:57:15 UTC) #21
Javier and I discussed this a bit in person yesterday.

We've already crossed the bridge with preferred sizes depending on layout when
we added the scrollbar logic here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

I now think it wouldn't be too bad to reuse that code for this issue with these
caveats:
1) grid will have to make sure to mark the container chain's preferred widths
dirty as well, when it calls this function:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
2) The LayoutBlockFlow code above should also freeze this preferred size change.
Perhaps we need a FreezePreferredSizesScope (like the existing
FreezeScrollbarsScope), and inside of that we never call setPreferredSizeDirty

2) is important to avoid the risk of infinite loops.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd