From 2a28a03ed45a22fe86ecf5457a02e9bc2f5686cd Mon Sep 17 00:00:00 2001 From: Cameron Kaiser Date: Sun, 8 Apr 2018 14:00:43 -0700 Subject: [PATCH] closes #493: fix height for flexbox case M1030952 M1180107 --- layout/base/FramePropertyTable.h | 17 ++ layout/base/nsLayoutUtils.cpp | 61 +++++--- layout/generic/nsFlexContainerFrame.cpp | 198 +++++++++++++++++------- layout/generic/nsIFrame.h | 6 + 4 files changed, 207 insertions(+), 75 deletions(-) diff --git a/layout/base/FramePropertyTable.h b/layout/base/FramePropertyTable.h index 3b4176579..188f5304f 100644 --- a/layout/base/FramePropertyTable.h +++ b/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; diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp index 6f3a3b6f4..1e34a0885 100644 --- a/layout/base/nsLayoutUtils.cpp +++ b/layout/base/nsLayoutUtils.cpp @@ -5165,33 +5165,58 @@ nsLayoutUtils::ComputeSizeWithIntrinsicDimensions(WritingMode aWM, bool isFlexItem = aFrame->IsFlexItem(); bool isInlineFlexItem = false; + Maybe 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 - // inlineStyleCoord and blockStyleCoord in nsFrame::ComputeSize(). - const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis); - if (flexBasis->GetUnit() != eStyleUnit_Auto) { + // If FlexItemMainSizeOverride frame-property is set, then that means the + // flex container is imposing a main-size on this flex item for it to use + // as its size in the container's main axis. + 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 - // 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; + 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(). + 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; + } } } } diff --git a/layout/generic/nsFlexContainerFrame.cpp b/layout/generic/nsFlexContainerFrame.cpp index 1773dc7c7..77a485427 100644 --- a/layout/generic/nsFlexContainerFrame.cpp +++ b/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 - // !aAxisTracker.IsCrossAxisHorizontal() check that guards this - // call in the caller. BUT, when we add support for vertical writing-modes, - // (in bug 1079155 or a dependency), we'll relax that check, and we'll need - // to be able to measure the baseline & width (given our resolved height) + MOZ_ASSERT(aItem.HasIntrinsicRatio(), + "For now, caller's CanMainSizeInfluenceCrossSize check should " + "only allow us to get here for items with intrinsic ratio"); + // XXXdholbert When we finish support for vertical writing-modes, + // (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 " - "of calling SizeItemInCrossAxis"); - // (But if we do happen to get here, just trust the passed-in reflow state - // for our cross size [width].) + // For now, we only expect to get here for items with an intrinsic aspect + // ratio; and for those items, we can just read the size off of the reflow + // state, without performing reflow. 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 - // in GenerateFlexItemForChild(), and we also may resolve it early for - // stretched flex items.) - // - // We can skip measuring an item's cross size here in a few scenarios: - // (A) If the flex item has already been stretched, then we're imposing - // the container's cross size on it; no need to measure. - // (B) If the flex item is a "strut", then it's just a placeholder with a - // predetermined cross size; no need to measure. - // (C) If the item's main-size can't affect its cross-size, then the - // item's tentative cross size (which we got from the reflow state in - // GenerateFlexItemForChild()) is correct. So, no need to re-measure. - // (For now, this is equivalent to checking if the cross-axis is - // 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 + // The item may already have the correct cross-size; only recalculate + // if the item's main size resolution (flexing) could have influenced it: + if (item->CanMainSizeInfluenceCrossSize(aAxisTracker)) { + Maybe sizeOverride; + if (item->HasIntrinsicRatio()) { + // For flex items with an aspect ratio, we have to impose an override + // for the main-size property *before* we even instantiate the reflow + // state, in order for aspect ratio calculations to produce the right + // cross size in the reflow state. (For other flex items, it's OK + // (and cheaper) to impose our main size *after* the reflow state has + // been constructed, since the main size shouldn't influence anything + // about cross-size measurement until we actually reflow the child.) + sizeOverride.emplace(*item); + } + 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 (aAxisTracker.IsMainAxisHorizontal()) { - childReflowState.SetComputedWidth(item->GetMainSize()); - } else { - // XXXdholbert NOTE: For now, we'll never hit this case, due to the - // !aAxisTracker.IsCrossAxisHorizontal() check above. But - // when we add support for vertical writing modes, we'll relax that - // check and be able to hit this code. - childReflowState.SetComputedHeight(item->GetMainSize()); + if (!sizeOverride) { + // Directly override the computed main-size, by tweaking reflow state: + if (aAxisTracker.IsMainAxisHorizontal()) { + childReflowState.SetComputedWidth(item->GetMainSize()); + } else { + 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. - if (aItem.IsStretched()) { - MOZ_ASSERT(aItem.GetAlignSelf() == NS_STYLE_ALIGN_STRETCH, - "stretched item w/o 'align-self: stretch'?"); + // Override reflow state's computed cross-size if either: + // - the item was stretched (in which case we're imposing a cross size) + // ...or... + // - 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 diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h index d8c1e8c3b..e1d756850 100644 --- a/layout/generic/nsIFrame.h +++ b/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) NS_DECLARE_FRAME_PROPERTY(CachedBackgroundImageDT, ReleaseValue)