[lexical] Bug Fix: Merge pasted paragraph into empty quote

[2wheeh]

Description

It adds handling for pasting paragraph into an empty quote node.

Closes #5108

Test plan

Before

https://github.com/facebook/lexical/assets/40269597/c0f404db-f816-4d28-b0ae-db48e4985a9d

After

https://github.com/facebook/lexical/assets/40269597/a0c90ce2-239a-49cf-aa76-17bc2c1f1f2a

[vercel[bot]]

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 1, 2024 11:22pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 1, 2024 11:22pm

[github-actions[bot]]

size-limit report 📦

Path	Size
lexical - cjs	29.05 KB (0%)
lexical - esm	28.86 KB (0%)
@lexical/rich-text - cjs	37.39 KB (0%)
@lexical/rich-text - esm	28.33 KB (0%)
@lexical/plain-text - cjs	36.02 KB (0%)
@lexical/plain-text - esm	25.6 KB (0%)
@lexical/react - cjs	39.29 KB (0%)
@lexical/react - esm	29.77 KB (0%)

[2wheeh]

integration test failure is not related to this PR: https://github.com/facebook/lexical/issues/6413

[StyleT]

Hi!

I think we lack documentation here. Feel free to add JSDoc + section to https://lexical.dev/docs/concepts/nodes about this new method

[StyleT]

@GermanJablo PTAL as discussed.

Afterthought: seems like call of the canMergeWhenEmpty shall not happen within CASE 3: At least 1 element of the array is not inline to solve issue with quote (which is not a list).

So more information & research is needed here...

[2wheeh]

@StyleT

Afterthought: seems like call of the canMergeWhenEmpty shall not happen within CASE 3: At least 1 element of the array is not inline to solve issue with quote (which is not a list).

Based on my understanding, CASE 3: At least 1 element of the array is not inline handles cases where inserted nodes contain non-inline elements such as Paragraphs.

The call of canMergeWhenEmpty determines whether an inserted non-inline node is mergeable with firstBlock when firstBlock (which can be a ListItem, Quote, etc.) is empty.

isMergeable returns true when firstBlock is not empty or when firstBlock can merge even if it's empty:

const isMergeable = (node: LexicalNode): node is ElementNode =>
  $isElementNode(node) &&
  INTERNAL_$isBlock(node) &&
  !node.isEmpty() &&
  $isElementNode(firstBlock) &&
  (!firstBlock.isEmpty() || firstBlock.canMergeWhenEmpty());

if (isMergeable(firstToInsert)) {
  // merge
}

Currently, we only check if firstBlock is a ListItem with the call of isLI. To fix this issue with empty Quote, we need Quote (or other elements) to also merge inserted nodes when it is empty.

That's why canMergeWhenEmpty is introduced as a generalized method. I don't see any reason why Case 3 should be limited to ListItems only.

If I've misunderstood your comment, could you please elaborate further?

[GermanJablo]

• I'm ok with a solution like canMergeWhenEmpty. My only observation is that isLi is still used in one more place. If it is replaced by canMergeWhenEmpty, does any test fail?
• Do we agree that the correct behavior should be that of the video "after" (or that of the new test)? I mean, there is also the possibility that the 2 paragraphs are inserted within the quote. I don't know how GDocs and Word do it.

[2wheeh]

@GermanJablo

My only observation is that isLi is still used in one more place. If it is replaced by canMergeWhenEmpty, does any test fail?

It doesn't fail.

But the other isLI call is to remove the ListItem inserted by insertNewAfter at the end. I'm not sure replacing this with canMergeWhenEmpty would be semantically correct.

Instead, we could check if insertedParagraph has same type as lastInsertedBlock:

- isLI(insertedParagraph) || INTERNAL_$isBlock(lastToInsert)
+ insertedParagraph.__type === lastInsertedBlock.__type || INTERNAL_$isBlock(lastToInsert)

or we could introduce new generalized method for this behavior, which could be useful for the nodes that might want to keep the inserted Paragraph from insertNewAfter.

I don't know how GDocs and Word do it.

AFAIK, GDocs and Word don't have a dedicated "block quote" format in the way we (or other block based editors such as Notion) do. Instead, they just visually mimic the appearance of a block quote by a combination of formatting options.

At least, Notion behaves like the video "after" but yes, it needs to agree.

[GermanJablo]

1. It seems fine to me.
2. My intuition tells me that both should be included in the quote, although I don't have strong opinions on the matter. Maybe someone else wants to contribute their opinion.

[2wheeh]

1. done it.
2. I don't have strong opinions on this either but just mentioning for agreement. If it's just about quote, more dedicated fix would be needed here. Otherwise ListItem insertion behavior changes as well:

https://github.com/user-attachments/assets/5783fae3-256c-4454-a10c-ff13fe257f2a

[GermanJablo]

LGTM

[etrepum]

I created #6443 to track the missing docs

[2wheeh]

@etrepum @StyleT Thank you. I added jsdoc here.

[2wheeh]

Failure of e2e-tests / prod (macos-latest) ... is due to #6463