Issue 1607463004: [css-grid] Allow to place positioned grid items on the padding

Issue 1607463004: [css-grid] Allow to place positioned grid items on the padding (Closed)

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.

More Reviews

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

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+161 lines, -24 lines)			Patch
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-implicit-grid.html	View		2 chunks	+4 lines, -2 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-implicit-grid-expected.txt	View		1 chunk	+16 lines, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html	View	1 2 3	1 chunk	+119 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutGrid.cpp	View	1 2	2 chunks	+22 lines, -21 lines	0 comments	Download

Messages

Total messages: 19 (10 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jfernandez

https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1517 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1517: LayoutUnit end = endIsAuto ? (direction == ForColumns) ? ...

4 years, 11 months ago (2016-01-18 17:57:42 UTC) #4

svillar

lgtm https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html 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/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html#newcode2 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html:2: <html> Don't need this, actually there is no ...

4 years, 11 months ago (2016-01-19 12:04:51 UTC) #5

svillar

BTW let's change the description of the bug. Let's include a brief explanation that describes ...

4 years, 11 months ago (2016-01-19 12:08:48 UTC) #6

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-19 15:11:58 UTC) #10

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-19 15:49:52 UTC) #14

Manuel Rego

Thanks for the reviews. Applied suggested changes. https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1607463004/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1496 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1496: int smallestStart ...

4 years, 11 months ago (2016-01-19 15:52:25 UTC) #15

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.

commit-bot: I haz the power

4 years, 11 months ago (2016-01-19 16:56:20 UTC) #19

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/ee130ce3012b9e82d4182aec50c1944ff9681e1b
Cr-Commit-Position: refs/heads/master@{#370127}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages