[$250] BUG: An extra blank line is added before and after a message with block quote is edited reported by @huzaifa-99

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Create a message with block quote (Send the message)
2. Try editing the message after it is sent.

Expected Result:

There should be no blank lines before/after the block quote message (in edit mode).

Actual Result:

There are extra blank lines before/after the block quote message (in edit mode).

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.19-1 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:

https://user-images.githubusercontent.com/43996225/197796218-1f81dc9a-18ca-45f8-a7bd-f34ec6f678a2.mp4 https://user-images.githubusercontent.com/43996225/197796261-28430ce3-93ca-4d49-acbf-c46612507ea2.mp4

Expensify/Expensify Issue URL: Issue reported by: @huzaifa-99 Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666647074971199

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @conorpendergrast (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[conorpendergrast]

This is not a duplicate, is reproducible, is a complete write-up, was created by a QA tester, no Slack conversation about it. I think this is External.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[conorpendergrast]

Internal: https://www.upwork.com/ab/applicants/1585068624908439552/job-details External: https://www.upwork.com/jobs/~01c63baf5aa3db8688

[varshamb]

Proposal

We are adding \n before and after while converting html to markdown , I think removing it would work.

https://github.com/Expensify/expensify-common/blob/a71aad37aaba2b43b3a2d2f4be8052b025b265d3/lib/ExpensiMark.js#L256

[sobitneupane]

@varshamb Thanks for your proposal.

We are adding \n before and after while converting html to markdown , I think removing it would work.

Removing it would create an issue. When plain text is sent along the quoted text, line break will be removed.

https://user-images.githubusercontent.com/25876548/197930646-bff2ac0a-afd9-4791-ba30-f2180abc7afc.mov

[0xmiroslav]

Proposal

https://github.com/Expensify/expensify-common/blob/a71aad37aaba2b43b3a2d2f4be8052b025b265d3/lib/ExpensiMark.js#L471-L477

        this.htmlToMarkdownRules.forEach((rule) => {
            // Pre-processes input HTML before applying regex
            if (rule.pre) {
                generatedMarkdown = rule.pre(generatedMarkdown);
            }
-           generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
+           generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement).trim();
        });

https://github.com/Expensify/expensify-common/blob/a71aad37aaba2b43b3a2d2f4be8052b025b265d3/lib/ExpensiMark.js#L251-L258

            {
                name: 'quote',
                regex: /\n?<(blockquote|q)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
                replacement: (match, g1, g2) => {
                    const resultString = g2.trim().split('\n').map(m => `> ${m}`).join('\n');
-                   return `\n${resultString}\n`;
+                   return `\n${match.substring(0, match.indexOf(match.trim()))}${resultString}\n`;
                },
            },

And match.substring(0, match.indexOf(match.trim())) is to handle cases like this:

Text1


> Text2

Before this fix, it will show like this: (one line trimmed)

Text1

> Text2

[conorpendergrast]

@sobitneupane has "sporadic" availability, so will get back to this next week!

[sobitneupane]

@0xmiroslav Thanks for your proposal. Your proposal looks promising.

Can you be more descriptive in your proposal. Will you please explain root cause of this issue? And how your proposal solves the issue?

[0xmiroslav]

-           generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
+           generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement).trim();

Explanation: https://github.com/Expensify/expensify-common/blob/a71aad37aaba2b43b3a2d2f4be8052b025b265d3/lib/ExpensiMark.js#L256

                    return `\n${resultString}\n`;

