Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by jfernandez
Modified:
5 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/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 Review-Url: https://codereview.chromium.org/2832783003 Cr-Commit-Position: refs/heads/master@{#468672} Committed: https://chromium.googlesource.com/chromium/src/+/00833a5660df5bddeb6dabc8a2dfe9fbb94662ff

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. #

Patch Set 6 : Patch rebased. #

Messages

Total messages: 19 (6 generated)
jfernandez
Patch sent out for review.
6 months 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: // ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months ago (2017-04-21 11:15:33 UTC) #9
jfernandez
Please reviewers, take another look.
6 months 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 ...
5 months, 3 weeks ago (2017-04-28 09:51:16 UTC) #12
svillar
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#newcode251 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:251: child->ForceLayout(); I know this forced layout was already there ...
5 months, 3 weeks ago (2017-04-28 15:33:31 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832783003/120001
5 months, 3 weeks ago (2017-05-02 14:18:04 UTC) #16
commit-bot: I haz the power
5 months, 3 weeks ago (2017-05-02 16:27:18 UTC) #19
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/00833a5660df5bddeb6dabc8a2df...
Sign in to reply to this message.

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