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 a=jcristau
authorNeil Deakin <neil@mozilla.com>
Tue, 06 Dec 2016 15:25:09 -1000
changeset 353569 eb0973de87ee755ca04ef794ed279b1717f2da51
parent 353568 be6b93fc358202fd89734172058293cd764e75b5
child 353570 b618acd89350d190d057ad8a42d9f370731073f7
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersksteuber, jcristau
bugs1313130
milestone52.0a2
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 a=jcristau
browser/base/content/browser.xul
browser/base/content/test/general/browser_selectpopup.js
layout/xul/nsXULPopupManager.cpp
layout/xul/nsXULPopupManager.h
toolkit/content/tests/chrome/test_popup_keys.xul
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -173,17 +173,17 @@
     <!-- for select dropdowns. The menupopup is what shows the list of options,
          and the popuponly menulist makes things like the menuactive attributes
          work correctly on the menupopup. ContentSelectDropdown expects the
          popuponly menulist to be its immediate parent. -->
     <menulist popuponly="true" id="ContentSelectDropdown" hidden="true">
       <menupopup rolluponmousewheel="true"
                  activateontab="true" position="after_start"
 #ifdef XP_WIN
-                 consumeoutsideclicks="false" ignorekeys="handled"
+                 consumeoutsideclicks="false" ignorekeys="shortcuts"
 #endif
         />
     </menulist>
 
     <!-- for invalid form error message -->
     <panel id="invalid-form-popup" type="arrow" orient="vertical" noautofocus="true" hidden="true" level="parent">
       <description/>
     </panel>
--- a/browser/base/content/test/general/browser_selectpopup.js
+++ b/browser/base/content/test/general/browser_selectpopup.js
@@ -153,16 +153,24 @@ function* doSelectTests(contentType, dtd
   is((yield getChangeEvents()), 0, "Before closed - number of change events");
 
   EventUtils.synthesizeKey("a", { accelKey: true });
   yield ContentTask.spawn(gBrowser.selectedBrowser, { isWindows }, function(args) {
     Assert.equal(String(content.getSelection()), args.isWindows ? "Text" : "",
       "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");
   is((yield getInputEvents()), 1, "After closed - number of input events");
   is((yield getChangeEvents()), 1, "After closed - number of change events");
 
   // Opening and closing the popup without changing the value should not fire a change event.
   yield openSelectPopup(selectPopup, true);
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -905,18 +905,18 @@ nsXULPopupManager::ShowPopupCallback(nsI
   // install keyboard event listeners for navigating menus. For panels, the
   // escape key may be used to close the panel. However, the ignorekeys
   // attribute may be used to disable adding these event listeners for popups
   // that want to handle their own keyboard events.
   nsAutoString ignorekeys;
   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) {
     // if the menu is on a menubar, use the menubar's listener instead
     nsMenuFrame* menuFrame = do_QueryFrame(aPopupFrame->GetParent());
     if (menuFrame) {
       item->SetOnMenuBar(menuFrame->IsOnMenuBar());
     }
@@ -2591,17 +2591,17 @@ nsresult
 nsXULPopupManager::KeyUp(nsIDOMKeyEvent* aKeyEvent)
 {
   // don't do anything if a menu isn't open or a menubar isn't active
   if (!mActiveMenuBar) {
     nsMenuChainItem* item = GetTopVisibleMenu();
     if (!item || item->PopupType() != ePopupTypeMenu)
       return NS_OK;
 
-    if (item->IgnoreKeys() == eIgnoreKeys_Handled) {
+    if (item->IgnoreKeys() == eIgnoreKeys_Shortcuts) {
       aKeyEvent->AsEvent()->StopCrossProcessForwarding();
       return NS_OK;
     }
   }
 
   aKeyEvent->AsEvent()->StopPropagation();
   aKeyEvent->AsEvent()->StopCrossProcessForwarding();
   aKeyEvent->AsEvent()->PreventDefault();
@@ -2621,17 +2621,17 @@ nsXULPopupManager::KeyDown(nsIDOMKeyEven
   }
 
   // don't do anything if a menu isn't open or a menubar isn't active
   if (!mActiveMenuBar && (!item || item->PopupType() != ePopupTypeMenu))
     return NS_OK;
 
   // 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();
   }
 
   int32_t menuAccessKey = -1;
 
   // If the key just pressed is the access key (usually Alt),
   // dismiss and unfocus the menu.
 
@@ -2686,25 +2686,31 @@ nsXULPopupManager::KeyPress(nsIDOMKeyEve
     return NS_OK;
   }
 
   nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
   NS_ENSURE_TRUE(keyEvent, NS_ERROR_UNEXPECTED);
   // 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();
   }
 
   return NS_OK; // I am consuming event
 }
 
 NS_IMETHODIMP
--- a/layout/xul/nsXULPopupManager.h
+++ b/layout/xul/nsXULPopupManager.h
@@ -97,17 +97,17 @@ enum nsNavigationDirection {
   eNavigationDirection_Before,
   eNavigationDirection_End,
   eNavigationDirection_After
 };
 
 enum nsIgnoreKeys {
   eIgnoreKeys_False,
   eIgnoreKeys_True,
-  eIgnoreKeys_Handled,
+  eIgnoreKeys_Shortcuts,
 };
 
 #define NS_DIRECTION_IS_INLINE(dir) (dir == eNavigationDirection_Start ||     \
                                      dir == eNavigationDirection_End)
 #define NS_DIRECTION_IS_BLOCK(dir) (dir == eNavigationDirection_Before || \
                                     dir == eNavigationDirection_After)
 #define NS_DIRECTION_IS_BLOCK_TO_EDGE(dir) (dir == eNavigationDirection_First ||    \
                                             dir == eNavigationDirection_Last)
@@ -623,23 +623,22 @@ public:
    * mouse over a sibling menuitem which would normally close the menu. This
    * menu is closed via a timer. However, if the user moves the mouse over the
    * submenu before the timer fires, we should instead cancel the timer. This
    * ensures that the user can move the mouse diagonally over a menu.
    */
   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);
+                                nsMenuPopupFrame* aFrame);
 
   /**
    * Handles cursor navigation within a menu. Returns true if the key has
    * been handled.
    */
   bool HandleKeyboardNavigation(uint32_t aKeyCode);
 
   /**
--- a/toolkit/content/tests/chrome/test_popup_keys.xul
+++ b/toolkit/content/tests/chrome/test_popup_keys.xul
@@ -79,33 +79,32 @@ function runTests()
 
     yield new Promise(resolve => setTimeout(() => resolve(), 1000));
     popupHiddenPromise = waitForEvent(popup, "popuphidden");
     popup.hidePopup();
     yield popupHiddenPromise;
 
     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")
     gIgnoreAttrChange = false;
 
     popupShownPromise = waitForEvent(popup, "popupshown");
     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();
     yield popupHiddenPromise;
 
     SimpleTest.finish();
   })();
 }