+shibasheikh@ to confirm that We do not close the KSH window with Esc. This is consistent with the behavior with other windows, e.g. feekback.

There doesn't appear to be *any* way to close the keyboard shortcut helper using the keyboard right now. That seems to be broken from an a11y perspective.

I'd suggest closing on Escape, since Ctrl+Alt+/ is easy to hit accidentally when trying to type other accelerators (see e.g. http://g/chromeos-discuss/prrSrqdoTiA/discussion) and since Escape is probably the first thing a user is going to press to try to close this window.

Comment #1 makes the argument that normal windows can't be closed with Escape, but Ctrl+W doesn't work for closing the KSH window either.

For good measure, I'd make Ctrl+Alt+/ also close the window so that it's easy to toggle it off again.

search keywords: keyboard overlay

shibasheikh@ and ovanieva@, could you please raise this to UI review. Thanks.

that's a good call abodeti@/
given that it's system app, we will add the same treatment as we do with others. Cntrl + W shortcut will close KSH.

hope that works.

Hi derat@, "Esc" is used in Keyboard Shortcut Helper to exit the "search mode". During the UI review, we try to make the behavior consistent to the "Settings" search mode, so to use "Esc" to close KSH window is not the first choice here.

I will add "Ctrl + W" to close the window first. If you think to use "Ctrl+Alt+/" to toggle the KSH window is necessary, let us discuss with UX and PM more about it. Thanks.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa466d805d47fe3447f426acc7b2c349588d0540

commit fa466d805d47fe3447f426acc7b2c349588d0540
Author: wutao <wutao@chromium.org>
Date: Thu Apr 12 18:32:56 2018

cros: Close Shortcuts window by CTRL+W

Currently Shortcuts window can only be closed by mouse click. This cl
adds CTRL+W accelerator to close the window.

Bug:  819815 
Test: KeyboardShortcutViewTest.CloseWindowByAccelerator
Change-Id: Icfa340e5c947f823ea62e25df06536f216563ec7
Reviewed-on: https://chromium-review.googlesource.com/1006115
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#550281}
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/BUILD.gn
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view.h
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc

I will mark this by fixed now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa466d805d47fe3447f426acc7b2c349588d0540

commit fa466d805d47fe3447f426acc7b2c349588d0540
Author: wutao <wutao@chromium.org>
Date: Thu Apr 12 18:32:56 2018

cros: Close Shortcuts window by CTRL+W

Currently Shortcuts window can only be closed by mouse click. This cl
adds CTRL+W accelerator to close the window.

Bug:  819815 
Test: KeyboardShortcutViewTest.CloseWindowByAccelerator
Change-Id: Icfa340e5c947f823ea62e25df06536f216563ec7
Reviewed-on: https://chromium-review.googlesource.com/1006115
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#550281}
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/BUILD.gn
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view.h
[modify] https://crrev.com/fa466d805d47fe3447f426acc7b2c349588d0540/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc

Please also support "Ctrl+Alt+/" a second time to close, so that this way, it is consistent with the old popup.  It was very disconcerting that typing "Ctrl+Alt+/" did not close the window for me, as that was the paradigm I was used to.

Assign this to PM and UX.

ovanieva@, could you please triage. Technically it is easy to do. But do we want to have this behavior? Thanks!

Friendly ping on this?  I am constantly frustrated that pressing "Ctrl+Alt+/" a second time does not close the KSH, whereas it used to on the old UI.

Tao implemented Ctrl+W to close KSH. I'm marking it as lower priority as its essentially FR. 

Tao - what's complexity of adding ESC to close KSH?

Understood there's Ctrl+W, and thanks for bringing up ESC, but the old UI supported "Ctrl+Alt+/" a second time to close, why doesn't the new UI?  It's frustrating that it was removed

I do not think we want to close the UI by "esc" because it is used to exit search mode. It will make users confusing by press "esc" will close the window as well.

For "Ctrl+Alt+/" to close the window, there is no technical difficulty. Do we need UX to justify why or why not do the toggle using the same shortcut?

I think that Ctrl+Alt+/ should close the window. Other similar modal UI elements are also closed when the accelerator used to open them is pressed a second time:

- Launcher (Search/Launcher key)
- System menu (Alt+Shift+s)
- Notifications tray (Alt+Shift+n)
- Overview mode ("[ ]]]" key)
- Power menu (power button)

All of these items are also closable by pressing Escape*, so I'd argue that Escape should dismiss the KSH as well. Regarding search mode, why not:

a) if in search mode, exit search mode
b) if not in search mode, dismiss the KSH

?

* except it only exits Overview mode if I have at least one window open, which seems like a bug

derat@, yes we can use "esc" as you said. But it might be annoying if the user only wants to exit the search mode, but pressed more than once "esc" and closed the window.

