Bug 1191897, add a flag for popups which allow shortcut keys to not be consumed, fixes shortcuts not working when an e10s select popup is open, r=neil

browser/base/content/browser.xul

@@ -163,7 +163,7 @@
     <menulist popuponly="true" id="ContentSelectDropdown" hidden="true">
       <menupopup rolluponmousewheel="true"
 #ifdef XP_WIN
-                 consumeoutsideclicks="false"
+                 consumeoutsideclicks="false" ignorekeys="handled"
 #endif
         />
     </menulist>

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

@@ -23,7 +23,7 @@ const PAGECONTENT =
   "  <optgroup label='Third Group'>" +
   "    <option value='Seven'>   Seven  </option>" +
   "    <option value='Eight'>&nbsp;&nbsp;Eight&nbsp;&nbsp;</option>" +
-  "  </optgroup></select><input />" +
+  "  </optgroup></select><input />Text" +
   "</body></html>";
 
 function openSelectPopup(selectPopup, withMouse)
@@ -108,6 +108,12 @@ function doSelectTests(contentType, dtd)
 
   is((yield getChangeEvents()), 0, "Before closed - number of change events");
 
+  EventUtils.synthesizeKey("a", { accelKey: true });
+  let selection = yield ContentTask.spawn(gBrowser.selectedBrowser, {}, function() {
+    return String(content.getSelection());
+  });
+  is(selection, isWindows ? "Text" : "", "Select all while popup is open");
+
   yield hideSelectPopup(selectPopup);
 
   is(menulist.selectedIndex, 3, "Item 3 still selected");

layout/xul/nsXULPopupManager.cpp

@@ -867,9 +867,13 @@ nsXULPopupManager::ShowPopupCallback(nsIContent* aPopup,
   // 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.
-  if (aPopup->AttrValueIs(kNameSpaceID_None, nsGkAtoms::ignorekeys,
-                           nsGkAtoms::_true, eCaseMatters))
-    item->SetIgnoreKeys(true);
+  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);
+  }
 
   if (ismenu) {
     // if the menu is on a menubar, use the menubar's listener instead
@@ -1835,8 +1839,9 @@ nsXULPopupManager::UpdateKeyboardListeners()
   bool isForMenu = false;
   nsMenuChainItem* item = GetTopVisibleMenu();
   if (item) {
-    if (!item->IgnoreKeys())
+    if (item->IgnoreKeys() != eIgnoreKeys_True) {
       newTarget = item->Content()->GetComposedDoc();
+    }
     isForMenu = item->PopupType() == ePopupTypeMenu;
   }
   else if (mActiveMenuBar) {
@@ -2002,6 +2007,14 @@ bool
 nsXULPopupManager::HandleShortcutNavigation(nsIDOMKeyEvent* aKeyEvent,
                                             nsMenuPopupFrame* aFrame)
 {
+  // On Windows, don't check shortcuts when the accelerator key is down.
+#ifdef XP_WIN
+  WidgetInputEvent* evt = aKeyEvent->GetInternalNSEvent()->AsInputEvent();
+  if (evt && evt->IsAccel()) {
+    return false;
+  }
+#endif
+
   nsMenuChainItem* item = GetTopVisibleMenu();
   if (!aFrame && item)
     aFrame = item->Frame();
@@ -2470,6 +2483,11 @@ nsXULPopupManager::KeyUp(nsIDOMKeyEvent* aKeyEvent)
     nsMenuChainItem* item = GetTopVisibleMenu();
     if (!item || item->PopupType() != ePopupTypeMenu)
       return NS_OK;
+
+    if (item->IgnoreKeys() == eIgnoreKeys_Handled) {
+      aKeyEvent->StopCrossProcessForwarding();
+      return NS_OK;
+    }
   }
 
   aKeyEvent->StopPropagation();
@@ -2525,13 +2543,18 @@ nsXULPopupManager::KeyDown(nsIDOMKeyEvent* aKeyEvent)
         else if (mActiveMenuBar)
           mActiveMenuBar->MenuClosed();
       }
+      aKeyEvent->StopPropagation();
       aKeyEvent->PreventDefault();
     }
   }
 
   // Since a menu was open, stop propagation of the event to keep other event
   // listeners from becoming confused.
-  aKeyEvent->StopPropagation();
+
+  if (!item || item->IgnoreKeys() != eIgnoreKeys_Handled) {
+    aKeyEvent->StopPropagation();
+  }
+
   aKeyEvent->StopCrossProcessForwarding();
   return NS_OK;
 }
@@ -2551,10 +2574,17 @@ nsXULPopupManager::KeyPress(nsIDOMKeyEvent* 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);
-  HandleShortcutNavigation(keyEvent, nullptr);
-  if (consume) {
+
+  // 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);
+
+  aKeyEvent->StopCrossProcessForwarding();
+  if (handled || (consume && !onlyHandled)) {
     aKeyEvent->StopPropagation();
-    aKeyEvent->StopCrossProcessForwarding();
     aKeyEvent->PreventDefault();
   }
 

layout/xul/nsXULPopupManager.h

@@ -99,6 +99,12 @@ enum nsNavigationDirection {
   eNavigationDirection_After
 };
 
+enum nsIgnoreKeys {
+  eIgnoreKeys_False,
+  eIgnoreKeys_True,
+  eIgnoreKeys_Handled,
+};
+
 #define NS_DIRECTION_IS_INLINE(dir) (dir == eNavigationDirection_Start ||     \
                                      dir == eNavigationDirection_End)
 #define NS_DIRECTION_IS_BLOCK(dir) (dir == eNavigationDirection_Before || \
@@ -129,7 +135,7 @@ private:
   nsPopupType mPopupType; // the popup type of the frame
   bool mIsContext; // true for context menus
   bool mOnMenuBar; // true if the menu is on a menu bar
-  bool mIgnoreKeys; // true if keyboard listeners should not be used
+  nsIgnoreKeys mIgnoreKeys; // indicates how keyboard listeners should be used
 
   nsMenuChainItem* mParent;
   nsMenuChainItem* mChild;
@@ -140,7 +146,7 @@ public:
       mPopupType(aPopupType),
       mIsContext(aIsContext),
       mOnMenuBar(false),
-      mIgnoreKeys(false),
+      mIgnoreKeys(eIgnoreKeys_False),
       mParent(nullptr),
       mChild(nullptr)
   {
@@ -158,9 +164,9 @@ public:
   nsPopupType PopupType() { return mPopupType; }
   bool IsMenu() { return mPopupType == ePopupTypeMenu; }
   bool IsContextMenu() { return mIsContext; }
-  bool IgnoreKeys() { return mIgnoreKeys; }
+  nsIgnoreKeys IgnoreKeys() { return mIgnoreKeys; }
+  void SetIgnoreKeys(nsIgnoreKeys aIgnoreKeys) { mIgnoreKeys = aIgnoreKeys; }
   bool IsOnMenuBar() { return mOnMenuBar; }
-  void SetIgnoreKeys(bool aIgnoreKeys) { mIgnoreKeys = aIgnoreKeys; }
   void SetOnMenuBar(bool aOnMenuBar) { mOnMenuBar = aOnMenuBar; }
   nsMenuChainItem* GetParent() { return mParent; }
   nsMenuChainItem* GetChild() { return mChild; }

toolkit/content/tests/chrome/test_popup_keys.xul

@@ -3,8 +3,8 @@
 <?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
 
 <window title="Menu ignorekeys Test"
-        onkeydown="keyDown()"
-        onload="setTimeout(runTests, 0);"
+        onkeydown="keyDown()" onkeypress="gKeyPressCount++; event.stopPropagation(); event.preventDefault();"
+        onload="runTests();"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
 
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>      
@@ -18,7 +18,7 @@
   should not be highlighted.
   -->
 
-<menupopup id="popup" onpopupshown="popupShown()" onpopuphidden="popupHidden()">
+<menupopup id="popup">
   <menuitem id="i1" label="One" onDOMAttrModified="attrModified(event)"/>
   <menuitem id="i2" label="Two"/>
   <menuitem id="i3" label="Three"/>
@@ -32,42 +32,82 @@ SimpleTest.waitForExplicitFinish();
 
 var gIgnoreKeys = false;
 var gIgnoreAttrChange = false;
+var gKeyPressCount = 0;
+
+let {Task} = Components.utils.import("resource://gre/modules/Task.jsm", {});
+
+function waitForEvent(target, eventName) {
+  return new Promise(resolve => {
+    target.addEventListener(eventName, function eventOccurred(event) {
+      target.removeEventListener(eventName, eventOccurred, false);
+      resolve();
+    }, false);
+  });
+}
 
 function runTests()
 {
-  var popup = $("popup");
-  popup.enableKeyboardNavigator(false);
-  is(popup.getAttribute("ignorekeys"), "true", "keys disabled");
-  popup.enableKeyboardNavigator(true);
-  is(popup.hasAttribute("ignorekeys"), false, "keys enabled");
-  popup.openPopup(null, "after_start");
-}
-
-function popupShown()
-{
-  synthesizeKey("VK_DOWN", { });
-  if (gIgnoreKeys) {
+  Task.async(function* () {
     var popup = $("popup");
-    setTimeout(function() { popup.hidePopup() }, 1000);
-  }
-}
+    popup.enableKeyboardNavigator(false);
+    is(popup.getAttribute("ignorekeys"), "true", "keys disabled");
+    popup.enableKeyboardNavigator(true);
+    is(popup.hasAttribute("ignorekeys"), false, "keys enabled");
+
+    let popupShownPromise = waitForEvent(popup, "popupshown");
+    popup.openPopup(null, "after_start");
+    yield popupShownPromise;
+
+    let popupHiddenPromise = waitForEvent(popup, "popuphidden");
+    synthesizeKey("VK_DOWN", { });
+    yield popupHiddenPromise;
+
+    is(gKeyPressCount, 0, "keypresses with ignorekeys='false'");
 
-function popupHidden()
-{
-  if (gIgnoreKeys) {
-    SimpleTest.finish();
-  }
-  else {
-    // try the test again with ignorekeys set
     gIgnoreKeys = true;
-    var popup = $("popup");
     popup.setAttribute("ignorekeys", "true");
     // 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;
+
+    synthesizeKey("VK_DOWN", { });
+
+    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");
+    // 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. 
+    synthesizeKey("t", { });
+    is(gKeyPressCount, 1, "keypresses after t pressed with ignorekeys='handled'");
+
+    let isWindows = navigator.platform.indexOf("Win") >= 0;
+    synthesizeKey("t", { accelKey: true });
+    is(gKeyPressCount, isWindows ? 2 : 1, "keypresses after accel+t pressed with ignorekeys='handled'");
+
+    popupHiddenPromise = waitForEvent(popup, "popuphidden");
+    popup.hidePopup();
+    yield popupHiddenPromise;
+
+    SimpleTest.finish();
+  })();
 }
 
 function attrModified(event)