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

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 05fd3d244912532c2fec9eb817b7fac0e7a959a7..3d125377b9952fdb1d04949f2bfbcd050b847416 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -249,7 +249,45 @@ public:
 
     enum SizingOperation { TrackSizing, IntrinsicSizeComputation };
     SizingOperation sizingOperation { TrackSizing };
-
+    enum SizingState { ColumnSizingFirstIteration, RowSizingFirstIteration, ColumnSizingSecondIteration, RowSizingSecondIteration};
+    SizingState sizingState { ColumnSizingFirstIteration };
+    void nextState()
+    {
+        switch (sizingState) {
+        case ColumnSizingFirstIteration:
+            sizingState = RowSizingFirstIteration;
+            break;
+        case RowSizingFirstIteration:
+            sizingState = ColumnSizingSecondIteration;
+            break;
+        case ColumnSizingSecondIteration:
+            sizingState = RowSizingSecondIteration;
+            break;
+        case RowSizingSecondIteration:
+            sizingState = ColumnSizingFirstIteration;
+            break;
+        default:
+            NOTREACHED();
+            sizingState = ColumnSizingFirstIteration;
+        }
+    }

[svillar, 2016/04/14 08:58:42]

I think it's better to merge SizingOperation and SizingState in a single enum.

[jfernandez, 2016/05/18 10:31:40]

I'm not sure about this, for several reasons. First, the SizingState enum
defines intermediate states of the TrackSizing algorithm; the intrinsic sizing
logic is torally unrelated. We can see these states as sub-states of
TrackSizing. Second, there is no clear transition between
IntrinsicSizeComputation and any of the TrackSizing related states. As a matter
of fact, computeIntrinsicLogicalHeight uses the same Sizingdata instance being
used during the TrackSizing operation, which implies we have to set the original
state back.

+    bool sanityCheck(GridTrackSizingDirection direction)
+    {
+        switch (sizingState) {
+        case ColumnSizingFirstIteration:
+            return direction == ForColumns ? true : false;
+        case RowSizingFirstIteration:
+            return direction == ForRows ? true : false;
+        case ColumnSizingSecondIteration:
+            if (direction == ForRows)
+                sizingState = RowSizingFirstIteration;
+            return true;
+        case RowSizingSecondIteration:
+            return direction == ForRows ? true : false;
+        }
+        NOTREACHED();
+        return false;
+    }

[svillar, 2016/04/14 08:58:41]

Let's enclose this in debugging guards as it's meant only to be used inside
DCHECK I guess. Also perhaps a more descriptive name? sanityCheck looks fine but
a little bit too generic.

[jfernandez, 2016/05/18 10:31:40]

Done.

 private:
     LayoutUnit freeSpaceForColumns { };
     LayoutUnit freeSpaceForRows { };
