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-table] Bug Fix: Add fallback selection to InsertTableCommand #7316

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patrick-atticus
Copy link
Contributor

Description

If you try to insert a table asynchronously when the editor is not focused, the table is not inserted (because there is no selection)

This can happen if you focus an input field (like row count in the playground), and then dispatch INSERT_TABLE_COMMAND async.

Screenshot 2025-03-12 at 3 01 13 pm

Tables are not inserted async in the playground so it's not a problem, but it can easily be reproduced by wrapping the insert command in a setTimeout in packages/lexical-playground/src/plugins/TablePlugin.tsx

This PR adds a fallback to $getPreviousSelection() in the table insert command handler. Same as #4570

Note that that actual insertion uses $insertNodeToNearestRoot, which already uses the same fallback. This change simply prevents the command from exiting early.

Copy link

vercel bot commented Mar 12, 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 Mar 12, 2025 9:22am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2025 9:22am

@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 Mar 12, 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.

Adding a test would be a good idea, since as you said the playground doesn’t do this there’s no confirmation that this fixes anything

@patrick-atticus
Copy link
Contributor Author

Unfortunately I could not reproduce this in the headless unit test environment.

I can't do an e2e test without changing playground behaviour. I updated the playground so now the insert is async and the button renders as disabled, which is nice when you insert a huge table and it takes a moment.

Now the table tests (eg Can a table be inserted from the toolbar) will fail without the updated $insertTableCommandListener.

I'll understand if you don't want these playground changes.

@patrick-atticus patrick-atticus requested a review from etrepum March 12, 2025 09:28
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Mar 12, 2025
Comment on lines +104 to +113
setIsDisabled(true);
// Large tables can take a moment. Defer insert to render button disabled
setTimeout(() => {
activeEditor.dispatchCommand(INSERT_TABLE_COMMAND, {
columns,
rows,
});

onClose();
onClose();
}, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's roll this change back, I don't see a good reason to demonstrate this anti-pattern in playground code that people will be copying. Table creation and rendering isn't async there's no real utility to disabling anything since the UI isn't going to be responsive while any of that is happening.

I don't see why you couldn't do this in a unit test with $setSelection(null); editor.dispatchCommand(INSERT_TABLE_COMMAND, …); given a previous update that had a selection.

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.

3 participants