[css-grid] Handle min-content/max-content with orthogonal flows

Source/core/rendering/RenderGrid.cpp

Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp b/Source/core/rendering/RenderGrid.cpp
index e4628969db7ef4037f9ddaf53914562ae6114bc8..75b241282ec7827eaee305a788f845c76210930f 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -610,30 +610,61 @@ GridTrackSize RenderGrid::gridTrackSize(GridTrackSizingDirection direction, size
     return trackSize;
 }
 
-LayoutUnit RenderGrid::logicalHeightForChild(RenderBox& child, Vector<GridTrack>& columnTracks)
+LayoutUnit RenderGrid::overrideContainingBlockLogicalBreadthInlineDirection(const RenderBox& child) const
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    if (hasOrthogonalWritingMode)
+        return child.hasOverrideContainingBlockLogicalHeight() ? child.overrideContainingBlockContentLogicalHeight() : LayoutUnit();
+    return child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
+}
+
+void RenderGrid::setOverrideContainingBlockLogicalBreadthInlineDirection(RenderBox& child, LayoutUnit breadth)
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    if (hasOrthogonalWritingMode)
+        child.setOverrideContainingBlockContentLogicalHeight(breadth);
+    else
+        child.setOverrideContainingBlockContentLogicalWidth(breadth);
+    // If |child| has a percentage logical height, we shouldn't let it override its intrinsic height, which is
+    // what we are interested in here. Thus we need to set the override logical height/width to -1 (no possible resolution).
+    if (child.style()->logicalHeight().isPercent()) {
+        if (hasOrthogonalWritingMode)
+            child.setOverrideContainingBlockContentLogicalWidth(-1);
+        else
+            child.setOverrideContainingBlockContentLogicalHeight(-1);

[Julien - ping for review, 2015/02/18 23:00:07]

We do 2 hash lookups in the percentage logical height case. We can avoid it by
doing:

LayoutUnit logicalBreadthInlineDirection =
child.style()->logicalHeight().isPercent() ? -1 : breadth;

[jfernandez, 2015/02/23 18:51:58]

Actually we don't; we need to set both, xxxLogicalHeight and xxxlogicalWidth.
However, My patch assumes that setting -1 (no possible resolution) was only
useful in the percentage logical height case. In the original code we were
setting -1 in all the cases.

I think it has an effect anyway, as it will be used in the layoutGridItems
later, so I'll modify the patch to keep the previous behavior. I have to admit
that I have several doubts about the percentage height case; rego and me have
been discussing about it and he shares my doubts. Actually, removing such
condition does not make any test to fail, which is weird. 

+    }
+}
+
+LayoutUnit RenderGrid::gridAreaBreadthForChildInlineDirection(const RenderBox& child, Vector<GridTrack>& tracksInlineDirection) const

[Julien - ping for review, 2015/02/18 23:00:07]

This name is confusing and I still am unsure I understand the meaning of 'inline
direction' as meant by this function.

[jfernandez, 2015/02/23 18:51:58]

I took the name inspired by other methods like computeLogicalWidth, where the
concept InlineDirection is used in some variables.

LayoutUnit containerWidthInInlineDirection = containerLogicalWidth;
if (hasPerpendicularContainingBlock)
    containerWidthInInlineDirection =
perpendicularContainingBlockLogicalHeight();

The idea is that the breadth InlineDirection is the one causing the
blockDirection to be adapted to the new dimensions. This breadth can be either
logicalWidth or logicalHeight depending on being under an orthogonal flows or
not. The fact of being vertical or horizontal modes does not affect, that's why
we are using logical dimensions; it just matter whether it's an orthogonal flow
or not. 

I have to admit that all of this is a bit confusing to me, so I might be
misunderstanding some concepts, but that's what I could get so far. 

Perhaps now that you understand my idea, if that's the case :), we could define
a better name for these functions.

+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    if (hasOrthogonalWritingMode)
+        return gridAreaBreadthForChild(child, ForRows, tracksInlineDirection);
+    return gridAreaBreadthForChild(child, ForColumns, tracksInlineDirection);
+}
+
+
+LayoutUnit RenderGrid::logicalHeightForChild(RenderBox& child, Vector<GridTrack>& tracksInlineDirection)
 {
     SubtreeLayoutScope layoutScope(child);
-    LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
-    LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, columnTracks);
-    if (child.style()->logicalHeight().isPercent() || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth)
+    LayoutUnit oldOverrideContainingBlockContentLogicalBreadthInlineDirection = overrideContainingBlockLogicalBreadthInlineDirection(child);
+    LayoutUnit overrideContainingBlockContentLogicalBreadthInlineDirection = gridAreaBreadthForChildInlineDirection(child, tracksInlineDirection);
+    if (child.style()->logicalHeight().isPercent() || oldOverrideContainingBlockContentLogicalBreadthInlineDirection != overrideContainingBlockContentLogicalBreadthInlineDirection)
         layoutScope.setNeedsLayout(&child);
 
