Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543193002:
[css-grid] Use child's marginLogicalWidth to compute content-sized track (Closed)
|
Created:
4 years ago by jfernandez Modified:
4 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. #
Messages
Total messages: 24 (13 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [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 ========== to ========== [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 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
Sent out for review.
LGTM. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... 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/Layo... 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 you need this. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:8: .row { grid-auto-flow: column; } Nit: This is already defined in grid.css: "gridAutoFlowColumnSparse". https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:9: .item { margin: 10px 5px 5px 15px; } Nit: Extra space "{ margin". https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:17: <p>This test checks that margin is computed correctly for content-sized tracks when using different writing modes.</p> Nit: s/that margin/that grid items' margin/
Thanks for the review. Uploaded a new patch for landing. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... 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/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:6: body { margin: 0; } On 2016/12/02 12:30:47, Manuel Rego (OOO til 07 Dec) wrote: > I don't think you need this. Done. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:8: .row { grid-auto-flow: column; } On 2016/12/02 12:30:47, Manuel Rego (OOO til 07 Dec) wrote: > Nit: This is already defined in grid.css: "gridAutoFlowColumnSparse". Done. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:9: .item { margin: 10px 5px 5px 15px; } On 2016/12/02 12:30:47, Manuel Rego (OOO til 07 Dec) wrote: > Nit: Extra space "{ margin". Done. https://codereview.chromium.org/2543193002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-track-sizing-with-margins-and-orthogonal-flows.html:17: <p>This test checks that margin is computed correctly for content-sized tracks when using different writing modes.</p> On 2016/12/02 12:30:47, Manuel Rego (OOO til 07 Dec) wrote: > Nit: s/that margin/that grid items' margin/ Done.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com Link to the patchset: https://codereview.chromium.org/2543193002/#ps40001 (title: "Applied suggested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com Link to the patchset: https://codereview.chromium.org/2543193002/#ps60001 (title: "Compute margins if child needs layout.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480714274524390,
"parent_rev": "8ecb9726e5c8ed6a71ab554d065ac656fbc2d2ff", "commit_rev":
"4dee35ed66fabeb4f1797994a86ed088d35f4470"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bed02e0d9a18e1355ee2c5bacee610b6036b4f88 Cr-Commit-Position: refs/heads/master@{#436149} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
