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

kbecciv · 2023-03-13T22:11:18Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

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

Expected Result:

The user should be able to use copy/paste without any issues

Actual Result:

Line breaks are removed from the copied text

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.83.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5976486_select_cursor.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team and @mdneyazahmad

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01728b823abb3cdacb
Upwork Job ID: 1635423253532684288
Last Price Increase: 2023-03-20

MelvinBot · 2023-03-13T22:11:23Z

Triggered auto assignment to @davidcardoza (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot · 2023-03-13T22:11:27Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

davidcardoza · 2023-03-13T23:30:50Z

reproducible on web, getting this assigned out.

MelvinBot · 2023-03-13T23:31:05Z

Job added to Upwork: https://www.upwork.com/jobs/~01728b823abb3cdacb

MelvinBot · 2023-03-13T23:31:09Z

Current assignee @davidcardoza is eligible for the External assigner, not assigning anyone new.

MelvinBot · 2023-03-13T23:31:18Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

MelvinBot · 2023-03-13T23:31:25Z

Triggered auto assignment to @grgia (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

allroundexperts · 2023-03-13T23:54:45Z

This seems to be a regression from this PR. We're removing the new lines explicitly from the selection here.

bernhardoj · 2023-03-14T02:04:28Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Line break is removed when we copy the selected text from context menu "Copy to clipboard".

What is the root cause of that problem?

When we copy a message from mouse selection, remove all \n with an empty string here.

App/src/pages/home/report/ReportActionItem.js

Lines 126 to 129 in 8baeab7

    
           // Newline characters need to be removed here because getCurrentSelection() returns html mixed with newlines, and when 
        
           // <br> tags are converted later to markdown, it creates duplicate newline characters. This means that when the content 
        
           // is pasted, there are extra newlines in the content that we want to avoid. 
        
           const selection = SelectionScraper.getCurrentSelection().replace(/\n/g, '');

If we look at the comment, the reason behind it because the HTML may contain   tag and new line at the same time which makes the new line double. However, that case only happens when we send a markdown message, for example:

_First line_
Second line

In this case, the html when we select it will be

<em>First line</em><br>\n
<span>Second line</span>

If we don't include markdown, the   tag won't be there. The reason the   tag is there because when we send a message, the newline will be converted with a   tag.

So, the   only present on a message with markdown, while new line always present.

What changes do you think we should make in order to solve the problem?

Instead of replacing the new line with empty string, we should replace the   tag instead with empty string.

Note: we can replace the   tag inside getCurrentSelection which will also fix another copying issue, but I will explain if it's needed. I'm afraid my proposal will be cluttered if I explain it here.

tienifr · 2023-03-14T04:56:10Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we use the "Copy to clipboard" function from the context menu to copy selected text, the line break is not included in the copied text.

What is the root cause of that problem?

What we are currently expecting is that every intentional line breaks are represented by a
tag, and any redundant \n character would be removed. That logic is currently be handled by the getCurrentSelection() function. The current logic of getCurrentSelection() is using the getHTMLOfSelection() to get the HTML representation of the copied text.

The HTML representation is then used to parse the HTML content. The thing is, for cases with markdown text, it is rendering correctly if a new line comes after the markdown, since the result of this function ...

Copied text:

_a_
*a*

Result of getHTMLOfSelection()

<div>
  <div class="css-view-175oi2r" data-testid="comment" style="align-items: flex-start; user-select: text;">
    <span class="css-textHasAncestor-1qaijid r-userSelect-1xnzce8" data-testid="em" style="font-size: 15px; color: rgb(231, 236, 233); font-family: ExpensifyNeue-Regular, &quot;Segoe UI Emoji&quot;, &quot;Noto Color Emoji&quot;; line-height: 20px; font-style: italic;">a</span>
    <span class="css-textHasAncestor-1qaijid" data-testid="br">
</span>
    <span class="css-textHasAncestor-1qaijid r-userSelect-1xnzce8" data-testid="strong" style="font-size: 15px; color: rgb(231, 236, 233); font-family: ExpensifyNeue-Regular, &quot;Segoe UI Emoji&quot;, &quot;Noto Color Emoji&quot;; line-height: 20px; font-weight: bold;">a</span>
  </div>
</div>

Notice they have elements between the lines. This is becase the app is rendering these tags within the content of the selected element.

Meanwhile, for simple text case, the current logic of getHTMLOfSelection() is not returning the right thing we want (not every intentional line breaks are represented by   tags.

Copied text:

a
a

Result of getHTMLOfSelection()

<div>
  <div dir="auto" class="css-text-1rynq56 r-userSelect-1xnzce8" style="color: rgb(231, 236, 233); font-size: 15px; text-align: left; font-family: ExpensifyNeue-Regular, &quot;Segoe UI Emoji&quot;, &quot;Noto Color Emoji&quot;; direction: ltr; line-height: 20px;">⁦a
a</div>
</div>

Notice the new line is not represeted with  , but a \n in the text content instead.

What changes do you think we should make in order to solve the problem?

We should check that if the selected element is a simple text, then we will replace the text \n with   tags. The logic to check if the selected item is a pure text node is already available here. For example, we could do this

            // This means "range.commonAncestorContainer" is a text node. We simply get its parent node.
            if (!node) {
                node = range.commonAncestorContainer.parentNode;
+                newFragment = document.createRange().createContextualFragment(clonedSelection.textContent.replace(/\n/g, '<br/>'))
            }

then, we can add the node with \n being replaced by   to the final result

-                node.appendChild(clonedSelection);
+                if (newFragment)
+                      node.appendChild(newFragment);
+                 else
+                      node.appendChild(clonedSelection);

Alternative

Alternatively, instead of replacing \n with  , we could split the text content by \n character, and iteratively add  dom elements between them, so that the result is similar to the markdown case result.

Result

Working well after the fix

Screen.Recording.2023-03-14.at.11.53.38.mov

hoangzinh · 2023-03-14T16:32:39Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Line break is removed when we copy the selected text from the context menu "Copy to clipboard".

What is the root cause of that problem?

As other people mentioned above, the root cause is from this line

App/src/pages/home/report/ReportActionItem.js

Lines 126 to 129 in 8baeab7

    
           // Newline characters need to be removed here because getCurrentSelection() returns html mixed with newlines, and when 
        
           // <br> tags are converted later to markdown, it creates duplicate newline characters. This means that when the content 
        
           // is pasted, there are extra newlines in the content that we want to avoid. 
        
           const selection = SelectionScraper.getCurrentSelection().replace(/\n/g, '');

According to the code comment, I think the current code is wrong. The author concerned about getCurrentSelection() will return mixed with   and newlines in markdown, and when we paste with Ctrl+Shift+V, it will auto replace
by a new line => duplicate new lines. So to fix this case, we should replace / \n with   will fix his concern.

Example:

a

b

the function getCurrentSelection() will return:

<pre><span>a</span><br>
<br>
<span>b</span></pre>

What changes do you think we should make in order to solve the problem?

-                const selection = SelectionScraper.getCurrentSelection().replace(/\n/g, ''); 
+                const selection = SelectionScraper.getCurrentSelection().replace(/<br>\n/g, '<br>');

MelvinBot · 2023-03-14T16:32:44Z

📣 @hoangzinh! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

MelvinBot · 2023-03-14T16:36:35Z

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

sobitneupane · 2023-03-17T07:07:33Z

Thanks for the proposal @bernhardoj .

Please go through the issue for which the line you are pointing out is introduced. Your proposed change will recreate #12769.

sobitneupane · 2023-03-17T07:31:16Z

Thanks for the proposal @tienifr and @hoangzinh .

Looks like both the proposal will solve the issue.

Proposal from @hoangzinh looks interesting. This line was introduced to remove double lines when   and \n appears together. And @hoangzinh is proposing to replace  \n with   instead of \n with '' which looks convincing.

Proposal from @hoangzinh looks good to me. @hoangzinh Please make sure that your changes will not reproduce #12769 and #14243 issue. Also please make sure to test your PR on test steps in #13739 PR.

cc: @grgia

🎀👀🎀 C+ reviewed

bernhardoj · 2023-03-17T08:13:43Z

@sobitneupane I can't reproduce the issue you mentioned. Here is my reproduction attempt video

Screen.Recording.2023-03-17.at.01.22.39.mov

tienifr · 2023-03-17T09:33:58Z

I think my proposal has more consistency, and will not only address the root cause but also prevent similar unexpected errors in the future. In my solution, all expected new line will be treated as   tags, and unexpected new lines are represented as \n characters. Then, the \n characters are removed.

MelvinBot · 2023-03-29T17:26:46Z

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot · 2023-03-29T17:26:49Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.91-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

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

If no regressions arise, payment will be issued on 2023-04-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

MelvinBot · 2023-03-29T17:26:51Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@sobitneupane / @grgia] The PR that introduced the bug has been identified. Link to the PR:
[@sobitneupane / @grgia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@sobitneupane / @grgia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[] Determine if we should create a regression test for this bug.
[@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

hoangzinh · 2023-04-06T13:31:52Z

will the payment be issued today/this week?

grgia · 2023-04-10T09:19:02Z

@sobitneupane would you mind filling out the BZ checklist? Otherwise ping me if you need to offload it

grgia · 2023-04-10T09:19:32Z

@davidcardoza mind checking on the payments for this one?

sobitneupane · 2023-04-10T16:37:05Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

The PR that introduced the bug has been identified. Link to the PR:

#13739

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#13739 (comment)

A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I don't think any other changes are required in reviewer checklist. It was missed case while reviewing the issue. It should have been caught by reviewer(me) in the PR.

davidcardoza · 2023-04-12T03:15:17Z

@hoangzinh Contract sent via Upwork

grgia · 2023-04-14T14:05:09Z

Not overdue, awaiting payment (?)

davidcardoza · 2023-04-14T14:25:08Z

Payment was sent, we can close.

sobitneupane · 2023-04-14T16:34:48Z

@davidcardoza I don't think I have received the payment yet.

mdneyazahmad · 2023-04-14T16:37:31Z

@davidcardoza am I not eligible for reporting issue?

davidcardoza · 2023-04-18T16:36:33Z

@sobitneupane @mdneyazahmad Contracts sent

sobitneupane · 2023-04-18T16:38:30Z

Accepted.

mdneyazahmad · 2023-04-18T16:51:44Z

Accepted

hoangzinh · 2023-04-18T17:24:56Z

Accepted

kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 13, 2023

melvin-bot bot locked and limited conversation to collaborators Mar 13, 2023

melvin-bot bot assigned davidcardoza Mar 13, 2023

davidcardoza added the External Added to denote the issue can be worked on by a contributor label Mar 13, 2023

melvin-bot bot unlocked this conversation Mar 13, 2023

melvin-bot bot changed the title ~~Web - Chat- Line breaks are removed after special copy/paste scenario~~ [$1000] Web - Chat- Line breaks are removed after special copy/paste scenario Mar 13, 2023

melvin-bot bot assigned sobitneupane Mar 13, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 13, 2023

melvin-bot bot assigned grgia Mar 13, 2023

This comment was marked as off-topic.

Sign in to view

melvin-bot bot added the Overdue label Mar 16, 2023

melvin-bot bot removed the Overdue label Mar 17, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Mar 29, 2023

melvin-bot bot changed the title ~~[$2000] Web - Chat- Line breaks are removed after special copy/paste scenario~~ [HOLD for payment 2023-04-05] [$2000] Web - Chat- Line breaks are removed after special copy/paste scenario Mar 29, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Mar 29, 2023

bernhardoj mentioned this issue Apr 4, 2023

[$1000] Getting errors on other workspaces too if you get an error in a single workspace #16830

Closed

6 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 5, 2023

melvin-bot bot added the Overdue label Apr 10, 2023

melvin-bot bot removed the Overdue label Apr 10, 2023

sobitneupane mentioned this issue Apr 10, 2023

Replace double newlines with a single newline #13739

Merged

53 tasks

melvin-bot bot added the Overdue label Apr 14, 2023

melvin-bot bot removed the Overdue label Apr 14, 2023

davidcardoza closed this as completed Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kbecciv commented Mar 13, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

davidcardoza commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

allroundexperts commented Mar 13, 2023

bernhardoj commented Mar 14, 2023 •

edited

Loading

tienifr commented Mar 14, 2023 •

edited

Loading

hoangzinh commented Mar 14, 2023

MelvinBot commented Mar 14, 2023

This comment was marked as off-topic.

MelvinBot commented Mar 14, 2023

sobitneupane commented Mar 17, 2023

sobitneupane commented Mar 17, 2023

bernhardoj commented Mar 17, 2023 •

edited

Loading

tienifr commented Mar 17, 2023 •

edited

Loading

MelvinBot commented Mar 29, 2023

MelvinBot commented Mar 29, 2023

MelvinBot commented Mar 29, 2023 •

edited by grgia

Loading

hoangzinh commented Apr 6, 2023

grgia commented Apr 10, 2023

grgia commented Apr 10, 2023

sobitneupane commented Apr 10, 2023

davidcardoza commented Apr 12, 2023

grgia commented Apr 14, 2023

davidcardoza commented Apr 14, 2023

sobitneupane commented Apr 14, 2023

mdneyazahmad commented Apr 14, 2023

davidcardoza commented Apr 18, 2023

sobitneupane commented Apr 18, 2023

mdneyazahmad commented Apr 18, 2023

hoangzinh commented Apr 18, 2023

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

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

Comments

kbecciv commented Mar 13, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

Bug0 Triage Checklist (Main S/O)

davidcardoza commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

MelvinBot commented Mar 13, 2023

allroundexperts commented Mar 13, 2023

bernhardoj commented Mar 14, 2023 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

tienifr commented Mar 14, 2023 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Alternative

Result

hoangzinh commented Mar 14, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

MelvinBot commented Mar 14, 2023

This comment was marked as off-topic.

MelvinBot commented Mar 14, 2023

sobitneupane commented Mar 17, 2023

sobitneupane commented Mar 17, 2023

bernhardoj commented Mar 17, 2023 • edited Loading

tienifr commented Mar 17, 2023 • edited Loading

MelvinBot commented Mar 29, 2023

MelvinBot commented Mar 29, 2023

MelvinBot commented Mar 29, 2023 • edited by grgia Loading

hoangzinh commented Apr 6, 2023

grgia commented Apr 10, 2023

grgia commented Apr 10, 2023

sobitneupane commented Apr 10, 2023

davidcardoza commented Apr 12, 2023

grgia commented Apr 14, 2023

davidcardoza commented Apr 14, 2023

sobitneupane commented Apr 14, 2023

mdneyazahmad commented Apr 14, 2023

davidcardoza commented Apr 18, 2023

sobitneupane commented Apr 18, 2023

mdneyazahmad commented Apr 18, 2023

hoangzinh commented Apr 18, 2023

kbecciv commented Mar 13, 2023 •

edited by melvin-bot bot

Loading

bernhardoj commented Mar 14, 2023 •

edited

Loading

tienifr commented Mar 14, 2023 •

edited

Loading

bernhardoj commented Mar 17, 2023 •

edited

Loading

tienifr commented Mar 17, 2023 •

edited

Loading

MelvinBot commented Mar 29, 2023 •

edited by grgia

Loading