Replace double newlines with a single newline #13739

tgolen · 2022-12-20T21:18:16Z

Fixed Issues

Tests

Use the following text for the test (it has 4 newlines):

I'm around if you are and wanna hop on a call https://meet.google.com/yjx-maas-pku



Nothing specific in mind

Web/Desktop

Copy that text
Paste it normally into the chat composer using ctrl+v
Verify that it still contains the 4 newlines
Paste it without formatting into the chat composer user ctrl+shift+v
Verify that it still contains the 4 newlines and no more
Send the message
Copy the previous message by right-clicking the message and select copy it to clipboard
Repeat steps 2-5
Copy the message by clicking the copy to clipboard in the hover menu
Repeat steps 2-5
Copy the message by highlighting all the text of the comment, right-click the message > select copy to clipboard
Repeat steps 2-5
Copy text inside the composer input that contains newlines by using ctrl+c
Repeat steps 2-5
Copy text inside the composer input that contains newlines by using ctrl+x
Repeat steps 2-5

Native and mobile web

Native platforms do not have a paste without formatting feature, so just do basic regression testing to ensure pasting functions normally

Copy that text
Paste it normally into the chat composer
Verify that it still contains the 4 newlines

Verify that no errors appear in the JS console

Offline tests

n/a this feature doesn't change or is impacted by offline mode

QA Steps

Same as above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

2022-12-20_14-06-50.mp4

Mobile Web - Chrome

2022-12-20_14-08-19.mp4

Mobile Web - Safari

2022-12-20_14-11-50.mp4

Desktop

2022-12-20_14-10-15.mp4

iOS

2022-12-20_14-11-13.mp4

Android

2022-12-20_14-09-15.mp4

melvin-bot · 2022-12-20T21:18:50Z

@sobitneupane @robertjchen One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

luacmartins · 2022-12-20T21:51:11Z

src/components/Composer/index.js

@@ -265,7 +265,9 @@ class Composer extends React.Component {
            return;
        }

-        const plainText = event.clipboardData.getData('text/plain');
+        // The regex replace is there because when doing a "paste without formatting", there are extra line breaks added to the content (I do not know why). To fix the problem, anytime
+        // there is a double set of newline characters, they are replaced with a single newline. This preserves the same number of newlines between pasting with and without formatting.


Suggested change

// there is a double set of newline characters, they are replaced with a single newline. This preserves the same number of newlines between pasting with and without formatting.

// there is a double set of newline characters, they are replaced with a single newline. This preserves the same number of new lines between pasting with and without formatting.

sobitneupane · 2022-12-21T11:36:44Z

Testing ....

sobitneupane

If we select the text and copy it with Ctl+C, it works as expected. But if we copy the text with "Copy to Clipboard" option from context menu and paste with Ctrl+Shift+V, new lines get removed.

Steps to reproduce:

Right click on the sent message and select Copy to clipboard
Paste the message with Ctrl+Shift+V
Expected new lines get removed.

Screen.Recording.2022-12-21.at.20.34.23.mov

tgolen · 2022-12-28T16:32:16Z

Thanks for reporting that! I've updated this branch so that the context menu and CTRL+c will have consistent behaviors for browsers.

sobitneupane · 2022-12-29T18:17:05Z

@tgolen When I copy the text from Github and paste it with "Ctrl+Shift+V", newlines get removed.

Steps to Reproduce:

Copy the following text.

I'm around if you are and wanna hop on a call https://meet.google.com/yjx-maas-pku



Nothing specific in mind

Paste it with Ctrl+Shift+V in new expensify.

Screen.Recording.2022-12-29.at.23.37.05.mov

sobitneupane · 2022-12-29T18:28:05Z

Copy the text with Ctrl+C from expensify and paste with with "Ctrl + Shift + V", new lines get added.

Send the following text in new expensify.

I'm around if you are and wanna hop on a call https://meet.google.com/yjx-maas-pku



Nothing specific in mind

Select the text and copy it with Ctrl+C.
Paste the text with Ctrl + Shift +V