@@ -332,12 +370,32 @@ LayoutUnit LayoutGrid::computeTrackBasedLogicalHeight(const GridSizingData& sizi
 void LayoutGrid::computeTrackSizesForDirection(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit freeSpace)
 {
     ASSERT(freeSpace >= 0);
+    DCHECK(sizingData.sanityCheck(direction));
     sizingData.freeSpaceForDirection(direction) = freeSpace - guttersSize(direction, direction == ForRows ? gridRowCount() : gridColumnCount());
     sizingData.sizingOperation = GridSizingData::TrackSizing;
 
     LayoutUnit baseSizes, growthLimits;
     computeUsedBreadthOfGridTracks(direction, sizingData, baseSizes, growthLimits, AvailableSpaceDefinite);
     ASSERT(tracksAreWiderThanMinTrackBreadth(direction, sizingData));
+    sizingData.nextState();
+}
+
+void LayoutGrid::repeatTracksSizingIfNeeded(GridSizingData& sizingData, LayoutUnit availableSpaceForColumns, LayoutUnit availableSpaceForRows)
+{
+    DCHECK(sizingData.sizingState > GridSizingData::RowSizingFirstIteration);
+
+    // In orthogonal flow cases column track's size is determined by using the computed
+    // row track's size, which it was estimated during the first cycle of the sizing
+    // algorithm. Hence we need to repeat computeUsedBreadthOfGridTracks for both,
+    // columns and rows, to determine the final values.
+    // TODO (lajava): orthogonal flows is just one of the cases which may require

[svillar, 2016/04/14 08:58:41]

which other cases?

[jfernandez, 2016/05/18 10:31:40]

Well, the spec states the following on this regard: 

"This cycle is necessary for cases where the inline size of a grid item depends
on the block size of its grid area."

Orthogonal flows are just an example of the above scenario; the spec mentions as
well wrapped column flex containers and multi-column elements. I wonder whether
there are other cases which apply directly to grid fulfilling the mentioned
condition. 

+    // a new cycle of the sizing algorithm; there may be more. In addition, not all the
+    // cases with orthogonal flows require this extra cycle; we need a more specific
+    // condition to detect whether child's min-content contribution has changed or not.
+    if (m_hasAnyOrthogonalChild) {
+        computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
+        computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
+    }
 }
 
 void LayoutGrid::layoutBlock(bool relayoutChildren)
@@ -368,14 +426,16 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
         // properly resolves intrinsic sizes. We cannot do the same for heights though because many code
         // paths inside updateLogicalHeight() require a previous call to setLogicalHeight() to resolve
         // heights properly (like for positioned items for example).
-        computeTrackSizesForDirection(ForColumns, sizingData, availableLogicalWidth());
+        LayoutUnit availableSpaceForColumns = availableLogicalWidth();
+        computeTrackSizesForDirection(ForColumns, sizingData, availableSpaceForColumns);
 
         // 2- Next, the track sizing algorithm resolves the sizes of the grid rows, using the
         // grid column sizes calculated in the previous step.
+        LayoutUnit availableSpaceForRows = availableLogicalHeight(ExcludeMarginBorderPadding);

[svillar, 2016/04/14 08:58:41]

Better not do this. Calling availableXXX only makes sense for definite logical
heights. Also you are not using this value because you're redefining the
variable later. So I'll rather keep it as it was before.

[jfernandez, 2016/05/18 10:31:40]

Done.

         if (logicalHeightWasIndefinite)
             computeIntrinsicLogicalHeight(sizingData);
         else
-            computeTrackSizesForDirection(ForRows, sizingData, availableLogicalHeight(ExcludeMarginBorderPadding));
+            computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
         setLogicalHeight(computeTrackBasedLogicalHeight(sizingData) + borderAndPaddingLogicalHeight());
 
         LayoutUnit oldClientAfterEdge = clientLogicalBottom();
@@ -383,8 +443,14 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
 
         // The above call might have changed the grid's logical height depending on min|max height restrictions.
         // Update the sizes of the rows whose size depends on the logical height (also on definite|indefinite sizes).
+        availableSpaceForRows = contentLogicalHeight();

[svillar, 2016/04/14 08:58:41]

Instead you can define this variable right here.

[jfernandez, 2016/05/18 10:31:40]

Done.

         if (logicalHeightWasIndefinite)
-            computeTrackSizesForDirection(ForRows, sizingData, contentLogicalHeight());
+            computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
+
+        // 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).
+        repeatTracksSizingIfNeeded(sizingData, availableSpaceForColumns, availableSpaceForRows);
 
         // Grid container should have the minimum height of a line if it's editable. That doesn't affect track sizing though.
         if (hasLineIfEmpty())
@@ -667,7 +733,7 @@ bool LayoutGrid::hasDefiniteLogicalSize(GridTrackSizingDirection direction) cons
 
 static bool hasOverrideContainingBlockContentSizeForChild(const LayoutBox& child, GridTrackSizingDirection direction)
 {
-    return direction == ForColumns ? child.hasOverrideContainingBlockLogicalWidth() : child.overrideContainingBlockContentLogicalHeight();
+    return direction == ForColumns ? child.hasOverrideContainingBlockLogicalWidth() : child.hasOverrideContainingBlockLogicalHeight();

[svillar, 2016/04/14 08:58:41]

Ups, this is a bug per se. Let's do it separately with a test.

 }
 
 static LayoutUnit overrideContainingBlockContentSizeForChild(const LayoutBox& child, GridTrackSizingDirection direction)
@@ -745,9 +811,6 @@ GridTrackSizingDirection LayoutGrid::flowAwareDirectionForChild(const LayoutBox&
 
 LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    // TODO(svillar): Properly support orthogonal writing mode.
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     bool isRowAxis = direction == childInlineDirection;
     const Length& childSize = isRowAxis ? child.styleRef().logicalWidth() : child.styleRef().logicalHeight();
@@ -779,9 +842,6 @@ bool LayoutGrid::updateOverrideContainingBlockContentSizeForChild(LayoutBox& chi
 
 LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    // FIXME: Properly support orthogonal writing mode.
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     if (direction == childInlineDirection) {
         // If |child| has a relative logical width, we shouldn't let it override its intrinsic width, which is
@@ -802,8 +862,6 @@ LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirec
 
 LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
-    if (isOrthogonalChild(child))
-        return LayoutUnit();
     GridTrackSizingDirection childInlineDirection = flowAwareDirectionForChild(child, ForColumns);
     if (direction == childInlineDirection) {
         // If |child| has a relative logical width, we shouldn't let it override its intrinsic width, which is
@@ -1190,10 +1248,13 @@ void LayoutGrid::placeItemsOnGrid()
 
     Vector<LayoutBox*> autoMajorAxisAutoGridItems;
     Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
+    m_hasAnyOrthogonalChild = false;
     for (LayoutBox* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
         if (child->isOutOfFlowPositioned())
             continue;
 
+        m_hasAnyOrthogonalChild = m_hasAnyOrthogonalChild || isOrthogonalChild(*child);
+
         GridArea area = cachedGridArea(*child);
         if (!area.rows.isIndefinite())
             area.rows.translate(abs(m_smallestRowStart));
@@ -1476,7 +1537,7 @@ void LayoutGrid::layoutGridItems(GridSizingData& sizingData)
         LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChildIncludingAlignmentOffsets(*child, ForRows, sizingData);
 
         SubtreeLayoutScope layoutScope(*child);
-        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
+        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && (child->hasRelativeLogicalHeight() || isOrthogonalChild(*child))))
             layoutScope.setNeedsLayout(child, LayoutInvalidationReason::GridChanged);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
@@ -1638,9 +1699,39 @@ GridSpan LayoutGrid::cachedGridSpan(const LayoutBox& gridItem, GridTrackSizingDi
     return direction == ForColumns ? area.columns : area.rows;
 }
 
+bool LayoutGrid::gridLengthIsIndefinite(const GridLength& length, GridTrackSizingDirection direction) const

[svillar, 2016/04/14 08:58:41]

The name suggests that you're checking the length of the grid container. Let's
use a different name because what you're actually checking is the size of a grid
track.

[jfernandez, 2016/05/18 10:31:40]

Acknowledged.

+{
+    return length.isContentSized() || length.isFlex() || !hasDefiniteLogicalSize(direction);

[svillar, 2016/04/14 08:58:42]

This usage of hasDefiniteLogicalSize() is not correct. Note that it checks the
logical size of the grid container.

Something like this:
<div style="display:grid; height: min-content; grid-template-rows: 10px 10px;">

will return false for every single row which is wrong. We should definitely
revisit the tests because if they are working then something weird is happening
here (or perhaps we don't have enough)

I think you don't need that check at all.

[jfernandez, 2016/05/18 10:31:40]

I don't think there is anything wrong with current tests, at most, not enough
coverage. However, this logic is just intended for the initial state where we
don't know yet row's size for computing the orthogonal grid item's inline size.
This assumed value will be overwritten later during the second cycle of the
tracks sizing algorithm.
I agree. 

[svillar, 2016/05/26 08:49:02]

BTW you are considering percentages as definite sizes when that is not always
the case.

[jfernandez, 2016/05/27 09:41:22]

Note that the GridLength is extracted suing the gridTracksize function, which
already deals with percentages, so either they are converted to 'auto', hence
content-sized, or they are considered as definite.

Anyway, I admit this "isIndefinite" function is not very clear. I'd try to
implement something better.

+}
+
+LayoutUnit LayoutGrid::assumedRowsBreadthForOrthogonalChild(const LayoutBox& child) const

[svillar, 2016/04/14 08:58:41]

Breadth -> Size

[jfernandez, 2016/05/18 10:31:40]

Done.

+{
+    ASSERT(isOrthogonalChild(child));

[svillar, 2016/04/14 08:58:41]

DCHECK

[jfernandez, 2016/05/18 10:31:40]

Done.

+    const GridSpan& span = cachedGridSpan(child, ForRows);
+    LayoutUnit gridAreaBreadth;
+    for (auto trackPosition : span) {
+        const GridLength& maxTrackBreadth = gridTrackSize(ForRows, trackPosition).maxTrackBreadth();
+        if (gridLengthIsIndefinite(maxTrackBreadth, ForRows)) {
+            gridAreaBreadth = child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
+            break;

[svillar, 2016/04/14 08:58:41]

This looks wrong, because the final value of gridAreaBreadth will be always its
max preferred width. That's correct only when the max preferred width is larger
than the size of the tracks the item spans through.

Imagine the following example. An item with max preferred with of 20px which
spans 3 rows defined like this "grid-template-rows: 10px max-content 50px;"

The code above will return 20px as the grid area breadth which is wrong. The
max-content row will be 0px because with the other 2 we have more than enough
for the max preferred width of the item. So the grid area breadth will be 60px
and not 20px.

[jfernandez, 2016/05/18 10:31:40]

I understand your concerns, however, I think that for the case we are using this
function bot values are correct. Let me explain why so we can decide later which
approach we should follow. 

The assumed row's size is used to determine the size of the column tracks when
the item contributing to such size is orthogonal and we still don't know its 
block-size because it depends on its inline-size (determined based on the size
of the rows track). That's why we only call this function under the following 
condition:

    if (direction == ForRows && sizingData.sizingState ==
GridSizingData::ColumnSizingFirstIteration)

In this case, we only want to know whether tracks are definite, so we use such
definite size, or not. In the later case, we will just assume we have enough
space to laid out item's full content, hence the size of the column will be set
as
the item's minimum height. 

As you can see, it doesn't matter if we return 20px or 60px, because the size of
the columns track will be the same in both cases. 

If you prefer, we can use max(max-preferred, sunmOfDefiniteTracksinSpan),

[svillar, 2016/05/26 08:49:02]

If this is the case then we should have some ASSERTs verifying that.
I understand what you mean, but still I prefer the function to return a valid
value, if it's computing the area breadth then it should return that, not the
item's max inline size.

[jfernandez, 2016/05/27 09:41:22]

I don't think ASSERT that makes sense. One should know when we need to
estimate the size, because it's not possible to be determined at this phase of
the track sizing algorithm. However, I agree with you on that not being a valid
argument for not retuning a precise value, as you suggest in the comment
below.
I understand your point and will provide a change in that direction.

+        }
+        gridAreaBreadth += valueForLength(maxTrackBreadth.length(), LayoutUnit());

[svillar, 2016/04/14 08:58:41]

If I am not wrong the second value is the maximum which is used to resolve
percentages. I'd bet that percentages (against definite heights) are not
correctly resolved here and would always be 0. Do you have tests for that?

[jfernandez, 2016/05/18 10:31:40]

Your assumption on the usage of such second argument is correct. Also, you are
right that we lack tests for percentage sizes and orthogonal cases. 

Do you think we need that for this patch ? I mean, it's expectable to provide
support for all the cases combining orthogonal flows in a single patch ? Perhaps
a TODO here would be enough. What do you think ?

[svillar, 2016/05/26 08:49:02]

Yes I expect it to work for all the potential cases. If we cannot do that then
perhaps it means that we should split the patch in smaller pieces.

[jfernandez, 2016/05/27 09:41:22]

Ill fix the case of percentage sizes for tracks and provide tests to verify it
works as expected with orthogonal flows.

+    }
+
+    gridAreaBreadth += guttersSize(ForRows, span.integerSpan());
+
+    return gridAreaBreadth;
+}
+
 LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData) const
 {
-    const Vector<GridTrack>& tracks = direction == ForColumns ? sizingData.columnTracks : sizingData.rowTracks;
+    // To determine the column track's size based on an orthogonal grid item we need it's logical height, which
+    // may depend on the row track's size. It's possible that the row tracks sizing logic has not been performed yet,
+    // so we will need to do an estimation.
+    if (direction == ForRows && sizingData.sizingState == GridSizingData::ColumnSizingFirstIteration)
+        return assumedRowsBreadthForOrthogonalChild(child);
+
+    const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;

[svillar, 2016/04/14 08:58:41]

No need for parentheses.

[jfernandez, 2016/05/18 10:31:40]

Done.

     const GridSpan& span = cachedGridSpan(child, direction);
     LayoutUnit gridAreaBreadth;
     for (const auto& trackPosition : span)
@@ -2108,6 +2199,7 @@ ContentAlignmentData LayoutGrid::computeContentPositionAndDistributionOffset(Gri
 
 LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSizingData& sizingData) const
 {
+    LayoutUnit columnAxisOffset = columnAxisOffsetForChild(child, sizingData);
     LayoutUnit rowAxisOffset = rowAxisOffsetForChild(child, sizingData);
     // We stored m_columnPosition's data ignoring the direction, hence we might need now
     // to translate positions from RTL to LTR, as it's more convenient for painting.
@@ -2117,7 +2209,12 @@ LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSiz
         rowAxisOffset = rightGridEdgePosition - (rowAxisOffset + child.logicalWidth());
     }
 
-    return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child, sizingData));
+    // "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.
+    LayoutPoint childLocation(rowAxisOffset, columnAxisOffset);
+    return isOrthogonalChild(child) ? childLocation.transposedPoint() : childLocation;
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const