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
authorNeil Deakin <neil@mozilla.com>
Thu, 17 Sep 2015 11:20:33 -0400
changeset 295683 837fa5b51ffafe2816b644440561ce2ad2a50cc5
parent 295682 af944d4a75f004837d28bf109c3956db63fc589d
child 295684 c5332735b2410b8e7231b3fe34f272c147f5583f
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersneil
bugs1191897
milestone43.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 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
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
@@ -158,17 +158,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"
 #ifdef XP_WIN
-                 consumeoutsideclicks="false"
+                 consumeoutsideclicks="false" ignorekeys="handled"
 #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
@@ -18,17 +18,17 @@ const PAGECONTENT =
   "  <optgroup label='Second Group' disabled='true'>" +
   "    <option value='Four'>Four</option>" +
   "    <option value='Five'>Five</option>" +
   "  </optgroup>" +
   "  <option value='Six' disabled='true'>Six</option>" +
   "  <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)
 {
   let popupShownPromise = BrowserTestUtils.waitForEvent(selectPopup, "popupshown");
 
   if (withMouse) {
     return Promise.all([popupShownPromise,
@@ -103,16 +103,22 @@ function doSelectTests(contentType, dtd)
   }
 
   EventUtils.synthesizeKey("KEY_ArrowUp", { code: "ArrowUp" });
   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(3), "Select item 3 again");
   is(menulist.selectedIndex, isWindows ? 3 : 1, "Select item 3 selectedIndex");
 
   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");
   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);
   yield hideSelectPopup(selectPopup, true);
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -862,19 +862,23 @@ nsXULPopupManager::ShowPopupCallback(nsI
     new nsMenuChainItem(aPopupFrame, aIsContextMenu, popupType);
   if (!item)
     return;
 
   // 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.
-  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
     nsMenuFrame* menuFrame = do_QueryFrame(aPopupFrame->GetParent());
     if (menuFrame) {
       item->SetOnMenuBar(menuFrame->IsOnMenuBar());
     }
   }
@@ -1830,18 +1834,19 @@ nsXULPopupManager::SetCaptureState(nsICo
 
 void
 nsXULPopupManager::UpdateKeyboardListeners()
 {
   nsCOMPtr<EventTarget> newTarget;
   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) {
     newTarget = mActiveMenuBar->GetContent()->GetComposedDoc();
     isForMenu = true;
   }
 
   if (mKeyListener != newTarget) {
@@ -1997,16 +2002,24 @@ nsXULPopupManager::CancelMenuTimer(nsMen
     mTimerMenu = nullptr;
   }
 }
 
 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();
 
   if (aFrame) {
     bool action;
     nsMenuFrame* result = aFrame->FindMenuWithShortcut(aKeyEvent, action);
     if (result) {
@@ -2465,16 +2478,21 @@ nsXULPopupManager::HandleEvent(nsIDOMEve
 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) {
+      aKeyEvent->StopCrossProcessForwarding();
+      return NS_OK;
+    }
   }
 
   aKeyEvent->StopPropagation();
   aKeyEvent->StopCrossProcessForwarding();
   aKeyEvent->PreventDefault();
 
   return NS_OK; // I am consuming event
 }
@@ -2520,23 +2538,28 @@ nsXULPopupManager::KeyDown(nsIDOMKeyEven
       if (!(ctrl || alt || shift || meta)) {
         // The access key just went down and no other
         // modifiers are already down.
         if (mPopups)
           Rollup(0, false, nullptr, nullptr);
         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;
 }
 
 nsresult
 nsXULPopupManager::KeyPress(nsIDOMKeyEvent* aKeyEvent)
 {
   // Don't check prevent default flag -- menus always get first shot at key events.
@@ -2546,20 +2569,27 @@ nsXULPopupManager::KeyPress(nsIDOMKeyEve
       (item->Frame()->IsMenuLocked() || item->PopupType() != ePopupTypeMenu)) {
     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);
-  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();
   }
 
   return NS_OK; // I am consuming event
 }
 
 NS_IMETHODIMP
 nsXULPopupShowingEvent::Run()
--- a/layout/xul/nsXULPopupManager.h
+++ b/layout/xul/nsXULPopupManager.h
@@ -94,16 +94,22 @@ enum nsNavigationDirection {
   eNavigationDirection_Last,
   eNavigationDirection_First,
   eNavigationDirection_Start,
   eNavigationDirection_Before,
   eNavigationDirection_End,
   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 || \
                                     dir == eNavigationDirection_After)
 #define NS_DIRECTION_IS_BLOCK_TO_EDGE(dir) (dir == eNavigationDirection_First ||    \
                                             dir == eNavigationDirection_Last)
 
 PR_STATIC_ASSERT(NS_STYLE_DIRECTION_LTR == 0 && NS_STYLE_DIRECTION_RTL == 1);
@@ -124,28 +130,28 @@ extern const nsNavigationDirection Direc
 // the lowest child in a chain of menus, as this is the active submenu.
 class nsMenuChainItem
 {
 private:
   nsMenuPopupFrame* mFrame; // the popup frame
   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;
 
 public:
   nsMenuChainItem(nsMenuPopupFrame* aFrame, bool aIsContext, nsPopupType aPopupType)
     : mFrame(aFrame),
       mPopupType(aPopupType),
       mIsContext(aIsContext),
       mOnMenuBar(false),
-      mIgnoreKeys(false),
+      mIgnoreKeys(eIgnoreKeys_False),
       mParent(nullptr),
       mChild(nullptr)
   {
     NS_ASSERTION(aFrame, "null frame passed to nsMenuChainItem constructor");
     MOZ_COUNT_CTOR(nsMenuChainItem);
   }
 
   ~nsMenuChainItem()
@@ -153,19 +159,19 @@ public:
     MOZ_COUNT_DTOR(nsMenuChainItem);
   }
 
   nsIContent* Content();
   nsMenuPopupFrame* Frame() { return mFrame; }
   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; }
 
   // set the parent of this item to aParent, also changing the parent
   // to have this as a child.
   void SetParent(nsMenuChainItem* aParent);
 
--- a/toolkit/content/tests/chrome/test_popup_keys.xul
+++ b/toolkit/content/tests/chrome/test_popup_keys.xul
@@ -1,78 +1,118 @@
 <?xml version="1.0"?>
 <?xml-stylesheet href="chrome://global/skin" type="text/css"?>
 <?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>      
   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>      
 
 <!--
   This test checks that the ignorekeys attribute can be used on a menu to
   disable key navigation. The test is performed twice by opening the menu,
   simulating a cursor down key, and closing the popup. When keys are enabled,
   the first item on the menu should be highlighted, otherwise the first item
   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"/>
   <menuitem id="i4" label="Four"/>
 </menupopup>
 
 <script class="testbody" type="application/javascript">
 <![CDATA[
 
 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");
 
-function popupHidden()
-{
-  if (gIgnoreKeys) {
-    SimpleTest.finish();
-  }
-  else {
-    // try the test again with ignorekeys set
+    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'");
+
     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)
 {
   if (gIgnoreAttrChange || event.attrName != "_moz-menuactive")
     return;
 
   // the attribute should not be changed when ignorekeys is enabled