Flexitem margins should be based on content width, not width
authortony@chromium.org <tony@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2012 18:39:27 +0000 (18:39 +0000)
committertony@chromium.org <tony@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2012 18:39:27 +0000 (18:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=96674

Reviewed by Ojan Vafai.

Source/WebCore:

Margins should be based on content width. Also remove some calls to
mainAxisContentExtent() when we can use contentLogicalWidth instead.

Tests: css3/flexbox/percent-margins.html has a new testcase.

* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::computeMainAxisExtentForChild): Use contentLogicalWidth instead of
passing in the maximum value (only used in the width case).
(WebCore::RenderFlexibleBox::preferredMainAxisContentExtentForChild): Don't pass in mainAxisContentExtent.
(WebCore::RenderFlexibleBox::computeChildMarginValue): Use contentLogicalWidth and simplify the code.
(WebCore::RenderFlexibleBox::computeMainAxisPreferredSizes): Remove unnecessary mainAxisContentExtent() call.
(WebCore::RenderFlexibleBox::adjustChildSizeForMinAndMax): Remove unnecessary flexboxAvailableContentExtent.
(WebCore::RenderFlexibleBox::computeNextFlexLine): Remove unnecessary mainAxisContentExtent() call.
(WebCore::RenderFlexibleBox::resolveFlexibleLengths): Remove unnecessary mainAxisContentExtent() call.
* rendering/RenderFlexibleBox.h: Remove unnecessary params.

LayoutTests:

Add a test case where contentWidth != width.

* css3/flexbox/percent-margins-expected.txt:
* css3/flexbox/percent-margins.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@128486 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/css3/flexbox/percent-margins-expected.txt
LayoutTests/css3/flexbox/percent-margins.html
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderFlexibleBox.h

index 8d87527..57fcd1f 100644 (file)
@@ -1,3 +1,15 @@
+2012-09-13  Tony Chang  <tony@chromium.org>
+
+        Flexitem margins should be based on content width, not width
+        https://bugs.webkit.org/show_bug.cgi?id=96674
+
+        Reviewed by Ojan Vafai.
+
+        Add a test case where contentWidth != width.
+
+        * css3/flexbox/percent-margins-expected.txt:
+        * css3/flexbox/percent-margins.html:
+
 2012-09-13  Joshua Bell  <jsbell@chromium.org>
 
         IndexedDB: Properties of IDBKeyRange should default to undefined
index c458aad..a552a32 100644 (file)
@@ -16,4 +16,9 @@ if (window.testRunner)
     <div style="margin: 10%; background-color: orange; height: 5px; width: 5px" data-expected-width=5 data-expected-height=5></div>
 </div>
 
+<div class="flexbox" style="display:-webkit-flex; background-color: salmon; height: 300px; width: 400px; padding: 100px;">
+    <div style="margin: 10%; background-color: orange; height: 5px; -webkit-flex: 1 5px" data-expected-width=235 data-expected-height=5></div>
+    <div style="margin: 10%; background-color: orange; height: 5px; width: 5px" data-expected-width=5 data-expected-height=5></div>
+</div>
+
 </body>
index 4e84615..450ccb5 100644 (file)
@@ -1,3 +1,26 @@
+2012-09-13  Tony Chang  <tony@chromium.org>
+
+        Flexitem margins should be based on content width, not width
+        https://bugs.webkit.org/show_bug.cgi?id=96674
+
+        Reviewed by Ojan Vafai.
+
+        Margins should be based on content width. Also remove some calls to
+        mainAxisContentExtent() when we can use contentLogicalWidth instead.
+
+        Tests: css3/flexbox/percent-margins.html has a new testcase.
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::computeMainAxisExtentForChild): Use contentLogicalWidth instead of
+        passing in the maximum value (only used in the width case).
+        (WebCore::RenderFlexibleBox::preferredMainAxisContentExtentForChild): Don't pass in mainAxisContentExtent.
+        (WebCore::RenderFlexibleBox::computeChildMarginValue): Use contentLogicalWidth and simplify the code.
+        (WebCore::RenderFlexibleBox::computeMainAxisPreferredSizes): Remove unnecessary mainAxisContentExtent() call.
+        (WebCore::RenderFlexibleBox::adjustChildSizeForMinAndMax): Remove unnecessary flexboxAvailableContentExtent.
+        (WebCore::RenderFlexibleBox::computeNextFlexLine): Remove unnecessary mainAxisContentExtent() call.
+        (WebCore::RenderFlexibleBox::resolveFlexibleLengths): Remove unnecessary mainAxisContentExtent() call.
+        * rendering/RenderFlexibleBox.h: Remove unnecessary params.
+
 2012-09-13  Thiago Marcos P. Santos  <thiago.santos@intel.com>
 
         [EFL] Assertion reached on RenderThemeEFL when setting a theme to an invalid path
index 286ab7c..69895f2 100644 (file)
@@ -403,13 +403,13 @@ LayoutUnit RenderFlexibleBox::mainAxisContentExtent()
     return contentLogicalWidth();
 }
 
-LayoutUnit RenderFlexibleBox::computeMainAxisExtentForChild(RenderBox* child, SizeType sizeType, const Length& size, LayoutUnit maximumValue)
+LayoutUnit RenderFlexibleBox::computeMainAxisExtentForChild(RenderBox* child, SizeType sizeType, const Length& size)
 {
     // FIXME: This is wrong for orthogonal flows. It should use the flexbox's writing-mode, not the child's in order
     // to figure out the logical height/width.
     if (isColumnFlow())
         return child->computeContentLogicalHeight(sizeType, size);
-    return child->adjustContentBoxLogicalWidthForBoxSizing(valueForLength(size, maximumValue, view()));
+    return child->adjustContentBoxLogicalWidthForBoxSizing(valueForLength(size, contentLogicalWidth(), view()));
 }
 
 WritingMode RenderFlexibleBox::transformedWritingMode() const
@@ -608,7 +608,7 @@ LayoutUnit RenderFlexibleBox::preferredMainAxisContentExtentForChild(RenderBox*
         LayoutUnit mainAxisExtent = hasOrthogonalFlow(child) ? child->logicalHeight() : child->maxPreferredLogicalWidth();
         return mainAxisExtent - mainAxisBorderAndPaddingExtentForChild(child);
     }
-    return std::max(LayoutUnit(0), computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis, mainAxisContentExtent()));
+    return std::max(LayoutUnit(0), computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis));
 }
 
 LayoutUnit RenderFlexibleBox::computeAvailableFreeSpace(LayoutUnit preferredMainAxisExtent)
@@ -752,17 +752,16 @@ LayoutUnit RenderFlexibleBox::marginBoxAscentForChild(RenderBox* child)
     return ascent + flowAwareMarginBeforeForChild(child);
 }
 
-LayoutUnit RenderFlexibleBox::computeMarginValue(Length margin, LayoutUnit availableSize, RenderView* view)
+LayoutUnit RenderFlexibleBox::computeChildMarginValue(Length margin, RenderView* view)
 {
-    // CSS always computes percent margins with respect to the containing block's width, even for margin-top/margin-bottom.
-    if (margin.isPercent())
-        availableSize = logicalWidth();
+    // When resolving the margins, we use the content size for resolving percent and calc (for percents in calc expressions) margins.
+    // Fortunately, percent margins are always computed with respect to the block's width, even for margin-top and margin-bottom.
+    LayoutUnit availableSize = contentLogicalWidth();
     return minimumValueForLength(margin, availableSize, view);
 }
 
 void RenderFlexibleBox::computeMainAxisPreferredSizes(bool relayoutChildren, OrderHashSet& orderValues)
 {
-    LayoutUnit flexboxAvailableContentExtent = mainAxisContentExtent();
     RenderView* renderView = view();
     for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
         orderValues.add(child->style()->order());
@@ -782,11 +781,11 @@ void RenderFlexibleBox::computeMainAxisPreferredSizes(bool relayoutChildren, Ord
         // Before running the flex algorithm, 'auto' has a margin of 0.
         // Also, if we're not auto sizing, we don't do a layout that computes the start/end margins.
         if (isHorizontalFlow()) {
-            child->setMarginLeft(computeMarginValue(child->style()->marginLeft(), flexboxAvailableContentExtent, renderView));
-            child->setMarginRight(computeMarginValue(child->style()->marginRight(), flexboxAvailableContentExtent, renderView));
+            child->setMarginLeft(computeChildMarginValue(child->style()->marginLeft(), renderView));
+            child->setMarginRight(computeChildMarginValue(child->style()->marginRight(), renderView));
         } else {
-            child->setMarginTop(computeMarginValue(child->style()->marginTop(), flexboxAvailableContentExtent, renderView));
-            child->setMarginBottom(computeMarginValue(child->style()->marginBottom(), flexboxAvailableContentExtent, renderView));
+            child->setMarginTop(computeChildMarginValue(child->style()->marginTop(), renderView));
+            child->setMarginBottom(computeChildMarginValue(child->style()->marginBottom(), renderView));
         }
     }
 }
@@ -802,12 +801,12 @@ LayoutUnit RenderFlexibleBox::lineBreakLength()
     return computedValues.m_extent - borderAndPaddingLogicalHeight() - scrollbarLogicalHeight();
 }
 
-LayoutUnit RenderFlexibleBox::adjustChildSizeForMinAndMax(RenderBox* child, LayoutUnit childSize, LayoutUnit flexboxAvailableContentExtent)
+LayoutUnit RenderFlexibleBox::adjustChildSizeForMinAndMax(RenderBox* child, LayoutUnit childSize)
 {
     // FIXME: Support intrinsic min/max lengths.
     Length max = isHorizontalFlow() ? child->style()->maxWidth() : child->style()->maxHeight();
     if (max.isSpecified()) {
-        LayoutUnit maxExtent = computeMainAxisExtentForChild(child, MaxSize, max, flexboxAvailableContentExtent);
+        LayoutUnit maxExtent = computeMainAxisExtentForChild(child, MaxSize, max);
         if (maxExtent != -1 && childSize > maxExtent)
             childSize = maxExtent;
     }
@@ -815,7 +814,7 @@ LayoutUnit RenderFlexibleBox::adjustChildSizeForMinAndMax(RenderBox* child, Layo
     Length min = isHorizontalFlow() ? child->style()->minWidth() : child->style()->minHeight();
     LayoutUnit minExtent = 0;
     if (min.isSpecified())
-        minExtent = computeMainAxisExtentForChild(child, MinSize, min, flexboxAvailableContentExtent);
+        minExtent = computeMainAxisExtentForChild(child, MinSize, min);
     else if (min.isAuto()) {
         minExtent = hasOrthogonalFlow(child) ? child->logicalHeight() : child->minPreferredLogicalWidth();
         minExtent -= mainAxisBorderAndPaddingExtentForChild(child);
@@ -833,7 +832,6 @@ bool RenderFlexibleBox::computeNextFlexLine(OrderIterator& iterator, OrderedFlex
     if (!iterator.currentChild())
         return false;
 
-    LayoutUnit flexboxAvailableContentExtent = mainAxisContentExtent();
     LayoutUnit lineBreak = lineBreakLength();
 
     for (RenderBox* child = iterator.currentChild(); child; child = iterator.next()) {
@@ -853,7 +851,7 @@ bool RenderFlexibleBox::computeNextFlexLine(OrderIterator& iterator, OrderedFlex
         totalFlexGrow += child->style()->flexGrow();
         totalWeightedFlexShrink += child->style()->flexShrink() * childMainAxisExtent;
 
-        LayoutUnit childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(child, childMainAxisExtent, flexboxAvailableContentExtent);
+        LayoutUnit childMinMaxAppliedMainAxisExtent = adjustChildSizeForMinAndMax(child, childMainAxisExtent);
         minMaxAppliedMainAxisExtent += childMinMaxAppliedMainAxisExtent - childMainAxisExtent + childMainAxisMarginBoxExtent;
     }
     return true;
@@ -876,7 +874,6 @@ void RenderFlexibleBox::freezeViolations(const WTF::Vector<Violation>& violation
 bool RenderFlexibleBox::resolveFlexibleLengths(FlexSign flexSign, const OrderedFlexItemList& children, LayoutUnit& availableFreeSpace, float& totalFlexGrow, float& totalWeightedFlexShrink, InflexibleFlexItemSize& inflexibleItems, WTF::Vector<LayoutUnit>& childSizes)
 {
     childSizes.clear();
-    LayoutUnit flexboxAvailableContentExtent = mainAxisContentExtent();
     LayoutUnit totalViolation = 0;
     LayoutUnit usedFreeSpace = 0;
     WTF::Vector<Violation> minViolations;
@@ -898,7 +895,7 @@ bool RenderFlexibleBox::resolveFlexibleLengths(FlexSign flexSign, const OrderedF
             else if (availableFreeSpace < 0 && totalWeightedFlexShrink > 0 && flexSign == NegativeFlexibility && isfinite(totalWeightedFlexShrink))
                 childSize += roundedLayoutUnit(availableFreeSpace * child->style()->flexShrink() * preferredChildSize / totalWeightedFlexShrink);
 
-            LayoutUnit adjustedChildSize = adjustChildSizeForMinAndMax(child, childSize, flexboxAvailableContentExtent);
+            LayoutUnit adjustedChildSize = adjustChildSizeForMinAndMax(child, childSize);
             childSizes.append(adjustedChildSize);
             usedFreeSpace += adjustedChildSize - preferredChildSize;
 
index d418ce8..c95105f 100644 (file)
@@ -86,7 +86,7 @@ private:
     LayoutUnit mainAxisExtent() const;
     LayoutUnit crossAxisContentExtent() const;
     LayoutUnit mainAxisContentExtent();
-    LayoutUnit computeMainAxisExtentForChild(RenderBox* child, SizeType, const Length& size, LayoutUnit maximumValue);
+    LayoutUnit computeMainAxisExtentForChild(RenderBox* child, SizeType, const Length& size);
     WritingMode transformedWritingMode() const;
     LayoutUnit flowAwareBorderStart() const;
     LayoutUnit flowAwareBorderEnd() const;
@@ -120,10 +120,10 @@ private:
     LayoutUnit availableAlignmentSpaceForChild(LayoutUnit lineCrossAxisExtent, RenderBox*);
     LayoutUnit marginBoxAscentForChild(RenderBox*);
 
-    LayoutUnit computeMarginValue(Length margin, LayoutUnit availableSize, RenderView*);
+    LayoutUnit computeChildMarginValue(Length margin, RenderView*);
     void computeMainAxisPreferredSizes(bool relayoutChildren, OrderHashSet&);
     LayoutUnit lineBreakLength();
-    LayoutUnit adjustChildSizeForMinAndMax(RenderBox*, LayoutUnit childSize, LayoutUnit flexboxAvailableContentExtent);
+    LayoutUnit adjustChildSizeForMinAndMax(RenderBox*, LayoutUnit childSize);
     bool computeNextFlexLine(OrderIterator&, OrderedFlexItemList& orderedChildren, LayoutUnit& preferredMainAxisExtent, float& totalFlexGrow, float& totalWeightedFlexShrink, LayoutUnit& minMaxAppliedMainAxisExtent);
     LayoutUnit computeAvailableFreeSpace(LayoutUnit preferredMainAxisExtent);