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

Issue 1576993003: [css-grid] Fix placement for unknown named grid lines (Closed)

Created:
3 years, 4 months ago by Manuel Rego
Modified:
3 years, 3 months ago
CC:
chromium-reviews, blink-reviews, svillar, jfernandez, Manuel Rego, blink-reviews-style_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Fix placement for unknown named grid lines The spec has changed and now all the implicit lines should be considered when we're resolving named grid lines with an unknown name. The relevant part of the spec is (http://dev.w3.org/csswg/css-grid/#line-placement): "If a name is given as a <custom-ident>, only lines with that name are counted. If not enough lines with that name exist, all implicit grid lines are assumed to have that name for the purpose of finding this position." Modified the code to resolve named grid lines in GridResolvedPosition. We need to keep the old behavior of considering "auto" unknown named grid lines for the case of positioned grid items. Updated current tests to the new expected behavior and created a new test checking different cases explicitly. BUG=442954 TEST=fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html Committed: https://crrev.com/4a86c9a04f09c3a0cbf741feb535a2680c1faa79 Cr-Commit-Position: refs/heads/master@{#371792}

Patch Set 1 #

Patch Set 2 : New version after spec changes #

Total comments: 12

Patch Set 3 : Applying suggested changes #

Patch Set 4 : Small changes #

Patch Set 5 : New version with simplified loop #

Total comments: 10

Patch Set 6 : Fixing last review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+602 lines, -139 lines) Patch
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-container-change-named-grid-lines-recompute-child.html View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-bad-named-area-auto-placement.html View 1 chunk +11 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-bad-named-area-auto-placement-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-named-grid-area-resolution.html View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html View 3 chunks +16 lines, -13 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-negative-position-resolution.html View 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-position-changed-dynamic.html View 6 chunks +27 lines, -25 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html View 1 1 chunk +447 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-resolution.html View 2 chunks +7 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.cpp View 1 2 3 4 5 7 chunks +84 lines, -79 lines 0 comments Download

Messages

Total messages: 17 (5 generated)
Manuel Rego
3 years, 4 months ago (2016-01-12 12:03:45 UTC) #2
Manuel Rego
I asked some doubts to the CSS WG: https://lists.w3.org/Archives/Public/www-style/2016Jan/0063.html And they changed the spec, so ...
3 years, 4 months ago (2016-01-20 11:37:06 UTC) #3
svillar
Looking good but not ready yet. https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp#newcode72 third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:72: start = 0; ...
3 years, 4 months ago (2016-01-26 09:29:37 UTC) #4
Manuel Rego
Thanks for the review! I've applied the changes and tried to improve the loop. Let's ...
3 years, 4 months ago (2016-01-26 11:07:35 UTC) #5
svillar
lgtm from the grid layout POV although you need a style/ owner
3 years, 3 months ago (2016-01-27 09:52:33 UTC) #6
Manuel Rego
Thanks for the review! @rune and/or @timloh could you please take a look?
3 years, 3 months ago (2016-01-27 09:55:40 UTC) #8
rune
https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp#newcode68 third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:68: ASSERT(numberOfLines > 0); ASSERT(numberOfLines); https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp#newcode76 third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:76: numberOfLines--; Skip for-loop ...
3 years, 3 months ago (2016-01-27 10:27:54 UTC) #9
Manuel Rego
@rune thanks for the quick review. Uploaded a new patch applying your suggestions. https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp File ...
3 years, 3 months ago (2016-01-27 12:34:25 UTC) #10
rune
lgtm
3 years, 3 months ago (2016-01-27 13:58:18 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576993003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576993003/100001
3 years, 3 months ago (2016-01-27 14:32:35 UTC) #14
commit-bot: I haz the power
Committed patchset #6 (id:100001)
3 years, 3 months ago (2016-01-27 15:49:43 UTC) #15
commit-bot: I haz the power
3 years, 3 months ago (2016-01-27 15:50:54 UTC) #17
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/4a86c9a04f09c3a0cbf741feb535a2680c1faa79
Cr-Commit-Position: refs/heads/master@{#371792}

Powered by Google App Engine
This is Rietveld 408576698