Fix `insertNodes` and `insertParagraph` #5002

GermanJablo · 2023-09-14T15:20:17Z

This PR greatly simplifies the insertNodes and insertParagraph methods and resolves Bug: Pasting text immediately after an inline element (e.g. link) causes the text and its following siblings to be moved into the inline element #4295
Due to design errors in those methods that were modified, insertNewAfter in LinkNode was not "inserting a new link after", but rather a new block. This hack has been fixed and removed as it is no longer needed. ~~I've also added a to-do comment that insertNewAfter should not return LexicalNode | null but this | null. This would be easier to achieve now~~. Change of mind. Actually what would have been better is for insertNewAfter to be called handleEnter, since it is used only for that, and leads to confusion.
I have moved the $isBlock function from range-selection.ts to @lexical/utils. I've marked it as internal use because I think maybe later some nodes should expose an isBlock() method. Topic for another PR.
I have moved the $getAncestor function from lexical-link/src/index.ts to @lexical/utils. I have corrected a minor error that this function had and have also made it return a more precise type. I have used this function in a couple of places where you can see how it simplifies the code. Surely there is potential to apply it in many more places. A tip: often when using the node.isInline method what you are actually trying to achieve is $getAncestor(node, $isBlock).

vercel · 2023-09-14T15:20:23Z

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	Oct 27, 2023 6:05pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 27, 2023 6:05pm

GermanJablo · 2023-09-14T15:22:06Z

packages/lexical-link/src/index.ts

  insertNewAfter(
-    selection: RangeSelection,
+    _: RangeSelection,
    restoreSelection = true,
  ): null | ElementNode {
-    const element = this.getParentOrThrow().insertNewAfter(
-      selection,
-      restoreSelection,
-    );
-    if ($isElementNode(element)) {
-      const linkNode = $createLinkNode(this.__url, {
-        rel: this.__rel,
-        target: this.__target,
-        title: this.__title,
-      });
-      element.append(linkNode);
-      return linkNode;
-    }
-    return null;
+    const linkNode = $createLinkNode(this.__url, {
+      rel: this.__rel,
+      target: this.__target,
+      title: this.__title,
+    });
+    this.insertAfter(linkNode, restoreSelection);
+    return linkNode;


Quoting from the PR description:

Due to design errors in those methods that were modified, insertNewAfter in LinkNode was not "inserting a new link after", but rather a new block. This hack has been fixed and removed as it is no longer needed. I've also added a to-do comment that insertNewAfter should not return LexicalNode | null but this | null. This would be easier to achieve now.

GermanJablo · 2023-09-14T15:23:44Z

packages/lexical-link/src/index.ts

-      const linkNode = $isLinkNode(firstNode)
-        ? firstNode
-        : $getLinkAncestor(firstNode);
+      const linkNode = $getAncestor(firstNode, $isLinkNode);


One detail that I noticed the new $getAncestor function improved

GermanJablo · 2023-09-14T15:26:07Z

packages/lexical-selection/src/range-selection.ts

+  const firstSelectedBlock = $getAncestor(
+    selection.anchor.getNode(),
+    INTERNAL_$isBlock,
+  );
+  if (firstSelectedBlock && nodes.indexOf(firstSelectedBlock) === -1) {
+    nodes.push(firstSelectedBlock);
  }


I moved $isBlock from this file. I remembered that a few lines above (here), I had this code where I searched for the closest block ancestor. Cleaner this way.

GermanJablo · 2023-09-14T15:28:01Z

packages/lexical/src/LexicalSelection.ts

  insertRawText(text: string): void {
    const parts = text.split(/(\r?\n|\t)/);
-    const nodes = [];
+    const paragraph = $createParagraphNode();
    const length = parts.length;
    for (let i = 0; i < length; i++) {
      const part = parts[i];
      if (part === '\n' || part === '\r\n') {
-        nodes.push($createLineBreakNode());
+        paragraph.append($createLineBreakNode());
      } else if (part === '\t') {
-        nodes.push($createTabNode());
+        paragraph.append($createTabNode());
      } else {
-        nodes.push($createTextNode(part));
+        paragraph.append($createTextNode(part));
      }
    }
-    this.insertNodes(nodes);
+    this.insertNodes([paragraph]);


This is required by modifications to the insertNodes method

packages/lexical/src/LexicalSelection.ts

GermanJablo · 2023-09-14T15:35:18Z

packages/lexical/src/LexicalSelection.ts

+function splitBlock(point: PointType, selection: RangeSelection) {
+  let pointNode = point.getNode();
+  if ($isElementNode(pointNode)) {
+    // TO-DO: InsertNewAfter should return 'null | this' instead of 'null | LexicalNode'
+    const newBlock = pointNode.insertNewAfter(selection, false) as ElementNode;
+    if (newBlock) newBlock.select();
+    return newBlock;
+  }
+
+  const splitElement = (element: ElementNode) => {
+    const {offset} = point;
+    const parent = pointNode.getParentOrThrow();
+    const x = parent.isInline() ? parent : pointNode;
+    const firstToAppend = offset === 0 ? x : pointNode.splitText(offset)[0];
+    const siblings = firstToAppend.getNextSiblings();
+    const nodesToAppend =
+      offset === 0 ? [firstToAppend, ...siblings] : siblings;
+    const newElement = element.insertNewAfter(selection, false) as ElementNode;
+    if (newElement) {
+      newElement.append(...nodesToAppend);
+      newElement.selectStart();
+    }
+    return newElement;
+  };
+
+  if (pointNode.getParentOrThrow().isInline()) {
+    splitElement(pointNode.getParentOrThrow());
+  }
+
+  pointNode = point.getNode();
+  const block = $getAncestor(pointNode, INTERNAL_$isBlock);
+  if (block) return splitElement(block);
+  return null;
+}


Basically the splitElement function is executed once, unless the anchor is inside an inline element (in which case it is executed twice).

Will it handle case when link is inside mark node (inline within inline)?

Good question. No. But right now it's not working on Lexical either. You can confirm it in the playground by pressing enter on a comment that involves a link. That's why it should be a job for another PR.

packages/lexical/src/LexicalSelection.ts

GermanJablo · 2023-10-19T17:22:47Z

packages/lexical-playground/__tests__/e2e/CopyAndPaste/html/HTMLCopyAndPaste.spec.mjs

      focusOffset: 16,
-      focusPath: [2, 0, 0],
+      focusPath: [1, 0, 0],
    });
  });



