[lexical][lexical-overflow] Refactor: simplified removeText and insertText rewrite (part 1) #6456

GermanJablo · 2024-07-24T21:48:25Z

This is something I've been wanting to do since I did the insertNodes rewrite. These are also methods with long and very complex implementations.

Currently, removeText depends on insertText, since removeText is defined as inserting an empty string (insertText("")). This PR seeks to reverse the dependency of these methods, making insertText dependent on removeText.

This can be achieved in 2 steps:

Rewrite removeText without making it dependent on insertText.
Rewrite insertText with the new removeText.

So far in this PR, I have managed to complete step 1 and 95% of step 2. I decided to leave step 2 for a later PR because there are about 4 tests that fail and, after having dedicated so much time to it, I want to do a pause on this.

…nsert-text

vercel · 2024-07-24T21:48:29Z

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

github-actions · 2024-07-24T21:50:11Z

size-limit report 📦

Path	Size
lexical - cjs	29.38 KB (0%)
lexical - esm	29.22 KB (0%)
@lexical/rich-text - cjs	37.87 KB (0%)
@lexical/rich-text - esm	31.08 KB (0%)
@lexical/plain-text - cjs	36.45 KB (0%)
@lexical/plain-text - esm	28.44 KB (0%)
@lexical/react - cjs	39.64 KB (0%)
@lexical/react - esm	32.52 KB (0%)

GermanJablo · 2024-07-24T21:51:05Z

packages/lexical-overflow/src/index.ts

+
+  isInline(): boolean {
+    return true;
+  }
+
+  static transform(): (node: LexicalNode) => void {
+    return (node: LexicalNode) => {
+      invariant($isOverflowNode(node), 'node is not a OverflowNode');
+      if (node.isEmpty()) {
+        node.remove();
+      }
+    };
+  }


It's likely that insert/removeText currently has some logic that prevents this. If it works in all cases, I don't know.

Ensuring that an overFlowNode cannot be empty in lexical-overflow is a more robust and less error-prone solution, and it frees up the load on other packages.

Due to this change, it can be understood why the eliminated unit test is no longer necessary.

GermanJablo · 2024-07-24T21:55:49Z

packages/lexical/src/LexicalSelection.ts

+    // Now that "removeText" has been improved and does not depend on
+    // insertText, insertText can be greatly simplified. The next
+    // commented version is a WIP (about 5 tests fail).
+    //
+    // this.removeText();
+    // if (text === '') {
+    //   return;
+    // }
+    // const anchorNode = this.anchor.getNode();
+    // const textNode = $createTextNode(text);
+    // textNode.setFormat(this.format);
+    // textNode.setStyle(this.style);
+    // if ($isTextNode(anchorNode)) {
+    //   const parent = anchorNode.getParentOrThrow();
+    //   if (this.anchor.offset === 0) {
+    //     if (parent.isInline() && !anchorNode.__prev) {
+    //       parent.insertBefore(textNode);
+    //     } else {
+    //       anchorNode.insertBefore(textNode);
+    //     }
+    //   } else if (this.anchor.offset === anchorNode.getTextContentSize()) {
+    //     if (parent.isInline() && !anchorNode.__next) {
+    //       parent.insertAfter(textNode);
+    //     } else {
+    //       anchorNode.insertAfter(textNode);
+    //     }
+    //   } else {
+    //     const [before] = anchorNode.splitText(this.anchor.offset);
+    //     before.insertAfter(textNode);
+    //   }
+    // } else {
+    //   anchorNode.splice(this.anchor.offset, 0, [textNode]);
+    // }
+    // const nodeToSelect = textNode.isAttached() ? textNode : anchorNode;
+    // nodeToSelect.selectEnd();
+    // // When composing, we need to adjust the anchor offset so that
+    // // we correctly replace that right range.
+    // if (
+    //   textNode.isComposing() &&
+    //   this.anchor.type === 'text' &&
+    //   anchorNode.getTextContent() !== ''
+    // ) {
+    //   this.anchor.offset -= text.length;
+    // }
+


Once the failing tests are fixed and this implementation is activated, 364 lines of code can be removed ($transferStartingElementPointToTextPoint is only used in the current implementation of insertText).

Do you expect to add that to this PR, or after this one lands?

GermanJablo · 2024-08-25T21:53:22Z

done @etrepum

etrepum

Could use another set of eyes on this one since it's such a fundamental change to removeText

ivailop7 · 2024-08-26T07:20:36Z

Let's do 0.17.1 first and then do this one as a 0.18 with the defaults change.

ivailop7 · 2024-08-26T14:21:34Z

packages/lexical-playground/src/plugins/CollapsiblePlugin/CollapsibleTitleNode.ts

@@ -96,6 +96,18 @@ export class CollapsibleTitleNode extends ElementNode {
    return true;
  }

+  static transform(): (node: LexicalNode) => void {


Why are we adding this method to this specific node, when deletion of this one here happens on container node-level?

Not always. If you remove this you will see that some tests fail because there may be empty CollapsibleTitleNodes.
Apparently, it wasn't necessary because it was one of the many things that insertText did.

OK, let's update those tests and remove the transform part from the node, since when people copy-paste the Playground it would create more confusion, if it's purely there for tests to pass.

I think both may be necessary for correct behavior, not just tests. The container won't be empty if it has an empty title since it's measured by children and not text, so the empty title is removed first and then the next pass removes the container

Yes, it's not just about the tests. I don't see anything wrong or confusing here. It's a pattern that other nodes use.

When I said "it wasn't necessary because it was one of the many things that insertText did", I meant that insertText solved this for certain cases, but we don't know what other situations weren't covered. This solution is more robust and secure.

That's the part that confuses me, the Layout plugin should be in the same boat by that rule, since it's near identical implementation to the Collapsible Plugin, unless it's purely because we are missing tests for it and the transform method should be added to the Layout plugin as well?

As a general rule I think it's a good idea to use transforms to make sure that nodes that should be used together can't be orphaned.
I don't know what the layout plugin is but I think improving that would be out of scope of this PR.

PS: I updated the branch and there is a flaky test that failed. Can you please restart it?

I think in general we are missing transforms for a handful of nodes to guarantee correct behavior, e.g. the merge-unmerge issue with TableCellNode not having paragraph children. The static transform method is experimental and new-ish, previously you had to have the correct editor and/or plugin configuration to have all of the required transforms. I would agree that adding other transforms is out of scope for this PR, unless the previous removeText implementation also handled them.

ivailop7

Approving, trusting you both you have this planned out. Haven't spent much time getting the whole context here.

adrianmxb · 2024-10-01T14:58:51Z

I am a bit late to the party but the changes here break removal of TextNodes in token mode. @ivailop7 @GermanJablo

One example

etrepum · 2024-10-01T15:20:49Z

Do you mind creating a new issue so that this isn't lost?

GermanJablo · 2024-10-01T15:24:17Z

Isn't that supposedly the correct behavior, or am I missing something? Are you sure it worked differently before? There were a lot of those tests I had to pass.

adrianmxb · 2024-10-01T16:26:51Z

Chose an unfortunate example here, the mention node created in the GIF is in token mode to demo the issue.

Anyway, our users can insert placeholder nodes, which are basically a custom text node in token mode.

According to the docs:

mode
token - acts as immutable node, can't change its content and is deleted all at once

Before these changes, token nodes behaved as described in the documentation.

For segmented mode the behaviour is correct, I updated my initial comment, sorry for that one.

GermanJablo and others added 14 commits July 11, 2024 19:32

save WIP

5dbe034

save WIP

87e095d

save WIP

b136ee7

save WIP

ffdb4b7

save WIP

7132252

save WIP

da14af6

ALL UNIT TEST PASS

cc94d4e

ALL E2E TEST PASS (I am not sure about 2 unit tests)

e3b5491

fix with heading insertNewAfter

473edd4

Merge remote-tracking branch 'origin/main' into remove-insert-text

f119003

Merge commit '5a7d9c71d8b877f77f7d618ff228159433f4d520' into remove-i…

3dbfe80

…nsert-text

fix almost all ime tests

7685247

save

f1e11b5

remove splice change

22ac972

GermanJablo requested review from zurfyx, fantactuka, acywatson, Fetz, ivailop7, Sahejkm and potatowagon as code owners July 24, 2024 21:48

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 Jul 24, 2024

vercel bot deployed to Preview – lexical July 24, 2024 21:49 View deployment

vercel bot deployed to Preview – lexical-playground July 24, 2024 21:49 View deployment

GermanJablo commented Jul 24, 2024

View reviewed changes

fix unit test overflow

5d84122

vercel bot deployed to Preview – lexical July 24, 2024 22:14 View deployment

vercel bot deployed to Preview – lexical-playground August 25, 2024 13:31 View deployment

fix unit test

046a911

vercel bot deployed to Preview – lexical-playground August 25, 2024 21:35 View deployment

vercel bot deployed to Preview – lexical August 25, 2024 21:35 View deployment

GermanJablo mentioned this pull request Aug 25, 2024

Refactor: simplified insertText rewrite (part 2) #6553

Open

etrepum approved these changes Aug 26, 2024

View reviewed changes

ivailop7 reviewed Aug 26, 2024

View reviewed changes

Merge branch 'main' into remove-insert-text

3b7bec7

vercel bot deployed to Preview – lexical-playground August 26, 2024 16:37 View deployment

vercel bot deployed to Preview – lexical August 26, 2024 16:37 View deployment

Merge branch 'main' into remove-insert-text

cfb2ca6

vercel bot deployed to Preview – lexical August 26, 2024 19:15 View deployment

vercel bot deployed to Preview – lexical-playground August 26, 2024 19:15 View deployment

ivailop7 approved these changes Aug 27, 2024

View reviewed changes

Merge branch 'main' into remove-insert-text

2146a7d

vercel bot deployed to Preview – lexical September 4, 2024 23:11 View deployment

vercel bot deployed to Preview – lexical-playground September 4, 2024 23:11 View deployment

etrepum added this pull request to the merge queue Sep 4, 2024

Merged via the queue into facebook:main with commit bd507b9 Sep 4, 2024
38 checks passed

adrianmxb mentioned this pull request Oct 1, 2024

Bug: removeText misbehaves for text nodes in token mode #6687

Closed

etrepum added a commit to etrepum/lexical that referenced this pull request Oct 1, 2024

Failing test for facebook#6687 (regression in facebook#6456)

ea1aab1

etrepum mentioned this pull request Oct 1, 2024

[lexical] Bug Fix: TextNode in token mode should not be split by removeText #6690

Merged

etrepum added a commit to etrepum/lexical that referenced this pull request Oct 1, 2024

Failing tests for facebook#6687 (regression in facebook#6456)

cd50c99

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lexical][lexical-overflow] Refactor: simplified removeText and insertText rewrite (part 1) #6456

