Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard Shortcuts: fix settings sidebar toggle shortcut #43428

Merged
merged 3 commits into from
Sep 20, 2022
Merged

Keyboard Shortcuts: fix settings sidebar toggle shortcut #43428

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
715f036
Keyboard Shortcuts: fix settings sidebar toggle shortcut
t-hamano Aug 19, 2022
8a5f0dc
Merge branch 'trunk' into fix/shortcut-settings-sidebar
t-hamano Sep 19, 2022
c4a76d5
Consider Keyboard layouts
t-hamano Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/keycodes/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,12 @@ export const isKeyboardEvent = mapValues( modifiers, ( getModifiers ) => {
key = String.fromCharCode( event.keyCode ).toLowerCase();
}

// Replace some characters to match the key indicated
// by the shortcut when a modifier key is pressed.
if ( event.shiftKey && character.length === 1 ) {
key = key.replace( '<', ',' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Is it a recent regression? It's surprising to me as this code seems to have been mostly unchanged for a while, and I would've expected quite a few bug reports.

I don't think this is the right solution as it makes an assumption that the user's keyboard has a layout where < and , share a key.

An example of where this wouldn't work is that many European keyboard layouts (Danish, Dutch, Italian) have ; and , sharing a key (https://en.wikipedia.org/wiki/List_of_QWERTY_keyboard_language_variants).

I think it'd be better to try to understand from the event payload which key was pressed (e.g. it looks like event.code === 'Comma')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@talldan It is a bug for sure. For example, pressing ctrl+ will move through landmarks but ctrl+shift+ no longer works. It has been this way for some time so I was hoping this PR could at least show us what was going wrong for more fixes.

I cannot remember the last time any of these worked well on Windows...

}

// For backwards compatibility.
if ( character === 'del' ) {
character = 'delete';
Expand Down