Bug 1590247 - Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 14 Oct 2021 21:19:14 +0000
changeset 595986 cb21a032e4ad3b8db96f38cef47b13837fb455e4
parent 595985 1492d117aca49c68e1c6f7791418c93519212137
child 595987 16504649f4893ace8dcc78da4900e546f610fe1f
push id151533
push userealvarez@mozilla.com
push dateThu, 14 Oct 2021 21:21:48 +0000
treeherderautoland@16504649f489 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1590247
milestone95.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1590247 - Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert This prevents jank when switching from overflow: auto -> hidden or such. Differential Revision: https://phabricator.services.mozilla.com/D128367
dom/events/test/window_wheel_default_action.html
layout/base/RestyleManager.cpp
layout/generic/nsGfxScrollFrame.cpp
layout/generic/nsGfxScrollFrame.h
layout/generic/nsIScrollableFrame.h
layout/style/nsStyleStruct.cpp
layout/style/test/test_dynamic_change_causing_reflow.html
--- a/dom/events/test/window_wheel_default_action.html
+++ b/dom/events/test/window_wheel_default_action.html
@@ -86,16 +86,23 @@ const kDefaultActionZoom                
 const kDefaultActionHorizontalizedScroll = 4;
 
 const kDefaultActionOverrideXNoOverride = -1;
 const kDefaultActionOverrideXNone       = kDefaultActionNone;
 const kDefaultActionOverrideXScroll     = kDefaultActionScroll;
 const kDefaultActionOverrideXHistory    = kDefaultActionHistory;
 const kDefaultActionOverrideXZoom       = kDefaultActionZoom;
 
+// We sometimes rely on the scroll position not getting preserved across tests.
+function resetScrollPosition(element) {
+  element.style.display = "none";
+  element.offsetTop;
+  element.style.display = "";
+}
+
 function is()
 {
   window.opener.is.apply(window.opener, arguments);
 }
 
 function ok()
 {
   window.opener.ok.apply(window.opener, arguments);
@@ -1868,16 +1875,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: true,
       expected: kAdjustedForDown.result,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "auto";
                  gScrollableElement.style.overflowY = "hidden";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to " + kAdjustedForDown.desc +
                    "(originally bottom) by pixel scroll when lineOrPageDelta is 1, " +
                    "no vertical scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_PIXEL,
                deltaX: 0.0, deltaY: 8.0, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
@@ -1956,16 +1964,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: false,
       expected: kScrollDown,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "hidden";
                  gScrollableElement.style.overflowY = "auto";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to bottom by pixel scroll when lineOrPageDelta is 1, " +
                    "no horizontal scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_PIXEL,
                deltaX: 0.0, deltaY: 8.0, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
@@ -2046,16 +2055,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: true,
       expected: kAdjustedForDown.result,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "auto";
                  gScrollableElement.style.overflowY = "hidden";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to " + kAdjustedForDown.desc +
                    "(originally bottom) by line scroll when lineOrPageDelta is 1, " +
                    "no vertical scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_LINE,
                deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
@@ -2134,16 +2144,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: false,
       expected: kScrollDown,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "hidden";
                  gScrollableElement.style.overflowY = "auto";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to bottom by line scroll when lineOrPageDelta is 1, " +
                    "no horizontal scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_LINE,
                deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
@@ -2224,16 +2235,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: true,
       expected: kAdjustedForDown.result,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "auto";
                  gScrollableElement.style.overflowY = "hidden";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to " + kAdjustedForDown.desc +
                    "(originally bottom) by page scroll even if lineOrPageDelta is 0, " +
                    "no vertical scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_PAGE,
                deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
@@ -2312,16 +2324,17 @@ function doTestAutoDirScroll2(aSettings,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
                shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false },
       adjusted: false,
       expected: kScrollDown,
       prepare (cb) {
                  gScrollableElement.style.overflowX = "hidden";
                  gScrollableElement.style.overflowY = "auto";
+                 resetScrollPosition(gScrollableElement);
                  cb();
                } },
     { description: "auto-dir scroll to bottom by page scroll even if lineOrPageDelta is 0, " +
                    "no horizontal scrollbar",
       event: { deltaMode: WheelEvent.DOM_DELTA_PAGE,
                deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0,
                lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false,
                expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0,
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -32,16 +32,17 @@
 #include "mozilla/dom/ChildIterator.h"
 #include "mozilla/dom/DocumentInlines.h"
 #include "mozilla/dom/ElementInlines.h"
 #include "mozilla/dom/HTMLBodyElement.h"
 
 #include "Layers.h"
 #include "nsAnimationManager.h"
 #include "nsBlockFrame.h"
+#include "nsIScrollableFrame.h"
 #include "nsContentUtils.h"
 #include "nsCSSFrameConstructor.h"
 #include "nsCSSRendering.h"
 #include "nsDocShell.h"
 #include "nsIFrame.h"
 #include "nsIFrameInlines.h"
 #include "nsImageFrame.h"
 #include "nsPlaceholderFrame.h"
@@ -1257,17 +1258,16 @@ static inline bool CanSkipOverflowUpdate
 }
 
 static inline void MaybeDealWithScrollbarChange(nsStyleChangeData& aData,
                                                 nsPresContext* aPc) {
   if (!(aData.mHint & nsChangeHint_ScrollbarChange)) {
     return;
   }
   aData.mHint &= ~nsChangeHint_ScrollbarChange;
-  bool doReconstruct = true;  // assume the worst
 
   // Only bother with this if we're html/body, since:
   //  (a) It'd be *expensive* to reframe these particular nodes.  They're
   //      at the root, so reframing would mean rebuilding the world.
   //  (b) It's often *unnecessary* to reframe for "overflow" changes on
   //      these particular nodes.  In general, the only reason we reframe
   //      for "overflow" changes is so we can construct (or destroy) a
   //      scrollframe & scrollbars -- and the html/body nodes often don't
@@ -1296,24 +1296,34 @@ static inline void MaybeDealWithScrollba
         // scrollbar-styles provider. (If it were, we'd potentially need to
         // reframe to create a dedicated scrollframe for whichever element
         // is being booted from providing viewport scrollbar styles.)
         //
         // Under these conditions, we're OK to assume that this "overflow"
         // change only impacts the root viewport's scrollframe, which
         // already exists, so we can simply reflow instead of reframing.
         aData.mHint |= nsChangeHint_ReflowHintsForScrollbarChange;
-        doReconstruct = false;
+        return;
       }
     }
   }
 
