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

Fix splitText when detached #6501

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Fix splitText when detached #6501

merged 1 commit into from
Aug 8, 2024

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented Aug 7, 2024

When splitText is run on a Node with no parent it crashes. I don't see any obvious reason why this function should only work on attached nodes and there are some valid cases when you'd want to attach the resulting nodes later and leverage splitText to preserve the styling and format.

Copy link

vercel bot commented Aug 7, 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 Aug 7, 2024 9:23pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2024 9:23pm

@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 Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

size-limit report 📦

Path Size
lexical - cjs 29.27 KB (0%)
lexical - esm 29.09 KB (0%)
@lexical/rich-text - cjs 37.68 KB (0%)
@lexical/rich-text - esm 30.92 KB (0%)
@lexical/plain-text - cjs 36.27 KB (0%)
@lexical/plain-text - esm 28.32 KB (0%)
@lexical/react - cjs 39.52 KB (0%)
@lexical/react - esm 32.39 KB (0%)

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 like a net improvement, but I think there may be other edge cases left in this method

Comment on lines +1012 to +1015
if (hasReplacedSelf) {
writableParent.splice(insertionIndex, 0, splitNodes);
this.remove();
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this isn't the code that's changing in this PR but it seems like there might be a hasReplacedSelf selection bug, the above code to fixup selections only handles parts>0 and the splice selection fixup code won't do anything either because the node removal is happening separately. It also seems a bit odd that the special case only happens with IS_SEGMENTED and not IS_TOKEN

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.

3 participants