[Needs regression test steps] [$1000] When copy pasting message containing email/links, email address/link occupies new line.

[mvtglobally]

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:

Typed a message a@mail.com is my email. b@mail.com is someone else's email.
Copied and pasted in the text box below

Expected Result:

Text should copy in the same line

Actual Result:

When copy pasting message containing email/links, email address/link occupies new line.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.69-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @MonilBhavsar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1653548218883739

Job Post: https://www.upwork.com/jobs/~01e09832b6e53bb854

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Julesssss]

Reproduced, this should be fixed.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[luacmartins]

Hmm I wonder why I got assigned if @Julesssss is an ECM and is still assigned as well 🤔 @Julesssss should I keep myself assigned to this one?

[Julesssss]

Odd, I was OOO but I didn't think auto-assignment was smart enough to know that? Anyway, I'll keep it 👍

[parasharrajat]

You can do this. Please share your proposal.

[NehaSantani]

Proposals :

Hey,
There is a method in CopySelectionHelper.js file

 copySelectionToClipboard() {
      const selectionMarkdown = SelectionScraper.getAsMarkdown();
      Clipboard.setString(selectionMarkdown);
  }

const getAsMarkdown = () => {
    const selectionHtml = getHTMLOfSelection();
    console.log("getMarkdown",selectionHtml);

    const domRepresentation = parseDocument(selectionHtml);
    domRepresentation.children = _.map(domRepresentation.children, c => replaceNodes(c));

    const newHtml = render(domRepresentation);

    const parser = new ExpensiMark();

    return parser.htmlToMarkdown(newHtml);
};

Above method is converting the copied text to HTML then to markdown.

There are two options we can have to solve this issue:

1.) We can update the getHtmlOfSelection() for selected text which is being used while converting it into markdown as below

const getHTMLOfSelection=()=>{
   if (window.getSelection) {
       const selection = window.getSelection().toString();
       const parser = new ExpensiMark();
       return parser.replace(selection);  
    }

     // If browser doesn't support Selection API, returns empty string.
     return '';
  
}

since, parser.replace() method is already being used in while we are trying to edit a comment, we can use the same method for selection !

2.) We can set the selected text in clipboard as a plain text which is being selected by updating copySelectionToClipboard() method as below.

copySelectionToClipboard() {
      if(window.getSelection())
       Clipboard.setString(window.getSelection().toString());
   }

or by updating current getHTMLOfSelection() as returning

             return div.innerText;

instead of

            return div.innerHTML;

[parasharrajat]

Hey @NehaSantani, thanks for the proposal. Did you test this solution on local setup?

How will you convert the text to Markdown?

[NehaSantani]

Hi @parasharrajat,
The text is being converted into MarkDown in the similar manner like it is currently there using below method

index.js in /SelectionScraper.js

const getAsMarkdown = () => {
    const selectionHtml = getHTMLOfSelection();

    const domRepresentation = parseDocument(selectionHtml);
    domRepresentation.children = _.map(domRepresentation.children, c => replaceNodes(c));

    const newHtml = render(domRepresentation);

    const parser = new ExpensiMark();

    return parser.htmlToMarkdown(newHtml);
};

which inturn is called from below method

 copySelectionToClipboard() {
        const selectionMarkdown = SelectionScraper.getAsMarkdown();
        Clipboard.setString(selectionMarkdown);
    }

The whole conversion currently in the code is going like this
(selectedText -----> HTML ------> Markdown)

However, the issue is inside HTML conversion of text!
Actual Issue: The code done inside getHTMLOfSelection() method is returning HTML with some default style tags which is appending it to next line after converting into markdown.

The only change we need to do to solve the issue mentioned is inside getHTMLOfSelection() method which is called from getAsMarkdown() method.

Updated getHTMLofSelection().

index.js in /SelectionScaper.js

