closes #493: fix height for flexbox case M1030952 M1180107

layout/base/FramePropertyTable.h

@@ -123,6 +123,16 @@ public:
    */
   void DeleteAll();
 
+  /**
+   * Check if a property exists (added for TenFourFox issue 493).
+   */
+  bool Has(const nsIFrame* aFrame, const FramePropertyDescriptor* aProperty)
+  {
+    bool foundResult = false;
+    (void)Get(aFrame, aProperty, &foundResult);
+    return foundResult;
+  }
+
   size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
 
 protected:
@@ -238,6 +248,13 @@ public:
   {
     mTable->Delete(mFrame, aProperty);
   }
+  // TenFourFox issue 493
+  bool Has(const FramePropertyDescriptor* aProperty)
+  {
+    bool foundResult;
+    (void)mTable->Get(mFrame, aProperty, &foundResult);
+    return foundResult;
+  }
 
 private:
   FramePropertyTable* mTable;

layout/base/nsLayoutUtils.cpp

@@ -5165,33 +5165,58 @@ nsLayoutUtils::ComputeSizeWithIntrinsicDimensions(WritingMode aWM,
   bool isFlexItem = aFrame->IsFlexItem();
   bool isInlineFlexItem = false;
 
+  Maybe<nsStyleCoord> imposedMainSizeStyleCoord;
+
+  // If this is a flex item, and we're measuring its cross size after flexing
+  // to resolve its main size, then we need to use the resolved main size
+  // that the container provides to us *instead of* the main-size coordinate
+  // from our style struct. (Otherwise, we'll be using an irrelevant value in
+  // the aspect-ratio calculations below.)
   if (isFlexItem) {
-    // 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.
     uint32_t flexDirection =
       aFrame->GetParent()->StylePosition()->mFlexDirection;
     isInlineFlexItem =
       flexDirection == NS_STYLE_FLEX_DIRECTION_ROW ||
       flexDirection == NS_STYLE_FLEX_DIRECTION_ROW_REVERSE;
 
-    // NOTE: The logic here should match the similar chunk for determining
+    // If FlexItemMainSizeOverride frame-property is set, then that means the
-    // inlineStyleCoord and blockStyleCoord in nsFrame::ComputeSize().
+    // flex container is imposing a main-size on this flex item for it to use
-    const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
+    // as its size in the container's main axis.
-    if (flexBasis->GetUnit() != eStyleUnit_Auto) {
+    FrameProperties props = aFrame->Properties();
+    bool didImposeMainSize;
+    nscoord imposedMainSize =
+      props.Get(nsIFrame::FlexItemMainSizeOverride(), &didImposeMainSize);
+    if (didImposeMainSize) {
+      imposedMainSizeStyleCoord.emplace(imposedMainSize,
+                                        nsStyleCoord::CoordConstructor);
       if (isInlineFlexItem) {
-        inlineStyleCoord = flexBasis;
+        inlineStyleCoord = imposedMainSizeStyleCoord.ptr();
       } else {
-        // One caveat for vertical flex items: We don't support enumerated
+        blockStyleCoord = imposedMainSizeStyleCoord.ptr();
-        // values (e.g. "max-content") for height properties yet. 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"
+    } else {
-        // (which is "height", in this case).
+      // Flex items use their "flex-basis" property in place of their main-size
-        // NOTE: Once we support intrinsic sizing keywords for "height",
+      // property (e.g. "width") for sizing purposes, *unless* they have
-        // we should remove this check.
+      // "flex-basis:auto", in which case they use their main-size property
-        if (flexBasis->GetUnit() != eStyleUnit_Enumerated) {
+      // after all.
-          blockStyleCoord = flexBasis;
+      // 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) {
+        if (isInlineFlexItem) {
+          inlineStyleCoord = flexBasis;
+        } else {
+          // One caveat for vertical flex items: We don't support enumerated
+          // values (e.g. "max-content") for height properties yet. 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.
+          if (flexBasis->GetUnit() != eStyleUnit_Enumerated) {
+            blockStyleCoord = flexBasis;
+          }
         }
       }
     }

layout/generic/nsFlexContainerFrame.cpp

@@ -462,6 +462,9 @@ public:
     return mFlexShrink * mFlexBaseSize;
   }
 
+  const nsSize& IntrinsicRatio() const { return mIntrinsicRatio; }
+  bool HasIntrinsicRatio() const { return mIntrinsicRatio != nsSize(); }
+
   // Getters for margin:
   // ===================
   const nsMargin& GetMargin() const { return mMargin; }
@@ -642,6 +645,10 @@ public:
 
   uint32_t GetNumAutoMarginsInAxis(AxisOrientationType aAxis) const;
 
+  // Once the main size has been resolved, should we bother doing layout to
+  // establish the cross size?
+  bool CanMainSizeInfluenceCrossSize(const FlexboxAxisTracker& aAxisTracker) const;
+
 protected:
   // Helper called by the constructor, to set mNeedsMinSizeAutoResolution:
   void CheckForMinSizeAuto(const nsHTMLReflowState& aFlexItemReflowState,
@@ -654,6 +661,10 @@ protected:
   const float mFlexGrow;
   const float mFlexShrink;
 
+public: // bustage kludge for bug 1030952 (see TenFourFox issue 493)
+  const nsSize mIntrinsicRatio;
+protected:
+
   const nsMargin mBorderPadding;
   nsMargin mMargin; // non-const because we need to resolve auto margins
 
@@ -1258,7 +1269,6 @@ MainSizeFromAspectRatio(nscoord aCrossSize,
 static nscoord
 PartiallyResolveAutoMinSize(const FlexItem& aFlexItem,
                             const nsHTMLReflowState& aItemReflowState,
-                            const nsSize& aIntrinsicRatio,
                             const FlexboxAxisTracker& aAxisTracker)
 {
   MOZ_ASSERT(aFlexItem.NeedsMinSizeAutoResolution(),
@@ -1295,7 +1305,7 @@ PartiallyResolveAutoMinSize(const FlexItem& aFlexItem,
   // * if the item has an intrinsic aspect ratio, the width (height) calculated
   //   from the aspect ratio and any definite size constraints in the opposite
   //   dimension.
-  if (aAxisTracker.GetCrossComponent(aIntrinsicRatio) != 0) {
+  if (aAxisTracker.GetCrossComponent(aFlexItem.mIntrinsicRatio) != 0) {
     // We have a usable aspect ratio. (not going to divide by 0)
     const bool useMinSizeIfCrossSizeIsIndefinite = true;
     nscoord crossSizeToUseWithRatio =
@@ -1304,7 +1314,7 @@ PartiallyResolveAutoMinSize(const FlexItem& aFlexItem,
                               aAxisTracker);
     nscoord minMainSizeFromRatio =
       MainSizeFromAspectRatio(crossSizeToUseWithRatio,
-                              aIntrinsicRatio, aAxisTracker);
+                              aFlexItem.mIntrinsicRatio, aAxisTracker);
     minMainSize = std::min(minMainSize, minMainSizeFromRatio);
   }
 
@@ -1318,7 +1328,6 @@ PartiallyResolveAutoMinSize(const FlexItem& aFlexItem,
 static bool
 ResolveAutoFlexBasisFromRatio(FlexItem& aFlexItem,
                               const nsHTMLReflowState& aItemReflowState,
-                              const nsSize& aIntrinsicRatio,
                               const FlexboxAxisTracker& aAxisTracker)
 {
   MOZ_ASSERT(NS_AUTOHEIGHT == aFlexItem.GetFlexBaseSize(),
@@ -1329,7 +1338,7 @@ ResolveAutoFlexBasisFromRatio(FlexItem& aFlexItem,
   //  - a definite cross size
   // then the flex base size is calculated from its inner cross size and the
   // flex item’s intrinsic aspect ratio.
-  if (aAxisTracker.GetCrossComponent(aIntrinsicRatio) != 0) {
+  if (aAxisTracker.GetCrossComponent(aFlexItem.mIntrinsicRatio) != 0) {
     // We have a usable aspect ratio. (not going to divide by 0)
     const bool useMinSizeIfCrossSizeIsIndefinite = false;
     nscoord crossSizeToUseWithRatio =
@@ -1340,7 +1349,7 @@ ResolveAutoFlexBasisFromRatio(FlexItem& aFlexItem,
       // We have a definite cross-size
       nscoord mainSizeFromRatio =
         MainSizeFromAspectRatio(crossSizeToUseWithRatio,
-                                aIntrinsicRatio, aAxisTracker);
+                                aFlexItem.mIntrinsicRatio, aAxisTracker);
       aFlexItem.SetFlexBaseSizeAndMainSize(mainSizeFromRatio);
       return true;
     }
@@ -1398,19 +1407,15 @@ nsFlexContainerFrame::
     }
   }
 
-  // We'll need the intrinsic ratio (if there is one), regardless of whether
-  // we're resolving min-[width|height]:auto or flex-basis:auto.
-  const nsSize ratio = aFlexItem.Frame()->GetIntrinsicRatio();
-
   nscoord resolvedMinSize; // (only set/used if isMainMinSizeAuto==true)
   bool minSizeNeedsToMeasureContent = false; // assume the best
   if (isMainMinSizeAuto) {
     // Resolve the min-size, except for considering the min-content size.
     // (We'll consider that later, if we need to.)
     resolvedMinSize = PartiallyResolveAutoMinSize(aFlexItem, aItemReflowState,
-                                                  ratio, aAxisTracker);
+                                                  aAxisTracker);
     if (resolvedMinSize > 0 &&
-        aAxisTracker.GetCrossComponent(ratio) == 0) {
+        aAxisTracker.GetCrossComponent(aFlexItem.IntrinsicRatio()) == 0) {
       // We don't have a usable aspect ratio, so we need to consider our
       // min-content size as another candidate min-size, which we'll have to
       // min() with the current resolvedMinSize.
@@ -1423,7 +1428,7 @@ nsFlexContainerFrame::
   bool flexBasisNeedsToMeasureContent = false; // assume the best
   if (isMainSizeAuto) {
     if (!ResolveAutoFlexBasisFromRatio(aFlexItem, aItemReflowState,
-                                       ratio, aAxisTracker)) {
+                                       aAxisTracker)) {
       flexBasisNeedsToMeasureContent = true;
     }
   }
@@ -1543,6 +1548,7 @@ FlexItem::FlexItem(nsHTMLReflowState& aFlexItemReflowState,
   : mFrame(aFlexItemReflowState.frame),
     mFlexGrow(aFlexGrow),
     mFlexShrink(aFlexShrink),
+    mIntrinsicRatio(mFrame->GetIntrinsicRatio()),
     mBorderPadding(aFlexItemReflowState.ComputedPhysicalBorderPadding()),
     mMargin(aFlexItemReflowState.ComputedPhysicalMargin()),
     mMainMinSize(aMainMinSize),
@@ -1624,6 +1630,7 @@ FlexItem::FlexItem(nsIFrame* aChildFrame, nscoord aCrossSize,
   : mFrame(aChildFrame),
     mFlexGrow(0.0f),
     mFlexShrink(0.0f),
+    mIntrinsicRatio(),
     // mBorderPadding uses default constructor,
     // mMargin uses default constructor,
     mFlexBaseSize(0),
@@ -1736,6 +1743,45 @@ FlexItem::GetNumAutoMarginsInAxis(AxisOrientationType aAxis) const
   return numAutoMargins;
 }
 
+bool
+FlexItem::CanMainSizeInfluenceCrossSize(
+  const FlexboxAxisTracker& aAxisTracker) const
+{
+  if (mIsStretched) {
+    // We've already had our cross-size stretched for "align-self:stretch").
+    // The container is imposing its cross size on us.
+    return false;
+  }
+
+  if (HasIntrinsicRatio()) {
+    // For flex items that have an intrinsic ratio (and maintain it, i.e. are
+    // not stretched, which we already checked above): changes to main-size
+    // *do* influence the cross size.
+    return true;
+  }
+
+  if (mIsStrut) {
+    // Struts (for visibility:collapse items) have a predetermined size;
+    // no need to measure anything.
+    return false;
+  }
+
+  if (aAxisTracker.IsCrossAxisHorizontal()) {
+    // If the cross axis is horizontal, then changes to the item's main size
+    // (height) can't influence its cross size (width), if the item is a block
+    // with a horizontal writing-mode.
+    // XXXdholbert This doesn't account for vertical writing-modes, items with
+    // aspect ratios, items that are multicol elements, & items that are
+    // multi-line vertical flex containers. In all of those cases, a change to
+    // the height could influence the width.
+    return false;
+  }
+
+  // Default assumption, if we haven't proven otherwise: the resolved main size
+  // *can* change the cross size.
+  return true;
+}
+
 // Keeps track of our position along a particular axis (where a '0' position
 // corresponds to the 'start' edge of that axis).
 // This class shouldn't be instantiated directly -- rather, it should only be
@@ -3488,16 +3534,17 @@ nsFlexContainerFrame::SizeItemInCrossAxis(
   FlexItem& aItem)
 {
   if (aAxisTracker.IsCrossAxisHorizontal()) {
-    // XXXdholbert NOTE: For now, we should never hit this case, due to a
+    MOZ_ASSERT(aItem.HasIntrinsicRatio(),
-    // !aAxisTracker.IsCrossAxisHorizontal() check that guards this
+               "For now, caller's CanMainSizeInfluenceCrossSize check should "
-    // call in the caller. BUT, when we add support for vertical writing-modes,
+               "only allow us to get here for items with intrinsic ratio");
-    // (in bug 1079155 or a dependency), we'll relax that check, and we'll need
+    // XXXdholbert When we finish support for vertical writing-modes,
-    // to be able to measure the baseline & width (given our resolved height)
+    // (in bug 1079155 or a dependency), we'll relax the horizontal check in
+    // CanMainSizeInfluenceCrossSize, and this function will need to be able
+    // to measure the baseline & width (given our resolved height)
     // of vertical-writing-mode flex items here.
-    MOZ_ASSERT_UNREACHABLE("Caller should use tentative cross size instead "
+    // For now, we only expect to get here for items with an intrinsic aspect
-                           "of calling SizeItemInCrossAxis");
+    // ratio; and for those items, we can just read the size off of the reflow
-    // (But if we do happen to get here, just trust the passed-in reflow state
+    // state, without performing reflow.
-    // for our cross size [width].)
     aItem.SetCrossSize(aChildReflowState.ComputedWidth());
     return;
   }
@@ -3709,6 +3756,40 @@ private:
   MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
 };
 
+// Class to let us temporarily provide an override value for the the main-size
+// CSS property ('width' or 'height') on a flex item, for use in
+// nsLayoutUtils::ComputeSizeWithIntrinsicDimensions.
+// (We could use this overridden size more broadly, too, but it's probably
+// better to avoid property-table accesses.  So, where possible, we communicate
+// the resolved main-size to the child via modifying its reflow state directly,
+// instead of using this class.)
+class MOZ_RAII AutoFlexItemMainSizeOverride final
+{
+public:
+  explicit AutoFlexItemMainSizeOverride(FlexItem& aItem
+                                        MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
+    : mItemProps(aItem.Frame()->Properties())
+  {
+    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
+
+    MOZ_ASSERT(!mItemProps.Has(nsIFrame::FlexItemMainSizeOverride()),
+               "FlexItemMainSizeOverride prop shouldn't be set already; "
+               "it should only be set temporarily (& not recursively)");
+    NS_ASSERTION(aItem.HasIntrinsicRatio(),
+                 "This should only be needed for items with an aspect ratio");
+
+    mItemProps.Set(nsIFrame::FlexItemMainSizeOverride(), aItem.GetMainSize());
+  }
+
+  ~AutoFlexItemMainSizeOverride() {
+    mItemProps.Remove(nsIFrame::FlexItemMainSizeOverride());
+  }
+
+private:
+  const FrameProperties mItemProps;
+  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
+};
+
 void
 nsFlexContainerFrame::DoFlexLayout(nsPresContext*           aPresContext,
                                    nsHTMLReflowMetrics&     aDesiredSize,
@@ -3744,41 +3825,35 @@ nsFlexContainerFrame::DoFlexLayout(nsPresContext*           aPresContext,
   nscoord sumLineCrossSizes = 0;
   for (FlexLine* line = lines.getFirst(); line; line = line->getNext()) {
     for (FlexItem* item = line->GetFirstItem(); item; item = item->getNext()) {
-      // Note that we may already have the correct cross size. (We guess at it
+      // The item may already have the correct cross-size; only recalculate
-      // in GenerateFlexItemForChild(), and we also may resolve it early for
+      // if the item's main size resolution (flexing) could have influenced it:
-      // stretched flex items.)
+      if (item->CanMainSizeInfluenceCrossSize(aAxisTracker)) {
-      //
+        Maybe<AutoFlexItemMainSizeOverride> sizeOverride;
-      // We can skip measuring an item's cross size here in a few scenarios:
+        if (item->HasIntrinsicRatio()) {
-      // (A) If the flex item has already been stretched, then we're imposing
+          // For flex items with an aspect ratio, we have to impose an override
-      //     the container's cross size on it; no need to measure.
+          // for the main-size property *before* we even instantiate the reflow
-      // (B) If the flex item is a "strut", then it's just a placeholder with a
+          // state, in order for aspect ratio calculations to produce the right
-      //     predetermined cross size; no need to measure.
+          // cross size in the reflow state. (For other flex items, it's OK
-      // (C) If the item's main-size can't affect its cross-size, then the
+          // (and cheaper) to impose our main size *after* the reflow state has
-      //     item's tentative cross size (which we got from the reflow state in
+          // been constructed, since the main size shouldn't influence anything
-      //     GenerateFlexItemForChild()) is correct. So, no need to re-measure.
+          // about cross-size measurement until we actually reflow the child.)
-      //     (For now, this is equivalent to checking if the cross-axis is
+          sizeOverride.emplace(*item);
-      //     horizontal, because until we enable vertical writing-modes, an
+        }
-      //     element's computed width can't be influenced by its computed
+
-      //     height.)
-      if (!item->IsStretched() && // !A
-          !item->IsStrut() &&     // !B
-          !aAxisTracker.IsCrossAxisHorizontal()) { // !C
         WritingMode wm = item->Frame()->GetWritingMode();
         LogicalSize availSize = aReflowState.ComputedSize(wm);
         availSize.BSize(wm) = NS_UNCONSTRAINEDSIZE;
         nsHTMLReflowState childReflowState(aPresContext, aReflowState,
                                            item->Frame(), availSize);
-        // Override computed main-size
+        if (!sizeOverride) {
-        if (aAxisTracker.IsMainAxisHorizontal()) {
+          // Directly override the computed main-size, by tweaking reflow state:
-          childReflowState.SetComputedWidth(item->GetMainSize());
+          if (aAxisTracker.IsMainAxisHorizontal()) {
-        } else {
+            childReflowState.SetComputedWidth(item->GetMainSize());
-          // XXXdholbert NOTE: For now, we'll never hit this case, due to the
+          } else {
-          // !aAxisTracker.IsCrossAxisHorizontal() check above. But
+            childReflowState.SetComputedHeight(item->GetMainSize());
-          // when we add support for vertical writing modes, we'll relax that
+          }
-          // check and be able to hit this code.
-          childReflowState.SetComputedHeight(item->GetMainSize());
         }
-        
+
         SizeItemInCrossAxis(aPresContext, aAxisTracker,
                             childReflowState, *item);
       }
@@ -4076,20 +4151,29 @@ nsFlexContainerFrame::ReflowFlexItem(nsPresContext* aPresContext,
     didOverrideComputedHeight = true;
   }
 
-  // Override reflow state's computed cross-size, for stretched items.
+  // Override reflow state's computed cross-size if either:
-  if (aItem.IsStretched()) {
+  // - the item was stretched (in which case we're imposing a cross size)
-    MOZ_ASSERT(aItem.GetAlignSelf() == NS_STYLE_ALIGN_STRETCH,
+  // ...or...
-               "stretched item w/o 'align-self: stretch'?");
+  // - the item it has an aspect ratio (in which case the cross-size that's
+  // currently in the reflow state is based on arithmetic involving a stale
+  // main-size value that we just stomped on above). (Note that we could handle
+  // this case using an AutoFlexItemMainSizeOverride, as we do elsewhere; but
+  // given that we *already know* the correct cross size to use here, it's
+  // cheaper to just directly set it instead of setting a frame property.)
+  if (aItem.IsStretched() ||
+      aItem.HasIntrinsicRatio()) {
     if (aAxisTracker.IsCrossAxisHorizontal()) {
       childReflowState.SetComputedWidth(aItem.GetCrossSize());
       didOverrideComputedWidth = true;
     } else {
-      // If this item's height is stretched, it's a relative height.
-      aItem.Frame()->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);
       childReflowState.SetComputedHeight(aItem.GetCrossSize());
       didOverrideComputedHeight = true;
     }
   }
+  if (aItem.IsStretched() && !aAxisTracker.IsCrossAxisHorizontal()) {
+    // If this item's height is stretched, it's a relative height.
+    aItem.Frame()->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);
+  }
 
   // XXXdholbert Might need to actually set the correct margins in the
   // reflow state at some point, so that they can be saved on the frame for

layout/generic/nsIFrame.h

@@ -893,6 +893,12 @@ public:
 
   NS_DECLARE_FRAME_PROPERTY(LineBaselineOffset, nullptr)
 
+  // Temporary override for a flex item's main-size property (either width
+  // or height), imposed by its flex container.
+  // XXX: We don't have bug 1064843, so use the previous declaration system
+  // (see bug 1030952 part 3 and TenFourFox issue 493).
+  NS_DECLARE_FRAME_PROPERTY(FlexItemMainSizeOverride, nullptr)
+
   NS_DECLARE_FRAME_PROPERTY(CachedBackgroundImage, ReleaseValue<gfxASurface>)
   NS_DECLARE_FRAME_PROPERTY(CachedBackgroundImageDT,
                             ReleaseValue<mozilla::gfx::DrawTarget>)