Bug 596723, Don't consume clicks outside of arrow panels by default, always consume the clicks on anchors of all popups, r=dao,neil
authorNeil Deakin <neil@mozilla.com>
Mon, 04 Nov 2013 11:22:24 -0500
changeset 153360 07ea7b11adef70b40eefdfe4ed5ac8434051ee12
parent 153359 214adf185651b6ab5ff63a9c01ec8ea2cf04621e
child 153361 09725c69fd76ad2a21edf3bb02264c89fd2adc18
push id35785
push userneil@mozilla.com
push dateMon, 04 Nov 2013 16:22:39 +0000
treeherdermozilla-inbound@07ea7b11adef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao, neil
bugs596723
milestone28.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 596723, Don't consume clicks outside of arrow panels by default, always consume the clicks on anchors of all popups, r=dao,neil
browser/base/content/browser.xul
browser/components/downloads/content/downloadsOverlay.xul
layout/forms/nsComboboxControlFrame.cpp
layout/forms/nsComboboxControlFrame.h
layout/xul/base/public/nsXULPopupManager.h
layout/xul/base/src/nsXULPopupManager.cpp
toolkit/content/widgets/popup.xml
widget/cocoa/nsAppShell.mm
widget/cocoa/nsChildView.mm
widget/cocoa/nsCocoaWindow.mm
widget/cocoa/nsMenuX.mm
widget/cocoa/nsToolkit.mm
widget/gtk/nsWindow.cpp
widget/nsIRollupListener.h
widget/windows/nsWindow.cpp
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -134,17 +134,16 @@
       <description/>
     </panel>
 
     <panel id="editBookmarkPanel"
            type="arrow"
            footertype="promobox"
            orient="vertical"
            ignorekeys="true"
-           consumeoutsideclicks="true"
            hidden="true"
            onpopupshown="StarUI.panelShown(event);"
            aria-labelledby="editBookmarkPanelTitle">
       <row id="editBookmarkPanelHeader" align="center" hidden="true">
         <vbox align="center">
           <image id="editBookmarkPanelStarIcon"/>
         </vbox>
         <vbox>
@@ -183,32 +182,30 @@
 #endif
       </hbox>
     </panel>
 
     <!-- UI tour experience -->
     <panel id="UITourTooltip"
            type="arrow"
            hidden="true"
-           consumeoutsideclicks="false"
            noautofocus="true"
            align="start"
            orient="vertical"
            role="alert">
       <label id="UITourTooltipTitle" flex="1"/>
       <description id="UITourTooltipDescription" flex="1"/>
     </panel>
     <html:div id="UITourHighlightContainer" style="position:relative">
       <html:div id="UITourHighlight"></html:div>
     </html:div>
 
     <panel id="socialActivatedNotification"
            type="arrow"
            hidden="true"
-           consumeoutsideclicks="true"
            align="start"
            orient="horizontal"
            role="alert">
       <image id="social-activation-icon" class="popup-notification-icon"/>
       <vbox flex="1">
         <description id="social-activation-message" class="popup-notification-description">&social.activated.description;</description>
         <spacer flex="1"/>
         <hbox pack="start" align="center" class="popup-notification-button-container">
@@ -229,17 +226,16 @@
     </panel>
 
     <panel id="social-share-panel"
            class="social-panel"
            type="arrow"
            orient="horizontal"
            onpopupshowing="SocialShare.onShowing()"
            onpopuphidden="SocialShare.onHidden()"
-           consumeoutsideclicks="true"
            hidden="true">
       <vbox class="social-share-toolbar">
         <vbox id="social-share-provider-buttons" flex="1"/>
       </vbox>
     </panel>
 
     <panel id="social-notification-panel"
            class="social-panel"
@@ -250,17 +246,16 @@
            class="social-panel"
            onpopupshown="SocialFlyout.onShown()"
            onpopuphidden="SocialFlyout.onHidden()"
            side="right"
            type="arrow"
            hidden="true"
            flip="slide"
            rolluponmousewheel="true"
-           consumeoutsideclicks="false"
            noautofocus="true"
            position="topcenter topright"/>
 
     <menupopup id="toolbar-context-menu"
                onpopupshowing="onViewToolbarsPopupShowing(event);">
       <menuseparator/>
       <menuitem command="cmd_ToggleTabsOnTop"
                 type="checkbox"
@@ -308,17 +303,16 @@
 
     <menupopup id="placesContext"/>
 
     <!-- Popup for site identity information -->
     <panel id="identity-popup"
            type="arrow"
            hidden="true"
            noautofocus="true"
