[css-flexbox] Use correct aspect ratio for min-size: auto

https://codereview.chromium.org/1421423005 caused this regression. We never handled aspect ratio correctly in flexbox (that's bug 249112), but that patch made our handling worse because we now distort and enlarge images when a cross axis size is specified that's bigger than the image's intrinsic size. This was tested by imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht which we now pass. I didn't realize the significance of failing that test at the time :( BUG=581361,581535 R=leviw@chromium.org Review URL: https://codereview.chromium.org/1639723003 Cr-Commit-Position: refs/heads/master@{#372000}
crosswalk-project · Jan 28, 2016 · eba8fa2 · eba8fa2
1 parent 0b271f5
commit eba8fa2
Show file tree

Hide file tree

Showing 4 changed files with 168 additions and 7 deletions.
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
@@ -971,13 +971,17 @@ crbug.com/471824 imported/web-platform-tests/pointerevents/ [ Skip ]
 crbug.com/471824 virtual/pointerevent/imported/web-platform-tests/pointerevents/pointerevent_touch-action-illegal.html [ Skip ]
 crbug.com/471824 virtual/pointerevent/imported/web-platform-tests/pointerevents/pointerevent_touch-action-verification.html [ Skip ]
 
-# Not implemented yet
-crbug.com/336604 imported/csswg-test/css-flexbox-1/flexbox_visibility-collapse-line-wrapping.html [ Failure ]
-crbug.com/336604 imported/csswg-test/css-flexbox-1/flexbox_visibility-collapse.html [ Failure ]
+# These testcases are incorrect, mark them as failing until they're fixed in the testsuite.
+# https://lists.w3.org/Archives/Public/www-style/2016Jan/0275.html
+# https://lists.w3.org/Archives/Public/www-style/2016Jan/0276.html
 crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-005.xht [ Failure ]
 crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-007.xht [ Failure ]
-crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-height-flex-items-008.xht [ Failure ]
 crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-width-flex-items-005.xht [ Failure ]
+crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-width-flex-items-007.xht [ Failure ]
+
+# Not implemented yet
+crbug.com/336604 imported/csswg-test/css-flexbox-1/flexbox_visibility-collapse-line-wrapping.html [ Failure ]
+crbug.com/336604 imported/csswg-test/css-flexbox-1/flexbox_visibility-collapse.html [ Failure ]
 crbug.com/249112 imported/csswg-test/css-flexbox-1/flex-minimum-width-flex-items-006.xht [ Failure ]
 
 crbug.com/467127 imported/csswg-test/css-flexbox-1/flex-grow-006.html [ Failure ]
@@ -987,7 +991,6 @@ crbug.com/467127 imported/csswg-test/css-flexbox-1/getcomputedstyle/flexbox_comp
 crbug.com/467127 imported/csswg-test/css-flexbox-1/getcomputedstyle/flexbox_computedstyle_flex-basis-0percent.html [ Failure ]
 crbug.com/467127 imported/csswg-test/css-flexbox-1/getcomputedstyle/flexbox_computedstyle_flex-basis-percent.html [ Failure ]
 crbug.com/467127 imported/csswg-test/css-flexbox-1/getcomputedstyle/flexbox_computedstyle_flex-shorthand-number.html [ Failure ]
-crbug.com/467127 [ Mac ] imported/csswg-test/css-flexbox-1/flex-minimum-width-flex-items-007.xht [ Failure ]
 crbug.com/467127 [ Mac Win ] imported/csswg-test/css-flexbox-1/ttwf-reftest-flex-wrap-reverse.html [ Failure ]
 crbug.com/467127 [ Mac Win ] imported/csswg-test/css-flexbox-1/ttwf-reftest-flex-wrap.html [ Failure ]
 

diff --git a/third_party/WebKit/LayoutTests/css3/flexbox/minimum-size-image.html b/third_party/WebKit/LayoutTests/css3/flexbox/minimum-size-image.html
@@ -0,0 +1,70 @@
+<!DOCTYPE html>
+
+<link href="resources/flexbox.css" rel="stylesheet">
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../../resources/check-layout-th.js"></script>
+
+<body onload="checkLayout('.flexbox')">
+<div id=log></div>
+
+<div class="flexbox" style="width: 10px;" data-expected-width="10">
+    <!-- should use content width, clamped by converted max-height, as minimum width. -->
+    <img src="../images/resources/green-100.png" style="max-height: 5px;"
+         data-expected-width="5" data-expected-height="5">
+</div>
+
+<div class="flexbox" style="width: 10px;" data-expected-width="10">
+    <!-- should use min(specified, content width) = 10px as minimum width,
+         which the image will shrink to due to default flex-shrink. -->
+    <img src="../images/resources/green-10.png" style="width: 100px;" data-expected-width="10">
+</div>
+
+
+<div class="flexbox" style="width: 10px;" data-expected-width="10">
+    <!-- should use content width, clamped by converted min-height, as minimum width. -->
+    <img src="../images/resources/green-100.png" style="max-height: 5px;"
+         data-expected-width="5" data-expected-height="5">
+</div>
+
+<div class="flexbox" style="width: 10px;" data-expected-width="10">
+    <!-- should use content width, clamped by converted min-height, as minimum width. -->
+    <img src="../images/resources/green-100.png" style="max-height: 5px; min-height: 10px;"
+         data-expected-width="10" data-expected-height="10">
+</div>
+
+<div class="flexbox" style="width: 10px;" data-expected-width="10">
+    <!-- should use min(transferred, content width) = 10px as minimum width,
+         which the image will shrink to due to default flex-shrink. -->
+    <img src="../images/resources/green-10.png" style="height: 100px;" data-expected-width="10">
+</div>
+
+<div class="flexbox column" style="height: 10px;" data-expected-height="10">
+    <!-- should use content height, clamped by converted max-width, as minimum height. -->
+    <img src="../images/resources/green-100.png" style="max-width: 5px;"
+         data-expected-width="5" data-expected-height="5">
+</div>
+
+<div class="flexbox column" style="height: 10px;" data-expected-height="10">
+    <!-- should use min(specified, content height) = 10px as minimum height,
+         which the image will shrink to due to default flex-shrink. -->
+    <img src="../images/resources/green-10.png" style="height: 100px;" data-expected-height="10">
+</div>
+
+<div class="flexbox" style="height: 10px;" data-expected-height="10">
+    <!-- should use content height, clamped by converted min-height, as minimum height. -->
+    <img src="../images/resources/green-100.png" style="max-height: 5px;"
+         data-expected-height="5" data-expected-height="5">
+</div>
+
+<div class="flexbox" style="height: 10px;" data-expected-height="10">
+    <!-- should use content height, clamped by converted min-width, as minimum height. -->
+    <img src="../images/resources/green-100.png" style="max-width: 5px; min-width: 10px;"
+         data-expected-height="10" data-expected-width="10">
+</div>
+
+<div class="flexbox column" style="height: 10px;" data-expected-height="10">
+    <!-- should use min(transferred, content height) = 10px as minimum height,
+         which the image will shrink to due to default flex-shrink. -->
+    <img src="../images/resources/green-10.png" style="width: 100px;" data-expected-height="10">
+</div>
diff --git a/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp b/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
@@ -42,6 +42,11 @@
 
 namespace blink {
 
+static bool hasAspectRatio(const LayoutBox& child)
+{
+    return child.isImage() || child.isCanvas() || child.isVideo();
+}
+
 struct LayoutFlexibleBox::LineContext {
     LineContext(LayoutUnit crossAxisOffset, LayoutUnit crossAxisExtent, size_t numberOfChildren, LayoutUnit maxAscent)
         : crossAxisOffset(crossAxisOffset)
@@ -452,7 +457,7 @@ LayoutUnit LayoutFlexibleBox::computeMainAxisExtentForChild(const LayoutBox& chi
     // computeLogicalWidth always re-computes the intrinsic widths. However, when our logical width is auto,
     // we can just use our cached value. So let's do that here. (Compare code in LayoutBlock::computePreferredLogicalWidths)
     LayoutUnit borderAndPadding = child.borderAndPaddingLogicalWidth();
-    if (child.styleRef().logicalWidth().isAuto()) {
+    if (child.styleRef().logicalWidth().isAuto() && !hasAspectRatio(child)) {
         if (size.type() == MinContent)
             return child.minPreferredLogicalWidth() - borderAndPadding;
         if (size.type() == MaxContent)
@@ -625,6 +630,44 @@ LayoutPoint LayoutFlexibleBox::flowAwareLocationForChild(const LayoutBox& child)
     return isHorizontalFlow() ? child.location() : child.location().transposedPoint();
 }
 
+bool LayoutFlexibleBox::useChildAspectRatio(const LayoutBox& child) const
+{
+    if (!hasAspectRatio(child))
+        return false;
+    if (child.intrinsicSize().height() == 0) {
+        // We can't compute a ratio in this case.
+        return false;
+    }
+    Length crossSize;
+    if (isHorizontalFlow())
+        crossSize = child.styleRef().height();
+    else
+        crossSize = child.styleRef().width();
+    return crossAxisLengthIsDefinite(child, crossSize);
+}
+
+LayoutUnit LayoutFlexibleBox::computeMainSizeFromAspectRatioUsing(const LayoutBox& child, Length crossSizeLength) const
+{
+    ASSERT(hasAspectRatio(child));
+    ASSERT(child.intrinsicSize().height() != 0);
+
+    LayoutUnit crossSize;
+    if (crossSizeLength.isFixed()) {
+        crossSize = crossSizeLength.value();
+    } else {
+        ASSERT(crossSizeLength.hasPercent());
+        crossSize = hasOrthogonalFlow(child) ?
+            adjustBorderBoxLogicalWidthForBoxSizing(valueForLength(crossSizeLength, contentWidth())) :
+            child.computePercentageLogicalHeight(crossSizeLength);
+    }
+
+    const LayoutSize& childIntrinsicSize = child.intrinsicSize();
+    double ratio = childIntrinsicSize.width().toFloat() / childIntrinsicSize.height().toFloat();
+    if (isHorizontalFlow())
+        return crossSize * ratio;
+    return crossSize / ratio;
+}
+
 void LayoutFlexibleBox::setFlowAwareLocationForChild(LayoutBox& child, const LayoutPoint& location)
 {
     if (isHorizontalFlow())
@@ -650,6 +693,20 @@ bool LayoutFlexibleBox::mainAxisLengthIsDefinite(const LayoutBox& child, const L
     return true;
 }
 
+bool LayoutFlexibleBox::crossAxisLengthIsDefinite(const LayoutBox& child, const Length& length) const
+{
+    if (length.isAuto())
+        return false;
+    if (length.hasPercent()) {
+        return hasOrthogonalFlow(child) ?
+            hasDefiniteLogicalWidth() :
+            child.computePercentageLogicalHeight(length) != -1;
+    }
+    // TODO(cbiesinger): Eventually we should support other types of sizes here. Requires updating
+    // computeMainSizeFromAspectRatioUsing.
+    return length.isFixed();
+}
+
 bool LayoutFlexibleBox::childFlexBaseSizeRequiresLayout(const LayoutBox& child) const
 {
     return !mainAxisLengthIsDefinite(child, flexBasisForChild(child)) && (
@@ -919,6 +976,8 @@ LayoutUnit LayoutFlexibleBox::adjustChildSizeForMinAndMax(const LayoutBox& child
         // css-flexbox section 4.5
         LayoutUnit contentSize = computeMainAxisExtentForChild(child, MinSize, Length(MinContent));
         ASSERT(contentSize >= 0);
+        if (hasAspectRatio(child) && child.intrinsicSize().height() > 0)
+            contentSize = adjustChildSizeForAspectRatioCrossAxisMinAndMax(child, contentSize);
         if (maxExtent != -1 && contentSize > maxExtent)
             contentSize = maxExtent;
 
@@ -929,15 +988,38 @@ LayoutUnit LayoutFlexibleBox::adjustChildSizeForMinAndMax(const LayoutBox& child
             LayoutUnit specifiedSize = maxExtent != -1 ? std::min(resolvedMainSize, maxExtent) : resolvedMainSize;
 
             minExtent = std::min(specifiedSize, contentSize);
+        } else if (useChildAspectRatio(child)) {
+            Length crossSizeLength = isHorizontalFlow() ? child.styleRef().height() : child.styleRef().width();
+            LayoutUnit transferredSize = computeMainSizeFromAspectRatioUsing(child, crossSizeLength);
+            transferredSize = adjustChildSizeForAspectRatioCrossAxisMinAndMax(child, transferredSize);
+            minExtent = std::min(transferredSize, contentSize);
         } else {
             minExtent = contentSize;
         }
-        // TODO(cbiesinger): Implement aspect ratio handling (here, transferred size) - crbug.com/249112
     }
     ASSERT(minExtent >= 0);
     return std::max(childSize, minExtent);
 }
 
+LayoutUnit LayoutFlexibleBox::adjustChildSizeForAspectRatioCrossAxisMinAndMax(const LayoutBox& child, LayoutUnit childSize)
+{
+    Length crossMin = isHorizontalFlow() ? child.style()->minHeight() : child.style()->minWidth();
+    Length crossMax = isHorizontalFlow() ? child.style()->maxHeight() : child.style()->maxWidth();
+
+
+    if (crossAxisLengthIsDefinite(child, crossMax)) {
+        LayoutUnit maxValue = computeMainSizeFromAspectRatioUsing(child, crossMax);
+        childSize = std::min(maxValue, childSize);
+    }
+
+    if (crossAxisLengthIsDefinite(child, crossMin)) {
+        LayoutUnit minValue = computeMainSizeFromAspectRatioUsing(child, crossMin);
+        childSize = std::max(minValue, childSize);
+    }
+
+    return childSize;
+}
+
 bool LayoutFlexibleBox::computeNextFlexLine(OrderedFlexItemList& orderedChildren, LayoutUnit& sumFlexBaseSize, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink, LayoutUnit& sumHypotheticalMainSize, bool relayoutChildren)
 {
     orderedChildren.clear();
@@ -1177,6 +1259,7 @@ bool LayoutFlexibleBox::needToStretchChildLogicalHeight(const LayoutBox& child)
     if (isHorizontalFlow() != child.styleRef().isHorizontalWritingMode())
         return false;
 
+    // TODO(cbiesinger): what about indefinite percentage heights?
     return isHorizontalFlow() ? child.styleRef().height().isAuto() : child.styleRef().width().isAuto();
 }
 
@@ -1206,6 +1289,7 @@ EOverflow LayoutFlexibleBox::crossAxisOverflowForChild(const LayoutBox& child) c
         return child.styleRef().overflowY();
     return child.styleRef().overflowX();
 }
+
 void LayoutFlexibleBox::layoutAndPlaceChildren(LayoutUnit& crossAxisOffset, const OrderedFlexItemList& children, const Vector<LayoutUnit, 16>& childSizes, LayoutUnit availableFreeSpace, bool relayoutChildren, SubtreeLayoutScope& layoutScope, Vector<LineContext>& lineContexts)
 {
     ASSERT(childSizes.size() == children.size());

diff --git a/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h b/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h
@@ -127,12 +127,15 @@ class CORE_EXPORT LayoutFlexibleBox : public LayoutBlock {
     LayoutUnit crossAxisScrollbarExtent() const;
     LayoutUnit crossAxisScrollbarExtentForChild(const LayoutBox& child) const;
     LayoutPoint flowAwareLocationForChild(const LayoutBox& child) const;
+    bool useChildAspectRatio(const LayoutBox& child) const;
+    LayoutUnit computeMainSizeFromAspectRatioUsing(const LayoutBox& child, Length crossSizeLength) const;
     void setFlowAwareLocationForChild(LayoutBox& child, const LayoutPoint&);
     void adjustAlignmentForChild(LayoutBox& child, LayoutUnit);
     ItemPosition alignmentForChild(const LayoutBox& child) const;
     LayoutUnit mainAxisBorderAndPaddingExtentForChild(const LayoutBox& child) const;
     LayoutUnit computeInnerFlexBaseSizeForChild(LayoutBox& child, ChildLayoutType = LayoutIfNeeded);
     bool mainAxisLengthIsDefinite(const LayoutBox& child, const Length& flexBasis) const;
+    bool crossAxisLengthIsDefinite(const LayoutBox& child, const Length& flexBasis) const;
     bool childFlexBaseSizeRequiresLayout(const LayoutBox& child) const;
     bool needToStretchChildLogicalHeight(const LayoutBox& child) const;
     bool childHasIntrinsicMainAxisSize(const LayoutBox& child) const;
@@ -154,6 +157,7 @@ class CORE_EXPORT LayoutFlexibleBox : public LayoutBlock {
     LayoutUnit computeChildMarginValue(Length margin);
     void prepareOrderIteratorAndMargins();
     LayoutUnit adjustChildSizeForMinAndMax(const LayoutBox& child, LayoutUnit childSize);
+    LayoutUnit adjustChildSizeForAspectRatioCrossAxisMinAndMax(const LayoutBox& child, LayoutUnit childSize);
     // The hypothetical main size of an item is the flex base size clamped according to its min and max main size properties
     bool computeNextFlexLine(OrderedFlexItemList& orderedChildren, LayoutUnit& sumFlexBaseSize, double& totalFlexGrow, double& totalFlexShrink, double& totalWeightedFlexShrink, LayoutUnit& sumHypotheticalMainSize, bool relayoutChildren);