This was added in Issue 602452.
As for why it's not overrideable, it should be, I will investigate.

Looks like it was actually bug 570761 - 602452 was the a11y review.

This passed UI review, but looks like we all missed that it was not overrideable.

Thanks for looking at it. If possible we should aim to merge to 58.

Logging what's happening in the code, the Alt+Shift+K accelerator is handled in 
BrowserView::PreHandleKeyboardEvent() [1] by the focus manager.

The comments in that function state that this is actually working as intended! Which is not what I expected:

  // What we have to do here is as follows:
  // - If the |browser_| is for an app, do nothing.
  // - On CrOS if |accelerator| is deprecated, we allow web contents to consume
  //   it if needed.
* // - If the |browser_| is not for an app, and the |accelerator| is not *
* //   associated with the browser (e.g. an Ash shortcut), process it.   *
  // - If the |browser_| is not for an app, and the |accelerator| is associated
  //   with the browser, and it is a reserved one (e.g. Ctrl+w), process it.
  // - If the |browser_| is not for an app, and the |accelerator| is associated
  //   with the browser, and it is not a reserved one, do nothing.

So Ash is basically given two opportunities to handle shortcuts:
1- At AcceleratorRouter::ProcessAccelerator(), which returns false (the accelerator should
   not be handled now --- as expected) [2].
2- And then later at BrowserView::PreHandleKeyboardEvent() [1].

If this is indeed WAI, what should we do now? Deprecate that newly added shortcut?



[1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_view.cc?type=cs&q=BrowserView::PreHandleKeyboardEvent&l=1377
[2]: https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_router.cc?type=cs&q=AcceleratorRouter::ProcessAccelerator&l=64

Interesting that it's only allowing apps to consume an accelerator if it's "deprecated". That's definitely not the behavior we want.

I'd be fine moving it to the deprecated list though just to get past this issue.

FWIW (2) won't happen in fullscreen mode.

Isn't the root problem that you can't have ash system shortcuts than can be overridden by web content? That is, that there's no accelerator handling pass in ash after web contents sees accelerators?

See issue 285308 and comments in ash accelerator_table.h
https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_table.h?sq=package:chromium&dr&l=16

I'm not sure if that's still the state of the world. sky made some changes recently to accelerator handling for mustash.

I vaguely remember there was some hack we did in the past, like add the shortcut as a chrome browser-level shortcut, then make that handler call into ash, that would get the web-contents-override behavior.

But yeah, stuff like this should be on search key. I suspect this was done for consistency with Alt-Shift-S for status bubble.

I think 'Alt-Shift-K' behaves consistently with 'Alt-Shift-S', and 'Alt-Shift-N'.

Re #8: If alt-shift-s and alt-shift-n are blocking sites from using those combos as well then those are also broken.

Key events that aren't handled by the renderer make there way to UnhandledKeyboardEventHandler, which calls to the FocusManager.

Re#10, so, do we still need the call to the FocusManager in PreHandleKeyboardEvent()? If the goal was to give Ash a chance to handle accelerators at the beginning, wasn't ash already given that chance through the AcceleratorRouter::ProcessAccelerator()?

Even if we deprecate the Alt+Shift+K now, usually we don't disable the old deprecated shortcut right away, not until a couple of milestones pass. So, Google Sheets still won't be able to use this shortcut right away.

So what should we do here?

So what's the plan for other Alt-Shift accelerators? Web apps could easily add those too:

Alt-Shift-S = open status area
Alt-Shift-N = open message center

etc. etc.

IIRC, our strategy is:

1) new ash accelerator should use search key. ahmed@ will add a unit test to catch if someone adds an accelerator that violates this.
2) we only deprecate and replace existing accelerator when it has an issue with application.
3) When we depreccate and add new accelerator first, with notification. 2 MS? later, we remove old accelerator.

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

commit af7963debe7aee03e201ea8ead465a7cbcff86cb
Author: afakhry <afakhry@chromium.org>
Date: Fri Mar 31 16:51:47 2017

Deprecate Alt+Shift+K and replace it by Search+Shift+K

This shortcut was newly added and should have been a Search-based
key from the beginning. It conflicts with A Google Sheets shortcut.

Screenshots:
1- https://screenshot.googleplex.com/ki3aepLOBRq.png
2- https://screenshot.googleplex.com/G6hj96dyfRo.png

