[CSS Grid Layout] Fix positioned grid children position and size

Source/core/rendering/RenderGrid.cpp

Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp b/Source/core/rendering/RenderGrid.cpp
index b24a0b5504b229d8335598d4f41d51f58b3c8dc7..6f148c94f0a35b2f53a9dfc01e88af72fc7767cb 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -1040,6 +1040,11 @@ void RenderGrid::dirtyGrid()
     m_gridItemsIndexesMap.clear();
 }
 
+bool RenderGrid::isAbsolutelyPositionedGridItemWithContainingBlockThis(const RenderBox& child) const

[Julien - ping for review, 2014/11/11 17:27:35]

That's better in my book (feel free to adapt / disagree / improve):
isContainingBlockForAbsolutelyPositionedGridItem

+{
+    return (child.style()->position() == AbsolutePosition) && (child.containingBlock() == this);

[Julien - ping for review, 2014/11/11 17:27:35]

The parentheses are not required and we don't put them in Blink.

+}
+
 void RenderGrid::layoutGridItems()
 {
     placeItemsOnGrid();
@@ -1054,28 +1059,49 @@ void RenderGrid::layoutGridItems()
     m_gridItemsOverflowingGridArea.resize(0);
 
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
-        if (child->isOutOfFlowPositioned()) {
-            // FIXME: Absolute positioned grid items should have a special
-            // behavior as described in the spec (crbug.com/273898):
-            // http://www.w3.org/TR/css-grid-1/#abspos-items
-            child->containingBlock()->insertPositionedObject(child);
-        }
-
         // 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 = gridAreaBreadthForChild(*child, ForColumns, sizingData.columnTracks);
-        LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(*child, ForRows, sizingData.rowTracks);
+        LayoutUnit columnExtraOffset = 0;
+        LayoutUnit rowExtraOffset = 0;
+        LayoutUnit overrideContainingBlockContentLogicalWidth;
+        LayoutUnit overrideContainingBlockContentLogicalHeight;
 
-        SubtreeLayoutScope layoutScope(*child);
-        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
-            layoutScope.setNeedsLayout(child);
+        if (isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {

[Julien - ping for review, 2014/11/11 17:27:35]

I think the logic is off actually.

RenderBlock ignores absolutely / fixed positioned elements during layout and lay
them out after using the positioned map. I think we will want to do something
similar.

The reason is that grid items now can be descendants and not just direct
children (which means we will have to rethink this deep assumption in
RenderGrid).

[Manuel Rego, 2014/11/12 13:36:34]

I'm just focusing in this comment right now, as I'm not sure if I'm getting it
properly.

Do you mean that any absolutely positioned element under a grid, where the
containing block is the grid container, should be considered a grid item?

If I'm not wrong according to the spec grid items are direct children of the
grid container.
And placement properties like grid-row/column only apply to grid items.

For example:
<div id="grid" style="display: grid; position: relative;">
  <div id="item">
    <div id="abspos" style="position: absolute;"></div>
  </div>
</div>

In this case, despite "abspos" containing block is "grid", IMHO it's not a grid
item.
It seems to me that these modifications related to the position inside the grid
shouldn't be applied.
And "abspos" should behave almost the same than if "grid" was a regular block.
Following what is defined at:
http://dev.w3.org/csswg/css-grid/#static-position

However, for grid items we should do what is explained at:
http://dev.w3.org/csswg/css-grid/#abspos-items

Which is basically what this patch is trying to implement. The other part is
pending yet (I'm not sure if it's better to do it in the same patch or a
separated
one, but that's a different issue).

What do you think?

[Julien - ping for review, 2014/11/12 19:16:57]

That's a good question and something the specification is not clear on actually.
I would have called them grid items as their containing block is the grid
container but you're right that grid item is defined as direct children of the
grid container.

Whether we call them grid item or not doesn't change the fact that any
descendants should follow http://dev.w3.org/csswg/css-grid/#abspos-items (note
that the paragraph doesn't mention grid item at all, just what containing block
an element has).
I think we should get the specification clarified if you disagree with me. I
think the specification is pretty clear on what to do with absolutely positioned
elements though.

Also regardless of our disagreement over how to interpret the specification, the
absolutely positioned elements should be handled separately like other
containers, which should make the code clearer.

[Manuel Rego, 2014/11/18 14:24:06]

The title of the section was talking about grid items, but I've
clarified it in www-style and it seems that you were right (check the
following thread
http://lists.w3.org/Archives/Public/www-style/2014Nov/0099.html for more
details).
Yeah, it's clear that we need to separate this code, so I've overridden
RenderBlock::layoutPositionedObjects() to deal with all this logic for
both grit items (direct children) but also any other descendant.

+            overrideContainingBlockContentLogicalWidth = gridAreaBreadthForAbsolutelyPositionedChild(*child, ForColumns, sizingData.columnTracks, columnExtraOffset);
+            overrideContainingBlockContentLogicalHeight = gridAreaBreadthForAbsolutelyPositionedChild(*child, ForRows, sizingData.rowTracks, rowExtraOffset);
+        } else {
+            overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(*child, ForColumns, sizingData.columnTracks);
+            overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(*child, ForRows, sizingData.rowTracks);
+        }
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
 
+        if (child->isOutOfFlowPositioned()) {
+            child->containingBlock()->insertPositionedObject(child);
+
+            if (isAbsolutelyPositionedGridItemWithContainingBlockThis(*child)) {
+                bool containerHasHorizontalWritingMode = isHorizontalWritingMode();
+                LayoutPoint position = LayoutPoint(startOfColumnForChild(*child), startOfRowForChild(*child));
+
+                RenderLayer* childLayer = child->layer();
+                // Set the layer position taking into account the extra size
+                // increased in RenderGrid::gridAreaBreadthForChild() due to the
+                // positioned items particularities.
+                childLayer->setStaticInlinePosition(containerHasHorizontalWritingMode ? position.x() - columnExtraOffset : position.y() - rowExtraOffset);
+                childLayer->setStaticBlockPosition(containerHasHorizontalWritingMode ? position.y() - rowExtraOffset : position.x() - columnExtraOffset);
+            }
+
+            continue;
+        }
+
+        SubtreeLayoutScope layoutScope(*child);
+        if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
+            layoutScope.setNeedsLayout(child);
+
         // FIXME: Grid items should stretch to fill their cells. Once we
         // implement grid-{column,row}-align, we can also shrink to fit. For
         // now, just size as if we were a regular child.
@@ -1109,13 +1135,68 @@ GridCoordinate RenderGrid::cachedGridCoordinate(const RenderBox& gridItem) const
     return m_gridItemCoordinate.get(&gridItem);
 }
 
+static LayoutUnit computeGridAreaBreadth(const GridSpan& span, const Vector<GridTrack>& tracks, LayoutUnit* extraInitialOffset = 0, bool startIsAuto = false, GridResolvedPosition* originalInitialPosition = 0)
+{
+    LayoutUnit gridAreaBreadth = 0;
+    for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition) {
+        LayoutUnit trackBreadth = tracks[trackPosition.toInt()].m_usedBreadth;
+        gridAreaBreadth += trackBreadth;
+        if (extraInitialOffset && startIsAuto && trackPosition < *originalInitialPosition)
+            *extraInitialOffset += trackBreadth;
+    }
+    return gridAreaBreadth;
+}
+
 LayoutUnit RenderGrid::gridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks) const
 {
     const GridCoordinate& coordinate = cachedGridCoordinate(child);
     const GridSpan& span = (direction == ForColumns) ? coordinate.columns : coordinate.rows;
+    return computeGridAreaBreadth(span, tracks);
+}
+
+LayoutUnit RenderGrid::gridAreaBreadthForAbsolutelyPositionedChild(const RenderBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks, LayoutUnit& extraInitialOffset) const
+{
+    ASSERT(isAbsolutelyPositionedGridItemWithContainingBlockThis(child));
+
     LayoutUnit gridAreaBreadth = 0;
-    for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition)
-        gridAreaBreadth += tracks[trackPosition.toInt()].m_usedBreadth;
+    bool startIsAuto = false;
+    bool endIsAuto = false;
+    // For absolutely positioned items the containing block corresponds to the
+    // padding edges of the grid. So, we need to increase the breadth if the
+    // positions are auto.
+    if (direction == ForColumns) {
+        if (child.style()->gridColumnStart().isAuto()) {
+            startIsAuto = true;
+            gridAreaBreadth += paddingLeft();
+            extraInitialOffset += paddingLeft();
+        }
+        if (child.style()->gridColumnEnd().isAuto()) {
+            endIsAuto = true;
+            gridAreaBreadth += paddingRight();
+        }
+    } else {
+        if (child.style()->gridRowStart().isAuto()) {
+            startIsAuto = true;
+            gridAreaBreadth += paddingTop();
+            extraInitialOffset += paddingTop();
+        }
+        if (child.style()->gridRowEnd().isAuto()) {
+            endIsAuto = true;
+            gridAreaBreadth += paddingBottom();
+        }
+    }
+
+    const GridCoordinate& coordinate = cachedGridCoordinate(child);
+    GridResolvedPosition initialPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
+    GridResolvedPosition originalInitialPosition = initialPosition;
+    GridResolvedPosition finalPosition = (direction == ForColumns) ? coordinate.columns.resolvedFinalPosition : coordinate.rows.resolvedFinalPosition;
+
+    if (startIsAuto)
+        initialPosition = GridResolvedPosition(0);
+    if (endIsAuto)
+        finalPosition = GridResolvedPosition((direction == ForColumns ? gridColumnCount() : gridRowCount()) - 1);
+
+    gridAreaBreadth += computeGridAreaBreadth(GridSpan(initialPosition, finalPosition), tracks, &extraInitialOffset, startIsAuto, &originalInitialPosition);
     return gridAreaBreadth;
 }