Enable keyboard accelerators while a menu is open

ui/views/controls/menu/menu_message_loop_aura.cc

 // Copyright 2014 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_message_loop_aura.h"
 
 #if defined(OS_WIN)
 #include <windowsx.h>
 #endif
 
 #include "base/run_loop.h"
 #include "ui/aura/client/screen_position_client.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/events/event.h"
 #include "ui/events/platform/platform_event_source.h"
 #include "ui/events/platform/scoped_event_dispatcher.h"
 #include "ui/views/controls/menu/menu_controller.h"
+#include "ui/views/controls/menu/menu_event_filter.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/public/activation_change_observer.h"
 #include "ui/wm/public/activation_client.h"
 #include "ui/wm/public/dispatcher_client.h"
 #include "ui/wm/public/drag_drop_client.h"
 
 #if defined(OS_WIN)
 #include "ui/base/win/internal_constants.h"
 #include "ui/views/controls/menu/menu_message_pump_dispatcher_win.h"
 #include "ui/views/win/hwnd_util.h"
@@ skipping 65 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(ActivationChangeObserverImpl);
 };
 
 }  // namespace
 
 // static
 MenuMessageLoop* MenuMessageLoop::Create() {
   return new MenuMessageLoopAura;
 }
 
-MenuMessageLoopAura::MenuMessageLoopAura() : owner_(NULL) {
+MenuMessageLoopAura::MenuMessageLoopAura() : owner_(nullptr) {
 }
 
 MenuMessageLoopAura::~MenuMessageLoopAura() {
 }
 
 void MenuMessageLoopAura::RepostEventToWindow(const ui::LocatedEvent& event,
                                               gfx::NativeWindow window,
                                               const gfx::Point& screen_loc) {
   aura::Window* root = window->GetRootWindow();
   ScreenPositionClient* spc = aura::client::GetScreenPositionClient(root);
@@ skipping 16 matching lines @@
   owner_ = owner;
   aura::Window* root = GetOwnerRootWindow(owner_);
   // It is possible for the same MenuMessageLoopAura to start a nested
   // message-loop while it is already running a nested loop. So make sure the
   // quit-closure gets reset to the outer loop's quit-closure once the innermost
   // loop terminates.
   base::AutoReset<base::Closure> reset_quit_closure(&message_loop_quit_,
                                                     base::Closure());
 
 #if defined(OS_WIN)
   internal::MenuMessagePumpDispatcher nested_dispatcher(controller);

[sky, 2015/09/03 17:23:21]

Why is the filtering specific to non-windows platforms?

[afakhry, 2015/09/04 01:49:42]

On Windows, we have the MenuMessagePumpDispatcher that has to handle certain
Windows specific key events, and the fact that Windows sends two event types for
each key press WM_KEYDOWN and WM_CHAR. 

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

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

I don't see how that handles the ash accelerators when run in ash mode though?

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

Exactly, because the current code is very unclear. That's why we want to get rid
of these nested dispatchers for Windows too, Probably in a follow up CL. This CL
is already too big.

To explain how Windows accelerators handling works. We create the
DispatcherRunLoop
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/public/dispa...
which will ask the DispatcherClient to prepare the runloop closures. This will
in this case create a NestedAcceleratorDispatcher
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...
Which will in turn create a NestedAcceleratorDispatcherWin
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...,
and that will handle the accelerator
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/nested_...

This is the same thing for Linux and chromeos rightnow, and the MenuEventHandler
is supposed to replace all of that.

   if (root) {
     scoped_ptr<ActivationChangeObserverImpl> observer;
     if (!nested_menu)
       observer.reset(new ActivationChangeObserverImpl(controller, root));
     aura::client::DispatcherRunLoop run_loop(
         aura::client::GetDispatcherClient(root), &nested_dispatcher);
     message_loop_quit_ = run_loop.QuitClosure();
     run_loop.Run();
   } else {
     base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
     base::MessageLoop::ScopedNestableTaskAllower allow(loop);
     base::RunLoop run_loop(&nested_dispatcher);
     message_loop_quit_ = run_loop.QuitClosure();
     run_loop.Run();
   }
 #else
-  internal::MenuEventDispatcher event_dispatcher(controller);
-  scoped_ptr<ui::ScopedEventDispatcher> dispatcher_override;
-  if (ui::PlatformEventSource::GetInstance()) {
-    dispatcher_override =
-        ui::PlatformEventSource::GetInstance()->OverrideDispatcher(
-            &event_dispatcher);
-  }
+  scoped_ptr<ActivationChangeObserverImpl> observer;
+  MenuEventFilter::Delegate* filter_delegate = nullptr;
   if (root) {
-    scoped_ptr<ActivationChangeObserverImpl> observer;
     if (!nested_menu)
       observer.reset(new ActivationChangeObserverImpl(controller, root));
-    aura::client::DispatcherRunLoop run_loop(
-        aura::client::GetDispatcherClient(root), NULL);
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
-  } else {
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
-    base::RunLoop run_loop;
-    message_loop_quit_ = run_loop.QuitClosure();
-    run_loop.Run();
+    filter_delegate = MenuEventFilter::GetMenuEventFilterDelegate(root);
   }
-#endif
+
+  MenuEventFilter menu_event_filter(controller, filter_delegate);

[sky, 2015/09/03 17:23:21]

AFAICT the functionality you need is the ability to install an EventHandlerwhen
the menu is shown. If this is the case, why do you need all this logic and not a
more straightforward way to do that? Say a function on ViewsDelegate that is
called when the menu is shown allowing you to add an EventHandler?

[afakhry, 2015/09/04 01:49:42]

I'm not sure I understand your suggestion. What's the problem with using what we
already have in aura::Env to add and remove the EventHandler? Currently
ViewsDelegate doesn't have this functionality, which will make this change an
even bigger refactor. Could you please explain a bit more?

[sky, 2015/09/04 14:38:54]

I'm trying to understand why you are doing the change you're doing.

I agree with you adding/removing an EventHandler is the way to go. What I don't
understand is why we need MenuEventFilter in views side of code. I'm suggesting
the logic of MenuEventFilter be moved into chrome/ash by way of ViewsDelegate.
ViewsDelegate gets a method that is called at the right time to allow you to
install the EventHandler you need to. Does that make sense?

[oshima, 2015/09/04 18:27:01]

My understanding of the role of ViewsDelegate is to customize views behavior for
views client such as ash/chrome, right?
I think the issue is that current MenuEventFilter has a mix of platform and
client behavior. How about
removing the accelerator handling part from MenuEventFilter and move it to the
implementation of MenuEventFilter::Delegate, then
let ViewsDelegate return the MenuEventFilter::Delegate instead of
MenuEventFilter::GetMenuEventFilterDelegate. What do you think?

I also think adding MenuEventFilter for every message loop is overkill though.
Can't this be moved to MenuController so that it gets created only once when
menu is open?

[oshima, 2015/09/04 18:32:10]

Or just have ViewsDelegate::HandleKeyEventOnMenu

(we probably don't need separate Delegate instance)

[afakhry, 2015/09/08 21:31:46]

Done.

+  aura::Env::GetInstanceDontCreate()->PrependPreTargetHandler(
+      &menu_event_filter);
+
+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+  base::RunLoop run_loop;
+  message_loop_quit_ = run_loop.QuitClosure();
+
+  run_loop.Run();
+
+  aura::Env::GetInstanceDontCreate()->RemovePreTargetHandler(
+      &menu_event_filter);
+#endif  // defined(OS_WIN)
 }
 
 void MenuMessageLoopAura::QuitNow() {
   CHECK(!message_loop_quit_.is_null());
   message_loop_quit_.Run();
 
 #if !defined(OS_WIN)
   // Ask PlatformEventSource to stop dispatching events in this message loop
   // iteration. We want our menu's loop to return before the next event.
   if (ui::PlatformEventSource::GetInstance())
     ui::PlatformEventSource::GetInstance()->StopCurrentEventStream();
 #endif
 }
 
 void MenuMessageLoopAura::ClearOwner() {
   owner_ = NULL;
 }
 
 }  // namespace views