Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_event_handler.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/controls/menu/menu_event_handler.h"
+
+#include "ui/aura/env.h"
+#include "ui/events/keycodes/keyboard_code_conversion.h"
+#include "ui/views/controls/menu/menu_controller.h"
+#include "ui/views/views_delegate.h"
+
+namespace views {
+
+namespace {
+
+const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
+
+}  // namespace
+
+MenuEventHandler::MenuEventHandler() {
+  aura::Env::GetInstanceDontCreate()->PrependPreTargetHandler(this);
+}
+
+MenuEventHandler::~MenuEventHandler() {
+  aura::Env::GetInstanceDontCreate()->RemovePreTargetHandler(this);
+}
+
+void MenuEventHandler::OnKeyEvent(ui::KeyEvent* event) {
+  MenuController* menu_controller = MenuController::GetActiveInstance();
+  DCHECK(menu_controller);
+
+  if (menu_controller->exit_type() == MenuController::EXIT_ALL ||
+      menu_controller->exit_type() == MenuController::EXIT_DESTROYED) {
+    // If the event has arrived after the menu's exit type had changed but
+    // before its message loop terminated, the event will continue its normal
+    // propagation for the following reason:
+    // If the user accepts a menu item in a nested menu, the menu item action is
+    // run after the base::RunLoop for the innermost menu has quit but before
+    // the base::RunLoop for the outermost menu has quit. If the menu item
+    // action starts a base::RunLoop, the outermost menu's base::RunLoop will
+    // not quit till the action's base::RunLoop ends. IDC_BOOKMARK_BAR_OPEN_ALL
+    // sometimes opens a modal dialog. The modal dialog starts a base::RunLoop
+    // and keeps the base::RunLoop running for the duration of its lifetime.
+    menu_controller->TerminateNestedMessageLoop();
+    return;
+  } else {

[sky, 2015/09/09 15:44:52]

Style guide says no else after return.

[afakhry, 2015/09/09 20:21:33]

Done.

+    if (event->type() == ui::ET_KEY_PRESSED) {
+      menu_controller->OnKeyDown(event->key_code());
+
+      // Below we'll handle the mnemonics only if neither Alt nor Ctrl is

[sky, 2015/09/09 15:44:52]

This comment is documenting the code and doesn't tell me why the processing is
doing what it is doing. Document the why. Why is it important to only handle
when alt nor ctrl is down?

[afakhry, 2015/09/09 20:21:33]

The why here is not so clear to me. I think it was a design decision. See the
original code here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

I think the reason is that for example Ctrl+t is an accelerator but 't' only is
a mnemonic. I improved the comment.

+      // pressed.
+      const int flags = event->flags();
+      if (menu_controller->exit_type() == MenuController::EXIT_NONE &&
+          (flags & kKeyFlagsMask) == 0) {
+        char c = ui::DomCodeToUsLayoutCharacter(event->code(), flags);
+        menu_controller->SelectByChar(c);
+      }
+    }
+  }
+
+  ViewsDelegate::GetInstance()->HandleKeyEventOnMenu(event);
+
+  if (menu_controller->exit_type() != MenuController::EXIT_NONE)
+    menu_controller->TerminateNestedMessageLoop();
+
+  event->StopPropagation();
+}
+
+void MenuEventHandler::OnTouchEvent(ui::TouchEvent* event) {
+  if (event->type() == ui::ET_TOUCH_RELEASED ||
+      event->type() == ui::ET_TOUCH_CANCELLED) {
+    // Don't allow the event copy to clear the native touch id
+    // mapping, or we'll lose the mapping before the initial event
+    // has finished being dispatched.
+    event->set_should_remove_native_touch_id_mapping(false);
+  }
+}
+
+}  // namespace views