Bug 1427449, don't close the menu in nsMenuBarFrame::FindMenuWithShortcut when just checking if such a menu shortcut key exists from the keydown event handler, also for extra safety this should only happen for menus not panels, r=felipe
authorNeil Deakin <neil@mozilla.com>
Mon, 15 Jan 2018 15:16:56 -0500
changeset 399304 76990027c9f806d786d92aa321094c817cc1279b
parent 399303 8c19564d609adce400e30662469503419258ecdd
child 399305 732a0fe9f2fb2be4fa05e7570273aeac8d2bbf44
push id98934
push userneil@mozilla.com
push dateMon, 15 Jan 2018 20:19:05 +0000
treeherdermozilla-inbound@76990027c9f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfelipe
bugs1427449
milestone59.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 1427449, don't close the menu in nsMenuBarFrame::FindMenuWithShortcut when just checking if such a menu shortcut key exists from the keydown event handler, also for extra safety this should only happen for menus not panels, r=felipe
browser/base/content/test/popupNotifications/browser.ini
browser/base/content/test/popupNotifications/browser_popupNotification_accesskey.js
layout/xul/nsMenuBarFrame.cpp
layout/xul/nsMenuBarFrame.h
layout/xul/nsMenuBarListener.cpp
layout/xul/nsMenuBarListener.h
layout/xul/nsXULPopupManager.cpp
--- a/browser/base/content/test/popupNotifications/browser.ini
+++ b/browser/base/content/test/popupNotifications/browser.ini
@@ -9,16 +9,18 @@ skip-if = (os == "linux" && (debug || as
 [browser_popupNotification_2.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_3.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_4.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_5.js]
 skip-if = true # bug 1332646
+[browser_popupNotification_accesskey.js]
+skip-if = (os == "linux" && (debug || asan)) || os == "mac"
 [browser_popupNotification_checkbox.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_selection_required.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_keyboard.js]
 skip-if = (os == "linux" && (debug || asan))
 [browser_popupNotification_no_anchors.js]
 skip-if = (os == "linux" && (debug || asan))
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/popupNotifications/browser_popupNotification_accesskey.js
@@ -0,0 +1,43 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+function test() {
+  waitForExplicitFinish();
+
+  ok(PopupNotifications, "PopupNotifications object exists");
+  ok(PopupNotifications.panel, "PopupNotifications panel exists");
+
+  setup();
+}
+
+let buttonPressed = false;
+
+function commandTriggered() {
+  buttonPressed = true;
+}
+
+var tests = [
+  // This test ensures that the accesskey closes the popup.
+  { id: "Test#1",
+    run() {
+      this.notifyObj = new BasicNotification(this.id);
+      showNotification(this.notifyObj);
+    },
+    onShown(popup) {
+      window.addEventListener("command", commandTriggered, true);
+      checkPopup(popup, this.notifyObj);
+      EventUtils.synthesizeKey("VK_ALT", { type: "keydown" });
+      EventUtils.synthesizeKey("M", { altKey: true });
+      EventUtils.synthesizeKey("VK_ALT", { type: "keyup" });
+
+      // If bug xxx was present, then the popup would be in the
+      // process of being hidden right now.
+      isnot(popup.state, "hiding", "popup is not hiding");
+    },
+    onHidden(popup) {
+      window.removeEventListener("command", commandTriggered, true);
+      ok(buttonPressed, "button pressed");
+    }
+  }
+ ];
--- a/layout/xul/nsMenuBarFrame.cpp
+++ b/layout/xul/nsMenuBarFrame.cpp
@@ -139,17 +139,17 @@ nsMenuBarFrame::ToggleMenuActiveState()
       mCurrentMenu = firstFrame;
     }
   }
 
   return nullptr;
 }
 
 nsMenuFrame*
-nsMenuBarFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent)
+nsMenuBarFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, bool aPeek)
 {
   uint32_t charCode;
   aKeyEvent->GetCharCode(&charCode);
 
   AutoTArray<uint32_t, 10> accessKeys;
   WidgetKeyboardEvent* nativeKeyEvent =
     aKeyEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent();
   if (nativeKeyEvent) {
@@ -197,33 +197,35 @@ nsMenuBarFrame::FindMenuWithShortcut(nsI
     currFrame = currFrame->GetNextSibling();
   }
   if (foundMenu) {
     return do_QueryFrame(foundMenu);
   }
 
   // didn't find a matching menu item
 #ifdef XP_WIN
-  // behavior on Windows - this item is on the menu bar, beep and deactivate the menu bar
-  if (mIsActive) {
-    nsCOMPtr<nsISound> soundInterface = do_CreateInstance("@mozilla.org/sound;1");
-    if (soundInterface)
-      soundInterface->Beep();
-  }
+  if (!aPeek) {
+    // behavior on Windows - this item is on the menu bar, beep and deactivate the menu bar
+    if (mIsActive) {
+      nsCOMPtr<nsISound> soundInterface = do_CreateInstance("@mozilla.org/sound;1");
+      if (soundInterface)
+        soundInterface->Beep();
+    }
 
-  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
-  if (pm) {
-    nsIFrame* popup = pm->GetTopPopup(ePopupTypeAny);
-    if (popup)
-      pm->HidePopup(popup->GetContent(), true, true, true, false);
+    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
+    if (pm) {
+      nsIFrame* popup = pm->GetTopPopup(ePopupTypeMenu);
+      if (popup)
+        pm->HidePopup(popup->GetContent(), true, true, true, false);
+    }
+
+    SetCurrentMenuItem(nullptr);
+    SetActive(false);
   }
 
-  SetCurrentMenuItem(nullptr);
-  SetActive(false);
-
 #endif  // #ifdef XP_WIN
 
   return nullptr;
 }
 
 /* virtual */ nsMenuFrame*
 nsMenuBarFrame::GetCurrentMenuItem()
 {
--- a/layout/xul/nsMenuBarFrame.h
+++ b/layout/xul/nsMenuBarFrame.h
@@ -80,17 +80,17 @@ public:
   // may deselect the menuitem.
   virtual bool MenuClosed() override;
 
   // Called when Enter is pressed while the menubar is focused. If the current
   // menu is open, let the child handle the key.
   nsMenuFrame* Enter(mozilla::WidgetGUIEvent* aEvent);
 
   // Used to handle ALT+key combos
-  nsMenuFrame* FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent);
+  nsMenuFrame* FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent, bool aPeek);
 
   virtual bool IsFrameOfType(uint32_t aFlags) const override
   {
     // Override bogus IsFrameOfType in nsBoxFrame.
     if (aFlags & (nsIFrame::eReplacedContainsBlock | nsIFrame::eReplaced))
       return false;
     return nsBoxFrame::IsFrameOfType(aFlags);
   }
--- a/layout/xul/nsMenuBarListener.cpp
+++ b/layout/xul/nsMenuBarListener.cpp
@@ -319,17 +319,17 @@ nsMenuBarListener::KeyPress(nsIDOMEvent*
           aKeyEvent->PreventDefault();
         }
       }
 
       return NS_OK;
     }
 #endif // !XP_MACOSX
 
