[css-grid] Fix track sizing algo w/ size restrictions and intrinsic sizes

Source/core/layout/LayoutGrid.cpp

Index: Source/core/layout/LayoutGrid.cpp
diff --git a/Source/core/layout/LayoutGrid.cpp b/Source/core/layout/LayoutGrid.cpp
index 2de38a480b67dc7c3a5bede6d2eb962a899aec5a..882203707b6a259d0e71ed4de92ba61ca83d29e6 100644
--- a/Source/core/layout/LayoutGrid.cpp
+++ b/Source/core/layout/LayoutGrid.cpp
@@ -391,9 +391,32 @@ void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, Layo
     maxLogicalWidth += scrollbarWidth;
 }
 
-bool LayoutGrid::gridElementIsShrinkToFit()
+static bool heightSizedUnderMinContentConstraint(const ComputedStyle& style)
 {
-    return isFloatingOrOutOfFlowPositioned();
+    return style.logicalMinHeight().isMinContent() || style.logicalHeight().isMinContent() || style.logicalMaxHeight().isMinContent();

[Manuel Rego, 2015/09/17 11:04:03]

What happens if it's not you, but it's an ancestor
the one sized under a constraint?

[svillar, 2015/09/17 14:05:44]

max-content
|_min-content
  |_auto <- this is the grid container

am I under min-content or max-content or neither?.

Apart from that, I also decided not to check the ancestors because this specific
behavior is totally different to how "normal" boxes are sized under those
constraints so I think it makes sense to limit it to the grid container.

[Manuel Rego, 2015/09/18 10:52:07]

Sorry but I'm not still 100% sure about this. :-/

The thing is that for width, we take into account
the sizing constraints of the ancestors.

So, with this patch we'll have a different behavior
for widths than for heights
(when the constraint comes from an ancestor).
Example:
http://jsbin.com/wiroyeduro/1/edit?html,css,output

Maybe we should clarify the meaning of
"is sized under a XXX constraint" in the specs.

[svillar, 2015/09/18 12:01:20]

Well in this particular example the fact that height and width are different is
because auto is different. For widths auto means ~ fill the container (as
min-content is 0 then the width is 0) while for heights auto means fit contents
(as the row is 100px tall that's the height of the grid).

[Manuel Rego, 2015/09/22 10:12:34]

My question is why the row is 100px tall and not 0px tall.

For example imagine that we don't have any item:
http://jsbin.com/kusore/1/edit?html,css,output

Sorry but I'm still not sure about all this,
but this kind of checks inside LayoutGrid
make me wonder if we're doing the right thing.

+}
+
+static bool heightSizedUnderMaxContentConstraint(const ComputedStyle& style)
+{
+    return style.logicalMinHeight().isMaxContent() || style.logicalHeight().isMaxContent() || style.logicalMaxHeight().isMaxContent();
+}
+
+void LayoutGrid::maximizeTracks(Vector<GridTrack>& tracks, GridSizingData& sizingData, LayoutUnit& spaceToDistribute)
+{
+    if (spaceToDistribute <= 0)
+        return;
+
+    size_t tracksSize = tracks.size();
+    Vector<GridTrack*> tracksForDistribution(tracksSize);
+    for (size_t i = 0; i < tracksSize; ++i) {
+        tracksForDistribution[i] = tracks.data() + i;
+        tracksForDistribution[i]->setPlannedSize(tracksForDistribution[i]->baseSize());
+    }
+
+    distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, nullptr, sizingData, spaceToDistribute);
+
+    for (auto* track : tracksForDistribution)
+        track->setBaseSize(track->plannedSize());
 }
 
 void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit& freeSpace)
@@ -429,27 +452,39 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
         freeSpace -= track.baseSize();
     }
 
-    const bool hasUndefinedRemainingSpace = (direction == ForRows) ? style()->logicalHeight().isAuto() : gridElementIsShrinkToFit();
-
-    if (!hasUndefinedRemainingSpace && freeSpace <= 0)
+    const bool hasDefiniteAvailableSpace = (direction == ForRows) ? hasDefiniteLogicalHeight() : true;

[jfernandez, 2015/09/01 23:41:39]

I don't get why hasDefiniteAvailableSpace is always true when direction is
ForColumns. I asks this specially regarding the flexible tracks size
computation.

[svillar, 2015/09/02 13:25:48]