-           consumeoutsideclicks="true"
            onpopupshown="if (event.target == this)
                            gIdentityHandler.onPopupShown(event);"
            orient="vertical"
            level="top">
       <hbox id="identity-popup-container" align="top">
         <image id="identity-popup-icon"/>
         <vbox id="identity-popup-content-box">
           <label id="identity-popup-brandName"
--- a/browser/components/downloads/content/downloadsOverlay.xul
+++ b/browser/components/downloads/content/downloadsOverlay.xul
@@ -41,17 +41,16 @@
          readers, we use a label on the panel instead of the anchor because the
          panel can also be displayed without an anchor. -->
     <panel id="downloadsPanel"
            aria-label="&downloads.title;"
            role="group"
            type="arrow"
            orient="vertical"
            level="top"
-           consumeoutsideclicks="true"
            onpopupshown="DownloadsPanel.onPopupShown(event);"
            onpopuphidden="DownloadsPanel.onPopupHidden(event);">
       <!-- The following popup menu should be a child of the panel element,
            otherwise flickering may occur when the cursor is moved over the area
            of a disabled menu item that overlaps the panel.  See bug 492960. -->
       <menupopup id="downloadsContextMenu"
                  class="download-state">
         <menuitem command="downloadsCmd_pauseResume"
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -1389,17 +1389,17 @@ nsComboboxControlFrame::SetInitialChildL
   }
   return rv;
 }
 
 //----------------------------------------------------------------------
   //nsIRollupListener
 //----------------------------------------------------------------------
 bool
