Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 11 months ago by Manuel Rego Modified:
4 years, 11 months ago Reviewers:
cbiesinger, svillar, jfernandez 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Allow to place positioned grid items on the padding
According to the following discussion on the CSS WG mailing list,
we should be able to place positioned grid items on the padding directly:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html
This means that a positioned grid item can be placed on the padding itself.
The "auto" value resolves to the padding edges (0th and -0th lines).
So if a positioned item is placed with: grid-column: auto / 1;
it'd be placed on the padding, from line 0th to 1st line.
BUG=273898
TEST=fast/css-grid-layout/grid-positioned-items-padding.html
Committed: https://crrev.com/ee130ce3012b9e82d4182aec50c1944ff9681e1b
Cr-Commit-Position: refs/heads/master@{#370127}
Patch Set 1 #Patch Set 2 : Fix test #
Total comments: 9
Patch Set 3 : Applying suggested changes #Patch Set 4 : Forgot to remove <html> from test #
Created: 4 years, 11 months ago
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Allow to position positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This patch modifies LayoutGrid::offsetAndBreadthForPositionedChild() to manage properly those cases. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ========== to ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This patch modifies LayoutGrid::offsetAndBreadthForPositionedChild() to manage properly those cases. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ==========
rego@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com
https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1517: LayoutUnit end = endIsAuto ? (direction == ForColumns) ? logicalWidth() : logicalHeight() : (direction == ForColumns) ? m_columnPositions[finalPosition] : m_rowPositions[finalPosition]; I'd simplify this line, if possible.
lgtm https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html:2: <html> Don't need this, actually there is no </html> https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1496: int smallestStart = abs(direction == ForColumns ? m_smallestColumnStart : m_smallestRowStart); I'm starting to do bool isRowAxis = direction == ForColumns and then use it all along the code. I think we can do it here as well, as you're anyway touching lines with that comparison. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1498: int resolvedFinalPosition = positions.untranslatedResolvedFinalPosition() + smallestStart; This manual translation requires some explanation in a comment. Also, as we're translating perhaps we should use size_t for those variables. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1502: int lastTrackIndex = (direction == ForColumns ? gridColumnCount() : gridRowCount()); Perhaps we should use this change to rename this to lastExplicitTrackIndex in order to make it clear that positioned items ignore implicit position
BTW let's change the description of the bug. Let's include a brief explanation that describes the issue (it isn't clear that we're talking about items that are positioned *only* in the padding of the *grid container*). Also there is no much point in the second paragraph describing the function that is modified. I prefer a better description of the issue which allows you just to read the git log instead of having to actually look a the diff.
Description was changed from ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This patch modifies LayoutGrid::offsetAndBreadthForPositionedChild() to manage properly those cases. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ========== to ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This means that a positioned grid item can be placed on the padding itself. The "auto" value resolves to the padding edges (0th and -0th lines). So if a positioned item is placed with: grid-column: auto / 1; it'd be placed on the padding, from line 0th to 1st line. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ==========
The CQ bit was checked by rego@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com Link to the patchset: https://codereview.chromium.org/1607463004/#ps40001 (title: "Applying suggested changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607463004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607463004/40001
The CQ bit was unchecked by rego@igalia.com
The CQ bit was checked by rego@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com Link to the patchset: https://codereview.chromium.org/1607463004/#ps60001 (title: "Forgot to remove <html> from test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1607463004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1607463004/60001
Thanks for the reviews. Applied suggested changes. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1496: int smallestStart = abs(direction == ForColumns ? m_smallestColumnStart : m_smallestRowStart); On 2016/01/19 12:04:51, svillar wrote: > I'm starting to do > > bool isRowAxis = direction == ForColumns > > and then use it all along the code. I think we can do it here as well, as you're > anyway touching lines with that comparison. Done. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1498: int resolvedFinalPosition = positions.untranslatedResolvedFinalPosition() + smallestStart; On 2016/01/19 12:04:51, svillar wrote: > This manual translation requires some explanation in a comment. Added comment. > Also, as we're translating perhaps we should use size_t for those variables. But that's precisely the issue, that for abspos items even after translated we could have negative values, so we need to use ints. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1502: int lastTrackIndex = (direction == ForColumns ? gridColumnCount() : gridRowCount()); On 2016/01/19 12:04:51, svillar wrote: > Perhaps we should use this change to rename this to lastExplicitTrackIndex in > order to make it clear that positioned items ignore implicit position Done. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1517: LayoutUnit end = endIsAuto ? (direction == ForColumns) ? logicalWidth() : logicalHeight() : (direction == ForColumns) ? m_columnPositions[finalPosition] : m_rowPositions[finalPosition]; On 2016/01/18 17:57:42, jfernandez wrote: > I'd simplify this line, if possible. Now that we use isForColumns I think it's not that bad. We could use if/else lines if needed.
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This means that a positioned grid item can be placed on the padding itself. The "auto" value resolves to the padding edges (0th and -0th lines). So if a positioned item is placed with: grid-column: auto / 1; it'd be placed on the padding, from line 0th to 1st line. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ========== to ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This means that a positioned grid item can be placed on the padding itself. The "auto" value resolves to the padding edges (0th and -0th lines). So if a positioned item is placed with: grid-column: auto / 1; it'd be placed on the padding, from line 0th to 1st line. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This means that a positioned grid item can be placed on the padding itself. The "auto" value resolves to the padding edges (0th and -0th lines). So if a positioned item is placed with: grid-column: auto / 1; it'd be placed on the padding, from line 0th to 1st line. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html ========== to ========== [css-grid] Allow to place positioned grid items on the padding According to the following discussion on the CSS WG mailing list, we should be able to place positioned grid items on the padding directly: https://lists.w3.org/Archives/Public/www-style/2015Nov/0070.html This means that a positioned grid item can be placed on the padding itself. The "auto" value resolves to the padding edges (0th and -0th lines). So if a positioned item is placed with: grid-column: auto / 1; it'd be placed on the padding, from line 0th to 1st line. BUG=273898 TEST=fast/css-grid-layout/grid-positioned-items-padding.html Committed: https://crrev.com/ee130ce3012b9e82d4182aec50c1944ff9681e1b Cr-Commit-Position: refs/heads/master@{#370127} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee130ce3012b9e82d4182aec50c1944ff9681e1b Cr-Commit-Position: refs/heads/master@{#370127} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
