Keyboard Shortcuts: fix settings sidebar toggle shortcut#43428
Merged
Conversation
9 tasks
|
Size Change: -4 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Sep 15, 2022
draganescu
left a comment
Contributor
There was a problem hiding this comment.
This does not seem to be a problem on OSX so I'd add an exclusion for it in the code.
Contributor
|
@draganescu Thanks for checking this but I do not see any new commits. Did you push your change here? |
talldan
reviewed
Sep 19, 2022
| // 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( '<', ',' ); |
Contributor
There was a problem hiding this comment.
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')
Contributor
There was a problem hiding this comment.
@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...
Contributor
Author
|
Thank you for the review, @talldan ! |
t-hamano
commented
Sep 19, 2022
| event.shiftKey && | ||
| character.length === 1 && | ||
| event.code === 'Comma' | ||
| ) { |
Contributor
Author
There was a problem hiding this comment.
I have changed the process to replace only certain keys so that it does not affect the keyboard layout.
I have nested the if statements in anticipation of replacing other keys in the future.
Additionally, as mentioned in this comment, I excluded AppleOS.
talldan
approved these changes
Sep 20, 2022
This was referenced Feb 9, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #43352
What?
This PR fixes settings sidebar toggle shortcut.
Why?
Ctrl + Shift + ,(on Windows) should open and close the settings sidebar, as shown in the window below:However, as described in the MDN documentation, KeyboardEvent.key returns a value that takes into consideration the state of modifier keys.
Therefore, in Windows,
,key is detected as the<key:0e4bc36ab35f69edb48f50c9cd154618.mp4
How?
The keyboard shortcut modal should display key labels that don't take modifier keys into consideration.
Therefore, I replaced
<with,in the detection condition byisKeyboardEvent.As far as I know,
<and,are not used as other keyboard events, so I think replacing them should have no effect.In addition, this fix does not make a judgment based on the operating system. If this was not a problem on the Mac, I would like to exclude Macby
isAppleOS().Testing Instructions
Ctrl + Shift + ,(Or shortcuts on the Mac)22aef74a88f48ef7bc78b4ab48dff550.mp4