Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath. r?mats draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Fri, 13 Apr 2018 12:17:51 -0700
changeset 781926 ec54ec22e70fe0d4d8a615bf571c4d1111329fc1
parent 781925 4b5fc02a2a82a119f898ab8c75053654bdb46c25
child 781927 55dd4eaee9eaf051ae1e72878cdb0220b563554a
push id106442
push userdholbert@mozilla.com
push dateFri, 13 Apr 2018 19:18:31 +0000
reviewersmats
bugs1374540
milestone61.0a1
Bug 1374540 part 3: Refactor nsFrame::ComputeSize methods to handle the two "used flex-basis of content" scenarios with a consistent codepath. r?mats This patch doesn't change behavior. It simply makes us share code/data for two different cases that both ended up producing mainAxisCoord->GetUnit() == eStyleUnit_Auto. Now, they'll *both* use the same static nsStyleCoord to represent this "auto" value. Originally, in one of these cases ("flex-basis:auto;[main-size-property]:auto), we left the mainAxisCoord untouched. Now we'll point it at this dummy 'auto' value. Either way we end up with mainAxisCoord->GetUnit() == eStyleUnit_Auto, so the behavior doesn't change. The next patch in this series will make further changes to one of these spots, as noted in the "XXXdholbert" code-comment included here. MozReview-Commit-ID: 5ClfbNHuKhO
layout/generic/nsFlexContainerFrame.cpp
layout/generic/nsFlexContainerFrame.h
layout/generic/nsFrame.cpp
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -4424,16 +4424,32 @@ nsFlexContainerFrame::IsItemInlineAxisMa
     flexDirection == NS_STYLE_FLEX_DIRECTION_ROW ||
     flexDirection == NS_STYLE_FLEX_DIRECTION_ROW_REVERSE;
 
   // aFrame's inline axis is its flex container's main axis IFF the above
   // questions have the same answer.
   return flexContainerIsRowOriented == itemInlineAxisIsParallelToParent;
 }
 
+/* static */
+bool
+nsFlexContainerFrame::IsUsedFlexBasisContent(const nsStyleCoord* aFlexBasis,
+                                             const nsStyleCoord* aMainSize)
+{
+  // We have a used flex-basis of 'content' if flex-basis explicitly has that
+  // value, OR if flex-basis is 'auto' (deferring to the main-size property)
+  // and the main-size property is also 'auto'.
+  // See https://drafts.csswg.org/css-flexbox-1/#valdef-flex-basis-auto
+  return
+    (aFlexBasis->GetUnit() == eStyleUnit_Enumerated &&
+     aFlexBasis->GetIntValue() == NS_STYLE_FLEX_BASIS_CONTENT) ||
+    (aFlexBasis->GetUnit() == eStyleUnit_Auto &&
+     aMainSize->GetUnit() == eStyleUnit_Auto);
+}
+
 void
 nsFlexContainerFrame::DoFlexLayout(nsPresContext*           aPresContext,
                                    ReflowOutput&     aDesiredSize,
                                    const ReflowInput& aReflowInput,
                                    nsReflowStatus&          aStatus,
                                    nscoord aContentBoxMainSize,
                                    nscoord aAvailableBSizeForContent,
                                    nsTArray<StrutInfo>& aStruts,
--- a/layout/generic/nsFlexContainerFrame.h
+++ b/layout/generic/nsFlexContainerFrame.h
@@ -7,16 +7,18 @@
 /* rendering object for CSS "display: flex" and "display: -webkit-box" */
 
 #ifndef nsFlexContainerFrame_h___
 #define nsFlexContainerFrame_h___
 
 #include "nsContainerFrame.h"
 #include "mozilla/UniquePtr.h"
 
+class nsStyleCoord;
+
 namespace mozilla {
 template <class T> class LinkedList;
 class LogicalPoint;
 } // namespace mozilla
 
 nsContainerFrame* NS_NewFlexContainerFrame(nsIPresShell* aPresShell,
                                            mozilla::ComputedStyle* aStyle);
 
@@ -207,16 +209,27 @@ public:
    * equivalent & more optimal.)
    *
    * @param aFrame a flex item (must return true from IsFlexItem)
    * @return true iff aFrame's inline axis is the same as (i.e. not orthogonal
    *              to) its flex container's main axis. Otherwise, false.
    */
   static bool IsItemInlineAxisMainAxis(nsIFrame* aFrame);
 
