Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis. r?mats draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Wed, 28 Mar 2018 18:45:09 -0700
changeset 774509 267765682fe40d752de2addcbf7d561728496de9
parent 774444 6aa3b57955fed5e137d0306478e1a4b424a6d392
push id104421
push userdholbert@mozilla.com
push dateThu, 29 Mar 2018 01:45:24 +0000
reviewersmats
bugs1436881
milestone61.0a1
Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis. r?mats This patch should not affect behavior. Logic-wise: the idea behind this patch is to behave as if the 'usingFlexBasisForHeight' variable were always false, which in turn lets us remove an "if (!usingFlexBasisForHeight || ...)" check, because it trivially passes when that bool is false. Background on this special case & why we can remove it: ======================================================= In the original flexbox implementation, we had some special-case code to be sure we didn't end up swapping in e.g. "flex-basis:-moz-min-content" for "height" in these ComputeSize functions, because that was a scenario that previously would've been prevented at the parser level (height:-moz-min-content is rejected for now), and hence may not have ended up being handled robustly. However, nowadays it'll be handled just fine in these functions, and will produce the same result as our special-case exception tries to achieve. In particular, for a nsFrame that is a flex item in a flex container with a vertical main axis (the scenario that these special cases are catching): - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this nsFrame has a vertical writing-mode), then Stylo actually converts enumerated flex-basis values like "-moz-min-content" to "auto" at the parser level. So it's not actually possible to get a computed "flex-basis" of -moz-min-content, in that scenario. - Otherwise, i.e. if the (vertical) main axis is this nsFrame's block axis, then these ComputeSize functions will now end up getting an enumerated "blockStyleCoord" (really pointing to flexBasis), but that'll still end up being treated like 'auto'. This happens by virtue of ComputeSize's calls to ComputeAutoSize (which initializes the tentative bsize value to NS_UNCONSTRAINEDSIZE) and to nsLayoutUtils::IsAutoBSize (which returns "true" for eStyleUnit_Enumerated values and then makes us leave the ComputeAutoSize result unperturbed). MozReview-Commit-ID: CU1pFwSoBQ4
layout/generic/nsFrame.cpp
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -5639,31 +5639,19 @@ nsFrame::ComputeSize(gfxContext*        
     usingFlexBasisForISize =
       (flexContainerIsRowOriented == inlineAxisSameAsParent);
 
     // NOTE: The logic here should match the similar chunk for determining
     // inlineStyleCoord and blockStyleCoord in
     // nsFrame::ComputeSizeWithIntrinsicDimensions().
     const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
     if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-      // One caveat for when flex-basis is stomping on 'height': We don't
-      // support enumerated values (e.g. "max-content") for height yet (that's
-      // bug 567039). So, if our computed flex-basis is an enumerated value,
-      // we'll just behave as if it were "auto", which means "use the main-size
-      // property after all" (which is "height", in this case).  NOTE: Once we
-      // support intrinsic sizing keywords for "height", we should remove this
-      // check.
-      bool usingFlexBasisForHeight =
-        (usingFlexBasisForISize != aWM.IsVertical());
-      if (!usingFlexBasisForHeight ||
-          flexBasis->GetUnit() != eStyleUnit_Enumerated) {
-        // Override whichever coord we're overriding:
-        (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
-          flexBasis;
-      }
+      // Override whichever styleCoord is in flex container's main axis:
+      (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
+        flexBasis;
     }
   }
 
   // Compute inline-axis size
   if (inlineStyleCoord->GetUnit() != eStyleUnit_Auto) {
     result.ISize(aWM) =
       ComputeISizeValue(aRenderingContext, aCBSize.ISize(aWM),
                         boxSizingAdjust.ISize(aWM), boxSizingToMarginEdgeISize,
@@ -5902,31 +5890,19 @@ nsFrame::ComputeSizeWithIntrinsicDimensi
       // 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().
       const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
       if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-        // One caveat for when flex-basis is stomping on 'height': We don't
-        // support enumerated values (e.g. "max-content") for height yet
-        // (that's bug 567039). So, if our computed flex-basis is an enumerated
-        // value, we'll just behave as if it were "auto", which means "use the
-        // main-size property after all" (which is "height", in this case).
-        // NOTE: Once we support intrinsic sizing keywords for "height", we
-        // should remove this check.
-        bool usingFlexBasisForHeight =
-          (usingFlexBasisForISize != aWM.IsVertical());
-        if (!usingFlexBasisForHeight ||
-            flexBasis->GetUnit() != eStyleUnit_Enumerated) {
-          // Override whichever coord we're overriding:
-          (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
-            flexBasis;
-        }
+        // Override whichever styleCoord is in flex container's main axis:
+        (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
+          flexBasis;
       }
     }
   }
 
   // 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