[css-grid] Fix indefinite height detection

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

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index bd3dd55b2e8ccb76b136b641254925cee03264ec..f2d83420e8653059e267893e595dc18eaa37967c 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -2696,22 +2696,9 @@ bool LayoutBox::skipContainingBlockForPercentHeightCalculation(const LayoutBox*
     return document().inQuirksMode() && !containingBlock->isTableCell() && !containingBlock->isOutOfFlowPositioned() && !containingBlock->isLayoutGrid() && containingBlock->style()->logicalHeight().isAuto();
 }
 
-LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
+LayoutUnit LayoutBox::availableLogicalHeightForPercentageComputation(const LayoutBlock* cb, bool skippedAutoHeightContainingBlock, bool scrollsOverflowY)

[jfernandez, 2016/07/18 06:13:24]

I find it weird how we use the skippedAutoHeightContainingBlock and
scrollsOverflowY arguments. I know these come from the previous
computedPercentageLogicalHeight function that was refactored in this patch, but
I'd prefer to avoid this way of passing info between functions. I think the
arguments may become meaningless when this new function is called in a different
context.

[svillar, 2016/07/18 08:43:30]

Agree with jfernandez, this is related to the fact of using bools here as well.

[Manuel Rego, 2016/07/19 13:12:32]

Yeah, I've redo the patch and have removed the bool arguments.

 {
-    LayoutUnit availableHeight(-1);
-
-    bool skippedAutoHeightContainingBlock = false;
-    LayoutBlock* cb = containingBlock();
-    const LayoutBox* containingBlockChild = this;
-    LayoutUnit rootMarginBorderPaddingHeight;
-    while (!cb->isLayoutView() && skipContainingBlockForPercentHeightCalculation(cb)) {
-        if (cb->isBody() || cb->isDocumentElement())
-            rootMarginBorderPaddingHeight += cb->marginBefore() + cb->marginAfter() + cb->borderAndPaddingLogicalHeight();
-        skippedAutoHeightContainingBlock = true;
-        containingBlockChild = cb;
-        cb = cb->containingBlock();
-    }
-    cb->addPercentHeightDescendant(const_cast<LayoutBox*>(this));
+    LayoutUnit availableHeight = LayoutUnit(-1);
 
     const ComputedStyle& cbstyle = cb->styleRef();
 
@@ -2719,26 +2706,18 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
     // explicitly specified that can be used for any percentage computations.
     bool isOutOfFlowPositionedWithSpecifiedHeight = cb->isOutOfFlowPositioned() && (!cbstyle.logicalHeight().isAuto() || (!cbstyle.logicalTop().isAuto() && !cbstyle.logicalBottom().isAuto()));
 
-    bool includeBorderPadding = isTable();
-
     LayoutUnit stretchedFlexHeight(-1);
     if (cb->isFlexItem())
         stretchedFlexHeight = toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
 
-    if (isHorizontalWritingMode() != cb->isHorizontalWritingMode()) {
-        availableHeight = containingBlockChild->containingBlockLogicalWidthForContent();
-    } else if (stretchedFlexHeight != LayoutUnit(-1)) {
+    if (stretchedFlexHeight != LayoutUnit(-1)) {
         availableHeight = stretchedFlexHeight;
-    } else if (hasOverrideContainingBlockLogicalHeight() && !isOutOfFlowPositionedWithSpecifiedHeight) {
-        availableHeight = overrideContainingBlockContentLogicalHeight();
     } else if (cb->isGridItem() && cb->hasOverrideLogicalContentHeight()) {
         availableHeight = cb->overrideLogicalContentHeight();
     } else if (cbstyle.logicalHeight().isFixed()) {
         LayoutUnit contentBoxHeight = cb->adjustContentBoxLogicalHeightForBoxSizing(cbstyle.logicalHeight().value());
         availableHeight = cb->constrainContentBoxLogicalHeightByMinMax(
             contentBoxHeight - cb->scrollbarLogicalHeight(), LayoutUnit(-1)).clampNegativeToZero();
-        if (cb->isTableCell())
-            includeBorderPadding = true;
     } else if (cb->isTableCell()) {
         if (!skippedAutoHeightContainingBlock) {
             // Table cells violate what the CSS spec says to do with heights. Basically we
@@ -2752,13 +2731,12 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
                 // no size and allow the flexing of the table or the cell to its specified height to cause us
                 // to grow to fill the space. This could end up being wrong in some cases, but it is
                 // preferable to the alternative (sizing intrinsically and making the row end up too big).
-                LayoutTableCell* cell = toLayoutTableCell(cb);
-                if (scrollsOverflowY() && (!cell->style()->logicalHeight().isAuto() || !cell->table()->style()->logicalHeight().isAuto()))
+                const LayoutTableCell* cell = toLayoutTableCell(cb);

[jfernandez, 2016/07/18 06:13:24]

Why do you need the cell to be const here ? 

[Manuel Rego, 2016/07/19 13:12:32]

This was not needed, just slipped into the patch.

+                if (scrollsOverflowY && (!cell->style()->logicalHeight().isAuto() || !cell->table()->style()->logicalHeight().isAuto()))
                     return LayoutUnit();
                 return LayoutUnit(-1);
             }
             availableHeight = cb->overrideLogicalContentHeight();
-            includeBorderPadding = true;
         }
     } else if (cbstyle.logicalHeight().hasPercent() && !isOutOfFlowPositionedWithSpecifiedHeight) {
         // We need to recur and compute the percentage height for our containing block.
@@ -2779,8 +2757,37 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
         cb->computeLogicalHeight(cb->logicalHeight(), LayoutUnit(), computedValues);
         availableHeight = computedValues.m_extent - cb->borderAndPaddingLogicalHeight() - cb->scrollbarLogicalHeight();
     } else if (cb->isLayoutView()) {
-        availableHeight = view()->viewLogicalHeightForPercentages();
+        availableHeight = toLayoutView(cb)->viewLogicalHeightForPercentages();

[svillar, 2016/07/18 08:43:30]

why this?

[Manuel Rego, 2016/07/19 13:12:32]

This was because it's a static method so I couldn't call view().

+    }
+
+    if (availableHeight == -1)
+        return availableHeight;
+
+    return availableHeight;
+}
+
+LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
+{
+    LayoutBlock* cb = containingBlock();
+    const LayoutBox* containingBlockChild = this;
+    bool skippedAutoHeightContainingBlock = false;
+    LayoutUnit rootMarginBorderPaddingHeight;
+    while (!cb->isLayoutView() && skipContainingBlockForPercentHeightCalculation(cb)) {
+        if (cb->isBody() || cb->isDocumentElement())
+            rootMarginBorderPaddingHeight += cb->marginBefore() + cb->marginAfter() + cb->borderAndPaddingLogicalHeight();
+        skippedAutoHeightContainingBlock = true;
+        containingBlockChild = cb;
+        cb = cb->containingBlock();
     }
+    cb->addPercentHeightDescendant(const_cast<LayoutBox*>(this));
+
+    LayoutUnit availableHeight;
+    if (isHorizontalWritingMode() != cb->isHorizontalWritingMode())
+        availableHeight = containingBlockChild->containingBlockLogicalWidthForContent();
+    else if (isGridItem() && hasOverrideContainingBlockLogicalHeight())
+        availableHeight = overrideContainingBlockContentLogicalHeight();

[jfernandez, 2016/07/18 06:13:24]

I guess this is new logic. I'd rather do it in a different patch, once the
refactoring has landed and has been proven there are not regressions.

[svillar, 2016/07/18 08:43:30]

I think that's a good idea. First the refactoring, then the fix.

[Manuel Rego, 2016/07/19 13:12:32]

This is not exactly new logic. There were some similar logic already.

The thing is that current code was wrong in some situations.
E.g. if a grid is a streched grid item, it's height should be considered
definite, however current code fails detecting that.
See: http://jsbin.com/dusoxe/edit?html,css,output

The problem with doing the refactoring first, is that I'd need to do
some weird and wrong stuff, to keep the previous bug and I believe
it's not worth it.

Also the other piece of new logic in this patch is using the new
hasDefiniteLogicalHeight() which is fixing more tests.

+    else
+        availableHeight = availableLogicalHeightForPercentageComputation(cb, skippedAutoHeightContainingBlock, scrollsOverflowY());
 
     if (availableHeight == -1)
         return availableHeight;
@@ -2791,6 +2798,7 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
         availableHeight += cb->paddingLogicalHeight();
 
     LayoutUnit result = valueForLength(height, availableHeight);
+    bool includeBorderPadding = isTable() || (cb->isTableCell() && (cb->styleRef().logicalHeight().isFixed() || (!skippedAutoHeightContainingBlock && cb->hasOverrideLogicalContentHeight())));
     if (includeBorderPadding) {
         // TODO(rhogan) crbug.com/467378: Doing this for content inside tables cells is wrong, it should fill
         // whatever height the cell makes available.