Bug 1500627 - Fix some potential bugs of sizing properties for keywords in the block axis. r=mats, a=RyanVM
authorBoris Chiou <boris.chiou@gmail.com>
Mon, 29 Oct 2018 22:56:42 +0000
changeset 500960 3dcc599a53b52e4a436017210768d5c1033c6b41
parent 500959 1a9849bb7eae0bf78a5a9fa6117e8e2f4d46e4b8
child 500961 812f83ad6fccfaca2cfc16d23fd7871bf19839ed
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, RyanVM
bugs1500627
milestone64.0
Bug 1500627 - Fix some potential bugs of sizing properties for keywords in the block axis. r=mats, a=RyanVM 1. Drop {Width|MinWidth|MaxWidh}DependsOnContainer() and {Height|MinHeight|MaxHeight}DependsOnContainer() because they are bogus after introducing the writing mode. Dropping these functions needs update nsLineLayout because it is the only one who still use these functions. 2. There are still some potential assertions and bugs on handling keywords in the block size, so we should update them as well. Depends on D9438 Differential Revision: https://phabricator.services.mozilla.com/D9439
layout/base/nsLayoutUtils.cpp
layout/generic/WritingModes.h
layout/generic/nsLineLayout.cpp
layout/style/nsStyleStruct.cpp
layout/style/nsStyleStruct.h
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -4823,32 +4823,31 @@ GetPercentBSize(const nsStyleCoord& aSty
 
   WritingMode wm = f->GetWritingMode();
 
   const nsStylePosition *pos = f->StylePosition();
   const nsStyleCoord& bSizeCoord = pos->BSize(wm);
   nscoord h;
   if (!GetAbsoluteCoord(bSizeCoord, h) &&
       !GetPercentBSize(bSizeCoord, f, aHorizontalAxis, h)) {
-    NS_ASSERTION(bSizeCoord.GetUnit() == eStyleUnit_Auto ||
-                 bSizeCoord.HasPercent(),
+    NS_ASSERTION(bSizeCoord.IsAutoOrEnum() || bSizeCoord.HasPercent(),
                  "unknown block-size unit");
     LayoutFrameType fType = f->Type();
     if (fType != LayoutFrameType::Viewport &&
         fType != LayoutFrameType::Canvas &&
         fType != LayoutFrameType::PageContent) {
       // There's no basis for the percentage height, so it acts like auto.
       // Should we consider a max-height < min-height pair a basis for
       // percentage heights?  The spec is somewhat unclear, and not doing
       // so is simpler and avoids troubling discontinuities in behavior,
       // so I'll choose not to. -LDB
       return false;
     }
 
-    NS_ASSERTION(bSizeCoord.GetUnit() == eStyleUnit_Auto,
+    NS_ASSERTION(bSizeCoord.IsAutoOrEnum(),
                  "Unexpected block-size unit for viewport or canvas or page-content");
     // For the viewport, canvas, and page-content kids, the percentage
     // basis is just the parent block-size.
     h = f->BSize(wm);
     if (h == NS_UNCONSTRAINEDSIZE) {
       // We don't have a percentage basis after all
       return false;
     }
@@ -4857,31 +4856,29 @@ GetPercentBSize(const nsStyleCoord& aSty
   const nsStyleCoord& maxBSizeCoord = pos->MaxBSize(wm);
 
   nscoord maxh;
   if (GetAbsoluteCoord(maxBSizeCoord, maxh) ||
       GetPercentBSize(maxBSizeCoord, f, aHorizontalAxis, maxh)) {
     if (maxh < h)
       h = maxh;
   } else {
-    NS_ASSERTION(maxBSizeCoord.GetUnit() == eStyleUnit_None ||
-                 maxBSizeCoord.HasPercent(),
+    NS_ASSERTION(maxBSizeCoord.IsAutoOrEnum() || maxBSizeCoord.HasPercent(),
                  "unknown max block-size unit");
   }
 
   const nsStyleCoord& minBSizeCoord = pos->MinBSize(wm);
 
   nscoord minh;
   if (GetAbsoluteCoord(minBSizeCoord, minh) ||
       GetPercentBSize(minBSizeCoord, f, aHorizontalAxis, minh)) {
     if (minh > h)
       h = minh;
   } else {
-    NS_ASSERTION(minBSizeCoord.HasPercent() ||
-                 minBSizeCoord.GetUnit() == eStyleUnit_Auto,
+    NS_ASSERTION(minBSizeCoord.IsAutoOrEnum() || minBSizeCoord.HasPercent(),
                  "unknown min block-size unit");
   }
 
   // Now adjust h for box-sizing styles on the parent.  We never ignore padding
   // here.  That could conceivably cause some problems with fieldsets (which are
   // the one place that wants to ignore padding), but solving that here without
   // hardcoding a check for f being a fieldset-content frame is a bit of a pain.
   nscoord bSizeTakenByBoxSizing =
@@ -8448,21 +8445,21 @@ nsLayoutUtils::FontSizeInflationInner(co
       // ruby annotations should have the same inflation as its
       // grandparent, which is the ruby frame contains the annotation.
       if (fType == LayoutFrameType::RubyText) {
         MOZ_ASSERT(parent && parent->IsRubyTextContainerFrame());
         nsIFrame* grandparent = parent->GetParent();
         MOZ_ASSERT(grandparent && grandparent->IsRubyFrame());
         return FontSizeInflationFor(grandparent);
       }
-      nsStyleCoord stylePosWidth = f->StylePosition()->mWidth;
-      nsStyleCoord stylePosHeight = f->StylePosition()->mHeight;
-      if (stylePosWidth.GetUnit() != eStyleUnit_Auto ||
-          stylePosHeight.GetUnit() != eStyleUnit_Auto) {
-
+      WritingMode wm = f->GetWritingMode();
+      nsStyleCoord stylePosISize = f->StylePosition()->ISize(wm);
+      nsStyleCoord stylePosBSize = f->StylePosition()->BSize(wm);
+      if (stylePosISize.GetUnit() != eStyleUnit_Auto ||
+          !stylePosBSize.IsAutoOrEnum()) {
         return 1.0;
       }
     }
   }
 
   int32_t interceptParam = nsLayoutUtils::FontSizeInflationMappingIntercept();
   float maxRatio = (float)nsLayoutUtils::FontSizeInflationMaxRatio() / 100.0f;
 
--- a/layout/generic/WritingModes.h
+++ b/layout/generic/WritingModes.h
@@ -2148,48 +2148,61 @@ inline const nsStyleCoord&
 nsStylePosition::MaxBSize(mozilla::WritingMode aWM) const
 {
   return aWM.IsVertical() ? mMaxWidth : mMaxHeight;
 }
 
 inline bool
 nsStylePosition::ISizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? HeightDependsOnContainer()
-                          : WidthDependsOnContainer();
+  const auto& iSize = aWM.IsVertical() ? mHeight : mWidth;
+  return iSize.GetUnit() == eStyleUnit_Auto ||
+         ISizeCoordDependsOnContainer(iSize);
 }
 inline bool
 nsStylePosition::MinISizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? MinHeightDependsOnContainer()
-                          : MinWidthDependsOnContainer();
+  // NOTE: For a flex item, "min-inline-size:auto" is supposed to behave like
+  // "min-content", which does depend on the container, so you might think we'd
+  // need a special case for "flex item && min-inline-size:auto" here. However,
+  // we don't actually need that special-case code, because flex items are
+  // explicitly supposed to *ignore* their min-inline-size (i.e. behave like
+  // it's 0) until the flex container explicitly considers it. So -- since the
+  // flex container doesn't rely on this method, we don't need to worry about
+  // special behavior for flex items' "min-inline-size:auto" values here.
+  return ISizeCoordDependsOnContainer(MinISize(aWM));
 }
 inline bool
 nsStylePosition::MaxISizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? MaxHeightDependsOnContainer()
-                          : MaxWidthDependsOnContainer();
+  // NOTE: The comment above MinISizeDependsOnContainer about flex items
+  // applies here, too.
+  return ISizeCoordDependsOnContainer(MaxISize(aWM));
 }
+// Note that these functions count `auto` as depending on the container
+// since that's the case for absolutely positioned elements.
+// However, some callers do not care about this case and should check
+// for it, since it is the most common case.
+// FIXME: We should probably change the assumption to be the other way
+// around.
 inline bool
 nsStylePosition::BSizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? WidthDependsOnContainer()
-                          : HeightDependsOnContainer();
+  const auto& bSize = aWM.IsVertical() ? mWidth : mHeight;
+  return bSize.IsAutoOrEnum() || BSizeCoordDependsOnContainer(bSize);
 }
 inline bool
 nsStylePosition::MinBSizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? MinWidthDependsOnContainer()
-                          : MinHeightDependsOnContainer();
+  return BSizeCoordDependsOnContainer(MinBSize(aWM));
 }
 inline bool
 nsStylePosition::MaxBSizeDependsOnContainer(mozilla::WritingMode aWM) const
 {
-  return aWM.IsVertical() ? MaxWidthDependsOnContainer()
-                          : MaxHeightDependsOnContainer();
+  return BSizeCoordDependsOnContainer(MaxBSize(aWM));
 }
 
 inline bool
 nsStyleMargin::HasBlockAxisAuto(mozilla::WritingMode aWM) const
 {
   return mMargin.HasBlockAxisAuto(aWM);
 }
 inline bool
--- a/layout/generic/nsLineLayout.cpp
+++ b/layout/generic/nsLineLayout.cpp
@@ -721,17 +721,17 @@ HasPercentageUnitSide(const nsStyleSides
   NS_FOR_CSS_SIDES(side) {
     if (aSides.Get(side).HasPercent())
       return true;
   }
   return false;
 }
 
 static bool
-IsPercentageAware(const nsIFrame* aFrame)
+IsPercentageAware(const nsIFrame* aFrame, WritingMode aWM)
 {
   NS_ASSERTION(aFrame, "null frame is not allowed");
 
   LayoutFrameType fType = aFrame->Type();
   if (fType == LayoutFrameType::Text) {
     // None of these things can ever be true for text frames.
     return false;
   }
@@ -750,26 +750,26 @@ IsPercentageAware(const nsIFrame* aFrame
   if (HasPercentageUnitSide(padding->mPadding)) {
     return true;
   }
 
   // Note that borders can't be aware of percentages
 
   const nsStylePosition* pos = aFrame->StylePosition();
 
-  if ((pos->WidthDependsOnContainer() &&
-       pos->mWidth.GetUnit() != eStyleUnit_Auto) ||
-      pos->MaxWidthDependsOnContainer() ||
-      pos->MinWidthDependsOnContainer() ||
-      pos->OffsetHasPercent(eSideRight) ||
-      pos->OffsetHasPercent(eSideLeft)) {
+  if ((pos->ISizeDependsOnContainer(aWM) &&
+       pos->ISize(aWM).GetUnit() != eStyleUnit_Auto) ||
+      pos->MaxISizeDependsOnContainer(aWM) ||
+      pos->MinISizeDependsOnContainer(aWM) ||
+      pos->OffsetHasPercent(aWM.IsVertical() ? eSideBottom : eSideRight) ||
+      pos->OffsetHasPercent(aWM.IsVertical() ? eSideTop : eSideLeft)) {
     return true;
   }
 
-  if (eStyleUnit_Auto == pos->mWidth.GetUnit()) {
+  if (eStyleUnit_Auto == pos->ISize(aWM).GetUnit()) {
     // We need to check for frames that shrink-wrap when they're auto
     // width.
     const nsStyleDisplay* disp = aFrame->StyleDisplay();
     if (disp->mDisplay == StyleDisplay::InlineBlock ||
         disp->mDisplay == StyleDisplay::InlineTable ||
         fType == LayoutFrameType::HTMLButtonControl ||
         fType == LayoutFrameType::GfxButtonControl ||
         fType == LayoutFrameType::FieldSet ||
@@ -782,17 +782,17 @@ IsPercentageAware(const nsIFrame* aFrame
     //   the element has an intrinsic ratio but no intrinsic height or
     //   width and the containing block's width does not itself depend
     //   on the replaced element's width, then the used value of 'width'
     //   is calculated from the constraint equation used for
     //   block-level, non-replaced elements in normal flow.
     nsIFrame *f = const_cast<nsIFrame*>(aFrame);
     if (f->GetIntrinsicRatio() != nsSize(0, 0) &&
         // Some percents are treated like 'auto', so check != coord
-        pos->mHeight.GetUnit() != eStyleUnit_Coord) {
+        pos->BSize(aWM).GetUnit() != eStyleUnit_Coord) {
       const IntrinsicSize &intrinsicSize = f->GetIntrinsicSize();
       if (intrinsicSize.width.GetUnit() == eStyleUnit_None &&
           intrinsicSize.height.GetUnit() == eStyleUnit_None) {
         return true;
       }
     }
   }
 
@@ -901,24 +901,24 @@ nsLineLayout::ReflowFrame(nsIFrame* aFra
     // subtract the margin from the available width if necessary.
     // The margin will be applied to the starting inline coordinates of
     // the frame in CanPlaceFrame() after reflowing the frame.
     AllowForStartMargin(pfd, reflowInput);
   }
   // if isText(), no need to propagate NS_FRAME_IS_DIRTY from the parent,
   // because reflow doesn't look at the dirty bits on the frame being reflowed.
 
-  // See if this frame depends on the width of its containing block.  If
-  // so, disable resize reflow optimizations for the line.  (Note that,
+  // See if this frame depends on the inline-size of its containing block.
+  // If so, disable resize reflow optimizations for the line.  (Note that,
   // to be conservative, we do this if we *try* to fit a frame on a
   // line, even if we don't succeed.)  (Note also that we can only make
   // this IsPercentageAware check *after* we've constructed our
   // ReflowInput, because that construction may be what forces aFrame
   // to lazily initialize its (possibly-percent-valued) intrinsic size.)
-  if (mGotLineBox && IsPercentageAware(aFrame)) {
+  if (mGotLineBox && IsPercentageAware(aFrame, lineWM)) {
     mLineBox->DisableResizeReflowOptimization();
   }
 
   // Note that we don't bother positioning the frame yet, because we're probably
   // going to end up moving it when we do the block-direction alignment.
 
   // Adjust float manager coordinate system for the frame.
   ReflowOutput reflowOutput(lineWM);
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1767,25 +1767,16 @@ nsStylePosition::CalcDifference(const ns
               nsChangeHint_UpdateParentOverflow;
     } else {
       hint |= nsChangeHint_AllReflowHints;
     }
   }
   return hint;
 }
 
-/* static */ bool
-nsStylePosition::WidthCoordDependsOnContainer(const nsStyleCoord &aCoord)
-{
-  return aCoord.HasPercent() ||
-         (aCoord.GetUnit() == eStyleUnit_Enumerated &&
-          (aCoord.GetIntValue() == NS_STYLE_WIDTH_FIT_CONTENT ||
-           aCoord.GetIntValue() == NS_STYLE_WIDTH_AVAILABLE));
-}
-
 uint8_t
 nsStylePosition::UsedAlignSelf(ComputedStyle* aParent) const
 {
   if (mAlignSelf != NS_STYLE_ALIGN_AUTO) {
     return mAlignSelf;
   }
   if (MOZ_LIKELY(aParent)) {
     auto parentAlignItems = aParent->StylePosition()->mAlignItems;
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -1457,60 +1457,16 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
 
   nsStyleGridLine mGridColumnStart;
   nsStyleGridLine mGridColumnEnd;
   nsStyleGridLine mGridRowStart;
   nsStyleGridLine mGridRowEnd;
   nsStyleCoord    mColumnGap;       // normal, coord, percent, calc
   nsStyleCoord    mRowGap;          // normal, coord, percent, calc
 
-  // FIXME: Logical-coordinate equivalents to these WidthDepends... and
-  // HeightDepends... methods have been introduced (see below); we probably
-  // want to work towards removing the physical methods, and using the logical
-  // ones in all cases.
-
-  bool WidthDependsOnContainer() const
-    {
-      return mWidth.GetUnit() == eStyleUnit_Auto ||
-        WidthCoordDependsOnContainer(mWidth);
-    }
-
-  // NOTE: For a flex item, "min-width:auto" is supposed to behave like
-  // "min-content", which does depend on the container, so you might think we'd
-  // need a special case for "flex item && min-width:auto" here.  However,
-  // we don't actually need that special-case code, because flex items are
-  // explicitly supposed to *ignore* their min-width (i.e. behave like it's 0)
-  // until the flex container explicitly considers it.  So -- since the flex
-  // container doesn't rely on this method, we don't need to worry about
-  // special behavior for flex items' "min-width:auto" values here.
-  bool MinWidthDependsOnContainer() const
-    { return WidthCoordDependsOnContainer(mMinWidth); }
-  bool MaxWidthDependsOnContainer() const
-    { return WidthCoordDependsOnContainer(mMaxWidth); }
-
-  // Note that these functions count 'auto' as depending on the
-  // container since that's the case for absolutely positioned elements.
-  // However, some callers do not care about this case and should check
-  // for it, since it is the most common case.
-  // FIXME: We should probably change the assumption to be the other way
-  // around.
-  // Consider this as part of moving to the logical-coordinate APIs.
-  bool HeightDependsOnContainer() const
-    {
-      return mHeight.GetUnit() == eStyleUnit_Auto || // CSS 2.1, 10.6.4, item (5)
-        HeightCoordDependsOnContainer(mHeight);
-    }
-
-  // NOTE: The comment above MinWidthDependsOnContainer about flex items
-  // applies here, too.
-  bool MinHeightDependsOnContainer() const
-    { return HeightCoordDependsOnContainer(mMinHeight); }
-  bool MaxHeightDependsOnContainer() const
-    { return HeightCoordDependsOnContainer(mMaxHeight); }
-
   bool OffsetHasPercent(mozilla::Side aSide) const
   {
     return mOffset.Get(aSide).HasPercent();
   }
 
   // Logical-coordinate accessors for width and height properties,
   // given a WritingMode value. The definitions of these methods are
   // found in WritingModes.h (after the WritingMode class is fully
@@ -1533,19 +1489,27 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
   inline bool BSizeDependsOnContainer(mozilla::WritingMode aWM) const;
   inline bool MinBSizeDependsOnContainer(mozilla::WritingMode aWM) const;
   inline bool MaxBSizeDependsOnContainer(mozilla::WritingMode aWM) const;
 
   const nsStyleGridTemplate& GridTemplateColumns() const;
   const nsStyleGridTemplate& GridTemplateRows() const;
 
 private:
-  static bool WidthCoordDependsOnContainer(const nsStyleCoord &aCoord);
-  static bool HeightCoordDependsOnContainer(const nsStyleCoord &aCoord)
-    { return aCoord.HasPercent(); }
+  static bool ISizeCoordDependsOnContainer(const nsStyleCoord &aCoord)
+  {
+    return aCoord.HasPercent() ||
+           (aCoord.GetUnit() == eStyleUnit_Enumerated &&
+            (aCoord.GetIntValue() == NS_STYLE_WIDTH_FIT_CONTENT ||
+             aCoord.GetIntValue() == NS_STYLE_WIDTH_AVAILABLE));
+  }
+  static bool BSizeCoordDependsOnContainer(const nsStyleCoord &aCoord)
+  {
+    return aCoord.HasPercent();
+  }
 };
 
 struct nsStyleTextOverflowSide
 {
   nsStyleTextOverflowSide() : mType(NS_STYLE_TEXT_OVERFLOW_CLIP) {}
 
   bool operator==(const nsStyleTextOverflowSide& aOther) const {
     return mType == aOther.mType &&