trimmed.mov

tgolen · 2022-12-29T18:52:13Z

When I copy the text from Github and paste it with "Ctrl+Shift+V", newlines get removed.

I might be wrong here, but I think I remember seeing a discussion about this and since GitHub formats its code blocks differently, we weren't going to bother with trying to preserve them perfectly. I'm having trouble finding the conversation, but do you feel that issue is in scope of the original GH?

Copy the text with Ctrl+C from expensify and paste with with "Ctrl + Shift + V", new lines get added.

I am not able to reproduce this one. Any pointers on what I'm doing differently?

2022-12-29_11-49-30.mp4

sobitneupane · 2022-12-30T05:16:10Z

So, I believe this issue is reproducible when the message contains link

Can you try sending something like

I'm around if you are and wanna hop on a call https://meet.google.com/yjx-maas-pku



Nothing specific in mind

tgolen · 2023-01-02T16:04:21Z

OK, I'll test it out with a link, but that is sure weird why a link would matter. I wonder if that's messing up a regex somewhere...

tgolen · 2023-01-02T17:24:23Z

All right, I tried it with the link, and I still can't reproduce it 🤔

This video shows me pasting twice. The first time is a normal paste with formatting, the second time is a paste without formatting. In both cases, the newlines are the same (you can see the link is treated a little differently though which is expected).

2023-01-02_09-22-31.mp4

sobitneupane · 2023-01-03T15:45:09Z

@tgolen I can still reproduce the issue on my end.

Steps to reproduce:

Send a message with links and newlines
Copy the message
Paste the message with Ctrl + Shift + V or Paste and Match Style on context menu.

Screen.Recording.2023-01-03.at.21.19.45.mov

tgolen · 2023-01-03T16:01:24Z

OK, I see in that video you are copying the message by using the context menu. That is different then from when you previously reported it, right? Can you verify if there is a difference between using the context menu and using CTRL+C to copy the message?

sobitneupane · 2023-01-03T16:14:25Z

Can you verify if there is a difference between using the context menu and using CTRL+C to copy the message?

There is no difference. Both context menu "Copy to clipboard" and CTRL+C has the same result.

Screen.Recording.2023-01-03.at.21.57.08.mov

tgolen · 2023-01-03T17:03:11Z

OK, since I am not able to reproduce it, are there any changes to the code you would suggest? Any idea why you can reproduce it but I can't?

tgolen · 2023-01-03T17:16:56Z

I just tested it again this morning and I can reproduce the problem 🤦 sorry about that! I'll keep digging

tgolen · 2023-01-03T17:28:33Z

All right, I found a different place to double the newlines for the context menu which seems to work better and fix the issue. Can you please test again? Thanks for your persistence on this!

sobitneupane · 2023-01-04T07:39:11Z

@tgolen It solves the above issue.

But with following steps, newlines are still getting added.

Send a message containing link.
Select the message.
From context menu, click on "Copy to clipboard"
Paste the message on Composer with CTRL + SHIFT + V or Select Paste and Match Style on context menu.

Screen.Recording.2023-01-04.at.13.18.34.mov

tgolen · 2023-01-05T22:50:58Z

OK, that is unfortunate. I will need to think about how to fix that 🤔

tgolen · 2023-01-06T00:35:11Z

All right, I pushed an update to fix what happens when you copy (or cut) stuff from inside the composer input

tgolen · 2023-01-06T00:35:23Z

I also added that to the list of tests

sobitneupane · 2023-01-06T15:08:58Z

Screenshots/Videos

Web

Screen.Recording.2023-01-06.at.20.14.09.mov

Mobile Web - Chrome

Screen.Recording.2023-01-06.at.21.01.46.mov

Mobile Web - Safari

Desktop

Screen.Recording.2023-01-06.at.20.21.28.mov

iOS

RPReplay_Final1673017424.MP4

Android

Screen.Recording.2023-01-06.at.20.38.07.mov

