Fix keyboard-activated context menus

ash/common/accelerators/accelerator_controller.cc

 // Copyright (c) 2012 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 "ash/common/accelerators/accelerator_controller.h"
 
 #include "ash/common/accelerators/accelerator_commands.h"
 #include "ash/common/accelerators/accelerator_controller_delegate.h"
 #include "ash/common/accelerators/debug_commands.h"
 #include "ash/common/accessibility_delegate.h"
@@ skipping 569 matching lines @@
   return GetAcceleratorProcessingRestriction(-1);
 }
 
 void AcceleratorController::SetImeControlDelegate(
     std::unique_ptr<ImeControlDelegate> ime_control_delegate) {
   ime_control_delegate_ = std::move(ime_control_delegate);
 }
 
 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.
 
   AcceleratorAction action = itr->second;
   return actions_keeping_menu_open_.count(action) == 0;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // AcceleratorController, ui::AcceleratorTarget implementation:
@@ skipping 523 matching lines @@
   if (wm_shell->mru_window_tracker()->BuildMruWindowList().empty() &&
       actions_needing_window_.find(action) != actions_needing_window_.end()) {
     wm_shell->accessibility_delegate()->TriggerAccessibilityAlert(
         A11Y_ALERT_WINDOW_NEEDED);
     return RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
   }
   return RESTRICTION_NONE;
 }
 
 }  // namespace ash