[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..21517c2f89fd339f26bb4f84385dbbe402ff8493 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()) {
+        if (!sibling->isOutOfFlowPositioned())
+            break;
+    }
     bool lastSibling = !sibling;
 
-    if (lastSibling)
-        sibling = child.previousSiblingBox();
+    if (lastSibling) {
+        for (sibling = child.previousSiblingBox(); sibling; sibling = sibling->previousSiblingBox()) {

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

That would probably be nice to move into a helper function like
previousInFlowSiblingBox(). Same for the nextInFlowSiblingBox().

[Manuel Rego, 2014/11/19 23:26:23]

Yes I agree, it makes the code easier to understand.

+            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()) {
+            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,86 @@ void RenderGrid::layoutGridItems()
     setLogicalHeight(logicalHeight() + borderAndPaddingLogicalHeight());
 }
 
+void RenderGrid::adjustPositionedGridItem(RenderBox* child)
+{
+    RenderLayer* childLayer = child->layer();
+    childLayer->setStaticInlinePosition(borderAndPaddingStart());
+    childLayer->setStaticBlockPosition(borderAndPaddingBefore());
+}
+
+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;
+        }
+
+        LayoutUnit columnBreadth = clientLogicalWidth();
+        LayoutUnit columnOffset = offsetForPositionedChild(*child, ForColumns, columnBreadth);
+        LayoutUnit rowBreadth = clientLogicalHeight();
+        LayoutUnit rowOffset = offsetForPositionedChild(*child, ForRows, rowBreadth);
+
+        RenderLayer* childLayer = child->layer();
+        childLayer->setStaticInlinePosition(childLayer->staticInlinePosition() + (containerHasHorizontalWritingMode ? columnOffset : rowOffset));
+        childLayer->setStaticBlockPosition(childLayer->staticBlockPosition() + (containerHasHorizontalWritingMode ? rowOffset : columnOffset));
+
+        child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
+        child->setOverrideContainingBlockContentLogicalHeight(rowBreadth);
+    }
+
+    RenderBlock::layoutPositionedObjects(relayoutChildren, info);
+}
+
+LayoutUnit RenderGrid::offsetForPositionedChild(const RenderBox& child, GridTrackSizingDirection direction, LayoutUnit& breadth)

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

The API is weird: you wouldn't assume that it touched |breadth| as the name
doesn't imply it.

I would probably modify it to something that conveys the intent more clearly
(feel free to disagree / improve upon, it's rough):

void RenderGrid::offsetAndBreadthForPositionedChild(const RenderBox& child,
GridTrackSizingDirection direction, LayoutUnit& offset, LayoutUnit& breadth)
{
    offset = LayoutUnit(0);
    breadth = (direction == ForColumn) ? clientLogicalWidth() :
clientLogicalHeight();
    ....
}

[Manuel Rego, 2014/11/19 23:26:23]

I added "andBreadth" but kept the return value, as the final code seems
simpler for me.
But I can make it an argument if needed.

+{
+    ASSERT(child.isHorizontalWritingMode() == isHorizontalWritingMode());
+
+    LayoutUnit offset = LayoutUnit(0);

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

You don't use offset until below so we should move this definition down.

[Manuel Rego, 2014/11/19 23:26:23]

Done.

+
+    OwnPtr<GridSpan> positions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), child, direction);
+    if (!positions)
+        return offset;

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

And explicitly return LayoutUnit(0) here (I like explicit return better as you
don't have to search the code for the definition).

[Manuel Rego, 2014/11/19 23:26:24]

Done.

+
+    if ((direction == ForColumns && !child.style()->gridColumnStart().isAuto()) || (direction == ForRows && !child.style()->gridRowStart().isAuto())) {

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

Unfortunately this doesn't handle correctly spanning column or named grid lines.
We can put a FIXME but we will have to think about that.

[Manuel Rego, 2014/11/19 23:26:24]

I'm not sure if I'm getting what you mean.

For example if you have:
grid-column: 1 / span 2;

Both gridColumnStart() and gridColumnEnd() won't be auto.

The same would happen for:
grid-column: A / span C;

In which case are you thinking of?

[Julien - ping for review, 2014/11/24 21:44:40]

This is the case I am thinking of:

grid-column: span 2 / span 2;

In this example, we end up resolving it as auto / auto but will not apply this
treatment here as we only look at the computed values.

The comment was also pointing out at:

grid-column: inexistent / span 2;
grid-column: 1 / span 2 nonexistent;

but they don't apply as there are rules to not resolve them as 'auto'.

[Manuel Rego, 2014/12/01 11:10:36]

I'm not sure about this one, so I've asked in www-style.
I mean, if we want to resolve this, we need to run
the auto-placement algorithm, to know the final resolved positions.
Yeah, in the case of inexistent named grid lines
my patch is not detecting it properly.

For example:
  grid-column: 3 / span C;

Where:
* gridColumnEnd().isAuto() would be false.
* And the resolved positions won't be null.

For the moment, I've added the FIXME.
Probably a solution could be to check if it's auto,
or check if the named grid line exists or not.

+        breadth -= (direction == ForColumns) ? paddingStart() : paddingBefore();
+
+        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())) {

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

Not sure why this is inside an 'else'. Nothing in the specification explains
that.

[Manuel Rego, 2014/11/19 23:26:23]

Let me explain with an example. Imagine a grid with:
  padding: 15px;
  grid-template-rows: 50px;
  grid-template-columns: 50px;
  position: relative;

We have one absolutely positioned items with:
  position: absolute;
  width: 100%;
  height: 100%;

So, we have different values for grid-column property (for example):
A) grid-row: 1 / 1; grid-column: 1 / 1;
  In this case we want the item to have a 50x50 size and be positioned at 15x15.
B) grid-row: 1 / auto; grid-column: 1 / auto;
  Size 65x65 and position 15x15.
C) grid-row: auto / 1; grid-column: auto / 1;
  Size 65x65 and position 0x0.
D) grid-row: auto / auto; grid-column: auto / auto;
  Size 80x80 and position 15x15.

This "else if" is checking that the start was auto but the end is not auto.
Which matches with case C), where we have to decrease the offset in order
to place the element properly.

If we remove the "else", it'll match also A). However in that case we don't
want to move the element.

However, now I've doubts regarding D), if painting the element in 15x15
is the expected behavior or not. I was doing it because if that matches
how display flex and block work for this case. But, maybe for grids we
should actually move it to 0x0 following the spec:
"the containing block will correspond to the padding edges of the grid
container"

In the 2nd example of this mail:
http://lists.w3.org/Archives/Public/www-style/2014Nov/0101.html
fantasai seems to agreed that the expected behavior would be using
the padding edge (in this case 0x0 and not 15x15) so different from
flex and block.

So probably I've to modify that, as my tests are considering what I
explained before (using 15x15).

What do you think?

[Manuel Rego, 2014/11/21 10:11:00]

I've removed the "else" as it was not needed at all.
This one was wrong, it should be:
  Size 65x65 and position 15x15. 

Yeah, exactly the same position than B). However, if you set "left: 0; top: 0"
to both B) and C), only C) will be moved to "0x0".
And it seems to make sense after checking the specs:
* left defines "how far a box's left margin edge is offset to the right of
the left edge of the the box's containing block".
* The "box's containing block" in this case would be the "padding edges of
the grid container".
* Then if left is auto, the spec says that in this situation:
"set 'left' to the static position".

So, while left is not set, we should keep it in the static position, which
includes the padding in C).
I'm uploading a new patch, removing the else, and updating the values
in the test.

+        offset -= (direction == ForColumns) ? paddingStart() : paddingBefore();
+    }
+
+    if ((direction == ForColumns && !child.style()->gridColumnEnd().isAuto()) || (direction == ForRows && !child.style()->gridRowEnd().isAuto())) {
+        breadth -= (direction == ForColumns) ? paddingEnd() : paddingAfter();
+
+        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));