const getHTMLOfSelection = () => {
    if (window.getSelection()) {
    const selection = window.getSelection().toString();
    const parser = new ExpensiMark();
    return parser.replace(selection);  
 }  // If browser doesn't support Selection API, returns empty string.
 return '';
}

My proposal to use above code in getHTMLOfSelection() method is because we are already using parser.replace to convert it to html while editing a message!

Also,Yes i tested it in local.
attached is the Recording for the same.

L1Screen.Recording.2022-06-09.at.9.29.34.AM.mov

[parasharrajat]

Try this to copy chat between two messages. Start from bottom to the third last line(Hi Neha) of the previous message. Then paste it on composer and let me know what output you get.

[Zvukamar]

Proposal:

In src/components/Composer/index.js

Line 222:

Change this:
const pastedHTML = event.clipboardData.getData(TEXT_HTML);

To this:
const pastedHTML = event.clipboardData.getData(TEXT_HTML).replace(/div/g, 'span');

Explain:
event.clipboardData.getData(TEXT_HTML) returns <comment><div>abc</div><span>abc</span><div>abc</div</comment>

In render each <div> by default get style of display: "block" and catch all the row,
because of that we have \n in plain text
If we replace all <div> elements with <span> elements all works good.

Tested all the cases above, everything seems good.

[ahmdshrif]

@ahmdshrif you're assigned now. please feel free to raise PR.

I am working on it

@ahmdshrif please apply. Thanks!

@kevinksullivan done. Thanks all.

[aimane-chnaif]

Testing blocked on web, mWeb because of this known issue (403 auth error)

[trjExpensify]

Commented here, but you should be able to get unblocked here now after whitelisting. 👍

[aimane-chnaif]

ok, @ahmdshrif please fill out the form too so you could complete PR.

[ahmdshrif]

I fill out the form. and wait to add me to the whitelist.

[melvin-bot]

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

• fix: email new line by skipping extra div #13503

If no regressions arise, payment will be issued on 2022-12-27. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• 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

[melvin-bot]

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:

• [@aimane-chnaif / @Julesssss] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif / @Julesssss] 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:
• [@aimane-chnaif / @Julesssss] 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:
• [@kevinksullivan] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[aimane-chnaif]

I think @kevinksullivan is OOO till Jan 4.
@mallenexpensify can you please help with payment here?

[aimane-chnaif]

There was a few days delay until PR complete due to login issue on web.
Is this considerable in timeline calculation?

[mallenexpensify]

Gotcha covered @aimane-chnaif , @ahmdshrif too!
Thanks @aimane-chnaif for stepping in to get this over the finish line and for @ahmdshrif for being extra patient with us. Thanks to the other contributors who submitted proposals and commented too!

@aimane-chnaif I did a quick check of the timeline but didn't factor in down time with IP issues. I'm seeing

• Hired Dec 9th
• Merged 16th

I'm seeing this post on 12/13 about the IP issue, with the 10th and 11th being weekend days, it makes sense that y'all might be affected by the issue. @aimane-chnaif and/or @ahmdshrif, if you were affected, can you provide some details/links and reasoning in a post so we can have it for audit purposes? It doesn't have to be detailed and we only need one of you to do it. Thx

[aimane-chnaif]

@ahmdshrif created draft PR on Dec 10

• commented on Dec 10 that ip blocked and couldn't login
• commented on Dec 14 that still waiting for ip whitelist
  So at least 2 business days because of login issue
  cc @mallenexpensify

[aimane-chnaif]

IP issue happened before Dec 10 and I found out @ahmdshrif's comment on slack on Dec 10

[mallenexpensify]

Thanks for the breakdown @aimane-chnaif, I agree. I have issued 50% bonuses to you and @ahmdshrif .

Leaving open for regression test steps/BZ check list

[melvin-bot]

@Julesssss, @ahmdshrif, @kevinksullivan, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[kevinksullivan]

Still climbing through the OOO backlog, will knock this out tomorrow AM.