Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_event_filter.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_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_SHIFT_DOWN | ui::EF_CONTROL_DOWN |
+                          ui::EF_ALT_DOWN | ui::EF_COMMAND_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 ProcessAccelerator(const ui::Accelerator& accelerator,
+                          MenuEventFilter* owner) override {
+    return true;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DefaultMenuDelegate);
+};
+
+}  // namespace
+
+MenuEventFilter::MenuEventFilter()
+    : default_delegate_(new DefaultMenuDelegate),
+      filter_delegate_(nullptr) {
+}
+
+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);
+
+  MenuController* menu_controller = MenuController::GetActiveInstance();
+  CHECK(menu_controller);
+
+  bool should_stop_propagation = true;
+  const int flags = event->flags();
+  if (event->type() != ui::ET_KEY_PRESSED || (flags & kKeyFlagsMask) != 0) {

[pkotwicz, 2015/06/16 16:04:59]

There's a bunch of subtle differences between
MenuEventDispatcher::DispatchEvent() and MenuEventFilter::OnKeyEvent().
For instance:
- MenuEventFilter::OnKeyEvent() only calls MenuController::SelectByChar() if
there are no ui::EventFlags
- MenuEventDispatcher::DispatchEvent() calls MenuController::OnKeyDown() even if
Ctrl is pressed but MenuEventFilter does not.
- The return values of MenuController::OnKeyDown() and
MenuController::SelectByChar() affect whether
MenuController::TerminateNestedMessageLoop() gets called in
MenuEventDispatcher::DispatchEvent() but not in
MenuEventDispatcher::DispatchEvent()

I think that all of the subtle changes that you are making are correct. However,
it would be great if you can put these changes in a different CL which you land
before you land this CL.
I think (but am not sure) that MenuController::OnKeyDown() does not need a
return value. (This would be an awesome simplification)

[afakhry, 2015/06/16 23:56:39]

Right, in the old code, we call MenuController::SelectByChar only if the flags
show that Alt and Ctrl are not pressed. I added the shift and command flags too
because when any is pressed we need to handle a potential accelerator and
otherwise handle MenuController::SelectByChar.
Right, MenuController::OnKeyDown() expect certain key codes that it will handle
and no flags are expected. For example, why should pressing the RIGHT key be
equivalent to pressing Ctrl+RIGHT. It doesn't make sense.
Calling MenuController::OnKeyDown() or MenuController::SelectByChar() will
result in changing the value of MenuController::exit_type() (if the menu needs
to exit in correspondence with the return values of these methods). This will
have same effect since we already have the following code below:

if (menu_controller->exit_type() != MenuController::EXIT_NONE)
  menu_controller->TerminateNestedMessageLoop();
I'm not sure why that would be more preferred here, but I'm open to whatever is
necessary. Let's see what other reviewers will say.

[pkotwicz, 2015/06/17 14:26:24]

I prefer making these subtle changes in a different CL because:
- It makes the job of reviewing this CL much easier. It is much simpler if
MenuEventFilter can be assumed to be a trivial refactor rather one with really
subtle differences. (It took me a while to notice them)
- It is possible to take the subtle changes that you are making in this CL to
completion in a separate CL. For instance, it does not make sense for
MenuController::OnKeyDown() to have a return value if the return value is
redundant and can be inferred from MenuController::exit_type().

[pkotwicz, 2015/07/10 17:33:21]

I still think that making these changes in a separate CL is useful.

+    should_stop_propagation =
+        delegate->ProcessAccelerator(accelerator, this);
+  } else {
+    if (menu_controller->OnKeyDown(event->key_code())) {
+      char c = ui::GetCharacterFromKeyCode(event->key_code(), flags);
+      if (!menu_controller->SelectByChar(c)) {
+        should_stop_propagation = delegate->ProcessAccelerator(accelerator,
+                                                               this);
+      }
+    }
+  }
+
+  if (menu_controller->exit_type() != MenuController::EXIT_NONE)
+    menu_controller->TerminateNestedMessageLoop();
+
+  if (should_stop_propagation)
+    event->StopPropagation();

[pkotwicz, 2015/06/16 16:04:59]

Mnemonics are only run when base::RunLoop has finished. I wonder whether we
should do the same for accelerators.

[afakhry, 2015/06/16 23:56:39]

That's exactly what we're doing here, right? Take a look at
AshMenuEventEventFilterDelegate::ProcessAccelerator(). If the accelerator is not
one of those that are allowed to keep the menu open while being processed, the
menu will be closed (which will result in the termination of the nested message
loop) and then the event will be allowed to continue its propagation, and the
accelerator will be processed normally by the AcceleratorController outside of
the menu message loop.

To test this you can apply the patch, add some logging statements, build and
run, open a menu and then press Ctrl+Alt+/ to open the keyboard overlay, which
is not one of the accelerators that will keep the menu open. See that it will be
processed only after the menu has been closed.

[pkotwicz, 2015/06/17 14:26:24]

Sorry that I was unclear.
In your CL, if a user presses an accelerator which closes the wrench menu:
- The menu closes
- The accelerator action is run
- The base::RunLoop ends sometime in the future.
MenuController::TerminateNestedMessageLoop() asynchronously ends the
base::RunLoop. If a user presses an accelerator while dragging a bookmark from
the wrench menu, it is possible that the base::RunLoop keeps on running for
minutes after MenuController::TerminateNestedMessageLoop() has been called.

If a user presses a mnemonic while the wrench menu is open:
- The menu closes
- The base::RunLoop ends sometime in the future
- The mnemonic action is run only once the base::RunLoop has ended (It is run in
MenuRunnerImpl::MenuDone())

[afakhry, 2015/06/17 18:48:00]

Thanks for clarifying. I understand what you mean now. In both situations
(mnemonics and accelerators) The

-> RunLoop::Quit();
   |
   --> MessageLopp::QuitNow();

are both executed before we handle either the mnemonic or accelerator. However,
in the case of mnemonics, it is executed as you mentioned in
MenuRunnerImpl::MenuDone() after:

-> RunLoop::AfterRun();

Which of course pops the previous run loop from the stack.

I tried the example you gave earlier (dragging a bookmark) and didn't see any
issues and validated the the runloops quit normally.

Whether processing the accelerators after RunLoop::AfterRun() has completed is
the right thing to do, I'm not sure, since I'm not seeing any issues, but
whether it's doable, yes it's very easy. So if you could explain this a bit more
so that I can understand why we should do it, that's be great.

[pkotwicz, 2015/06/18 19:45:57]

I am unsure what the right thing to do is. It would be useful to understand why
the mnemonics processing is the way it is. The scary prospect is than an
accelerator starts a base::RunLoop (I do not know of any which do). Accelerators
do pretty complex things (log out of Chrome OS, open a new window)

+}
+
+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);
+  }
+}
+
+void MenuEventFilter::SetDelegate(MenuEventFilter::Delegate* delegate) {
+  DCHECK(delegate);
+  filter_delegate_ = delegate;
+}
+
+void MenuEventFilter::ClearDelegate() {
+  filter_delegate_ = nullptr;
+}
+
+void MenuEventFilter::CloseMenu() const {
+  MenuController* menu_controller = MenuController::GetActiveInstance();
+  CHECK(menu_controller);
+  menu_controller->CancelAll();
+}
+
+}  // namespace views