[css-grid] Fix indefinite height detection

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 2680 matching lines @@
         EDisplay display = containingBlock->styleRef().display();
         return display == BLOCK || display == INLINE_BLOCK;
     }
 
     // For quirks mode, we skip most auto-height containing blocks when computing percentages.
     return document().inQuirksMode() && !containingBlock->isTableCell() && !containingBlock->isOutOfFlowPositioned() && !containingBlock->isLayoutGrid() && containingBlock->style()->logicalHeight().isAuto();
 }
 
 LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
 {
-    LayoutUnit availableHeight(-1);
-
-    bool skippedAutoHeightContainingBlock = false;
     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));
 
-    const ComputedStyle& cbstyle = cb->styleRef();
-
-    // A positioned element that specified both top/bottom or that specifies height should be treated as though it has a height
-    // 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);
-
+    LayoutUnit availableHeight;

[svillar, 2016/07/20 08:24:00]

My main doubt is, what's the criteria you have used to move the code to the new
function? I'm having a hard time trying to know that.

[Manuel Rego, 2016/07/21 09:46:01]

Basically the idea is having a function that I can call from the grid container,
to know if it has a definite or indefinite height.
So I tried to move everything I could to an independent funciton,
so calls to current object like isHorizontalWritingMode() or
hasOverrideContainingBlockLogicalHeight() couldn't be moved.
Also I wanted to avoid to pass weird bool params to that method
(like in previous version).

     if (isHorizontalWritingMode() != cb->isHorizontalWritingMode()) {
         availableHeight = containingBlockChild->containingBlockLogicalWidthForContent();
-    } else if (stretchedFlexHeight != LayoutUnit(-1)) {
-        availableHeight = stretchedFlexHeight;
-    } else if (hasOverrideContainingBlockLogicalHeight() && !isOutOfFlowPositionedWithSpecifiedHeight) {
+    } else if (isGridItem() && hasOverrideContainingBlockLogicalHeight()) {

[svillar, 2016/07/20 08:24:00]

Why just for grid items? What about !isOutOfFlowPositionedWithSpecifiedHeight ?

[Manuel Rego, 2016/07/21 09:46:01]

True the isGridItem() is not needed.

About "!isOutOfFlowPositionedWithSpecifiedHeight" that was wrong.
It was needed for some cases as we were using it to know if the height is
indefinite.
But it was causing bugs too: http://jsbin.com/hisova/1/edit?html,css,output

And the thing is that it's not possible in the current code to just remove
!isOutOfFlowPositionedWithSpecifiedHeight.
As it'll be making other good tests to fail.
The main issue is how bad we're detecting that the height is indefinite,
using the firstInFlowChildBox()->percentageLogicalHeightIsResolvable().

         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()) {
+    } else if (!cb->styleRef().logicalHeight().isFixed() && cb->isTableCell()) {
         if (!skippedAutoHeightContainingBlock) {
             // Table cells violate what the CSS spec says to do with heights. Basically we
             // don't care if the cell specified a height or not. We just always make ourselves
             // be a percentage of the cell's current content height.
             if (!cb->hasOverrideLogicalContentHeight()) {
                 // Normally we would let the cell size intrinsically, but scrolling overflow has to be
                 // treated differently, since WinIE lets scrolled overflow regions shrink as needed.
                 // While we can't get all cases right, we can at least detect when the cell has a specified
                 // height or when the table has a specified height. In these cases we want to initially have
                 // 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()))
                     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.
-        LayoutUnit heightWithScrollbar = cb->computePercentageLogicalHeight(cbstyle.logicalHeight());
-        if (heightWithScrollbar != -1) {
-            LayoutUnit contentBoxHeightWithScrollbar = cb->adjustContentBoxLogicalHeightForBoxSizing(heightWithScrollbar);
-            // We need to adjust for min/max height because this method does not
-            // handle the min/max of the current block, its caller does. So the
-            // return value from the recursive call will not have been adjusted
-            // yet.
-            LayoutUnit contentBoxHeight = cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeightWithScrollbar - cb->scrollbarLogicalHeight(), LayoutUnit(-1));
-            availableHeight = std::max(LayoutUnit(), contentBoxHeight);
-        }
-    } else if (isOutOfFlowPositionedWithSpecifiedHeight) {
-        // Don't allow this to affect the block' size() member variable, since this
-        // can get called while the block is still laying out its kids.
-        LogicalExtentComputedValues computedValues;
-        cb->computeLogicalHeight(cb->logicalHeight(), LayoutUnit(), computedValues);
-        availableHeight = computedValues.m_extent - cb->borderAndPaddingLogicalHeight() - cb->scrollbarLogicalHeight();
-    } else if (cb->isLayoutView()) {
-        availableHeight = view()->viewLogicalHeightForPercentages();
+    } else {
+        availableHeight = LayoutBlock::availableLogicalHeightForPercentageComputation(*cb);
     }
 
     if (availableHeight == -1)
         return availableHeight;
 
     availableHeight -= rootMarginBorderPaddingHeight;
 
     if (isTable() && isOutOfFlowPositioned())
         availableHeight += cb->paddingLogicalHeight();
 
     LayoutUnit result = valueForLength(height, availableHeight);
+    bool includeBorderPadding = isTable() || (cb->isTableCell() && (cb->styleRef().logicalHeight().isFixed() || (!skippedAutoHeightContainingBlock && cb->hasOverrideLogicalContentHeight())));

[svillar, 2016/07/20 08:24:00]

I think this is not correct, for example if cb->isTableCell() and
cb->styleRef().logicalHeight().isFixed() are both true we set this to true, but
in the previous code that was only done whenever 
!skippedAutoHeightContainingBlock.

Apart from that such a complex condition in just 1 line seems a bit too much to
my taste :).

[Manuel Rego, 2016/07/21 09:46:01]

Are you sure? In the previous code we've:
  else if (cbstyle.logicalHeight().isFixed()) {
    ...
    if (cb->isTableCell())
      includeBorderPadding = true;
  }

So that was enough to make it true.
Yeah, I can split it in several lines.

     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.
         result -= borderAndPaddingLogicalHeight();
         return std::max(LayoutUnit(), result);
     }
     return result;
 }
 
 LayoutUnit LayoutBox::computeReplacedLogicalWidth(ShouldComputePreferred shouldComputePreferred) const
@@ skipping 90 matching lines @@
     ASSERT(sizeType == MinSize || sizeType == MainOrPreferredSize || !logicalHeight.isAuto());
     if (sizeType == MinSize && logicalHeight.isAuto())
         return adjustContentBoxLogicalHeightForBoxSizing(LayoutUnit());
 
     switch (logicalHeight.type()) {
     case Fixed:
         return adjustContentBoxLogicalHeightForBoxSizing(logicalHeight.value());
     case Percent:
     case Calculated:
     {
+        // TODO(rego): Check if we can somehow reuse LayoutBox::computePercentageLogicalHeight() and/or LayoutBlock::availableLogicalHeightForPercentageComputation().
         LayoutObject* cb = isOutOfFlowPositioned() ? container() : containingBlock();
         while (cb->isAnonymous())
             cb = cb->containingBlock();
         LayoutUnit stretchedHeight(-1);
         if (cb->isLayoutBlock()) {
             LayoutBlock* block = toLayoutBlock(cb);
             block->addPercentHeightDescendant(const_cast<LayoutBox*>(this));
             if (block->isFlexItem())
                 stretchedHeight = toLayoutFlexibleBox(block->parent())->childLogicalHeightForPercentageResolution(*block);
             else if (block->isGridItem() && block->hasOverrideLogicalContentHeight())
@@ skipping 2012 matching lines @@
         m_rareData->m_snapAreas->remove(&snapArea);
     }
 }
 
 SnapAreaSet* LayoutBox::snapAreas() const
 {
     return m_rareData ? m_rareData->m_snapAreas.get() : nullptr;
 }
 
 } // namespace blink