[css-grid] Allow percentage values for column and row gutters

[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.

BUG=615248

[jfernandez, 2016-06-22 11:15:53]

Description was changed from

==========
[css-grid] Allow percentage values for column and row tracks

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.
==========

to

==========
[css-grid] Allow percentage values for column and row tracks

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.

BUG=615248
==========

[jfernandez, 2016-06-22 11:15:53]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com

[jfernandez, 2016-06-22 11:16:32]

Description was changed from

==========
[css-grid] Allow percentage values for column and row tracks

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.

BUG=615248
==========

to

==========
[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.

BUG=615248
==========

[cbiesinger, 2016-06-22 18:46:13]

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:845:
LayoutUnit gutterSize = grid->guttersSize(direction, 2, isRowAxis ?
grid->logicalWidth() : grid->logicalHeight());
The spec says they refer to the content area, so shouldn't this be
contentLogical{Width,Height}?

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3701: return
consumeLengthOrPercent(m_range, m_context.mode(), ValueRangeNonNegative,
UnitlessQuirk::Allow);
Why do you want to allow unitless numbers, particularly when you didn't use to?
Same question below.

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:252: void
setTrackGapsForDirection(GridTrackSizingDirection direction, LayoutUnit gap) {
direction == ForColumns ? gapsForColumns = gap : gapsForRows = gap; }
Please don't use the ternary operator instead of an if when there's side-effects
in the expressions like here

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:411: LayoutUnit
availableHeight = availableLogicalHeight(ExcludeMarginBorderPadding);
Does this code do something reasonable with an unspecified height?

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1761: end -=
guttersSize(direction, 2, isForColumns ? logicalWidth() :
logicalHeightWasIndefinite ? LayoutUnit() : logicalHeight());
Again, I think this needs to be contentLogicalHeight or availableLogicalHeight
(and width).

Instead of using the logicalHeightWasIndefinite, why not make a LayoutUnit
availableWidthToUse that stores the result of this check? Nested ternary
expressions are hard to read.

[jfernandez, 2016-06-27 23:56:48]

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:845:
LayoutUnit gutterSize = grid->guttersSize(direction, 2, isRowAxis ?
grid->logicalWidth() : grid->logicalHeight());
On 2016/06/22 18:46:13, cbiesinger wrote:
> The spec says they refer to the content area, so shouldn't this be
> contentLogical{Width,Height}?

Done.

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3701: return
consumeLengthOrPercent(m_range, m_context.mode(), ValueRangeNonNegative,
UnitlessQuirk::Allow);
On 2016/06/22 18:46:13, cbiesinger wrote:
> Why do you want to allow unitless numbers, particularly when you didn't use
to?
> Same question below.

I did allow unitless numbers that for consistency with <track-breadth> [1], used
for grid-columns and grid-rows definition. But I guess it's debatable. 

[1] https://drafts.csswg.org/css-grid/#typedef-track-breadth

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:252: void
setTrackGapsForDirection(GridTrackSizingDirection direction, LayoutUnit gap) {
direction == ForColumns ? gapsForColumns = gap : gapsForRows = gap; }
On 2016/06/22 18:46:13, cbiesinger wrote:
> Please don't use the ternary operator instead of an if when there's
side-effects
> in the expressions like here

Acknowledged.

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:411: LayoutUnit
availableHeight = availableLogicalHeight(ExcludeMarginBorderPadding);
On 2016/06/22 18:46:13, cbiesinger wrote:
> Does this code do something reasonable with an unspecified height?

This branch of code supposes height is "definite", hence specified.

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1761: end -=
guttersSize(direction, 2, isForColumns ? logicalWidth() :
logicalHeightWasIndefinite ? LayoutUnit() : logicalHeight());
On 2016/06/22 18:46:13, cbiesinger wrote:
> Again, I think this needs to be contentLogicalHeight or availableLogicalHeight
> (and width).
> 
> Instead of using the logicalHeightWasIndefinite, why not make a LayoutUnit
> availableWidthToUse that stores the result of this check? Nested ternary
> expressions are hard to read.

Done.

[Timothy Loh, 2016-06-28 00:04:45]

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3701: return
consumeLengthOrPercent(m_range, m_context.mode(), ValueRangeNonNegative,
UnitlessQuirk::Allow);
On 2016/06/27 23:56:47, jfernandez wrote:
> On 2016/06/22 18:46:13, cbiesinger wrote:
> > Why do you want to allow unitless numbers, particularly when you didn't use
> to?
> > Same question below.
> 
> I did allow unitless numbers that for consistency with <track-breadth> [1],
used
> for grid-columns and grid-rows definition. But I guess it's debatable. 
> 
> [1] https://drafts.csswg.org/css-grid/#typedef-track-breadth

<track-breadth> doesn't support the unitless quirk

https://quirks.spec.whatwg.org/#the-unitless-length-quirk

[jfernandez, 2016-06-28 00:15:19]

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/2057113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3701: return
consumeLengthOrPercent(m_range, m_context.mode(), ValueRangeNonNegative,
UnitlessQuirk::Allow);
On 2016/06/28 00:04:45, Timothy Loh wrote:
> On 2016/06/27 23:56:47, jfernandez wrote:
> > On 2016/06/22 18:46:13, cbiesinger wrote:
> > > Why do you want to allow unitless numbers, particularly when you didn't
use
> > to?
> > > Same question below.
> > 
> > I did allow unitless numbers that for consistency with <track-breadth> [1],
> used
> > for grid-columns and grid-rows definition. But I guess it's debatable. 
> > 
> > [1] https://drafts.csswg.org/css-grid/#typedef-track-breadth
> 
> <track-breadth> doesn't support the unitless quirk
> 
> https://quirks.spec.whatwg.org/#the-unitless-length-quirk

Done.

I'll add a TODO to change the track-breadth parsing then.

[Manuel Rego, 2016-07-01 06:45:57]

Nice patch and really cool test!

I think we still need some more work, added comments inline.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html
(right):

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:79:
<p>Height is indefinte, so row gaps should be 0. Column gaps based on a grid's
'intrinsic' width, which reduce the available space for tracks; however, they
are fixed sized and don't fit so thet overflow the grid container.</p>
Typo: "thet"

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:119:
<p>Both row and column gaps are based on definite sizes, they will reduce the
available space for tracks; however, they are fixed sized and don't fit so thet
overflow the grid container.</p>
Typo: "thet"

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:155:
<div class="secondRowSecondColumn" data-offset-x="140" data-offset-y="90"
data-expected-width="100" data-expected-height="50">XXXXX X XX</div>
Nit: I'd add items on a 3rd column and row to check that repetitions are working
fine.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-as-percentage.html:179:
<p>Grid with positioned items. Both row and column gaps are based on definite
sizes, they will reduce the available space for tracks; however, they are fixed
sized and don't fit so thet overflow the grid container.</p>
Typo: "thet"

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-get-set.html
(right):

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-gutters-get-set.html:7:
.gridSimpleGap { grid-gap: 25px; }
I'd add some cases here (for the grid-gap shorthand) using percentages too.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:538: LayoutUnit
totalGuttersSize = sizingData.guttersSize(ForColumns,
sizingData.columnTracks.size());
I think you've just calculated the gap size a few lines above,
we can reuse it here.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:555: LayoutUnit
totalGuttersSize = sizingData.guttersSize(ForRows, gridRowCount());
Ditto.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1832: bool
logicalHeightWasIndefinite = computeContentLogicalHeight(MainOrPreferredSize,
style()->logicalHeight(), LayoutUnit(-1)) == LayoutUnit(-1);
We know this check is wrong, but it's part of bug #624301 to get it fixed.

Anyway we're now using this check (with your patch) in 3 places:
1) LayoutGrid::layoutBlock()
2) LayoutGrid::computeAutoRepeatTracksCount()
3) Here :-)

