[css-grid] Constrain by min-height on auto-repeat computation

[css-grid] Constrain by min-height on auto-repeat computation

The max-height (if definite) is used to compute the number of auto repeat
rows whenever the height is indefinite. We were using the min-height only in
case both values were indefinite.

However, it's reasonable to assume that the min-height trumps the used value
of height/max-height like it always does, per CSS 2.2. Note that the number
of rows still needs to fit within that size even if using min-height,
because we're just using min-height to compute the used value of the height
property. If both height and max-height are indefinite we keep doing the
same, i.e., compute the minimum number of rows that fulfill min-height (if
definite).

BUG=648814
Committed: https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780
Cr-Commit-Position: refs/heads/master@{#425327}

[svillar, 2016-10-13 19:11:19]

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

[svillar, 2016-10-13 19:11:19]

Adding reviewers

[jfernandez, 2016-10-13 21:21:08]

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1798: }
Why the availableLogicalHeightForPercentageComputation value, if valid, does not
need to be constrained between min/max limits ?

[Manuel Rego, 2016-10-14 08:10:20]

The patch LGTM but let's wait for the answer to @jfernandez question.

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html
(right):

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html:69:
</div>
Wouldn't be nice to have the same tests for columns?
I know they're working right now, but I guess we don't have
test coverage for them.

Also, it might be also nice to have some cases where
height is not "auto".

[svillar, 2016-10-14 12:46:37]

Thanks for both reviews.

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html
(right):

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html:69:
</div>
On 2016/10/14 08:10:19, Manuel Rego wrote:
> Wouldn't be nice to have the same tests for columns?
> I know they're working right now, but I guess we don't have
> test coverage for them.

OK I can add them too.
 
> Also, it might be also nice to have some cases where
> height is not "auto".

Well the key is to have indefinite heights, I might have used an intrinsic
height but it would be essentially the same.

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2421593002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1798: }
On 2016/10/13 21:21:08, jfernandez wrote:
> Why the availableLogicalHeightForPercentageComputation value, if valid, does
not
> need to be constrained between min/max limits ?

Because that method already does it while availableLogicalHeightUsing() does
not.

[jfernandez, 2016-10-14 13:30:15]

Thanks for the reply. The patch LGTM.

[svillar, 2016-10-14 13:38:33]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-10-14 13:38:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 15:25:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-14 15:27:16]

Description was changed from

==========
[css-grid] Constrain by min-height on auto-repeat computation

The max-height (if definite) is used to compute the number of auto repeat
rows whenever the height is indefinite. We were using the min-height only in
case both values were indefinite.

However, it's reasonable to assume that the min-height trumps the used value
of height/max-height like it always does, per CSS 2.2. Note that the number
of rows still needs to fit within that size even if using min-height,
because we're just using min-height to compute the used value of the height
property. If both height and max-height are indefinite we keep doing the
same, i.e., compute the minimum number of rows that fulfill min-height (if
definite).

BUG=648814
==========

to

==========
[css-grid] Constrain by min-height on auto-repeat computation

The max-height (if definite) is used to compute the number of auto repeat
rows whenever the height is indefinite. We were using the min-height only in
case both values were indefinite.

However, it's reasonable to assume that the min-height trumps the used value
of height/max-height like it always does, per CSS 2.2. Note that the number
of rows still needs to fit within that size even if using min-height,
because we're just using min-height to compute the used value of the height
property. If both height and max-height are indefinite we keep doing the
same, i.e., compute the minimum number of rows that fulfill min-height (if
definite).

BUG=648814

Committed: https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780
Cr-Commit-Position: refs/heads/master@{#425327}
==========

[commit-bot: I haz the power, 2016-10-14 15:27:17]

Patchset 1 (id:??) landed as
https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780
Cr-Commit-Position: refs/heads/master@{#425327}