It's explained in the comment I added bellow, for the case of columns
computeIntrinsicLogicalWidths() has been already called, so we always get a
valid available space, something that does not happen for heights (as they
depend on widths, 'auto' means a different thing, etc...)

Regarding flex tracks computation I think you have a point. I'd need to double
check that it's still working fine.

[Manuel Rego, 2015/09/17 11:04:02]

Probably the comment should be before the variable itself. :-)

[svillar, 2015/09/17 14:05:44]

Acknowledged.

+    if (hasDefiniteAvailableSpace && freeSpace <= 0)
         return;
 
     // 3. Grow all Grid tracks in GridTracks from their baseSize up to their growthLimit value until freeSpace is exhausted.
-    const size_t tracksSize = tracks.size();
-    if (!hasUndefinedRemainingSpace) {
-        Vector<GridTrack*> tracksForDistribution(tracksSize);
-        for (size_t i = 0; i < tracksSize; ++i) {
-            tracksForDistribution[i] = tracks.data() + i;
-            tracksForDistribution[i]->setPlannedSize(tracksForDistribution[i]->baseSize());
-        }
-
-        distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, nullptr, sizingData, freeSpace);
-
-        for (auto* track : tracksForDistribution)
-            track->setBaseSize(track->plannedSize());
+    // We only need to check for the interactions of the track sizing algorithm with intrinsic sizes and min/max size
+    // restrictions **for rows**, as for columns we already have the right available space because the intrinsic sizes
+    // were previously computed by computeIntrinsicLogicalWidths()
+    if (direction == ForColumns) {
+        maximizeTracks(tracks, sizingData, freeSpace);
     } else {
-        for (auto& track : tracks)
-            track.setBaseSize(track.growthLimit());
+        if (heightSizedUnderMaxContentConstraint(styleRef())) {
+            if (styleRef().logicalMaxHeight().isMaxSizeNone()) {
+                for (auto& track : tracks)
+                    track.setBaseSize(track.growthLimit());
+            } else {
+                LayoutUnit tracksBreadth = initialFreeSpace - freeSpace;
+                LayoutUnit spaceToDistribute = computeLogicalHeightUsing(MaxSize, styleRef().logicalMaxHeight(), contentLogicalHeight()) - tracksBreadth;
+                maximizeTracks(tracks, sizingData, spaceToDistribute);
+            }
+        } else if (heightSizedUnderMinContentConstraint(styleRef())) {
+            const Length& logicalMinSize = styleRef().logicalMinHeight();
+            if (!logicalMinSize.isZero() || logicalMinSize.isIntrinsic()) {
+                LayoutUnit tracksBreadth = initialFreeSpace - freeSpace;
+                LayoutUnit spaceToDistribute = computeLogicalHeightUsing(MinSize, styleRef().logicalMinHeight(), contentLogicalHeight()) - tracksBreadth;
+                maximizeTracks(tracks, sizingData, spaceToDistribute);
+            }
+        } else if (hasDefiniteAvailableSpace) {
+            maximizeTracks(tracks, sizingData, freeSpace);
+        } else {
+            for (auto& track : tracks)
+                track.setBaseSize(track.growthLimit());
+        }

[Manuel Rego, 2015/09/17 11:04:03]

In order to try to simplify this "if-else",
would it be possible to modify the freeSpace variable
and always call maximizeTracks()?

[svillar, 2015/09/17 14:05:44]

If you mean the whole if-else thing the answer is no, because as you can see
there are some branches that lead to no track maximization (like being under
min-content and min-size being a fixed length).

If you mean the setBaseSize(growthLimit()) ones then the answer is, yes it can
be done, but setting the base size to the growth limit directly is much more
efficient than distributing that free space.

     }
 
     if (flexibleSizedTracksIndex.isEmpty())
@@ -457,7 +492,7 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
 
     // 4. Grow all Grid tracks having a fraction as the MaxTrackSizingFunction.
     double normalizedFractionBreadth = 0;
-    if (!hasUndefinedRemainingSpace) {
+    if (hasDefiniteAvailableSpace) {
         normalizedFractionBreadth = computeNormalizedFractionBreadth(tracks, GridSpan(0, tracks.size() - 1), direction, initialFreeSpace);
     } else {
         for (const auto& trackIndex : flexibleSizedTracksIndex) {