sobitneupane · 2023-01-06T15:20:11Z

@tgolen This issue still exists on mWeb both on android - Chrome and IOS-safari.

Screen.Recording.2023-01-06.at.21.01.46.mov

sobitneupane · 2023-01-06T15:26:15Z

@tgolen It looks like #13739 (comment) issue is not present in staging. But when copied from other sources(notes, docs) and pasted in app (and vice versa), there are inconsistencies in staging as well.

tgolen · 2023-01-06T20:05:46Z

@sobitneupane Thanks! I am going to go out on a limb and say that the mobile web issue is OK for now. I don't think that platform provides a very good UX for copy/pasting text within the composer.

sobitneupane

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

luacmartins

LGTM and tests well. Thanks for all the comments - it made reviewing the PR really easy and I'm sure it will help others who come across these lines too!

luacmartins · 2023-01-09T17:46:46Z

all yours @robertjchen!

robertjchen

Code/Comments look great and it tests well on my end as well, this is good to go! 👍

OSBotify · 2023-01-10T15:53:07Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-01-10T17:07:21Z

🚀 Deployed to staging by @robertjchen in version: 1.2.52-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-01-12T13:37:57Z

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mollfpr · 2023-02-16T14:44:24Z

Hi!! Coming from this issue #14243 where the PR is introducing the regression specifically on this code const plainText = Str.htmlDecode(parser.htmlToText(content)).replace(/\n/g, '\n\n'); —the explanation at here #14243 (comment).

Thank you!

sobitneupane · 2023-04-10T16:28:47Z

The PR introduces this regression.

Steps To Reproduce:

Highlight text with the cursor with few line breaks
Right click on the hightailed text --> click on the Copy icon at the Context menu
Paste result to the chat

The regression is now fixed by #16464

eVoloshchak · 2023-04-14T13:35:25Z

The same issue is happening when you copy highlighted text using ctrl/cmd+c, this has been resolved in #16473

s77rt · 2023-05-26T23:16:30Z

src/components/Composer/index.js

+        if (!event.metaKey || (event.key !== 'c' && event.key !== 'x')) {
+            return;
+        }


@tgolen Here we are only checking for the metaKey modifier which is only available on macOS. I'm not sure if this is intended but I think we should also check for ctrlKey to have same behaviour on other OS (Windows & Linux).

Sounds fine to me, but it's very low value since we don't officially support those platforms. Just my thoughts

Actually we are removing the whole putSelectionInClipboard function here #19738. We no longer need it as the issue that this function was meant to solve is already fixed by #15034 and the original issue #12769 is no longer reproducible.

Replace double newlines with a single newline

a95ddfc

tgolen self-assigned this Dec 20, 2022

tgolen requested a review from a team as a code owner December 20, 2022 21:18

melvin-bot bot requested review from robertjchen and sobitneupane and removed request for a team December 20, 2022 21:18

luacmartins reviewed Dec 20, 2022

View reviewed changes

Change "newlines" to "new lines"

de90a62

sobitneupane reviewed Dec 21, 2022

View reviewed changes

Add extra newlines when copying from context-menu

6b69596

tgolen requested review from sobitneupane and luacmartins December 29, 2022 15:33

Merge branch 'main' into tgolen-fix-paste-extra-lines

dba3cf8

Move the place where newlines are doubled

19b6546

Fix removal of newlines when copying inside the composer

cb94d2b

sobitneupane approved these changes Jan 9, 2023

View reviewed changes

luacmartins approved these changes Jan 9, 2023

View reviewed changes

robertjchen approved these changes Jan 10, 2023

View reviewed changes

robertjchen merged commit 6a43fc7 into main Jan 10, 2023

robertjchen deleted the tgolen-fix-paste-extra-lines branch January 10, 2023 15:45

OSBotify mentioned this pull request Jan 10, 2023

Deploy Checklist: New Expensify 2023-01-10 #14188

Closed

15 tasks

kbecciv mentioned this pull request Jan 11, 2023

Chat - Multiple lines are appearing if message is copied by right-click and select to clipboard #14219

Closed

3 tasks

melvin-bot bot mentioned this pull request Jan 12, 2023

[HOLD for payment 2023-01-19] [$250] Web - Chat - Additional lines appear when pasting as plain text message with links #12769

Closed

kbecciv mentioned this pull request Jan 12, 2023

[HOLD for payment 2023-02-22] [$4000] Desktop - Chat - 4 lines are not appearing pasting with "cmd+v and cmd+shift+v #14243

Closed

1 task

TMisiukiewicz mentioned this pull request Feb 10, 2023

Fix issues with newlines when copypasting #15034

Merged

52 tasks

allroundexperts mentioned this pull request Mar 13, 2023

[HOLD for payment 2023-04-05] [$2000] Web - Chat- Line breaks are removed after special copy/paste scenario #15925

Closed

6 tasks

sobitneupane mentioned this pull request Mar 24, 2023

Fix Line breaks are removed after special copy/paste scenario #16464

Merged

55 tasks

eVoloshchak mentioned this pull request Apr 14, 2023

[HOLD for payment 2023-04-12] [$1000] Some new lines are removed from multiline message #16473

Closed

6 tasks

s77rt reviewed May 26, 2023

View reviewed changes

s77rt mentioned this pull request May 26, 2023

[HOLD for payment 2023-06-08] [$1000] Copying HTML character entities from the composer and then pasting them in the composer converts them to a literal character i.e. > becomes >. #19456

Closed

6 tasks

	// there is a double set of newline characters, they are replaced with a single newline. This preserves the same number of newlines between pasting with and without formatting.
	// there is a double set of newline characters, they are replaced with a single newline. This preserves the same number of new lines between pasting with and without formatting.

Replace double newlines with a single newline #13739

Replace double newlines with a single newline #13739

Conversation

tgolen commented Dec 20, 2022 • edited Loading

Fixed Issues

Tests

Web/Desktop

Native and mobile web

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 20, 2022

luacmartins Dec 20, 2022

Choose a reason for hiding this comment

sobitneupane commented Dec 21, 2022

sobitneupane left a comment

Choose a reason for hiding this comment

tgolen commented Dec 28, 2022

sobitneupane commented Dec 29, 2022 • edited Loading

sobitneupane commented Dec 29, 2022 • edited Loading

tgolen commented Dec 29, 2022

sobitneupane commented Dec 30, 2022

tgolen commented Jan 2, 2023

tgolen commented Jan 2, 2023

sobitneupane commented Jan 3, 2023

tgolen commented Jan 3, 2023

sobitneupane commented Jan 3, 2023

tgolen commented Jan 3, 2023

tgolen commented Jan 3, 2023

tgolen commented Jan 3, 2023

sobitneupane commented Jan 4, 2023 • edited Loading

tgolen commented Jan 5, 2023

tgolen commented Jan 6, 2023

tgolen commented Jan 6, 2023

sobitneupane commented Jan 6, 2023 • edited Loading

Screenshots/Videos

sobitneupane commented Jan 6, 2023

sobitneupane commented Jan 6, 2023

tgolen commented Jan 6, 2023

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins commented Jan 9, 2023

robertjchen left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 10, 2023

OSBotify commented Jan 10, 2023

OSBotify commented Jan 12, 2023

mollfpr commented Feb 16, 2023

sobitneupane commented Apr 10, 2023

eVoloshchak commented Apr 14, 2023

s77rt May 26, 2023

Choose a reason for hiding this comment

tgolen May 29, 2023

Choose a reason for hiding this comment

s77rt May 29, 2023

Choose a reason for hiding this comment

tgolen commented Dec 20, 2022 •

edited

Loading

sobitneupane commented Dec 29, 2022 •

edited

Loading

sobitneupane commented Dec 29, 2022 •

edited

Loading

sobitneupane commented Jan 4, 2023 •

edited

Loading

sobitneupane commented Jan 6, 2023 •

edited

Loading