[css-grid] Implement grid gutters

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..150c940754545a24687d7f304beb16228309684f 100644
--- a/Source/core/layout/LayoutGrid.cpp
+++ b/Source/core/layout/LayoutGrid.cpp
@@ -370,6 +370,16 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
     clearNeedsLayout();
 }
 
+LayoutUnit LayoutGrid::guttersSize(GridTrackSizingDirection direction, size_t span) const
+{
+    if (span == 1)
+        return 0;
+
+    DEFINE_STATIC_LOCAL(LayoutUnit, zero, ());
+    const Length& trackGap = direction == ForColumns ? styleRef().gridColumnGap() : styleRef().gridRowGap();
+    return valueForLength(trackGap, zero) * (span - 1);
+}
+
 void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
 {
     const_cast<LayoutGrid*>(this)->placeItemsOnGrid();
@@ -386,6 +396,10 @@ void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, Layo
         maxLogicalWidth += maxTrackBreadth;
     }
 
+    LayoutUnit totalGuttersSize = guttersSize(ForColumns, sizingData.columnTracks.size());
+    minLogicalWidth += totalGuttersSize;
+    maxLogicalWidth += totalGuttersSize;
+
     LayoutUnit scrollbarWidth = intrinsicScrollbarLogicalWidth();
     minLogicalWidth += scrollbarWidth;
     maxLogicalWidth += scrollbarWidth;
@@ -398,8 +412,9 @@ bool LayoutGrid::gridElementIsShrinkToFit()
 
 void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit& freeSpace)
 {
-    const LayoutUnit initialFreeSpace = freeSpace;
     Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;

[jfernandez, 2015/09/09 21:07:45]

Why this line has been moved ? 

[svillar, 2015/09/10 11:55:45]

I had to move it for a previous version of the patch. Will revert this change.

+    const LayoutUnit initialFreeSpace = freeSpace;
+
     Vector<size_t> flexibleSizedTracksIndex;
     sizingData.contentSizedTracksIndex.shrink(0);
 
@@ -953,6 +968,8 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
         if (sizingData.filteredTracks.isEmpty())
             continue;
 
+        spanningTracksSize += guttersSize(direction, itemSpan.integerSpan());
+
         LayoutUnit extraSpace = currentItemSizeForTrackSizeComputationPhase(phase, gridItemWithSpan.gridItem(), direction, sizingData.columnTracks) - spanningTracksSize;
         extraSpace = std::max<LayoutUnit>(extraSpace, 0);
         auto& tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? sizingData.filteredTracks : sizingData.growBeyondGrowthLimitsTracks;
@@ -1309,6 +1326,11 @@ void LayoutGrid::layoutGridItems()
 
     LayoutUnit availableSpaceForColumns = availableLogicalWidth();
     LayoutUnit availableSpaceForRows = availableLogicalHeight(IncludeMarginBorderPadding);
+
+    // Remove space consumed by gutters from the available logical space.
+    availableSpaceForColumns -= guttersSize(ForColumns, gridColumnCount());
+    availableSpaceForRows -= guttersSize(ForRows, gridRowCount());
+

[jfernandez, 2015/09/09 21:07:45]

Why these lines are not part of the computeUsedBreadthOfGridTracks logic ?

[svillar, 2015/09/10 11:55:45]

Because I wanted to avoid modifying the algorithm as much as possible.

     GridSizingData sizingData(gridColumnCount(), gridRowCount());
     computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
@@ -1363,10 +1385,12 @@ void LayoutGrid::layoutGridItems()
             m_gridItemsOverflowingGridArea.append(child);
     }
 
+    LayoutUnit height = borderAndPaddingLogicalHeight();
     for (const auto& row : sizingData.rowTracks)
-        setLogicalHeight(logicalHeight() + row.baseSize());
+        height += row.baseSize();
+
+    height += guttersSize(ForRows, sizingData.rowTracks.size());
 
-    LayoutUnit height = logicalHeight() + borderAndPaddingLogicalHeight();
     if (hasLineIfEmpty())
         height = std::max(height, minimumLogicalHeightForEmptyLine());
 
@@ -1478,6 +1502,9 @@ LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrack
     LayoutUnit gridAreaBreadth = 0;
     for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition)
         gridAreaBreadth += tracks[trackPosition.toInt()].baseSize();
+
+    gridAreaBreadth += guttersSize(direction, span.integerSpan());

[jfernandez, 2015/09/09 21:07:45]

Why the gutters size affect the gridArea breadth where an item is placed in ? AS
far as I know, it'd only affect when the item is spanned across several tracks,
since it will fill the gaps between start end end tracks. 

[svillar, 2015/09/10 11:55:45]

If you take a look at guttersSize(), if the span is 1 (meaning non-spanning
item) then this value is 0.

+
     return gridAreaBreadth;
 }
 
