Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: the problem where text inserted into a node with a leaf node wo… #6493

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

keiseiTi
Copy link
Contributor

@keiseiTi keiseiTi commented Aug 5, 2024

Description

The manifestation of this problem is:
When text is pasted into the parent node of a leaf node, the parent node will be split into two nodes. For example, the link node behaves like this.

I'm not sure this is normal behavior of lexical so I tried to fix this.

Closes #6477

Test plan

Before

iShot_2024-08-06_00 40 55

After

iShot_2024-08-06_00 45 39

Copy link

vercel bot commented Aug 5, 2024

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

@facebook-github-bot 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 Aug 5, 2024
Copy link

github-actions bot commented Aug 5, 2024

size-limit report 📦

Path Size
lexical - cjs 29.38 KB (0%)
lexical - esm 29.24 KB (0%)
@lexical/rich-text - cjs 37.74 KB (0%)
@lexical/rich-text - esm 31.05 KB (0%)
@lexical/plain-text - cjs 36.41 KB (0%)
@lexical/plain-text - esm 28.42 KB (0%)
@lexical/react - cjs 39.64 KB (0%)
@lexical/react - esm 32.51 KB (0%)

@etrepum
Copy link
Collaborator

etrepum commented Aug 5, 2024

It looks like this is currently the intended behavior, there's a test for it that's failing with this implementation

  1) [chromium] › packages/lexical-playground/__tests__/e2e/CopyAndPaste/html/LinksHTMLCopyAndPaste.spec.mjs:267:3 › HTML Links CopyAndPaste › Paste text into a link 

    Error: innerHTML of contenteditable in page did not match

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

    - Expected  - 9
    + Received  + 1

    @@ -4,16 +4,8 @@
        <a
          class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
          dir="ltr"
          href="https://"
          rel="noreferrer">
    -     <span data-lexical-text="true">Link</span>
    -   </a>
    -   <span data-lexical-text="true">normal text</span>
    -   <a
    -     class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
    -     dir="ltr"
    -     href="https://"
    -     rel="noreferrer">
    -     <span data-lexical-text="true">text</span>
    +     <span data-lexical-text="true">Link normal texttext</span>
        </a>
      </p>

It looks like this test was added in #5394

@keiseiTi
Copy link
Contributor Author

keiseiTi commented Aug 8, 2024

The core of this problem is whether the pasted content is only a text node type or another node type.

If it is a text node type, the parent node will merge the text node.

If it is another node type, the parent node will be split into two nodes.

Of course, other conditions must be considered here, such as the pasted content is of html type, and the content is another link type node, etc.

@etrepum
Copy link
Collaborator

etrepum commented Aug 8, 2024

The current test case puts text on the clipboard and pastes it into the link and expects the split node behavior. I don't have any knowledge of the decision making process here (or how other editors behave in this situation) but it the current behavior appears intentional

  test('Paste text into a link', async ({page, isPlainText}) => {
    test.skip(isPlainText);
    await focusEditor(page);

    await page.keyboard.type('Link text');
    await selectAll(page);
    await click(page, '.link');
    await click(page, '.link-confirm');
    await moveRight(page, 1);
    await moveLeft(page, 4);

    await pasteFromClipboard(page, {
      'text/html': 'normal text',
    });

    await assertHTML(
      page,
      html`
        <p
          class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
          dir="ltr">
          <a
            class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
            dir="ltr"
            href="https://"
            rel="noreferrer">
            <span data-lexical-text="true">Link</span>
          </a>
          <span data-lexical-text="true">normal text</span>
          <a
            class="PlaygroundEditorTheme__link PlaygroundEditorTheme__ltr"
            dir="ltr"
            href="https://"
            rel="noreferrer">
            <span data-lexical-text="true">text</span>
          </a>
        </p>
      `,
    );
  });

@2wheeh
Copy link
Contributor

2wheeh commented Aug 8, 2024

how other editors behave in this situation

FYI: Notion, Tiptap, Word works as in the current Lexical.

@keiseiTi
Copy link
Contributor Author

keiseiTi commented Aug 8, 2024

how other editors behave in this situation

FYI: Notion, Tiptap, Word works as in the current Lexical.

In notion, I tried to copy the text into the link node, and the link node also merged the copied text.

iShot_2024-08-09_00 12 30

@2wheeh
Copy link
Contributor

2wheeh commented Aug 8, 2024

In notion, I tried to copy the text into the link node, and the link node also merged the copied text.

you are right, It seemed unexpected bug in Notion. I cannot reproduce this:

2024-08-09.1.17.31.mov

@DanielMaass
Copy link

I would like to ask carefully. Is anyone still on this issue? The test failed 2 weeks ago. I am eagerly awaiting this fix.

@DanielMaass
Copy link

Is it possible that you review this issue and fix the failed e2e-test? I would appreciate it very much. @fantactuka @zurfyx @Fetz @ivailop7 @ivailop7 @acywatson @potatowagon @Sahejkm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Pasting text from the clipboard into a CustomElementNode does not work properly
5 participants