\n is added at the beginning of and at the end of resultString. This is needed because it cannot have the same line as normal text. (when convert markdown to html, \n at the start and at the end is removed so no \n before <blockquote>, after </blockquote> reference: https://github.com/Expensify/expensify-common/blob/a71aad37aaba2b43b3a2d2f4be8052b025b265d3/lib/ExpensiMark.js#L154-L163) But if quote text is at the beginning of message, no need \n at the beginning because of no other text before quote text. So does end of message case. So it should be trimmed after full replacement for all occurrences.

-                   return `\n${resultString}\n`;
+                   return `\n${match.substring(0, match.indexOf(match.trim()))}${resultString}\n`;

Explanation:

                regex: /\n?<(blockquote|q)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,

regex starts with \n?. This means text to replace can start with \n. Therefore, if match has \n at the beginning (empty line is before quote text), it's trimmed and then \n added so final return value has just \n which expects \n\n i.e. Text1\n\n> Text2 markdown is converted to Text1\n<blockquote>Text2</blockquote> html match : \n<blockquote>Text2</blockquote> resultString : > Text2 return value : \nText2\n after regex applied: Text1\n> Text2

[sobitneupane]

@0xmiroslav Your proposal's first step is enough to solve the issue mentioned in issue description. But we must make sure that it does not cause any regression as suggested change will effect all html to markdown translations.

Your proposal's second step does not completely solves the issue that you are mentioning. It is retaining the new line when html is converted to markdown but not the opposite way (i.e., markdown to html)

https://user-images.githubusercontent.com/25876548/199223967-87cb1e01-babe-4ce1-9648-3a4b009263f0.mov

I am not sure weather we want to solve the issue that you mentioned in 2nd step of the proposal here or create a separate issue for it? cc: @MonilBhavsar

[0xmiroslav]

I agree 2nd step is beyond the scope of this GH. And regarding regression test, 1st step also fixes other similar issues along with this GH. i.e. # This is a heading1

https://user-images.githubusercontent.com/97473779/199227327-3e806634-af11-4152-890b-e3045e8adebd.mp4

[michaelhaxhiu]

Hey @conorpendergrast just dropping a note as a reminder to keep the pressure on to find a contributor and get this one closed out!

cc @MonilBhavsar to weigh in on the latest question to move this forward, and ideally select a proposal + begin PR phase 🙏

[MonilBhavsar]

Your proposal's first step is enough to solve the issue mentioned in issue description. But we must make sure that it does not cause any regression as suggested change will effect all html to markdown translations.

I strongly agree with this. The function is utilized by all rules, so we should evaluate if it doesn't cause any regression on other markdown rules.

@0xmiroslav could you please explain what exactly you're trying to fix in your second proposal.

And regarding regression test, 1st step also fixes other similar issues along with this GH.

In the video, the blank line still appears no? Is it before the fix? Regarding regression, I would say in testing steps we check for all markdowns affected by this change.

[0xmiroslav]

In the video, the blank line still appears no? Is it before the fix?

@MonilBhavsar yes, exactly. I explained another bug (i.e. html header tag) before the fix. After the fix, it will be gone as well

[0xmiroslav]

could you please explain what exactly you're trying to fix in your second proposal

Text1


> Text2

It shows like this when try to edit: (one line trimmed)

Text1

> Text2

And this is another bug separated from this GH. Not sure if we should fix this as well in this GH.

[MonilBhavsar]

Okay got it. I think we should fix that as a part of this issue only as they are almost similar.

[sobitneupane]

Okay got it. I think we should fix that as a part of this issue only as they are almost similar.

@MonilBhavsar In that case, I believe we should update the issue description to include this case as well.

https://user-images.githubusercontent.com/25876548/199932697-9b2169be-092b-4c39-8b7d-145c737e95ce.mov

[MonilBhavsar]

Ok, something like a blank line is removed/added when block quote is edited?

[sobitneupane]

Ok, something like a blank line is removed/added when block quote is edited?

Yes. We can also update Actions Performed with something like this:

1. Send following two messages:

A.

> Quote text

B.

 Line1
 
 
 > Quote text
 
 
 Line2

2. Edit each of them.
3. While editing message A, you can notice an extra line both above and below the Quote text. While editing message B, you can notice a line removed between Line1 and Quote text

[conorpendergrast]

Looks like this is making progress, let's keep it going to find a proposal that we'll accept!

[s77rt]

@sobitneupane Message B gets trimmed at the insertion, not on the edit mode

[conorpendergrast]

@sobitneupane @MonilBhavsar bump on keeping this proposal making progress 🙏

[sobitneupane]

Waiting for proposal.

[conorpendergrast]

@sobitneupane @MonilBhavsar So the first part of this proposal is not a go? Sorry, having trouble following!

[sobitneupane]

Yes, It is not. We are looking for proposal which solves issues for both type of messages (A and B) mentioned here. The proposal only gives solution for type A message in https://github.com/Expensify/App/issues/12120#issuecomment-1305590364

[conorpendergrast]

Thanks so much, I appreciate it! I'll bump up the price to $500 give that it's been more than a week without an accepted solution

[bernhardoj]

Proposal

For the first case, we can do the trim as the last step of htmlToMarkdown function.

expensify-common/ExpensiMark.js

htmlToMarkdown(htmlString) {
        let generatedMarkdown = htmlString;
        const body = /<(body)(?:"[^"]*"|'[^']*'|[^'"><])*>(?:\n|\r\n)?([\s\S]*?)(?:\n|\r\n)?<\/\1>(?![^<]*(<\/pre>|<\/code>))/im;
        const parseBodyTag = generatedMarkdown.match(body);

        // If body tag is found then use the content of body rather than the whole HTML
        if (parseBodyTag) {
            generatedMarkdown = parseBodyTag[2];
        }

        this.htmlToMarkdownRules.forEach((rule) => {
            // Pre-processes input HTML before applying regex
            if (rule.pre) {
                generatedMarkdown = rule.pre(generatedMarkdown);
            }
            generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
        });
        generatedMarkdown = generatedMarkdown.replace(/<div.*?>|<\/div>|<comment.*?>|\n<\/comment>|<\/comment>|<h2>|<\/h2>|<h3>|<\/h3>|<h4>|<\/h4>|<h5>|<\/h5>|<h6>|<\/h6>/gm, '[block]');
-       return Str.htmlDecode(this.replaceBlockWithNewLine(Str.stripHTML(generatedMarkdown)));
+       return Str.htmlDecode(this.replaceBlockWithNewLine(Str.stripHTML(generatedMarkdown))).trim();
    }

For the second case, here is what I found. When we send this message

Line1
 
 
> Quote text
 
 
Line2

the html representation is: Line1<br /><br /><blockquote>Quote text</blockquote><br /><br />Line2. When we press edit, htmlToMarkdown() is called which used htmlToMarkdownRules to do the conversion. The rule for blockquote will match additional line break before the blockquote which in results remove a line before it. To fix it, we can simply remove the line break match from the regex.

{
    name: 'quote',
-   regex: /\n?<(blockquote|q)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
+   regex: /<(blockquote|q)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
    replacement: (match, g1, g2) => {
        const resultString = g2.trim().split('\n').map(m => `> ${m}`).join('\n');
        return `\n${resultString}\n`;
    },
},

We want to match the additional before blockquote because previously, there is this issue where when we send above message, the html representation is Line1<br /><br /><br /><blockquote>Quote text</blockquote><br /><br />Line2. Notice that there is an extra <br /> before <blockquote>. However, the issue has been fixed by removing the extra <br /> here.

@0xmiroslav Your proposal's first step is enough to solve the issue mentioned in issue description. But we must make sure that it does not cause any regression as suggested change will effect all html to markdown translations.

Your proposal's second step does not completely solves the issue that you are mentioning. It is retaining the new line when html is converted to markdown but not the opposite way (i.e., markdown to html)

Screen.Recording.2022-11-01.at.17.14.55.mov I am not sure weather we want to solve the issue that you mentioned in 2nd step of the proposal here or create a separate issue for it? cc: @MonilBhavsar

Now, for the last issue where the line is missing before the blockquote on the chat (not in edit mode), I found out that that it only happens on web platform. A blockquote should be on its own line, but the behavior is different between native and web. Here is what I tested by writing this html Line1<br /><br /><blockquote>Quote text</blockquote><br /><br />Line2 on Edge. You can see that the line before blockquote is missing, while on native the line is shown correctly. The same thing happens on Chrome, Safari, and Firefox. So, the solution I could think of is to add additional <br /> before blockquote only for web platform.

if (Platform.OS === 'web') {
    html = Str.replaceAll(html, /<br \/><blockquote>/gi, '<br /><br /><blockquote>');
}

https://github.com/Expensify/App/blob/a3bf7a0f1f3c56e58f97836bf46d09acd010daae/src/pages/home/report/ReportActionItemFragment.js#L101-L124

Result

https://user-images.githubusercontent.com/50919443/202898294-1e5c7354-383d-45b8-b5d2-6009041d35f3.mov

https://user-images.githubusercontent.com/50919443/202899101-cbd6b557-1628-45e4-8c48-65e214bdc7d5.mp4