Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_event_filter.cc

Index: ui/views/controls/menu/menu_event_filter.cc
diff --git a/ui/views/controls/menu/menu_event_filter.cc b/ui/views/controls/menu/menu_event_filter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7774dbc7f52cc91812351e76f53ea2037b035187
--- /dev/null
+++ b/ui/views/controls/menu/menu_event_filter.cc
@@ -0,0 +1,159 @@
+// 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_filter.h"
+
+#include "ui/aura/window.h"
+#include "ui/aura/window_property.h"
+#include "ui/base/accelerators/accelerator.h"
+#include "ui/events/keycodes/keyboard_code_conversion.h"
+#include "ui/views/controls/menu/menu_controller.h"
+
+DECLARE_WINDOW_PROPERTY_TYPE(views::MenuEventFilter::Delegate*);
+
+DEFINE_LOCAL_WINDOW_PROPERTY_KEY(views::MenuEventFilter::Delegate*,
+                                 kMenuEventFilterDelegateKey,
+                                 nullptr);
+
+namespace views {
+
+namespace {
+
+const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
+
+// Defines a NULL-Object delegate for non-ash platforms.
+class DefaultMenuDelegate : public MenuEventFilter::Delegate {
+ public:
+  DefaultMenuDelegate() {}
+  ~DefaultMenuDelegate() override {}
+
+  // MenuEventFilter::Delegate:
+  void StoreInHistory(const ui::Accelerator& accelerator) override {}
+  bool ShouldCloseMenuAndRepostAccelerator(
+      const ui::Accelerator& accelerator) const override {
+    return false;
+  }
+  void ProcessAcceleratorNow(const ui::Accelerator& accelerator) override {
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DefaultMenuDelegate);
+};
+
+// Reposts the |accelerator| to be processed later.
+void RepostAccelerator(const ui::Accelerator& accelerator,
+                       MenuEventFilter::Delegate* delegate) {
+  base::MessageLoopForUI::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&MenuEventFilter::Delegate::ProcessAcceleratorNow,
+                 base::Unretained(delegate),
+                 accelerator));
+}
+
+}  // namespace
+
+MenuEventFilter::MenuEventFilter(MenuController* menu_controller)
+    : menu_controller_(menu_controller),
+      default_delegate_(new DefaultMenuDelegate),
+      filter_delegate_(nullptr) {
+  DCHECK(menu_controller_);
+}
+
+MenuEventFilter::~MenuEventFilter() {
+}
+
+// static
+void MenuEventFilter::SetMenuEventFilterDelegate(
+    aura::Window* root_window,
+    MenuEventFilter::Delegate* delegate) {
+  DCHECK(root_window);
+  DCHECK(delegate);
+  DCHECK_EQ(root_window->GetRootWindow(), root_window);
+  root_window->SetProperty(kMenuEventFilterDelegateKey, delegate);
+}
+
+// static
+MenuEventFilter::Delegate* MenuEventFilter::GetMenuEventFilterDelegate(
+    aura::Window* root_window) {
+  DCHECK(root_window);
+  DCHECK_EQ(root_window->GetRootWindow(), root_window);
+  return root_window->GetProperty(kMenuEventFilterDelegateKey);
+}
+
+void MenuEventFilter::OnKeyEvent(ui::KeyEvent* event) {
+  DCHECK(event);
+
+  MenuEventFilter::Delegate* delegate =
+      filter_delegate_ ? filter_delegate_ : default_delegate_.get();
+  DCHECK(delegate);
+
+  // First record the current accelerator (this is normally done by the
+  // AcceleratorFilter, but since the MenuEventFilter will precede the
+  // AcceleratorFilter in the pre-target handlers list, we have to do it here).
+  ui::Accelerator accelerator(*event);
+  delegate->StoreInHistory(accelerator);
+
+  CHECK(menu_controller_);
+
+  bool should_close_and_repost_accelerator = false;
+
+  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.

[pkotwicz, 2015/08/28 22:10:09]

Please include a comment as to why we do not stop propagation of the event. My
suggestion:
"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."

[afakhry, 2015/08/28 23:32:12]

Copied your suggested comment as is.

+    menu_controller_->TerminateNestedMessageLoop();
+    return;
+  } else {
+    if (event->type() == ui::ET_KEY_PRESSED) {
+      menu_controller_->OnKeyDown(event->key_code());
+
+      const int flags = event->flags();
+      if (menu_controller_->exit_type() == MenuController::EXIT_NONE &&
+          (flags & kKeyFlagsMask) == 0) {
+        // Only check mnemonics if the menu hasn't exited as a result from
+        // MenuController::OnKeyDown() and no modifiers are pressed.
+        char c = ui::GetCharacterFromKeyCode(event->key_code(), flags);
+        menu_controller_->SelectByChar(c);
+      }
+    }
+  }
+
+  if (menu_controller_->exit_type() == MenuController::EXIT_NONE) {
+    should_close_and_repost_accelerator =
+        delegate->ShouldCloseMenuAndRepostAccelerator(accelerator);
+  } else {
+    menu_controller_->TerminateNestedMessageLoop();
+  }
+
+  // The event's propagation will always be stopped.
+  event->StopPropagation();
+
+  if (should_close_and_repost_accelerator) {
+    menu_controller_->CancelAll();
+    RepostAccelerator(accelerator, delegate);

[pkotwicz, 2015/08/28 22:10:10]

I have an idea to make this slightly clearer:
Change MenuController::Run() to return a MenuController::PostCloseAction struct.
I am thinking:
struct MenuController::PostCloseAction {
int command;
ui::Accelerator accelerator;
};

I would call MenuEventFilter::Delegate::ProcessAcceleratorNow() from
MenuRunnerImpl::MenuDone().
MenuEventFilter would call MenuController::set_post_close_action() if the
accelerator should be run once the menu is closed

I think I better understand why mnemonics actions are run in
MenuRunnerImpl::MenuDone() thanks to the
BookmarkBarViewTest12.CloseWithModalDialog test failure in the earlier CL

I will defer to sky@ as to what the correct design is.

[afakhry, 2015/08/28 23:32:12]

Sure, let's defer to sky@.

+  } else {
+    delegate->ProcessAcceleratorNow(accelerator);
+  }
+}
+
+void MenuEventFilter::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);

