[HOLD for payment 2023-06-01] [$4000] Paste H1(# test) message in chat causes incorrect line breaks

[kavimuru]

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. Navigate to chat in Chrome and Safari
2. Open an existing chat and compose a H1 message (e.g., # hello world)
3. Send the message
4. Hover over the message and click on the edit pencil
5. Edit the H1 message (e.g., # hello world1234)
6. Click Save Changes
7. Hover over the edited message
8. Triple-click on the edited H1 message to highlight it
9. Copy/paste in the composer
10. Observe the pasted H1 message is formatted with differently

Expected Result:

Copying H1 message should not add a new line after #, and it should be the same as the original message

Actual Result:

Copying H1 message using triple click is adding a new line after #

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.94-0
Reproducible in staging?: y
Reproducible in production?: y
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

Recording.131.mp4

h1-tag-bug.mov

Expensify/Expensify Issue URL:
Issue reported by: @jayeshmangwani
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680592375624629

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015e2698490538f13d
• Upwork Job ID: 1643345155600601088
• Last Price Increase: 2023-05-02

[MelvinBot]

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

[MelvinBot]

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

[Christinadobrzyn]

I can replicate this and I think it's good to go external. Adding External label

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~015e2698490538f13d

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[alexxxwork]

Proposal

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

When we copy h1 text manually (Ctrl+C) and paste it to composer, the formatting/markdown is lost.

What is the root cause of that problem?

The problem is similar to #16525 but not the same.
We also get HTML in copied string

App/src/libs/SelectionScraper/index.js

Lines 133 to 139 in 2f922c4

	const getCurrentSelection = () => {
	const domRepresentation = parseDocument(getHTMLOfSelection());
	domRepresentation.children = _.map(domRepresentation.children, replaceNodes);
	const newHtml = render(domRepresentation);
	return newHtml || '';
	};

But in this case we use BaseHTMLEngineProvider and provide custom styling so it generates <h1> with a <div> inside

App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Lines 69 to 76 in 669970b

	<TRenderEngineProvider
	customHTMLElementModels={customHTMLElementModels}
	baseStyle={styles.webViewStyles.baseFontStyle}
	tagsStyles={styles.webViewStyles.tagStyles}
	enableCSSInlineProcessing={false}
	systemFonts={_.values(fontFamily)}
	fallbackFonts={fallbackFonts}
	>

When we use Ctrl-C CopySelectionHelper calls Clipboard.setString(parser.htmlToMarkdown(selection)); and it uses rules in ExpensiMark to generate Markdown from HTML. It ends up generating [block] instead of <div> which later generates additional \n in replace rules.

App/src/components/CopySelectionHelper.js

Lines 29 to 41 in 669970b

	copySelectionToClipboard() {
	const selection = SelectionScraper.getCurrentSelection();
	if (!selection) {
	return;
	}
	const parser = new ExpensiMark();
	if (!Clipboard.canSetHtml()) {
	Clipboard.setString(parser.htmlToMarkdown(selection));
	return;
	}
	Clipboard.setHtml(selection, Str.htmlDecode(parser.htmlToText(selection)));
	}

https://github.com/Expensify/expensify-common/blob/9cf047b9741d3c02820d515dccb9e0a45b6595f5/lib/ExpensiMark.js#L524

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

To avoid this we should get rid of additional empty <div> in <h1> tag. To do so we could add a pre option in 'heading1' rule of htmlToMarkdownRules in Expensimark:
https://github.com/Expensify/expensify-common/blob/9cf047b9741d3c02820d515dccb9e0a45b6595f5/lib/ExpensiMark.js#L231-L235

pre: inputString => inputString.replace(/<(h1)><div>(.*)<\/div>(<\/\1>)/gi,'<$1>$2$3'),

[Arslaan0124]

There's more to it. If you skip the edit part and just copy the message, it copies the emojis too.
Here's a loom: https://www.loom.com/share/43b14497131c4e5f9e76c033d005038b

[NikkiWines]

@Arslaan0124 the issue with emojis is a known bug and is being handled in this issue

[tienifr]

Proposal

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

Copying H1 message using triple click is adding a new line after #

What is the root cause of that problem?

When we triple click and Ctrl C, the text that's copied is this <comment><h1><div>hello world</div></h1></comment> (I'm ignoring the copying of the emojis here since it's part of another issue as mentioned here)

Why is there the extra div while we're only copying the h1? It's because the BaseHTMLEngineProvider that we use will add the div for styling purpose

App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Line 60 in 669970b

const BaseHTMLEngineProvider = (props) => {

.

The div will subsequently be converted to a new line, which leads to this issue.

So the root cause here is the extra div that's only for styling purpose, but is still added to the copied html.

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

We should ignore the div that's used for styling purpose when we copy the html.

The div that's relevant to us will have the data-testid which indicates which html element it's supposed to represent (for example strong, blockquote, h1), or the div is used to group many html elements, in which case it will have more than 1 child.

So we can ignore that div that:

• Has only 1 child
• Does not have data-testid

It turns out we're already doing very close to that here

App/src/libs/SelectionScraper/index.js

Line 105 in 3abe7b2

} else if (dom.name === 'div' && dom.children.length === 1 && dom.children[0].type !== 'text') {

, but we're keeping div that has 1 single text child, which leads to this issue.

To fix this we just have to remove the && dom.children[0].type !== 'text' in the condition

App/src/libs/SelectionScraper/index.js

Line 105 in 3abe7b2

} else if (dom.name === 'div' && dom.children.length === 1 && dom.children[0].type !== 'text') {

.

Updates from here
I've tested all possible markdown cases that we have in the app and there's no case of a div being used to represent a new line, and the approach of removing the div doesn't cause any regression. The cases that I've tested are: heading, inline quote, code block, bold, italic, strike-through, normal text, normal multiline text, link, email, auto link, auto email, empty line.

Feel free to double check thoroughly on your end.

My take is: Any text-wrapping div/HTML element that is relevant to us will have the data-testid that represents the elements (h1, a, em, strong, ...). If it doesn't have it, it's created by react-native-render-html for styling purpose and should be stripped.

If we want to limit that logic to inside our markdown tags only (the tags that have data-testid) we can also do that by passing a new field to the replaceNodes to indicate we're replacing inside our markdown tags. But it might not be necessary now since there's no such case.

What alternative solutions did you explore? (Optional)

• FYI The solution to replace the additional div in the htmlToMarkdown step will only fix the issue with the h1 tag, the bug with the div might happen for other tags as well. Also if we copy the html to other non-Expensify places then the redundant div will potentially cause other issues, I think we should keep in the Clipboard only the tags that are relevant.

[MelvinBot]

@NikkiWines, @Christinadobrzyn, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

[Christinadobrzyn]

I think we're still collecting proposals

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Christinadobrzyn]

Hey @0xmiroslav would you be able to review the provided proposals to see if they will work or if we need to seek others? Thanks!

[Christinadobrzyn]

@0xmiroslav just checking in to see if you have an update on the proposal review!

[0xmiros]

Proposals are in review. Will post update today

[tienifr]

@rzvc I don’t think having ‘div’ inside ‘h1’ is a common (or recommended) case in other web pages, and we shouldn’t have to patch our code based on other sites’ malformed HTML

[rzvc]

@0xmiroslav That is not the case. The proof of concept was a way for me to understand and explore the problem, the actual solution would be/look a lot simpler. (edit: maybe not a lot, IDK, didn't write it yet)

Also, I don't think it's particularly complex even as it is, it's an "unwrapping" implementation, which is something that's often necessary when simplifying HTML.

@tienifr DIVs inside H1s are perfectly valid HTML, and also used quite often (to convey subtext usually). So given it's standard HTML, I don't think the expectation of handling VALID input from other sources is outlandish. Choosing to modify existing HTML at copy time, in order to hide this issue is questionable at best tho.

You can look at this issue from any angle you want, but my solution is still the only correct one. As I said tho, it's not my decision, but I felt compelled to point out what it's otherwise obvious to me.

[tienifr]

@rzvc please see this thread. div inside h1 is poor web design https://www.quora.com/Is-the-usage-of-DIV-within-an-H1-tag-bad-for-SEO-If-yes-why. Although it won’t throw an error, it’s bad practice from a HTML standpoint , so I doubt anyone would intentionally design the HTML that way, unless it’s a constraint of a library/other issues.

[rzvc]

@tienifr so then you agree that it's not malformed as you claimed, and in fact standard.

[tienifr]

@NikkiWines if you and @0xmiroslav are aligned here, shall I start with the PR or wait until assignment?

Thanks!

[NikkiWines]

Oops, yes sorry - meant to assign you earlier @tienifr. Updated!

[Christinadobrzyn]

Hired in Upwork - External job posting here - https://www.upwork.com/jobs/~015e2698490538f13d

• @0xmiroslav as C+
• @tienifr as contributor
• @jayeshmangwani at reporter

[melvin-bot]

@NikkiWines, @Christinadobrzyn, @0xmiroslav, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Christinadobrzyn]

working on the PR - #19030

[melvin-bot]

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

[melvin-bot]

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

• Fix paste heading message in chat causes incorrect line breaks #19030

If no regressions arise, payment will be issued on 2023-06-01. 🎊

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

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

• [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav] 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:
• [@0xmiroslav] 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:
• [@0xmiroslav] Determine if we should create a regression test for this bug.
• [@0xmiroslav] 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.
• [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/288384

[Christinadobrzyn]

No regressions so it looks like we're at pay day! 🎉
Speed bonus applies - PR created May 16th and merged May 18th.

Paid jobs in upwork - #16921 (comment)
Closed Upwork job

@0xmiroslav can you let me know if you think a regression test is needed for this? I assume yes?

[0xmiros]

It's tough to find out offending PR which caused regression. I think the issue always existed after implement copy selection scrapper and heading1 markdown.

Regression Test Proposal

1. Open any chat room on web/desktop
2. Send new H1 message (i.e. # hello world)
3. Edit this message keeping H1 style (i.e. # hello world1234)
4. Click Save Changes
5. Triple-click on the edited H1 message to highlight it
6. Copy/paste in the composer
7. Verify that the pasted H1 message doesn't add a new line after #, and it's the same as the original message

[Christinadobrzyn]

Thanks @0xmiroslav I'll send this over to QA and note your research on the offending PR just in case they need anything else.

Closing this out!