This could be considered a bug fixed, since there is no need to insert an empty paragraph

GermanJablo · 2023-10-19T17:24:19Z

packages/lexical-playground/__tests__/e2e/CopyAndPaste/html/ListsHTMLCopyAndPaste.spec.mjs

+        </ul>
+        <hr class="" contenteditable="false" data-lexical-decorator="true" />
+        <ul class="PlaygroundEditorTheme__ul">
          <li
-            value="2"
            class="PlaygroundEditorTheme__listItem PlaygroundEditorTheme__ltr"
-            dir="ltr">
+            dir="ltr"
+            value="1">


another improvement to insertNodes.
It doesn't make sense for the hr to be inserted after the list-item the cursor is over, if the cursor is at the beginning.

GermanJablo · 2023-10-19T17:39:41Z

Ok I did it!
My only doubt is that perhaps I caused a regression in #5066, as I asked in one of the comments.
I can confirm that this PR resolves #4295. I will try to add a test of this case in another PR.
Several bugs that were not reported were also solved, as can be seen in the modified tests.

Additionally, this PR lays the groundwork for greatly simplifying insertText and removeText (I already have an idea of how to do it in a few lines of code using my new RemoveTextAndSplitBlock primitive). I also think that with the new insertNodes many of the things that are done to normalize the nodes before pasting them are not going to be necessary. I will try to do all this in another PR.

GermanJablo · 2023-10-19T17:44:52Z

packages/lexical/src/LexicalSelection.ts

+    if ('__language' in firstBlock) {
+      if ('__language' in nodes[0]) {
+        this.insertText(nodes[0].getTextContent());
      } else {
-        target = placementNode;
+        const index = RemoveTextAndSplitBlock(this);
+        firstBlock.splice(index, 0, nodes);
+        const last = nodes.at(-1)!;
+        if (last.select) last.select();
+        else last.selectNext(0, 0);
      }
+      return;
    }


The node insertion logic is different in code.

I used '__language' in node instead of isCodeNode(node) to prevent core from depending on @lexical/code. Something that may be more elegant is to create an INSERT_NODES_COMMAND to which lexical/code is hooked.
I am going to address this at #4900

kevinansfield · 2023-10-20T15:12:24Z

Tried this out locally, looks like it also fixes #5088

zurfyx · 2023-10-23T13:29:55Z

