[HOLD for payment 2022-05-20] [$2000] When copy&paste multiple messages line breaks aren't maintained

[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:

1. Open app
2. Copy a message with multiple line breaks
3. Paste into another chat

Steps 2

1. Send 3 separate messages
2. Copy all 3 messages
3. Paste in compose box

Expected Result:

Line breaks should be maintained. The three messages would show on three lines

Actual Result:

Line breaks are not maintained. The three messages are on one line.

Workaround:

Unknown.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.30-3
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Related to #5142

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

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[mallenexpensify]

cc @kursat since you worked on #5142
In case you wanted to work on this

[sig5]

Proposal
The problem arises due to the fact that only comments that are multiline are wrapped inside the <comment> tag. So when we parse the DOM Nodes in SelectionScrapper, single-line messages aren't appended with the <br> tag.

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

Lines 65 to 77 in 8a31526

	? (
	<RenderHTML
	html={`<comment>${props.fragment.html + (props.fragment.isEdited ? '<edited></edited>' : '')}</comment>`}
	/>
	) : (
	<Text
	selectable={!canUseTouchScreen() || !props.isSmallScreenWidth}
	style={EmojiUtils.isSingleEmoji(props.fragment.text) ? styles.singleEmojiText : undefined}
	>
	{Str.htmlDecode(props.fragment.text)}
	{props.fragment.isEdited && (
	<Text
	fontSize={variables.fontSizeSmall}

There is a simple solution for the problem:

We can enclose the decoded HTML for the single line messages in a div with the data-testid passed explicitly as comment. The id gets attached to the DOM element and parsing is done accordingly.

{<div data-testid='comment'>{Str.htmlDecode(props.fragment.text)}</div>}

1.New.Expensify.Mozilla.Firefox.2022-01-24.16-25-41.mp4

The above approach works fine as shown.
I have a couple of questions:

1. What other purpose does data-testId serve? Because this selectionScrapper functionality seems to have been implemented recently, and I can't find the occurrence of this attribute elsewhere.
2. If there is indeed another purpose, then we should probably introduce a new custom attribute for handling this kind of scenario. The steps will remain more or less similar along with modifying the SelectionScrapper for handling the new attribute.

[thienlnam]

@kursat Hey Kursat! Just looping back to this issue #6732, could you comment some additional context where data-testID comes from and what significance it serves?

[roryabraham]

What other purpose does data-testId serve? Because this selectionScrapper functionality seems to have been implemented recently, and I can't find the occurrence of this attribute elsewhere.

I too am trying to figure out where this comes from. It seems like it might be an implementation detail from either react-native-render-html or react-native-web 🤔

If that's the case, I think we need to decouple the SelectionScraper implementation from the data-testid attribute and instead make it reliant upon something we set explicitly in our app. I am still trying to understand all the implementation details of the SelectionScraper library, but it seems like at a high level it:

1. Uses the Selection Web API and then, for each Range in the selection, finds the closest div tag with the data-testid attribute. Aggregate all those results under a single div.
   • This part seems like it's dependent upon low-level implementation details of react-native-web and is very unclear to me.
2. Takes the result of the previous step and parses it using htmlparser2
3. Takes the result of the previous step, and recursively (for the dom tree parsed in the previous step) transform any div tags with the data-testid attribute to the actual tag specified in the attribute.
   • There is a comment here that alludes to that, but unless I'm mistaken this transform actually happens in a separate function here.
   • After each time we include a custom <comment> tag, we insert a <br> here.
4. "Re-render" the transformed html tree using dom-serializer here
   • This doesn't actually render anything, but instead just simulates a render and returns the resultant html as a string.
5. Parse the html from the previous step to mark down using our own ExpensiMark library.

I have several thoughts about how this could all be simplified/cleaned up, but first I want to confirm that my high-level understanding of this is correct. @kursat Any comments?

[kadiealexander]

Posted to upwork: https://www.upwork.com/jobs/~0104b0a207be0dbbcb

[MelvinBot]

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

[MelvinBot]

Current assignee @roryabraham is eligible for the Exported assigner, not assigning anyone new.

[rushatgabhane]

We can enclose the decoded HTML for the single line messages in a div with the data-testid passed explicitly as comment. The id gets attached to the DOM element and parsing is done accordingly.

Hmm I think that'll lead to an extra new line at the end of copied text.
Copying a single line of text will have a new line, which isn't ideal.

[kursat]

Hey @thienlnam, data-testid is added by react-native-render-html.

@roryabraham all steps are correct.

@sig5's proposal looks like a workaround to me. So I am trying to find a better solution for this for a couple days. @roryabraham If I can be able to find a better solution, we can also implement your improvement ideas.

[rushatgabhane]

If I can be able to find a better solution, we can also implement your improvement ideas.

@kursat any updates on this one? Thanks!

[kursat]

Hey @rushatgabhane, I couldn't find enough time to find a proper solution yet. Please let me work some more this weekend. Hopefully, I will send a proposal on Monday.

[mallenexpensify]

@roryabraham should this job be doubled? I know @kursat said they're working on it but that shouldn't be a reason to not double because anyone can propose a solution and be hired

[roryabraham]

Yep, let's double this one.

[mallenexpensify]

Doubled price to $500 - https://www.upwork.com/jobs/~0104b0a207be0dbbcb

[rushatgabhane]

Thanks @ahmdshrif

C+ reviewed 🎀 👀 🎀

@roryabraham I think we should hire @ahmdshrif, we can then work together on all the test cases. Let me know what you think
@ahmdshrif's proposal looks good to me.

[roryabraham]

Okay. Honestly, I had some difficulty following the language used throughout this discussion, but the videos give me enough confidence to think that we're on the right track.

@roryabraham We might be compounding on a wrong path, so I'd love your opinion on @ahmdshrif's approach. It seems like we're gonna have too many cases to test, and debuging the code later on won't be easy.

I agree that there's a substantial burden on maintenance in our Markdown parser, but that predates my time at Expensify so I don't have all the context around why we decided to build our own Markdown parser. Generally I think the reason is that we wanted to have full control over the markdown syntax we choose to support not to support in our chat application.

[MelvinBot]

📣 @ahmdshrif You have been assigned to this job by @roryabraham!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[kadiealexander]

Have sent contracts to @ahmdshrif and @rushatgabhane to ensure they're paid when this is completed. Please accept when you can!

[ahmdshrif]

pr here @rushatgabhane @roryabraham

Expensify/expensify-common#436

[roryabraham]

Looks like the PR is almost ready. @rushatgabhane mentioned that he wants to review, so I'm waiting on the C+ 👍 before I do a final review.

[roryabraham]

PR review is still ongoing

[roryabraham]

Expensify-common PR was merged just a few minutes ago. Next up is the NewDot PR.

[mallenexpensify]

@roryabraham in case anything is weird with this issue, can you drop a comment in denoting payment date if one isn't auto-added?

[kadiealexander]

See update above! Begone, Melvin.

[kadiealexander]

@roryabraham do we need to issue payment for anything so far? I'm unclear on if this issue has anything unusual I need to manage.

[rushatgabhane]

@kadiealexander this issue should be setttled on May 20.
PR #8743 was deployed to production on May 13.

[kadiealexander]

Thanks @rushatgabhane!

[kadiealexander]

Paid!!! Thanks for your work team.

[rushatgabhane]

Cheers!