[pkotwicz, 2015/08/28 22:10:10]

I think that this is no longer necessary because we no longer call
ui::EventFromNative(). You should double check with tdresser@ though who added
this code. See https://codereview.chromium.org/886593004

[afakhry, 2015/08/28 23:32:12]

If it exists in MenuEventDispatcher, it should still remain here. Isn't that
your whole philosophy of not introducing too many changes in one CL? :) This is
a subject for another CL I think.

[pkotwicz, 2015/08/29 00:05:11]

I think that:

Correct:
scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
copy->set_should_remove_native_touch_id_mapping(false);

Correct:
// No code

Incorrect:
scoped_ptr<ui::Event> copy(ui::EventFromNative(event));
copy->set_should_remove_native_touch_id_mapping(true);

Incorrect:
event->set_should_remove_native_touch_id_mapping(false)

You should double check with tdresser@ though

[afakhry, 2015/08/29 01:31:23]

|event| here is not a native event so we can't do ui::EventFromNative() here.
The copy also needs to be a TouchEvent.
But we should we create a copy?
Once sky@ and oshima@ review this CL, we can ask tdresser@ about this  line.

[pkotwicz, 2015/08/29 02:30:45]

Ok, let's ask tdresser@

+  }
+}
+
+void MenuEventFilter::SetDelegate(MenuEventFilter::Delegate* delegate) {
+  filter_delegate_ = delegate;
+}
+
+void MenuEventFilter::ClearDelegate() {

[pkotwicz, 2015/08/28 22:10:10]

You no longer use this method. Can you remove it?

[afakhry, 2015/08/28 23:32:12]

But I already did. You are reviewing an older patch!

+  filter_delegate_ = nullptr;
+}
+
+}  // namespace views