BUG= 706018 
TEST=covered by tests in ash_unittests.
TEST=manually: Should see the deprecated notification when using the
     old shortcut. Should see an entry for the new one in the
     keyboard overlay.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2784393002
Cr-Commit-Position: refs/heads/master@{#461151}

[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/ash/ash_strings.grd
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/ash/common/accelerators/accelerator_table.cc
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/chrome/browser/resources/chromeos/keyboard_overlay_data.js
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc
[modify] https://crrev.com/af7963debe7aee03e201ea8ead465a7cbcff86cb/tools/metrics/histograms/histograms.xml

This bug requires manual review: There is .grd file changes and we are only 20 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(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/+/579880a51bcd7ec59e66d85d2d764a536cd06530

commit 579880a51bcd7ec59e66d85d2d764a536cd06530
Author: Ahmed Fakhry <afakhry@google.com>
Date: Thu Apr 06 23:49:40 2017

[Merge to M58] Deprecate Alt+Shift+K and replace it by Search+Shift+K

This shortcut was newly added and should have been a Search-based
key from the beginning. It conflicts with A Google Sheets shortcut.

Screenshots:
1- https://screenshot.googleplex.com/ki3aepLOBRq.png
2- https://screenshot.googleplex.com/G6hj96dyfRo.png

TBR=xiyuan@chromium.org, jamescook@chromium.org, mpearson@chromium.org
BUG= 706018 
TEST=covered by tests in ash_unittests.
TEST=manually: Should see the deprecated notification when using the
     old shortcut. Should see an entry for the new one in the
     keyboard overlay.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2784393002
Cr-Commit-Position: refs/heads/master@{#461151}
(cherry picked from commit af7963debe7aee03e201ea8ead465a7cbcff86cb)

Review-Url: https://codereview.chromium.org/2799973002 .
Cr-Commit-Position: refs/branch-heads/3029@{#618}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/ash/ash_strings.grd
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/ash/common/accelerators/accelerator_table.cc
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/chrome/browser/resources/chromeos/keyboard_overlay_data.js
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc
[modify] https://crrev.com/579880a51bcd7ec59e66d85d2d764a536cd06530/tools/metrics/histograms/histograms.xml

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

commit 77744550580e6f83fa1ada19e8f96722b911640f
Author: wutao <wutao@chromium.org>
Date: Fri Apr 07 04:55:37 2017

Add test to check search key based accelerators.

For new added accelerators, we want to ensure it is search key based by
adding the test.

BUG= 706018 , 706462 
TEST=AcceleratorTableTest.CheckSearchBasedAccelerators

Review-Url: https://codereview.chromium.org/2802583002
Cr-Commit-Position: refs/heads/master@{#462753}

[modify] https://crrev.com/77744550580e6f83fa1ada19e8f96722b911640f/ash/common/accelerators/accelerator_table_unittest.cc

Can we revert this change from comment #19 for 58?

It looks like this changes strings which were frozen back in February for R58, so we don't have translations in place for 58.

Is there anything else we can do other than revert?

I don't think we can get translations in time, so revert is our only option. 

It is not clear to me what happens if this gets left in there (I guess other languages see these strings in English?). 

Yes, the notification will be displayed in English if no translations are found.

Any update on getting this reverted? It looks like it needs to be done by someone with a checkout, the revert button on the code review site does not seem to like the file sizes involved.

I will do this shortly.

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

commit e42548579da70327f15bb97ad05a85bf28d76d96
Author: Ahmed Fakhry <afakhry@google.com>
Date: Thu Apr 13 00:44:13 2017

Revert "[Merge to M58] Deprecate Alt+Shift+K and replace it by Search+Shift+K"

Reason:
this changes strings which were frozen back in February for R58,
so we don't have translations in place for 58.

TBR=xiyuan@chromium.org, jamescook@chromium.org, mpearson@chromium.org
BUG= 706018 
TEST=Alt+Shift+K should continue to work.

This reverts commit 579880a51bcd7ec59e66d85d2d764a536cd06530.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2816903002 .
Cr-Commit-Position: refs/branch-heads/3029@{#685}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/ash/ash_strings.grd
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/ash/common/accelerators/accelerator_table.cc
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/chrome/browser/resources/chromeos/keyboard_overlay_data.js
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc
[modify] https://crrev.com/e42548579da70327f15bb97ad05a85bf28d76d96/tools/metrics/histograms/histograms.xml