-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyboard shortcut mappings with localization. #10570
base: develop
Are you sure you want to change the base?
Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyboard shortcut mappings with localization. #10570
Conversation
…specific keyboard shortcut mappings with localization
…specific keyboard shortcut mappings with localization
…specific keyboard shortcut mappings with localization
'⌘': mac ? '⌘ ' : t('shortcuts.key.ctrl'), | ||
'⇧': mac ? '⇧ ' : t('shortcuts.key.shift'), | ||
'⌥': mac ? '⌥ ' : t('shortcuts.key.alt'), | ||
'⌫': mac ? '⌫ ' : t('shortcuts.key.backspace'), | ||
'⌦': mac ? '⌦ ' : t('shortcuts.key.del'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially the same as what c22fa60 did in uiCmd.display
down below to fix the Keyboard Shortcuts screen. I suspect c22fa60 didn’t touch this function because it’s also being used in places where raw key identifiers are needed rather than human-readable labels. For example, the following code uses the return value to register the actual keyboard shortcut:
Lines 72 to 73 in 3025d4f
var keys = (detected.os === 'mac' ? [uiCmd('⌃⌘F'), 'f11'] : ['f11']); | |
context.keybinding().on(keys, fullScreen); |
Lines 102 to 110 in 3025d4f
utilKeybinding.plusKeys.forEach(function(key) { | |
context.keybinding().on([key], zoomIn); | |
context.keybinding().on([uiCmd('⌥' + key)], zoomInFurther); | |
}); | |
utilKeybinding.minusKeys.forEach(function(key) { | |
context.keybinding().on([key], zoomOut); | |
context.keybinding().on([uiCmd('⌥' + key)], zoomOutFurther); | |
}); |
If you run this branch on macOS, or on Windows with the German localization, will the shortcuts function correctly when you press them? If not, then maybe we need to revert this function to what it was previously. If we look back at the code I pointed to earlier, notice how it’s only being used to create the tooltip – so it’s probably only for display purposes:
iD/modules/ui/sections/data_layers.js
Line 404 in b5d45e3
.keys([uiCmd('⌘⇧' + t('info_panels.history.key'))]) |
If that’s the case, then changing that code to call uiCmd.display
instead might fix the issue for that particular tooltip. So instead of implementing anything new, you’d be reviewing each of the calls to uiCmd
, deciding whether it should be replaced with a call to uiCmd.display
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank very much for the detailed explanation, it's very helpful and I now understood with more clarity, gotta read more about this codebase, and I've marked the calls where uiCmd been called and now checking how useful, the uiCmd.display would be for a particular tooltip...will submit a pull request again after high detailed review of each call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working, I found these 2 issues/bug
- Issue with Key Functionality.
It seems that the keyboard shortcut for ? isn’t functioning as expected. Based on testing, it appears the shortcut should be labeled as Shift + ? instead of just ?, as pressing ? alone triggers the black overlay below the inspect button in the top-left corner.
Image 1 :
In Image 2, The Keyboard Shortcut should also be labeled as Shift + ?.
Image 2 :
- Interaction Limitation with Quality Assurance Tab
When the Quality Assurance tab is open in the Help section, the Keyboard Shortcuts option becomes unclickable. Interestingly, this behavior is not observed with other tabs, which allow seamless access to the keyboard shortcuts.
Image 1 : For example, the Keyboard Shortcuts options is clickable in case of points tab.
Image 2 : When the Quality Assurance tab is open in the Help section, the Keyboard Shortcuts option becomes unclickable.
Checked with latest changes on both local host and online deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help/Keyboard Shortcuts.
I’ve resolved the labeling issue mentioned in the previous comment (Issue 1, Image 2) by replacing ?
with Shift + ?
in the relevant file for "Show Keyboard Shortcuts". Should I create a separate issue and raise a PR for this fix, or is it okay to include it as part of this current work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the keyboard shortcut for ? isn’t functioning as expected. Based on testing, it appears the shortcut should be labeled as Shift + ? instead of just ?, as pressing ? alone triggers the black overlay below the inspect button in the top-left corner.
I think spelling it as just “?” is a standard convention, at least on macOS. If you’re using an English keyboard layout, pressing that key alone would be /, not ?. A user-selectable keyboard layout could conceivably have ? as the primary function of a key, or it could require a modifier key other than Shift. All that matters to a Web application like iD is whether the user has pressed the some key combination that would input “?” into a text field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the Quality Assurance tab is open in the Help section, the Keyboard Shortcuts option becomes unclickable.
I can reproduce this issue – you can only click on a tiny sliver at the bottom of the button. This is probably unrelated to displaying localized keyboard shortcuts in the tooltip, so please open a separate issue about it. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing! I’ll go ahead and create a separate issue for this fix and then focus on the uiCmd
functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the keyboard shortcut for ? isn’t functioning as expected. Based on testing, it appears the shortcut should be labeled as Shift + ? instead of just ?, as pressing ? alone triggers the black overlay below the inspect button in the top-left corner.
I think spelling it as just “?” is a standard convention, at least on macOS. If you’re using an English keyboard layout, pressing that key alone would be /, not ?. A user-selectable keyboard layout could conceivably have ? as the primary function of a key, or it could require a modifier key other than Shift. All that matters to a Web application like iD is whether the user has pressed the some key combination that would input “?” into a text field.
You're absolutely right—using "?" as the shortcut makes sense, especially since it's about the input rather than the exact key combination. It’s a good reminder that different keyboard layouts might handle this differently, Thanks for pointing that out. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reference undo_redo.js
if we check console.log(uiCmd('⌘Z')); // Outputs either '⌘Z' on Mac or 'Ctrl+Z' on Windows/Linux
and now we replace that with console.log(uiCmd.display('⌘Z')); // Outputs either '⌘Z' on Mac or '⌘Z' on Windows/Linux
Also uiCmd outputs 'CTRL+Z' on German localization or any other regional language, like in case of german it should be STRG+Z when using uiCmd.display but this function shows Outputs either '⌘Z' on Mac or '⌘Z' on Windows/Linux.
It looks like there's some inconsistency with uiCmd.display when handling localization for keyboard shortcuts. While uiCmd correctly adapts to regional differences (e.g., showing "STRG+Z" in German locales) on applying t('shortcuts.key.ctrl') there, uiCmd.display seems to bypass this behavior and hardcodes "⌘Z" for both Mac and Windows/Linux.
I tried to fix this bug using the localization function t(), and it worked with uiCmd but not with uiCmd.display. I'm a bit unsure whether using uiCmd.display for this undo action is a bad idea, or if it's an exception due to hardcoding, or if there's something else I'm missing.
Description of Code Change
The proposed change enhances keyboard shortcut mappings to dynamically adapt based on the operating system.
Original Code
The replacements object statically maps symbols (e.g., ⌘, ⇧) to generic labels (e.g., Ctrl, Shift), with no differentiation for macOS.
This shows CTRL + Shift + C on German language.
Proposed Change :
OS Detection
Introduces a mac variable to determine if the operating system is macOS (detected.os === 'mac').
Dynamic Mapping
For macOS:
Uses macOS-specific symbols and labels (e.g., ⌘ Cmd, ⇧ Shift).
Prepends symbols to enhance clarity.
For Others:
Defaults to generic labels like Ctrl, Alt, and Shift.
Localization:
Integrates t() to fetch OS-specific, human-readable labels, supporting multiple languages.
This newly proposed shows STRG + Umschalt + C on German language.