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

Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1312 matching lines @@
     m_columnPositions[0] = borderAndPaddingStart() + sizingData.columnsPositionOffset;
     for (unsigned i = 0; i < numberOfColumnTracks; ++i)
         m_columnPositions[i + 1] = m_columnPositions[i] + sizingData.columnsDistributionOffset + sizingData.columnTracks[i].baseSize();
 
     m_rowPositions.resize(numberOfRowTracks + 1);
     m_rowPositions[0] = borderAndPaddingBefore() + sizingData.rowsPositionOffset;
     for (unsigned i = 0; i < numberOfRowTracks; ++i)
         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();
     return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
 }
 
 bool LayoutGrid::allowedToStretchLogicalHeightForChild(const LayoutBox& child) const
 {
     return child.style()->logicalHeight().isAuto() && !child.style()->marginBeforeUsing(style()).isAuto() && !child.style()->marginAfterUsing(style()).isAuto();
 }
@@ skipping 81 matching lines @@
             if (childNeedsRelayout || !child.hasOverrideLogicalContentHeight())
                 child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
             if (childNeedsRelayout) {
                 child.setLogicalHeight(0);
                 child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
             }
         }
     }
 }
 
+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.

+        // 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.
+        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.
+        // 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.
+        return (hasOrthogonalWritingMode || hasSameWritingMode) ? GridAxisEnd : GridAxisStart;
+    case ItemPositionLeft:
+        // The alignment axis (column axis) and the inline axis are parallell in
+        // orthogonal writing mode. Otherwise this this is equivalent to 'start'.
+        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
+        return GridAxisStart;
+    case ItemPositionRight:
+        // The alignment axis (column axis) and the inline axis are parallell in
+        // orthogonal writing mode. Otherwise this this is equivalent to 'start'.
+        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
+        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;
+    }
+
+    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 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.
+        return GridAxisStart;
+    case ItemPositionAuto:
+        break;
+    }
+
+    ASSERT_NOT_REACHED();
+    return GridAxisStart;
+}
+
 LayoutUnit LayoutGrid::rowPositionForChild(const LayoutBox& child) const
 {
-    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    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.
-        if (hasOrthogonalWritingMode)
-            return startOfRowForChild(child);
+    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());
 
-        // 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);
-    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);
-    case ItemPositionLeft:
-        // The alignment axis (column axis) and the inline axis are parallell in
-        // orthogonal writing mode.
-        // 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);
-    case ItemPositionRight:
-        // The alignment axis (column axis) and the inline axis are parallell in
-        // orthogonal writing mode.
-        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        if (hasOrthogonalWritingMode)
-            return endOfRowForChild(child);
-
-        // Otherwise this this is equivalent to 'start'.
-        return startOfRowForChild(child);
-    case ItemPositionCenter:
-        return centeredRowPositionForChild(child);
-        // Only used in flex layout, for other layout, it's equivalent to 'start'.
-    case ItemPositionFlexStart:
-    case ItemPositionStart:
-        return startOfRowForChild(child);
-        // Only used in flex layout, for other layout, it's equivalent to 'end'.
-    case ItemPositionFlexEnd:
-    case ItemPositionEnd:
-        return endOfRowForChild(child);
-    case ItemPositionStretch:
-        return startOfRowForChild(child);
-    case ItemPositionBaseline:
-    case ItemPositionLastBaseline:
-        // FIXME: Implement the ItemPositionBaseline value. For now, we always 'start' align the child.
-        return startOfRowForChild(child);
-    case ItemPositionAuto:
-        break;
+    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.

+
 ContentPosition static resolveContentDistributionFallback(ContentDistributionType distribution)
 {
     switch (distribution) {
     case ContentDistributionSpaceBetween:
         return ContentPositionStart;
     case ContentDistributionSpaceAround:
         return ContentPositionCenter;
     case ContentDistributionSpaceEvenly:
         return ContentPositionCenter;
     case ContentDistributionStretch:
@@ skipping 160 matching lines @@
 
     return LayoutPoint(columnPosition, rowPositionForChild(child));
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink