Skip to content

[lexical] Bug Fix exact modifier matching for built-in keyboard shortcuts - #7443

Merged
etrepum merged 19 commits into
facebook:mainfrom
suwonthugger:fix/7434-exact-shortcut-match
Apr 15, 2025
Merged

etrepum merged 19 commits into
facebook:mainfrom
suwonthugger:fix/7434-exact-shortcut-match

Conversation

@suwonthugger

@suwonthugger suwonthugger commented Apr 7, 2025 •

Copy link
Copy Markdown
Contributor

Description

Built-in keyboard shortcuts like Cmd+B could previously be triggered even when extra modifiers such as Shift were 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 isExactShortcutMatch was 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+K and Shift+Cmd+K as examples, those already behave correctly in the playground. To verify the fix, Cmd+B and Shift+Cmd+B were used instead, since Cmd+B triggers 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+B would incorrectly trigger bold formatting (same as Cmd+B)
  • Modifier keys were not strictly checked
2025-04-08.3.56.49.mov

After

  • Cmd+B correctly toggles bold
  • Shift+Cmd+B does nothing, as expected
  • Verified manually in the playground editor
2025-04-08.4.00.37.mov

@vercel

vercel Bot commented Apr 7, 2025 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 8:00pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2025 8:00pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2025
@suwonthugger
suwonthugger force-pushed the fix/7434-exact-shortcut-match branch from 0efb2ff to 1e8b582 Compare April 7, 2025 19:05
@suwonthugger suwonthugger changed the title [lexical-playground] Fix exact modifier matching for built-in keyboard shortcuts [lexical-playground] Bug Fix exact modifier matching for built-in keyboard shortcuts Apr 7, 2025
@suwonthugger suwonthugger changed the title [lexical-playground] Bug Fix exact modifier matching for built-in keyboard shortcuts [lexical] Bug Fix exact modifier matching for built-in keyboard shortcuts Apr 7, 2025
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Apr 7, 2025

@etrepum etrepum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a great start, some of the refactoring looks like it broke a few of the IS_APPLE conditions

Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
@suwonthugger

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I’ve incorporated your suggestions into the latest changes.

@etrepum etrepum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
Comment thread packages/lexical/src/LexicalUtils.ts Outdated
@etrepum

etrepum commented Apr 8, 2025

Copy link
Copy Markdown
Collaborator

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.

@suwonthugger

Copy link
Copy Markdown
Contributor Author

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.

@suwonthugger

Copy link
Copy Markdown
Contributor Author

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!

@etrepum

etrepum commented Apr 12, 2025

Copy link
Copy Markdown
Collaborator

Tentatively I think this is in a good state, but the 'any' workarounds should get a more careful audit that I haven't had time for (to verify all use sites of those commands). It will be another few days before merge, but no feedback to address at the moment.

@etrepum etrepum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@takuyakanbr

takuyakanbr commented Apr 17, 2025 •

Copy link
Copy Markdown
Contributor

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 KEY_ENTER_COMMAND).

(I think the naming of the KEY_[X]_COMMAND commands also does not help here. Wondering if we should keep these as inexact and create new commands that are more descriptive of the action being performed)

@etrepum

etrepum commented Apr 17, 2025

Copy link
Copy Markdown
Collaborator

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

@suwonthugger

Copy link
Copy Markdown
Contributor Author

I’m aware that the change might affect existing behaviors more broadly, and I’m happy to help if any adjustments are needed.

This branch was successfully deployed

2 active deployments
Preview – lexical-playground — c721995b Deployed Apr 15, 2025 by vercel[bot]
Preview – lexical — c721995b Deployed Apr 15, 2025 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Keybindings match too broadly

5 participants