[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 11474d6b9153b483983e0f05f4bbff8f4b23fdc0..c490ed0452ee217aaf7ae2e58c32300d04bdcf3c 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -224,6 +224,10 @@ void RenderGrid::addChild(RenderObject* newChild, RenderObject* beforeChild)
         return;
     }
 
+    // Positioned items that shouldn't take up space or otherwise participate in the layout of the grid.
+    if (newChild->isOutOfFlowPositioned())
+        return;
+
     // If the new child has been inserted inside an existent anonymous block, we can simply ignore it as the anonymous
     // block is an already known grid item.
     if (newChild->parent() != this)
@@ -252,19 +256,30 @@ void RenderGrid::addChild(RenderObject* newChild, RenderObject* beforeChild)
 void RenderGrid::addChildToIndexesMap(RenderBox& child)
 {
     ASSERT(!m_gridItemsIndexesMap.contains(&child));
-    RenderBox* sibling = child.nextSiblingBox();
+    RenderBox* sibling;
+    for (sibling = child.nextSiblingBox(); sibling; sibling = sibling->nextSiblingBox()) {

[Julien - ping for review, 2014/11/18 21:12:44]

nextSiblingBox doesn't check the type and IIRC we had quite some issues with bad
casts. Won't that make us more prone to such mistakes?

[Manuel Rego, 2014/11/19 15:18:29]

We solved those issues directly in RenderGrid::addChild().

I don't really know what we can do here as we need to skip the
positioned grid items. Besides, we were already calling nextSiblingBox()
before. Any idea?

[Julien - ping for review, 2014/11/19 18:46:43]

I didn't have a good way forward, only mentioned that as you may have had some
better idea :-)

There is not much we can do AFAICT so let's ignore that.

+        if (!sibling->isOutOfFlowPositioned())
+            break;
+    }
     bool lastSibling = !sibling;
 
-    if (lastSibling)
-        sibling = child.previousSiblingBox();
+    if (lastSibling) {
+        for (sibling = child.previousSiblingBox(); sibling; sibling = sibling->previousSiblingBox()) {
+            if (!sibling->isOutOfFlowPositioned())
+                break;
+        }
+    }
 
     size_t index = 0;
     if (sibling)
         index = lastSibling ? m_gridItemsIndexesMap.get(sibling) + 1 : m_gridItemsIndexesMap.get(sibling);
 
     if (sibling && !lastSibling) {
-        for (; sibling; sibling = sibling->nextSiblingBox())
+        for (; sibling; sibling = sibling->nextSiblingBox()) {
+            if (sibling->isOutOfFlowPositioned())
+                continue;
             m_gridItemsIndexesMap.set(sibling, m_gridItemsIndexesMap.get(sibling) + 1);
+        }
     }
 
     m_gridItemsIndexesMap.set(&child, index);
@@ -286,6 +301,9 @@ void RenderGrid::removeChild(RenderObject* child)
         return;
     }
 
+    if (child->isOutOfFlowPositioned())
+        return;
+
     const RenderBox* childBox = toRenderBox(child);
     GridCoordinate coordinate = m_gridItemCoordinate.take(childBox);
 
@@ -898,6 +916,10 @@ void RenderGrid::placeItemsOnGrid()
     Vector<RenderBox*> autoMajorAxisAutoGridItems;
     Vector<RenderBox*> specifiedMajorAxisAutoGridItems;
     for (RenderBox* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
+        if (child->isOutOfFlowPositioned()) {
+            continue;
+        }
+
         // FIXME: We never re-resolve positions if the grid is grown during auto-placement which may lead auto / <integer>
         // positions to not match the author's intent. The specification is unclear on what should be done in this case.
         OwnPtr<GridSpan> rowPositions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), *child, ForRows);
@@ -940,6 +962,10 @@ void RenderGrid::populateExplicitGridAndOrderIterator()
     ASSERT(m_gridItemsIndexesMap.isEmpty());
     size_t childIndex = 0;
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+        if (child->isOutOfFlowPositioned()) {

[Julien - ping for review, 2014/11/18 21:12:44]

FWIW that's the kind of code that makes me advocate for some
no-out-of-flow-positioned-iterator. Most rendering traversals follow this
pattern and those who need it should be investigated.

[Manuel Rego, 2014/11/19 15:18:29]

Yeah, I understand what you mean, in several cases we don't need the
positioned items.

+            continue;
+        }
+
         populator.collectChild(child);
         m_gridItemsIndexesMap.set(child, childIndex++);
 
@@ -1098,10 +1124,9 @@ void RenderGrid::layoutGridItems()
 
     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);
+            adjustPositionedGridItem(child);
+            continue;
         }
 
         // Because the grid area cannot be styled, we don't need to adjust
@@ -1145,6 +1170,82 @@ void RenderGrid::layoutGridItems()
     setLogicalHeight(logicalHeight() + borderAndPaddingLogicalHeight());
 }
 