+    // Needed to properly implement the stretch logic.
     child.clearOverrideLogicalContentHeight();
 
-    child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
-    // If |child| has a percentage logical height, we shouldn't let it override its intrinsic height, which is
-    // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
-    child.setOverrideContainingBlockContentLogicalHeight(-1);
+    setOverrideContainingBlockLogicalBreadthInlineDirection(child, overrideContainingBlockContentLogicalBreadthInlineDirection);
     child.layoutIfNeeded();
     return child.logicalHeight() + child.marginLogicalHeight();
 }
 
-LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInlineDirection)

[Julien - ping for review, 2015/02/18 23:00:07]

In correct English, this would be tracksInInlineDirection.

I don't understand what's wrong with using column / row direction insofar as it
is clear we are talking about the grid container's direction.

[jfernandez, 2015/02/23 18:51:58]

That s right. However, I saw source code using just Inline, where other
functions and variables are using the preposition as well. I thought it was
clearer the shorter name, but I had doubts too.
What makes me think that column / row concepts are not what we are looking for
is that we use either column or row tracks for computing the min content based
on both, container's and item's direction; whether they are orthogonal or not.
In case of parallel flows we always use the column tracks to determine the row
tracks breadth, independently of the container's direction. 

 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
     if (hasOrthogonalWritingMode)
-        return 0;
+        direction = direction == ForColumns ? ForRows : ForColumns;
 
     if (direction == ForColumns) {
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
@@ -641,15 +672,14 @@ LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirec
         return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, tracksInlineDirection);
 }
 
-LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInlineDirection)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
     if (hasOrthogonalWritingMode)
-        return LayoutUnit();
+        direction = direction == ForColumns ? ForRows : ForColumns;
 
     if (direction == ForColumns) {
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
@@ -657,7 +687,7 @@ LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirec
         return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, tracksInlineDirection);
 }
 
 // We're basically using a class instead of a std::pair for two reasons. First of all, accessing gridItem() or
@@ -716,10 +746,11 @@ void RenderGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
         GridIterator iterator(m_grid, direction, trackIndex);
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
         while (RenderBox* gridItem = iterator.nextGridItem()) {
+            bool hasOrthogonalWritingMode = gridItem->isHorizontalWritingMode() != isHorizontalWritingMode();
             if (itemsSet.add(gridItem).isNewEntry) {
                 const GridCoordinate& coordinate = cachedGridCoordinate(*gridItem);
                 if (integerSpanForDirection(coordinate, direction) == 1) {
-                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, coordinate, *gridItem, track, sizingData.columnTracks);
+                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, coordinate, *gridItem, track, hasOrthogonalWritingMode ? sizingData.rowTracks : sizingData.columnTracks);
                 } else if (!spanningItemCrossesFlexibleSizedTracks(coordinate, direction)) {
                     sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, coordinate, direction));
                 }
@@ -744,20 +775,20 @@ void RenderGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
     }
 }
 
-void RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridCoordinate& coordinate, RenderBox& gridItem, GridTrack& track, Vector<GridTrack>& columnTracks)
+void RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridCoordinate& coordinate, RenderBox& gridItem, GridTrack& track, Vector<GridTrack>& tracksInlineDirection)
 {
     const GridResolvedPosition trackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
     GridTrackSize trackSize = gridTrackSize(direction, trackPosition.toInt());
 
     if (trackSize.hasMinContentMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, tracksInlineDirection)));
     else if (trackSize.hasMaxContentMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, tracksInlineDirection)));
 
     if (trackSize.hasMinContentMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, tracksInlineDirection)));
     else if (trackSize.hasMaxContentMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, tracksInlineDirection)));
 }
 
 void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, GridItemWithSpan& gridItemWithSpan, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, FilterFunction growAboveMaxBreadthFilterFunction)
@@ -787,7 +818,8 @@ void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
 
     // Specs mandate to floor extraSpace to 0. Instead we directly avoid the function call in those cases as it will be
     // a noop in terms of track sizing.
