New issue
Advanced search Search tips

Issue 773625 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[css-grid] REGRESSION(r501249): Row stretch doesn't work for grid container with min-height

Project Member Reported by r...@igalia.com, Oct 11 2017

Issue description


This was working fine before r501249 ( bug #763386 ):
https://chromium.googlesource.com/chromium/src/+/bb05a60c04d5754764ca5c8f35b7fa0c77fce9e3

Just open the attached example and both grids should have the same output (the item should have 200px height).
However the one with "min-height: 200px" is not stretching.
 
grid-regression-stretch.html
344 bytes View Download

Comment 1 by r...@igalia.com, Oct 13 2017

After analyzing the problem I believe the spec needs to be modified again.
I've posted a new issue to the CSS WG: https://github.com/w3c/csswg-drafts/issues/1866

We could implement this already as it's not something hard,
probably that's better than having a regression.
So next week I'll work on a patch.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e19a13c2e69dcf169267de804cb67a629e190442

commit e19a13c2e69dcf169267de804cb67a629e190442
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Fri Oct 20 16:32:14 2017

[css-grid] Take into account min size constrains for stretching auto tracks

In r501249 we moved the stretch phase as the last step of
the track sizing algorithm.
However this introduced a regression as we were no longer
taking into account the grid container min-width|height constraints
during this step.

The CSS WG modified the spec so it now defines what to do
in these situations (https://drafts.csswg.org/css-grid/#algo-stretch):
  "If the free space is indefinite, but the grid container
   has a definite min-width/height, use that size to calculate
   the free space for this step instead."

This patch adds a new method
GridTrackSizingAlgorithmStrategy::FreeSpaceForStretchAutoTracksStep().
When we're in the DefiniteSizeStrategy it just returns the current
free space. For the IndefiniteSizeStrategy it uses the min size
of the grid container (respecting min-width|height properties)
to calculate the free space.

BUG= 773625 
TEST=external/wpt/css/css-grid-1/layout-algorithm/grid-stretch-respects-min-size-001.html

Change-Id: I8864f9c5885e3caea534c4b69acd93ad28eb5bc9
Reviewed-on: https://chromium-review.googlesource.com/720918
Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#510461}
[add] https://crrev.com/e19a13c2e69dcf169267de804cb67a629e190442/third_party/WebKit/LayoutTests/external/wpt/css/css-grid-1/layout-algorithm/grid-stretch-respects-min-size-001.html
[modify] https://crrev.com/e19a13c2e69dcf169267de804cb67a629e190442/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp
[modify] https://crrev.com/e19a13c2e69dcf169267de804cb67a629e190442/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.h

Comment 3 by r...@igalia.com, Oct 23 2017

Status: Fixed (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2061c449e671b7c8b16bbc7c5b7fbe57c01114d

commit f2061c449e671b7c8b16bbc7c5b7fbe57c01114d
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Tue Oct 31 00:36:05 2017

[css-grid] Remove unneeded code in FreeSpaceForStretchAutoTracksStep()

This is a follow up after r510461. In that patch we added a TODO in
IndefiniteSizeStrategy::FreeSpaceForStretchAutoTracksStep().
This was related to how preferred widths were computed,
that could be missing the min-width information.

However after r511451 removing some wrong code
in LayoutGrid::UpdateBlockLayout(), that was trying to compute
the preferred widths, we can avoid doing any special calculation
for the columns case, as that's already managed
by LayoutBlock::ComputePreferredLogicalWidths().

Basically GridTrackSizing provides a min and max logical widths
that ignore min-width. But that's not an issue as LayoutGrid only
overrides ComputeIntrinsicLogicalWidths(),
then ComputePreferredLogicalWidths() will use that information
together with the min-size and properly compute the preferred widths.

For this reason in FreeSpaceForStretchAutoTracksStep() we don't need
any special computation for columns, the same that happens
in RecomputeUsedFlexFractionIfNeeded().

Added two unit tests to verify that the preferred widths are right
in both cases (auto and flexible tracks with min-width).

Bug= 773625 

Change-Id: I69efeb064fa03685c3e77bca33d294df3e1db79d
Reviewed-on: https://chromium-review.googlesource.com/741505
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Commit-Queue: Manuel Rego Casasnovas <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#512692}
[modify] https://crrev.com/f2061c449e671b7c8b16bbc7c5b7fbe57c01114d/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/f2061c449e671b7c8b16bbc7c5b7fbe57c01114d/third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp

Sign in to add a comment