Enable keyboard accelerators while a menu is open

ash/accelerators/accelerator_table.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/accelerators/accelerator_table.h"
 
 #include "base/basictypes.h"
 
 namespace ash {
 
@@ skipping 409 matching lines @@
     WINDOW_CYCLE_SNAP_DOCK_RIGHT,
     WINDOW_MINIMIZE,
     TOGGLE_FULLSCREEN,
     TOGGLE_MAXIMIZED,
     WINDOW_POSITION_CENTER,
     ROTATE_WINDOW,
 };
 
 const size_t kActionsNeedingWindowLength = arraysize(kActionsNeedingWindow);
 
+const AcceleratorAction kActionsKeepingMenuOpen[] = {

[pkotwicz, 2015/08/27 17:19:55]

You should double check which accelerators should not close the menu with UX. My
preference would be to make this list as short as possible. Potentially have
just: { TOGGLE_CAPS_LOCK, DISABLE_CAPS_LOCK, PREVIOUS_IME, NEXT_IME, SWITCH_IME,
TAKE_SCREENSHOT, SILENCE_SPOKEN_FEEDBACK }

[afakhry, 2015/08/28 01:24:41]

I agree, however, I also believe that one should be able to control the
brightness, the volume, and the spoken feedback while the menu is open.
Especially the volume and brightness because I remember seeing those in one of
your design docs. In any case, I will try to get feedback on this list as you
suggested.

+    MEDIA_NEXT_TRACK,
+    MEDIA_PLAY_PAUSE,
+    MEDIA_PREV_TRACK,
+    NEXT_IME,
+    PREVIOUS_IME,
+    PRINT_UI_HIERARCHIES,
+    SWITCH_IME,
+    TAKE_PARTIAL_SCREENSHOT,

[pkotwicz, 2015/08/27 17:19:55]

There are bugs with taking a partial screenshot while a menu is open. e.g. I can
use the arrow keys to navigate through the menu because MenuEventFilter gets key
events before PartialScreenshotController does. Perhaps the partial screenshot
functionality should be fixed to work better with menus but that should be done
in a separate CL

[afakhry, 2015/08/28 01:24:41]

I see. That can be easily fixed later by maybe adding a null-pre-target handler
while the partial screenshot taking is in progress.

[pkotwicz, 2015/08/28 23:44:25]

Can you file a bug for fixing this?

[afakhry, 2015/08/29 01:31:23]

Will do once this CL lands. It doesn't make sense to file a bug saying hey we
can navigate the menu using the arrow keys while a partial screenshot is in
progress when we can't even do that now.

+    TAKE_SCREENSHOT,
+#if defined(OS_CHROMEOS)
+    BRIGHTNESS_DOWN,
+    BRIGHTNESS_UP,
+    DISABLE_CAPS_LOCK,
+    DISABLE_GPU_WATCHDOG,
+    KEYBOARD_BRIGHTNESS_DOWN,
+    KEYBOARD_BRIGHTNESS_UP,
+    SILENCE_SPOKEN_FEEDBACK,
+    TOGGLE_CAPS_LOCK,
+    TOGGLE_MIRROR_MODE,

[pkotwicz, 2015/08/27 17:19:55]

The TOGGLE_MIRROR_MODE accelerator causes a crash if it is processed while the
menu is still open. Removing it from this list seems to fix the problem

[afakhry, 2015/08/28 01:24:41]

That's weird! I tried the mirror mode with two displays and the menu open. No
crash occurred. I removed this accelerator from this list. I'm not sure why I
added it here in the first place.

[pkotwicz, 2015/08/28 23:44:25]

To get the crash you must press the TOGGLE_MIRROR_MODE accelerator when a menu
is open for a browser window which is on the secondary display

+    TOGGLE_SPOKEN_FEEDBACK,
+    TOGGLE_TOUCH_VIEW_TESTING,

[pkotwicz, 2015/08/27 17:19:55]

This accelerator maximizes the window (if you are running with
--ash-enable-touch-view-testing). The menu does not reposition itself to take
into account the fact that the window got maximized. We should remove the
accelerator from this list to get rid of the weird behavior

[afakhry, 2015/08/28 01:24:41]

Done.

+    TOGGLE_WIFI,
+    VOLUME_DOWN,
+    VOLUME_MUTE,
+    VOLUME_UP,
+#endif  // defined(OS_CHROMEOS)
+};
+
+const size_t kActionsKeepingMenuOpenLength = arraysize(kActionsKeepingMenuOpen);
+
 }  // namespace ash