[CSS Grid Layout] Do not stretch always grid items with auto width

Source/core/layout/LayoutGrid.cpp

Index: Source/core/layout/LayoutGrid.cpp
diff --git a/Source/core/layout/LayoutGrid.cpp b/Source/core/layout/LayoutGrid.cpp
index 794e1175df1f9cd644a7e0c04f952ca1ff21d6fa..147c79b5e7549037f02f1d15e2cad16d50a9d3a5 100644
--- a/Source/core/layout/LayoutGrid.cpp
+++ b/Source/core/layout/LayoutGrid.cpp
@@ -1368,7 +1368,7 @@ void LayoutGrid::layoutGridItems()
         // Stretching logic might force a child layout, so we need to run it before the layoutIfNeeded
         // call to avoid unnecessary relayouts. This might imply that child margins, needed to correctly
         // determine the available space before stretching, are not set yet.
-        applyStretchAlignmentToChildIfNeeded(*child, overrideContainingBlockContentLogicalHeight);
+        applyStretchAlignmentToChildIfNeeded(*child);
 
         child->layoutIfNeeded();
 
@@ -1568,11 +1568,6 @@ static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const Lay
     return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
 }
 
-bool LayoutGrid::allowedToStretchLogicalHeightForChild(const LayoutBox& child) const
-{
-    return child.style()->logicalHeight().isAuto() && !child.style()->marginBeforeUsing(style()).isAuto() && !child.style()->marginAfterUsing(style()).isAuto();
-}
-
 // FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
 bool LayoutGrid::needToStretchChildLogicalHeight(const LayoutBox& child) const
 {
@@ -1638,25 +1633,42 @@ LayoutUnit LayoutGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUni
 }
 
 // FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
-void LayoutGrid::applyStretchAlignmentToChildIfNeeded(LayoutBox& child, LayoutUnit gridAreaBreadthForChild)
-{
-    if (!allowedToStretchLogicalHeightForChild(child) || ComputedStyle::resolveAlignment(styleRef(), child.styleRef(), ItemPositionStretch) != ItemPositionStretch) {
-        child.clearOverrideLogicalContentHeight();
-        return;
+void LayoutGrid::applyStretchAlignmentToChildIfNeeded(LayoutBox& child)
+{
+    // We clear botth width and height override values because we will decide now whether they

[svillar, 2015/07/30 10:11:27]

Nit botth -> both

[jfernandez, 2015/07/31 22:25:42]

Done.

+    // are allowed or not, evaluating the conditions which might have changed since the old
+    // values were set.
+    child.clearOverrideSize();
+
+    bool isHorizontalMode = isHorizontalWritingMode();
+    bool hasAutoSizeInRowAxis = isHorizontalMode ? child.style()->width().isAuto() : child.style()->height().isAuto();

[svillar, 2015/07/30 10:11:27]

Nit: hasAutoSizeInRowAxisForChild to match your function name

+    bool allowedToStretchChildAlongRowAxis = hasAutoSizeInRowAxis && !child.style()->marginStartUsing(style()).isAuto() && !child.style()->marginEndUsing(style()).isAuto();
+    if (!allowedToStretchChildAlongRowAxis || ComputedStyle::resolveJustification(styleRef(), child.styleRef(), ItemPositionStretch) != ItemPositionStretch) {
+        bool hasAutoMinSizeInRowAxis = isHorizontalMode ? child.style()->minWidth().isAuto() : child.style()->minHeight().isAuto();
+        bool canShrinkToFitInRowAxisForChild = !hasAutoMinSizeInRowAxis || child.minPreferredLogicalWidth() <= child.overrideContainingBlockContentLogicalWidth();

[svillar, 2015/07/30 10:11:27]

As a general comment, perhaps we should use styleRef() to deal with references
instead of pointers. Actually we use it so many times that perhaps we should
consider

auto childStyle = child.styleRef()

and use it all along the method

[jfernandez, 2015/07/31 22:25:42]

Good idea.

+        // TODO(lajava): how to handle orthogonality in this case ?.
+        // TODO(lajava): grid track sizing and positioning do not support orthogonal modes yet.
+        if (hasAutoSizeInRowAxis && canShrinkToFitInRowAxisForChild) {
+            LayoutUnit childWidthToFitContent = std::max(std::min(child.maxPreferredLogicalWidth(), child.overrideContainingBlockContentLogicalWidth()  - child.marginLogicalWidth()), child.minPreferredLogicalWidth());
+            LayoutUnit desiredLogicalWidth = child.constrainLogicalHeightByMinMax(childWidthToFitContent, -1);
+            child.setOverrideLogicalContentWidth(desiredLogicalWidth - child.borderAndPaddingLogicalWidth());
+            if (desiredLogicalWidth != child.logicalWidth())
+                child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
+        }
     }
 
-    bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
-    // FIXME: If the child has orthogonal flow, then it already has an override height set, so use it.
-    // FIXME: grid track sizing and positioning do not support orthogonal modes yet.
-    if (!hasOrthogonalWritingMode) {
-        LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(gridAreaBreadthForChild, child);
+    bool hasAutoSizeInColumnAxis = isHorizontalMode ? child.style()->height().isAuto() : child.style()->width().isAuto();
+    bool allowedToStretchChildAlongColumnAxis = hasAutoSizeInColumnAxis && !child.style()->marginBeforeUsing(style()).isAuto() && !child.style()->marginAfterUsing(style()).isAuto();
+    if (allowedToStretchChildAlongColumnAxis && ComputedStyle::resolveAlignment(styleRef(), child.styleRef(), ItemPositionStretch) == ItemPositionStretch) {
+        // TODO (lajava): If the child has orthogonal flow, then it already has an override height set, so use it.
+        // TODO (lajava): grid track sizing and positioning do not support orthogonal modes yet.
+        if (child.isHorizontalWritingMode() != isHorizontalMode)
+            return;

[svillar, 2015/07/30 10:11:27]

Sorry for the back and forth but I agree that perhaps it's better to keep it as
it was before without the early return which indeed looks weird here.

[jfernandez, 2015/07/31 22:25:42]

Acknowledged.

+        LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(child.overrideContainingBlockContentLogicalHeight(), child);
         LayoutUnit desiredLogicalHeight = child.constrainLogicalHeightByMinMax(stretchedLogicalHeight, -1);
-
-        // FIXME: Can avoid laying out here in some cases. See https://webkit.org/b/87905.
-        bool childNeedsRelayout = desiredLogicalHeight != child.logicalHeight();
-        if (childNeedsRelayout || !child.hasOverrideLogicalContentHeight())
-            child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
-        if (childNeedsRelayout) {
+        child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
+        if (desiredLogicalHeight != child.logicalHeight()) {
+            // TODO (lajava): Can avoid laying out here in some cases. See https://webkit.org/b/87905.
             child.setLogicalHeight(0);
             child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
         }