[css-grid] Fix behavior of positioned items without specific dimensions

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 2067dba4ca5bb552c93c4b402f9aaf3a2f135154..e130e94abb322297321a92b137998dde9598856b 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -2601,8 +2601,8 @@ void LayoutGrid::prepareChildForPositionedLayout(LayoutBox& child) {
   child.containingBlock()->insertPositionedObject(&child);
 
   PaintLayer* childLayer = child.layer();
-  childLayer->setStaticInlinePosition(borderAndPaddingStart());
-  childLayer->setStaticBlockPosition(borderAndPaddingBefore());
+  childLayer->setStaticInlinePosition(LayoutUnit(borderStart()));
+  childLayer->setStaticBlockPosition(LayoutUnit(borderBefore()));

[svillar, 2017/01/31 15:28:40]

Mind explaining this change?

[Manuel Rego, 2017/02/01 09:08:20]

This was a mistake, we shouldn't include padding here per spec [1]:
"The static position [CSS21] of an absolutely-positioned child
 of a grid container is determined as if it were the sole grid item
 in a grid area whose edges coincide with the padding edges
 of the grid container."

[1] https://drafts.csswg.org/css-grid/

[svillar, 2017/02/01 09:49:07]

I guess you're adding a test for this change. In any case please mention it in
the bug description.

[Manuel Rego, 2017/02/01 11:10:34]

This is the part that I've moved to a different patch:
https://codereview.chromium.org/2665983004/

Sorry for the noise. :-(

 }
 
 void LayoutGrid::layoutPositionedObjects(bool relayoutChildren,
@@ -2628,16 +2628,29 @@ void LayoutGrid::layoutPositionedObjects(bool relayoutChildren,
 
     child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
     child->setOverrideContainingBlockContentLogicalHeight(rowBreadth);
-    child->setExtraInlineOffset(columnOffset);
-    child->setExtraBlockOffset(rowOffset);
 
-    if (child->parent() == this) {
-      PaintLayer* childLayer = child->layer();
-      childLayer->setStaticInlinePosition(borderStart() + columnOffset);
-      childLayer->setStaticBlockPosition(borderBefore() + rowOffset);
-    }
+    bool hasStaticInlinePosition = child->styleRef().hasStaticInlinePosition(
+        child->isHorizontalWritingMode());
+    bool hasStaticBlockPosition = child->styleRef().hasStaticBlockPosition(
+        child->isHorizontalWritingMode());
+    bool childNeedsLayout = child->needsLayout();
+
+    if (hasStaticInlinePosition || childNeedsLayout)
+      child->setX(LayoutUnit());
+    if (hasStaticBlockPosition || childNeedsLayout)
+      child->setY(LayoutUnit());
 
     layoutPositionedObject(child, relayoutChildren, info);
+
+    LayoutUnit x = child->logicalLeft();
+    LayoutUnit y = child->logicalTop();
+
+    if (hasStaticInlinePosition || childNeedsLayout)
+      x += columnOffset;
+    if (hasStaticBlockPosition || childNeedsLayout)
+      y += rowOffset;
+
+    child->setLogicalLocation(LayoutPoint(x, y));

[svillar, 2017/01/31 15:28:40]

I don't get this part too...

[Manuel Rego, 2017/02/01 09:08:20]

I agree this part is tricky but I didn't find a better way to do it.

I'm relying on layoutPositionedObject() to resolve things like
"left: 10px; right: 50px;", so I don't need to do all that computations here.
Then we use the position calculated there to place manually the item.

As you know layoutPositionedObjects() can be called twice,
and in some cases in the first call child->needsLayout() is true,
but 2nd time it's false. That combined with the fact that a grid item
can have a static position (offsets are auto), required this kind of conditions
to get the right values.

Note that the logic when you have a static position or not in
layoutPositionedObject() is quite different.

What's more important about being able to use setLogicalLocation()
is that it allows us to support alignment for positioned items,
which was not possible with the previous approach.

[svillar, 2017/02/01 09:49:07]

Having conditions like (hasStaticBlockPosition || childNeedsLayout) makes the
code extremely mysterious as they seem so unrelated that I feel totally unable
to review it now or to eventually change it in the future. We should find a
clear way to do this.

   }
 }
 
@@ -2723,11 +2736,8 @@ void LayoutGrid::offsetAndBreadthForPositionedChild(
   breadth = std::max(end - start, LayoutUnit());
   offset = start;
 
-  if (isForColumns && !styleRef().isLeftToRightDirection() &&
-      !child.styleRef().hasStaticInlinePosition(
-          child.isHorizontalWritingMode())) {
-    // If the child doesn't have a static inline position (i.e. "left" and/or
-    // "right" aren't "auto", we need to calculate the offset from the left
+  if (isForColumns && !styleRef().isLeftToRightDirection()) {
+    // We always want to calculate the static position from the left
     // (even if we're in RTL).
     if (endIsAuto) {
       offset = LayoutUnit();