-nsComboboxControlFrame::Rollup(uint32_t aCount, nsIContent** aLastRolledUp)
+nsComboboxControlFrame::Rollup(uint32_t aCount, const nsIntPoint* pos, nsIContent** aLastRolledUp)
 {
   if (!mDroppedDown)
     return false;
 
   nsWeakFrame weakFrame(this);
   mListControlFrame->AboutToRollup(); // might destroy us
   if (!weakFrame.IsAlive())
     return true;
--- a/layout/forms/nsComboboxControlFrame.h
+++ b/layout/forms/nsComboboxControlFrame.h
@@ -159,17 +159,17 @@ public:
   NS_IMETHOD OnOptionSelected(int32_t aIndex, bool aSelected) MOZ_OVERRIDE;
   NS_IMETHOD OnSetSelectedIndex(int32_t aOldIndex, int32_t aNewIndex) MOZ_OVERRIDE;
 
   //nsIRollupListener
   /**
    * Hide the dropdown menu and stop capturing mouse events.
    * @note This method might destroy |this|.
    */
-  virtual bool Rollup(uint32_t aCount, nsIContent** aLastRolledUp) MOZ_OVERRIDE;
+  virtual bool Rollup(uint32_t aCount, const nsIntPoint* pos, nsIContent** aLastRolledUp) MOZ_OVERRIDE;
   virtual void NotifyGeometryChange() MOZ_OVERRIDE;
 
   /**
    * A combobox should roll up if a mousewheel event happens outside of
    * the popup area.
    */
   virtual bool ShouldRollupOnMouseWheelEvent() MOZ_OVERRIDE
     { return true; }
--- a/layout/xul/base/public/nsXULPopupManager.h
+++ b/layout/xul/base/public/nsXULPopupManager.h
@@ -277,17 +277,17 @@ public:
   friend class nsXULMenuCommandEvent;
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIOBSERVER
   NS_DECL_NSITIMERCALLBACK
   NS_DECL_NSIDOMEVENTLISTENER
 
   // nsIRollupListener
-  virtual bool Rollup(uint32_t aCount, nsIContent** aLastRolledUp) MOZ_OVERRIDE;
+  virtual bool Rollup(uint32_t aCount, const nsIntPoint* pos, nsIContent** aLastRolledUp) MOZ_OVERRIDE;
   virtual bool ShouldRollupOnMouseWheelEvent() MOZ_OVERRIDE;
   virtual bool ShouldConsumeOnMouseWheelEvent() MOZ_OVERRIDE;
   virtual bool ShouldRollupOnMouseActivate() MOZ_OVERRIDE;
   virtual uint32_t GetSubmenuWidgetChain(nsTArray<nsIWidget*> *aWidgetChain) MOZ_OVERRIDE;
   virtual void NotifyGeometryChange() MOZ_OVERRIDE {}
   virtual nsIWidget* GetRollupWidget() MOZ_OVERRIDE;
 
   static nsXULPopupManager* sInstance;
--- a/layout/xul/base/src/nsXULPopupManager.cpp
+++ b/layout/xul/base/src/nsXULPopupManager.cpp
@@ -175,17 +175,17 @@ nsXULPopupManager::Observe(nsISupports *
 nsXULPopupManager*
 nsXULPopupManager::GetInstance()
 {
   MOZ_ASSERT(sInstance);
   return sInstance;
 }
 
 bool
-nsXULPopupManager::Rollup(uint32_t aCount, nsIContent** aLastRolledUp)
+nsXULPopupManager::Rollup(uint32_t aCount, const nsIntPoint* pos, nsIContent** aLastRolledUp)
 {
   bool consume = false;
 
   nsMenuChainItem* item = GetTopVisibleMenu();
   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
@@ -197,16 +197,30 @@ nsXULPopupManager::Rollup(uint32_t aCoun
       // already still be open.
       nsMenuChainItem* first = item;
       while (first->GetParent())
         first = first->GetParent();
       *aLastRolledUp = first->Content();
     }
 
     consume = item->Frame()->ConsumeOutsideClicks();
+    // If the click was over the anchor, always consume the click. This way,
+    // clicking on a menu doesn't reopen the menu.
+    if (!consume && pos) {
+      nsCOMPtr<nsIContent> anchor = item->Frame()->GetAnchor();
+      if (anchor && anchor->GetPrimaryFrame()) {
+        // It's possible that some other element is above the anchor at the same
+        // position, but the only thing that would happen is that the mouse
+        // 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.
+        if (anchor->GetPrimaryFrame()->GetScreenRect().Contains(*pos)) {
+          consume = true;
+        }
+      }
+    }
 
     // if a number of popups to close has been specified, determine the last
     // popup to close
     nsIContent* lastPopup = nullptr;
     if (aCount != UINT32_MAX) {
       nsMenuChainItem* last = item;
       while (--aCount && last->GetParent()) {
         last = last->GetParent();
@@ -2000,17 +2014,17 @@ nsXULPopupManager::HandleKeyboardEventWi
       break;
 
     case nsIDOMKeyEvent::DOM_VK_TAB:
 #ifndef XP_MACOSX
     case nsIDOMKeyEvent::DOM_VK_F10:
 #endif
       // close popups or deactivate menubar when Tab or F10 are pressed
       if (aTopVisibleMenuItem) {
-        Rollup(0, nullptr);
+        Rollup(0, nullptr, nullptr);
       } else if (mActiveMenuBar) {
         mActiveMenuBar->MenuClosed();
       }
       break;
 
     case nsIDOMKeyEvent::DOM_VK_ENTER:
     case nsIDOMKeyEvent::DOM_VK_RETURN: {
       // If there is a popup open, check if the current item needs to be opened.
@@ -2235,17 +2249,17 @@ nsXULPopupManager::KeyDown(nsIDOMKeyEven
         aKeyEvent->GetShiftKey(&shift);
       bool meta=false;
       if (menuAccessKey != nsIDOMKeyEvent::DOM_VK_META)
         aKeyEvent->GetMetaKey(&meta);
       if (!(ctrl || alt || shift || meta)) {
         // The access key just went down and no other
         // modifiers are already down.
         if (mPopups)
-          Rollup(0, nullptr);
+          Rollup(0, nullptr, nullptr);
         else if (mActiveMenuBar)
           mActiveMenuBar->MenuClosed();
       }
       aKeyEvent->PreventDefault();
     }
   }
 
   // Since a menu was open, stop propagation of the event to keep other event
--- a/toolkit/content/widgets/popup.xml
+++ b/toolkit/content/widgets/popup.xml
@@ -308,17 +308,17 @@
             currentFocus = currentFocus.parentNode;
           }
         }
       ]]></handler>
     </handlers>
   </binding>
 
   <binding id="arrowpanel" extends="chrome://global/content/bindings/popup.xml#panel">
-    <content flip="both" side="top" position="bottomcenter topleft">
+    <content flip="both" side="top" position="bottomcenter topleft" consumeoutsideclicks="false">
       <xul:box anonid="container" class="panel-arrowcontainer" flex="1"
                xbl:inherits="side,panelopen">
         <xul:box anonid="arrowbox" class="panel-arrowbox">
           <xul:image anonid="arrow" class="panel-arrow" xbl:inherits="side"/>
         </xul:box>
         <xul:box class="panel-arrowcontent" xbl:inherits="side,align,dir,orient,pack" flex="1">
           <children/>
           <xul:box class="panel-inner-arrowcontentfooter" xbl:inherits="footertype" hidden="true"/>
--- a/widget/cocoa/nsAppShell.mm
+++ b/widget/cocoa/nsAppShell.mm
@@ -947,17 +947,17 @@ nsAppShell::AfterProcessNextEvent(nsIThr
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   NSString *sender = [aNotification object];
   if (!sender || ![sender isEqualToString:@"org.mozilla.gecko.PopupWindow"]) {
     nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
     nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
     if (rollupWidget)
-      rollupListener->Rollup(0, nullptr);
+      rollupListener->Rollup(0, nullptr, nullptr);
   }
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 @end
 
 // We hook beginModalSessionForWindow: and endModalSession: in order to
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -3821,17 +3821,25 @@ NSEvent* gLastDragMouseDownEvent = nil;
           else {
             popupsToRollup = sameTypeCount;
           }
           break;
         }
       }
 
       if (shouldRollup) {
-        consumeEvent = (BOOL)rollupListener->Rollup(popupsToRollup, nullptr);
+        if ([theEvent type] == NSLeftMouseDown) {
+          NSPoint point = [NSEvent mouseLocation];
+          FlipCocoaScreenCoordinate(point);
+          nsIntPoint pos(point.x, point.y);
+          consumeEvent = (BOOL)rollupListener->Rollup(popupsToRollup, &pos, nullptr);
+        }
+        else {
+          consumeEvent = (BOOL)rollupListener->Rollup(popupsToRollup, nullptr, nullptr);
+        }
       }
     }
   }
 
   return consumeEvent;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NO);
 }
--- a/widget/cocoa/nsCocoaWindow.mm
+++ b/widget/cocoa/nsCocoaWindow.mm
@@ -92,17 +92,17 @@ NS_IMPL_ISUPPORTS_INHERITED1(nsCocoaWind
 
 static void RollUpPopups()
 {
   nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
   NS_ENSURE_TRUE_VOID(rollupListener);
   nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
   if (!rollupWidget)
     return;
-  rollupListener->Rollup(0, nullptr);
+  rollupListener->Rollup(0, nullptr, nullptr);
 }
 
 nsCocoaWindow::nsCocoaWindow()
 : mParent(nullptr)
 , mWindow(nil)
 , mDelegate(nil)
 , mSheetWindowParent(nil)
 , mPopupContentView(nil)
--- a/widget/cocoa/nsMenuX.mm
+++ b/widget/cocoa/nsMenuX.mm
@@ -829,17 +829,17 @@ nsresult nsMenuX::SetupIcon()
   // menus, but it also resolves many other problems.
   if (nsMenuX::sIndexingMenuLevel > 0)
     return;
 
   nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
   if (rollupListener) {
     nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
     if (rollupWidget) {
-      rollupListener->Rollup(0, nullptr);
+      rollupListener->Rollup(0, nullptr, nullptr);
       [menu cancelTracking];
       return;
     }
   }
   mGeckoMenu->MenuOpened();
 }
 
 - (void)menuDidClose:(NSMenu *)menu
--- a/widget/cocoa/nsToolkit.mm
+++ b/widget/cocoa/nsToolkit.mm
@@ -179,17 +179,17 @@ static CGEventRef EventTapCallback(CGEve
   NSWindow *ctxMenuWindow = (NSWindow*) rollupWidget->GetNativeData(NS_NATIVE_WINDOW);
   if (!ctxMenuWindow)
     return event;
   NSPoint screenLocation = ConvertCGGlobalToCocoaScreen(CGEventGetLocation(event));
   // Don't roll up the rollup widget if our mouseDown happens over it (doing
   // so would break the corresponding context menu).
   if (NSPointInRect(screenLocation, [ctxMenuWindow frame]))
     return event;
-  rollupListener->Rollup(0, nullptr);
+  rollupListener->Rollup(0, nullptr, nullptr);
   return event;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NULL);
 }
 
 // Cocoa Firefox's use of custom context menus requires that we explicitly
 // handle mouse events from other processes that the OS handles
 // "automatically" for native context menus -- mouseMoved events so that
--- a/widget/gtk/nsWindow.cpp
+++ b/widget/gtk/nsWindow.cpp
@@ -628,17 +628,17 @@ nsWindow::Destroy(void)
     g_signal_handlers_disconnect_by_func(gtk_settings_get_default(),
                                          FuncToGpointer(theme_changed_cb),
                                          this);
 
     nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
     if (rollupListener) {
         nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
         if (static_cast<nsIWidget *>(this) == rollupWidget) {
-            rollupListener->Rollup(0, nullptr);
+            rollupListener->Rollup(0, nullptr, nullptr);
         }
     }
 
     // dragService will be null after shutdown of the service manager.
     nsDragService *dragService = nsDragService::GetInstance();
     if (dragService && this == dragService->GetMostRecentDestWindow()) {
         dragService->ScheduleLeaveEvent();
     }
@@ -4779,17 +4779,19 @@ nsWindow::CheckForRollup(gdouble aMouseX
                     popupsToRollup = sameTypeCount;
                   }
                   break;
                 }
             } // foreach parent menu widget
         } // if rollup listener knows about menus
 
         // if we've determined that we should still rollup, do it.
-        if (rollup && rollupListener->Rollup(popupsToRollup, nullptr)) {
+        bool usePoint = !aIsWheel && !aAlwaysRollup;
+        nsIntPoint point(aMouseX, aMouseY);
+        if (rollup && rollupListener->Rollup(popupsToRollup, usePoint ? &point : nullptr, nullptr)) {
             retVal = true;
         }
     }
     return retVal;
 }
 
 /* static */
 bool
--- a/widget/nsIRollupListener.h
+++ b/widget/nsIRollupListener.h
@@ -6,32 +6,36 @@
 
 #ifndef __nsIRollupListener_h__
 #define __nsIRollupListener_h__
 
 #include "nsTArray.h"
 
 class nsIContent;
 class nsIWidget;
+class nsIntPoint;
 
 class nsIRollupListener {
  public: 
 
   /**
    * Notifies the object to rollup, optionally returning the node that
    * was just rolled up.
    *
+   * aPoint is the mouse pointer position where the event that triggered the
+   * rollup occurred, which may be NULL.
+   *
    * 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.
    */
-  virtual bool Rollup(uint32_t aCount, nsIContent** aLastRolledUp) = 0;
+  virtual bool Rollup(uint32_t aCount, const nsIntPoint* aPoint, nsIContent** aLastRolledUp) = 0;
 
   /**
    * Asks the RollupListener if it should rollup on mouse wheel events
    */
   virtual bool ShouldRollupOnMouseWheelEvent() = 0;
 
   /**
    * Asks the RollupListener if it should consume mouse wheel events
--- a/widget/windows/nsWindow.cpp
+++ b/widget/windows/nsWindow.cpp
@@ -6515,17 +6515,17 @@ void nsWindow::OnDestroy()
   // turn off capture.
   nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
   nsCOMPtr<nsIWidget> rollupWidget;
   if (rollupListener) {
     rollupWidget = rollupListener->GetRollupWidget();
   }
   if (this == rollupWidget) {
     if ( rollupListener )
-      rollupListener->Rollup(0, nullptr);
+      rollupListener->Rollup(0, nullptr, nullptr);
     CaptureRollupEvents(nullptr, false);
   }
 
   IMEHandler::OnDestroyWindow(this);
 
   // Turn off mouse trails if enabled.
   MouseTrailer* mtrailer = nsToolkit::gMouseTrailer;
   if (mtrailer) {
@@ -7337,19 +7337,31 @@ nsWindow::DealWithPopups(HWND inWnd, UIN
           }
         }
       }
     }
     // if we've still determined that we should still rollup everything, do it.
     else if (rollup) {
       // only need to deal with the last rollup for left mouse down events.
       NS_ASSERTION(!mLastRollup, "mLastRollup is null");
-      bool consumeRollupEvent =
-        rollupListener->Rollup(popupsToRollup, inMsg == WM_LBUTTONDOWN ? &mLastRollup : nullptr);
-      NS_IF_ADDREF(mLastRollup);
+
+      bool consumeRollupEvent;
+      if (inMsg == WM_LBUTTONDOWN) {
+        POINT pt;
+        pt.x = GET_X_LPARAM(inLParam);
+        pt.y = GET_Y_LPARAM(inLParam);
+        ::ClientToScreen(inWnd, &pt);
+        nsIntPoint pos(pt.x, pt.y);
+
+        consumeRollupEvent = rollupListener->Rollup(popupsToRollup, &pos, &mLastRollup);
+        NS_IF_ADDREF(mLastRollup);
+      }
+      else {
+        consumeRollupEvent = rollupListener->Rollup(popupsToRollup, nullptr, nullptr);
+      }
 
       // Tell hook to stop processing messages
       sProcessHook = false;
       sRollupMsgId = 0;
       sRollupMsgWnd = nullptr;
 
       // return TRUE tells Windows that the event is consumed,
       // false allows the event to be dispatched