+void RenderGrid::adjustPositionedGridItem(RenderBox* child)
+{
+    bool containerHasHorizontalWritingMode = isHorizontalWritingMode();
+    RenderLayer* childLayer = child->layer();
+    childLayer->setStaticInlinePosition(containerHasHorizontalWritingMode ? borderLeft() + paddingLeft() : borderTop() + paddingTop());
+    childLayer->setStaticBlockPosition(containerHasHorizontalWritingMode ? borderTop() + paddingTop() : borderLeft() + paddingLeft());
+}
+
+void RenderGrid::layoutPositionedObjects(bool relayoutChildren, PositionedLayoutBehavior info)
+{
+    TrackedRendererListHashSet* positionedDescendants = positionedObjects();
+    if (!positionedDescendants)
+        return;
+
+    RenderBox* child;

[Julien - ping for review, 2014/11/18 21:12:44]

Why is child defined outside the for-loop?

[Manuel Rego, 2014/11/19 15:18:29]

A mistake, sorry.

+    TrackedRendererListHashSet::iterator end = positionedDescendants->end();
+    for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) {
+        child = *it;
+
+        if (child->style()->position() == AbsolutePosition) {

[Julien - ping for review, 2014/11/18 21:12:44]

Fixed positioned elements are never handled here. The specification doesn't
account for them either, which is an oversight as some properties make them
containing block for fixed positioned elements (e.g 'transform')

[Manuel Rego, 2014/11/19 15:18:29]

Yeah, I think you're right. I've removed the if and notified CSS WG in
order to update the spec.
In addition, I've added some examples with fixed elements in the tests.

+            LayoutUnit columnBreadth = clientLogicalWidth();
+            LayoutUnit columnOffset = offsetForAbsolutelyPositionedChild(*child, ForColumns, columnBreadth);
+            LayoutUnit rowBreadth = clientLogicalHeight();
+            LayoutUnit rowOffset = offsetForAbsolutelyPositionedChild(*child, ForRows, rowBreadth);
+
+            bool containerHasHorizontalWritingMode = isHorizontalWritingMode();
+            RenderLayer* childLayer = child->layer();
+            childLayer->setStaticInlinePosition(childLayer->staticInlinePosition() + (containerHasHorizontalWritingMode ? columnOffset : rowOffset));
+            childLayer->setStaticBlockPosition(childLayer->staticBlockPosition() + (containerHasHorizontalWritingMode ? rowOffset : columnOffset));
+
+            child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);

[Julien - ping for review, 2014/11/18 21:12:44]

columnBreadth is the client box, which is the padding box. You want to use the
content box here (as the name of the function implies).

[Manuel Rego, 2014/11/19 15:18:29]

It's the padding box of the grid container, and it seems that should be
the containing block of the positioned items. From the spec:
"the containing block will correspond to the padding edges of the grid
container"

[Julien - ping for review, 2014/11/19 18:46:43]

You're totally right about the specification. I meant that
setOverrideContainingBlockContentLogicalWidth expects the containing block's
content box and we are passing in the padding box. I am concern that this
mismatch will hurt us, especially with box-sizing.

+            child->setOverrideContainingBlockContentLogicalHeight(rowBreadth);
+        }
+    }
+
+    RenderBlock::layoutPositionedObjects(relayoutChildren, info);
+}
+
+LayoutUnit RenderGrid::offsetForAbsolutelyPositionedChild(const RenderBox& child, GridTrackSizingDirection direction, LayoutUnit& breadth)
+{
+    LayoutUnit offset = LayoutUnit(0);
+
+    OwnPtr<GridSpan> positions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), child, direction);
+    if (!positions)
+        return offset;
+
+    if ((direction == ForColumns && !child.style()->gridColumnStart().isAuto()) || (direction == ForRows && !child.style()->gridRowStart().isAuto())) {
+        breadth -= (direction == ForColumns) ? paddingLeft() : paddingTop();

[Julien - ping for review, 2014/11/18 21:12:44]

Columns / rows follow the logical order so those should be paddingLogicalLeft /
paddingLogicalTop. We should also ASSERT that we are not in an orthogonal flow
as the logic would be totally wrong in this case.

[Manuel Rego, 2014/11/19 15:18:29]

Yes, you're totally right.
I've added a new test to check that writing modes behavior is right.

+
+        GridResolvedPosition firstPosition = GridResolvedPosition(0);
+        if (positions->resolvedInitialPosition != firstPosition) {
+            for (GridResolvedPosition position = firstPosition; position < positions->resolvedInitialPosition; ++position) {
+                LayoutUnit trackBreadth = (direction == ForColumns) ? m_columnPositions[position.toInt() + 1] - m_columnPositions[position.toInt()] : m_rowPositions[position.toInt() + 1] - m_rowPositions[position.toInt()];
+                breadth -= trackBreadth;
+                offset += trackBreadth;
+            }
+        }
+    } else if ((direction == ForColumns && !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows && !child.style()->gridRowEnd().isAuto())) {
+        offset -= (direction == ForColumns) ? paddingLeft() : paddingTop();

[Julien - ping for review, 2014/11/18 21:12:44]

Same comment, physical properties vs logical ones.

[Manuel Rego, 2014/11/19 15:18:29]

Done.

+    }
+
+    if ((direction == ForColumns && !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows && !child.style()->gridRowEnd().isAuto())) {
+        breadth -= (direction == ForColumns) ? paddingRight() : paddingBottom();

[Julien - ping for review, 2014/11/18 21:12:44]

Ditto.

[Manuel Rego, 2014/11/19 15:18:29]

Done.

+
+        GridResolvedPosition lastPosition = GridResolvedPosition(direction == ForColumns ? gridColumnCount() : gridRowCount());
+        if (positions->resolvedFinalPosition.next() != lastPosition) {
+            for (GridResolvedPosition position = positions->resolvedFinalPosition.next(); position < lastPosition; ++position) {
+                LayoutUnit trackBreadth = (direction == ForColumns) ? m_columnPositions[position.toInt() + 1] - m_columnPositions[position.toInt()] : m_rowPositions[position.toInt() + 1] - m_rowPositions[position.toInt()];
+                breadth -= trackBreadth;
+            }
+        }
+    }
+
+    return offset;
+}
+
 GridCoordinate RenderGrid::cachedGridCoordinate(const RenderBox& gridItem) const
 {
     ASSERT(m_gridItemCoordinate.contains(&gridItem));