[css-grid] Handle alignment with orthogonal flows

third_party/WebKit/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 1652 matching lines @@
 
     LayoutUnit sizeToIncrease = availableSpace / numberOfAutoSizedTracks;
     for (const auto& trackIndex : autoSizedTracksIndex) {
         GridTrack* track = tracks.data() + trackIndex;
         LayoutUnit baseSize = track->baseSize() + sizeToIncrease;
         track->setBaseSize(baseSize);
     }
     availableSpace = LayoutUnit();
 }
 
+bool LayoutGrid::childOverflowingContainingBlockHeight(const LayoutBox& child) const
+{
+    LayoutUnit containingBlockContentHeight = child.hasOverrideContainingBlockHeight() ? child.overrideContainingBlockContentHeight() : LayoutUnit();
+    return child.size().height() > containingBlockContentHeight;

[cbiesinger, 2016/06/23 18:35:12]

Don't you need to add the margin-top to the child's size here?

[jfernandez, 2016/06/24 12:59:15]

Umm, that's a good question. The issue is that we weren't doing so before this
changes. I'm just adapting the code to consider an orthogonal flow scenario. 

Not sure where I should fix that issue in this patch, or perhaps doing a
different patch after filing a new bug with its proper test case. I'll start
investigating this a I'll file the bug if I finally detect the error in current
trunk. 

[Manuel Rego, 2016/07/14 10:13:31]

Indeed there's a bug here, I've just reported it:
https://bugs.chromium.org/p/chromium/issues/detail?id=628155

Maybe it's worth add a TODO in the code pointing to the issue.

+}
+
+bool LayoutGrid::childOverflowingContainingBlockWidth(const LayoutBox& child) const
+{
+    LayoutUnit containingBlockContentWidth = child.hasOverrideContainingBlockWidth() ? child.overrideContainingBlockContentWidth() : LayoutUnit();
+    return child.size().width() > containingBlockContentWidth;

[cbiesinger, 2016/06/23 18:35:12]

same question here

[jfernandez, 2016/06/24 12:59:15]

Done.

+}
+
 void LayoutGrid::layoutGridItems(GridSizingData& sizingData)
 {
     populateGridPositionsForDirection(sizingData, ForColumns);
     populateGridPositionsForDirection(sizingData, ForRows);
     m_gridItemsOverflowingGridArea.resize(0);
 
     for (LayoutBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
         if (child->isOutOfFlowPositioned()) {
             prepareChildForPositionedLayout(*child);
             continue;
         }
 
         // Because the grid area cannot be styled, we don't need to adjust
         // the grid breadth to account for 'box-sizing'.
         LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
         LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
 
         LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChildIncludingAlignmentOffsets(*child, ForColumns, sizingData);
         LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChildIncludingAlignmentOffsets(*child, ForRows, sizingData);
 
         SubtreeLayoutScope layoutScope(*child);
-        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && (child->hasRelativeLogicalHeight() || isOrthogonalChild(*child))))
+        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
             layoutScope.setNeedsLayout(child, LayoutInvalidationReason::GridChanged);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
 
         // Stretching logic might force a child layout, so we need to run it before the layoutIfNeeded
         // call to avoid unnecessary relayouts. This might imply that child margins, needed to correctly
         // determine the available space before stretching, are not set yet.
         applyStretchAlignmentToChildIfNeeded(*child);
 
         child->layoutIfNeeded();
 
         // We need pending layouts to be done in order to compute auto-margins properly.
         updateAutoMarginsInColumnAxisIfNeeded(*child);
         updateAutoMarginsInRowAxisIfNeeded(*child);
 
 #if ENABLE(ASSERT)
         const GridArea& area = cachedGridArea(*child);
         ASSERT(area.columns.startLine() < sizingData.columnTracks.size());
         ASSERT(area.rows.startLine() < sizingData.rowTracks.size());
 #endif
         child->setLogicalLocation(findChildLogicalPosition(*child, sizingData));
 
         // Keep track of children overflowing their grid area as we might need to paint them even if the grid-area is
-        // not visible
-        if (child->logicalHeight() > overrideContainingBlockContentLogicalHeight
-            || child->logicalWidth() > overrideContainingBlockContentLogicalWidth)
+        // not visible.
+        // Using physical dimensions for simplicity, so we can forget about orthogonalty.
+        if (childOverflowingContainingBlockHeight(*child)
+            || childOverflowingContainingBlockWidth(*child))

[svillar, 2016/07/13 09:00:40]

I think we lack a test for this change.

[jfernandez, 2016/07/14 09:29:15]

