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

Issue 2832783003: [css-grid] Clearing the override height before layout

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by jfernandez
Modified:
1 day, 7 hours 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/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Clearing the override height before layout The stretching logic sets an override height to the grid item so it's used instead of the regular one, computed in the LayoutBox class. This override height is computed using the 'grid area' abstraction as item's containing block. It's expectable that a style update where item's Self Alginment CSS properties change from 'stretch' to any other value implies recomputing the item's size. The currently implemented stretching logic clears the override flags before determining whether to assign an stretched override height. We expect that the layout performed later will adjust the grid item's height. However, if the grid item has been laid out already it doesn't adjust its height, even though the override has been cleared already. This is precisely the situation detected in the bug, because the Baseline Alignment logic performs a layout of the grid item before computing the baseline offsets. This layout causes that the grid item's size is not computed again after clearing the override flag. There may be other cases like the one detected in the bug, so with this patch we are clearing the override flag at the beginning of the layout. This is a change we wanted to do ling time ago, since the override flags should be more consistent and not affecting intermediate operations, even repeating the layout operation. BUG=709902

Patch Set 1 #

Total comments: 3

Patch Set 2 : Added test and some optimizations. #

Total comments: 12

Patch Set 3 : Applied some changes to the test. #

Patch Set 4 : Applied suggested changes. #

Total comments: 2

Patch Set 5 : Modified comment, as requested. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -23 lines) Patch
A third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html View 1 2 1 chunk +38 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp View 1 chunk +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 2 chunks +16 lines, -18 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 13 (2 generated)
jfernandez
Patch sent out for review.
1 week, 1 day ago (2017-04-20 23:44:10 UTC) #2
Manuel Rego
Don't we miss a test in this CL? https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode238 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // ...
1 week, 1 day ago (2017-04-21 05:21:11 UTC) #3
svillar
Wouldn't we be forcing a layout of every single grid item by clearing the overrides ...
1 week, 1 day ago (2017-04-21 07:45:06 UTC) #4
jfernandez
On 2017/04/21 07:45:06, svillar wrote: > Wouldn't we be forcing a layout of every single ...
1 week, 1 day ago (2017-04-21 08:31:16 UTC) #5
jfernandez
Yes, I forgot to add it to the commit, but I've got it already. I'll ...
1 week, 1 day ago (2017-04-21 08:31:47 UTC) #6
Manuel Rego
https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2832783003/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode238 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // computed again in the updateLogicalWidth call bellow. On ...
1 week, 1 day ago (2017-04-21 08:51:51 UTC) #7
Manuel Rego
Some comments on the test. https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html (right): https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html#newcode15 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:15: <p>Test to verify that ...
1 week, 1 day ago (2017-04-21 10:45:46 UTC) #8
jfernandez
https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html (right): https://codereview.chromium.org/2832783003/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html#newcode15 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-change-alignment-from-stretch.html:15: <p>Test to verify that grid item's stretched size is ...
1 week, 1 day ago (2017-04-21 11:15:33 UTC) #9
jfernandez
Please reviewers, take another look.
5 days, 12 hours ago (2017-04-24 10:34:11 UTC) #11
Manuel Rego
lgtm https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode238 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:238: // interfere in current layout execution. Thanks for ...
1 day, 13 hours ago (2017-04-28 09:51:16 UTC) #12
svillar
1 day, 7 hours ago (2017-04-28 15:33:31 UTC) #13
https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2832783003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:251: child->ForceLayout();
I know this forced layout was already there in order to mimic the pre-layout
done in FrameView. The thing is that we no longer to that for grid items, so
perhaps it'd be enough to call SetNeedsLayout() but we can do it in a separate
patch.
Sign in to reply to this message.

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