refactor: replace insertText from #6553 changes to reflect new removeText feature

[vantage-ola]

This PR aims to Refactor #6553 : simplified insertText rewrite (part 2)

Right now, without changing the commented code that uses the new removeText feature. It passes almost all the tests except
2 cases

● LexicalSelectionHelpers tests › Collapsed › Can handle a text point

   expect(received).toBe(expected) // Object.is equality

   Expected: "Testa"
   Received: "a"

     148 |         selection.insertText('Test');
     149 |
   > 150 |         expect($getNodeByKey('a')!.getTextContent()).toBe('Testa');
         |                                                      ^
     151 |
     152 |         expect(selection.anchor).toEqual(
     153 |           expect.objectContaining({

 ● LexicalSelectionHelpers tests › Simple range › Can handle multiple text points

   expect(received).toBe(expected) // Object.is equality

   Expected: "Test"
   Received: ""

     1352 |         selection.insertText('Test');
     1353 |
   > 1354 |         expect($getNodeByKey('a')!.getTextContent()).toBe('Test');
          |                                                      ^
     1355 |
     1356 |         expect(selection.anchor).toEqual(
     1357 |           expect.objectContaining({

I will be investigating why this is the issue and as @GermanJablo said, i will make some refinements to his draft insertText to fix those errors

[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	Nov 15, 2024 6:17pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 15, 2024 6:17pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	30.85 KB (0%)
lexical - esm	30.73 KB (0%)
@lexical/rich-text - cjs	39.58 KB (0%)
@lexical/rich-text - esm	32.67 KB (0%)
@lexical/plain-text - cjs	38.22 KB (0%)
@lexical/plain-text - esm	29.93 KB (0%)
@lexical/react - cjs	41.35 KB (0%)
@lexical/react - esm	34.03 KB (0%)

[vantage-ola]

Its funny now, coz it passes all the tests here, but fails those two issues locally...

[GermanJablo]

This is great! Thanks for continuing this ❤️

[etrepum]

As a new contributor, for security reasons, GitHub doesn't automatically run the CI tests before a reviewer clicks a button. It'll probably fail when they are run.

[vantage-ola]

mehnn, was this tuff... rewrote the inserText functions so many times ( i do have four implemenatations of it lol ), before i could come close to figuring out the issue.
Right now, one test is still failing becuase the anchor offset goes off by one...

  ● LexicalSelectionHelpers tests › Collapsed › Can handle a text point

    expect(received).toEqual(expected) // deep equality

    - Expected  -  2
    + Received  + 15

    - ObjectContaining {
    + Point {
    +   "_selection": RangeSelection {
    +     "_cachedNodes": null,
    +     "anchor": [Circular],
    +     "dirty": true,
    +     "focus": Point {
    +       "_selection": [Circular],
    +       "key": "a",
    +       "offset": 5,
    +       "type": "text",
    +     },
    +     "format": 0,
    +     "style": "",
    +   },
        "key": "a",
    -   "offset": 4,
    +   "offset": 5,
        "type": "text",
      }

      150 |                 expect($getNodeByKey('a')!.getTextContent()).toBe('Testa');
      151 |
    > 152 |                 expect(selection.anchor).toEqual(
          |                                          ^
      153 |                     expect.objectContaining({
      154 |                         key: 'a',
      155 |                         offset: 4,

      at packages/lexical-selection/src/__tests__/unit/LexicalSelectionHelpers.test.ts:152:42
      at packages/lexical-selection/src/__tests__/unit/LexicalSelectionHelpers.test.ts:132:21
      at $beginUpdate (packages/lexical/src/LexicalUpdates.ts:931:5)
      at updateEditor (packages/lexical/src/LexicalUpdates.ts:1038:5)
      at LexicalEditor.update (packages/lexical/src/LexicalEditor.ts:1188:17)
      at setupTestCase (packages/lexical-selection/src/__tests__/unit/LexicalSelectionHelpers.test.ts:97:24)
      at Object.<anonymous> (packages/lexical-selection/src/__tests__/unit/LexicalSelectionHelpers.test.ts:147:13)

[vantage-ola]

  ` 
  insertText(text: string): void {
...
  if (this.anchor.offset === 0) {
        // If parent is inline and no previous siblings, insert before parent
        if (parent.isInline() && !anchorNode.__prev) {
          parent.insertBefore(textNode);
        } else if (anchor.key === focus.key) {
          // it sets the offset by +1 dont know why
          //     -   "offset": 4,
          //+   "offset": 5,
          // Still WIP, fixing it asap...one test depends on this...
          const currentContent = anchorNode.getTextContent();
          const newContent = text + currentContent.slice(currentOffset);
          anchorNode.setTextContent(newContent);
...          
`

anchorNode.setTextContent(newContent); // offset will be length of currentContent + newContent ...

ideally for that condition where there is pre existing content in the node, the offset should be the length of the new text we want to insert not the lenght of both..

// packages/lexical-selection/src/tests/unit/LexicalSelectionHelpers.test.ts line 152

expect($getNodeByKey('a')!.getTextContent()).toBe('Testa');

the test fails
`- Expected - 2
+ Received + 15

- ObjectContaining {
+ Point {
+   "_selection": RangeSelection {
+     "_cachedNodes": null,
+     "anchor": [Circular],
+     "dirty": true,
+     "focus": Point {
+       "_selection": [Circular],
+       "key": "a",
+       "offset": 5,
+       "type": "text",
+     },
+     "format": 0,
+     "style": "",
+   },
    "key": "a",
-   "offset": 4,
+   "offset": 5,
    "type": "text",
  }`

[vantage-ola]

i could add this after setting new content, anchorNode.setTextContent(newContent);

          this.anchor.offset = text.length;
          this.focus.offset = text.length;

which should work, but i think because its stuck inside the loop, so it just defaults the offset of the anchor and focus....

@GermanJablo @etrepum what do you suggest i do?

[etrepum]

I think in this scenario you also need to consider cases where anchor.offset !== focus.offset, we know that anchor.offset === 0 but we don't know what focus.offset is, so it should probably be this:

        } else if (anchor.key === focus.key) {
          // it sets the offset by +1 dont know why
          //     -   "offset": 4,
          //+   "offset": 5,
          // Still WIP, fixing it asap...one test depends on this...
          const currentContent = anchorNode.getTextContent();
          const newContent = text + currentContent.slice(focus.offset);
          anchorNode.setTextContent(newContent);
        }

I'm sure there are other problems though, because focus.offset isn't even considered and I don't see how the selection will be updated correctly in the cases where setTextContent is called

[vantage-ola]

@etrepum @GermanJablo think it's good to go

[vantage-ola]

i didnt even realise there were e2e tests... wow , i wonder how I will debug this thing and proceed 😅

[vantage-ola]

ok, looking at the older 'insertText' implementation....

the refactored one I am working on(although passes the core tests but fails some e2e tests)...there are some missing features like

• Different node types (segmented/token nodes)
• Format preservation during insertion
• some Complex selection scenarios
• and Insertion restrictions (before/after certain nodes)
  from the older implementation I could add...

I will try to add them features and try again 👍

[GermanJablo]

In the draft version I commented on at the time, only about 5 tests failed, considering both unit tests and e2e, so it shouldn't require any major changes.
Also, my draft version starts with this line: this.removeText(). Basically, that's the end goal behind all this effort. Make insertText depend on removeText and not vice versa.

You are calling removeText only if the text to be inserted is an empty string. I can't guarantee it, but I don't think that's the best approach. In removeText I've left some heuristics that take into account some complex cases like the ones you mention (segmented/token nodes, etc.). If some tests fail because you use removeText in all cases, it's probably the removeText method that requires some additional modification.

The selection module is very complex, so this is not an easy task.

[vantage-ola]

@GermanJablo hmmm, i understand. I have been doing it all wrong(or taking a wrong apprach) I did not pay attention to the removeText much. The reason I removed this.removeText() in the first line and added a condition is because a test require you not to remove text at first or else it will fail..

But as you said, the removeText might need some modification to pass both tests... Thanks

[vantage-ola]

I am having some problems moving forward in the pr, would love a review to point me in the right direction. Im a little blocked ... @GermanJablo @etrepum

[etrepum]

I think the first step would be to follow @GermanJablo's suggestion and make the removeText unconditional, without going deep into the code and seeing which tests that affects it would be hard to provide any more direction