feat(lexical-playground): copy button for @lexical/code

[LuciNyan]

Description

A code copy button

closes #2392

https://user-images.githubusercontent.com/22126563/173842316-48347a07-9de7-4c4b-88ce-26f37e74337a.mov

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Jul 1, 2022 at 11:16AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Jul 1, 2022 at 11:16AM (UTC)

[zurfyx]

Thank you, no objections about the copy to clipboard button on the toolbar but imo this shouldn't live in the node, either as a utility function in the same package or into the client app. I'd argue that as-is this is a pretty generic plain text copy and should maybe just be a playground utility and part of the toolbar.

I wonder if other folks have different opinions on this.

[zignis]

The button should not be inserted inside the toolbar itself, as it would not allow copying the code in read only mode. The button should be placed within the code block node.

[LuciNyan]

Thank you, no objections about the copy to clipboard button on the toolbar but imo this shouldn't live in the node, either as a utility function in the same package or into the client app. I'd argue that as-is this is a pretty generic plain text copy and should maybe just be a playground utility and part of the toolbar.

Yes! You're right, and I now think it shouldn't live in the node too.

[LuciNyan]

The button should not be inserted inside the toolbar itself, as it would not allow copying the code in read only mode. The button should be placed within the code block node.

Hello @zurfyx! There are some other considerations here, and I'd like to know where you think it would be better to put the button

[zurfyx]

Good point from @HexM7, inside the code block it sounds like the most reasonable space but ideally, we want to decouple it from the code block implementation. Notice that we already decoupled lines and language and defer this to the app:

.PlaygroundEditorTheme__code:after {
  content: attr(data-highlight-language);
  ...
}

For the copy functionality we need something a bit more complex since we need to attach some JavaScript. You could build a plugin that reads for newly inserted code blocks and attaches the copy UI and functionality. You can leverage mutations for this.

In the future we may provide an easy way to modify basic Lexical nodes but it doesn't exist at this point.

[fantactuka]

It can use similar approach as table actions, which renders react component for dropdown menu, and then reposition it over editor on selection change: https://github.com/facebook/lexical/blob/main/packages/lexical-playground/src/plugins/TableActionMenuPlugin.tsx#L85-L103. For this case it might be better to reposition it on code block mouse enter/leave

[LuciNyan]

It can use similar approach as table actions, which renders react component for dropdown menu, and then reposition it over editor on selection change: https://github.com/facebook/lexical/blob/main/packages/lexical-playground/src/plugins/TableActionMenuPlugin.tsx#L85-L103. For this case it might be better to reposition it on code block mouse enter/leave

Thank you very much for your suggestion!

[LuciNyan]

Hello @fantactuka! Thank you very much. Your advice is very useful! I've rewritten this PR, but now I'm having some CI problems... Could you tell me what I should do to solve this problem? I tried re-trigger, but it still didn't work.

[fantactuka]

Hello @fantactuka! Thank you very much. Your advice is very useful! I've rewritten this PR, but now I'm having some CI problems... Could you tell me what I should do to solve this problem? I tried re-trigger, but it still didn't work.

I've noticed you have yarn.lock file, and CI seem to complain about npm install. I'd start with removing yarn file and rebasing to latest

[LuciNyan]

I've noticed you have yarn.lock file, and CI seem to complain about npm install. I'd start with removing yarn file and rebasing to latest

Thank you for the reminder!

[LuciNyan]

Could someone please take a look at the problem?

When I run the test "Can copy code, when click 'Copy' button" via debug-test-e2e:chromium, the test passes. But when I use test-e2e:chromium, the test fails, and it looks like the copy function (await navigator.clipboard.writeText(text)) is not working.

I don't know if this is a playwright problem or if there is something I can do to avoid the problem in non-debug mode.

[acywatson]

@LuciNyan - thanks for this contribution - I will take a look at the CI issue and see if we can get this merged.

[LuciNyan]

@acywatson Thank you for your response. I've tried a lot but I can't get through the tests in non-debug mode. I don't know what to do, because when I debug in debug mode, everything works. 😣

[LuciNyan]

Hello @fantactuka! This PR has been blocked by testing problem for a long time, but is now ready to be reviewed. Because navigator.clipboard.writeText(text) has not been working properly in playwright's headless mode, And I have tried a lot but have not found a good solution, so I switched to a more indirect way of testing. I'm wondering if you think it's possible to test this way too? 👀

[acywatson]

Hello @fantactuka! This PR has been blocked by testing problem for a long time, but is now ready to be reviewed. Because navigator.clipboard.writeText(text) has not been working properly in playwright's headless mode, And I have tried a lot but have not found a good solution, so I switched to a more indirect way of testing. I'm wondering if you think it's possible to test this way too? 👀