-    LayoutUnit extraSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks) - spanningTracksSize;
+    bool hasOrthogonalWritingMode = gridItemWithSpan.gridItem().isHorizontalWritingMode() != isHorizontalWritingMode();
+    LayoutUnit extraSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, hasOrthogonalWritingMode ? sizingData.rowTracks : sizingData.columnTracks) - spanningTracksSize;
     if (extraSpace > 0) {
         Vector<GridTrack*>* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
         distributeSpaceToTracks(sizingData.filteredTracks, tracksToGrowBeyondGrowthLimits, trackGetter, trackGrowthFunction, sizingData, extraSpace);
@@ -1079,6 +1111,22 @@ void RenderGrid::dirtyGrid()
     m_gridItemsIndexesMap.clear();
 }
 
+bool RenderGrid::childOverflowingContainingBlockHeight(const RenderBox& child) const
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    LayoutUnit overrideContainingBlockContentLogicalHeight = child.hasOverrideContainingBlockLogicalHeight() ? child.overrideContainingBlockContentLogicalHeight() : LayoutUnit();
+
+    return hasOrthogonalWritingMode ? child.logicalWidth() > overrideContainingBlockContentLogicalHeight : child.logicalHeight() > overrideContainingBlockContentLogicalHeight;
+}

[Julien - ping for review, 2015/02/18 23:00:07]

This seems super complicated to me. We don't *need* to do this computation in
logical coordinates and it should be easier in physical ones.

// You need to implement the trivial physical function for the override
containing block sizes.
LayoutUnit containingBlockContentHeight =
child.hasOverrideContainingBlockHeight() ?
child.overrideContainingBlockContentHeight() : LayoutUnit();
return child.height() > containingBlockContentHeight;

[jfernandez, 2015/02/23 18:51:58]

Oh, you are so right. I'll implement it that way in the next patch.

+
+bool RenderGrid::childOverflowingContainingBlockWidth(const RenderBox& child) const
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    LayoutUnit overrideContainingBlockContentLogicalWidth = child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
+
+    return hasOrthogonalWritingMode ? child.logicalHeight() > overrideContainingBlockContentLogicalWidth : child.logicalHeight() > overrideContainingBlockContentLogicalWidth;
+}
+
 void RenderGrid::layoutGridItems()
 {
     placeItemsOnGrid();
@@ -1086,10 +1134,27 @@ void RenderGrid::layoutGridItems()
     LayoutUnit availableSpaceForColumns = availableLogicalWidth();
     LayoutUnit availableSpaceForRows = availableLogicalHeight(IncludeMarginBorderPadding);
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
+    // 1- First, the track sizing algorithm is used to resolve the sizes of the grid columns.
     computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
+    // 2- Next, the track sizing algorithm resolves the sizes of the grid rows, using the
+    // grid column sizes calculated in the previous step.
     computeUsedBreadthOfGridTracks(ForRows, sizingData, availableSpaceForRows);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
+    // 3- If the min-content contribution of any grid items have changed based on the row
+    // sizes calculated in step 2, steps 1 and 2 are repeated with the new min-content
+    // contribution (once only).
+    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != isHorizontalWritingMode();
+        // In orthogonal flow cases column tracks size is determined based on the computed
+        // row track sizes, hence we need to repeat computeUsedBreadthOfGridTracks for columns.
+        if (hasOrthogonalWritingMode) {
+            availableSpaceForColumns = availableLogicalWidth();
+            computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
+            ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
+            break;
+        }
+    }
 
     populateGridPositions(sizingData, availableSpaceForColumns, availableSpaceForRows);
     m_gridItemsOverflowingGridArea.resize(0);
@@ -1135,8 +1200,8 @@ void RenderGrid::layoutGridItems()
 
         // Keep track of children overflowing their grid area as we might need to paint them even if the grid-area is
         // not visible
-        if (child->logicalHeight() > overrideContainingBlockContentLogicalHeight
-            || child->logicalWidth() > overrideContainingBlockContentLogicalWidth)
+        if (childOverflowingContainingBlockHeight(*child)
+            || childOverflowingContainingBlockWidth(*child))
             m_gridItemsOverflowingGridArea.append(child);
     }
 
@@ -1711,7 +1776,12 @@ LayoutPoint RenderGrid::findChildLogicalPosition(const RenderBox& child, LayoutS
     LayoutPoint childLocation(columnPosition, rowPositionForChild(child));
     childLocation.move(contentAlignmentOffset);
 
-    return childLocation;
+    // "In the positioning phase [...] calculations are performed according to the writing mode
+    // of the containing block of the box establishing the orthogonal flow." However, the
+    // resulting LayoutPoint will be used in 'setLogicalPosition' in order to set the child's
+    // logical position, which will only take into account the child's writing-mode.
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    return hasOrthogonalWritingMode ? childLocation.transposedPoint() : childLocation;
 }
 
 void RenderGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)