[CSS Grid Layout] Implement auto-margins alignment of grid items

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 fa0f179b1a0fff95e96561186f81dfd73a1ad9b9..fab44ac9f8c36a13ee6d72ba319888d0c321f80e 100644
--- a/Source/core/layout/LayoutGrid.cpp
+++ b/Source/core/layout/LayoutGrid.cpp
@@ -1337,6 +1337,8 @@ void LayoutGrid::layoutGridItems()
         SubtreeLayoutScope layoutScope(*child);
         if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
             layoutScope.setNeedsLayout(child, LayoutInvalidationReason::GridChanged);
+        else
+            resetAutoMarginsAndLogicalTopInColumnAxis(*child);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
@@ -1348,6 +1350,10 @@ void LayoutGrid::layoutGridItems()
 
         child->layoutIfNeeded();
 
+        // We need pending layouts to be done in order to compute auto-margins properly.
+        updateAutoMarginsInColumnAxisIfNeeded(*child);
+        updateAutoMarginsInRowAxisIfNeeded(*child);
+
 #if ENABLE(ASSERT)
         const GridCoordinate& coordinate = cachedGridCoordinate(*child);
         ASSERT(coordinate.columns.resolvedInitialPosition.toInt() < sizingData.columnTracks.size());
@@ -1659,6 +1665,109 @@ void LayoutGrid::applyStretchAlignmentToChildIfNeeded(LayoutBox& child)
     }
 }
 
+// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
+bool LayoutGrid::hasAutoMarginsInColumnAxis(const LayoutBox& child) const
+{
+    if (isHorizontalWritingMode())
+        return child.style()->marginTop().isAuto() || child.style()->marginBottom().isAuto();
+    return child.style()->marginLeft().isAuto() || child.style()->marginRight().isAuto();
+}
+
+// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
+bool LayoutGrid::hasAutoMarginsInRowAxis(const LayoutBox& child) const
+{
+    if (isHorizontalWritingMode())
+        return child.style()->marginLeft().isAuto() || child.style()->marginRight().isAuto();
+    return child.style()->marginTop().isAuto() || child.style()->marginBottom().isAuto();
+}
+
+// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
+void LayoutGrid::resetAutoMarginsAndLogicalTopInColumnAxis(LayoutBox& child)
+{
+    if (hasAutoMarginsInColumnAxis(child) || child.needsLayout()) {
+        child.clearOverrideLogicalContentHeight();

[cbiesinger, 2015/08/17 23:50:40]

Why do you have this line?

[jfernandez, 2015/08/24 10:28:23]

Good you realized about this "obscure" change. The reason for that line is to
avoid the assertion in LayoutBox::computeLogicalHeight

https://src.chromium.org/viewvc/blink/trunk/Source/core/layout/LayoutBox.cpp?...

This assertion was added sometime ago by @rego to deal with minHeight auto
values when block has an oiverrideLogicalContentHeight (hence, it was
stretched). Since we only stretch grid items with auto height, this condition
was always true, hence he decided to ensure that with an assert.

Now in this patch we are requesting to computeLogicalHeight to reset the
previously set auto-margins, hence it triggers the assert when forcing a
relayout (for instance, when changing margins value, which is one of the
relayout tests cases available for grid).

We have been discussing about this and @rego thinks we can change change the
assert by an additional condition in the mentioned branch. Obviously clearing
the override height would work as well, but it's perhaps a but hacky. 

I'd appreciate your feedback on this issue.  

[cbiesinger, 2015/08/26 23:07:44]

Is your link correct? It does not point to computeLogicalHeight, or an assert at
all, so I am not sure I completely follow the argument. I hope you can show me
the right link but for now, without seeing the assert, it seems better to adjust
the assert?

+        child.updateLogicalHeight();
+        if (isHorizontalWritingMode()) {
+            if (child.style()->marginTop().isAuto())
+                child.setMarginTop(0);
+            if (child.style()->marginBottom().isAuto())
+                child.setMarginBottom(0);
+        } else {
+            if (child.style()->marginLeft().isAuto())
+                child.setMarginLeft(0);
+            if (child.style()->marginRight().isAuto())
+                child.setMarginRight(0);
+        }
+
+    }
+}
+
+// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
+void LayoutGrid::updateAutoMarginsInRowAxisIfNeeded(LayoutBox& child)
+{
+    ASSERT(!child.isOutOfFlowPositioned());
+
+    LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalWidth() - child.logicalWidth();
+    if (availableAlignmentSpace <= 0)
+        return;
+
+    bool isHorizontal = isHorizontalWritingMode();
+    Length topOrLeft = isHorizontal ? child.style()->marginLeft() : child.style()->marginTop();
+    Length bottomOrRight = isHorizontal ? child.style()->marginRight() : child.style()->marginBottom();
+    if (topOrLeft.isAuto() && bottomOrRight.isAuto()) {
+        if (isHorizontal) {
+            child.setMarginLeft(availableAlignmentSpace / 2);
+            child.setMarginRight(availableAlignmentSpace / 2);
+        } else {
+            child.setMarginTop(availableAlignmentSpace / 2);
+            child.setMarginBottom(availableAlignmentSpace / 2);
+        }
+    } else if (topOrLeft.isAuto()) {
+        if (isHorizontal)
+            child.setMarginLeft(availableAlignmentSpace);
+        else
+            child.setMarginTop(availableAlignmentSpace);
+    } else if (bottomOrRight.isAuto()) {
+        if (isHorizontal)
+            child.setMarginRight(availableAlignmentSpace);
+        else
+            child.setMarginBottom(availableAlignmentSpace);
+    }
+}
+
+// TODO(lajava): This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
+void LayoutGrid::updateAutoMarginsInColumnAxisIfNeeded(LayoutBox& child)
+{
+    ASSERT(!child.isOutOfFlowPositioned());
+
+    LayoutUnit availableAlignmentSpace = child.overrideContainingBlockContentLogicalHeight() - child.logicalHeight();
+    if (availableAlignmentSpace <= 0)
+        return;
+
+    bool isHorizontal = isHorizontalWritingMode();
+    Length topOrLeft = isHorizontal ? child.style()->marginTop() : child.style()->marginLeft();
+    Length bottomOrRight = isHorizontal ? child.style()->marginBottom() : child.style()->marginRight();
+    if (topOrLeft.isAuto() && bottomOrRight.isAuto()) {
+        if (isHorizontal) {
+            child.setMarginTop(availableAlignmentSpace / 2);
+            child.setMarginBottom(availableAlignmentSpace / 2);
+        } else {
+            child.setMarginLeft(availableAlignmentSpace / 2);
+            child.setMarginRight(availableAlignmentSpace / 2);
+        }
+    } else if (topOrLeft.isAuto()) {
+        if (isHorizontal)
+            child.setMarginTop(availableAlignmentSpace);
+        else
+            child.setMarginLeft(availableAlignmentSpace);
+    } else if (bottomOrRight.isAuto()) {
+        if (isHorizontal)
+            child.setMarginBottom(availableAlignmentSpace);
+        else
+            child.setMarginRight(availableAlignmentSpace);
+    }
+}
+
 GridAxisPosition LayoutGrid::columnAxisPositionForChild(const LayoutBox& child) const
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
@@ -1756,6 +1865,8 @@ LayoutUnit LayoutGrid::columnAxisOffsetForChild(const LayoutBox& child) const
     const GridCoordinate& coordinate = cachedGridCoordinate(child);
     LayoutUnit startOfRow = m_rowPositions[coordinate.rows.resolvedInitialPosition.toInt()];
     LayoutUnit startPosition = startOfRow + marginBeforeForChild(child);
+    if (hasAutoMarginsInColumnAxis(child))
+        return startPosition;
     GridAxisPosition axisPosition = columnAxisPositionForChild(child);
     switch (axisPosition) {
     case GridAxisStart:
@@ -1777,6 +1888,8 @@ LayoutUnit LayoutGrid::rowAxisOffsetForChild(const LayoutBox& child) const
     const GridCoordinate& coordinate = cachedGridCoordinate(child);
     LayoutUnit startOfColumn = m_columnPositions[coordinate.columns.resolvedInitialPosition.toInt()];
     LayoutUnit startPosition = startOfColumn + marginStartForChild(child);
+    if (hasAutoMarginsInRowAxis(child))
+        return startPosition;
     GridAxisPosition axisPosition = rowAxisPositionForChild(child);
     switch (axisPosition) {
     case GridAxisStart: