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

Don't mutate the accelerator character with the shift key if it's a number #9743

Merged
merged 2 commits into from Jun 26, 2017

Conversation

Projects
None yet
5 participants
@MarshallOfSound
Member

MarshallOfSound commented Jun 13, 2017

Before this fix if you had an accelerator.

CommandOrControl+Shift+1

It would be represented in menus as "Command+!" which although is correct in regards to keycodes it isn't semantically correct from a users perspective, nor from the intention of the shortcut in most cases.

This fix will unmutate the character if it is a number

@matheuss

👍

@@ -26,7 +26,13 @@ void SetPlatformAccelerator(ui::Accelerator* accelerator) {
&characterIgnoringModifiers);
if (character != characterIgnoringModifiers) {
modifiers ^= NSShiftKeyMask;
// 48 === '0', 57 === '9'
if (characterIgnoringModifiers >= 48 && characterIgnoringModifiers <= 57) {

This comment has been minimized.

@poiru

poiru Jun 14, 2017

Member

Could change this to use isdigit()?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 16, 2017

Member

Haha, yeah, I guess it could 😆 #functionalize

@tonyganch

This comment has been minimized.

Member

tonyganch commented Jun 20, 2017

@MarshallOfSound, do i get it right that this PR will fix #6731?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 20, 2017

@tonyganch This particular PR won't fix that exact issue. No doubt it's related (Chromium methods trying to collapse keys to a visual representation with / without the shift key) but this PR only affects numbers.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 21, 2017

@kevinsawicki Is this good to go? 😄

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 26, 2017

Is this good to go?

@MarshallOfSound yeah, this seems good to me, looks like this is how other macOS apps represent these, here is a screenshot from Xcode:

screen shot 2017-06-26 at 11 20 05 am

👍 🚀 ⌨️

@kevinsawicki kevinsawicki merged commit bda2121 into master Jun 26, 2017

5 of 6 checks passed

electron-win-ia32 Build #3427 failed in 14 min
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #4452 succeeded in 11 min
Details
electron-osx-x64 Build #4457 succeeded in 9 min 43 sec
Details
electron-win-x64 Build #3404 succeeded in 9 min 28 sec
Details

@kevinsawicki kevinsawicki deleted the number-accelerator-modifier branch Jun 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment