Bug 380637, when an Alt+accesskey for a menubar menu is pressed, check if the user has blocked sites from overriding keyboard shortcuts, and don't send the event to the page if so. Add a similar check for the F10 key which focuses the menubar. An additional capturing keydown listener is added because the key needs to be blocked before content sees it. r=masayuki

2017-11-09 18:42:40 -05:00 · 2017-11-09 18:42:40 -05:00 · 52d3f646c7
--- a/browser/base/content/test/permissions/browser_reservedkey.js
+++ b/browser/base/content/test/permissions/browser_reservedkey.js
@ -42,3 +42,51 @@ add_task(async function test_reserved_shortcuts() {

  await BrowserTestUtils.removeTab(tab);
 });
+
+// This test checks that Alt+<key> and F10 cannot be blocked when the preference is set.
+if (navigator.platform.indexOf("Mac") == -1) {
+  add_task(async function test_accesskeys_menus() {
+    await new Promise(resolve => {
+      SpecialPowers.pushPrefEnv({"set": [["permissions.default.shortcuts", 2]]}, resolve);
+    });
+
+    const uri = "data:text/html,<body onkeydown='if (event.key == \"H\" || event.key == \"F10\") event.preventDefault();'>";
+    let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, uri);
+
+    // Pressing Alt+H should open the Help menu.
+    let helpPopup = document.getElementById("menu_HelpPopup");
+    let popupShown = BrowserTestUtils.waitForEvent(helpPopup, "popupshown");
+    EventUtils.synthesizeKey("VK_ALT", { type: "keydown" });
+    EventUtils.synthesizeKey("H", { altKey: true });
+    EventUtils.synthesizeKey("VK_ALT", { type: "keyup" });
+    await popupShown;
+
+    ok(true, "Help menu opened");
+
+    let popupHidden = BrowserTestUtils.waitForEvent(helpPopup, "popuphidden");
+    helpPopup.hidePopup();
+    await popupHidden;
+
+    // Pressing F10 should focus the menubar. On Linux, the file menu should open, but on Windows,
+    // pressing Down will open the file menu.
+    let menubar = document.getElementById("main-menubar");
+    let menubarActive = BrowserTestUtils.waitForEvent(menubar, "DOMMenuBarActive");
+    EventUtils.sendKey("F10");
+    await menubarActive;
+
+    let filePopup = document.getElementById("menu_FilePopup");
+    popupShown = BrowserTestUtils.waitForEvent(filePopup, "popupshown");
+    if (navigator.platform.indexOf("Win") >= 0) {
+      EventUtils.synthesizeKey("KEY_ArrowDown", { code: "ArrowDown" });
+    }
+    await popupShown;
+
+    ok(true, "File menu opened");
+
+    popupHidden = BrowserTestUtils.waitForEvent(filePopup, "popuphidden");
+    filePopup.hidePopup();
+    await popupHidden;
+
+    await BrowserTestUtils.removeTab(tab1);
+  });
+}
--- a/layout/xul/nsMenuBarListener.cpp
+++ b/layout/xul/nsMenuBarListener.cpp
@ -61,6 +61,9 @@ nsMenuBarListener::nsMenuBarListener(nsMenuBarFrame* aMenuBarFrame,
  mEventTarget->AddSystemEventListener(NS_LITERAL_STRING("keyup"), this, false);
  mEventTarget->AddSystemEventListener(NS_LITERAL_STRING("mozaccesskeynotfound"),
                                       this, false);
+  // Need a capturing event listener if the user has blocked pages from overriding
+  // system keys so that we can prevent menu accesskeys from being cancelled.
+  mEventTarget->AddEventListener(NS_LITERAL_STRING("keydown"), this, true);

  // mousedown event should be handled in all phase
  mEventTarget->AddEventListener(NS_LITERAL_STRING("mousedown"), this, true);
@ -97,6 +100,8 @@ nsMenuBarListener::OnDestroyMenuBarFrame()
                                          this, false);
  mEventTarget->RemoveSystemEventListener(
                  NS_LITERAL_STRING("mozaccesskeynotfound"), this, false);
+  mEventTarget->RemoveEventListener(NS_LITERAL_STRING("keydown"),
+                                    this, true);

  mEventTarget->RemoveEventListener(NS_LITERAL_STRING("mousedown"), this, true);
  mEventTarget->RemoveEventListener(NS_LITERAL_STRING("mousedown"),
@ -277,56 +282,17 @@ nsMenuBarListener::KeyPress(nsIDOMEvent* aKeyEvent)
    }

    nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
-    uint32_t keyCode, charCode;
+    uint32_t keyCode;
    keyEvent->GetKeyCode(&keyCode);