-    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent);
+    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent, false);
     if (!menuFrameForKey) {
       return NS_OK;
     }
 
     // If the keyboard event matches with a menu item's accesskey and
     // will be sent to a remote process, it should be executed with
     // reply event from the focused remote process.  Note that if the
     // menubar is active, the event is already marked as "stop cross
@@ -377,17 +377,17 @@ nsMenuBarListener::GetModifiersForAccess
 
   static const Modifiers kPossibleModifiersForAccessKey =
     (MODIFIER_SHIFT | MODIFIER_CONTROL | MODIFIER_ALT | MODIFIER_META |
      MODIFIER_OS);
   return (inputEvent->mModifiers & kPossibleModifiersForAccessKey);
 }
 
 nsMenuFrame*
-nsMenuBarListener::GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent)
+nsMenuBarListener::GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent, bool aPeek)
 {
   if (!IsAccessKeyPressed(aKeyEvent)) {
     return nullptr;
   }
 
   uint32_t charCode;
   aKeyEvent->GetCharCode(&charCode);
   bool hasAccessKeyCandidates = charCode != 0;
@@ -399,17 +399,17 @@ nsMenuBarListener::GetMenuForKeyEvent(ns
     nativeKeyEvent->GetAccessKeyCandidates(keys);
     hasAccessKeyCandidates = !keys.IsEmpty();
   }
 
   if (hasAccessKeyCandidates) {
     // Do shortcut navigation.
     // A letter was pressed. We want to see if a shortcut gets matched. If
     // so, we'll know the menu got activated.
-    return mMenuBarFrame->FindMenuWithShortcut(aKeyEvent);
+    return mMenuBarFrame->FindMenuWithShortcut(aKeyEvent, aPeek);
   }
 
   return nullptr;
 }
 
 void
 nsMenuBarListener::ReserveKeyIfNeeded(nsIDOMEvent* aKeyEvent)
 {
@@ -488,17 +488,17 @@ nsMenuBarListener::KeyDown(nsIDOMEvent* 
     }
 
     // Some key other than the access key just went down,
     // so we won't activate the menu bar when the access key is released.
     mAccessKeyDownCanceled = !isAccessKeyDownEvent;
   }
 
   if (capturing && mAccessKey) {
-    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent);
+    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent, true);
     if (menuFrameForKey) {
       ReserveKeyIfNeeded(aKeyEvent);
     }
   }
 
   return NS_OK; // means I am NOT consuming event
 }
 
--- a/layout/xul/nsMenuBarListener.h
+++ b/layout/xul/nsMenuBarListener.h
@@ -72,19 +72,20 @@ protected:
   nsresult Fullscreen(nsIDOMEvent* aEvent);
 
   static void InitAccessKey();
 
   static mozilla::Modifiers GetModifiersForAccessKey(nsIDOMKeyEvent* event);
 
   /**
    * Given a key event for an Alt+shortcut combination,
-   * return the menu, if any, that would be opened.
+   * return the menu, if any, that would be opened. If aPeek
+   * is false, then play a beep and deactivate the menubar on Windows.
    */
-  nsMenuFrame* GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent);
+  nsMenuFrame* GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent, bool aPeek);
 
   /**
    * Call MarkAsReservedByChrome if the user's preferences indicate that
    * the key should be chrome-only.
    */
   void ReserveKeyIfNeeded(nsIDOMEvent* aKeyEvent);
 
   // This should only be called by the nsMenuBarListener during event dispatch,
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -2128,17 +2128,17 @@ nsXULPopupManager::HandleShortcutNavigat
       }
       return true;
     }
 
     return false;
   }
 
   if (mActiveMenuBar) {
-    nsMenuFrame* result = mActiveMenuBar->FindMenuWithShortcut(aKeyEvent);
+    nsMenuFrame* result = mActiveMenuBar->FindMenuWithShortcut(aKeyEvent, false);
     if (result) {
       mActiveMenuBar->SetActive(true);
       result->OpenMenu(true);
       return true;
     }
   }
 
   return false;