[lexical] Bug Fix exact modifier matching for built-in keyboard shortcuts#7443
[lexical] Bug Fix exact modifier matching for built-in keyboard shortcuts#7443etrepum merged 19 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
0efb2ff to
1e8b582
Compare
etrepum
left a comment
There was a problem hiding this comment.
This is a great start, some of the refactoring looks like it broke a few of the IS_APPLE conditions
|
Thanks for the review! I’ve incorporated your suggestions into the latest changes. |
etrepum
left a comment
There was a problem hiding this comment.
Looks like there's a couple more mistakes in here, might be the reason why the tests are failing. I'll go ahead and apply these to see
|
It looks like tests are still failing, so there must still be some behavior change. Without further investigation I can't be sure if the problem is the test issuing keys that were matched incorrectly before which requires fixing of the tests, or if there's still some mistakes to catch in this PR. |
|
Thanks for pointing this out — I’ll take a closer look to figure out what’s causing the issue. Appreciate you highlighting the potential areas to investigate. |
…false for accurate shortcut detection
|
I really appreciate all the detailed feedback and support throughout this PR. I've applied the suggested changes and believe everything is now in order. Please let me know if any further adjustments are needed! |
|
Tentatively I think this is in a good state, but the |
etrepum
left a comment
There was a problem hiding this comment.
Thanks for your patience and making this nice improvement!
I made some minor changes to keep the implementation consistent and make this code more generally useful outside of the core. Mostly what I did was write comments for the commands related to event processing here so that there's API documentation for what they are supposed to mean, especially in cases where some or all modifiers are ignored.
|
Hmm just realized this should probably be considered a breaking change. Internally we have quite a bit of plugins relying on the non-exact matching for custom handling of the Enter key (on event (I think the naming of the |
|
I don’t think it was intentional to make the KEY_[X]_COMMAND behave differently. The code was a mess, some of them did check for modifiers but this PR may have added a few more constraints |
|
I’m aware that the change might affect existing behaviors more broadly, and I’m happy to help if any adjustments are needed. |
…cuts (#7443) Co-authored-by: Bob Ippolito <bob@redivi.com>
Description
Built-in keyboard shortcuts like
Cmd+Bcould previously be triggered even when extra modifiers such asShiftwere held (e.g.,Shift+Cmd+B). This was due to Lexical not checking all modifier keys when matching keyboard shortcuts.To address this, a new helper function
isExactShortcutMatchwas implemented and applied to the relevant key handling logic in the playground. This ensures that shortcuts only trigger when the modifier keys match exactly.Although the original issue (#7434) used
Cmd+KandShift+Cmd+Kas examples, those already behave correctly in the playground. To verify the fix,Cmd+BandShift+Cmd+Bwere used instead, sinceCmd+Btriggers bold formatting.If this change looks good, I’d appreciate any guidance on how best to add a corresponding test case for this behavior. Would it make sense to place it in one of the existing e2e test suites (e.g., Selection or Bold), or should a new test be added specifically for exact shortcut matching?
Closes #7434
Test plan
Before
Shift+Cmd+Bwould incorrectly trigger bold formatting (same asCmd+B)2025-04-08.3.56.49.mov
After
Cmd+Bcorrectly toggles boldShift+Cmd+Bdoes nothing, as expected2025-04-08.4.00.37.mov