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 unwanted additional spaces added around pasted text on Windows #33607

Merged
merged 4 commits into from
Aug 12, 2021

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jul 21, 2021

Description

When pasting text internally within the editor on Windows additional spaces are added to either side of the pasted text.

  • Copied text (i.e. what the user intended to copy) two.
  • Resulting text when pasted two.

This appears to be due to windows providing additional unwanted data when retrieving text/html from the clipboard event data. The actual content copied is:

<html>
 <body>
   <!--StartFragment-->
   Copied text here
   <!--EndFragment-->
 </body>
</html>

When this passes through the editor's paste mechanics and the format API we end up with spaces around the text.

To fix this, this PR attempts to use regex to remove:

  1. The two "Fragment" comments.
  2. Whitespace either side of the comments.
    It does not remove whitespace inside the "Fragment" comments because the user may have intentionally copied some spaces which we would want to preserve.

Fixes #33283

How has this been tested?

  • Use Windows 10 and latest MS Edge browser.
  • Create a new Post.
  • Write some text such as Hello I am some text. Let's copy me.
  • Using your mouse highlight a word in that text being sure not to copy any whitespace.
  • Paste the copied text into the middle of another word.
  • See that there are no spaces added either side of the text your pasted.
  • Repeat but this time copy some text that does include some whitespace either side. We need to check that in this scenario the whitespace is preserved.

Also test on Mac to ensure it doesn't regress this implementation.

Screenshots

Windows

Before

Screen.Capture.on.2021-07-21.at.17-05-19.mov

After

Screen.Capture.on.2021-07-22.at.11-59-51.mp4

Mac OS

The below shows only the after fix as the bug did not manifest itself on Mac even before this fix.

Screen.Capture.on.2021-07-22.at.11-58-39.mp4

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getdave getdave self-assigned this Jul 21, 2021
@getdave getdave changed the title Fix additional spaces added around pasted text on Windows Fix unwanted additional spaces added around pasted text on Windows Jul 21, 2021
@github-actions

This comment has been minimized.

@getdave getdave requested a review from ellatrix July 22, 2021 11:04
@getdave getdave added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended labels Jul 22, 2021
@getdave getdave marked this pull request as ready for review July 22, 2021 11:05
@getdave
Copy link
Contributor Author

getdave commented Jul 23, 2021

Ok @ellatrix it looks like this solves the reported Issue. If you have time to review my implementation that would be great.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Given that this is based on a documented OS specific issue that other editos encountered I say it's ✅ But we have a paste-handler that could handle this better than directly in Rich Text?

@getdave
Copy link
Contributor Author

getdave commented Jul 29, 2021

...we have a paste-handler that could handle this better than directly in Rich Text?

🤔 Yeh actually it might be better moved to there. Let me see...

@getdave

This comment has been minimized.

@getdave
Copy link
Contributor Author

getdave commented Jul 29, 2021

Ok it looks like we go down the isInternal code path when pasting within the editor. That doesn't invoke the paste handler which is why we need the additional fix in this PR.

if ( isInternal ) {
const pastedValue = create( {
html,
multilineTag,
multilineWrapperTags:
multilineTag === 'li' ? [ 'ul', 'ol' ] : undefined,
preserveWhiteSpace,
} );
addActiveFormats( pastedValue, value.activeFormats );
onChange( insert( value, pastedValue ) );
return;
}

@ellatrix I'd like to understand whether you agree that it's a good idea to strip the Windows specific fragment markers in all cases as I've done in this PR. If we don't do it here then it's never handled because whilst the pasteHandler does deal with the fragments in this "internal" paste scenario it is never invoked. As a result Windows exhibits this bug.

…larity

As we're not sure about this fix let's not create a public API via utils. Instead revert to inlining.

Also renaming the function to make it's purpose clearer. It's not a generic normalizing function so let's make it obvious this is Windows specific.
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I think this is a good fix and despite my doubts about the place where the fix is applied we can always refactor this later.

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

I tested it in Edge. Sometimes when you copy more than one words (with multiple spaces in between) and paste it somewhere, the copied texts are messed up, in a unpredictable way. Are you able to reproduce it?

@getdave
Copy link
Contributor Author

getdave commented Aug 10, 2021

I tested it in Edge. Sometimes when you copy more than one words (with multiple spaces in between) and paste it somewhere, the copied texts are messed up, in a unpredictable way. Are you able to reproduce it?

No I'm not.

Screen.Capture.on.2021-08-10.at.13-48-34.mov

Are you able to provide exact steps to reproduce?

@getdave getdave added the [Type] Regression Related to a regression in the latest release label Aug 10, 2021
@getdave
Copy link
Contributor Author

getdave commented Aug 10, 2021

@kevin940726 Let me know if can no longer replicate this. If you can't then I think we should look to merge the fix as it's been around for a while now and folk are waiting on the PR to land.

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

I can't reproduce the bug anymore, let's ship it 👍

@getdave getdave force-pushed the fix/additional-spaces-on-paste-on-windows branch from b4a86eb to 6a0db13 Compare August 12, 2021 11:12
@getdave getdave merged commit db73369 into trunk Aug 12, 2021
@getdave getdave deleted the fix/additional-spaces-on-paste-on-windows branch August 12, 2021 11:52
@github-actions github-actions bot added this to the Gutenberg 11.4 milestone Aug 12, 2021
@ellatrix
Copy link
Member

Could you please add an integration test?

@kevin940726
Copy link
Member

I believe the bug is only reproducible on Windows though, so not sure if we have the established testing environment yet.

@ellatrix
Copy link
Member

@kevin940726 A lot of the integration tests we have are for Windows. We just need a copy of the HTML that was inserted. See https://github.com/WordPress/gutenberg/tree/trunk/test/integration/fixtures/documents.

Please add an integration test because issues like this are easy to regress with refactors.

@kevin940726
Copy link
Member

@ellatrix Cool! Didn't know that! So it's more like a unit/integration test. For some reasons I was thinking e2e tests 😅. @getdave Care to create a follow-up PR? Or maybe someone else can do that.

@getdave
Copy link
Contributor Author

getdave commented Aug 23, 2021

Greetings 👋

Yes good point. I was thinking along same lines as Kai with the Windows specific nature of the issue, but Ella's information puts a different spin on it.

I'll put it on my list to add a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Paste [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in Gutenberg 11 around pasting with spaces?
4 participants