Thanks @GermanJablo, appreciate your hard work on this! insertNodes has always been a hotspot for bugs and incredibly difficult to debug, arguably the hardest function to follow in the entire Lexical codebase. For reference, we discussed this PR on our last sync and we have intention to merge it.

GermanJablo · 2023-10-23T14:28:03Z

I'm glad to be of help!

I would appreciate any priority that can be given to this PR since keeping it updated with main can be difficult. If there's anything else I can do to move this forward, just let me know.

Thank you!

zurfyx

Thanks, great work GermanJablo! Left some minor comments

packages/lexical-clipboard/src/clipboard.ts

zurfyx · 2023-10-26T10:29:59Z

packages/lexical-code/src/CodeNode.ts

+      const split = firstSelectionNode.splitText(anchor.offset)[0];
+      const x = anchor.offset === 0 ? 0 : 1;
+      const index = split.getIndexWithinParent() + x;
+      const codeNode = firstSelectionNode.getParentOrThrow();
+      const nodesToInsert = [$createLineBreakNode(), ...insertNodes];
+      codeNode.splice(index, 0, nodesToInsert);
+      const last = insertNodes.at(-1);
+      if (last) last.select();
+      else if (anchor.offset === 0) split.selectPrevious();
+      else split.getNextSibling()!.selectNext(0, 0);
+    }
+    if ($isCodeNode(firstSelectionNode)) {
+      const {offset} = selection.anchor;
+      firstSelectionNode.splice(offset, 0, [$createLineBreakNode()]);
+      firstSelectionNode.select(offset + 1, offset + 1);


So add what point would you draw the line between using insertNodes and having to handle it yourself? My first impression on the above code is that you're repeating the TextNode split logic present in insertNodes and insertText.

Also, this logic is particularly complicated for anyone else to replicate, is there a possibility to have utilities for this?

One of the reasons why users use insertNodes heavily is to avoid having to split nodes at selection (otherwise insertAfter would do)

packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/CopyAndPaste.spec.mjs

zurfyx · 2023-10-26T10:43:27Z

packages/lexical-rich-text/src/index.ts

+      anchorOffet === this.getTextContentSize() || !selection
+        ? $createParagraphNode()
+        : $createHeadingNode(this.getTag());
    const direction = this.getDirection();
    newElement.setDirection(direction);
    this.insertAfter(newElement, restoreSelection);
+    if (anchorOffet === 0 && !this.isEmpty() && selection) {
+      this.replace($createParagraphNode(), restoreSelection);
+    }


Same thoughts as above even though in this case the resulting code is much simpler

packages/lexical-selection/src/__tests__/unit/LexicalSelectionHelpers.test.ts

packages/lexical/src/LexicalSelection.ts

zurfyx · 2023-10-26T11:06:46Z

packages/lexical/src/nodes/LexicalTabNode.ts

-  ): TextNode {
-    invariant(false, 'TabNode does not support spliceText');
-  }
-


Do we need to remove this? Basically TabNode is a one-off node that it's __text should never be modified, it should be a static \t. Other text should be preprended or inserted after, this node can only render \t

GermanJablo · 2023-10-26T12:38:24Z

Thanks to you @zurfyx. I already answered/resolved the comments!

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

GermanJablo added 6 commits September 13, 2023 00:14

helping comments

45f23de

first version splitBlock

dadfb30

splitBlock 2

539ec8f

fix insertNodes

79263fb

Merge 'main' into insertNodes

acee2ca

make $isBlock an internal function

dbe5b51

GermanJablo requested review from zurfyx, fantactuka, acywatson and thegreatercurve as code owners September 14, 2023 15:20

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 Sep 14, 2023

GermanJablo commented Sep 14, 2023

View reviewed changes

vercel bot deployed to Preview – lexical September 14, 2023 15:22 View deployment

vercel bot deployed to Preview – lexical-playground September 14, 2023 15:22 View deployment

GermanJablo commented Sep 14, 2023

View reviewed changes

packages/lexical/src/LexicalSelection.ts Outdated Show resolved Hide resolved

GermanJablo commented Sep 14, 2023

View reviewed changes

packages/lexical/src/LexicalSelection.ts Outdated Show resolved Hide resolved

GermanJablo mentioned this pull request Sep 14, 2023

Modularize the paste process with a new INSERT_NODES_COMMAND #4900

Closed

GermanJablo added 8 commits September 14, 2023 17:01

