feat: add registerAccelerator flag to allow menu items to optionally skip accelerator registration #15723
Conversation
brenca
force-pushed
the
brenca/fix-paste-and-match-style
branch
from
8b247f5
to
eac4ff7
Compare
brenca
changed the title
| MenuItem item = {i, model}; | ||
| (*table)[accelerator] = item; | ||
| if (model->ShouldRegisterAcceleratorAt(i)) { | ||
| if (model->GetAcceleratorAtWithParams(i, true, &accelerator)) { |
There was a problem hiding this comment.
nit: use && instead of nesting ifs unless there's code that should be run in when the outer condition is true but the inner is false.
|
@nornagon These things are handled by blink, fairly early on in the chain. See https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/editing/editor_key_bindings.cc?gsn=CreateCommand&g=0&l=47 |
|
I would love if someone could build this on Mac and check if things are still working OK, this comment worries me slightly https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/editing/editing_behavior.cc?gsn=key_down_commands_map&l=169 |
|
I tried building with this patched in locally (macOS 10.13.6) and ran the default app, opened the inspector and added the |
|
Release Notes Persisted
|
|
@brenca would you do the manual backport for 3-0-x? 🙇 |
|
I was unable to backport this PR to "3-0-x" cleanly; |
|
I have automatically backported this PR to "4-0-x", please check out #15840 |
|
A maintainer has manually backported this PR to "3-1-x", please check out #15892 |
Description of Change
Fixes #15719.
Instead of the former patch, I believe we should first match the behaviour of chromium on a higher level. They don't register accelerators for most of their things in the app menu, because most of those actions are handled much deeper down the hierarchy - for example, the
Cut/Copy/Paste/PasteAsactions are handled inblink.During the review of the upstreamed patch, a reviewer linked the specs of those actions, and it looks like the behaviour of
Paste and Match Styleon Windows should be the norm, not the exception, so even if we fix things upstream, it's just going to get worse for us (Pastewill behave similarly to the "buggy" behaviour ofPaste and Match Style).I solved this by adding an option to skip the registration of the accelerator for menu items, and added a default value to skip the registration of the four clipboard commands.
Checklist
npm testpassesRelease Notes
Notes: Added an option to
MenuItemthat makes it possible to skip accelerator registration.