[CSS Grid Layout] Refactoring of grid's alignment logic

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 ef03ebb483a99b7fa37ae86d253eefa4f8970a6e..f704f2bc4e66f876dd5358bd3bbf3fa86b578247 100644
--- a/Source/core/layout/LayoutGrid.cpp
+++ b/Source/core/layout/LayoutGrid.cpp
@@ -1330,167 +1330,26 @@ void LayoutGrid::populateGridPositions(const GridSizingData& sizingData)
         m_rowPositions[i + 1] = m_rowPositions[i] + sizingData.rowsDistributionOffset + sizingData.rowTracks[i].baseSize();
 }
 
-static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit startOfTrack, LayoutUnit endOfTrack, LayoutUnit childBreadth)
+static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit trackBreadth, LayoutUnit childBreadth)
 {
-    LayoutUnit trackBreadth = endOfTrack - startOfTrack;
     LayoutUnit offset = trackBreadth - childBreadth;
-
-    // If overflow is 'safe', we have to make sure we don't overflow the 'start'
-    // edge (potentially cause some data loss as the overflow is unreachable).
-    if (overflow == OverflowAlignmentSafe)
-        offset = std::max<LayoutUnit>(0, offset);
-
-    // If we overflow our alignment container and overflow is 'true' (default), we
-    // ignore the overflow and just return the value regardless (which may cause data
-    // loss as we overflow the 'start' edge).
-    return offset;
-}
-
-LayoutUnit LayoutGrid::startOfColumnForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-    LayoutUnit startOfColumn = m_columnPositions[coordinate.columns.resolvedInitialPosition.toInt()];
-    // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    return startOfColumn + marginStartForChild(child);
-}
-
-LayoutUnit LayoutGrid::endOfColumnForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-    LayoutUnit startOfColumn = m_columnPositions[coordinate.columns.resolvedInitialPosition.toInt()];
-    // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    LayoutUnit columnPosition = startOfColumn + marginStartForChild(child);
-
-    LayoutUnit endOfColumn = m_columnPositions[coordinate.columns.resolvedFinalPosition.next().toInt()];
-    // FIXME: This might not work as expected with orthogonal writing-modes.
-    LayoutUnit offsetFromColumnPosition = computeOverflowAlignmentOffset(child.style()->justifySelfOverflowAlignment(), startOfColumn, endOfColumn, child.logicalWidth() + child.marginLogicalWidth());
-
-    return columnPosition + offsetFromColumnPosition;
-}
-
-LayoutUnit LayoutGrid::columnPositionLeft(const LayoutBox& child) const
-{
-    if (style()->isLeftToRightDirection())
-        return startOfColumnForChild(child);
-
-    return endOfColumnForChild(child);
-}
-
-LayoutUnit LayoutGrid::columnPositionRight(const LayoutBox& child) const
-{
-    if (!style()->isLeftToRightDirection())
-        return startOfColumnForChild(child);
-
-    return endOfColumnForChild(child);
-}
-
-LayoutUnit LayoutGrid::centeredColumnPositionForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-    LayoutUnit startOfColumn = m_columnPositions[coordinate.columns.resolvedInitialPosition.toInt()];
-    LayoutUnit endOfColumn = m_columnPositions[coordinate.columns.resolvedFinalPosition.next().toInt()];
-    LayoutUnit columnPosition = startOfColumn + marginStartForChild(child);
-    // FIXME: This might not work as expected with orthogonal writing-modes.
-    LayoutUnit offsetFromColumnPosition = computeOverflowAlignmentOffset(child.style()->justifySelfOverflowAlignment(), startOfColumn, endOfColumn, child.logicalWidth() + child.marginLogicalWidth());
-
-    return columnPosition + offsetFromColumnPosition / 2;
-}
-
-LayoutUnit LayoutGrid::columnPositionForChild(const LayoutBox& child) const
-{
-    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-
-    switch (ComputedStyle::resolveJustification(styleRef(), child.styleRef(), ItemPositionStretch)) {
-    case ItemPositionSelfStart:
-        // For orthogonal writing-modes, this computes to 'start'
-        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return startOfColumnForChild(child);
-
-        // self-start is based on the child's direction. That's why we need to check against the grid container's direction.
-        if (child.style()->direction() != style()->direction())
-            return endOfColumnForChild(child);
-
-        return startOfColumnForChild(child);
-    case ItemPositionSelfEnd:
-        // For orthogonal writing-modes, this computes to 'start'
-        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return endOfColumnForChild(child);
-
-        // self-end is based on the child's direction. That's why we need to check against the grid container's direction.
-        if (child.style()->direction() != style()->direction())
-            return startOfColumnForChild(child);
-
-        return endOfColumnForChild(child);
-    case ItemPositionFlexStart:
-        // Only used in flex layout, for other layout, it's equivalent to 'start'.
-        return startOfColumnForChild(child);
-    case ItemPositionFlexEnd:
-        // Only used in flex layout, for other layout, it's equivalent to 'end'.
-        return endOfColumnForChild(child);
-    case ItemPositionLeft:
-        return columnPositionLeft(child);
-    case ItemPositionRight:
-        return columnPositionRight(child);
-    case ItemPositionCenter:
-        return centeredColumnPositionForChild(child);
-    case ItemPositionStart:
-        return startOfColumnForChild(child);
-    case ItemPositionEnd:
-        return endOfColumnForChild(child);
-    case ItemPositionAuto:
-        break;
-    case ItemPositionStretch:
-        return startOfColumnForChild(child);
-    case ItemPositionBaseline:
-    case ItemPositionLastBaseline:
-        // FIXME: Implement the previous values. For now, we always 'start' align the child.
-        return startOfColumnForChild(child);
+    switch (overflow) {
+    case OverflowAlignmentSafe:
+        // If overflow is 'safe', we have to make sure we don't overflow the 'start'
+        // edge (potentially cause some data loss as the overflow is unreachable).
+        return std::max<LayoutUnit>(0, offset);
+    case OverflowAlignmentTrue:
+    case OverflowAlignmentDefault:
+        // If we overflow our alignment container and overflow is 'true' (default), we
+        // ignore the overflow and just return the value regardless (which may cause data
+        // loss as we overflow the 'start' edge).
+        return offset;
     }
 
     ASSERT_NOT_REACHED();
     return 0;
 }
 
-LayoutUnit LayoutGrid::endOfRowForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-
-    LayoutUnit startOfRow = m_rowPositions[coordinate.rows.resolvedInitialPosition.toInt()];
-    // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    LayoutUnit rowPosition = startOfRow + marginBeforeForChild(child);
-
-    LayoutUnit endOfRow = m_rowPositions[coordinate.rows.resolvedFinalPosition.next().toInt()];
-    LayoutUnit offsetFromRowPosition = computeOverflowAlignmentOffset(child.style()->alignSelfOverflowAlignment(), startOfRow, endOfRow, child.logicalHeight() + child.marginLogicalHeight());
-
-    return rowPosition + offsetFromRowPosition;
-}
-
-LayoutUnit LayoutGrid::startOfRowForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-
-    LayoutUnit startOfRow = m_rowPositions[coordinate.rows.resolvedInitialPosition.toInt()];
-    // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    LayoutUnit rowPosition = startOfRow + marginBeforeForChild(child);
-
-    return rowPosition;
-}
-
-LayoutUnit LayoutGrid::centeredRowPositionForChild(const LayoutBox& child) const
-{
-    const GridCoordinate& coordinate = cachedGridCoordinate(child);
-
-    // The grid items should be inside the grid container's border box, that's why they need to be shifted.
-    LayoutUnit startOfRow = m_rowPositions[coordinate.rows.resolvedInitialPosition.toInt()];
-    LayoutUnit endOfRow = m_rowPositions[coordinate.rows.resolvedFinalPosition.next().toInt()];
-    LayoutUnit rowPosition = startOfRow + marginBeforeForChild(child);
-    LayoutUnit offsetFromRowPosition = computeOverflowAlignmentOffset(child.style()->alignSelfOverflowAlignment(), startOfRow, endOfRow, child.logicalHeight() + child.marginLogicalHeight());
-
-    return rowPosition + offsetFromRowPosition / 2;
-}
-
 static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const LayoutBox& child)
 {
     LayoutUnit childIntrinsicContentLogicalHeight = child.intrinsicContentLogicalHeight();
@@ -1592,71 +1451,136 @@ void LayoutGrid::applyStretchAlignmentToChildIfNeeded(LayoutBox& child, LayoutUn
     }
 }
 
-LayoutUnit LayoutGrid::rowPositionForChild(const LayoutBox& child) const
+GridAxisPosition LayoutGrid::columnAxisPositionForChild(const LayoutBox& child) const
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    bool hasSameWritingMode = child.styleRef().writingMode() == styleRef().writingMode();
+
     switch (ComputedStyle::resolveAlignment(styleRef(), child.styleRef(), ItemPositionStretch)) {
     case ItemPositionSelfStart:
         // If orthogonal writing-modes, this computes to 'start'.
         // FIXME: grid track sizing and positioning do not support orthogonal modes yet.

[svillar, 2015/06/10 08:27:20]

You're repeating this same FIXME for every single case, perhaps it would be
better to just write it once before the switch.

[jfernandez, 2015/06/11 12:00:31]

I'm not totally sure. The comment helps to realize why the specific alignment
value does not work as expected in orthogonal flow cases. I'd rather keep the
comments as they are, if you don't mind.

-        if (hasOrthogonalWritingMode)
-            return startOfRowForChild(child);
-
         // self-start is based on the child's block axis direction. That's why we need to check against the grid container's block flow.
-        if (child.style()->writingMode() != style()->writingMode())
-            return endOfRowForChild(child);
-
-        return startOfRowForChild(child);
+        return (hasOrthogonalWritingMode || hasSameWritingMode) ? GridAxisStart : GridAxisEnd;
     case ItemPositionSelfEnd:
         // If orthogonal writing-modes, this computes to 'end'.
         // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return endOfRowForChild(child);
-
         // self-end is based on the child's block axis direction. That's why we need to check against the grid container's block flow.
-        if (child.style()->writingMode() != style()->writingMode())
-            return startOfRowForChild(child);
-
-        return endOfRowForChild(child);
+        return (hasOrthogonalWritingMode || hasSameWritingMode) ? GridAxisEnd : GridAxisStart;
     case ItemPositionLeft:
         // The alignment axis (column axis) and the inline axis are parallell in
-        // orthogonal writing mode.
+        // orthogonal writing mode. Otherwise this this is equivalent to 'start'.
         // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return startOfRowForChild(child);
-
-        // Otherwise this this is equivalent to 'start'.
-        return startOfRowForChild(child);
+        return GridAxisStart;
     case ItemPositionRight:
         // The alignment axis (column axis) and the inline axis are parallell in
-        // orthogonal writing mode.
+        // orthogonal writing mode. Otherwise this this is equivalent to 'start'.
         // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return endOfRowForChild(child);
+        return hasOrthogonalWritingMode ? GridAxisEnd : GridAxisStart;

[svillar, 2015/06/10 08:27:20]

I don't get why you use hastOrthogonalWritingMode for ItemPositionRight but not
for ItemPositionLeft.

[jfernandez, 2015/06/11 12:00:31]

It's explained in the comment above; we can only apply desired alignment value
(end) in case of orthogonal flows (otherwise we default to start). Since the
specified value is End, it's different to the default one, which is not the case
for ItemPositionLeft where both specific and default are the same.

+    case ItemPositionCenter:
+        return GridAxisCenter;
+    case ItemPositionFlexStart: // Only used in flex layout, otherwise equivalent to 'start'.
+    case ItemPositionStart:
+        return GridAxisStart;
+    case ItemPositionFlexEnd: // Only used in flex layout, otherwise equivalent to 'end'.
+    case ItemPositionEnd:
+        return GridAxisEnd;
+    case ItemPositionStretch:
+        return GridAxisStart;
+    case ItemPositionBaseline:
+    case ItemPositionLastBaseline:
+        // FIXME: Implement the previous values. For now, we always 'start' align the child.

[svillar, 2015/06/10 08:27:20]

Does "previous values" mean baseline alignment? If so, I'd prefer to be explicit
like "FIXME: these two requires implementing baseline alignment" or something
similar

[jfernandez, 2015/06/11 12:00:31]

Yes, you are right. That comment was already there when more than those two
values were not implemented. Now it makes much more sense a comment like the one
you are suggesting.

+        return GridAxisStart;
+    case ItemPositionAuto:
+        break;
+    }
 
-        // Otherwise this this is equivalent to 'start'.
-        return startOfRowForChild(child);
+    ASSERT_NOT_REACHED();
+    return GridAxisStart;
+}
+
+GridAxisPosition LayoutGrid::rowAxisPositionForChild(const LayoutBox& child) const
+{
+    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+    bool hasSameDirection = child.styleRef().direction() == styleRef().direction();
+    bool isLTR = styleRef().isLeftToRightDirection();
+
+    switch (ComputedStyle::resolveJustification(styleRef(), child.styleRef(), ItemPositionStretch)) {
+    case ItemPositionSelfStart:
+        // For orthogonal writing-modes, this computes to 'start'
+        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
+        // self-start is based on the child's direction. That's why we need to check against the grid container's direction.
+        return (hasOrthogonalWritingMode || hasSameDirection) ? GridAxisStart : GridAxisEnd;
+    case ItemPositionSelfEnd:
+        // For orthogonal writing-modes, this computes to 'start'
+        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
+        return (hasOrthogonalWritingMode || hasSameDirection) ? GridAxisEnd : GridAxisStart;
+    case ItemPositionLeft:
+        return isLTR ? GridAxisStart : GridAxisEnd;
+    case ItemPositionRight:
+        return isLTR ? GridAxisEnd : GridAxisStart;
     case ItemPositionCenter:
-        return centeredRowPositionForChild(child);
-        // Only used in flex layout, for other layout, it's equivalent to 'start'.
-    case ItemPositionFlexStart:
+        return GridAxisCenter;
+    case ItemPositionFlexStart: // Only used in flex layout, otherwise equivalent to 'start'.
     case ItemPositionStart:
-        return startOfRowForChild(child);
-        // Only used in flex layout, for other layout, it's equivalent to 'end'.
-    case ItemPositionFlexEnd:
+        return GridAxisStart;
+    case ItemPositionFlexEnd: // Only used in flex layout, otherwise equivalent to 'end'.
     case ItemPositionEnd:
-        return endOfRowForChild(child);
+        return GridAxisEnd;
     case ItemPositionStretch:
-        return startOfRowForChild(child);
+        return GridAxisStart;
     case ItemPositionBaseline:
     case ItemPositionLastBaseline:
-        // FIXME: Implement the ItemPositionBaseline value. For now, we always 'start' align the child.
-        return startOfRowForChild(child);
+        // FIXME: Implement the previous values. For now, we always 'start' align the child.
+        return GridAxisStart;
     case ItemPositionAuto:
         break;
     }
 
     ASSERT_NOT_REACHED();
+    return GridAxisStart;
+}
+
+LayoutUnit LayoutGrid::rowPositionForChild(const LayoutBox& child) const
+{
+    const GridCoordinate& coordinate = cachedGridCoordinate(child);
+    LayoutUnit startOfRow = m_rowPositions[coordinate.rows.resolvedInitialPosition.toInt()];
+    LayoutUnit endOfRow = m_rowPositions[coordinate.rows.resolvedFinalPosition.next().toInt()];
+    LayoutUnit startPosition = startOfRow + marginBeforeForChild(child);
+    LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().justifySelfOverflowAlignment(), endOfRow - startOfRow, child.logicalHeight() + child.marginLogicalHeight());
+
+    switch (columnAxisPositionForChild(child)) {
+    case GridAxisStart:
+        return startPosition;
+    case GridAxisEnd:
+        return startPosition + offsetFromStartPosition;
+    case GridAxisCenter:
+        return startPosition + offsetFromStartPosition / 2;
+    }
+
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+LayoutUnit LayoutGrid::columnPositionForChild(const LayoutBox& child) const
+{
+    const GridCoordinate& coordinate = cachedGridCoordinate(child);
+    LayoutUnit startOfColumn = m_columnPositions[coordinate.columns.resolvedInitialPosition.toInt()];
+    LayoutUnit endOfColumn = m_columnPositions[coordinate.columns.resolvedFinalPosition.next().toInt()];
+    LayoutUnit startPosition = startOfColumn + marginStartForChild(child);
+    // FIXME: This should account for the grid item's <overflow-position>.
+    LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().justifySelfOverflowAlignment(), endOfColumn - startOfColumn, child.logicalWidth() + child.marginLogicalWidth());
+
+    switch (rowAxisPositionForChild(child)) {
+    case GridAxisStart:
+        return startPosition;
+    case GridAxisEnd:
+        return startPosition + offsetFromStartPosition;
+    case GridAxisCenter:
+        return startPosition + offsetFromStartPosition / 2;
+    }
+
+    ASSERT_NOT_REACHED();
     return 0;
 }

[svillar, 2015/06/10 08:27:20]

Looks like we could even apply a further refactoring to columnPositionForChild
and rowPositionForChild ? They're almost identical...

[jfernandez, 2015/06/11 12:00:31]

Yeah, I already considered that idea and actually I gave it a try for some time,
but I didn't come up with something useful. Both functions use the same
structure but they call different functions on almost every line. I could give
it a new try in a follow up patch.