Fix keyboard-activated context menus

ash/common/accelerators/accelerator_controller.cc

Index: ash/common/accelerators/accelerator_controller.cc
diff --git a/ash/common/accelerators/accelerator_controller.cc b/ash/common/accelerators/accelerator_controller.cc
index aa8bcf0573b83f1d71afcfe1d39df6d2c34ccccb..a74f20d7561512c1945c2089a318da8b2c20875f 100644
--- a/ash/common/accelerators/accelerator_controller.cc
+++ b/ash/common/accelerators/accelerator_controller.cc
@@ -587,6 +587,14 @@ void AcceleratorController::SetImeControlDelegate(
 
 bool AcceleratorController::ShouldCloseMenuAndRepostAccelerator(
     const ui::Accelerator& accelerator) const {
+  // The context menu can be activated via Search+Shift+Volume up. When seeing
+  // any key up events involving these keys, don't ever close the menu as it was
+  // involved in opening the menu.
+  if (accelerator.type() == ui::ET_KEY_RELEASED &&
+      (accelerator.key_code() == ui::VKEY_LWIN || accelerator.IsShiftDown() ||
+       accelerator.key_code() == ui::VKEY_F10))
+    return false;

[oshima, 2016/10/21 18:10:42]

Looks like this is caused by the TOGGLE_APP_LIST accelerator, which happens to
be VKEY_LWIN + release, is not registered as "keep menu".

This will make TOGGLE_APP_LIST (and maybe others) keeps the menu opened.

Can you try adding TOGGLE_APP_TEST to the actions_keeping_menu_open_ and see if
it fixes the issue, and then test if app list opens correctly when you press
that accelerator while context menu is open?

[David Tseng, 2016/10/21 20:29:25]

Done.

This appears to fix the issue; thanks for the suggestion. Tested on device with
app list shortcut and search+shift+f10 and ChromeVox on/off.

In general though, I'm surprised we're closing the menu by default. Seems like
if the shortcut causes a focus change, then the menu would close anyway, but I'm
probably missing the original motivation. There are probably a few other
accelerators that should leave menu open (e.g. scale ui up).

[oshima, 2016/10/24 18:53:04]

This is because the menu runs nested message loop, and it's not safe to run
arbitrary
command inside nested loop. The regular menu command works this way too (it
first exit,
then execute).

We can add more if it's important and safe to do so, so please file a bug.

[David Tseng, 2016/10/24 19:03:56]

The menu used to run a message loop:
https://codereview.chromium.org/2255923002/

+
   auto itr = accelerators_.find(accelerator);
   if (itr == accelerators_.end())
     return false;  // Menu shouldn't be closed for an invalid accelerator.