Done.

             m_gridItemsOverflowingGridArea.append(child);
     }
 }
 
 void LayoutGrid::prepareChildForPositionedLayout(LayoutBox& child)
 {
     ASSERT(child.isOutOfFlowPositioned());
     child.containingBlock()->insertPositionedObject(&child);
 
     PaintLayer* childLayer = child.layer();
@@ skipping 202 matching lines @@
     if (numberOfLines > 1) {
         size_t nextToLastLine = numberOfLines - 2;
         for (size_t i = 0; i < nextToLastLine; ++i)
             positions[i + 1] = positions[i] + offset.distributionOffset + tracks[i].baseSize() + trackGap;
         positions[lastLine] = positions[nextToLastLine] + tracks[nextToLastLine].baseSize();
     }
     auto& offsetBetweenTracks = isRowAxis ? m_offsetBetweenColumns : m_offsetBetweenRows;
     offsetBetweenTracks = offset.distributionOffset;
 }
 
-static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit trackBreadth, LayoutUnit childBreadth)
+static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit trackSize, LayoutUnit childSize)
 {
-    LayoutUnit offset = trackBreadth - childBreadth;
+    LayoutUnit offset = trackSize - childSize;
     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 offset.clampNegativeToZero();
     case OverflowAlignmentUnsafe:
     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).