+  /**
+   * Returns true iff the given computed 'flex-basis' & main-size property
+   * values collectively represent a used flex-basis of 'content'.
+   * See https://drafts.csswg.org/css-flexbox-1/#valdef-flex-basis-auto
+   *
+   * @param aFlexBasis the computed 'flex-basis' for a flex item.
+   * @param aMainSize the computed main-size property for a flex item.
+   */
+  static bool IsUsedFlexBasisContent(const nsStyleCoord* aFlexBasis,
+                                     const nsStyleCoord* aMainSize);
+
 protected:
   // Protected constructor & destructor
   explicit nsFlexContainerFrame(ComputedStyle* aStyle)
     : nsContainerFrame(aStyle, kClassID)
     , mBaselineFromLastReflow(NS_INTRINSIC_WIDTH_UNKNOWN)
     , mLastBaselineFromLastReflow(NS_INTRINSIC_WIDTH_UNKNOWN)
   {}
 
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -5638,38 +5638,34 @@ nsFrame::ComputeSize(gfxContext*        
   LogicalAxis flexMainAxis = eLogicalAxisInline; // (init to make valgrind happy)
   if (isFlexItem) {
     // Flex items use their "flex-basis" property in place of their main-size
     // property for sizing purposes, *unless* they have "flex-basis:auto", in
     // which case they use their main-size property after all.
     flexMainAxis = nsFlexContainerFrame::IsItemInlineAxisMainAxis(this) ?
       eLogicalAxisInline : eLogicalAxisBlock;
 
-    // NOTE: The logic here should match the similar chunk for determining
-    // inlineStyleCoord and blockStyleCoord in
-    // nsFrame::ComputeSizeWithIntrinsicDimensions().
+    // NOTE: The logic here should match the similar chunk for updating
+    // mainAxisCoord in nsFrame::ComputeSizeWithIntrinsicDimensions().
     const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
-    if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-      // Replace our main-axis styleCoord pointer with a different one,
-      // depending on our flex-basis value.
-      auto& mainAxisCoord = (flexMainAxis == eLogicalAxisInline
-                             ? inlineStyleCoord : blockStyleCoord);
-      if (flexBasis->GetUnit() == eStyleUnit_Enumerated &&
-          flexBasis->GetIntValue() == NS_STYLE_FLEX_BASIS_CONTENT) {
-        // We have 'flex-basis: content', which is equivalent to
-        // 'flex-basis:auto; {main-size}: auto'. So, just swap in a dummy
-        // 'auto' value to use for the main size property:
-        static const nsStyleCoord autoStyleCoord(eStyleUnit_Auto);
-        mainAxisCoord = &autoStyleCoord;
-      } else {
-        // For all other flex-basis values, we just swap in the flex-basis
-        // itself for the main-size property here:
-        mainAxisCoord = flexBasis;
-      }
-    }
+    auto& mainAxisCoord = (flexMainAxis == eLogicalAxisInline
+                           ? inlineStyleCoord : blockStyleCoord);
+
+    if (nsFlexContainerFrame::IsUsedFlexBasisContent(flexBasis,
+                                                     mainAxisCoord)) {
+      // XXXdholbert Technically we shouldn't be using 'auto' here. A later
+      // patch will convert this to max-content instead.
+      static const nsStyleCoord autoStyleCoord(eStyleUnit_Auto);
+      mainAxisCoord = &autoStyleCoord;
+    } else if (flexBasis->GetUnit() != eStyleUnit_Auto) {
+      // For all other non-'auto' flex-basis values, we just swap in the
+      // flex-basis itself for the main-size property.
+      mainAxisCoord = flexBasis;
+    } // else: flex-basis is 'auto', which is deferring to some explicit value
+      // in mainAxisCoord. So we proceed w/o touching mainAxisCoord.
   }
 
   // Compute inline-axis size
   if (inlineStyleCoord->GetUnit() != eStyleUnit_Auto) {
     result.ISize(aWM) =
       ComputeISizeValue(aRenderingContext, aCBSize.ISize(aWM),
                         boxSizingAdjust.ISize(aWM), boxSizingToMarginEdgeISize,
                         *inlineStyleCoord, aFlags);
@@ -5894,38 +5890,44 @@ nsFrame::ComputeSizeWithIntrinsicDimensi
         blockStyleCoord = imposedMainSizeStyleCoord.ptr();
       }
 
     } else {
       // Flex items use their "flex-basis" property in place of their main-size
       // property (e.g. "width") for sizing purposes, *unless* they have
       // "flex-basis:auto", in which case they use their main-size property
       // after all.
-      // NOTE: The logic here should match the similar chunk for determining
-      // inlineStyleCoord and blockStyleCoord in nsFrame::ComputeSize().
+      // NOTE: The logic here should match the similar chunk for updating
+      // mainAxisCoord in nsFrame::ComputeSize().
       const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
-      if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-        // Replace our main-axis styleCoord pointer with a different one,
-        // depending on our flex-basis value.
-        auto& mainAxisCoord = (flexMainAxis == eLogicalAxisInline
-                               ? inlineStyleCoord : blockStyleCoord);
-
-        if (flexBasis->GetUnit() == eStyleUnit_Enumerated &&
-            flexBasis->GetIntValue() == NS_STYLE_FLEX_BASIS_CONTENT) {
-          // We have 'flex-basis: content', which is equivalent to
-          // 'flex-basis:auto; {main-size}: auto'. So, just swap in a dummy
-          // 'auto' value to use for the main size property:
-          static const nsStyleCoord autoStyleCoord(eStyleUnit_Auto);
-          mainAxisCoord = &autoStyleCoord;
-        } else {
-          // For all other flex-basis values, we just swap in the flex-basis
-          // itself for the main-size property here:
-          mainAxisCoord = flexBasis;
-        }
-      }
+      auto& mainAxisCoord = (flexMainAxis == eLogicalAxisInline
+                             ? inlineStyleCoord : blockStyleCoord);
+
+      if (nsFlexContainerFrame::IsUsedFlexBasisContent(flexBasis,
+                                                       mainAxisCoord)) {
+        // If we get here, we're resolving the flex base size for a flex item,
+        // and we fall into the flexbox spec section 9.2 step 3, substep C (if
+        // we have a definite cross size) or E (if not). And specifically:
+        //
+        // * If we have a definite cross size, we're supposed to resolve our
+        //   main-size based on that and our intrinsic ratio.
+        // * Otherwise, we're supposed to produce our max-content size.
+        //
+        // Conveniently, we can handle both of those scenarios (regardless of
+        // which substep we fall into) by using the 'auto' keyword for our
+        // main-axis coordinate here. (This makes sense, because the spec is
+        // effectively trying to produce the 'auto' sizing behavior).
+        static const nsStyleCoord autoStyleCoord(eStyleUnit_Auto);
+        mainAxisCoord = &autoStyleCoord;
+      } else if (flexBasis->GetUnit() != eStyleUnit_Auto) {
+        // For all other non-'auto' flex-basis values, we just swap in the
+        // flex-basis itself for the main-size property.
+        mainAxisCoord = flexBasis;
+      } // else: flex-basis is 'auto', which is deferring to some explicit
+        // value in mainAxisCoord. So we proceed w/o touching mainAxisCoord.
     }
   }
 
   // Handle intrinsic sizes and their interaction with
   // {min-,max-,}{width,height} according to the rules in
   // http://www.w3.org/TR/CSS21/visudet.html#min-max-widths
 
   // Note: throughout the following section of the function, I avoid