[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 1ba30a4c7d752b6d879c9bc5a22fc0ab6632938d..826dab4bb5b5246deb48f7d986fac0959e82324d 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -608,12 +608,29 @@ LayoutUnit RenderGrid::logicalHeightForChild(RenderBox& child, Vector<GridTrack>
     return child.logicalHeight() + child.marginLogicalHeight();
 }
 
-LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit RenderGrid::logicalHeightForOrthogonalChild(RenderBox& child, Vector<GridTrack>& rowTracks)
+{
+    SubtreeLayoutScope layoutScope(child);
+    LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child.hasOverrideContainingBlockLogicalHeight() ? child.overrideContainingBlockContentLogicalHeight() : LayoutUnit();
+    LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(child, ForRows, rowTracks);
+    if (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight)
+        layoutScope.setNeedsLayout(&child);
+
+    child.clearOverrideLogicalContentHeight();
+
+    child.setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
+    child.layoutIfNeeded();
+    return child.logicalHeight() + child.marginLogicalHeight();
+}

[Julien - ping for review, 2015/01/29 13:43:48]

I don't think we should fork this function as it's too easy to get extra bugs
with the new code.

Instead we should use or introduce new functions that would handle setting /
getting the right value when dealing with orthogonal flows. You can find
examples of this on RenderBlock with e.g. setMarginStartForChild (which checks
for orthogonality) or logicalHeightForChild (which doesn't).

This can be applied to other snippets of this change, when we need to get the
right dimension based on the writing mode.

[jfernandez, 2015/02/06 22:59:58]

Done.

+
+LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInTheInlineDirection)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
-    if (hasOrthogonalWritingMode)
-        return 0;
+    if (hasOrthogonalWritingMode) {
+        if (direction == ForColumns)
+            return logicalHeightForOrthogonalChild(child, tracksInTheInlineDirection);
+        return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
+    }
 
     if (direction == ForColumns) {
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
@@ -621,15 +638,17 @@ LayoutUnit RenderGrid::minContentForChild(RenderBox& child, GridTrackSizingDirec
         return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, tracksInTheInlineDirection);
 }
 
-LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
+LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& tracksInTheInlineDirection)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: Properly support orthogonal writing mode.
-    if (hasOrthogonalWritingMode)
-        return LayoutUnit();
+    if (hasOrthogonalWritingMode) {
+        if (direction == ForColumns)
+            return logicalHeightForOrthogonalChild(child, tracksInTheInlineDirection);
+        return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
+    }
 
     if (direction == ForColumns) {
         // FIXME: It's unclear if we should return the intrinsic width or the preferred width.
@@ -637,7 +656,7 @@ LayoutUnit RenderGrid::maxContentForChild(RenderBox& child, GridTrackSizingDirec
         return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, columnTracks);
+    return logicalHeightForChild(child, tracksInTheInlineDirection);
 }
 
 // We're basically using a class instead of a std::pair for two reasons. First of all, accessing gridItem() or
@@ -702,10 +721,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));
                 }
@@ -730,20 +750,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>& tracksInTheInlineDirection)
 {
     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, tracksInTheInlineDirection)));
     else if (trackSize.hasMaxContentMinTrackBreadth())
-        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, tracksInTheInlineDirection)));
 
     if (trackSize.hasMinContentMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, tracksInTheInlineDirection)));
     else if (trackSize.hasMaxContentMaxTrackBreadth())
-        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, columnTracks)));
+        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, tracksInTheInlineDirection)));
 }
 
 void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, GridItemWithSpan& gridItemWithSpan, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, FilterFunction growAboveMaxBreadthFilterFunction)
@@ -770,7 +790,8 @@ void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
     if (sizingData.filteredTracks.isEmpty())
         return;
 
-    LayoutUnit additionalBreadthSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks);
+    bool hasOrthogonalWritingMode = gridItemWithSpan.gridItem().isHorizontalWritingMode() != isHorizontalWritingMode();
+    LayoutUnit additionalBreadthSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, hasOrthogonalWritingMode ? sizingData.rowTracks : sizingData.columnTracks);
     for (GridResolvedPosition trackIndexForSpace = initialTrackPosition; trackIndexForSpace <= finalTrackPosition; ++trackIndexForSpace) {
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndexForSpace.toInt()] : sizingData.rowTracks[trackIndexForSpace.toInt()];
         additionalBreadthSpace -= (track.*trackGetter)();
@@ -1080,10 +1101,29 @@ 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).

[Julien - ping for review, 2015/01/29 13:43:48]

I wonder if we shouldn't just do this step if we know we have a child with an
orthogonal writing mode as this is what it's meant to cover.

[jfernandez, 2015/02/06 22:59:58]

I think it's a better approach, indeed.

+    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
+        LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(*child, ForRows, sizingData.rowTracks);
+        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 && oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight) {
+            availableSpaceForColumns = availableLogicalWidth();
+            computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
+            ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
+            break;
+        }
+    }
 
     populateGridPositions(sizingData, availableSpaceForColumns, availableSpaceForRows);
     m_gridItemsOverflowingGridArea.resize(0);
@@ -1130,8 +1170,10 @@ 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)
+        bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != isHorizontalWritingMode();
+        bool overflowingContainingBlockHeight = hasOrthogonalWritingMode ? child->logicalWidth() > overrideContainingBlockContentLogicalHeight : child->logicalHeight() > overrideContainingBlockContentLogicalHeight;
+        bool overflowingContainingBlockWidth = hasOrthogonalWritingMode ? child->logicalHeight() > overrideContainingBlockContentLogicalWidth : child->logicalHeight() > overrideContainingBlockContentLogicalWidth;
+        if (overflowingContainingBlockHeight || overflowingContainingBlockWidth)
             m_gridItemsOverflowingGridArea.append(child);
     }
 
@@ -1692,7 +1734,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

[Julien - ping for review, 2015/01/29 13:43:48]

setLogicalPosition?

[jfernandez, 2015/02/06 22:59:58]

Sorry, I meant setLogicalLocation. This function transposes the LayoutPoint
received as argument depending on the container's writing mode. My theory is
that  setLogicalLocation misses orthogonal flows support, which in that case
should just use the point as it's passed, independently of the container's
writing mode.

We could perhaps adding such support to the RenderBox::setLogicalLocation but I
thought it would be safer handling in just in RenderGrid logic.

+    // 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)