@@ -1497,7 +1524,7 @@ LayoutUnit LayoutGrid::gridAreaBreadthForChildIncludingAlignmentOffsets(const La
 
 void LayoutGrid::populateGridPositions(GridSizingData& sizingData, LayoutUnit availableSpaceForColumns, LayoutUnit availableSpaceForRows)
 {
-    // Since we add alignment offsets, grid lines are not always adjacent. Hence we will have to
+    // Since we add alignment offsets and track gutters, grid lines are not always adjacent. Hence we will have to
     // assume from now on that we just store positions of the initial grid lines of each track,
     // except the last one, which is the only one considered as a final grid line of a track.
     // FIXME: This will affect the computed style value of grid tracks size, since we are
@@ -1508,10 +1535,11 @@ void LayoutGrid::populateGridPositions(GridSizingData& sizingData, LayoutUnit av
     unsigned lastLine = numberOfLines - 1;
     unsigned nextToLastLine = numberOfLines - 2;
     ContentAlignmentData offset = computeContentPositionAndDistributionOffset(ForColumns, availableSpaceForColumns, numberOfTracks);
+    LayoutUnit trackGap = guttersSize(ForColumns, 2);
     m_columnPositions.resize(numberOfLines);
     m_columnPositions[0] = borderAndPaddingStart() + offset.positionOffset;
     for (unsigned i = 0; i < lastLine; ++i)
-        m_columnPositions[i + 1] = m_columnPositions[i] + offset.distributionOffset + sizingData.columnTracks[i].baseSize();
+        m_columnPositions[i + 1] = m_columnPositions[i] + offset.distributionOffset + sizingData.columnTracks[i].baseSize() + trackGap;
     m_columnPositions[lastLine] = m_columnPositions[nextToLastLine] + sizingData.columnTracks[nextToLastLine].baseSize();
 
     numberOfTracks = sizingData.rowTracks.size();
@@ -1519,10 +1547,11 @@ void LayoutGrid::populateGridPositions(GridSizingData& sizingData, LayoutUnit av
     lastLine = numberOfLines - 1;
     nextToLastLine = numberOfLines - 2;
     offset = computeContentPositionAndDistributionOffset(ForRows, availableSpaceForRows, numberOfTracks);
+    trackGap = guttersSize(ForRows, 2);
     m_rowPositions.resize(numberOfLines);
     m_rowPositions[0] = borderAndPaddingBefore() + offset.positionOffset;
     for (unsigned i = 0; i < lastLine; ++i)
-        m_rowPositions[i + 1] = m_rowPositions[i] + offset.distributionOffset + sizingData.rowTracks[i].baseSize();
+        m_rowPositions[i + 1] = m_rowPositions[i] + offset.distributionOffset + sizingData.rowTracks[i].baseSize() + trackGap;
     m_rowPositions[lastLine] = m_rowPositions[nextToLastLine] + sizingData.rowTracks[nextToLastLine].baseSize();
 }
 
@@ -1763,7 +1792,12 @@ LayoutUnit LayoutGrid::columnAxisOffsetForChild(const LayoutBox& child) const
         return startPosition;
     case GridAxisEnd:
     case GridAxisCenter: {
-        LayoutUnit endOfRow = m_rowPositions[coordinate.rows.resolvedFinalPosition.next().toInt()];
+        size_t childEndLine = coordinate.rows.resolvedFinalPosition.next().toInt();
+        LayoutUnit endOfRow = m_rowPositions[childEndLine];
+        // // m_rowPositions include gutters so we need to substract them to get the actual end position for a given
+        // // row (this does not have to be done for the last track as there are no more m_rowPositions after it)
+        if (childEndLine < m_rowPositions.size() - 1)
+            endOfRow -= guttersSize(ForRows, 2);

[jfernandez, 2015/09/09 21:07:45]

I think we are doing something wrong somewhere, since we shouldn't have to
substrack gutters size to determine the endOfRow position. The content alignment
also is also added to the m_rowPositions value and we don't need to substract it
here. 

It may be that we are adding it somewhere where we shouldn't; perhaps related,
somehow, with my comment on the gridAreaBreadthForChild change.

[svillar, 2015/09/10 11:55:45]

We're just removing 1 gutter, and just for the case of the endRow not being the
last one (because the last one does not have gutter). We need to do that because
m_rowPositions already has the gaps, so we cannot blindly get the value it has
for the endOfRow because it contains the gutter and we don't want to pick it for
the final resolved position.

[jfernandez, 2015/09/11 08:11:09]

It seems your implementation may be correct, hence we have some bug somewhere in
the content alignment logic. However, I'm not sure it you have defined a test to
verify gutters logic works fine with alignment and span. 

         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().alignSelfOverflowAlignment(), endOfRow - startOfRow, child.logicalHeight() + child.marginLogicalHeight());
         return startPosition + (axisPosition == GridAxisEnd ? offsetFromStartPosition : offsetFromStartPosition / 2);
     }
@@ -1784,7 +1818,12 @@ LayoutUnit LayoutGrid::rowAxisOffsetForChild(const LayoutBox& child) const
         return startPosition;
     case GridAxisEnd:
     case GridAxisCenter: {
-        LayoutUnit endOfColumn = m_columnPositions[coordinate.columns.resolvedFinalPosition.next().toInt()];
+        size_t childEndLine = coordinate.columns.resolvedFinalPosition.next().toInt();
+        LayoutUnit endOfColumn = m_columnPositions[childEndLine];
+        // m_columnPositions include gutters so we need to substract them to get the actual end position for a given
+        // column (this does not have to be done for the last track as there are no more m_columnPositions after it)
+        if (childEndLine < m_columnPositions.size() - 1)
+            endOfColumn -= guttersSize(ForRows, 2);

[jfernandez, 2015/09/09 21:07:45]

Ditto.

[svillar, 2015/09/10 11:55:45]

See above.

         LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().justifySelfOverflowAlignment(), endOfColumn - startOfColumn, child.logicalWidth() + child.marginLogicalWidth());
         return startPosition + (axisPosition == GridAxisEnd ? offsetFromStartPosition : offsetFromStartPosition / 2);
     }