Skip to content
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

[lexical-react] Bug Fix: Make typeahead menu respect read-only mode #7185

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

kirandash
Copy link
Contributor

Description

Currently, the typeahead menu (emoji/mentions picker) does not respect the editor's read-only mode. When the editor is in read-only mode:

  1. The typeahead menu still opens when typing triggers (: or @)
  2. Users can still select and insert emojis/mentions
  3. The menu stays open even if editor switches to read-only mode while menu is open

This PR fixes the issue by:

  1. Adding a read-only mode check before showing the typeahead menu
  2. Adding an editable state listener to close the menu when editor becomes read-only
  3. Ensuring complete compliance with read-only mode behavior

Closes #7160

Test plan

Before

lexical-typeahead-menu-bug.mov

After

lexical-typeahead-fix.mov

- Add read-only mode check before showing typeahead menu
- Add editable state listener to close menu when editor becomes read-only
- Ensures emoji/mentions picker respects editor's read-only state

Fixes facebook#7160
Copy link

vercel bot commented Feb 16, 2025

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 Feb 16, 2025 5:12pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 16, 2025 5:12pm

@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 Feb 16, 2025
etrepum
etrepum previously approved these changes Feb 16, 2025
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

Looks good! Confirmed the fix

Comment on lines 314 to 319
const removeEditableListener =
editor.registerEditableListener(editableListener);

return () => {
removeEditableListener();
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a nit for future reference, but this could be simpler:

Suggested change
const removeEditableListener =
editor.registerEditableListener(editableListener);
return () => {
removeEditableListener();
};
return editor.registerEditableListener(editableListener);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etrepum Thanks for the nit suggestion about simplifying the editable listener cleanup! I'll keep this in mind for future PRs. Sorry about the force push - I should have left the original approved version. Would you mind re-approving when you get a chance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well if you're here, might as well make that simplification and I'll re-approve and add it to the merge queue once the CI test run finishes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etrepum I've pushed the simplified editable listener change using the implicit return approach. The change removes the intermediate variables and explicit return statement, making the code more concise while maintaining the same functionality. Looking forward to your re-review. Thanks a lot for the feedback! 👍

Directly return the cleanup function from registerEditableListener in useEffect
instead of storing intermediate variables.

Suggested by: etrepum <[email protected]>
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

LGTM!

@etrepum etrepum added this pull request to the merge queue Feb 16, 2025
Merged via the queue into facebook:main with commit e5f3c0c Feb 16, 2025
42 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Typeahead menu does not respect read-only mode
3 participants