-  if (doReconstruct) {
-    aData.mHint |= nsChangeHint_ReconstructFrame;
+  if (nsIScrollableFrame* sf = do_QueryFrame(aData.mFrame)) {
+    if (aData.mFrame->StyleDisplay()->IsScrollableOverflow() &&
+        sf->HasAllNeededScrollbars()) {
+      // Once we've created scrollbars for a frame, don't bother reconstructing
+      // it just to remove them if we still need a scroll frame.
+      aData.mHint |= nsChangeHint_ReflowHintsForScrollbarChange;
+      return;
+    }
   }
+
+  // Oh well, we couldn't optimize it out, just reconstruct frames for the
+  // subtree.
+  aData.mHint |= nsChangeHint_ReconstructFrame;
 }
 
 void RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) {
   NS_ASSERTION(!nsContentUtils::IsSafeToRunScript(),
                "Someone forgot a script blocker");
 
   // See bug 1378219 comment 9:
   // Recursive calls here are a bit worrying, but apparently do happen in the
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -1246,21 +1246,21 @@ void nsHTMLScrollFrame::Reflow(nsPresCon
   DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
   MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");
 
   mHelper.HandleScrollbarStyleSwitching();
 
   ScrollReflowInput state(this, aReflowInput);
   // sanity check: ensure that if we have no scrollbar, we treat it
   // as hidden.
-  if (!mHelper.mVScrollbarBox || mHelper.mNeverHasVerticalScrollbar) {
+  if (!mHelper.mVScrollbarBox) {
     state.mVScrollbarAllowedForScrollingVVInsideLV = false;
     state.mVScrollbar = ShowScrollbar::Never;
   }
-  if (!mHelper.mHScrollbarBox || mHelper.mNeverHasHorizontalScrollbar) {
+  if (!mHelper.mHScrollbarBox) {
     state.mHScrollbarAllowedForScrollingVVInsideLV = false;
     state.mHScrollbar = ShowScrollbar::Never;
   }
 
   //------------ Handle Incremental Reflow -----------------
   bool reflowHScrollbar = true;
   bool reflowVScrollbar = true;
   bool reflowScrollCorner = true;
@@ -2205,18 +2205,16 @@ ScrollFrameHelper::ScrollFrameHelper(nsC
       mLastPos(-1, -1),
       mApzScrollPos(0, 0),
       mLastUpdateFramesPos(-1, -1),
       mDisplayPortAtLastFrameUpdate(),
       mScrollParentID(mozilla::layers::ScrollableLayerGuid::NULL_SCROLL_ID),
       mAnchor(this),
       mAllowScrollOriginDowngrade(false),
       mHadDisplayPortAtLastFrameUpdate(false),
-      mNeverHasVerticalScrollbar(false),
-      mNeverHasHorizontalScrollbar(false),
       mHasVerticalScrollbar(false),
       mHasHorizontalScrollbar(false),
       mOnlyNeedVScrollbarToScrollVVInsideLV(false),
       mOnlyNeedHScrollbarToScrollVVInsideLV(false),
       mFrameIsUpdatingScrollbar(false),
       mDidHistoryRestore(false),
       mIsRoot(aIsRoot),
       mSuppressScrollbarUpdate(false),
@@ -5340,113 +5338,134 @@ bool ScrollFrameHelper::IsForTextControl
   auto* content = mOuter->GetContent();
   if (!content) {
     return false;
   }
   auto* input = content->GetClosestNativeAnonymousSubtreeRootParent();
   return input && input->IsHTMLElement(nsGkAtoms::input);
 }
 
-nsresult ScrollFrameHelper::CreateAnonymousContent(
-    nsTArray<nsIAnonymousContentCreator::ContentInfo>& aElements) {
-  typedef nsIAnonymousContentCreator::ContentInfo ContentInfo;
-
-  nsPresContext* presContext = mOuter->PresContext();
+auto ScrollFrameHelper::GetCurrentAnonymousContent() const
+    -> EnumSet<AnonymousContentType> {
+  EnumSet<AnonymousContentType> result;
+  if (mHScrollbarContent) {
+    result += AnonymousContentType::HorizontalScrollbar;
+  }
+  if (mVScrollbarContent) {
+    result += AnonymousContentType::VerticalScrollbar;
+  }
+  if (mResizerContent) {
+    result += AnonymousContentType::Resizer;
+  }
+  return result;
+}
+
+auto ScrollFrameHelper::GetNeededAnonymousContent() const
+    -> EnumSet<AnonymousContentType> {
+  nsPresContext* pc = mOuter->PresContext();
 
   // Don't create scrollbars if we're an SVG document being used as an image,
   // or if we're printing/print previewing.
   // (In the printing case, we allow scrollbars if this is the child of the
   // viewport & paginated scrolling is enabled, because then we must be the
   // scroll frame for the print preview window, & that does need scrollbars.)
-  if (presContext->Document()->IsBeingUsedAsImage() ||
-      (!presContext->IsDynamic() &&
-       !(mIsRoot && presContext->HasPaginatedScrolling()))) {
-    mNeverHasVerticalScrollbar = mNeverHasHorizontalScrollbar = true;
-    return NS_OK;
+  if (pc->Document()->IsBeingUsedAsImage() ||
+      (!pc->IsDynamic() && !(mIsRoot && pc->HasPaginatedScrolling()))) {
+    return {};
+  }
+
+  if (IsForTextControlWithNoScrollbars()) {
+    return {};
+  }
+
+  EnumSet<AnonymousContentType> result;
+  // If we're the scrollframe for the root, then we want to construct our
+  // scrollbar frames no matter what.  That way later dynamic changes to
+  // propagated overflow styles will show or hide scrollbars on the viewport
+  // without requiring frame reconstruction of the viewport (good!).
+  //
+  // TODO(emilio): Figure out if we can remove this special-case now that we
+  // have more targeted optimizations.
+  if (mIsRoot) {
+    result += AnonymousContentType::HorizontalScrollbar;
+    result += AnonymousContentType::VerticalScrollbar;
+    // If scrollbar-width is none, don't generate scrollbars.
+  } else if (mOuter->StyleUIReset()->mScrollbarWidth !=
+             StyleScrollbarWidth::None) {
+    nsIScrollableFrame* scrollable = do_QueryFrame(mOuter);
+    ScrollStyles styles = scrollable->GetScrollStyles();
+    if (styles.mHorizontal != StyleOverflow::Hidden) {
+      result += AnonymousContentType::HorizontalScrollbar;
+    }
+    if (styles.mVertical != StyleOverflow::Hidden) {
+      result += AnonymousContentType::VerticalScrollbar;
+    }
   }
 
   // Check if the frame is resizable. Note:
   // "The effect of the resize property on generated content is undefined.
   //  Implementations should not apply the resize property to generated
   //  content." [1]
   // For info on what is generated content, see [2].
   // [1]: https://drafts.csswg.org/css-ui/#resize
   // [2]: https://www.w3.org/TR/CSS2/generate.html#content
   auto resizeStyle = mOuter->StyleDisplay()->mResize;
-  bool isResizable = resizeStyle != StyleResize::None &&
-                     !mOuter->HasAnyStateBits(NS_FRAME_GENERATED_CONTENT);
-
-  nsIScrollableFrame* scrollable = do_QueryFrame(mOuter);
-
-  // If we're the scrollframe for the root, then we want to construct
-  // our scrollbar frames no matter what.  That way later dynamic
-  // changes to propagated overflow styles will show or hide
-  // scrollbars on the viewport without requiring frame reconstruction
-  // of the viewport (good!).
-  bool canHaveHorizontal;
-  bool canHaveVertical;
-  if (!mIsRoot) {
-    if (mOuter->StyleUIReset()->mScrollbarWidth == StyleScrollbarWidth::None) {
-      // If scrollbar-width is none, don't generate scrollbars.
-      canHaveHorizontal = false;
-      canHaveVertical = false;
-    } else {
-      ScrollStyles styles = scrollable->GetScrollStyles();
-      canHaveHorizontal = styles.mHorizontal != StyleOverflow::Hidden;
-      canHaveVertical = styles.mVertical != StyleOverflow::Hidden;
-    }
-    if (!canHaveHorizontal && !canHaveVertical && !isResizable) {
-      // Nothing to do.
-      return NS_OK;
-    }
-  } else {
-    canHaveHorizontal = true;
-    canHaveVertical = true;
-  }
-
-  if (IsForTextControlWithNoScrollbars()) {
-    mNeverHasVerticalScrollbar = mNeverHasHorizontalScrollbar = true;
-    return NS_OK;
-  }
-
+  if (resizeStyle != StyleResize::None &&
+      !mOuter->HasAnyStateBits(NS_FRAME_GENERATED_CONTENT)) {
+    result += AnonymousContentType::Resizer;
+  }
+
+  return result;
+}
+
+nsresult ScrollFrameHelper::CreateAnonymousContent(
+    nsTArray<nsIAnonymousContentCreator::ContentInfo>& aElements) {
+  typedef nsIAnonymousContentCreator::ContentInfo ContentInfo;
+
+  nsPresContext* presContext = mOuter->PresContext();
   nsNodeInfoManager* nodeInfoManager =
       presContext->Document()->NodeInfoManager();
 
+  auto neededAnonContent = GetNeededAnonymousContent();
+  if (neededAnonContent.isEmpty()) {
+    return NS_OK;
+  }
+
   {
     RefPtr<NodeInfo> nodeInfo = nodeInfoManager->GetNodeInfo(
         nsGkAtoms::scrollbar, nullptr, kNameSpaceID_XUL, nsINode::ELEMENT_NODE);
     NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
 
-    if (canHaveHorizontal) {
+    if (neededAnonContent.contains(AnonymousContentType::HorizontalScrollbar)) {
       AnonymousContentKey key;
       mHScrollbarContent = MakeScrollbar(nodeInfo, /* aVertical */ false, key);
       aElements.AppendElement(ContentInfo(mHScrollbarContent, key));
     }
 
-    if (canHaveVertical) {
+    if (neededAnonContent.contains(AnonymousContentType::VerticalScrollbar)) {
       AnonymousContentKey key;
       mVScrollbarContent = MakeScrollbar(nodeInfo, /* aVertical */ true, key);
       aElements.AppendElement(ContentInfo(mVScrollbarContent, key));
     }
   }
 
-  if (isResizable) {
+  if (neededAnonContent.contains(AnonymousContentType::Resizer)) {
     MOZ_ASSERT(!mIsRoot, "Root scroll frame shouldn't be resizable");
 
     AnonymousContentKey key = AnonymousContentKey::Type_Resizer;
 
     RefPtr<NodeInfo> nodeInfo;
     nodeInfo = nodeInfoManager->GetNodeInfo(
         nsGkAtoms::resizer, nullptr, kNameSpaceID_XUL, nsINode::ELEMENT_NODE);
     NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
 
     NS_TrustedNewXULElement(getter_AddRefs(mResizerContent), nodeInfo.forget());
 
     nsAutoString dir;
-    switch (resizeStyle) {
+    switch (mOuter->StyleDisplay()->mResize) {
       case StyleResize::Horizontal:
         if (IsScrollbarOnRight()) {
           dir.AssignLiteral("right");
           key |= AnonymousContentKey::Flag_Resizer_Right;
         } else {
           dir.AssignLiteral("left");
         }
         break;
@@ -5475,17 +5494,18 @@ nsresult ScrollFrameHelper::CreateAnonym
     mResizerContent->SetAttr(kNameSpaceID_None, nsGkAtoms::dir, dir, false);
     mResizerContent->SetAttr(kNameSpaceID_None, nsGkAtoms::element,
                              u"_parent"_ns, false);
     mResizerContent->SetAttr(kNameSpaceID_None, nsGkAtoms::clickthrough,
                              u"always"_ns, false);
     aElements.AppendElement(ContentInfo(mResizerContent, key));
   }
 
-  if (canHaveHorizontal && canHaveVertical) {
+  if (neededAnonContent.contains(AnonymousContentType::HorizontalScrollbar) &&
+      neededAnonContent.contains(AnonymousContentType::VerticalScrollbar)) {
     AnonymousContentKey key = AnonymousContentKey::Type_ScrollCorner;
 
     RefPtr<NodeInfo> nodeInfo =
         nodeInfoManager->GetNodeInfo(nsGkAtoms::scrollcorner, nullptr,
                                      kNameSpaceID_XUL, nsINode::ELEMENT_NODE);
     NS_TrustedNewXULElement(getter_AddRefs(mScrollCornerContent),
                             nodeInfo.forget());
     if (mIsRoot) {
@@ -5815,17 +5835,17 @@ void nsXULScrollFrame::RemoveVerticalScr
   DebugOnly<bool> result = AddRemoveScrollbar(aState, aOnRight, false, false);
   NS_ASSERTION(result, "Removing vertical scrollbar failed to fit??");
 }
 
 bool nsXULScrollFrame::AddRemoveScrollbar(nsBoxLayoutState& aState,
                                           bool aOnRightOrBottom,
                                           bool aHorizontal, bool aAdd) {
   if (aHorizontal) {
-    if (mHelper.mNeverHasHorizontalScrollbar || !mHelper.mHScrollbarBox) {
+    if (!mHelper.mHScrollbarBox) {
       return false;
     }
 
     nsSize hSize = mHelper.mHScrollbarBox->GetXULPrefSize(aState);
     nsIFrame::AddXULMargin(mHelper.mHScrollbarBox, hSize);
 
     ScrollFrameHelper::SetScrollbarVisibility(mHelper.mHScrollbarBox, aAdd);
 
@@ -5836,17 +5856,17 @@ bool nsXULScrollFrame::AddRemoveScrollba
                                   mHelper.mScrollPort.height, hSize.height,
                                   aOnRightOrBottom, aAdd);
     mHelper.mHasHorizontalScrollbar = hasHorizontalScrollbar;
     if (!fit) {
       ScrollFrameHelper::SetScrollbarVisibility(mHelper.mHScrollbarBox, !aAdd);
     }
     return fit;
   } else {
-    if (mHelper.mNeverHasVerticalScrollbar || !mHelper.mVScrollbarBox) {
+    if (!mHelper.mVScrollbarBox) {
       return false;
     }
 
     nsSize vSize = mHelper.mVScrollbarBox->GetXULPrefSize(aState);
     nsIFrame::AddXULMargin(mHelper.mVScrollbarBox, vSize);
 
     ScrollFrameHelper::SetScrollbarVisibility(mHelper.mVScrollbarBox, aAdd);
 
--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@@ -63,16 +63,28 @@ class ScrollFrameHelper : public nsIRefl
   ScrollFrameHelper(nsContainerFrame* aOuter, bool aIsRoot);
   ~ScrollFrameHelper();
 
   mozilla::ScrollStyles GetScrollStylesFromFrame() const;
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo() const;
 
   bool IsForTextControlWithNoScrollbars() const;
 
+  enum class AnonymousContentType {
+    VerticalScrollbar,
+    HorizontalScrollbar,
+    Resizer,
+  };
+  EnumSet<AnonymousContentType> GetNeededAnonymousContent() const;
+  EnumSet<AnonymousContentType> GetCurrentAnonymousContent() const;
+
+  bool HasAllNeededScrollbars() const {
+    return GetCurrentAnonymousContent().contains(GetNeededAnonymousContent());
+  }
+
   // If a child frame was added or removed on the scrollframe,
   // reload our child frame list.
   // We need this if a scrollbar frame is recreated.
   void ReloadChildFrames();
 
   nsresult CreateAnonymousContent(
       nsTArray<nsIAnonymousContentCreator::ContentInfo>& aElements);
   void AppendAnonymousContentTo(nsTArray<nsIContent*>& aElements,
@@ -620,18 +632,16 @@ class ScrollFrameHelper : public nsIRefl
 
   // Timer to remove the displayport some time after scrolling has stopped
   nsCOMPtr<nsITimer> mDisplayPortExpiryTimer;
 
   ScrollAnchorContainer mAnchor;
 
   bool mAllowScrollOriginDowngrade : 1;
   bool mHadDisplayPortAtLastFrameUpdate : 1;
-  bool mNeverHasVerticalScrollbar : 1;
-  bool mNeverHasHorizontalScrollbar : 1;
   bool mHasVerticalScrollbar : 1;
   bool mHasHorizontalScrollbar : 1;
   // If mHas(Vertical|Horizontal)Scrollbar is true then
   // mOnlyNeed(V|H)ScrollbarToScrollVVInsideLV indicates if the only reason we
   // need that scrollbar is to scroll the visual viewport inside the layout
   // viewport. These scrollbars are special in that even if they are layout
   // scrollbars they do not take up any layout space.
   bool mOnlyNeedVScrollbarToScrollVVInsideLV : 1;
@@ -899,16 +909,19 @@ class nsHTMLScrollFrame : public nsConta
     return mHelper.GetScrolledFrame();
   }
   mozilla::ScrollStyles GetScrollStyles() const override {
     return mHelper.GetScrollStylesFromFrame();
   }
   bool IsForTextControlWithNoScrollbars() const final {
     return mHelper.IsForTextControlWithNoScrollbars();
   }
+  bool HasAllNeededScrollbars() const final {
+    return mHelper.HasAllNeededScrollbars();
+  }
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const final {
     return mHelper.GetOverscrollBehaviorInfo();
   }
   mozilla::layers::ScrollDirections
   GetAvailableScrollingDirectionsForUserInputEvents() const final {
     return mHelper.GetAvailableScrollingDirectionsForUserInputEvents();
   }
@@ -1380,16 +1393,19 @@ class nsXULScrollFrame final : public ns
     return mHelper.GetScrolledFrame();
   }
   mozilla::ScrollStyles GetScrollStyles() const final {
     return mHelper.GetScrollStylesFromFrame();
   }
   bool IsForTextControlWithNoScrollbars() const final {
     return mHelper.IsForTextControlWithNoScrollbars();
   }
+  bool HasAllNeededScrollbars() const final {
+    return mHelper.HasAllNeededScrollbars();
+  }
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const final {
     return mHelper.GetOverscrollBehaviorInfo();
   }
   mozilla::layers::ScrollDirections
   GetAvailableScrollingDirectionsForUserInputEvents() const final {
     return mHelper.GetAvailableScrollingDirectionsForUserInputEvents();
   }
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@@ -80,16 +80,22 @@ class nsIScrollableFrame : public nsIScr
 
   /**
    * Returns whether this scroll frame is for a text control element with no
    * scrollbars (for <input>, basically).
    */
   virtual bool IsForTextControlWithNoScrollbars() const = 0;
 
   /**
+   * Returns whether we already have anonymous content nodes for all our needed
+   * scrollbar parts (or a superset thereof).
+   */
+  virtual bool HasAllNeededScrollbars() const = 0;
+
+  /**
    * Get the overscroll-behavior styles.
    */
   virtual mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const = 0;
 
   /**
    * Return the scrollbars which are visible. It's OK to call this during reflow
    * of the scrolled contents, in which case it will reflect the current
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2445,27 +2445,20 @@ nsChangeHint nsStyleDisplay::CalcDiffere
   if (mOverflowX != aNewData.mOverflowX || mOverflowY != aNewData.mOverflowY) {
     const bool isScrollable = IsScrollableOverflow();
     if (isScrollable != aNewData.IsScrollableOverflow()) {
       // We may need to construct or destroy a scroll frame as a result of this
       // change.
       hint |= nsChangeHint_ScrollbarChange;
     } else if (isScrollable) {
       if (ScrollbarGenerationChanged(*this, aNewData)) {
-        // We need to reframe in the case of hidden -> non-hidden case though,
-        // since ScrollFrameHelper::CreateAnonymousContent avoids creating
-        // scrollbars altogether for overflow: hidden. That seems it could
-        // create some interesting perf cliffs...
-        //
-        // We reframe when non-hidden -> hidden too, for now.
-        //
-        // FIXME(bug 1590247): Seems we could avoid reframing once we've created
-        // scrollbars, which should get us the optimization for elements that
-        // have toggled scrollbars, but would prevent the cliff of toggling
-        // overflow causing jank.
+        // We might need to reframe in the case of hidden -> non-hidden case
+        // though, since ScrollFrameHelper::CreateAnonymousContent avoids
+        // creating scrollbars altogether for overflow: hidden. That seems it
+        // could create some interesting perf cliffs...
         hint |= nsChangeHint_ScrollbarChange;
       } else {
         // Otherwise, for changes where both overflow values are scrollable,
         // means that scrollbars may appear or disappear. We need to reflow,
         // since reflow is what determines which scrollbars if any are visible.
         hint |= nsChangeHint_ReflowHintsForScrollbarChange;
       }
     } else {
--- a/layout/style/test/test_dynamic_change_causing_reflow.html
+++ b/layout/style/test/test_dynamic_change_causing_reflow.html
@@ -11,25 +11,27 @@ https://bugzilla.mozilla.org/show_bug.cg
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1131371">Mozilla Bug 1131371</a>
 <div id="display">
   <div id="content">
   </div>
   <div id="elemWithAbsPosChild"><div style="position:absolute"></div></div>
   <div id="elemWithFixedPosChild"><div style="position:fixed"></div></div>
+  <div id="elemWithScrollbars" style="overflow: scroll"></div>
 </div>
 <pre id="test">
 <script type="application/javascript">
 "use strict";
 
 /** Test for Bug 1131371 **/
 
 const elemWithAbsPosChild = document.getElementById("elemWithAbsPosChild");
 const elemWithFixedPosChild = document.getElementById("elemWithFixedPosChild");
+const elemWithScrollbars = document.getElementById("elemWithScrollbars");
 
 /**
  * This test verifies that certain style changes do or don't cause reflow
  * and/or frame construction. We do this by checking the framesReflowed &
  * framesConstructed counts, before & after a style-change, and verifying
  * that any change to these counts is in line with our expectations.
  *
  * Each entry in gTestcases contains these member-values:
@@ -212,41 +214,45 @@ const gTestcases = [
   },
   {
     beforeStyle: "overflow: auto",
     afterStyle:  "overflow: scroll",
     expectConstruction: false,
     expectReflow: true,
   },
 
-  // * FIXME(emilio, bug 1590247): Changes to or from hidden
-  // reconstruct, as we optimize out the scrollbars completely in the overflow:
-  // hidden case, but we could do better.
   {
     beforeStyle: "overflow: hidden",
     afterStyle:  "overflow: auto",
     expectConstruction: true,
     expectReflow: true,
   },
   {
     beforeStyle: "overflow: auto",
     afterStyle:  "overflow: hidden",
-    expectConstruction: true,
+    expectConstruction: false,
     expectReflow: true,
   },
   {
     beforeStyle: "overflow: hidden",
     afterStyle:  "overflow: scroll",
     expectConstruction: true,
     expectReflow: true,
   },
   {
     beforeStyle: "overflow: scroll",
     afterStyle:  "overflow: hidden",
-    expectConstruction: true,
+    expectConstruction: false,
+    expectReflow: true,
+  },
+  {
+    elem: elemWithScrollbars,
+    beforeStyle: "overflow: hidden",
+    afterStyle:  "overflow: scroll",
+    expectConstruction: false,
     expectReflow: true,
   },
   // * Changing 'display' should cause frame construction and reflow.
   {
     beforeStyle: "display: inline",
     afterStyle:  "display: table",
     expectConstruction: true,
     expectReflow: true,
@@ -359,25 +365,20 @@ function runOneTest(aTestcase)
 {
   // sanity-check that we have the one main thing we need:
   if (!aTestcase.afterStyle) {
     ok(false, "testcase is missing an 'afterStyle' to change to");
     return;
   }
 
   // Figure out which element we'll be tweaking (defaulting to gElem)
-  let elem = aTestcase.elem ?
-    aTestcase.elem : gElem;
+  let elem = aTestcase.elem ? aTestcase.elem : gElem;
 
   // Verify that 'style' attribute is unset (avoid causing ourselves trouble):
-  if (elem.hasAttribute("style")) {
-    ok(false,
-       "test element has 'style' attribute already set! We're going to stomp " +
-       "on whatever's there when we clean up...");
-  }
+  const oldStyle = elem.getAttribute("style");
 
   // Set the "before" style, and compose the first part of the message
   // to be used in our "is"/"isnot" invocations:
   let msgPrefix = "Changing style ";
   if (aTestcase.beforeStyle) {
     elem.setAttribute("style", aTestcase.beforeStyle);
     msgPrefix += "from '" + aTestcase.beforeStyle + "' ";
   }
@@ -397,17 +398,23 @@ function runOneTest(aTestcase)
   checkFinalCount(gUtils.framesConstructed, origFramesConstructed,
                   aTestcase.expectConstruction, msgPrefix,
                   "frame construction");
   checkFinalCount(gUtils.framesReflowed, origFramesReflowed,
                   aTestcase.expectReflow, msgPrefix,
                   "reflow");
 
   // Clean up!
-  elem.removeAttribute("style");
+  if (oldStyle) {
+    elem.setAttribute("style", oldStyle);
+  } else {
+    elem.removeAttribute("style");
+  }
+
+  unusedVal = elem.offsetHeight; // flush layout
 }
 
 gTestcases.forEach(runOneTest);
 
 </script>
 </pre>
 </body>
 </html>