8000 fix: keep shifted character in menu accelerator by zcbenz · Pull Request #29202 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: keep shifted character in menu accelerator #29202

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

Jump to bottom
Merged
merged 4 commits into from
Jun 2, 2021
Merged

fix: keep shifted character in menu accelerator #29202

merged 4 commits into from
Jun 2, 2021

Conversation

zcbenz
Copy link
Contributor
@zcbenz zcbenz commented May 18, 2021 •
edited
Loading

Description of Change

When a shifted character is used in accelerator, for example Ctrl+Plus and Ctrl+?, it should be kept as it is. And when Shift key and unshifted character are used, for example Ctrl+Shift+= and Ctrl+Shift+/, it should not be converted to shifted character.

Close #21790.

const template = [
  {
    label: 'Test',
    submenu: [
      { label: 'Alt+Shift+3',
        accelerator: 'Alt+Shift+3' }
      },
      { label: 'Alt+#',
        accelerator: 'Alt+#' }
      },
      { label: 'Ctrl+Plus',
        accelerator: 'Ctrl+Plus' }
      },
      { label: 'Ctrl+Shift+=',
        accelerator: 'Ctrl+Shift+=' }
      },
      { label: 'Ctrl+?',
        accelerator: 'Ctrl+?' }
      },
      { label: 'Ctrl+Shift+/',
        accelerator: 'Ctrl+Shift+/' }
      }
    ]
  },
]

Screen Shot 2021-05-18 at 9 05 37

Screen Shot 2021-05-18 at 9 25 10

Screen Shot 2021-05-18 at 10 47 36

Also as a side effect of this change, the accelerator.patch removes the hack hat uses DomCodeToUsLayoutCharacter to get key character on Windows, which closes #26888.

Screen Shot 2021-05-18 at 11 22 01

(Germany keyboard layout)

Checklist

Release Notes

Notes: Fix shifted character getting changed in menu accelerator.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/11-x-y labels May 18, 2021
@zcbenz zcbenz requested a review from a team as a code owner May 18, 2021 02:24
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 18, 2021
@zcbenz
Copy link
Contributor Author
zcbenz commented May 19, 2021

/cc people who had looked into the accelerator issues @clavin @alexdima @nornagon

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label May 19, 2021
@zcbenz zcbenz force-pushed the fix-shifted-char-accelerator branch from 4d4a3b9 to e5a12e4 Compare June 1, 2021 10:15
clavin
clavin approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member
@clavin clavin left a comment

Choose a reason for hiding this comment

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

Pretty much looks good to me, just wrote down some questions that popped out at me while reading through the code. 👍

shell/common/keyboard_util.cc
@@ -36,7 +37,7 @@ ui::KeyboardCode KeyboardCodeFromKeyIdentifier(const std::string& s,
} else if (str == "altgr") {
return ui::VKEY_ALTGR;
} else if (str == "plus") {
*shifted = true;
shifted_char->emplace('+');
Copy link
Member

Choose a reason for hiding this comment

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

Side note: This isn't strictly true, + isn't a shifted character for all keyboard layouts. This PR doesn't really aim to fix that functionality, I'm just noting it here so we don't close some linked issues that do talk about that as well.

Copy link
Contributor Author
@zcbenz zcbenz Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The internal representation of accelerators is using US keyboard layout, and then the accelerator will be converted to actual keys according to the actual keyboard layout.

(below comment explains more.)

shell/common/keyboard_util.cc
else
return KeyboardCodeFromKeyIdentifier(str, shifted);
ui::KeyboardCode KeyboardCodeFromStr(const std::string& str,
base::Optional<char16_t>* shifted_char) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm noticing a lot of code for shifted_char, but what about characters that require alt (aka option on macOS) to input? For example, in the french keyboard layout, typing @ requires pressing the option key (at least on macOS). Would this "alternate kind of shifted character" still work with these changes since they assume a US keyboard layout?

Copy link
Contributor Author
@zcbenz zcbenz Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In Electron apps, the accelerators are written in US keyboard layout, so we only consider the shifted state for the keys.

For macOS, if the accelerator is @, the character will be used as it is, and macOS will automatically use the correct keys of the actual keyboard layout.

But for Windows/Linux, the @ accelerator is internally translated as Shift + 2, and it will show as different keys depending on the actual keyboard layout.

So when using @ as accelerator, the actual key might be different on macOS and Windows/Linux depending on keyboard layout. This is because macOS accepts arbitrary characters as key shortcuts, while Windows/Linux only accept combinations of physical keys.

@zcbenz zcbenz force-pushed the fix-shifted-char-accelerator branch from e5a12e4 to 7fb9204 Compare June 2, 2021 01:40
@zcbenz zcbenz merged commit 3cfe5c6 into master Jun 2, 2021
@zcbenz zcbenz deleted the fix-shifted-char-accelerator branch June 2, 2021 07:32
@release-clerk
Copy link
release-clerk bot commented Jun 2, 2021

Release Notes Persisted

Fix shifted character getting changed in menu accelerator.

@trop
Copy link
Contributor
trop bot commented Jun 2, 2021

I was unable to backport this PR to "13-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot mentioned this pull request Jun 2, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Jun 2, 2021

I was unable to backport this PR to "12-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor
trop bot commented Jun 2, 2021

I have automatically backported this PR to "14-x-y", please check out #29481

@zcbenz zcbenz mentioned this pull request Jun 2, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Jun 2, 2021

@zcbenz has manually backported this PR to "13-x-y", please check out #29482

@zcbenz zcbenz mentioned this pull request Jun 2, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Jun 2, 2021

@zcbenz has manually backported this PR to "12-x-y", please check out #29483

bpasero referenced this pull request in microsoft/vscode Jun 15, 2021
@deepak1556
chore: bump electron@12.0.11
746fb6a
@alexdima alexdima mentioned this pull request Jun 15, 2021
Closed
@nornagon nornagon mentioned this pull request Jul 9, 2021
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebytere codebytere codebytere approved these changes

@clavin clavin clavin approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accelerators in menus no longer reflect keyboard layout on Windows Menu Accelerators don't follow macOS HIG
3 participants
@zcbenz @codebytere @clavin
0