Bug 1313130, change menu shortcut handling so that Windows does not call preventDefault only when the accelerator key is down rather than when a key isn't handled, r=ksteuber

browser/base/content/browser.xul

@@ -178,7 +178,7 @@
       <menupopup rolluponmousewheel="true"
                  activateontab="true" position="after_start"
 #ifdef XP_WIN
-                 consumeoutsideclicks="false" ignorekeys="handled"
+                 consumeoutsideclicks="false" ignorekeys="shortcuts"
 #endif
         />
     </menulist>

browser/base/content/test/general/browser_selectpopup.js

@@ -164,6 +164,14 @@ function* doSelectTests(contentType, dtd)
       "Select all while popup is open");
   });
 
+  // Backspace should not go back
+  let handleKeyPress = function(event) {
+    ok(false, "Should not get keypress event");
+  }
+  window.addEventListener("keypress", handleKeyPress);
+  EventUtils.synthesizeKey("VK_BACK_SPACE", { });
+  window.removeEventListener("keypress", handleKeyPress);
+
   yield hideSelectPopup(selectPopup);
 
   is(menulist.selectedIndex, 3, "Item 3 still selected");

layout/xul/nsXULPopupManager.cpp

@@ -910,8 +910,8 @@ nsXULPopupManager::ShowPopupCallback(nsIContent* aPopup,
   aPopup->GetAttr(kNameSpaceID_None, nsGkAtoms::ignorekeys, ignorekeys);
   if (ignorekeys.EqualsLiteral("true")) {
     item->SetIgnoreKeys(eIgnoreKeys_True);
-  } else if (ignorekeys.EqualsLiteral("handled")) {
-    item->SetIgnoreKeys(eIgnoreKeys_Handled);
+  } else if (ignorekeys.EqualsLiteral("shortcuts")) {
+    item->SetIgnoreKeys(eIgnoreKeys_Shortcuts);
   }
 
   if (ismenu) {
@@ -2631,7 +2631,7 @@ nsXULPopupManager::KeyUp(nsIDOMKeyEvent* aKeyEvent)
     if (!item || item->PopupType() != ePopupTypeMenu)
       return NS_OK;
 
-    if (item->IgnoreKeys() == eIgnoreKeys_Handled) {
+    if (item->IgnoreKeys() == eIgnoreKeys_Shortcuts) {
       aKeyEvent->AsEvent()->StopCrossProcessForwarding();
       return NS_OK;
     }
@@ -2661,7 +2661,7 @@ nsXULPopupManager::KeyDown(nsIDOMKeyEvent* aKeyEvent)
 
   // Since a menu was open, stop propagation of the event to keep other event
   // listeners from becoming confused.
-  if (!item || item->IgnoreKeys() != eIgnoreKeys_Handled) {
+  if (!item || item->IgnoreKeys() != eIgnoreKeys_Shortcuts) {
     aKeyEvent->AsEvent()->StopPropagation();
   }
 
@@ -2726,15 +2726,21 @@ nsXULPopupManager::KeyPress(nsIDOMKeyEvent* aKeyEvent)
   // if a menu is open or a menubar is active, it consumes the key event
   bool consume = (mPopups || mActiveMenuBar);
 
-  // When ignorekeys="handled" is used, we don't call preventDefault on the key
-  // event, which allows another listener to handle keys that the popup hasn't
-  // already handled. For instance, this allows global shortcuts to still apply
-  // while a menu is open.
-  bool onlyHandled = item && item->IgnoreKeys() == eIgnoreKeys_Handled;
-  bool handled = HandleShortcutNavigation(keyEvent, nullptr);
+  WidgetInputEvent* evt = aKeyEvent->AsEvent()->WidgetEventPtr()->AsInputEvent();
+  bool isAccel = evt && evt->IsAccel();
+
+  // When ignorekeys="shortcuts" is used, we don't call preventDefault on the
+  // key event when the accelerator key is pressed. This allows another
+  // listener to handle keys. For instance, this allows global shortcuts to
+  // still apply while a menu is open.
+  if (item && item->IgnoreKeys() == eIgnoreKeys_Shortcuts && isAccel) {
+    consume = false;
+  }
+
+  HandleShortcutNavigation(keyEvent, nullptr);
 
   aKeyEvent->AsEvent()->StopCrossProcessForwarding();
-  if (handled || (consume && !onlyHandled)) {
+  if (consume) {
     aKeyEvent->AsEvent()->StopPropagation();
     aKeyEvent->AsEvent()->PreventDefault();
   }

layout/xul/nsXULPopupManager.h

@@ -102,7 +102,7 @@ enum nsNavigationDirection {
 enum nsIgnoreKeys {
   eIgnoreKeys_False,
   eIgnoreKeys_True,
-  eIgnoreKeys_Handled,
+  eIgnoreKeys_Shortcuts,
 };
 
 #define NS_DIRECTION_IS_INLINE(dir) (dir == eNavigationDirection_Start ||     \
@@ -632,10 +632,9 @@ public:
   void CancelMenuTimer(nsMenuParent* aMenuParent);
 
   /**
-   * Handles navigation for menu accelkeys. Returns true if the key has
-   * been handled. If aFrame is specified, then the key is handled by that
-   * popup, otherwise if aFrame is null, the key is handled by the active
-   * popup or menubar.
+   * Handles navigation for menu accelkeys. If aFrame is specified, then the
+   * key is handled by that popup, otherwise if aFrame is null, the key is
+   * handled by the active popup or menubar.
    */
   bool HandleShortcutNavigation(nsIDOMKeyEvent* aKeyEvent,
                                 nsMenuPopupFrame* aFrame);

toolkit/content/tests/chrome/test_popup_keys.xul

@@ -84,7 +84,7 @@ function runTests()
 
     is(gKeyPressCount, 1, "keypresses with ignorekeys='true'");
 
-    popup.setAttribute("ignorekeys", "handled");
+    popup.setAttribute("ignorekeys", "shortcuts");
     // clear this first to avoid confusion
     gIgnoreAttrChange = true;
     $("i1").removeAttribute("_moz-menuactive")
@@ -94,13 +94,12 @@ function runTests()
     popup.openPopup(null, "after_start");
     yield popupShownPromise;
 
-    // When ignorekeys="handled", T should be handled but accel+T should propagate. 
+    // When ignorekeys="shortcuts", T should be handled but accel+T should propagate. 
     synthesizeKey("t", { });
-    is(gKeyPressCount, 1, "keypresses after t pressed with ignorekeys='handled'");
+    is(gKeyPressCount, 1, "keypresses after t pressed with ignorekeys='shortcuts'");
 
-    let isWindows = navigator.platform.indexOf("Win") >= 0;
     synthesizeKey("t", { accelKey: true });
-    is(gKeyPressCount, isWindows ? 2 : 1, "keypresses after accel+t pressed with ignorekeys='handled'");
+    is(gKeyPressCount, 2, "keypresses after accel+t pressed with ignorekeys='shortcuts'");
 
     popupHiddenPromise = waitForEvent(popup, "popuphidden");
     popup.hidePopup();