-    keyEvent->GetCharCode(&charCode);
-
-    bool hasAccessKeyCandidates = charCode != 0;
-    if (!hasAccessKeyCandidates) {
-      if (nativeKeyEvent) {
-        AutoTArray<uint32_t, 10> keys;
-        nativeKeyEvent->GetAccessKeyCandidates(keys);
-        hasAccessKeyCandidates = !keys.IsEmpty();
-      }
-    }

    // Cancel the access key flag unless we are pressing the access key.
    if (keyCode != (uint32_t)mAccessKey) {
      mAccessKeyDownCanceled = true;
    }

-    if (IsAccessKeyPressed(keyEvent) && hasAccessKeyCandidates) {
-      // Do shortcut navigation.
-      // A letter was pressed. We want to see if a shortcut gets matched. If
-      // so, we'll know the menu got activated.
-      nsMenuFrame* result = mMenuBarFrame->FindMenuWithShortcut(keyEvent);
-      if (result) {
-        // If the keyboard event matches with a menu item's accesskey and
-        // will be sent to a remote process, it should be executed with
-        // reply event from the focused remote process.  Note that if the
-        // menubar is active, the event is already marked as "stop cross
-        // process dispatching".  So, in that case, this won't wait
-        // reply from the remote content.
-        if (nativeKeyEvent->WillBeSentToRemoteProcess()) {
-          nativeKeyEvent->StopImmediatePropagation();
-          nativeKeyEvent->MarkAsWaitingReplyFromRemoteProcess();
-          return NS_OK;
-        }
-        mMenuBarFrame->SetActiveByKeyboard();
-        mMenuBarFrame->SetActive(true);
-        result->OpenMenu(true);
-
-        // The opened menu will listen next keyup event.
-        // Therefore, we should clear the keydown flags here.
-        mAccessKeyDown = mAccessKeyDownCanceled = false;
-
-        aKeyEvent->StopPropagation();
-        aKeyEvent->PreventDefault();
-      }
-    }
 #ifndef XP_MACOSX
-    // Also need to handle F10 specially on Non-Mac platform.
-    else if (nativeKeyEvent->mMessage == eKeyPress && keyCode == NS_VK_F10) {
+    // Need to handle F10 specially on Non-Mac platform.
+    if (nativeKeyEvent->mMessage == eKeyPress && keyCode == NS_VK_F10) {
      if ((GetModifiersForAccessKey(keyEvent) & ~MODIFIER_CONTROL) == 0) {
        // If the keyboard event should activate the menubar and will be
        // sent to a remote process, it should be executed with reply
@ -353,8 +319,38 @@ nsMenuBarListener::KeyPress(nsIDOMEvent* aKeyEvent)
          aKeyEvent->PreventDefault();
        }
      }
+
+      return NS_OK;
    }
 #endif // !XP_MACOSX
+
+    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent);
+    if (!menuFrameForKey) {
+      return NS_OK;
+    }
+
+    // If the keyboard event matches with a menu item's accesskey and
+    // will be sent to a remote process, it should be executed with
+    // reply event from the focused remote process.  Note that if the
+    // menubar is active, the event is already marked as "stop cross
+    // process dispatching".  So, in that case, this won't wait
+    // reply from the remote content.
+    if (nativeKeyEvent->WillBeSentToRemoteProcess()) {
+      nativeKeyEvent->StopImmediatePropagation();
+      nativeKeyEvent->MarkAsWaitingReplyFromRemoteProcess();
+      return NS_OK;
+    }
+
+    mMenuBarFrame->SetActiveByKeyboard();
+    mMenuBarFrame->SetActive(true);
+    menuFrameForKey->OpenMenu(true);
+
+    // The opened menu will listen next keyup event.
+    // Therefore, we should clear the keydown flags here.
+    mAccessKeyDown = mAccessKeyDownCanceled = false;
+
+    aKeyEvent->StopPropagation();
+    aKeyEvent->PreventDefault();
  }

  return NS_OK;
@ -385,6 +381,45 @@ nsMenuBarListener::GetModifiersForAccessKey(nsIDOMKeyEvent* aKeyEvent)
  return (inputEvent->mModifiers & kPossibleModifiersForAccessKey);
 }

