[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..7195d7e8b89183cddf56dba753ede6e37453b10b 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 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,18 +256,18 @@ void RenderGrid::addChild(RenderObject* newChild, RenderObject* beforeChild)
 void RenderGrid::addChildToIndexesMap(RenderBox& child)
 {
     ASSERT(!m_gridItemsIndexesMap.contains(&child));
-    RenderBox* sibling = child.nextSiblingBox();
+    RenderBox* sibling = child.nextInFlowSiblingBox();
     bool lastSibling = !sibling;
 
     if (lastSibling)
-        sibling = child.previousSiblingBox();
+        sibling = child.previousInFlowSiblingBox();
 
     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->nextInFlowSiblingBox())
             m_gridItemsIndexesMap.set(sibling, m_gridItemsIndexesMap.get(sibling) + 1);
     }
 
@@ -286,6 +290,9 @@ void RenderGrid::removeChild(RenderObject* child)
         return;
     }
 
+    if (child->isOutOfFlowPositioned())
+        return;
+
     const RenderBox* childBox = toRenderBox(child);
     GridCoordinate coordinate = m_gridItemCoordinate.take(childBox);
 
@@ -898,6 +905,9 @@ 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);
@@ -939,7 +949,8 @@ void RenderGrid::populateExplicitGridAndOrderIterator()
 
     ASSERT(m_gridItemsIndexesMap.isEmpty());
     size_t childIndex = 0;
-    for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
+    for (RenderBox* child = firstChildBox(); child; child = child->nextInFlowSiblingBox()) {
+

[Julien - ping for review, 2014/12/02 20:46:05]

Unneeded space.

         populator.collectChild(child);
         m_gridItemsIndexesMap.set(child, childIndex++);
 
@@ -1098,10 +1109,8 @@ 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);
+            continue;
         }
 
         // Because the grid area cannot be styled, we don't need to adjust
@@ -1145,6 +1154,85 @@ void RenderGrid::layoutGridItems()
     setLogicalHeight(logicalHeight() + borderAndPaddingLogicalHeight());
 }
 
