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

Issue 2361373002: [css-grid] Clearing override sizes before running grid's layout logic. (Closed)

Created:
4 years, 3 months ago by jfernandez
Modified:
4 years, 2 months 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] Clearing override sizes before running grid's layout logic. Grid's layout logic manages two different override sizes; one it's designed to implement the grid item's stretching behavior, identified with the concept of 'overrideContentLogicalSize'; there is another override size, known as overrideContainingBlockContentLogicalSize, used to implement the Grid Area abstraction, which will behave as the actual containing block of any grid item. During grid's layout logic these override sizes are set according to the CSS style rules. This affects how the grid container and its children are going to be sized during layout. Grid Tracks sizing algorithm depend on these override sizes. In order to ensure that the tracks sizing algorithm produces the same results when it's run consecutively several times, we need to clear these override sizes and performs a layout of the affected grid items. Otherwise, the affected items will return sizing values which depend on the override values set in the previous layout, which in some cases, like orthogonal flows, may change through different runs of the sizing algorithm. BUG=628565 Committed: https://crrev.com/45a34a1a8f382bcaa36e8d1c58575f34f2832237 Cr-Commit-Position: refs/heads/master@{#424250}

Patch Set 1 #

Total comments: 6

Patch Set 2 : A new approach, just facing orthogonal flow issues. #

Total comments: 4

Patch Set 3 : Getting back the initial approach, adding some constraints to avoid unneeded relayouts. #

Total comments: 2

Patch Set 4 : Patch for landing. #

Patch Set 5 : Patch rebased for landing. #

Patch Set 6 : Wrapped comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -7 lines) Patch
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-percentages-and-orthogonal-flows.html View 2 2 chunks +2 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/css-grid-layout/repeating-layout-must-produce-the-same-results.html View 1 2 1 chunk +53 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 5 3 chunks +23 lines, -4 lines 0 comments Download

Messages

Total messages: 28 (10 generated)
svillar
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode464 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:464: child->layoutIfNeeded(); Why are you laying out the items even ...
4 years, 3 months ago (2016-09-23 15:42:29 UTC) #3
cbiesinger
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode463 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:463: layoutScope.setChildNeedsLayout(child); (I would use forceChildLayout() instead of this sequence) ...
4 years, 3 months ago (2016-09-23 16:57:09 UTC) #4
jfernandez
https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2361373002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode463 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:463: layoutScope.setChildNeedsLayout(child); On 2016/09/23 16:57:09, cbiesinger wrote: > (I would ...
4 years, 3 months ago (2016-09-23 18:47:22 UTC) #5
jfernandez
Ok, here we go with a totally different approach. I hope it addresses most of ...
4 years, 2 months ago (2016-09-25 08:02:05 UTC) #6
cbiesinger
https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode1891 third_party/WebKit/Source/core/frame/FrameView.cpp:1891: if (toLayoutBox(*root).hasContainingBlockOverrideSize() || toLayoutBox(*root).hasOverrideSize()) { Hmm, so I understand ...
4 years, 2 months ago (2016-09-26 11:48:33 UTC) #8
cbiesinger
I guess what I'm wondering is: Why can't you do this force-layout at the point ...
4 years, 2 months ago (2016-09-26 11:49:31 UTC) #9
jfernandez
On 2016/09/26 11:49:31, cbiesinger wrote: > I guess what I'm wondering is: Why can't you ...
4 years, 2 months ago (2016-09-26 13:37:50 UTC) #10
jfernandez
https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2361373002/diff/40001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode1891 third_party/WebKit/Source/core/frame/FrameView.cpp:1891: if (toLayoutBox(*root).hasContainingBlockOverrideSize() || toLayoutBox(*root).hasOverrideSize()) { On 2016/09/26 11:48:32, cbiesinger ...
4 years, 2 months ago (2016-09-26 13:38:02 UTC) #11
cbiesinger
Summarizing our in-person discussion: The problem is in computeLogicalWidth() because the issue is the calculation ...
4 years, 2 months ago (2016-09-26 14:56:38 UTC) #12
jfernandez
4 years, 2 months ago (2016-09-27 09:35:28 UTC) #13
cbiesinger
thanks for your patience and persistence with this patch :) lgtm with some comments below ...
4 years, 2 months ago (2016-09-27 09:47:42 UTC) #14
svillar
lgtm with @cbiesinger nits
4 years, 2 months ago (2016-10-06 10:17:50 UTC) #15
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/2361373002/80001
4 years, 2 months ago (2016-10-06 10:31:07 UTC) #18
commit-bot: I haz the power
Failed to apply patch for third_party/WebKit/Source/core/layout/LayoutGrid.cpp: While running git apply --index -3 -p1; error: patch ...
4 years, 2 months ago (2016-10-06 11:39:52 UTC) #20
cbiesinger
BTW -- now that we reformatted blink, please wrap your comment at 80 characters. git ...
4 years, 2 months ago (2016-10-06 12:19:24 UTC) #21
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/2361373002/120001
4 years, 2 months ago (2016-10-10 19:54:51 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:120001)
4 years, 2 months ago (2016-10-10 21:36:36 UTC) #26
commit-bot: I haz the power
4 years, 2 months ago (2016-10-10 21:39:11 UTC) #28
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/45a34a1a8f382bcaa36e8d1c58575f34f2832237
Cr-Commit-Position: refs/heads/master@{#424250}

Powered by Google App Engine
This is Rietveld 408576698