Hey @LuciNyan - sorry it took me so long to get back to this. I think the issue with your test is that you didn't grant write or read permissions for the clipboard, which is necessary for it to work. Not sure how it was working in debug mode, but here is a gist with a working test (I pulled your branch from your repo and made sure this test works there):

https://gist.github.com/acywatson/ad306bc750d2340cf01c325fb427bfb2

Can we try testing this way and see if it works? The permissions are granted on line 1 of the gist. Doing it this way actually give those permissions for the whole file, which is OK, but ideally we could scope it to just the single test via a describe block.

[LuciNyan]

Can we try testing this way and see if it works? The permissions are granted on line 1 of the gist. Doing it this way actually give those permissions for the whole file, which is OK, but ideally we could scope it to just the single test via a describe block.

GREAT!!! It works! Thank you Morty Watson. 😆

[LuciNyan]

emmm..., it looks like this (clipboard-read) doesn't work on firefox 😣. I would like to know what you think is the best way to test it. Or we can just fall back to a more indirect approach on firefox? @acywatson

[acywatson]

emmm..., it looks like this (clipboard-read) doesn't work on firefox 😣. I would like to know what you think is the best way to test it. Or we can just fall back to a more indirect approach on firefox? @acywatson

Ah, here's one thing you can try:

https://github.com/microsoft/playwright/issues/2011#issuecomment-1165849986

However, I was thinking more about this: why don't we just use the paste keyboard shortcut?

await keyDownCtrlOrMeta(page);
await page.keyboard.press('KeyV');
await keyUpCtrlOrMeta(page);

[LuciNyan]

However, I was thinking more about this: why don't we just use the paste keyboard shortcut?

It seems like there was some misunderstanding in our conversation. I used this approach(keyboard shortcut) at the beginning, but I ran into the problem I mentioned at the beginning of not being able to copy properly in headless mode, so I chose a more indirect approach. This way there is no need to apply additional clipboard-read permissions.

  // 1. keyboard shortcut. This way there is need to apply additional `clipboard-read` permissions.
  await keyDownCtrlOrMeta(page);
  await page.keyboard.press('KeyV');
  await keyUpCtrlOrMeta(page);

  // 2. This way there is no need to apply additional `clipboard-read` permissions.
  const copiedText = await evaluate(page, () => {
    const textRef = {current: null};

    navigator.clipboard._writeText = navigator.clipboard.writeText;
    navigator.clipboard.writeText = function (data) {
      textRef.current = data;
      this._writeText(data);
    };

    document.querySelector('button[aria-label=copy]').click();

    return textRef.current;
  });

  await pasteFromClipboard(page, {
    'text/plain': copiedText,
  });

[LuciNyan]

Ah, here's one thing you can try:

microsoft/playwright#2011 (comment)

Thanks for the tip! I will try it tomorrow .

[acywatson]

I ran into the problem I mentioned at the beginning of not being able to copy properly in headless mode

Weird. This works for me in both debug and headless mode (chromium), even without permissions:

  test.only('test keyboard copy paste', async ({page, isRichText}) => {
    await focusEditor(page);
    await page.keyboard.type('Hello');
    await selectAll(page);
    await keyDownCtrlOrMeta(page);
    await page.keyboard.press('KeyC');
    await keyUpCtrlOrMeta(page);
    await page.keyboard.press('Delete');
    await keyDownCtrlOrMeta(page);
    await page.keyboard.press('KeyV');
    await keyUpCtrlOrMeta(page);
    await assertHTML(
      page,
      html`<p class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr" dir="ltr">
       <span data-lexical-text="true">Hello</span>
      </p>`
    );
  });

I believe the only reason you need permissions is if you are programmatically reading from or writing to the clipboard via the readText and writeText clipboard APIs. So, you'll need the clipboard-write permission for your actual copy button to work, but pasting with Command + V should not require clipboard-read permission.

The only issue I see with the "indirect" approach is that the more we diverge from the actual user actions and the more we rely on workarounds like this, the more likely it is for an edge case to slip past us.

[LuciNyan]

I believe the only reason you need permissions is if you are programmatically reading from or writing to the clipboard via the readText and writeText clipboard APIs. So, you'll need the clipboard-write permission for your actual copy button to work, but pasting with Command + V should not require clipboard-read permission.

The only issue I see with the "indirect" approach is that the more we diverge from the actual user actions and the more we rely on workarounds like this, the more likely it is for an edge case to slip past us.

Yes, you are right, we only need to apply clipboard-write permission to be able to test in a chromium environment with Ctrl C V. But I still can't apply clipboard-write permission on firefox and webkit.

I agree with you about the indirect approach, and we should avoid it as much as possible. But it looks like on firefox and webkit we may have to go with a more indirect approach.

[acywatson]

Cool, nice work. Let's merge this and we can work on improving the tests later. Thanks so much!