WIP codehighlight

eddff37

WIP codehighlight

e18b4ed

fix insertNewAfter of CodeNode. Temporal highlight alg

b01827a

plain text and code WIP

815831a

I DID IT!

a8ecdb5

simplify insertLineBreak

9321873

collocate splitBlock inside insertParagraph

3c3adbe

solve failed tests on code blocks

a5c1f06

remove toThrow from test node

87e6509

vercel bot deployed to Preview – lexical October 19, 2023 17:20 View deployment

vercel bot deployed to Preview – lexical-playground October 19, 2023 17:21 View deployment

GermanJablo commented Oct 19, 2023

View reviewed changes

zurfyx approved these changes Oct 26, 2023

View reviewed changes

GermanJablo added 3 commits October 26, 2023 08:29

replace forEach with for... of

c3e9626

fix uppercase. Add explicit return type

1c0b205

add brackets to if blocks with one statement

0f30d68

vercel bot deployed to Preview – lexical October 26, 2023 12:30 View deployment

vercel bot deployed to Preview – lexical-playground October 26, 2023 12:30 View deployment

fixme - flaky test

e04bb0f

vercel bot deployed to Preview – lexical October 27, 2023 18:04 View deployment

vercel bot deployed to Preview – lexical-playground October 27, 2023 18:05 View deployment

zurfyx merged commit f15a175 into facebook:main Oct 27, 2023
43 of 45 checks passed

GermanJablo deleted the insertNodes branch October 27, 2023 19:47

birtles mentioned this pull request Nov 20, 2023

Bug: insertNodes does weird things when pasting into inline elements [regression] #5251

Closed

birtles mentioned this pull request Dec 15, 2023

Fix insertNodes when inserting into inline elements #5394

Merged

birtles added a commit to birtles/lexical that referenced this pull request Dec 15, 2023

Make insertNodes append children to the first element

983123a

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

birtles added a commit to birtles/lexical that referenced this pull request Dec 15, 2023

Make insertNodes append children to the first element

11f5c07

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

birtles added a commit to birtles/lexical that referenced this pull request Dec 18, 2023

Make insertNodes append children to the first element

f245a4a

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

birtles added a commit to birtles/lexical that referenced this pull request Dec 21, 2023

Make insertNodes append children to the first element

baf0803

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

birtles added a commit to birtles/lexical that referenced this pull request Dec 27, 2023

Make insertNodes append children to the first element

0ddffa0

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

birtles added a commit to birtles/lexical that referenced this pull request Dec 27, 2023

Make insertNodes append children to the first element

165b25a

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `insertNodes` and `insertParagraph` #5002

Fix `insertNodes` and `insertParagraph` #5002

GermanJablo commented Sep 14, 2023 •

edited

Loading

vercel bot commented Sep 14, 2023 •

edited

Loading

GermanJablo Sep 14, 2023

GermanJablo Sep 14, 2023

GermanJablo Sep 14, 2023

GermanJablo Sep 14, 2023

GermanJablo Sep 14, 2023

fantactuka Sep 14, 2023

GermanJablo Sep 19, 2023

GermanJablo Oct 19, 2023

GermanJablo Oct 19, 2023

GermanJablo commented Oct 19, 2023

GermanJablo Oct 19, 2023

kevinansfield commented Oct 20, 2023 •

edited

Loading

zurfyx commented Oct 23, 2023

GermanJablo commented Oct 23, 2023

zurfyx left a comment

zurfyx Oct 26, 2023

zurfyx Oct 26, 2023

zurfyx Oct 26, 2023

GermanJablo commented Oct 26, 2023

Fix insertNodes and insertParagraph #5002

Fix insertNodes and insertParagraph #5002

Conversation

GermanJablo commented Sep 14, 2023 • edited Loading

vercel bot commented Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GermanJablo commented Oct 19, 2023

Choose a reason for hiding this comment

kevinansfield commented Oct 20, 2023 • edited Loading

zurfyx commented Oct 23, 2023

GermanJablo commented Oct 23, 2023

zurfyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GermanJablo commented Oct 26, 2023

Fix `insertNodes` and `insertParagraph` #5002

Fix `insertNodes` and `insertParagraph` #5002

GermanJablo commented Sep 14, 2023 •

edited

Loading

vercel bot commented Sep 14, 2023 •

edited

Loading

kevinansfield commented Oct 20, 2023 •

edited

Loading