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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 months ago by jfernandez
Modified:
11 months, 2 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] 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
Commit queue not available (can’t edit this change).

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 ...
12 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) ...
12 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 ...
12 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 ...
11 months, 4 weeks 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 ...
11 months, 4 weeks 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 ...
11 months, 4 weeks 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 ...
11 months, 4 weeks 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 ...
11 months, 4 weeks 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 ...
11 months, 4 weeks ago (2016-09-26 14:56:38 UTC) #12
jfernandez
11 months, 3 weeks ago (2016-09-27 09:35:28 UTC) #13
cbiesinger
thanks for your patience and persistence with this patch :) lgtm with some comments below ...
11 months, 3 weeks ago (2016-09-27 09:47:42 UTC) #14
svillar
lgtm with @cbiesinger nits
11 months, 2 weeks 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
11 months, 2 weeks 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 ...
11 months, 2 weeks ago (2016-10-06 11:39:52 UTC) #20
cbiesinger
BTW -- now that we reformatted blink, please wrap your comment at 80 characters. git ...
11 months, 2 weeks 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
11 months, 2 weeks ago (2016-10-10 19:54:51 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:120001)
11 months, 2 weeks ago (2016-10-10 21:36:36 UTC) #26
commit-bot: I haz the power
11 months, 2 weeks 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}
Sign in to reply to this message.

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