+nsMenuFrame*
+nsMenuBarListener::GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent)
+{
+  if (!IsAccessKeyPressed(aKeyEvent)) {
+    return nullptr;
+  }
+
+  uint32_t charCode;
+  aKeyEvent->GetCharCode(&charCode);
+  bool hasAccessKeyCandidates = charCode != 0;
+  if (!hasAccessKeyCandidates) {
+    WidgetKeyboardEvent* nativeKeyEvent =
+      aKeyEvent->AsEvent()->WidgetEventPtr()->AsKeyboardEvent();
+
+    AutoTArray<uint32_t, 10> keys;
+    nativeKeyEvent->GetAccessKeyCandidates(keys);
+    hasAccessKeyCandidates = !keys.IsEmpty();
+  }
+
+  if (hasAccessKeyCandidates) {
+    // Do shortcut navigation.
+    // A letter was pressed. We want to see if a shortcut gets matched. If
+    // so, we'll know the menu got activated.
+    return mMenuBarFrame->FindMenuWithShortcut(aKeyEvent);
+  }
+
+  return nullptr;
+}
+
+void
+nsMenuBarListener::ReserveKeyIfNeeded(nsIDOMEvent* aKeyEvent)
+{
+  WidgetKeyboardEvent* nativeKeyEvent =
+    aKeyEvent->WidgetEventPtr()->AsKeyboardEvent();
+  if (nsContentUtils::ShouldBlockReservedKeys(nativeKeyEvent)) {
+    nativeKeyEvent->MarkAsReservedByChrome();
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////
 nsresult
 nsMenuBarListener::KeyDown(nsIDOMEvent* aKeyEvent)
@ -397,18 +432,34 @@ nsMenuBarListener::KeyDown(nsIDOMEvent* aKeyEvent)
    aKeyEvent->GetIsTrusted(&trustedEvent);
  }

-  if (!trustedEvent)
+  if (!trustedEvent) {
    return NS_OK;
+  }
+
+  nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
+  if (!keyEvent) {
+    return NS_OK;
+  }
+
+  uint32_t theChar;
+  keyEvent->GetKeyCode(&theChar);
+
+  uint16_t eventPhase;
+  aKeyEvent->GetEventPhase(&eventPhase);
+  bool capturing = (eventPhase == nsIDOMEvent::CAPTURING_PHASE);
+
+#ifndef XP_MACOSX
+  if (capturing && !mAccessKeyDown && theChar == NS_VK_F10 &&
+      (GetModifiersForAccessKey(keyEvent) & ~MODIFIER_CONTROL) == 0) {
+    ReserveKeyIfNeeded(aKeyEvent);
+  }
+#endif

  if (mAccessKey && mAccessKeyFocuses)
  {
    bool defaultPrevented = false;
    aKeyEvent->GetDefaultPrevented(&defaultPrevented);

-    nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
-    uint32_t theChar;
-    keyEvent->GetKeyCode(&theChar);
-
    // No other modifiers can be down.
    // Especially CTRL.  CTRL+ALT == AltGR, and we'll fuck up on non-US
    // enhanced 102-key keyboards if we don't check this.
@ -416,7 +467,7 @@ nsMenuBarListener::KeyDown(nsIDOMEvent* aKeyEvent)
      ((theChar == (uint32_t)mAccessKey) &&
       (GetModifiersForAccessKey(keyEvent) & ~mAccessKeyMask) == 0);

-    if (!mAccessKeyDown) {
+    if (!capturing && !mAccessKeyDown) {
      // If accesskey isn't being pressed and the key isn't the accesskey,
      // ignore the event.
      if (!isAccessKeyDownEvent) {
@ -441,6 +492,13 @@ nsMenuBarListener::KeyDown(nsIDOMEvent* aKeyEvent)
    mAccessKeyDownCanceled = !isAccessKeyDownEvent;
  }

+  if (capturing && mAccessKey) {
+    nsMenuFrame* menuFrameForKey = GetMenuForKeyEvent(keyEvent);
+    if (menuFrameForKey) {
+      ReserveKeyIfNeeded(aKeyEvent);
+    }
+  }
+
  return NS_OK; // means I am NOT consuming event
 }

--- a/layout/xul/nsMenuBarListener.h
+++ b/layout/xul/nsMenuBarListener.h
@ -15,6 +15,7 @@
 #undef KeyPress
 #endif

+class nsMenuFrame;
 class nsMenuBarFrame;
 class nsIDOMKeyEvent;

@ -74,6 +75,18 @@ protected:

  static mozilla::Modifiers GetModifiersForAccessKey(nsIDOMKeyEvent* event);

+  /**
+   * Given a key event for an Alt+shortcut combination,
+   * return the menu, if any, that would be opened.
+   */
+  nsMenuFrame* GetMenuForKeyEvent(nsIDOMKeyEvent* aKeyEvent);
+
+  /**
+   * Call MarkAsReservedByChrome if the user's preferences indicate that
+   * the key should be chrome-only.
+   */
+  void ReserveKeyIfNeeded(nsIDOMEvent* aKeyEvent);
+
  // This should only be called by the nsMenuBarListener during event dispatch,
  // thus ensuring that this doesn't get destroyed during the process.
  void ToggleMenuActiveState();