Fix insertNodes when inserting into inline elements

[birtles]

This restores the behavior that changed in #5002 particularly when
pasting into links and other inline elements.

Fixes #5251.

[vercel]

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 27, 2023 9:29am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 27, 2023 9:29am

[birtles]

This fixes a lot of the weirdness around pasting into links for me and at least gets all our E2E tests for pasting which were broken by 0.12.3 to pass again.

[birtles]

Looking at the CI failures, I think all of them existed prior to this PR except the newly added test appears to fail on Windows due to the selection range differing there. I guess I need to find a more cross-platform way of setting the correct selection range there. Something to look into next week, I guess.

Edit: Hopefully fixed the windows-specific failure.

[ivailop7]

@zurfyx and @GermanJablo I think you guys are aware of this one, you might be more appropriate at having a look at this one.

[GermanJablo]

Thank you very much for reporting the problem and trying to solve it!

Currently, your PR is solving one problem but causing others. I suggest adding the following tests:

With the anchor inside a link, paste:

1. normal text: foo bar baz
2. normal text with bold: foo <b>bar</b> baz
3. normal text with link: foo <a>bar</a> baz
4. multiple blocks: <p>foo</p><p>bar</p>

I think it is clear what has to happen in the third case. The first link has to be separated and the content of the clipboard inserted in the middle, since there cannot be a link within another link.

For case 1 or 2 one could think of inserting the nodes inside the link without splitting it. I looked at gDocs and Word and neither do this, they both split the link. After thinking about it I completely agree with this behaviour. Doing otherwise would bring several complexities.

To note: insertNodes has 3 well-defined cases. The point that cases 2 and 3 have in common (where the bug is) is removeTextAndSplitBlock. In case 3 it is called inside insertParagraph. So I THINK the most elegant and/or easiest solution is to modify removeTextAndSplitBlock.

[birtles]

Excellent, thank you!

Yes, I didn't expect this PR would be sufficient but wanted to get the conversation going. I need to fix all the regressions from 0.12.3 before we can update our app and this is the biggest one. We have a ruby editor where the user can paste text into the ruby (inline) element. This worked before 0.12.3 but not anymore.

Having links reject pasting into them might make sense but the default behavior should be to allow pasting into inline elements.

I'll make up tests for those cases you mentioned and see if I can get something sensible to happen for each.

[birtles]

I dug into this a bit more and here's what I found out:

removeTextAndSplitBlock is supposed to return an index into the block ancestor that can be used to insert the nodes but there are a number of cases where it returns the wrong result.

For example here:

lexical/packages/lexical/src/LexicalSelection.ts

Lines 2852 to 2854 in 794e6dd

	if (!$isTextNode(pointNode)) {
	return point.offset;
	}

We determine that the selection anchor lands on a non-text node and assume that the selection offset corresponds to an offset in the ancestor block, but it may be the offset into an inline element's child nodes.

Similarly here:

lexical/packages/lexical/src/LexicalSelection.ts

Lines 2868 to 2873 in 794e6dd

	const x = point.offset === 0 ? 0 : 1;
	const index = split[0].getIndexWithinParent() + x;
	if (!pointParent.isInline() || index === 0) {
	return index;
	}

If we determine our parent is an inline element but see that we're at the start of it, we realize we don't need to split the inline element so we just return 0 despite the fact that it's not an index into the block ancestor's children. This is the cause of the weird behavior reported in #5251 and reproduced in the test case in this PR.

Even when we do split the parent inline element, we don't do it recursively, assuming that its parent must be a block ancestor:

lexical/packages/lexical/src/LexicalSelection.ts

Lines 2875 to 2880 in 794e6dd

	const firstToAppend = pointParent.getChildAtIndex(index);
	if (firstToAppend) {
	const newBlock = pointParent.insertNewAfter(selection) as ElementNode;
	newBlock.append(firstToAppend, ...firstToAppend.getNextSiblings());
	}
	return pointParent.getIndexWithinParent() + x;

So I think we can go a couple of directions here:

1. Make removeTextAndSplitBlock handle inline elements properly included nested inlines and ensure it always returns an index into the nearest block ancestor.

2. Allow nodes to be inserted into inlines and let inlines take care of ensuring the sanity of their children (their content model).

I think (2) is actually better for users and authors in the long term.

For example, even if Word and Google Docs do it, it's weird for a user that if they cut "abc" from a rich text editor and paste it into the middle of a link the link gets split, but if they cut the same "abc" from a plain text editor and paste it, the link doesn't get split. I know you can use Ctrl + Shift + V to paste plain text but 99% of users won't know that.

If we went with (2), for the case of pasting a link into a link, presumably LinkNode would have a node transform that handles that case. For other types of inline elements, nesting the same type of element might be valid so it should be up to the specific inline element to sanitize its child content.

Anyway, I will give (1) a try anyway to see what it looks like.

Update: I notice that if we do (2) we wouldn't need the special case for code blocks—which possibly isn't quite right anyway since it only copies the first node's text content. The fact that this special case exists suggests that other custom node types may want similar handling.

[birtles]

I've updated this PR to implement approach 1 from #5394 (comment)

Before I go ahead and add more tests, however, it's probably worth discussing which approach we want to take.

If we end up going with approach 1 (i.e. always split inline elements) then in our app we'll probably intercept pastes, detect if the selection is one our custom nodes and, if so, strip the text from the pasted content and force-paste as text instead. I think that would be workable for us.

[GermanJablo]

Approach 2 would imply that Lexical would have to somehow expose a set of default transformations. It's something that will surely happen at some point, but not currently and it will probably require a lot of thought about how to do it.

On the other hand, converting the clipboard to plain text does not seem like a safe solution to me.

I'm not decidedly against approach 2, but it would require (1) default transformations on Lexical's part and (2) agreeing on what those transformations should be for nested inline nodes. Reaching agreement on these 2 points can be a longer and more complicated process than the simpler approach 1.

[birtles]

The schema proposal is interesting. I hadn't seen that. I was imagining something simpler along the lines of the existing canBeEmpty, canReplaceWith methods of the ElementNode interface. For example, a canInsertNode method which would allow parent nodes to restrict what nodes can be added as children. When it returns false, we'd split the node. That would seem to be sufficient for the purposes of this issue. If it further had a "see parent" sentinel value then I think it could represent HTML's content models including the transparent content model concept.

I'm more than happy to go ahead with approach 1 for now since I don't have the bandwidth to drive approach 2 forwards.

From the point of view of our app, doing the plain text conversion would give us something reasonably useful for now and if/when approach 2 is ready, we could switch to that to get higher fidelity pasting into our custom inline elements.

[GermanJablo]

I'm more than happy to go ahead with approach 1 for now since I don't have the bandwidth to drive approach 2 forwards [...] if/when approach 2 is ready, we could switch to that [...]

That seems perfect to me, I think it's the best way to approach this.

Disclaimer: PR approval is not up to me so, CC: @zurfyx @fantactuka @acywatson

[birtles]

That seems perfect to me, I think it's the best way to approach this.

Great, thank you! I'll add some tests to the PR and squash it down so it's easier to review.

I'd like to write an E2E test for pasting into nested inline elements but do you happen to know of any inline elements available in the playground that support child inline elements?

[GermanJablo]

I'd like to write an E2E test for pasting into nested inline elements but do you happen to know of any inline elements available in the playground that support child inline elements?

The only ElementNodes of type inline that I remember are LinkNode, AutoLinkNode and MarkNode.

I think that with the 4 tests I mentioned above, we would be covered.

[birtles]

I think that with the 4 tests I mentioned above, we would be covered.

I've added those tests and tidied up the patch so this PR should be ready for review now.

[GermanJablo]

Incredible work, thank you very much!
This looks good to me 👌

[ivailop7]

LGTM