Maybe it's worth to move it to a boolean that we can reuse.
Despite it being wrong.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1846: end -=
guttersSize(direction, 2, isForColumns ? logicalWidth() :
logicalHeightWasIndefinite ? LayoutUnit() : contentLogicalHeight());
This is weird, you're using here logicalWidth() and contentLogicalHeight(),
in other parts you're using availableLogicalWidth() and
availableLogicalHeight().

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1863: offset +=
guttersSize(direction, 2, isForColumns ? logicalWidth() :
logicalHeightWasIndefinite ? LayoutUnit() : contentLogicalHeight());
Ditto.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1898: gridAreaSize +=
guttersSize(ForRows, span.integerSpan(), LayoutUnit());
I'm not sure if this call is right, if the sizingOperation is "TrackSizing"
we might need to use the availableLogicalHeight() instead of 0.

Didn't debug or test it so I don't know if it's possible to call this
with that sizingOperation or not.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.h:74: LayoutUnit
guttersSize(GridTrackSizingDirection, size_t span, LayoutUnit availableSize)
const;
I'm not really convinced about this API, specially it being public.

From the outside you shouldn't need to pass the avilableSize,
it should always use the availableLogicalWidth|Height() to determine
the gutter.

On top of that, we're already passing different values here
(contentLogicalWidth(),
availableLogicalWidth(), logicalWidth()). And this is just the initial patch,
we can pass more things in the future.

I really believe we should be able to do it internally, my approach would be
passing
the SizingOperation (actually in the public method don't pass anything
as it'd always be TrackSizing):
A) If we're computing intrinsic sizes the availableSize is always 0.
B) If we're in the track sizing phase, for columns the availableSize is
availableLogicalWidth()
   for rows, if the height of the grid container is indefinite it's 0,
  otherwise it's the availableLogicalHeight().

In most cases we'll be using the B) so that could be the default.

That way we avoid to pass this size over and over.

[svillar, 2016-07-13 09:08:08]

I'd prefer to review it after landing auto-fit as things are going to change a
little bit.

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/2057113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.h:74: LayoutUnit
guttersSize(GridTrackSizingDirection, size_t span, LayoutUnit availableSize)
const;
On 2016/07/01 06:45:57, Manuel Rego wrote:
> I'm not really convinced about this API, specially it being public.

FWIW I'm making it private in the auto-fit patch.

[Manuel Rego, 2016-09-09 05:45:03]

We should close this CL as a new version has already landed:
https://codereview.chromium.org/2323793002/