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

Issue 2323793002: [css-grid] Allow percentage values for column and row gutters (Closed)

Created:
4 years, 3 months ago by Manuel Rego
Modified:
4 years, 3 months ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, blink-reviews-css, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, blink-reviews, eae+blinkwatch, blink-reviews-layout_chromium.org, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Allow percentage values for column and row gutters The grid-column-gap and grid-row-gap allow now percentage as one of the possible values. This patch adds the required support for both parsing and layout logic. This is based on a patch by Javier Fernández http://crrev.com/2057113002/. However it uses a different approach to compute the percentage gaps based on the sizing operation. During intrinsic size phase the percentage gaps are considered 0px. Also for indefinite heights the percentage row gaps are 0px too. BUG=615248 TEST=fast/css-grid-layout/grid-gutters-as-percentage.html Committed: https://crrev.com/78579c71b9013a88936d31b6ac4ce4b18b0ac339 Cr-Commit-Position: refs/heads/master@{#417386}

Patch Set 1 #

Total comments: 11

Patch Set 2 : Apply review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+240 lines, -26 lines) Patch
A third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html View 1 1 chunk +191 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-get-set.html View 4 chunks +13 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-get-set-expected.txt View 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp View 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 17 chunks +25 lines, -20 lines 0 comments Download

Messages

Total messages: 16 (6 generated)
Manuel Rego
In the CSS WG confcall yesterday they agreed on the resolution for the issue related ...
4 years, 3 months ago (2016-09-08 15:15:32 UTC) #2
jfernandez
https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode542 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:542: return valueForLength(direction == ForColumns ? styleRef().gridColumnGap() : styleRef().gridRowGap(), availableSize); ...
4 years, 3 months ago (2016-09-08 15:48:03 UTC) #3
svillar
lgtm https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode539 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:539: availableSize = availableLogicalHeightForPercentageComputation(); Ternary operator?. Also it's enough ...
4 years, 3 months ago (2016-09-08 16:03:35 UTC) #4
svillar
https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html (right): https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html#newcode29 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:29: <p>Height is indefinte, so row gaps should be 0. ...
4 years, 3 months ago (2016-09-08 16:15:03 UTC) #5
svillar
https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html (right): https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html#newcode98 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:98: There are a lot of test cases for indefinite ...
4 years, 3 months ago (2016-09-08 16:20:18 UTC) #6
Manuel Rego
Thanks for the reviews, uploading a new patch for landing. https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html (right): https://codereview.chromium.org/2323793002/diff/1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html#newcode29 ...
4 years, 3 months ago (2016-09-08 16:34:24 UTC) #7
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/2323793002/20001
4 years, 3 months ago (2016-09-08 16:38:09 UTC) #10
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/2323793002/20001
4 years, 3 months ago (2016-09-08 16:57:50 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 3 months ago (2016-09-08 20:37:22 UTC) #14
commit-bot: I haz the power
4 years, 3 months ago (2016-09-08 20:40:55 UTC) #16
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/78579c71b9013a88936d31b6ac4ce4b18b0ac339
Cr-Commit-Position: refs/heads/master@{#417386}

Powered by Google App Engine
This is Rietveld 408576698