@@ skipping 119 matching lines @@
         child.setMarginAfter(availableAlignmentSpace, style());
     }
 }
 
 GridAxisPosition LayoutGrid::columnAxisPositionForChild(const LayoutBox& child) const
 {
     bool hasSameWritingMode = child.styleRef().getWritingMode() == styleRef().getWritingMode();
 
     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.
-        // 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 (isOrthogonalChild(child) || hasSameWritingMode) ? GridAxisStart : GridAxisEnd;
+        // Aligns the alignment subject to be flush with the edge of the alignment container

[svillar, 2016/07/13 09:00:40]

"Aligns the alignment subject to be flush"  don't understand that part of the
sentence.

[jfernandez, 2016/07/14 09:29:15]

It's what the Alignment spec literally states at:
https://drafts.csswg.org/css-align/#positional-values

It means it should move the "Alignment Subject" (grid item) to the "Alignment
Container" 's (grid area) edge corresponding to the grid item's start edge.

+        // corresponding to the alignment subject's 'start' side in the column axis.
+        if (isOrthogonalChild(child)) {
+            // If orthogonal writing-modes, self-start will be based on the child's inline-axis
+            // direction (inline-start), because it's the one parallel to the column axis.
+            if (isFlippedBlocksWritingMode(style()->getWritingMode()))
+                return child.style()->isLeftToRightDirection() ? GridAxisEnd : GridAxisStart;
+            return child.style()->isLeftToRightDirection() ? GridAxisStart : GridAxisEnd;
+        }
+        // self-start is based on the child's block-flow direction. That's why we need to check against the grid container's block-flow direction.
+        return 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 (isOrthogonalChild(child) || hasSameWritingMode) ? GridAxisEnd : GridAxisStart;
+        // Aligns the alignment subject to be flush with the edge of the alignment container
+        // corresponding to the alignment subject's 'end' side in the column axis.
+        if (isOrthogonalChild(child)) {
+            // If orthogonal writing-modes, self-end will be based on the child's inline-axis
+            // direction, (inline-end) because it's the one parallel to the column axis.
+            if (isFlippedBlocksWritingMode(style()->getWritingMode()))
+                return child.style()->isLeftToRightDirection() ? GridAxisStart : GridAxisEnd;
+            return child.style()->isLeftToRightDirection() ? GridAxisEnd : GridAxisStart;
+        }
+        // self-end is based on the child's block-flow direction. That's why we need to check against the grid container's block-flow direction.
+        return 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.
+        // Aligns the alignment subject to be flush with the alignment container's 'line-left' edge.
+        // The alignment axis (column axis) is always orthogonal to the inline axis, hence this value behaves as 'start'.
         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 isOrthogonalChild(child) ? GridAxisEnd : GridAxisStart;
+        // Aligns the alignment subject to be flush with the alignment container's 'line-right' edge.
+        // The alignment axis (column axis) is always orthogonal to the inline axis, hence this value behaves as 'start'.
+        return GridAxisStart;
     case ItemPositionCenter:
+        // Centers the alignment subject within its alignment container.

[svillar, 2016/07/13 09:00:40]

The other comments are great but this is not really needed.

[jfernandez, 2016/07/14 09:29:15]

Done.

         return GridAxisCenter;
     case ItemPositionFlexStart: // Only used in flex layout, otherwise equivalent to 'start'.
+        // Aligns the alignment subject to be flush with the alignment container's 'start' edge (block-start) in the column axis.
     case ItemPositionStart:
         return GridAxisStart;
     case ItemPositionFlexEnd: // Only used in flex layout, otherwise equivalent to 'end'.
+        // Aligns the alignment subject to be flush with the alignment container's 'end' edge (block-end) in the column axis.
     case ItemPositionEnd:
         return GridAxisEnd;
     case ItemPositionStretch:
         return GridAxisStart;
     case ItemPositionBaseline:
     case ItemPositionLastBaseline:
         // FIXME: These two require implementing Baseline Alignment. For now, we always 'start' align the child.
         // crbug.com/234191
         return GridAxisStart;
     case ItemPositionAuto:
         break;
     }
 
     ASSERT_NOT_REACHED();
     return GridAxisStart;
 }
 
 GridAxisPosition LayoutGrid::rowAxisPositionForChild(const LayoutBox& child) const
 {
     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 (isOrthogonalChild(child) || hasSameDirection) ? GridAxisStart : GridAxisEnd;
+        // Aligns the alignment subject to be flush with the edge of the alignment container
+        // corresponding to the alignment subject's 'start' side in the row axis.
+        if (isOrthogonalChild(child)) {
+            // If orthogonal writing-modes, self-start will be based on the child's block-axis
+            // direction, because it's the one parallel to the row axis.
+            if (isFlippedBlocksWritingMode(child.style()->getWritingMode()))
+                return styleRef().isLeftToRightDirection() ? GridAxisEnd : GridAxisStart;
+            return styleRef().isLeftToRightDirection() ? GridAxisStart : GridAxisEnd;
+        }
+        // self-start is based on the child's inline-flow direction. That's why we need to check against the grid container's direction.
+        return hasSameDirection ? GridAxisStart : GridAxisEnd;

[svillar, 2016/07/13 09:00:40]

Isn't this exactly the same code as in columnAxisPositionForChild ?

We should not have these pieces of important code duplicated.

[jfernandez, 2016/07/14 09:29:15]

No, It's not the same. Alignment along column-axis (block-axis) does
not depend on the inline flow drection (direction property). Instead, it
only depends on the block flow direction (writing-mode property). 
I don't think they are. We could think in further refactoring of this logic,
but If so, it should be part of a different patch.

     case ItemPositionSelfEnd:
-        // For orthogonal writing-modes, this computes to 'start'
-        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-        return (isOrthogonalChild(child) || hasSameDirection) ? GridAxisEnd : GridAxisStart;
+        // Aligns the alignment subject to be flush with the edge of the alignment container
+        // corresponding to the alignment subject's 'end' side in the row axis.
+        if (isOrthogonalChild(child)) {
+            // If orthogonal writing-modes, self-end will be based on the child's block-axis
+            // direction, because it's the one parallel to the row axis.
+            if (isFlippedBlocksWritingMode(child.style()->getWritingMode()))
+                return styleRef().isLeftToRightDirection() ? GridAxisStart : GridAxisEnd;
+            return styleRef().isLeftToRightDirection() ? GridAxisEnd : GridAxisStart;
+        }
+        // self-end is based on the child's inline-flow direction. That's why we need to check against the grid container's direction.
+        return hasSameDirection ? GridAxisEnd : GridAxisStart;

[svillar, 2016/07/13 09:00:40]

Ditto.

[jfernandez, 2016/07/14 09:29:15]

Already replied above.

     case ItemPositionLeft:
-        return isLTR ? GridAxisStart : GridAxisEnd;
+        // Aligns the alignment subject to be flush with the alignment container's 'line-left' edge.
+        // We want the physical 'left' side, so we have to take account, container's inline-flow direction.
+        return styleRef().isLeftToRightDirection() ? GridAxisStart : GridAxisEnd;
     case ItemPositionRight:
-        return isLTR ? GridAxisEnd : GridAxisStart;
+        // Aligns the alignment subject to be flush with the alignment container's 'line-right' edge.
+        // We want the physical 'right' side, so we have to take account, container's inline-flow direction.
+        return styleRef().isLeftToRightDirection() ? GridAxisEnd : GridAxisStart;
     case ItemPositionCenter:
+        // Centers the alignment subject within its alignment container.

[svillar, 2016/07/13 09:00:40]

Ditto comment.

[jfernandez, 2016/07/14 09:29:15]

Done.

         return GridAxisCenter;
     case ItemPositionFlexStart: // Only used in flex layout, otherwise equivalent to 'start'.
+        // Aligns the alignment subject to be flush with the alignment container's 'start' edge (inline-start) in the row axis.
     case ItemPositionStart:
         return GridAxisStart;
     case ItemPositionFlexEnd: // Only used in flex layout, otherwise equivalent to 'end'.
+        // Aligns the alignment subject to be flush with the alignment container's 'end' edge (inline-end) in the row axis.
     case ItemPositionEnd:
         return GridAxisEnd;
     case ItemPositionStretch:
         return GridAxisStart;
     case ItemPositionBaseline:
     case ItemPositionLastBaseline:
         // FIXME: These two require implementing Baseline Alignment. For now, we always 'start' align the child.
         // crbug.com/234191
         return GridAxisStart;
     case ItemPositionAuto:
@@ skipping 21 matching lines @@
         size_t childEndLine = rowsSpan.endLine();
         LayoutUnit endOfRow = m_rowPositions[childEndLine];
         // m_rowPositions include distribution offset (because of content alignment) and gutters
         // so we need to subtract 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_columnPositions after it).
         LayoutUnit trackGap = guttersSize(ForRows, 2);
         if (childEndLine < m_rowPositions.size() - 1) {
             endOfRow -= trackGap;
             endOfRow -= m_offsetBetweenRows;
         }
-        LayoutUnit childBreadth = child.logicalHeight() + child.marginLogicalHeight();
+        LayoutUnit columnAxisChildSize = isOrthogonalChild(child) ? child.logicalWidth() + child.marginLogicalWidth() : child.logicalHeight() + child.marginLogicalHeight();
         OverflowAlignment overflow = child.styleRef().resolvedAlignment(styleRef(), ItemPositionStretch).overflow();
-        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfRow - startOfRow, childBreadth);
+        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfRow - startOfRow, columnAxisChildSize);
         return startPosition + (axisPosition == GridAxisEnd ? offsetFromStartPosition : offsetFromStartPosition / 2);
     }
     }
 
     ASSERT_NOT_REACHED();
     return LayoutUnit();
 }
 
 LayoutUnit LayoutGrid::rowAxisOffsetForChild(const LayoutBox& child, GridSizingData& sizingData) const
 {
@@ skipping 12 matching lines @@
         size_t childEndLine = columnsSpan.endLine();
         LayoutUnit endOfColumn = m_columnPositions[childEndLine];
         // m_columnPositions include distribution offset (because of content alignment) and gutters
         // so we need to subtract 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).
         LayoutUnit trackGap = guttersSize(ForColumns, 2);
         if (childEndLine < m_columnPositions.size() - 1) {
             endOfColumn -= trackGap;
             endOfColumn -= m_offsetBetweenColumns;
         }
-        LayoutUnit childBreadth = child.logicalWidth() + child.marginLogicalWidth();
-        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().justifySelfOverflowAlignment(), endOfColumn - startOfColumn, childBreadth);
+        LayoutUnit rowAxisChildSize = isOrthogonalChild(child) ? child.logicalHeight() + child.marginLogicalHeight() : child.logicalWidth() + child.marginLogicalWidth();
+        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(child.styleRef().justifySelfOverflowAlignment(), endOfColumn - startOfColumn, rowAxisChildSize);
         return startPosition + (axisPosition == GridAxisEnd ? offsetFromStartPosition : offsetFromStartPosition / 2);
     }
     }
 
     ASSERT_NOT_REACHED();
     return LayoutUnit();
 }
 
 ContentPosition static resolveContentDistributionFallback(ContentDistributionType distribution)
 {
@@ skipping 105 matching lines @@
     return rightGridEdgePosition + alignmentOffset - coordinate;
 }
 
 LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSizingData& sizingData) const
 {
     LayoutUnit columnAxisOffset = columnAxisOffsetForChild(child, sizingData);
     LayoutUnit rowAxisOffset = rowAxisOffsetForChild(child, sizingData);
     // We stored m_columnPosition's data ignoring the direction, hence we might need now
     // to translate positions from RTL to LTR, as it's more convenient for painting.
     if (!style()->isLeftToRightDirection())
-        rowAxisOffset = translateRTLCoordinate(rowAxisOffset) - child.logicalWidth();
+        rowAxisOffset = translateRTLCoordinate(rowAxisOffset) - (isOrthogonalChild(child) ? child.logicalHeight()  : child.logicalWidth());
 
     // "In the positioning phase [...] calculations are performed according to the writing mode
     // of the containing block of the box establishing the orthogonal flow." However, the
     // resulting LayoutPoint will be used in 'setLogicalPosition' in order to set the child's
     // logical position, which will only take into account the child's writing-mode.
     LayoutPoint childLocation(rowAxisOffset, columnAxisOffset);
     return isOrthogonalChild(child) ? childLocation.transposedPoint() : childLocation;
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     if (!m_gridItemArea.isEmpty())
         GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink