Bug 1811487 - Clean-up popup hide / rollup APIs. r=cmartin

I'm about to extend them for bug 1811486, where I want to force in some
cases the rolled up popups to hide synchronously. These APIs use a ton
of boolean arguments that make them error prone, so refactor them a bit
to use strongly typed enums and flags.

Differential Revision: https://phabricator.services.mozilla.com/D167381

dom/xul/XULButtonElement.cpp

@@ -95,9 +95,9 @@ void XULButtonElement::HandleEnterKeyPress(WidgetEvent& aEvent) {
 #ifdef XP_WIN
     if (XULPopupElement* popup = GetContainingPopupElement()) {
       if (nsXULPopupManager* pm = nsXULPopupManager::GetInstance()) {
-        pm->HidePopup(popup, /* aHideChain = */ true,
+        pm->HidePopup(
-                      /* aDeselectMenu = */ true, /* aAsynchronous = */ true,
+            popup, {HidePopupOption::HideChain, HidePopupOption::DeselectMenu,
-                      /* aIsCancel = */ false);
+                    HidePopupOption::Async});
       }
     }
 #endif
@@ -211,7 +211,11 @@ void XULButtonElement::CloseMenuPopup(bool aDeselectMenu) {
     return;
   }
   if (auto* popup = GetMenuPopupContent()) {
-    pm->HidePopup(popup, false, aDeselectMenu, true, false);
+    HidePopupOptions options{HidePopupOption::Async};
+    if (aDeselectMenu) {
+      options += HidePopupOption::DeselectMenu;
+    }
+    pm->HidePopup(popup, options);
   }
 }
 

dom/xul/XULPopupElement.cpp

