Bug 401627, better way of handling click to close menu so that it doesn't reopen the menu again, r+sr=roc

layout/forms/nsComboboxControlFrame.cpp

@@ -1301,8 +1301,11 @@ nsComboboxControlFrame::GetAdditionalChildListName(PRInt32 aIndex) const
   //nsIRollupListener
 //----------------------------------------------------------------------
 NS_IMETHODIMP 
-nsComboboxControlFrame::Rollup()
+nsComboboxControlFrame::Rollup(nsIContent** aLastRolledUp)
 {
+  if (aLastRolledUp)
+    *aLastRolledUp = nsnull;
+
   if (mDroppedDown) {
     nsWeakFrame weakFrame(this);
     mListControlFrame->AboutToRollup(); // might destroy us

layout/forms/nsComboboxControlFrame.h

@@ -194,7 +194,7 @@ public:
    * Hide the dropdown menu and stop capturing mouse events.
    * @note This method might destroy |this|.
    */
-  NS_IMETHOD Rollup();
+  NS_IMETHOD Rollup(nsIContent** aLastRolledUp);
   /**
    * A combobox should roll up if a mousewheel event happens outside of
    * the popup area.

layout/xul/base/src/nsIMenuParent.h

@@ -85,10 +85,6 @@ public:
   // cleared. This should return true if the menu should be deselected
   // by the caller.
   virtual PRBool MenuClosed() = 0;
-
-  // return true if aMenuFrame is the menu that was recently closed. The
-  // recently closed menu state is cleared by this method.
-  virtual PRBool IsRecentlyClosed(nsMenuFrame* aMenuFrame) = 0;
 };
 
 #endif

layout/xul/base/src/nsMenuBarFrame.cpp

@@ -86,7 +86,6 @@ nsMenuBarFrame::nsMenuBarFrame(nsIPresShell* aShell, nsStyleContext* aContext):
     mStayActive(PR_FALSE),
     mIsActive(PR_FALSE),
     mCurrentMenu(nsnull),
-    mRecentlyClosedMenu(nsnull),
     mTarget(nsnull),
     mCaretWasVisible(PR_FALSE)
 {
@@ -338,7 +337,6 @@ nsMenuBarFrame::SetCurrentMenuItem(nsMenuFrame* aMenuItem)
     aMenuItem->SelectMenu(PR_TRUE);
 
   mCurrentMenu = aMenuItem;
-  mRecentlyClosedMenu = nsnull;
 
   return NS_OK;
 }
@@ -460,7 +458,6 @@ nsMenuBarFrame::MenuClosed()
 {
   SetActive(PR_FALSE);
   if (!mIsActive && mCurrentMenu) {
-    SetRecentlyClosed(mCurrentMenu);
     mCurrentMenu->SelectMenu(PR_FALSE);
     mCurrentMenu = nsnull;
     return PR_TRUE;

layout/xul/base/src/nsMenuBarFrame.h

@@ -79,19 +79,6 @@ public:
 
   PRBool IsMenuOpen() { return mCurrentMenu && mCurrentMenu->IsOpen(); }
 
-  // return true if aMenuFrame was the recently closed menu, clearing the
-  // the recent menu state in the process.
-  PRBool IsRecentlyClosed(nsMenuFrame* aMenuFrame)
-  {
-    PRBool match = (aMenuFrame == mRecentlyClosedMenu);
-    mRecentlyClosedMenu = nsnull;
-    return match;
-  }
-  void SetRecentlyClosed(nsMenuFrame* aRecentlyClosedMenu)
-  {
-    mRecentlyClosedMenu = aRecentlyClosedMenu;
-  }
-
   void InstallKeyboardNavigator();
   void RemoveKeyboardNavigator();
 
@@ -150,12 +137,6 @@ protected:
   // be null if no menu is active.
   nsMenuFrame* mCurrentMenu;
 
-  // When a menu is closed by clicking the menu label, the menu is rolled up
-  // and the mouse event is fired at the menu. The menu that was closed is
-  // stored here, to avoid having it reopen again during the mouse event.
-  // This is OK to be a weak reference as it is never dereferenced.
-  nsMenuFrame* mRecentlyClosedMenu;
-
   nsIDOMEventTarget* mTarget;
 
 private:

layout/xul/base/src/nsMenuBarListener.cpp

@@ -193,8 +193,6 @@ nsMenuBarListener::KeyUp(nsIDOMEvent* aKeyEvent)
 nsresult
 nsMenuBarListener::KeyPress(nsIDOMEvent* aKeyEvent)
 {
-  mMenuBarFrame->SetRecentlyClosed(nsnull);
-
   // if event has already been handled, bail
   nsCOMPtr<nsIDOMNSUIEvent> uiEvent ( do_QueryInterface(aKeyEvent) );
   if ( uiEvent ) {
@@ -384,7 +382,6 @@ nsMenuBarListener::MouseDown(nsIDOMEvent* aMouseEvent)
 nsresult 
 nsMenuBarListener::MouseUp(nsIDOMEvent* aMouseEvent)
 {
-  mMenuBarFrame->SetRecentlyClosed(nsnull);
   return NS_OK; // means I am NOT consuming event
 }
 

layout/xul/base/src/nsMenuFrame.cpp

@@ -557,7 +557,7 @@ nsMenuFrame::ToggleMenuState()
 {
   if (IsOpen())
     CloseMenu(PR_FALSE);
-  else if (!mMenuParent || !mMenuParent->IsRecentlyClosed(this))
+  else
     OpenMenu(PR_FALSE);
 }
 

layout/xul/base/src/nsMenuPopupFrame.h

@@ -156,8 +156,6 @@ public:
 
   virtual PRBool MenuClosed() { return PR_TRUE; }
 
-  virtual PRBool IsRecentlyClosed(nsMenuFrame* aMenuFrame) { return PR_FALSE; }
-
   NS_IMETHOD GetWidget(nsIWidget **aWidget);
 
   // The dismissal listener gets created and attached to the window.

layout/xul/base/src/nsXULPopupManager.cpp

@@ -158,11 +158,30 @@ nsXULPopupManager::GetInstance()
 }
 
 NS_IMETHODIMP
-nsXULPopupManager::Rollup()
+nsXULPopupManager::Rollup(nsIContent** aLastRolledUp)
 {
+  if (aLastRolledUp)
+    *aLastRolledUp = nsnull;
+
   nsMenuChainItem* item = GetTopVisibleMenu();
-  if (item)
+  if (item) {
+    if (aLastRolledUp) {
+      // we need to get the popup that will be closed last, so that
+      // widget can keep track of it so it doesn't reopen if a mouse
+      // down event is going to processed.
+      // Keep going up the menu chain to get the first level menu. This will
+      // be the one that closes up last. It's possible that this menu doesn't
+      // end up closing because the popuphiding event was cancelled, but in
+      // that case we don't need to deal with the menu reopening as it will
+      // already still be open.
+      nsMenuChainItem* first = item;
+      while (first->GetParent())
+        first = first->GetParent();
+      if (first)
+        NS_ADDREF(*aLastRolledUp = first->Content());
+    }
     HidePopup(item->Content(), PR_TRUE, PR_TRUE, PR_FALSE);
+  }
   return NS_OK;
 }
 
@@ -1090,6 +1109,12 @@ nsXULPopupManager::MayShowPopup(nsMenuPopupFrame* aPopup)
   if (state != ePopupClosed && state != ePopupInvisible)
     return PR_FALSE;
 
+  // if the popup was just rolled up, don't reopen it
+  nsCOMPtr<nsIWidget> widget;
+  aPopup->GetWidget(getter_AddRefs(widget));
+  if (widget && widget->GetLastRollup() == aPopup->GetContent())
+      return PR_FALSE;
+
   nsCOMPtr<nsISupports> cont = aPopup->PresContext()->GetContainer();
   nsCOMPtr<nsIDocShellTreeItem> dsti = do_QueryInterface(cont);
   if (!dsti)
@@ -1751,7 +1776,7 @@ nsXULPopupManager::KeyDown(nsIDOMEvent* aKeyEvent)
         // The access key just went down and no other
         // modifiers are already down.
         if (mCurrentMenu)
-          Rollup();
+          Rollup(nsnull);
         else if (mActiveMenuBar)
           mActiveMenuBar->MenuClosed();
       }
@@ -1826,7 +1851,7 @@ nsXULPopupManager::KeyPress(nsIDOMEvent* aKeyEvent)
   ) {
     // close popups or deactivate menubar when Tab or F10 are pressed
     if (item)
-      Rollup();
+      Rollup(nsnull);
     else if (mActiveMenuBar)
       mActiveMenuBar->MenuClosed();
   }
@@ -1919,15 +1944,9 @@ nsXULMenuCommandEvent::Run()
     // need to be hidden.
     nsIFrame* popupFrame = menuFrame->GetParent();
     while (popupFrame) {
-      // If the menu is a descendant of a menubar, clear the recently closed
+      if (popupFrame->GetType() == nsGkAtoms::menuPopupFrame) {
-      // state. Break out afterwards, as the menubar is the top level of a
-      // menu hierarchy.
-      if (popupFrame->GetType() == nsGkAtoms::menuBarFrame) {
-        (static_cast<nsMenuBarFrame *>(popupFrame))->SetRecentlyClosed(nsnull);
-        break;
-      }
-      else if (!popup && popupFrame->GetType() == nsGkAtoms::menuPopupFrame) {
         popup = popupFrame->GetContent();
+        break;
       }
       popupFrame = popupFrame->GetParent();
     }

toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

@@ -657,8 +657,11 @@ nsAutoCompleteController::OnSearchResult(nsIAutoCompleteSearch *aSearch, nsIAuto
 //// nsIRollupListener
 
 NS_IMETHODIMP
-nsAutoCompleteController::Rollup()
+nsAutoCompleteController::Rollup(nsIContent** aLastRolledUp)
 {
+  if (aLastRolledUp)
+    *aLastRolledUp = nsnull;
+
   ClearSearchTimer();
   ClearResults();
   ClosePopup();

widget/public/nsIRollupListener.idl

@@ -41,14 +41,17 @@
 
 #include "nsISupports.idl"
 
-[uuid(23C2BA03-6C76-11d3-96ED-0060B0FB9956)]
+interface nsIContent;
+
+[uuid(ee6efe03-77dc-4aac-a6a8-905731a1796e)]
 interface nsIRollupListener : nsISupports
 {
   /**
-   * Notifies the object to rollup
+   * Notifies the object to rollup, optionally returning the node that
+   * was just rolled up.
    * @result NS_Ok if no errors
    */
-  void Rollup();
+  nsIContent Rollup();
 
   /**
    * Asks the RollupListener if it should rollup on mousevents

widget/public/nsIWidget.h

@@ -63,6 +63,7 @@ struct  nsColorMap;
 class   imgIContainer;
 class   gfxASurface;
 class   nsIMouseListener;
+class   nsIContent;
 
 /**
  * Callback function that processes events.
@@ -94,10 +95,10 @@ typedef nsEventStatus (*PR_CALLBACK EVENT_CALLBACK)(nsGUIEvent *event);
 #define NS_NATIVE_PLUGIN_PORT_CG    101
 #endif
 
-// 3B4E560A-11E6-4EBD-B987-35385624970D
+// 092c37e8-2806-4ebc-b04b-e0bb624ce0d4
 #define NS_IWIDGET_IID \
-{ 0x3B4E560A, 0x11E6, 0x4EBD, \
+{ 0x092c37e8, 0x2806, 0x4ebc, \
-  { 0xB9, 0x87, 0x35, 0x38, 0x56, 0x24, 0x97, 0x0D } }
+  { 0xb0, 0x4b, 0xe0, 0xbb, 0x62, 0x4c, 0xe0, 0xd4 } }
 
 // Hide the native window systems real window type so as to avoid
 // including native window system types and api's. This is necessary
@@ -1039,6 +1040,10 @@ class nsIWidget : public nsISupports {
      */
     virtual gfxASurface *GetThebesSurface() = 0;
 
+    /**
+     * Return the popup that was last rolled up, or null if there isn't one.
+     */
+    virtual nsIContent* GetLastRollup() = 0;
 
 protected:
     // keep the list of children.  We also keep track of our siblings.

widget/src/cocoa/nsAppShell.mm

@@ -702,7 +702,7 @@ nsAppShell::AfterProcessNextEvent(nsIThreadInternal *aThread,
   NSString *sender = [aNotification object];
   if (!sender || ![sender isEqualToString:@"org.mozilla.gecko.PopupWindow"]) {
     if (gRollupListener && gRollupWidget)
-      gRollupListener->Rollup();
+      gRollupListener->Rollup(nsnull);
   }
 }
 

widget/src/cocoa/nsChildView.mm

@@ -2524,7 +2524,7 @@ class nsNonNativeContextMenuEvent : public nsRunnable {
 
       // if we've determined that we should still rollup, do it.
       if (rollup) {
-        gRollupListener->Rollup();
+        gRollupListener->Rollup(nsnull);
         retVal = PR_TRUE;
       }
     }

widget/src/cocoa/nsCocoaWindow.mm

@@ -80,7 +80,7 @@ NS_IMPL_ISUPPORTS_INHERITED1(nsCocoaWindow, Inherited, nsPIWidgetCocoa)
 static void RollUpPopups()
 {
   if (gRollupListener && gRollupWidget)
-    gRollupListener->Rollup();
+    gRollupListener->Rollup(nsnull);
 }
 
 

widget/src/cocoa/nsMenuX.mm

@@ -1118,7 +1118,7 @@ static pascal OSStatus MyMenuEventHandler(EventHandlerCallRef myHandler, EventRe
   }
   else if (kind == kEventMenuOpening || kind == kEventMenuClosed) {
     if (kind == kEventMenuOpening && gRollupListener && gRollupWidget) {
-      gRollupListener->Rollup();
+      gRollupListener->Rollup(nsnull);
       return userCanceledErr;
     }
     

widget/src/cocoa/nsToolkit.mm

@@ -233,7 +233,7 @@ static CGEventRef EventTapCallback(CGEventTapProxy proxy, CGEventType type, CGEv
   // so would break the corresponding context menu).
   if (NSPointInRect(screenLocation, [ctxMenuWindow frame]))
     return event;
-  gRollupListener->Rollup();
+  gRollupListener->Rollup(nsnull);
   return event;
 }
 

widget/src/gtk2/nsWindow.cpp

@@ -431,7 +431,7 @@ nsWindow::Destroy(void)
     nsCOMPtr<nsIWidget> rollupWidget = do_QueryReferent(gRollupWindow);
     if (static_cast<nsIWidget *>(this) == rollupWidget.get()) {
         if (gRollupListener)
-            gRollupListener->Rollup();
+            gRollupListener->Rollup(nsnull);
         gRollupWindow = nsnull;
         gRollupListener = nsnull;
     }
@@ -4050,7 +4050,7 @@ check_for_rollup(GdkWindow *aWindow, gdouble aMouseX, gdouble aMouseY,
 
             // if we've determined that we should still rollup, do it.
             if (rollup) {
-                gRollupListener->Rollup();
+                gRollupListener->Rollup(nsnull);
                 retVal = PR_TRUE;
             }
         }

widget/src/os2/nsFrameWindow.cpp

@@ -331,7 +331,7 @@ MRESULT EXPENTRY fnwpFrame( HWND hwnd, ULONG msg, MPARAM mp1, MPARAM mp2)
           msg == WM_BUTTON1DOWN || msg == WM_BUTTON2DOWN || msg == WM_BUTTON3DOWN) {
          // Rollup if the event is outside the popup
          if (PR_FALSE == nsWindow::EventIsInsideWindow((nsWindow*)gRollupWidget)) {
-            gRollupListener->Rollup();
+            gRollupListener->Rollup(nsnull);
 
             // if we are supposed to be consuming events and it is
             // a Mouse Button down, let it go through

widget/src/os2/nsWindow.cpp

@@ -664,7 +664,8 @@ nsWindow :: DealWithPopups ( ULONG inMsg, MRESULT* outResult )
 
       // if we've determined that we should still rollup everything, do it.
       if ( rollup ) {
-        gRollupListener->Rollup();
+        // only need to deal with the last rollup for left mouse down events.
+        gRollupListener->Rollup(inMsg == WM_LBUTTONDOWN ? &mLastRollup : nsnull);
 
         // return TRUE tells Windows that the event is consumed, 
         // false allows the event to be dispatched
@@ -723,6 +724,8 @@ BOOL bothFromSameWindow( HWND hwnd1, HWND hwnd2 )
 //-------------------------------------------------------------------------
 MRESULT EXPENTRY fnwpNSWindow( HWND hwnd, ULONG msg, MPARAM mp1, MPARAM mp2)
 {
+   nsAutoRollup autoRollup;
+
    MRESULT popupHandlingResult;
    if( nsWindow::DealWithPopups(msg, &popupHandlingResult) )
       return popupHandlingResult;
@@ -767,7 +770,7 @@ MRESULT EXPENTRY fnwpNSWindow( HWND hwnd, ULONG msg, MPARAM mp1, MPARAM mp2)
          if( !mp2 && 
              !bothFromSameWindow( ((nsWindow*)gRollupWidget)->GetMainWindow(), 
                                   (HWND)mp1) ) {
-            gRollupListener->Rollup();
+            gRollupListener->Rollup(nsnull);
          }
       }
    }
@@ -1120,7 +1123,7 @@ NS_METHOD nsWindow::Destroy()
     // the rollup widget, rollup and turn off capture.
     if (this == gRollupWidget) {
       if (gRollupListener) {
-        gRollupListener->Rollup();
+        gRollupListener->Rollup(nsnull);
       }
       CaptureRollupEvents(nsnull, PR_FALSE, PR_TRUE);
     }

widget/src/photon/nsWindow.cpp

@@ -651,13 +651,13 @@ int nsWindow::WindowWMHandler( PtWidget_t *widget, void *data, PtCallbackInfo_t
 		case Ph_WM_CONSWITCH:
 			gConsoleRectValid = PR_FALSE; /* force a call tp PhWindowQueryVisible() next time, since we might have moved this window into a different console */
       /* rollup the menus */
-      if( gRollupWidget && gRollupListener ) gRollupListener->Rollup();
+      if( gRollupWidget && gRollupListener ) gRollupListener->Rollup(nsnull);
 			break;
 
 		case Ph_WM_FOCUS:
 			if( we->event_state == Ph_WM_EVSTATE_FOCUSLOST ) {
       	/* rollup the menus */
-      	if( gRollupWidget && gRollupListener ) gRollupListener->Rollup();
+      	if( gRollupWidget && gRollupListener ) gRollupListener->Rollup(nsnull);
 
 				if( sFocusWidget ) sFocusWidget->DispatchStandardEvent(NS_DEACTIVATE);
 				}
@@ -902,7 +902,7 @@ NS_METHOD nsWindow::Move( PRInt32 aX, PRInt32 aY ) {
 int nsWindow::MenuRegionCallback( PtWidget_t *widget, void *data, PtCallbackInfo_t *cbinfo ) {
 	if( gRollupWidget && gRollupListener ) {
 		/* rollup the menu */
-		gRollupListener->Rollup();
+		gRollupListener->Rollup(nsnull);
 		}
 	return Pt_CONTINUE;
 	}

widget/src/windows/nsWindow.cpp

@@ -1232,6 +1232,10 @@ BOOL nsWindow::SetNSWindowPtr(HWND aWnd, nsWindow * ptr) {
 //-------------------------------------------------------------------------
 LRESULT CALLBACK nsWindow::WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
 {
+  // create this here so that we store the last rolled up popup until after
+  // the event has been processed.
+  nsAutoRollup autoRollup;
+
   LRESULT popupHandlingResult;
   if ( DealWithPopups(hWnd, msg, wParam, lParam, &popupHandlingResult) )
     return popupHandlingResult;
@@ -1526,7 +1530,7 @@ NS_METHOD nsWindow::Destroy()
   // the rollup widget, rollup and turn off capture.
   if ( this == gRollupWidget ) {
     if ( gRollupListener )
-      gRollupListener->Rollup();
+      gRollupListener->Rollup(nsnull);
     CaptureRollupEvents(nsnull, PR_FALSE, PR_TRUE);
   }
 
@@ -7670,6 +7674,7 @@ VOID CALLBACK nsWindow::HookTimerForPopups(HWND hwnd, UINT uMsg, UINT idEvent, D
     // Note: DealWithPopups does the check to make sure that
     // gRollupListener and gRollupWidget are not NULL
     LRESULT popupHandlingResult;
+    nsAutoRollup autoRollup;
     DealWithPopups(gRollupMsgWnd, gRollupMsgId, 0, 0, &popupHandlingResult);
     gRollupMsgId = 0;
     gRollupMsgWnd = NULL;
@@ -7770,7 +7775,8 @@ nsWindow :: DealWithPopups ( HWND inWnd, UINT inMsg, WPARAM inWParam, LPARAM inL
       else
 #endif
       if ( rollup ) {
-        gRollupListener->Rollup();
+        // only need to deal with the last rollup for left mouse down events.
+        gRollupListener->Rollup(inMsg == WM_LBUTTONDOWN ? &mLastRollup : nsnull);
 
         // Tell hook to stop processing messages
         gProcessHook = PR_FALSE;

widget/src/xpwidgets/nsBaseWidget.cpp

@@ -46,6 +46,7 @@
 #include "nsIScreenManager.h"
 #include "nsAppDirectoryServiceDefs.h"
 #include "nsISimpleEnumerator.h"
+#include "nsIContent.h"
 
 #ifdef DEBUG
 #include "nsIServiceManager.h"
@@ -62,10 +63,27 @@ static PRBool debug_InSecureKeyboardInputMode = PR_FALSE;
 static PRInt32 gNumWidgets;
 #endif
 
+nsIContent* nsBaseWidget::mLastRollup = nsnull;
+
 // nsBaseWidget
 NS_IMPL_ISUPPORTS1(nsBaseWidget, nsIWidget)
 
 
+nsAutoRollup::nsAutoRollup()
+{
+  // remember if mLastRollup was null, and only clear it upon destruction
+  // if so. This prevents recursive usage of nsAutoRollup from clearing
+  // mLastRollup when it shouldn't.
+  wasClear = !nsBaseWidget::mLastRollup;
+}
+
+nsAutoRollup::~nsAutoRollup()
+{
+  if (nsBaseWidget::mLastRollup && wasClear) {
+    NS_RELEASE(nsBaseWidget::mLastRollup);
+  }
+}
+
 //-------------------------------------------------------------------------
 //
 // nsBaseWidget constructor

widget/src/xpwidgets/nsBaseWidget.h

@@ -49,6 +49,9 @@
 #include "nsCOMPtr.h"
 #include "nsGUIEvent.h"
 
+class nsIContent;
+class nsAutoRollup;
+
 /**
  * Common widget implementation used as base class for native
  * or crossplatform implementations of Widgets. 
@@ -60,6 +63,7 @@
 
 class nsBaseWidget : public nsIWidget
 {
+  friend class nsAutoRollup;
 
 public:
   nsBaseWidget();
@@ -149,6 +153,11 @@ protected:
                                      nsIToolkit *aToolkit,
                                      nsWidgetInitData *aInitData);
 
+  virtual nsIContent* GetLastRollup()
+  {
+    return mLastRollup;
+  }
+
 protected: 
   void*             mClientData;
   EVENT_CALLBACK    mEventCallback;
@@ -170,6 +179,10 @@ protected:
   nsRect*           mOriginalBounds;
   PRInt32           mZIndex;
   nsSizeMode        mSizeMode;
+
+  // the last rolled up popup. Only set this when an nsAutoRollup is in scope,
+  // so it can be cleared automatically.
+  static nsIContent* mLastRollup;
     
     // Enumeration of the methods which are accessible on the "main GUI thread"
     // via the CallMethod(...) mechanism...
@@ -211,4 +224,26 @@ protected:
 #endif
 };
 
+// A situation can occur when a mouse event occurs over a menu label while the
+// menu popup is already open. The expected behaviour is to close the popup.
+// This happens by calling nsIRollupListener::Rollup before the mouse event is
+// processed. However, in cases where the mouse event is not consumed, this
+// event will then get targeted at the menu label causing the menu to open
+// again. To prevent this, we store in mLastRollup a reference to the popup
+// that was closed during the Rollup call, and prevent this popup from
+// reopening while processing the mouse event.
+// mLastRollup should only be set while an nsAutoRollup is in scope;
+// when it goes out of scope mLastRollup is cleared automatically.
+// As mLastRollup is static, it can be retrieved by calling
+// nsIWidget::GetLastRollup on any widget.
+class nsAutoRollup
+{
+  PRBool wasClear;
+
+  public:
+
+  nsAutoRollup();
+  ~nsAutoRollup();
+};
+
 #endif // nsBaseWidget_h__