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.

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

Normalize key to uppercase on MacOS when Shift is held #116

Merged
merged 10 commits into from
Dec 12, 2023

Conversation

iansan5653
Copy link
Member

@iansan5653 iansan5653 commented Dec 7, 2023

Uses a map of 'lowercase' (non-shifted) symbols to their corresponded uppercase/shifted counterparts to normalize key on MacOS to always be uppercase when Shift is held.

This only attempts to address the issue for US/QWERTY layouts as was our approach with Alt support, however the approach should work for all US layouts because it still uses event.key and not event.code. We could use the more general toUpperCase for character keys to try and support more languages, but I think it's better to limit this normalization to what we know for sure, and let consumers handle more advanced cases if they need to. This works for all GitHub applications because we only use latin characters for shortcuts.

@iansan5653 iansan5653 self-assigned this Dec 7, 2023
@iansan5653 iansan5653 requested a review from a team as a code owner December 7, 2023 19:09
@iansan5653 iansan5653 requested a review from jfuchs December 7, 2023 19:09
@iansan5653 iansan5653 marked this pull request as draft December 7, 2023 19:09
@iansan5653 iansan5653 removed request for a team and jfuchs December 7, 2023 19:09
Copy link
Contributor

@theinterned theinterned left a comment

Choose a reason for hiding this comment

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

it is unfortunate that this makes hokey only functional for US QWERTY keyboards for the time being, but I hope we can follow up with the ability to provide mappings for other layouts.

@@ -108,7 +108,6 @@ for (const el of document.querySelectorAll('[data-shortcut]')) {
6. `"Mod"` is a special modifier that localizes to `Meta` on MacOS/iOS, and `Control` on Windows/Linux.
1. `"Mod+"` can appear in any order in a hotkey string. For example: `"Mod+Alt+Shift+KEY"`
2. Neither the `Control` or `Meta` modifiers should appear in a hotkey string with `Mod`.
3. Due to the inconsistent lowercasing of `event.key` on Mac and iOS when `Meta` is pressed along with `Shift`, it is recommended to avoid hotkey strings containing both `Mod` and `Shift`.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should still mention that this might cause inaccessible shortcuts for non-US-QWERTY users?

src/hotkey.ts Outdated Show resolved Hide resolved

// MacOS outputs lowercase characters when `Command+Shift` is held, so we map them back to uppercase if we can
const shiftNormalizedKey =
hotkeyString.includes('Shift') && matchApplePlatform.test(platform)
Copy link
Contributor

@theinterned theinterned Dec 7, 2023

Choose a reason for hiding this comment

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

a thought: maybe we should only normalize for Mod hotkeys: that way using the raw Meta like Meta+Shift+lowercase would still be an option?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although Meta+Shift+lowercase wouldn't work on Linux :(

Copy link
Member Author

@iansan5653 iansan5653 Dec 7, 2023

Choose a reason for hiding this comment

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

That's not a bad idea - I like the opt-in normalization you get there. Meta alone won't actually work on Linux at all I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, but it doesn't work in this case because we are just converting events to hotkey strings - we don't yet know what the target string is that we will be comparing it to since this is a lower-level util.

Base automatically changed from include-shift to main December 7, 2023 22:06
@iansan5653 iansan5653 force-pushed the normalize-uppercase-macos branch from 3d792c5 to f2c43dc Compare December 7, 2023 22:13
@iansan5653 iansan5653 marked this pull request as ready for review December 8, 2023 14:18
@iansan5653 iansan5653 merged commit bd832b7 into main Dec 12, 2023
2 checks passed
@iansan5653 iansan5653 deleted the normalize-uppercase-macos branch December 12, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants