Blink doesn't honor percent heights on children of "align-self:stretch" flex items in a fixed-height

Source/core/rendering/RenderBox.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 2255 matching lines @@
     LayoutUnit logicalWidthResult = fillAvailableMeasure(availableLogicalWidth, marginStart, marginEnd);
 
     if (shrinkToAvoidFloats() && cb->isRenderBlockFlow() && toRenderBlockFlow(cb)->containsFloats())
         logicalWidthResult = std::min(logicalWidthResult, shrinkLogicalWidthToAvoidFloats(marginStart, marginEnd, toRenderBlockFlow(cb)));
 
     if (widthType == MainOrPreferredSize && sizesLogicalWidthToFitContent(logicalWidth))
         return std::max(minPreferredLogicalWidth(), std::min(maxPreferredLogicalWidth(), logicalWidthResult));
     return logicalWidthResult;
 }
 
-static bool columnFlexItemHasStretchAlignment(const RenderObject* flexitem)
+static bool flexItemHasStretchAlignment(const RenderObject* flexitem)
 {
     RenderObject* parent = flexitem->parent();
-    // auto margins mean we don't stretch. Note that this function will only be used for
-    // widths, so we don't have to check marginBefore/marginAfter.
-    ASSERT(parent->style()->isColumnFlexDirection());
-    if (flexitem->style()->marginStart().isAuto() || flexitem->style()->marginEnd().isAuto())
-        return false;
+    // auto margins mean we don't stretch.
+    if (parent->style()->isColumnFlexDirection()) {
+        if (flexitem->style()->marginStart().isAuto() || flexitem->style()->marginEnd().isAuto())
+            return false;
+    } else {
+        if (flexitem->style()->marginBefore().isAuto() || flexitem->style()->marginAfter().isAuto())

[ojan, 2014/08/28 03:23:09]

I think you just want this. You don't need the isColumnFlexDirection block at
all. Both code paths can call this. Calling marginStart/End was just a premature
performance optimization I think.

[harpreet.sk, 2014/08/28 14:30:19]

I tried removing it but because of it following layout test is getting failed
"css3/flexbox/columns-center-with-margins.html"

In above LayoutTest the flex-item of colmun flexbox having auto margins is not
getting centered.

I think it's not correct to remove isColumnFlexDirection block .

+            return false;
+    }
     return flexitem->style()->alignSelf() == ItemPositionStretch || (flexitem->style()->alignSelf() == ItemPositionAuto && parent->style()->alignItems() == ItemPositionStretch);
 }
 
 static bool isStretchingColumnFlexItem(const RenderObject* flexitem)
 {
     RenderObject* parent = flexitem->parent();
     if (parent->isDeprecatedFlexibleBox() && parent->style()->boxOrient() == VERTICAL && parent->style()->boxAlign() == BSTRETCH)
         return true;
 
     // We don't stretch multiline flexboxes because they need to apply line spacing (align-content) first.
-    if (parent->isFlexibleBox() && parent->style()->flexWrap() == FlexNoWrap && parent->style()->isColumnFlexDirection() && columnFlexItemHasStretchAlignment(flexitem))
+    if (parent->isFlexibleBox() && parent->style()->flexWrap() == FlexNoWrap && parent->style()->isColumnFlexDirection() && flexItemHasStretchAlignment(flexitem))
         return true;
     return false;
 }
 
 bool RenderBox::sizesLogicalWidthToFitContent(const Length& logicalWidth) const
 {
     // Marquees in WinIE are like a mixture of blocks and inline-blocks.  They size as though they're blocks,
     // but they allow text to sit on the same line as the marquee.
     if (isFloating() || (isInlineBlockOrInlineTable() && !isMarquee()))
         return true;
@@ skipping 12 matching lines @@
             return true;
     }
 
     // Flexible box items should shrink wrap, so we lay them out at their intrinsic widths.
     // In the case of columns that have a stretch alignment, we go ahead and layout at the
     // stretched size to avoid an extra layout when applying alignment.
     if (parent()->isFlexibleBox()) {
         // For multiline columns, we need to apply align-content first, so we can't stretch now.
         if (!parent()->style()->isColumnFlexDirection() || parent()->style()->flexWrap() != FlexNoWrap)
             return true;
-        if (!columnFlexItemHasStretchAlignment(this))
+        if (!flexItemHasStretchAlignment(this))
             return true;
     }
 
     // Flexible horizontal boxes lay out children at their intrinsic widths.  Also vertical boxes
     // that don't stretch their kids lay out their children at their intrinsic widths.
     // FIXME: Think about block-flow here.
     // https://bugs.webkit.org/show_bug.cgi?id=46473
     if (parent()->isDeprecatedFlexibleBox() && (parent()->style()->boxOrient() == HORIZONTAL || parent()->style()->boxAlign() != BSTRETCH))
         return true;
 
@@ skipping 283 matching lines @@
     cb->addPercentHeightDescendant(const_cast<RenderBox*>(this));
 
     RenderStyle* cbstyle = cb->style();
 
     // 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();
 
-    if (isHorizontalWritingMode() != cb->isHorizontalWritingMode())
+    if (isHorizontalWritingMode() != cb->isHorizontalWritingMode()) {
         availableHeight = containingBlockChild->containingBlockLogicalWidthForContent();
-    else if (hasOverrideContainingBlockLogicalHeight())
+    } else if (hasOverrideContainingBlockLogicalHeight()) {
         availableHeight = overrideContainingBlockContentLogicalHeight();
-    else if (cb->isTableCell()) {
+    } else if (cb->isFlexItem() && flexItemHasStretchAlignment(cb) && cb->hasOverrideHeight()) {
+        availableHeight = cb->overrideLogicalContentHeight();
+    } else if (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->hasOverrideHeight()) {
                 // 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
@@ skipping 2126 matching lines @@
 RenderBox::BoxDecorationData::BoxDecorationData(const RenderStyle& style)
 {
     backgroundColor = style.visitedDependentColor(CSSPropertyBackgroundColor);
     hasBackground = backgroundColor.alpha() || style.hasBackgroundImage();
     ASSERT(hasBackground == style.hasBackground());
     hasBorder = style.hasBorder();
     hasAppearance = style.hasAppearance();
 }
 
 } // namespace blink