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
 
@@ skipping 78 matching lines @@
     root_->RemoveObserver(this);
     root_ = NULL;
   }
 
   MenuController* controller_;
   aura::Window* root_;
 
   DISALLOW_COPY_AND_ASSIGN(ActivationChangeObserverImpl);
 };
 
+// The ID that has been given to the last created run_loop.
+int g_last_run_loop_id_ = 0;
+
 }  // namespace
 
 // static
 MenuMessageLoop* MenuMessageLoop::Create() {
   return new MenuMessageLoopAura;
 }
 
-MenuMessageLoopAura::MenuMessageLoopAura() : owner_(NULL) {
+MenuMessageLoopAura::MenuMessageLoopAura()
+    : owner_(NULL),
+      menu_event_filter_(new MenuEventFilter) {
 }
 
 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 15 matching lines @@
   // |owner_| may be NULL.
   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);
+  scoped_ptr<ActivationChangeObserverImpl> observer;
   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);

[pkotwicz, 2015/08/26 01:26:54]

Don't we still need DispatcherRunLoop on Windows?

[afakhry, 2015/08/26 18:39:21]

Done.

-    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();
+    MenuEventFilter::Delegate* filter_delegate =
+        MenuEventFilter::GetMenuEventFilterDelegate(root);
+    if (filter_delegate)
+      menu_event_filter_->SetDelegate(filter_delegate);
   }
+
+  CHECK(menu_event_filter_.get());
+  aura::Env::GetInstanceDontCreate()->PrependPreTargetHandler(
+      menu_event_filter_.get());

[pkotwicz, 2015/08/26 01:26:54]

Shouldn't we only be attaching the MenuEventFilter on non-Windows? I understand
that the Windows code will never call into MenuEventFilter::OnKeyEvent() but
this is super subtle.

[afakhry, 2015/08/26 18:39:21]

Done. I kept the windows code exactly as is.

+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+
+#if defined(OS_WIN)
+  // On Windows we need the following nested dispatcher to handle some
+  // Windows-specific events like WM_CONTEXTMENU and others that can't be
+  // captured by the MenuEventFilter.
+  internal::MenuMessagePumpDispatcher nested_dispatcher(controller);
+  base::RunLoop run_loop(&nested_dispatcher);
 #else
-  internal::MenuEventDispatcher event_dispatcher(controller);
-  scoped_ptr<ui::ScopedEventDispatcher> dispatcher_override;
-  if (ui::PlatformEventSource::GetInstance()) {
-    dispatcher_override =
-        ui::PlatformEventSource::GetInstance()->OverrideDispatcher(
-            &event_dispatcher);
-  }
-  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();
-  }
-#endif
+  base::RunLoop run_loop;
+#endif  // defined(OS_WIN)
+  message_loop_quit_ = run_loop.QuitClosure();
+  // The run_loop is about to start, QuitNow() hasn't been called for it yet.
+  int current_run_loop_id = ++g_last_run_loop_id_;
+  run_loop_ids_stack_.push(current_run_loop_id);
+  run_loops_status_[current_run_loop_id] = false;
+
+  run_loop.Run();
+
+  // Now erase this run_loop ID to mark it as returned.
+  run_loops_status_.erase(current_run_loop_id);
+  run_loop_ids_stack_.pop();
 }
 
 void MenuMessageLoopAura::QuitNow() {
   CHECK(!message_loop_quit_.is_null());
+
+  if (run_loop_ids_stack_.empty())
+    return;
+
+  // This RunLoop has already returned? We should not call QuitNow() again.
+  int current_run_loop_id = run_loop_ids_stack_.top();
+  auto itr = run_loops_status_.find(current_run_loop_id);
+  if (itr == run_loops_status_.end())
+    return;
+
+  // QuitNow() has already been called?
+  if (itr->second)
+    return;
+
+  // Overwrite the status so that QuitNow() is marked as called for this
+  // RunLoop.
+  run_loops_status_[current_run_loop_id] = true;
+
   message_loop_quit_.Run();
 
+  aura::Env::GetInstanceDontCreate()->RemovePreTargetHandler(
+      menu_event_filter_.get());
+  menu_event_filter_->ClearDelegate();

[pkotwicz, 2015/08/26 01:26:54]

With this CL applied, if I:
1) Open the wrench menu
2) Right click on one of the wrench menu's bookmarks to bring up the context
menu
3) Press escape to close the context menu
I am unable to use the arrow keys for navigating the menu anymore

[afakhry, 2015/08/26 18:39:21]

That's weird. I can't repro this at all. In fact the above code has been added
to prevent this bug, since MenuController::TerminateNestedMessageLoop() can be
called multiple times before the RunLoop has actually exited, which results in
calling MenuMessageLoopAura::QuitNow() multiple times before the runloop exits. 

There's event an interactive_ui_test I believe that was written by you,
BookmarkBarViewTest24.ContextMenusKeyboardEscape, which I made sure it passes
after I added the above code.

+
 #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