[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..814e32e94824fa13a948e85fb998000dd8dc987a 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,9 +1568,34 @@ static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const Lay
     return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
 }
 
-bool LayoutGrid::allowedToStretchLogicalHeightForChild(const LayoutBox& child) const
+bool LayoutGrid::canShrinkToFitInRowAxisForChild(const LayoutBox& child) const
 {
-    return child.style()->logicalHeight().isAuto() && !child.style()->marginBeforeUsing(style()).isAuto() && !child.style()->marginAfterUsing(style()).isAuto();
+    return !hasAutoMinSizeInRowAxis(child) || child.minPreferredLogicalWidth() <= child.overrideContainingBlockContentLogicalWidth();
+}
+
+bool LayoutGrid::hasAutoMinSizeInRowAxis(const LayoutBox& child) const
+{
+    return isHorizontalWritingMode() ? child.style()->minWidth().isAuto() : child.style()->minHeight().isAuto();
+}

[svillar, 2015/07/28 15:14:08]

Let's not add a oneliner new function for a code that is called just from one
place.

[jfernandez, 2015/07/28 21:31:50]

Done.

+
+bool LayoutGrid::hasAutoSizeInColumnAxisForChild(const LayoutBox& child) const
+{
+    return isHorizontalWritingMode() ? child.style()->height().isAuto() : child.style()->width().isAuto();
+}

[svillar, 2015/07/28 15:14:08]

Ditto.

[jfernandez, 2015/07/28 21:31:50]

Done.

+
+bool LayoutGrid::hasAutoSizeInRowAxisForChild(const LayoutBox& child) const
+{
+    return isHorizontalWritingMode() ? child.style()->width().isAuto() : child.style()->height().isAuto();
+}
+
+bool LayoutGrid::allowedToStretchChildAlongColumnAxis(const LayoutBox& child) const
+{
+    return hasAutoSizeInColumnAxisForChild(child) && !child.style()->marginBeforeUsing(style()).isAuto() && !child.style()->marginAfterUsing(style()).isAuto();
+}

[svillar, 2015/07/28 15:14:08]

Ditto.

[jfernandez, 2015/07/28 21:31:50]

Done.

+
+bool LayoutGrid::allowedToStretchChildAlongRowAxis(const LayoutBox& child) const
+{
+    return hasAutoSizeInRowAxisForChild(child) && !child.style()->marginStartUsing(style()).isAuto() && !child.style()->marginEndUsing(style()).isAuto();
 }

[svillar, 2015/07/28 15:14:08]

Ditto.

[jfernandez, 2015/07/28 21:31:50]

Done.

 
 // FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
@@ -1638,27 +1663,39 @@ 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)
+{
+    child.clearOverrideSize();

[svillar, 2015/07/28 15:14:08]

I think we need a comment here explaining why we unconditionally clear the
override size here and why we unconditionally set the new ones bellow

[jfernandez, 2015/07/28 21:31:50]

Done.

+
+    if (!allowedToStretchChildAlongRowAxis(child) || ComputedStyle::resolveJustification(styleRef(), child.styleRef(), ItemPositionStretch) != ItemPositionStretch) {
+        // TODO(lajava): how to handle orthogonality in this case ?.
+        // TODO(lajava): grid track sizing and positioning do not support orthogonal modes yet.
+        if (hasAutoSizeInRowAxisForChild(child) && canShrinkToFitInRowAxisForChild(child)) {
+            LayoutUnit childWidthToFit = std::max(std::min(child.maxPreferredLogicalWidth(), child.overrideContainingBlockContentLogicalWidth()  - child.marginLogicalWidth()), child.minPreferredLogicalWidth());

[svillar, 2015/07/28 15:14:07]

Not a big fan of the variable name. Don't we have a better one from specs?

[jfernandez, 2015/07/28 21:31:50]

Well, this value is computed to fulfill the following sentence from the spec:
"..., grid items will auto-size to fit their contents." 

I'll change the variable name to "childWidthToFitContent", which may describe
better what the specs say, but I'm open to alternatives.

[svillar, 2015/07/29 08:18:05]

What about childFitContentWidth? Just to be coherent with the rest of XXXWidth
names

+            LayoutUnit desiredLogicalWidth = child.constrainLogicalHeightByMinMax(childWidthToFit, -1);
+            bool childNeedsRelayout = desiredLogicalWidth != child.logicalWidth();
+            child.setOverrideLogicalContentWidth(desiredLogicalWidth - child.borderAndPaddingLogicalWidth());
+            if (childNeedsRelayout) {
+                child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
+            }

[svillar, 2015/07/28 15:14:08]

Remove braces.

Also you don't need the childNeedsRelayout variable, just check the condition
here in the if.

[jfernandez, 2015/07/28 21:31:50]

Done.

+        }
     }
 
-    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);
-        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())
+    if (allowedToStretchChildAlongColumnAxis(child) && ComputedStyle::resolveAlignment(styleRef(), child.styleRef(), ItemPositionStretch) == ItemPositionStretch) {
+        bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
+        // FIXME: If the child has orthogonal flow, then it already has an override height set, so use it.

[svillar, 2015/07/28 15:14:08]

Use TODO()

[jfernandez, 2015/07/28 21:31:50]

Done.

+        // FIXME: grid track sizing and positioning do not support orthogonal modes yet.

[svillar, 2015/07/28 15:14:08]

Ditto.

[jfernandez, 2015/07/28 21:31:50]

Done.

+        if (!hasOrthogonalWritingMode) {

[svillar, 2015/07/28 15:34:14]

Now that I think about this perhaps we should just break on
(hasOrthogonalWritingMode) to get away from this block (kind of an early return)

[jfernandez, 2015/07/28 21:31:50]

I kind of agree; however, there is TODO specifically addressing the else branch,
so I though it was a good idea to keep that if/then/else structure. Anyway, I
think we can get it back when we try to figure out how to face the issue.

[svillar, 2015/07/29 08:18:05]

OK, let's keep it as you say then

+            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();
             child.setOverrideLogicalContentHeight(desiredLogicalHeight - child.borderAndPaddingLogicalHeight());
-        if (childNeedsRelayout) {
-            child.setLogicalHeight(0);
-            child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
+            if (childNeedsRelayout) {

[svillar, 2015/07/28 15:14:08]

Same comment about childNeedsRelayout.

[jfernandez, 2015/07/28 21:31:50]

Done.

+                child.setLogicalHeight(0);
+                child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
+            }
         }
     }
 }