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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by jfernandez
Modified:
1 month, 3 weeks ago
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego (OOO til July 3rd), 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.
2 months, 1 week ago (2017-04-20 23:44:10 UTC) #2
Manuel Rego (OOO til July 3rd)
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: // ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-04-21 08:31:47 UTC) #6
Manuel Rego (OOO til July 3rd)
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 ...
2 months, 1 week ago (2017-04-21 08:51:51 UTC) #7
Manuel Rego (OOO til July 3rd)
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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-04-21 11:15:33 UTC) #9
jfernandez
Please reviewers, take another look.
2 months ago (2017-04-24 10:34:11 UTC) #11
Manuel Rego (OOO til July 3rd)
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 ...
2 months 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 ...
2 months 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
1 month, 3 weeks ago (2017-05-02 14:18:04 UTC) #16
commit-bot: I haz the power
1 month, 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 cb946e318