From a21a0e4cb386763cbe2d7be88fe767dcce5f9dec Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Tue, 6 Dec 2016 15:25:09 -1000 Subject: [PATCH] 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 | 2 +- .../test/general/browser_selectpopup.js | 8 ++++++ layout/xul/nsXULPopupManager.cpp | 28 +++++++++++-------- layout/xul/nsXULPopupManager.h | 11 ++++---- .../content/tests/chrome/test_popup_keys.xul | 9 +++--- 5 files changed, 35 insertions(+), 23 deletions(-) diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul index 48a2dbf1efe1..3907f62d6952 100644 --- a/browser/base/content/browser.xul +++ b/browser/base/content/browser.xul @@ -178,7 +178,7 @@ diff --git a/browser/base/content/test/general/browser_selectpopup.js b/browser/base/content/test/general/browser_selectpopup.js index aa994add3fa6..287511aeb91e 100644 --- a/browser/base/content/test/general/browser_selectpopup.js +++ b/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"); diff --git a/layout/xul/nsXULPopupManager.cpp b/layout/xul/nsXULPopupManager.cpp index 194c341c2220..f13f9d8d8108 100644 --- a/layout/xul/nsXULPopupManager.cpp +++ b/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(); } diff --git a/layout/xul/nsXULPopupManager.h b/layout/xul/nsXULPopupManager.h index b68b2d8ce1cb..ad099bea92f8 100644 --- a/layout/xul/nsXULPopupManager.h +++ b/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,13 +632,12 @@ 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); + nsMenuPopupFrame* aFrame); /** * Handles cursor navigation within a menu. Returns true if the key has diff --git a/toolkit/content/tests/chrome/test_popup_keys.xul b/toolkit/content/tests/chrome/test_popup_keys.xul index 4338b4b0c487..37135af57e1b 100644 --- a/toolkit/content/tests/chrome/test_popup_keys.xul +++ b/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();