+void RenderGrid::layoutPositionedObjects(bool relayoutChildren, PositionedLayoutBehavior info)
+{
+    TrackedRendererListHashSet* positionedDescendants = positionedObjects();
+    if (!positionedDescendants)
+        return;
+
+    bool containerHasHorizontalWritingMode = isHorizontalWritingMode();
+    TrackedRendererListHashSet::iterator end = positionedDescendants->end();
+    for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) {
+        RenderBox* child = *it;
+
+        bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != containerHasHorizontalWritingMode;
+        if (hasOrthogonalWritingMode) {
+            // FIXME: Properly support orthogonal writing mode.
+            continue;
+        }
+
+        // FIXME: Detect properly if start/end is auto for inexistent named grid lines.

[Julien - ping for review, 2014/12/02 20:46:05]

Should we refer to the www-style thread here for future reference?

[Manuel Rego, 2014/12/03 09:48:58]

Not sure if to the thread itself, as it's expected that the spec gets
an update on this topic (also there're already 2 related threads :-)).

At this moment, there isn't a final resolution. And some things
might change depending on the discussion in the CSS WG.

+        bool columnStartIsAuto = child->style()->gridColumnStart().isAuto();
+        LayoutUnit columnBreadth = LayoutUnit(0);
+        LayoutUnit columnOffset = offsetAndBreadthForPositionedChild(*child, ForColumns, columnStartIsAuto, child->style()->gridColumnEnd().isAuto(), columnBreadth);
+        bool rowStartIsAuto = child->style()->gridRowStart().isAuto();
+        LayoutUnit rowBreadth = LayoutUnit(0);
+        LayoutUnit rowOffset = offsetAndBreadthForPositionedChild(*child, ForRows, rowStartIsAuto, child->style()->gridRowEnd().isAuto(), rowBreadth);
+
+        child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
+        child->setOverrideContainingBlockContentLogicalHeight(rowBreadth);
+        child->setOverrideInlineOffset(columnOffset);
+        child->setOverrideBlockOffset(rowOffset);

[Julien - ping for review, 2014/12/02 20:46:05]

Those 2 fields are a lot cleaner of the previous version but:
- the name is not aligned with the intent (it's an extra offset, not an override
offset).
- the logic is now harder to follow as you set the values here but read them in
another part of the code.

The increase in cleanliness seems greater than the small loss in hackability
though.

[Manuel Rego, 2014/12/03 09:48:58]

Yeah, I've changed the name.
Before it was similar as I was setting it here and using it in RenderBox too.

+
+        if (child->parent() == this) {

[Julien - ping for review, 2014/12/02 20:46:05]

I don't understand that check. It seems *very wrong* to check parent() and not
containingBlock() (which would be trivially true but that's beside the point).

[Manuel Rego, 2014/12/03 09:48:58]

This was on purpose to differentiate grid items (direct children)
from other descendant.
As you said checking the containingBlock() won't be needed
as it's going to be always true.

Basically this follows what Tab said in the next mail (2nd question):
http://lists.w3.org/Archives/Public/www-style/2014Nov/0549.html
Because of the not direct children will use the standard static position
already defined, and we won't need to update it here.

Of course, this might change as it's still under discussion.

+            RenderLayer* childLayer = child->layer();
+            if (columnStartIsAuto)
+                childLayer->setStaticInlinePosition(borderAndPaddingStart());
+            else
+                childLayer->setStaticInlinePosition(borderStart() + columnOffset);

[Julien - ping for review, 2014/12/02 20:46:05]

AFAICT the column starts in the content box so the offset for an element should
always include the border and padding from the containing block.

[Manuel Rego, 2014/12/03 09:48:58]

This is for the case where we have grid-column-start different from auto.
And we don't have set any offset (left or right), so it's going to use
the static position.

For example:
grid-column: 3 / auto;
In this case the columnOffset already includes the padding
(included in RenderGrid::offsetAndBreadthForPositionedChild())
so we don't need to add the padding too.

[Julien - ping for review, 2014/12/05 00:32:55]

Let's put a comment in this case. It's weird that offset doesn't include the
border too.

[Manuel Rego, 2014/12/05 10:47:08]

Done.

+            if (rowStartIsAuto)
+                childLayer->setStaticBlockPosition(borderAndPaddingBefore());
+            else
+                childLayer->setStaticBlockPosition(borderBefore() + rowOffset);
+        }
+    }
+
+    RenderBlock::layoutPositionedObjects(relayoutChildren, info);
+}
+
+LayoutUnit RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child, GridTrackSizingDirection direction, bool startIsAuto, bool endIsAuto, LayoutUnit& breadth)
+{
+    ASSERT(child.isHorizontalWritingMode() == isHorizontalWritingMode());
+
+    OwnPtr<GridSpan> positions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), child, direction);
+    if (!positions) {
+        breadth = (direction == ForColumns) ? clientLogicalWidth() : clientLogicalHeight();
+        return LayoutUnit(0);
+    }
+
+    GridResolvedPosition firstPosition = GridResolvedPosition(0);
+    GridResolvedPosition initialPosition = startIsAuto ? firstPosition : positions->resolvedInitialPosition;
+    GridResolvedPosition lastPosition = GridResolvedPosition((direction == ForColumns ? gridColumnCount() : gridRowCount()) - 1);
+    GridResolvedPosition finalPosition = endIsAuto ? lastPosition : positions->resolvedFinalPosition;
+
+    LayoutUnit start = startIsAuto ? LayoutUnit(0) : (direction == ForColumns) ?  m_columnPositions[initialPosition.toInt()] : m_rowPositions[initialPosition.toInt()];
+    LayoutUnit end = endIsAuto ? (direction == ForColumns) ? logicalWidth() : logicalHeight() : (direction == ForColumns) ?  m_columnPositions[finalPosition.next().toInt()] : m_rowPositions[finalPosition.next().toInt()];
+
+    breadth = end - start;
+
+    if (startIsAuto)
+        breadth -= (direction == ForColumns) ? borderStart() : borderBefore();
+    else
+        start -= ((direction == ForColumns) ? borderStart() : borderBefore());
+
+    if (endIsAuto) {
+        breadth -= (direction == ForColumns) ? borderEnd() : borderAfter();
+        breadth -= isHorizontalWritingMode() ? verticalScrollbarWidth() : horizontalScrollbarHeight();
+    }
+
+    return start;
+}
+
 GridCoordinate RenderGrid::cachedGridCoordinate(const RenderBox& gridItem) const
 {
     ASSERT(m_gridItemCoordinate.contains(&gridItem));