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

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

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 2583 matching lines @@
     if (!gridAreaRect.contains(child->frameRect()))
       m_gridItemsOverflowingGridArea.push_back(child);
   }
 }
 
 void LayoutGrid::prepareChildForPositionedLayout(LayoutBox& child) {
   ASSERT(child.isOutOfFlowPositioned());
   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,
                                          PositionedLayoutBehavior info) {
   TrackedLayoutBoxListHashSet* positionedDescendants = positionedObjects();
   if (!positionedDescendants)
     return;
 
   for (auto* child : *positionedDescendants) {
     if (isOrthogonalChild(*child)) {
       // FIXME: Properly support orthogonal writing mode.
       layoutPositionedObject(child, relayoutChildren, info);
       continue;
     }
 
     LayoutUnit columnOffset = LayoutUnit();
     LayoutUnit columnBreadth = LayoutUnit();
     offsetAndBreadthForPositionedChild(*child, ForColumns, columnOffset,
                                        columnBreadth);
     LayoutUnit rowOffset = LayoutUnit();
     LayoutUnit rowBreadth = LayoutUnit();
     offsetAndBreadthForPositionedChild(*child, ForRows, rowOffset, rowBreadth);
 
     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.

   }
 }
 
 void LayoutGrid::offsetAndBreadthForPositionedChild(
     const LayoutBox& child,
     GridTrackSizingDirection direction,
     LayoutUnit& offset,
     LayoutUnit& breadth) {
   ASSERT(!isOrthogonalChild(child));
   bool isForColumns = direction == ForColumns;
@@ skipping 65 matching lines @@
     if (endLine > 0 && endLine < lastLine) {
       DCHECK(!m_grid.needsItemsPlacement());
       end -= guttersSize(m_grid, direction, endLine - 1, 2, TrackSizing);
       end -= isForColumns ? m_offsetBetweenColumns : m_offsetBetweenRows;
     }
   }
 
   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();
     } else {
       offset = translateRTLCoordinate(m_columnPositions[endLine]) -
                borderLogicalLeft();
 
       if (endLine > 0 && endLine < lastLine) {
         DCHECK(!m_grid.needsItemsPlacement());
         offset += guttersSize(m_grid, direction, endLine - 1, 2, TrackSizing);
@@ skipping 887 matching lines @@
   if (direction == ForRows)
     return grid.numTracks(ForRows);
 
   return grid.numTracks(ForRows)
              ? grid.numTracks(ForColumns)
              : GridPositionsResolver::explicitGridColumnCount(
                    styleRef(), grid.autoRepeatTracks(ForColumns));
 }
 
 }  // namespace blink