@@ -106,9 +106,14 @@ void XULPopupElement::OpenPopupAtScreenRect(const nsAString& aPosition,
 
 void XULPopupElement::HidePopup(bool aCancel) {
   nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
-  if (pm) {
+  if (!pm) {
-    pm->HidePopup(this, false, true, false, aCancel);
+    return;
   }
+  HidePopupOptions options{HidePopupOption::DeselectMenu};
+  if (aCancel) {
+    options += HidePopupOption::IsRollup;
+  }
+  pm->HidePopup(this, options);
 }
 
 static Modifiers ConvertModifiers(const ActivateMenuItemOptions& aModifiers) {

dom/xul/nsXULPopupListener.cpp

@@ -170,7 +170,9 @@ void nsXULPopupListener::ClosePopup() {
     // popup is hidden. Use asynchronous hiding just to be safe so we don't
     // fire events during destruction.
     nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
-    if (pm) pm->HidePopup(mPopupContent, false, true, true, false);
+    if (pm)
+      pm->HidePopup(mPopupContent,
+                    {HidePopupOption::DeselectMenu, HidePopupOption::Async});
     mPopupContent = nullptr;  // release the popup
   }
 }  // ClosePopup

layout/xul/nsMenuBarListener.cpp

@@ -207,7 +207,7 @@ nsresult nsMenuBarListener::KeyUp(Event* aKeyEvent) {
       // handle key events when menubar is active and IME should be
       // disabled.
       if (nsXULPopupManager* pm = nsXULPopupManager::GetInstance()) {
-        pm->Rollup(0, false, nullptr, nullptr);
+        pm->Rollup({});
       }
       // If menubar active state is changed or the menubar is destroyed
       // during closing the popups, we should do nothing anymore.

layout/xul/nsMenuPopupFrame.cpp

@@ -62,6 +62,7 @@
 #include <algorithm>
 
 #include "X11UndefineNone.h"
+#include "nsXULPopupManager.h"
 
 using namespace mozilla;
 using mozilla::dom::Document;
@@ -2463,7 +2464,8 @@ void nsMenuPopupFrame::CheckForAnchorChange(nsRect& aRect) {
     if (pm) {
       // As the caller will be iterating over the open popups, hide
       // asyncronously.
-      pm->HidePopup(mContent, false, true, true, false);
+      pm->HidePopup(mContent,
+                    {HidePopupOption::DeselectMenu, HidePopupOption::Async});
     }
 
     return;

layout/xul/nsXULPopupManager.cpp

@@ -278,14 +278,12 @@ nsXULPopupManager* nsXULPopupManager::GetInstance() {
 }
 
 bool nsXULPopupManager::RollupTooltips() {
-  return RollupInternal(RollupKind::Tooltip, UINT32_MAX, false, nullptr,
+  return RollupInternal(RollupKind::Tooltip, {}, nullptr);
-                        nullptr);
 }
 
-bool nsXULPopupManager::Rollup(uint32_t aCount, bool aFlush,
+bool nsXULPopupManager::Rollup(const RollupOptions& aOptions,
-                               const LayoutDeviceIntPoint* aPos,
                                nsIContent** aLastRolledUp) {
-  return RollupInternal(RollupKind::Menu, aCount, aFlush, aPos, aLastRolledUp);
+  return RollupInternal(RollupKind::Menu, aOptions, aLastRolledUp);
 }
 
 bool nsXULPopupManager::RollupNativeMenu() {
@@ -296,9 +294,8 @@ bool nsXULPopupManager::RollupNativeMenu() {
   return false;
 }
 
-bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
+bool nsXULPopupManager::RollupInternal(RollupKind aKind,
-                                       bool aFlush,
+                                       const RollupOptions& aOptions,
-                                       const LayoutDeviceIntPoint* pos,
                                        nsIContent** aLastRolledUp) {
   if (aLastRolledUp) {
     *aLastRolledUp = nullptr;
@@ -349,7 +346,7 @@ bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
   // This would be used to allow adjusting the caret position in an
   // autocomplete field without hiding the popup for example.
   bool noRollupOnAnchor =
-      (!consume && pos &&
+      (!consume && aOptions.mPoint &&
        item->Frame()->GetContent()->AsElement()->AttrValueIs(
            kNameSpaceID_None, nsGkAtoms::norolluponanchor, nsGkAtoms::_true,
            eCaseMatters));
@@ -358,7 +355,7 @@ bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
   // when the click was over the anchor. This way, clicking on a menu doesn't
   // reopen the menu.
   if ((consumeResult == ConsumeOutsideClicks_ParentOnly || noRollupOnAnchor) &&
-      pos) {
+      aOptions.mPoint) {
     nsMenuPopupFrame* popupFrame = item->Frame();
     CSSIntRect anchorRect = [&] {
       if (popupFrame->IsAnchored()) {
@@ -399,7 +396,8 @@ bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
     // event will get consumed, so here only a quick coordinates check is
     // done rather than a slower complete check of what is at that location.
     nsPresContext* presContext = item->Frame()->PresContext();
-    CSSIntPoint posCSSPixels = presContext->DevPixelsToIntCSSPixels(*pos);
+    CSSIntPoint posCSSPixels =
+        presContext->DevPixelsToIntCSSPixels(*aOptions.mPoint);
     if (anchorRect.Contains(posCSSPixels)) {
       if (consumeResult == ConsumeOutsideClicks_ParentOnly) {
         consume = true;
@@ -415,12 +413,13 @@ bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
     return false;
   }
 
-  // if a number of popups to close has been specified, determine the last
+  // If a number of popups to close has been specified, determine the last
-  // popup to close
+  // popup to close.
   nsIContent* lastPopup = nullptr;
-  if (aCount != UINT32_MAX) {
+  uint32_t count = aOptions.mCount;
+  if (count && count != UINT32_MAX) {
     nsMenuChainItem* last = item;
-    while (--aCount && last->GetParent()) {
+    while (--count && last->GetParent()) {
       last = last->GetParent();
     }
     if (last) {
@@ -432,9 +431,12 @@ bool nsXULPopupManager::RollupInternal(RollupKind aKind, uint32_t aCount,
   RefPtr<nsViewManager> viewManager =
       presContext->PresShell()->GetViewManager();
 
-  HidePopup(item->Content(), true, true, false, true, lastPopup);
+  HidePopup(item->Content(),
+            {HidePopupOption::HideChain, HidePopupOption::DeselectMenu,
+             HidePopupOption::IsRollup},
+            lastPopup);
 
-  if (aFlush) {
+  if (aOptions.mFlush == FlushViews::Yes) {
     // The popup's visibility doesn't update until the minimize animation
     // has finished, so call UpdateWidgetGeometry to update it right away.
     viewManager->UpdateWidgetGeometry();
@@ -944,7 +946,7 @@ void nsXULPopupManager::OnNativeMenuClosed() {
     // menus.
     // Close the non-native menus now. This matches the HidePopup call in
     // nsXULMenuCommandEvent::Run.
-    HidePopup(mPopups->Content(), true, false, false, false);
+    HidePopup(mPopups->Content(), {HidePopupOption::HideChain});
   }
 }
 
@@ -1130,9 +1132,8 @@ nsMenuChainItem* nsXULPopupManager::FindPopup(nsIContent* aPopup) const {
   return nullptr;
 }
 
-void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
+void nsXULPopupManager::HidePopup(nsIContent* aPopup, HidePopupOptions aOptions,
-                                  bool aDeselectMenu, bool aAsynchronous,
+                                  nsIContent* aLastPopup) {
-                                  bool aIsCancel, nsIContent* aLastPopup) {
   if (mNativeMenu && mNativeMenu->Element() == aPopup) {
     RefPtr<NativeMenu> menu = mNativeMenu;
     (void)menu->Close();
@@ -1146,7 +1147,6 @@ void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
 
   nsMenuChainItem* foundPopup = FindPopup(aPopup);
 
-  bool deselectMenu = false;
   nsCOMPtr<nsIContent> popupToHide, nextPopup, lastPopup;
 
   if (foundPopup) {
@@ -1154,6 +1154,8 @@ void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
       // If this is a noautohide panel, remove it but don't close any other
       // panels.
       popupToHide = aPopup;
+      // XXX This preserves behavior but why is it the right thing to do?
+      aOptions -= HidePopupOption::DeselectMenu;
     } else {
       // At this point, foundPopup will be set to the found item in the list. If
       // foundPopup is the topmost menu, the one to remove, then there are no
@@ -1183,14 +1185,15 @@ void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
         }
       }
 
-      deselectMenu = aDeselectMenu;
       popupToHide = topMenu->Content();
       popupFrame = topMenu->Frame();
 
+      const bool hideChain = aOptions.contains(HidePopupOption::HideChain);
+
       // Close up another popup if there is one, and we are either hiding the
       // entire chain or the item to hide isn't the topmost popup.
       nsMenuChainItem* parent = topMenu->GetParent();
-      if (parent && (aHideChain || topMenu != foundPopup)) {
+      if (parent && (hideChain || topMenu != foundPopup)) {
         while (parent && parent->IsNoAutoHide()) {
           parent = parent->GetParent();
         }
@@ -1200,17 +1203,19 @@ void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
         }
       }
 
-      lastPopup = aLastPopup ? aLastPopup : (aHideChain ? nullptr : aPopup);
+      lastPopup = aLastPopup ? aLastPopup : (hideChain ? nullptr : aPopup);
     }
   } else if (popupFrame->PopupState() == ePopupPositioning) {
     // When the popup is in the popuppositioning state, it will not be in the
     // mPopups list. We need another way to find it and make sure it does not
     // continue the popup showing process.
-    deselectMenu = aDeselectMenu;
     popupToHide = aPopup;
   }
 
-  if (popupToHide) {
+  if (!popupToHide) {
+    return;
+  }
+
   nsPopupState state = popupFrame->PopupState();
   // If the popup is already being hidden, don't attempt to hide it again
   if (state == ePopupHiding) {
@@ -1225,16 +1230,14 @@ void nsXULPopupManager::HidePopup(nsIContent* aPopup, bool aHideChain,
   }
 
   // For menus, popupToHide is always the frontmost item in the list to hide.
-    if (aAsynchronous) {
+  if (aOptions.contains(HidePopupOption::Async)) {
     nsCOMPtr<nsIRunnable> event = new nsXULPopupHidingEvent(
-          popupToHide, nextPopup, lastPopup, popupFrame->PopupType(),
+        popupToHide, nextPopup, lastPopup, popupFrame->PopupType(), aOptions);
-          deselectMenu, aIsCancel);
     aPopup->OwnerDoc()->Dispatch(TaskCategory::Other, event.forget());
   } else {
     RefPtr<nsPresContext> presContext = popupFrame->PresContext();
     FirePopupHidingEvent(popupToHide, nextPopup, lastPopup, presContext,
-                           popupFrame->PopupType(), deselectMenu, aIsCancel);
+                         popupFrame->PopupType(), aOptions);
-    }
   }
 }
 
@@ -1253,7 +1256,7 @@ void nsXULPopupManager::HideMenu(nsIContent* aMenu) {
   if (!popup) {
     return;
   }
-  HidePopup(popup, false, true, false, false);
+  HidePopup(popup, {HidePopupOption::DeselectMenu});
 }
 
 // This is used to hide the popup after a transition finishes.
@@ -1266,13 +1269,13 @@ class TransitionEnder final : public nsIDOMEventListener {
   virtual ~TransitionEnder() = default;
 
  public:
-  bool mDeselectMenu;
+  HidePopupOptions mOptions;
 
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_CLASS(TransitionEnder)
 
-  TransitionEnder(nsIContent* aContent, bool aDeselectMenu)
+  TransitionEnder(nsIContent* aContent, HidePopupOptions aOptions)
-      : mContent(aContent), mDeselectMenu(aDeselectMenu) {}
+      : mContent(aContent), mOptions(aOptions) {}
 
   MOZ_CAN_RUN_SCRIPT NS_IMETHOD HandleEvent(Event* aEvent) override {
     mContent->RemoveSystemEventListener(u"transitionend"_ns, this, false);
@@ -1287,7 +1290,7 @@ class TransitionEnder final : public nsIDOMEventListener {
     // the first one ending.
     if (RefPtr<nsXULPopupManager> pm = nsXULPopupManager::GetInstance()) {
       pm->HidePopupCallback(mContent, popupFrame, nullptr, nullptr,
-                            popupFrame->PopupType(), mDeselectMenu);
+                            popupFrame->PopupType(), mOptions);
     }
 
     return NS_OK;
@@ -1304,7 +1307,7 @@ NS_INTERFACE_MAP_END
 NS_IMPL_CYCLE_COLLECTION(TransitionEnder, mContent);
 void nsXULPopupManager::HidePopupCallback(
     nsIContent* aPopup, nsMenuPopupFrame* aPopupFrame, nsIContent* aNextPopup,
-    nsIContent* aLastPopup, nsPopupType aPopupType, bool aDeselectMenu) {
+    nsIContent* aLastPopup, nsPopupType aPopupType, HidePopupOptions aOptions) {
   if (mCloseTimer && mTimerMenu == aPopupFrame) {
     mCloseTimer->Cancel();
     mCloseTimer = nullptr;
@@ -1325,7 +1328,8 @@ void nsXULPopupManager::HidePopupCallback(
   }
 
   AutoWeakFrame weakFrame(aPopupFrame);
-  aPopupFrame->HidePopup(aDeselectMenu, ePopupClosed);
+  aPopupFrame->HidePopup(aOptions.contains(HidePopupOption::DeselectMenu),
+                         ePopupClosed);
   NS_ENSURE_TRUE_VOID(weakFrame.IsAlive());
 
   // send the popuphidden event synchronously. This event has no default
@@ -1363,7 +1367,7 @@ void nsXULPopupManager::HidePopupCallback(
 
       RefPtr<nsPresContext> presContext = popupFrame->PresContext();
       FirePopupHidingEvent(popupToHide, nextPopup, aLastPopup, presContext,
-                           foundMenu->PopupType(), aDeselectMenu, false);
+                           foundMenu->PopupType(), aOptions);
     }
   }
 }
@@ -1620,10 +1624,12 @@ void nsXULPopupManager::BeginShowingPopup(const PendingPopup& aPendingPopup,
   }
 }
 
-void nsXULPopupManager::FirePopupHidingEvent(
+void nsXULPopupManager::FirePopupHidingEvent(nsIContent* aPopup,
-    nsIContent* aPopup, nsIContent* aNextPopup, nsIContent* aLastPopup,
+                                             nsIContent* aNextPopup,
-    nsPresContext* aPresContext, nsPopupType aPopupType, bool aDeselectMenu,
+                                             nsIContent* aLastPopup,
-    bool aIsCancel) {
+                                             nsPresContext* aPresContext,
+                                             nsPopupType aPopupType,
+                                             HidePopupOptions aOptions) {
   nsCOMPtr<nsIContent> popup = aPopup;
   RefPtr<PresShell> presShell = aPresContext->PresShell();
   Unused << presShell;  // This presShell may be keeping things alive
@@ -1694,7 +1700,8 @@ void nsXULPopupManager::FirePopupHidingEvent(
     }
     // If animate="cancel", only show the transition if cancelling the popup
     // or rolling up.
-    if (animate.EqualsLiteral("cancel") && !aIsCancel) {
+    if (animate.EqualsLiteral("cancel") &&
+        !aOptions.contains(HidePopupOption::IsRollup)) {
       return false;
     }
     return true;
@@ -1705,13 +1712,13 @@ void nsXULPopupManager::FirePopupHidingEvent(
   // view will be hidden and you won't be able to see it.
   if (shouldAnimate && AnimationUtils::HasCurrentTransitions(
                            aPopup->AsElement(), PseudoStyleType::NotPseudo)) {
-    RefPtr<TransitionEnder> ender = new TransitionEnder(aPopup, aDeselectMenu);
+    RefPtr<TransitionEnder> ender = new TransitionEnder(aPopup, aOptions);
     aPopup->AddSystemEventListener(u"transitionend"_ns, ender, false, false);
     return;
   }
 
   HidePopupCallback(aPopup, popupFrame, aNextPopup, aLastPopup, aPopupType,
-                    aDeselectMenu);
+                    aOptions);
 }
 
 bool nsXULPopupManager::IsPopupOpen(nsIContent* aPopup) {
@@ -1933,7 +1940,7 @@ void nsXULPopupManager::PopupDestroyed(nsMenuPopupFrame* aPopup) {
       } else {
         // HidePopup will take care of hiding any of its children, so
         // break out afterwards
-        HidePopup(child->Content(), false, false, true, false);
+        HidePopup(child->Content(), {HidePopupOption::Async});
         break;
       }
     }
@@ -2135,7 +2142,7 @@ void nsXULPopupManager::KillMenuTimer() {
     mCloseTimer = nullptr;
 
     if (mTimerMenu->IsOpen()) {
-      HidePopup(mTimerMenu->GetContent(), false, false, true, false);
+      HidePopup(mTimerMenu->GetContent(), {HidePopupOption::Async});
     }
   }
 
@@ -2372,9 +2379,7 @@ bool nsXULPopupManager::HandleKeyboardNavigationInPopup(
     // close a submenu when Left is pressed
     if (nsMenuPopupFrame* popupFrame =
             currentItem->GetMenuPopup(FlushType::None)) {
-      HidePopup(popupFrame->GetContent(), /* aHideChain = */ false,
+      HidePopup(popupFrame->GetContent(), {});
-                /* aDeselectMenu = */ false, /* aAsynchronous = */ false,
-                /* aIsCancel = */ false);
     }
     return true;
   }
@@ -2390,7 +2395,7 @@ bool nsXULPopupManager::HandleKeyboardEventWithKeyCode(
   if (aTopVisibleMenuItem &&
       aTopVisibleMenuItem->PopupType() != ePopupTypeMenu) {
     if (keyCode == KeyboardEvent_Binding::DOM_VK_ESCAPE) {
-      HidePopup(aTopVisibleMenuItem->Content(), false, false, false, true);
+      HidePopup(aTopVisibleMenuItem->Content(), {HidePopupOption::IsRollup});
       aKeyEvent->StopPropagation();
       aKeyEvent->StopCrossProcessForwarding();
       aKeyEvent->PreventDefault();
@@ -2406,7 +2411,7 @@ bool nsXULPopupManager::HandleKeyboardEventWithKeyCode(
       // roll up the popup when alt+up/down are pressed within a menulist.
       if (aKeyEvent->AltKey() && aTopVisibleMenuItem &&
           aTopVisibleMenuItem->Frame()->IsMenuList()) {
-        Rollup(0, false, nullptr, nullptr);
+        Rollup({});
         break;
       }
       [[fallthrough]];
@@ -2433,7 +2438,7 @@ bool nsXULPopupManager::HandleKeyboardEventWithKeyCode(
       // though in this latter case, a menu didn't actually close, the effect
       // ends up being the same. Similar for the tab key below.
       if (aTopVisibleMenuItem) {
-        HidePopup(aTopVisibleMenuItem->Content(), false, false, false, true);
+        HidePopup(aTopVisibleMenuItem->Content(), {HidePopupOption::IsRollup});
       } else if (mActiveMenuBar) {
         mActiveMenuBar->MenuClosed();
       }
@@ -2448,7 +2453,7 @@ bool nsXULPopupManager::HandleKeyboardEventWithKeyCode(
               kNameSpaceID_None, nsGkAtoms::activateontab, nsGkAtoms::_true,
               eCaseMatters)) {
         // Close popups or deactivate menubar when Tab or F10 are pressed
-        Rollup(0, false, nullptr, nullptr);
+        Rollup({});
         break;
       } else if (mActiveMenuBar) {
         mActiveMenuBar->MenuClosed();
@@ -2594,7 +2599,7 @@ nsresult nsXULPopupManager::KeyDown(KeyboardEvent* aKeyEvent) {
         // modifiers are already down.
         nsMenuChainItem* item = GetTopVisibleMenu();
         if (item && !item->Frame()->IsMenuList()) {
-          Rollup(0, false, nullptr, nullptr);
+          Rollup({});
         } else if (mActiveMenuBar) {
           mActiveMenuBar->MenuClosed();
         }
@@ -2657,7 +2662,7 @@ nsXULPopupHidingEvent::Run() {
       nsCOMPtr<nsIContent> nextPopup = mNextPopup;
       nsCOMPtr<nsIContent> lastPopup = mLastPopup;
       pm->FirePopupHidingEvent(popup, nextPopup, lastPopup, presContext,
-                               mPopupType, mDeselectMenu, mIsRollup);
+                               mPopupType, mOptions);
     }
   }
   return NS_OK;
@@ -2810,8 +2815,11 @@ nsXULMenuCommandEvent::Run() {
 
   if (mCloseMenuMode != CloseMenuMode_None) {
     if (RefPtr popup = menu->GetContainingPopupElement()) {
-      pm->HidePopup(popup, mCloseMenuMode == CloseMenuMode_Auto, true, false,
+      HidePopupOptions options{HidePopupOption::DeselectMenu};
-                    false);
+      if (mCloseMenuMode == CloseMenuMode_Auto) {
+        options += HidePopupOption::HideChain;
+      }
+      pm->HidePopup(popup, options);
     }
   }
 

layout/xul/nsXULPopupManager.h

@@ -161,6 +161,21 @@ enum nsIgnoreKeys {
   eIgnoreKeys_Shortcuts,
 };
 
+enum class HidePopupOption : uint8_t {
+  // If the entire chain of menus should be closed.
+  HideChain,
+  // If the parent <menu> of the popup should not be deselected. This will not
+  // be set when the menu is closed by pressing the Escape key.
+  DeselectMenu,
+  // If the first popuphiding event should be sent asynchrously. This should
+  // be set if HidePopup is called from a frame.
+  Async,
+  // If this popup is hiding due to being cancelled.
+  IsRollup,
+};
+
+using HidePopupOptions = mozilla::EnumSet<HidePopupOption>;
+
 #define NS_DIRECTION_IS_INLINE(dir) \
   (dir == eNavigationDirection_Start || dir == eNavigationDirection_End)
 #define NS_DIRECTION_IS_BLOCK(dir) \
@@ -277,14 +292,13 @@ class nsXULPopupHidingEvent : public mozilla::Runnable {
  public:
   nsXULPopupHidingEvent(nsIContent* aPopup, nsIContent* aNextPopup,
                         nsIContent* aLastPopup, nsPopupType aPopupType,
-                        bool aDeselectMenu, bool aIsCancel)
+                        HidePopupOptions aOptions)
       : mozilla::Runnable("nsXULPopupHidingEvent"),
         mPopup(aPopup),
         mNextPopup(aNextPopup),
         mLastPopup(aLastPopup),
         mPopupType(aPopupType),
-        mDeselectMenu(aDeselectMenu),
+        mOptions(aOptions) {
-        mIsRollup(aIsCancel) {
     NS_ASSERTION(aPopup,
                  "null popup supplied to nsXULPopupHidingEvent constructor");
     // aNextPopup and aLastPopup may be null
@@ -297,8 +311,7 @@ class nsXULPopupHidingEvent : public mozilla::Runnable {
   nsCOMPtr<nsIContent> mNextPopup;
   nsCOMPtr<nsIContent> mLastPopup;
   nsPopupType mPopupType;
-  bool mDeselectMenu;
+  HidePopupOptions mOptions;
-  bool mIsRollup;
 };
 
 // this class is used for dispatching popuppositioned events asynchronously.
@@ -370,9 +383,8 @@ class nsXULPopupManager final : public nsIDOMEventListener,
 
   // nsIRollupListener
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
-  bool Rollup(uint32_t aCount, bool aFlush,
+  bool Rollup(const RollupOptions&,
-              const mozilla::LayoutDeviceIntPoint* aPos,
+              nsIContent** aLastRolledUp = nullptr) override;
-              nsIContent** aLastRolledUp) override;
   bool ShouldRollupOnMouseWheelEvent() override;
   bool ShouldConsumeOnMouseWheelEvent() override;
   bool ShouldRollupOnMouseActivate() override;
@@ -384,8 +396,7 @@ class nsXULPopupManager final : public nsIDOMEventListener,
 
   enum class RollupKind { Tooltip, Menu };
   MOZ_CAN_RUN_SCRIPT
-  bool RollupInternal(RollupKind, uint32_t aCount, bool aFlush,
+  bool RollupInternal(RollupKind, const RollupOptions&,
-                      const mozilla::LayoutDeviceIntPoint* pos,
                       nsIContent** aLastRolledUp);
 
   // NativeMenu::Observer
@@ -502,21 +513,10 @@ class nsXULPopupManager final : public nsIDOMEventListener,
   /*
    * Hide a popup aPopup. If the popup is in a <menu>, then also inform the
    * menu that the popup is being hidden.
-   *
-   * aHideChain - true if the entire chain of menus should be closed. If false,
-   *              only this popup is closed.
-   * aDeselectMenu - true if the parent <menu> of the popup should be
-   *                 deselected. This will be false when the menu is closed by
-   *                 pressing the Escape key.
-   * aAsynchronous - true if the first popuphiding event should be sent
-   *                 asynchrously. This should be true if HidePopup is called
-   *                 from a frame.
-   * aIsCancel - true if this popup is hiding due to being cancelled.
    * aLastPopup - optional popup to close last when hiding a chain of menus.
    *              If null, then all popups will be closed.
    */
-  void HidePopup(nsIContent* aPopup, bool aHideChain, bool aDeselectMenu,
+  void HidePopup(nsIContent* aPopup, HidePopupOptions,
-                 bool aAsynchronous, bool aIsCancel,
                  nsIContent* aLastPopup = nullptr);
 
   /*
@@ -747,7 +747,7 @@ class nsXULPopupManager final : public nsIDOMEventListener,
                                             bool aSelectFirstItem);
   MOZ_CAN_RUN_SCRIPT void HidePopupCallback(
       nsIContent* aPopup, nsMenuPopupFrame* aPopupFrame, nsIContent* aNextPopup,
-      nsIContent* aLastPopup, nsPopupType aPopupType, bool aDeselectMenu);
+      nsIContent* aLastPopup, nsPopupType aPopupType, HidePopupOptions);
 
   /**
    * Trigger frame construction and reflow in the popup, fire a popupshowing
@@ -779,14 +779,13 @@ class nsXULPopupManager final : public nsIDOMEventListener,
    * aLastPopup - the last popup in the chain to hide
    * aPresContext - nsPresContext for the popup's frame
    * aPopupType - the PopupType of the frame.
-   * aDeselectMenu - true to unhighlight the menu when hiding it
+   * aOptions - the relevant options to hide the popup. Only a subset is looked
-   * aIsCancel - true if this popup is hiding due to being cancelled.
+   * at.
    */
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   void FirePopupHidingEvent(nsIContent* aPopup, nsIContent* aNextPopup,
                             nsIContent* aLastPopup, nsPresContext* aPresContext,
-                            nsPopupType aPopupType, bool aDeselectMenu,
+                            nsPopupType aPopupType, HidePopupOptions aOptions);
-                            bool aIsCancel);
 
   /**
    * Handle keyboard navigation within a menu popup specified by aItem.

layout/xul/nsXULTooltipListener.cpp

@@ -474,7 +474,7 @@ void nsXULTooltipListener::LaunchTooltip() {
 nsresult nsXULTooltipListener::HideTooltip() {
   if (nsCOMPtr<nsIContent> currentTooltip = do_QueryReferent(mCurrentTooltip)) {
     if (nsXULPopupManager* pm = nsXULPopupManager::GetInstance()) {
-      pm->HidePopup(currentTooltip, false, false, false, false);
+      pm->HidePopup(currentTooltip, {});
     }
   }
 

view/nsView.cpp

@@ -1037,9 +1037,8 @@ void nsView::DynamicToolbarOffsetChanged(ScreenIntCoord aOffset) {
 
 bool nsView::RequestWindowClose(nsIWidget* aWidget) {
   if (mFrame && IsPopupWidget(aWidget) && mFrame->IsMenuPopupFrame()) {
-    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
+    if (nsXULPopupManager* pm = nsXULPopupManager::GetInstance()) {
-    if (pm) {
+      pm->HidePopup(mFrame->GetContent(), {HidePopupOption::DeselectMenu});
-      pm->HidePopup(mFrame->GetContent(), false, true, false, false);
       return true;
     }
   }

widget/cocoa/nsChildView.mm

@@ -2435,14 +2435,16 @@ NSEvent* gLastDragMouseDownEvent = nil;  // [strong]
       }
 
       if (shouldRollup) {
+        LayoutDeviceIntPoint devPoint;
+        nsIRollupListener::RollupOptions rollupOptions{popupsToRollup,
+                                                       nsIRollupListener::FlushViews::Yes};
         if ([theEvent type] == NSEventTypeLeftMouseDown) {
           NSPoint point = [NSEvent mouseLocation];
           FlipCocoaScreenCoordinate(point);
-          LayoutDeviceIntPoint devPoint = mGeckoChild->CocoaPointsToDevPixels(point);
+          devPoint = mGeckoChild->CocoaPointsToDevPixels(point);
-          consumeEvent = (BOOL)rollupListener->Rollup(popupsToRollup, true, &devPoint, nullptr);
+          rollupOptions.mPoint = &devPoint;
-        } else {
-          consumeEvent = (BOOL)rollupListener->Rollup(popupsToRollup, true, nullptr, nullptr);
         }
+        consumeEvent = (BOOL)rollupListener->Rollup(rollupOptions);
       }
     }
   }

widget/cocoa/nsCocoaWindow.mm

@@ -120,7 +120,7 @@ static void RollUpPopups() {
 
   nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
   if (!rollupWidget) return;
-  rollupListener->Rollup(0, true, nullptr, nullptr);
+  rollupListener->Rollup({0, nsIRollupListener::FlushViews::Yes});
 }
 
 nsCocoaWindow::nsCocoaWindow()

widget/cocoa/nsMenuX.mm

@@ -1159,7 +1159,7 @@ void nsMenuX::Dump(uint32_t aIndent) const {
     if (rollupListener) {
       nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
       if (rollupWidget) {
-        rollupListener->Rollup(0, true, nullptr, nullptr);
+        rollupListener->Rollup({0, nsIRollupListener::FlushViews::Yes});
         [menu cancelTracking];
         return;
       }

widget/cocoa/nsToolkit.mm

@@ -179,7 +179,7 @@ void nsToolkit::MonitorAllProcessMouseEvents() {
                                           return;
                                         }
 
-                                        rollupListener->Rollup(0, false, nullptr, nullptr);
+                                        rollupListener->Rollup({});
                                       }];
   }
 

widget/gtk/nsWindow.cpp

@@ -625,7 +625,7 @@ void nsWindow::Destroy() {
   if (rollupListener) {
     nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
     if (static_cast<nsIWidget*>(this) == rollupWidget) {
-      rollupListener->Rollup(0, false, nullptr, nullptr);
+      rollupListener->Rollup({});
     }
   }
 
@@ -4017,7 +4017,7 @@ gboolean nsWindow::OnConfigureEvent(GtkWidget* aWidget,
     // This check avoids unwanted rollup on spurious configure events from
     // Cygwin/X (bug 672103).
     if (mBounds.x != screenBounds.x || mBounds.y != screenBounds.y) {
-      CheckForRollup(0, 0, false, true);
+      RollupAllMenus();
     }
   }
 
@@ -4782,7 +4782,7 @@ void nsWindow::OnContainerFocusOutEvent(GdkEventFocus* aEvent) {
     }();
 
     if (shouldRollupMenus) {
-      CheckForRollup(0, 0, false, true);
+      RollupAllMenus();
     }
 
     if (RefPtr pm = nsXULPopupManager::GetInstance()) {
@@ -7404,19 +7404,24 @@ bool nsWindow::CheckForRollup(gdouble aMouseX, gdouble aMouseY, bool aIsWheel,
     return false;
   }
 
-  bool retVal = false;
   auto* currentPopup =
       (GdkWindow*)rollupWidget->GetNativeData(NS_NATIVE_WINDOW);
-  if (aAlwaysRollup || !is_mouse_in_window(currentPopup, aMouseX, aMouseY)) {
+  if (!aAlwaysRollup && is_mouse_in_window(currentPopup, aMouseX, aMouseY)) {
-    bool rollup = true;
+    return false;
-    if (aIsWheel) {
-      rollup = rollupListener->ShouldRollupOnMouseWheelEvent();
-      retVal = rollupListener->ShouldConsumeOnMouseWheelEvent();
   }
+  bool retVal = false;
+  if (aIsWheel) {
+    retVal = rollupListener->ShouldConsumeOnMouseWheelEvent();
+    if (!rollupListener->ShouldRollupOnMouseWheelEvent()) {
+      return retVal;
+    }
+  }
+  LayoutDeviceIntPoint point;
+  nsIRollupListener::RollupOptions options{0,
+                                           nsIRollupListener::FlushViews::Yes};
   // if we're dealing with menus, we probably have submenus and
   // we don't want to rollup if the click is in a parent menu of
   // the current submenu
-    uint32_t popupsToRollup = UINT32_MAX;
   if (!aAlwaysRollup) {
     AutoTArray<nsIWidget*, 5> widgetChain;
     uint32_t sameTypeCount =
@@ -7425,33 +7430,27 @@ bool nsWindow::CheckForRollup(gdouble aMouseX, gdouble aMouseY, bool aIsWheel,
       nsIWidget* widget = widgetChain[i];
       auto* currWindow = (GdkWindow*)widget->GetNativeData(NS_NATIVE_WINDOW);
       if (is_mouse_in_window(currWindow, aMouseX, aMouseY)) {
-          // don't roll up if the mouse event occurred within a
+        // Don't roll up if the mouse event occurred within a menu of the same
-          // menu of the same type. If the mouse event occurred
+        // type.
-          // in a menu higher than that, roll up, but pass the
+        // If the mouse event occurred in a menu higher than that, roll up, but
-          // number of popups to Rollup so that only those of the
+        // pass the number of popups to Rollup so that only those of the same
-          // same type close up.
+        // type close up.
         if (i < sameTypeCount) {
-            rollup = false;
+          return retVal;
-          } else {
-            popupsToRollup = sameTypeCount;
         }
+        options.mCount = sameTypeCount;
         break;
       }
     }  // foreach parent menu widget
-    }    // if rollup listener knows about menus
+    if (!aIsWheel) {
-
-    // if we've determined that we should still rollup, do it.
-    bool usePoint = !aIsWheel && !aAlwaysRollup;
-    LayoutDeviceIntPoint point;
-    if (usePoint) {
       point = GdkEventCoordsToDevicePixels(aMouseX, aMouseY);
+      options.mPoint = &point;
     }
-    if (rollup &&
+  }
-        rollupListener->Rollup(popupsToRollup, true,
+
-                               usePoint ? &point : nullptr, nullptr)) {
+  if (rollupListener->Rollup(options)) {
     retVal = true;
   }
-  }
   return retVal;
 }
 

widget/gtk/nsWindow.h

@@ -504,7 +504,8 @@ class nsWindow final : public nsBaseWidget {
                        const mozilla::LayoutDeviceIntPoint& aRefPoint);
   bool CheckForRollup(gdouble aMouseX, gdouble aMouseY, bool aIsWheel,
                       bool aAlwaysRollup);
-  void CheckForRollupDuringGrab() { CheckForRollup(0, 0, false, true); }
+  void RollupAllMenus() { CheckForRollup(0, 0, false, true); }
+  void CheckForRollupDuringGrab() { RollupAllMenus(); }
 
   bool GetDragInfo(mozilla::WidgetMouseEvent* aMouseEvent, GdkWindow** aWindow,
                    gint* aButton, gint* aRootX, gint* aRootY);

widget/nsBaseDragService.cpp

@@ -567,7 +567,7 @@ nsBaseDragService::EndDragSession(bool aDoneDrag, uint32_t aKeyModifiers) {
   if (mDragPopup) {
     nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
     if (pm) {
-      pm->HidePopup(mDragPopup, false, true, false, false);
+      pm->HidePopup(mDragPopup, {HidePopupOption::DeselectMenu});
     }
   }
 

widget/nsIRollupListener.h

@@ -16,26 +16,29 @@ class nsIWidget;
 
 class nsIRollupListener {
  public:
+  enum class FlushViews : bool { No, Yes };
+  struct RollupOptions {
+    // aCount is the number of popups in a chain to close. If this is
+    // zero, then all popups are closed.
+    uint32_t mCount = 0;
+    // If this is true, then views should be flushed after the rollup.
+    FlushViews mFlush = FlushViews::No;
+    // This is the mouse pointer position where the event that triggered the
+    // rollup occurred, which may be nullptr.
+    const mozilla::LayoutDeviceIntPoint* mPoint = nullptr;
+  };
+
   /**
    * Notifies the object to rollup, optionally returning the node that
-   * was just rolled up.
+   * was just rolled up in aLastRolledUp, if non-null.
    *
-   * If aFlush is true, then views should be flushed after the rollup.
+   * aLastRolledUp is not addrefed.
-   *
-   * aPoint is the mouse pointer position where the event that triggered the
-   * rollup occurred, which may be nullptr.
-   *
-   * aCount is the number of popups in a chain to close. If this is
-   * UINT32_MAX, then all popups are closed.
-   * If aLastRolledUp is non-null, it will be set to the last rolled up popup,
-   * if this is supported. aLastRolledUp is not addrefed.
    *
    * Returns true if the event that the caller is processing should be consumed.
    */
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
-  virtual bool Rollup(uint32_t aCount, bool aFlush,
+  virtual bool Rollup(const RollupOptions&,
-                      const mozilla::LayoutDeviceIntPoint* aPoint,
+                      nsIContent** aLastRolledUp = nullptr) = 0;
-                      nsIContent** aLastRolledUp) = 0;
 
   /**
    * Asks the RollupListener if it should rollup on mouse wheel events

widget/windows/nsWindow.cpp

@@ -7414,7 +7414,7 @@ void nsWindow::OnDestroy() {
     rollupWidget = rollupListener->GetRollupWidget();
   }
   if (this == rollupWidget) {
-    if (rollupListener) rollupListener->Rollup(0, false, nullptr, nullptr);
+    rollupListener->Rollup({});
     CaptureRollupEvents(false);
   }
 
@@ -8408,6 +8408,11 @@ bool nsWindow::DealWithPopups(HWND aWnd, UINT aMessage, WPARAM aWParam,
   // Only need to deal with the last rollup for left mouse down events.
   NS_ASSERTION(!nsAutoRollup::GetLastRollup(), "last rollup is null");
 
+  nsIRollupListener::RollupOptions rollupOptions{
+      popupsToRollup,
+      nsIRollupListener::FlushViews::Yes,
+  };
+
   if (nativeMessage == WM_TOUCH || nativeMessage == WM_LBUTTONDOWN ||
       nativeMessage == WM_POINTERDOWN) {
     LayoutDeviceIntPoint pos;
@@ -8425,13 +8430,12 @@ bool nsWindow::DealWithPopups(HWND aWnd, UINT aMessage, WPARAM aWParam,
       pos = LayoutDeviceIntPoint(pt.x, pt.y);
     }
 
-    nsIContent* lastRollup;
+    rollupOptions.mPoint = &pos;
-    consumeRollupEvent =
+    nsIContent* lastRollup = nullptr;
-        rollupListener->Rollup(popupsToRollup, true, &pos, &lastRollup);
+    consumeRollupEvent = rollupListener->Rollup(rollupOptions, &lastRollup);
     nsAutoRollup::SetLastRollup(lastRollup);
   } else {
-    consumeRollupEvent =
+    consumeRollupEvent = rollupListener->Rollup(rollupOptions);
-        rollupListener->Rollup(popupsToRollup, true, nullptr, nullptr);
   }
 
   // Tell hook to stop processing messages