Skip to content

[lexical-clipboard] Preventing copying empty string#7880

Merged
etrepum merged 20 commits intofacebook:mainfrom
niikkhilsharma:copy-empty-string
Sep 30, 2025
Merged

[lexical-clipboard] Preventing copying empty string#7880
etrepum merged 20 commits intofacebook:mainfrom
niikkhilsharma:copy-empty-string

Conversation

@niikkhilsharma
Copy link
Copy Markdown
Contributor

@niikkhilsharma niikkhilsharma commented Sep 27, 2025

Description

  • The current behavior allows copying an empty string to the clipboard when the cursor is placed inside the editor when no text is selected.
  • This PR modifies the clipboard copy logic to also check if the selection is collapsed, preventing the clipboard from being overwritten with an empty string.
  • The condition now ensures that if the selection is null or collapsed, the copy action is skipped.

Closes #7862

Test plan

Before

  • Copying with no selection would copy an empty string to the clipboard.
before.1.mov

After

  • Copying with no selection does not update the clipboard.
  • Verified manually to ensure clipboard remains unchanged when their is no text to copy.
after.1.mov

@meta-cla meta-cla 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 27, 2025
@vercel
Copy link
Copy Markdown

vercel bot commented Sep 27, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
lexical Ready Ready Preview Comment Sep 30, 2025 1:39pm
lexical-playground Ready Ready Preview Comment Sep 30, 2025 1:39pm

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Sep 27, 2025
etrepum
etrepum reviewed Sep 28, 2025
View reviewed changes
etrepum
etrepum reviewed Sep 28, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the right approach, if combined with the code that already checks the selection. It would be very nice to have a test that shows that this works

etrepum
etrepum reviewed Sep 28, 2025
View reviewed changes
etrepum
etrepum previously approved these changes Sep 28, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good assuming the existing tests pass, but the one thing it's missing is a test to show that it works since no existing test expects this new behavior.

@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 28, 2025

Yes, i'm working on writing tests for this new behaviour

@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 28, 2025 •
edited
Loading

@etrepum
`
test('Copy and paste empty string', async ({isRichText, page}) => {
await focusEditor(page);

await assertHTML(
  page,
  html`
    <p class="PlaygroundEditorTheme__paragraph" dir="auto"><br /></p>
  `,
);

// Set clipboard to some initial content to verify it doesn't get overwritten
await withExclusiveClipboardAccess(async () => {
  await page.evaluate(() =>
    navigator.clipboard.writeText('initial content'),
  );
});

await withExclusiveClipboardAccess(async () => {
  await copyToClipboard(page);
});

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

// Check editor content after paste, making sure it only contains the pasted content
if (isRichText) {
  await assertHTML(
    page,
    html`
      <p class="PlaygroundEditorTheme__paragraph" dir="auto">
        <span data-lexical-text="true">test content</span>
      </p>
    `,
  );
} else {
  await assertHTML(
    page,
    html`
      <p class="PlaygroundEditorTheme__paragraph" dir="auto">
        <span data-lexical-text="true">test content</span>
      </p>
    `,
  );
}

});`

i was writing this, but this required write permission, should I add the write permission here in the respective file.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 28, 2025

You can select something on the page and copy it which wouldn't require write permission. This works in more browsers so it's a better test.

@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 28, 2025

We want to implement no-copying behavior only when selecting an empty string.
Selecting something won't work. I need a way to test that my clipboard content has not been over-written after performing copy on an empty editor

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 28, 2025

Yes. Select something, copy it. Now the clipboard has something. Then collapse the selection and copy again. Confirm that the clipboard still has the original content.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 28, 2025

Another sequence would be:

  • Select something
  • Cut (which copies it to the clipboard, removes it, and collapses the selection)
  • Confirm that the content was removed
  • Copy (the selection is collapsed so this should do nothing)
  • Paste
  • Confirm that the content has been added back

@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 28, 2025

Sure, Thank you

@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 29, 2025

@etrepum can you please see why these tests are failing

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 30, 2025

The tests are failing in webkit and firefox because you wrote them in a way that only works in chromium. It's failing at await context.grantPermissions(['clipboard-read', 'clipboard-write']); you can look at the test failures, by clicking on the failed check in the github PR UI e.g. https://github.com/facebook/lexical/actions/runs/18090320132/job/51469283931?pr=7880

Can be reproduced locally as well, here's how you would run that test directly (with npm run start in another tab):

npm run test-e2e-firefox packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/CopyAndPaste.spec.mjs:967

Current output:

   Error: browserContext.grantPermissions: Unknown permission: clipboard-read

      970 |     context,
      971 |   }) => {
    > 972 |     await context.grantPermissions(['clipboard-read', 'clipboard-write']);
          |                   ^
      973 |     await focusEditor(page);
      974 |
      975 |     await page.keyboard.type('Hello world');
        at /Users/bob/src/lexical/packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/CopyAndPaste.spec.mjs:972:19

    attachment #1: video (video/webm) ──────────────────────────────────────────────────────────────
    test-results/e2e-CopyAndPaste-lexical-C-aa88b-lection-preserves-clipboard-firefox-retry1/video.webm
    ────────────────────────────────────────────────────────────────────────────────────────────────

  1 failed
    [firefox] › packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/CopyAndPaste.spec.mjs:967:3 › CopyAndPaste › Cut then copy empty selection preserves clipboard

I've just pushed a commit that fixes the test to work correctly without using any chrome specific code

@etrepum etrepum changed the title preventing copying empty string [lexical][lexical-clipboard] Preventing copying empty string Sep 30, 2025
@etrepum etrepum changed the title [lexical][lexical-clipboard] Preventing copying empty string [lexical-clipboard] Preventing copying empty string Sep 30, 2025
@niikkhilsharma
Copy link
Copy Markdown
Contributor Author

niikkhilsharma commented Sep 30, 2025 •
edited
Loading

Yes, it's working now
Thank you

@etrepum etrepum added this pull request to the merge queue Sep 30, 2025
Merged via the queue into facebook:main with commit b1ff639 Sep 30, 2025
39 checks passed
@etrepum etrepum mentioned this pull request Oct 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@etrepum etrepum etrepum approved these changes

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

@Fetz Fetz Awaiting requested review from Fetz

@ivailop7 ivailop7 Awaiting requested review from ivailop7 ivailop7 is a code owner

@Sahejkm Sahejkm Awaiting requested review from Sahejkm

@potatowagon potatowagon Awaiting requested review from potatowagon potatowagon is a code owner

@takuyakanbr takuyakanbr Awaiting requested review from takuyakanbr takuyakanbr is a code owner

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: should not be able to copy empty string

2 participants

@niikkhilsharma @etrepum