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

Issue 2543193002: [css-grid] Use child's marginLogicalWidth to compute content-sized track (Closed)

Created:
3 years ago by jfernandez
Modified:
3 years 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] Use child's marginLogicalWidth to compute content-sized track When computing min-content and max-content of the content-sized tracks we are using the marginIntrinsicLogicalWidthForChild function, which uses the grid's writing-mode to determine wether to use the child's margin width or height. This is not correct when the grid item is orthogonal. This patch changes how we compute the tracks width so we use always the item's marginLogicalWidth, which depends only on its own writing mode. BUG=670642 Committed: https://crrev.com/bed02e0d9a18e1355ee2c5bacee610b6036b4f88 Cr-Commit-Position: refs/heads/master@{#436149}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Applied suggested changes. #

Patch Set 3 : Compute margins if child needs layout. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -10 lines) Patch
A third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html View 1 1 chunk +59 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 chunks +14 lines, -10 lines 0 comments Download

Messages

Total messages: 24 (13 generated)
jfernandez
Sent out for review.
3 years ago (2016-12-02 11:58:42 UTC) #4
Manuel Rego
LGTM. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html (right): https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html#newcode6 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:6: body { margin: 0; } I don't think ...
3 years ago (2016-12-02 12:30:47 UTC) #5
jfernandez
Thanks for the review. Uploaded a new patch for landing. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html (right): https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html#newcode6 ...
3 years ago (2016-12-02 13:36:07 UTC) #6
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/2543193002/40001
3 years ago (2016-12-02 13:36:21 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/346816)
3 years ago (2016-12-02 15:32:49 UTC) #11
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/2543193002/40001
3 years ago (2016-12-02 16:36:53 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/342923)
3 years ago (2016-12-02 17:59:38 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/2543193002/60001
3 years ago (2016-12-02 21:32:38 UTC) #18
cbiesinger
lgtm
3 years ago (2016-12-02 22:38:50 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:60001)
3 years ago (2016-12-03 02:47:22 UTC) #22
commit-bot: I haz the power
3 years ago (2016-12-03 02:49:08 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/bed02e0d9a18e1355ee2c5bacee610b6036b4f88
Cr-Commit-Position: refs/heads/master@{#436149}

Powered by Google App Engine
This is Rietveld 408576698