[lexical][lexical-overflow] Refactor: simplified removeText and insertText rewrite (part 1) #6456

GermanJablo commented Jul 24, 2024

vercel bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

GermanJablo Jul 24, 2024 •

edited

Loading

GermanJablo Jul 24, 2024

etrepum Aug 4, 2024

GermanJablo commented Aug 25, 2024

etrepum left a comment

ivailop7 commented Aug 26, 2024

ivailop7 Aug 26, 2024 •

edited

Loading

GermanJablo Aug 26, 2024

ivailop7 Aug 26, 2024

etrepum Aug 26, 2024

GermanJablo Aug 26, 2024 •

edited

Loading

ivailop7 Aug 26, 2024

GermanJablo Aug 26, 2024

etrepum Aug 26, 2024

ivailop7 left a comment

adrianmxb commented Oct 1, 2024 •

edited

Loading

etrepum commented Oct 1, 2024

GermanJablo commented Oct 1, 2024

adrianmxb commented Oct 1, 2024 •

edited

Loading

[lexical][lexical-overflow] Refactor: simplified removeText and insertText rewrite (part 1) #6456

[lexical][lexical-overflow] Refactor: simplified removeText and insertText rewrite (part 1) #6456

Conversation

GermanJablo commented Jul 24, 2024

vercel bot commented Jul 24, 2024 • edited Loading

github-actions bot commented Jul 24, 2024 • edited Loading

size-limit report 📦

GermanJablo Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GermanJablo commented Aug 25, 2024

etrepum left a comment

Choose a reason for hiding this comment

ivailop7 commented Aug 26, 2024

ivailop7 Aug 26, 2024 • 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

GermanJablo Aug 26, 2024 • 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

ivailop7 left a comment

Choose a reason for hiding this comment

adrianmxb commented Oct 1, 2024 • edited Loading

etrepum commented Oct 1, 2024

GermanJablo commented Oct 1, 2024

adrianmxb commented Oct 1, 2024 • edited Loading

vercel bot commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

GermanJablo Jul 24, 2024 •

edited

Loading

ivailop7 Aug 26, 2024 •

edited

Loading

GermanJablo Aug 26, 2024 •

edited

Loading

adrianmxb commented Oct 1, 2024 •

edited

Loading

adrianmxb commented Oct 1, 2024 •

edited

Loading