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][lexical-table] Bug fix: TablePlugin: - check is current selection in target table node #6979

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Ulop
Copy link
Contributor

@Ulop Ulop commented Dec 19, 2024

Description

For $deleteCellHandler method, like for deleteTextHandler and etc., a check has been added to ensure the current selection belongs in the target table node (tableNode param passed to applyTableHandlers)

Closes #6972

Test plan

  1. Open playground
  2. Remove all content
  3. Append first table
  4. Append some text
  5. Append second table
  6. Select all cell's of second table
  7. Press Del

Before

First table deleted

table_remove.mp4

After

Second(selected table) deleted

table_remove_fixed.mp4

Copy link

vercel bot commented Dec 19, 2024

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 Dec 19, 2024 8:55am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 8:55am

@facebook-github-bot
Copy link
Contributor

Hi @Ulop!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Copy link

size-limit report 📦

Path Size
lexical - cjs 31.29 KB (0%)
lexical - esm 31.11 KB (0%)
@lexical/rich-text - cjs 40.34 KB (0%)
@lexical/rich-text - esm 33.04 KB (0%)
@lexical/plain-text - cjs 38.88 KB (0%)
@lexical/plain-text - esm 30.3 KB (0%)
@lexical/react - cjs 42.12 KB (0%)
@lexical/react - esm 34.29 KB (0%)

Comment on lines -473 to -474
const rootNode = $getRoot();
rootNode.selectStart();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, there is no need for these lines, because when deleting a table via Del, the selection is always transferred to the beginning of the editor, no to the previous node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this indeed fixes post-deletion selection when nested in a different widget like a collapsible/column box.

Comment on lines +459 to +462
if (!$isSelectionInTable(selection, tableNode)) {
return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary to place the check after the lines above, otherwise the extension of the selection to the entire table is skipped and the table is deleted incorrectly when anchor or focus points outside the table.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 Dec 19, 2024
@ivailop7 ivailop7 added the extended-tests Run extended e2e tests on a PR label Dec 20, 2024
@etrepum etrepum added this pull request to the merge queue Dec 20, 2024
Merged via the queue into facebook:main with commit 23715f5 Dec 20, 2024
51 of 53 checks passed
@Ulop Ulop deleted the fix_6972 branch December 20, 2024 15:36
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
4 participants