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

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 b2205b79aab9ab159f6fff8407c6f05288f09743..e76c0529af35c6c60ec81bc40d6eb70b0f77445f 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -255,6 +255,8 @@ LayoutGrid::LayoutGrid(Element* element)
     : LayoutBlock(element)
     , m_gridIsDirty(true)
     , m_orderIterator(this)
+    , m_minContentHeight(-1)
+    , m_maxContentHeight(-1)
 {
     ASSERT(!childrenInline());
 }
@@ -308,6 +310,38 @@ bool LayoutGrid::namedGridLinesDefinitionDidChange(const ComputedStyle& oldStyle
         || oldStyle.namedGridColumnLines() != styleRef().namedGridColumnLines();
 }
 
+void LayoutGrid::computeGridTrackSizes(GridSizingData& sizingData, LayoutUnit& freeSpaceForColumns, LayoutUnit& freeSpaceForRows)
+{
+    freeSpaceForColumns = contentLogicalWidth();
+    computeUsedBreadthOfGridTracks(ForColumns, sizingData, freeSpaceForColumns);
+    ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
+
+    freeSpaceForRows = computeContentAndScrollbarLogicalHeightUsing(MainOrPreferredSize, style()->logicalHeight(), -1);
+    computeUsedBreadthOfGridTracks(ForRows, sizingData, freeSpaceForRows);
+    ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
+}
+
+LayoutUnit LayoutGrid::computeTrackBasedLogicalHeight(const GridSizingData& sizingData) const
+{
+    LayoutUnit logicalHeight = borderAndPaddingLogicalHeight() + scrollbarLogicalHeight();
+
+    for (const auto& row : sizingData.rowTracks)
+        logicalHeight += row.baseSize();
+
+    return logicalHeight;
+}
+
+void LayoutGrid::updateLogicalHeightDependentGridTracks(const LayoutUnit& trackBasedLogicalHeight, GridSizingData& sizingData, LayoutUnit& availableSpaceForRows)

[jfernandez, 2015/10/19 10:46:22]

Which are those dependent tracks ? This function will recompute logical height
of all row tracks, even those with a fixed size. Is that what we want ? 

If we really want to update all row tracks, shouldn't we change the function
name accordingly ? 

[svillar, 2015/10/19 12:14:20]

We need to rerun the algorithm, and it'll indeed update all row tracks, you
cannot just update a subset of them (updating fixed size tracks is ultra-cheap
BTW). Also it does not happen always as you can see, actually only in cases
where you have indefinite sizes with min|max-content restrictions as far as I
have observed.

