Fix keyboard-activated context menus

Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator command. However, the accelerator that opens the context menu itself can and does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any of these keys should leave the menu as is otherwise, the context menu is effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume up; verify context menu opens. Press Search+m with ChromeVox on, verify context menu opens.
BUG=621331, 616130
Committed: https://crrev.com/ae9dad85f214ce7a33c821e28b1dacbadea9bac0
Cr-Commit-Position: refs/heads/master@{#427178}

[David Tseng, 2016-10-19 17:23:40]

Description was changed from

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

BUG=
==========

to

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

BUG=621331,616130
==========

[David Tseng, 2016-10-19 17:23:41]

dtseng@chromium.org changed reviewers:
+ afakhry@chromium.org

[David Tseng, 2016-10-19 22:50:54]

dtseng@chromium.org changed reviewers:
+ oshima@chromium.org

[David Tseng, 2016-10-19 22:50:55]

+ oshima who's an OWNER

[David Tseng, 2016-10-20 20:45:43]

On 2016/10/19 22:50:55, David Tseng wrote:
> + oshima who's an OWNER

Friendly ping; would appreciate any feedback on this long-standing issue.

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

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:596: return false;
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:26]

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:596: return false;
On 2016/10/21 18:10:42, oshima wrote:
> 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?

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).

[David Tseng, 2016-10-21 20:30:16]

Description was changed from

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

BUG=621331,616130
==========

to

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume
up; verify context menu opens. Press Search+m with ChromeVox on, verify context
menu opens.
BUG=621331,616130
==========

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

lgtm

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:596: return false;
On 2016/10/21 20:29:25, David Tseng wrote:
> On 2016/10/21 18:10:42, oshima wrote:
> > 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?
> 
> 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).

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:01:00]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 19:01:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:596: return false;
On 2016/10/24 18:53:04, oshima wrote:
> On 2016/10/21 20:29:25, David Tseng wrote:
> > On 2016/10/21 18:10:42, oshima wrote:
> > > 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?
> > 
> > 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).
> 
> 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.
> 

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

[commit-bot: I haz the power, 2016-10-24 20:45:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 20:45:36]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-10-24 23:03:53]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:04:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-10-24 23:21:34]

On 2016/10/24 19:03:56, David Tseng wrote:
>
https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
> File ash/common/accelerators/accelerator_controller.cc (right):
> 
>
https://codereview.chromium.org/2434483003/diff/1/ash/common/accelerators/acc...
> ash/common/accelerators/accelerator_controller.cc:596: return false;
> On 2016/10/24 18:53:04, oshima wrote:
> > On 2016/10/21 20:29:25, David Tseng wrote:
> > > On 2016/10/21 18:10:42, oshima wrote:
> > > > 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?
> > > 
> > > 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).
> > 
> > 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.
> > 
> 
> The menu used to run a message loop:
> https://codereview.chromium.org/2255923002/

Not all menus are converted, unfortunately (bookmark menu). We can revisit if
the flag is removed (otherwise someone may add new menu with it).

[commit-bot: I haz the power, 2016-10-24 23:21:55]

Description was changed from

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume
up; verify context menu opens. Press Search+m with ChromeVox on, verify context
menu opens.
BUG=621331,616130
==========

to

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume
up; verify context menu opens. Press Search+m with ChromeVox on, verify context
menu opens.
BUG=621331,616130
==========

[commit-bot: I haz the power, 2016-10-24 23:21:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-24 23:24:35]

Description was changed from

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume
up; verify context menu opens. Press Search+m with ChromeVox on, verify context
menu opens.
BUG=621331,616130
==========

to

==========
Fix keyboard-activated context menus

In Ash, there is logic to close menus then asynchronisly perform an accelerator
command. However, the accelerator that opens the context menu itself can and
does trigger the close path depending on how a person executes the command.

On Chrome OS, the shortcut to open a context menu is Search+Shift+Volume Up. Any
of these keys should leave the menu as is otherwise, the context menu is
effectively a no-op when triggered from the keyboard.

TEST=on device, hit search; verify apps list opens. Press Search+Shift+Volume
up; verify context menu opens. Press Search+m with ChromeVox on, verify context
menu opens.
BUG=621331,616130

Committed: https://crrev.com/ae9dad85f214ce7a33c821e28b1dacbadea9bac0
Cr-Commit-Position: refs/heads/master@{#427178}
==========

[commit-bot: I haz the power, 2016-10-24 23:24:36]

Patchset 2 (id:??) landed as
https://crrev.com/ae9dad85f214ce7a33c821e28b1dacbadea9bac0
Cr-Commit-Position: refs/heads/master@{#427178}