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

Issue 2398043002: [css-grid] Stretch should grow and shrink items to fit its grid area (Closed)

Created:
4 years, 2 months ago by Manuel Rego
Modified:
4 years, 2 months ago
CC:
chromium-reviews, jfernandez, 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] Stretch should grow and shrink items to fit its grid area After some discussions the CSS WG agreed that stretch should not only grow items, but also stretch them to fit its grid area. That way the "min-width|height: auto" is somehow ignored for grid items. More info at: https://github.com/w3c/csswg-drafts/issues/283 The good part is that this allows us to remove some ugly code we've in LayoutBox that was only used by Grid Layout. The tests have been updated according to the new expected behavior. For images, we'll be stretching on both axis right nos, so the aspect ratio won't be preserved. The default behavior might change in those cases, but that should be implemented in a different patch. BUG=653433 Committed: https://crrev.com/da340247a837f07594b3fcfb59a4e4493dc0fd45 Cr-Commit-Position: refs/heads/master@{#423839}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -33 lines) Patch
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-percentage-columns.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto.html View 4 chunks +7 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html View 3 chunks +3 lines, -3 lines 2 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 2 chunks +1 line, -22 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
Manuel Rego
4 years, 2 months ago (2016-10-06 09:14:09 UTC) #2
jfernandez
I'm really happy we could get rid of that code. Clearly LGTM. https://codereview.chromium.org/2398043002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html ...
4 years, 2 months ago (2016-10-06 10:23:28 UTC) #3
Manuel Rego
https://codereview.chromium.org/2398043002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html (right): https://codereview.chromium.org/2398043002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html#newcode118 third_party/WebKit/LayoutTests/fast/css-grid-layout/min-width-height-auto-and-margins.html:118: <div class="item" data-offset-x="20" data-offset-y="20" data-expected-width="110" data-expected-height="0">XXXX</div> On 2016/10/06 10:23:28, ...
4 years, 2 months ago (2016-10-06 10:41:36 UTC) #4
svillar
Explicit congrats to that removal. Of course lgtm
4 years, 2 months ago (2016-10-07 10:49:36 UTC) #5
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/2398043002/1
4 years, 2 months ago (2016-10-07 10:52:44 UTC) #7
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 2 months ago (2016-10-07 12:34:25 UTC) #8
commit-bot: I haz the power
4 years, 2 months ago (2016-10-07 12:35:53 UTC) #10
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/da340247a837f07594b3fcfb59a4e4493dc0fd45
Cr-Commit-Position: refs/heads/master@{#423839}

Powered by Google App Engine
This is Rietveld 408576698