+{
+    LayoutUnit contentHeight = contentLogicalHeight();
+    if (trackBasedLogicalHeight == contentHeight)
+        return;
+
+    availableSpaceForRows = contentHeight;
+    computeUsedBreadthOfGridTracks(ForRows, sizingData, availableSpaceForRows);
+    ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
+}
+
 void LayoutGrid::layoutBlock(bool relayoutChildren)
 {
     ASSERT(needsLayout());
@@ -315,8 +349,6 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
     if (!relayoutChildren && simplifiedLayout())
         return;
 
-    // FIXME: Much of this method is boiler plate that matches LayoutBox::layoutBlock and Layout*FlexibleBox::layoutBlock.
-    // It would be nice to refactor some of the duplicate code.
     {
         // LayoutState needs this deliberate scope to pop before updating scroll information (which
         // may trigger relayout).
@@ -324,16 +356,33 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
 
         LayoutSize previousSize = size();
 
-        setLogicalHeight(0);
         updateLogicalWidth();
 
         TextAutosizer::LayoutScope textAutosizerLayoutScope(this);
 
-        layoutGridItems();
+        placeItemsOnGrid();
+
+        // TODO(svillar): move freeSpaceRows|Columns to GridSizingData.
+        LayoutUnit freeSpaceForRows;
+        LayoutUnit freeSpaceForColumns;
+        GridSizingData sizingData(gridColumnCount(), gridRowCount());
+        computeGridTrackSizes(sizingData, freeSpaceForColumns, freeSpaceForRows);
 
         LayoutUnit oldClientAfterEdge = clientLogicalBottom();
+        LayoutUnit trackBasedLogicalHeight = computeTrackBasedLogicalHeight(sizingData);
+        setLogicalHeight(trackBasedLogicalHeight);
+
         updateLogicalHeight();
 
+        // 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).
+        updateLogicalHeightDependentGridTracks(trackBasedLogicalHeight, sizingData, freeSpaceForRows);
+
+        applyStretchAlignmentToTracksIfNeeded(ForColumns, sizingData, freeSpaceForColumns);
+        applyStretchAlignmentToTracksIfNeeded(ForRows, sizingData, freeSpaceForRows);
+
+        layoutGridItems(sizingData, freeSpaceForColumns, freeSpaceForRows);
+
         if (size() != previousSize)
             relayoutChildren = true;
 
@@ -369,6 +418,37 @@ void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, Layo
     maxLogicalWidth += scrollbarWidth;
 }
 
+LayoutUnit LayoutGrid::computeIntrinsicLogicalContentHeightUsing(const Length& logicalHeightLength, LayoutUnit intrinsicContentHeight, LayoutUnit borderAndPadding) const
+{
+    if (logicalHeightLength.isMinContent())
+        return m_minContentHeight;
+
+    if (logicalHeightLength.isMaxContent())

[jfernandez, 2015/10/19 10:46:22]

Current LayoutBox: logic doesn't implement the new min/max-content logic and it
has a FIXME stating the following:

"// FIXME(cbiesinger): The css-sizing spec is considering changing what
min-content/max-content should resolve to.
// If that happens, this code will have to change."

Are we sure we want to do this for Grid ? 

If we want to add this logic to grid, shouldn't we consider move/refactor some
code to LayoutBox ?

[svillar, 2015/10/19 12:14:20]

Yes it does implement the min/max-content, it's just they behave the same for
normal blocks.
No because the behavior, as I said is totally different for grid. For "normal"
blocks min-content=max-content=fit-content for heights.

That's the reason why I need to add a custom behavior for grids.

+        return m_maxContentHeight;
+
+    if (logicalHeightLength.isFitContent()) {
+        LayoutUnit fillAvailableExtent = containingBlock()->availableLogicalHeight(ExcludeMarginBorderPadding);
+        return std::min<LayoutUnit>(m_maxContentHeight, std::max<LayoutUnit>(m_minContentHeight, fillAvailableExtent));
+    }
+
+    if (logicalHeightLength.isFillAvailable())
+        return containingBlock()->availableLogicalHeight(ExcludeMarginBorderPadding) - borderAndPadding;
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+LayoutUnit LayoutGrid::computeContentAndScrollbarLogicalHeightUsing(SizeType heightType, const Length& height, LayoutUnit intrinsicContentHeight) const
+{
+    // The grid container’s auto block size is its max-content size.
+    if (height.isAuto())
+        return heightType == MinSize ? LayoutUnit(0) : m_maxContentHeight + scrollbarLogicalHeight();
+
+    if (height.isIntrinsic())
+        return computeIntrinsicLogicalContentHeightUsing(height, intrinsicContentHeight, borderAndPaddingLogicalHeight()) + scrollbarLogicalHeight();
+
+    return LayoutBox::computeContentAndScrollbarLogicalHeightUsing(heightType, height, intrinsicContentHeight);
+}
+
 bool LayoutGrid::gridElementIsShrinkToFit()
 {
     return isFloatingOrOutOfFlowPositioned();
@@ -407,13 +487,29 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
     if (!sizingData.contentSizedTracksIndex.isEmpty())
         resolveContentBasedTrackSizingFunctions(direction, sizingData);
 
+    LayoutUnit totalBaseSizes = 0;
+    LayoutUnit totalGrowthLimits = 0;
+
     for (const auto& track: tracks) {
         ASSERT(!track.infiniteGrowthPotential());
-        freeSpace -= track.baseSize();
+        totalBaseSizes += track.baseSize();
+        totalGrowthLimits += track.growthLimit();
     }
+    freeSpace = initialFreeSpace - totalBaseSizes;
 
-    const bool hasUndefinedRemainingSpace = (direction == ForRows) ? style()->logicalHeight().isAuto() : gridElementIsShrinkToFit();
+    // Cache our intrinisc heights so that we could resolve them properly in computeIntrinsicLogicalContentHeightUsing().
+    if (direction == ForRows) {
+        m_minContentHeight = totalBaseSizes;
+        m_maxContentHeight = totalGrowthLimits;
+    }
+
+    // Grid container should have the minimum height of a line if it's editable. That does not affect track sizing though.
+    if (hasLineIfEmpty()) {

[Manuel Rego, 2015/10/09 14:38:35]

Do we need this if "direction == ForColumns"?

Otherwise, we could move it inside the previous if.

[svillar, 2015/10/19 12:14:20]

Will do that.

+        m_minContentHeight = std::max(m_minContentHeight, minimumLogicalHeightForEmptyLine());
+        m_maxContentHeight = std::max(m_maxContentHeight, m_minContentHeight);
+    }
 
+    const bool hasUndefinedRemainingSpace = (direction == ForRows) ? initialFreeSpace == -1 : false;
     if (!hasUndefinedRemainingSpace && freeSpace <= 0)
         return;
 
@@ -464,9 +560,15 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
     for (const auto& trackIndex : flexibleSizedTracksIndex) {
         GridTrackSize trackSize = gridTrackSize(direction, trackIndex);
 
-        LayoutUnit baseSize = std::max<LayoutUnit>(tracks[trackIndex].baseSize(), flexFraction * trackSize.maxTrackBreadth().flex());
-        tracks[trackIndex].setBaseSize(baseSize);
-        freeSpace -= baseSize;
+        LayoutUnit oldBaseSize = tracks[trackIndex].baseSize();
+        LayoutUnit baseSize = std::max<LayoutUnit>(oldBaseSize, flexFraction * trackSize.maxTrackBreadth().flex());
+        if (LayoutUnit increment = baseSize - oldBaseSize) {
+            tracks[trackIndex].setBaseSize(baseSize);
+            freeSpace -= increment;

[Manuel Rego, 2015/10/09 14:38:35]

We're changing this regarding the previous code
that was subtracting "baseSize".

"baseSize" was already subtracted from the "freeSpace"
so probably this version is right.
However it seems not directly related with the rest of the patch.

[jfernandez, 2015/10/19 10:46:22]

Wow, that changes seems like a bug fix, but why we haven't detected it so far ?

[svillar, 2015/10/19 12:14:20]

Because we need very specific tests for that, as the remaining freeSpace will
only be used by the alignment logic, it won't be used by the track sizing
algorithm anymore.

+
+            if (direction == ForRows)
+                m_maxContentHeight += increment;
+        }
     }
 
     // FIXME: Should ASSERT flexible tracks exhaust the freeSpace ? (see issue 739613002).
@@ -1286,22 +1388,9 @@ void LayoutGrid::applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection
     }
 }
 
-void LayoutGrid::layoutGridItems()
+void LayoutGrid::layoutGridItems(GridSizingData& sizingData, LayoutUnit& freeSpaceForColumns, LayoutUnit& freeSpaceForRows)
 {
-    placeItemsOnGrid();
-
-    LayoutUnit availableSpaceForColumns = availableLogicalWidth();
-    LayoutUnit availableSpaceForRows = availableLogicalHeight(IncludeMarginBorderPadding);
-    GridSizingData sizingData(gridColumnCount(), gridRowCount());
-    computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
-    ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
-    computeUsedBreadthOfGridTracks(ForRows, sizingData, availableSpaceForRows);
-    ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
-
-    applyStretchAlignmentToTracksIfNeeded(ForColumns, sizingData, availableSpaceForColumns);
-    applyStretchAlignmentToTracksIfNeeded(ForRows, sizingData, availableSpaceForRows);
-
-    populateGridPositions(sizingData, availableSpaceForColumns, availableSpaceForRows);
+    populateGridPositions(sizingData, freeSpaceForColumns, freeSpaceForRows);
     m_gridItemsOverflowingGridArea.resize(0);
 
     for (LayoutBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
@@ -1345,16 +1434,6 @@ void LayoutGrid::layoutGridItems()
             || child->logicalWidth() > overrideContainingBlockContentLogicalWidth)
             m_gridItemsOverflowingGridArea.append(child);
     }
-
-    for (const auto& row : sizingData.rowTracks)
-        setLogicalHeight(logicalHeight() + row.baseSize());
-
-    LayoutUnit height = logicalHeight() + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight();
-    if (hasLineIfEmpty())
-        height = std::max(height, minimumLogicalHeightForEmptyLine());
-
-    // Min / max logical height is handled by the call to updateLogicalHeight in layoutBlock.
-    setLogicalHeight(height);
 }
 
 void LayoutGrid::prepareChildForPositionedLayout(LayoutBox& child)