In UI review, it was explicitly pointed out to make the behaviors same as the searching behaviors in the "Settings" for the keys of "esc", "back", "backspace". "esc" will not close the "Settings" window. 

Tao, Derat@ and I had a chat offline. 
let's not use ESC. You are right: its the feedback we got from UI review.
Let's instead introduce ctrl + alt +/ as opening and closing shortcut.

+msw@, not sure will adding toggle logic in #20 affect your work in  bug 841020 .

derat@ and ijpedowitz@

The example UIs in #18 are almost visible all the time. So that toggle works fine.

But for KSH, the window could be in-visible. Can we make the behavior a little more complicate as below.
  // Toggle the Keyboard Shortcut Viewer window. If there is a
  // KSH window open already, if it is the currently active
  // window, close it, otherwise, active the window.

I think this would be less confusing: say if the window is not visible, user wants to open the KSH window and press "ctrl+alt+/", but instead close it in the background.

This also reduce the code complexity for opening the KSH from the Settings -> keyboard -> show keyboard shortcut. It shares the same code path. It is a little weird to toggle the KSH by clicking the "show keyboard shortcut" button.

With the behavior proposed above, when user clicking the button, the KSH window is in the state of opened but not activated. The user can bring the KSH window to the front, just as the expectation.

Just to make sure I understand correctly, are you proposing the following behavior for Ctrl+Alt+/?

- If the KSH window is active, close it.
- Otherwise, open it (if it's not open already) or activate it (if it's open but not active).

If so, that makes sense to me. I agree it would be confusing if Ctrl+Alt+/ closed a window that was hiding in the background.

derat@, #23 is what I was proposing. Thanks!

That proposal seems okay, and it'd be awesome if your impl works for the current version and the mojo app (--keyboard-shortcut-viewer-app). Hopefully the KSV window/app can register an accelerator for Ctrl+Alt+/ that takes precedence over Chrome's accelerator to activate/close the window.

Otherwise, if only Chrome can handle the Ctrl+Alt+/ accelerator and we use that to show/activate/close, then we'll probably have to hook up a chrome->app mojo interface to active/close KSV when running. That's not ideal, but workable. I can help with supporting the app as needed.

The proposal in comment 22 and comment 23 makes sense to me as well.  Thank you for doing this!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49

commit 8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49
Author: wutao <wutao@chromium.org>
Date: Fri May 25 21:31:09 2018

cros: Toggle shortcut view by the same shortcut

This cl adds the support to toggle the shortcut view by the same
shortcut: Ctrl + Alt + /:
1. If the KSH window is active, close it.
2. Otherwise, open it (if it's not open already) or activate it (if it's
   open but not active).

Bug:  819815 
Test: New unit test KeyboardShortcutViewTest.ToggleWindow
Change-Id: Icbc9fba5644208750d1632b7473c66f53b923a80
Reviewed-on: https://chromium-review.googlesource.com/1073101
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562018}
[modify] https://crrev.com/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49/ash/components/shortcut_viewer/shortcut_viewer_application.cc
[modify] https://crrev.com/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49/ash/components/shortcut_viewer/views/keyboard_shortcut_view.h
[modify] https://crrev.com/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc
[modify] https://crrev.com/8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc

Request to merge cl on #27 to M68.

THANK YOU!!

Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e3f847e47711fe30d576cce26bf5e103d403939

commit 9e3f847e47711fe30d576cce26bf5e103d403939
Author: wutao <wutao@chromium.org>
Date: Tue May 29 18:11:53 2018

M68 merge: cros: Toggle shortcut view by the same shortcut

This cl adds the support to toggle the shortcut view by the same
shortcut: Ctrl + Alt + /:
1. If the KSH window is active, close it.
2. Otherwise, open it (if it's not open already) or activate it (if 
   it's open but not active).

TBR=afakhry@chromium.org,msw@chromium.org

Bug:  819815 
Test: New unit test KeyboardShortcutViewTest.ToggleWindow
Change-Id: Icbc9fba5644208750d1632b7473c66f53b923a80
Reviewed-on: https://chromium-review.googlesource.com/1073101
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#562018}(cherry picked from commit 8bcbe49dd1f7de2f65fa4ffbed9bbb126141fe49)
Reviewed-on: https://chromium-review.googlesource.com/1076987
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#28}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/9e3f847e47711fe30d576cce26bf5e103d403939/ash/components/shortcut_viewer/shortcut_viewer_application.cc
[modify] https://crrev.com/9e3f847e47711fe30d576cce26bf5e103d403939/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/9e3f847e47711fe30d576cce26bf5e103d403939/ash/components/shortcut_viewer/views/keyboard_shortcut_view.h
[modify] https://crrev.com/9e3f847e47711fe30d576cce26bf5e103d403939/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc
[modify] https://crrev.com/9e3f847e47711fe30d576cce26bf5e103d403939/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc

10860.0.0, 69.0.3480.0 dev-channel eve