[HOLD for payment 2023-03-01] [$1000] Editing header bold text in chat increases vertical gap above the text #14668

kavimuru · 2023-01-30T21:16:02Z

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:

Open the app
Open any chat
Send bold text using #
Edit the text

Expected Result:

Vertical gap above the text should not change after edit

Actual Result:

Vertical gap above the text is increased after editing

Workaround:

unknown

Platforms:

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

Version Number: 1.2.61-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:

vertical.gap.above.header.text.mp4

Recording.1404.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675067869948869

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~011432c27e94a6da98
Upwork Job ID: 1623338262676652032
Last Price Increase: 2023-02-08

dylanexpensify · 2023-02-02T12:24:53Z

@shawnborton can you confirm this is indeed a bug, and I'll get it moved over?

dylanexpensify · 2023-02-06T10:22:37Z

Shawn is OOO, but back this week!

shawnborton · 2023-02-07T21:43:58Z

Hm yeah, this seems like a bug! It's a bit strange that there was even a blank line inserted above the #/headline in the first place.

dylanexpensify · 2023-02-08T14:54:49Z

Nice, thanks Shawn!

dylanexpensify · 2023-02-08T15:08:22Z

Replicated

MelvinBot · 2023-02-08T15:09:40Z

Job added to Upwork: https://www.upwork.com/jobs/~011432c27e94a6da98

MelvinBot · 2023-02-08T15:09:44Z

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

MelvinBot · 2023-02-08T15:09:54Z

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

MelvinBot · 2023-02-08T15:10:01Z

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

eh2077 · 2023-02-08T15:10:29Z

Proposal

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

If we edit a sent header bold text, # heading , and save it in chat, there'll be an extra leading line break above the header.

What is the root cause of that problem?

When translating html h1 tag to markdown, we simply wrap it with a leading and a trailing line break. For example, html

<h1>heading</h1>

will be translated into markdown string

\n# heading\n

If we change the markdown text to

\n# heading test\n

and save it, then markdown text

\n# heading test\n

will be translated to html

<br /><h1>heading test<h1>

by applying markdown-to-html rules

https://github.com/Expensify/expensify-common/blob/668be7533215fc8d683d658da839361664320965/lib/ExpensiMark.js#L164-L173

The leading line break will be translated to the extra <br /> tag.

To have a closer look at the root cause, let's try another example.

Send a heading message

# heading

In the editing mode, change the markdown string

\n# heading\n

to (remove the leading and trailing line break)

# heading test

Save it and the leading line break is still there.

This is because when we save a changed message, we don't simply call ExpensiMark#replace() once to get the html to save in backend. Instead, we do markdown-to-html, html-to-markdown and markdown-to-html conversion to get the final html to save. So the markdown input of the last markdown-to-html conversion is

\n# heading test\n

instead of

# heading test

See https://github.com/Expensify/App/blob/main/src/libs/actions/Report.js#L829-L832

So the root cause is the invalid regex rule to translate html <h1> header to markdown.

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

To fix this issue, we should improve the regex rule to translate h1 tag to markdown. Instead of simply wrapping it with a leading and trailing line break, we should only add line break if there's non-empty text before or after the <h1> heading. We can improve the regex rule from

https://github.com/Expensify/expensify-common/blob/668be7533215fc8d683d658da839361664320965/lib/ExpensiMark.js#L232-L233

to the following

regex: /([\s\S]*?)\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\2>(?![^<]*(<\/pre>|<\/code>))\s*([\s\S]*)/gi,
replacement: (match, g1, g2, g3, g4, g5) => `${g1}${g1 && '\n'}# ${g3}${g5 && '\n'}${g5}`,

So the html

<h1>heading</h1>

will be translated to markdown text

# heading

and the edited markdown text

# heading test

will be translated into html

<h1>heading test</h1>

What alternative solutions did you explore? (Optional)

Call trim method for markdown text

hackykitty · 2023-02-09T04:58:12Z

I reviewed @eh2077 and @tienifr's proposals. Great.
But string 'edited' is laid horizontally in other line, and adding bold string 'edited' is laid vertically.
To fix this, we need to follow the style:

flex-direction: row;

align-items: center;

Applying style eliminates the need to remove two duplicate \n's before converting the markdown to HTML.
All problems can be solved by adding only 2 styles.
thanks.

daraksha-dk · 2023-02-09T05:18:08Z

Hey @hackykitty, you might want to read our contributing guidelines to help you better understand the flow.

(After that you might want to join expensify's slack channel!)

tienifr · 2023-02-09T06:42:59Z

Proposal

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

Vertical gap above the text is increased after editing a h1 tag

What is the root cause of that problem?

In htmlToMarkdown, we're adding new line to both before and after the # , which should not be the case, we should only add new lines if there's non-new-line content before and after the heading.

Why non-new-line? because if before the heading there's already a new line, the # will be on the next line already and we should not add a (redundant) new line in this case. This complies with how HTML renders the h1 tag.

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

In here, we should use a regex that satisfies the requirement we should only add new lines if there's non-new-line content before the heading.

diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js
index bac4b81..5622980 100644
--- a/lib/ExpensiMark.js
+++ b/lib/ExpensiMark.js
@@ -229,8 +229,8 @@ export default class ExpensiMark {
             },
             {
                 name: 'heading1',
-                regex: /\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))\s*/gi,
-                replacement: '\n# $2\n',
+                regex: /([^\r\n]*?)\s*<(h1)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\2>(?![^<]*(<\/pre>|<\/code>))\s*([^\r\n]*)/gi,
+                replacement: (match, g1, g2, g3, g4, g5) => `${g1}${g1 && '\n'}# ${g3}${g5 && '\n'}${g5}`,
             },
             {
                 name: 'listItem',

After this fix, another issue will occur. Let's consider this HTML
<h1>heading 1</h1><h1>heading 2</h1>. In here if both h1 blocks matches the regex, it will add 2 new lines in between the blocks, one after heading 1 and one before heading 2. This is because in the match, heading 1 has non-new-line content after it and heading 2 has non-new-line content before it. This is incorrect understanding since after heading 1 is matched, it will add 1 new line after it and heading 2 no longer has non-new-line content before it.

In order to fix this we need to introduce another property into the rule, which can be sequential: true. If the rule is sequential, the regex replacement will be applied once at a time. Example of this in htmlToMarkdown:

if (rule.sequential) {
                while (rule.regex.test(generatedMarkdown)) {
                    generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
                }
            } else {
                generatedMarkdown = generatedMarkdown.replace(rule.regex, rule.replacement);
            }

Of course the regex above will need to have the global flag /g removed for this to work.

We'll need to evaluate other block elements (like blockquote, pre) to make sure it's not having this bug. If we're doing to those elements the same thing we do to h1 here then it will have the same issues.

There's another bug relating to the heading tag I found during debugging, reported here.

What alternative solutions did you explore? (Optional)

eh2077 · 2023-02-09T09:28:37Z

Proposal

Updated

To fix the following test case mentioned by @tienifr in his latest proposal #14668 (comment)

<h1>heading 1</h1><h1>heading 2</h1>

I'd like to update the solution section of my previous proposal

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

I'd like to propose a new solution to improve the regex rule which translates html <h1> header to markdown. The new solution is to replace

https://github.com/Expensify/expensify-common/blob/668be7533215fc8d683d658da839361664320965/lib/ExpensiMark.js#L233

with

replacement: '[block]# $2[block]',

By replacing

\n# $2\n

with

[block]# $2[block]

we can reuse this function ExpensiMark#replaceBlockWithNewLine to handle line break properly. See

https://github.com/Expensify/expensify-common/blob/668be7533215fc8d683d658da839361664320965/lib/ExpensiMark.js#L521

https://github.com/Expensify/expensify-common/blob/668be7533215fc8d683d658da839361664320965/lib/ExpensiMark.js#L455-L467

/**
* replace [block] with '\n' if :
* 1. We have text before the [block].
* 2. The text does not end with a new line.
* 3. The text does not have quote mark '>' .
* 4. [block] not the last [block] in the string.
*
* @param {String} htmlString
* @returns {String}
*/
replaceBlockWithNewLine(htmlString) {

danieldoglas · 2023-02-11T01:38:02Z

@eVoloshchak what's your saying on these proposals?

MelvinBot · 2023-02-22T22:39:01Z

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:

[@eVoloshchak / @danieldoglas] The PR that introduced the bug has been identified. Link to the PR:
[@eVoloshchak / @danieldoglas] 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:
[@eVoloshchak / @danieldoglas] 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:
[@eh2077] Propose regression test steps to ensure the same bug will not reach production again.
[@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/266639

eh2077 · 2023-02-23T01:20:23Z

Regression Test Proposal

Open a chat and send this message
```
# heading
```
Edit the sent message and verify it's displayed in one line in editing mode
Edit and change the sent message to
```
# heading test
```
and save it
Verify the edited header bold text is still displayed in one line

Do we agree 👍 or 👎

MelvinBot · 2023-02-23T01:20:26Z

📣 @eh2077! 📣

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-02-23T01:23:26Z

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

danieldoglas · 2023-03-02T16:05:39Z

@eVoloshchak can you fill this #14668 (comment)? Thanks!

dylanexpensify · 2023-03-03T13:34:51Z

I like those steps, @eh2077 with some slight tweaking on the verbiage. @danieldoglas I'm curious on your thoughts if we should create a test for this? Or if you think this would get caught without a test? I'm tempted to say the latter.

danieldoglas · 2023-03-03T13:37:19Z

@dylanexpensify I think we should include this as part of the tests related to the markdown edits.

eVoloshchak · 2023-03-03T13:45:08Z

@eVoloshchak can you fill this #14668 (comment)? Thanks!

Sure, doing it today

eVoloshchak · 2023-03-06T08:17:28Z

The PR that introduced the bug has been identified. Link to the PR: Add h1 markdown expensify-common#468
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: the PR description mentioned the design doc, which I can't access, I can't verify if it's a regression or was by design at the time
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: automated regression tests will catch this

dylanexpensify · 2023-03-06T09:34:31Z

Whipping up RT and payments today

dylanexpensify · 2023-03-06T11:13:54Z

Hmm @danieldoglas, under the existing markdown test Edit a message with Markdown I feel this scenario is covered for. Thoughts?

danieldoglas · 2023-03-06T13:06:30Z

@dylanexpensify I think we're being very generalistic in Edit a message with Markdown. the test suggested here will test exactly for the situation where editing a header, so I think it makes sense to update it.

dylanexpensify · 2023-03-07T11:37:11Z

Bump @danieldoglas! 🙇‍♂️

eh2077 · 2023-03-07T11:46:36Z

@dylanexpensify I think we're being very generalistic in Edit a message with Markdown. the test suggested here will test exactly for the situation where editing a header, so I think it makes sense to update it.

Hi @dylanexpensify , is this comment from @danieldoglas you're waiting for?

dylanexpensify · 2023-03-07T12:48:45Z

Wow, yes @eh2077 haha, I didn't refresh issue before posting 🤦

dylanexpensify · 2023-03-07T12:56:31Z

RT linked above! Working on payments now!

dylanexpensify · 2023-03-07T12:59:29Z

@eh2077 sent offer!
@eVoloshchak sent invite to apply to job!
@dhanashree-sawant sent invite to apply to job as reporter!

eh2077 · 2023-03-07T13:04:09Z

@dylanexpensify Thanks and accepted the offer!

dylanexpensify · 2023-03-07T13:11:51Z

@eh2077 payment sent!
@eVoloshchak offer sent!

dhanashree-sawant · 2023-03-08T03:11:26Z

@dylanexpensify Thanks and accepted the offer!

dylanexpensify · 2023-03-08T11:57:01Z

@dhanashree-sawant please apply when you can!

dhanashree-sawant · 2023-03-08T13:37:21Z

Hi @dylanexpensify, I have accepted your invite

dylanexpensify · 2023-03-08T13:58:10Z

oh woops! JEEZ I really have to start reloading the page before replying 🤦

dylanexpensify · 2023-03-08T13:59:20Z

Offer sent @dhanashree-sawant !

dylanexpensify · 2023-03-08T14:24:02Z

alrightyyyy we're all settled up!! Thanks everyone!!

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 30, 2023

melvin-bot bot locked and limited conversation to collaborators Jan 30, 2023

melvin-bot bot assigned dylanexpensify Jan 30, 2023

melvin-bot bot added the Overdue label Feb 6, 2023

melvin-bot bot removed the Overdue label Feb 6, 2023

dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Feb 8, 2023

melvin-bot bot unlocked this conversation Feb 8, 2023

melvin-bot bot changed the title ~~Editing header bold text in chat increases vertical gap above the text~~ [$1000] Editing header bold text in chat increases vertical gap above the text Feb 8, 2023

melvin-bot bot assigned eVoloshchak Feb 8, 2023

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

melvin-bot bot assigned danieldoglas Feb 8, 2023

This comment was marked as outdated.

Sign in to view

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

melvin-bot bot removed the Overdue label Feb 11, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 1, 2023

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

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

dylanexpensify closed this as completed Mar 8, 2023

[HOLD for payment 2023-03-01] [$1000] Editing header bold text in chat increases vertical gap above the text #14668

[HOLD for payment 2023-03-01] [$1000] Editing header bold text in chat increases vertical gap above the text #14668

Comments

kavimuru commented Jan 30, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

dylanexpensify commented Feb 2, 2023

dylanexpensify commented Feb 6, 2023

shawnborton commented Feb 7, 2023

dylanexpensify commented Feb 8, 2023

dylanexpensify commented Feb 8, 2023

MelvinBot commented Feb 8, 2023

MelvinBot commented Feb 8, 2023

MelvinBot commented Feb 8, 2023

MelvinBot commented Feb 8, 2023

This comment was marked as outdated.

eh2077 commented Feb 8, 2023 • edited Loading

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?

What alternative solutions did you explore? (Optional)

hackykitty commented Feb 9, 2023 • edited Loading

flex-direction: row;

align-items: center;

daraksha-dk commented Feb 9, 2023

tienifr commented Feb 9, 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?

What alternative solutions did you explore? (Optional)

eh2077 commented Feb 9, 2023 • edited Loading

Proposal

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

danieldoglas commented Feb 11, 2023

MelvinBot commented Feb 22, 2023 • edited by dylanexpensify Loading

eh2077 commented Feb 23, 2023

Regression Test Proposal

MelvinBot commented Feb 23, 2023

MelvinBot commented Feb 23, 2023

danieldoglas commented Mar 2, 2023

dylanexpensify commented Mar 3, 2023

danieldoglas commented Mar 3, 2023

eVoloshchak commented Mar 3, 2023

eVoloshchak commented Mar 6, 2023

dylanexpensify commented Mar 6, 2023

dylanexpensify commented Mar 6, 2023

danieldoglas commented Mar 6, 2023

dylanexpensify commented Mar 7, 2023

eh2077 commented Mar 7, 2023

dylanexpensify commented Mar 7, 2023

dylanexpensify commented Mar 7, 2023

dylanexpensify commented Mar 7, 2023

eh2077 commented Mar 7, 2023

dylanexpensify commented Mar 7, 2023

dhanashree-sawant commented Mar 8, 2023

dylanexpensify commented Mar 8, 2023

dhanashree-sawant commented Mar 8, 2023

dylanexpensify commented Mar 8, 2023

dylanexpensify commented Mar 8, 2023

dylanexpensify commented Mar 8, 2023

kavimuru commented Jan 30, 2023 •

edited by melvin-bot bot

Loading

eh2077 commented Feb 8, 2023 •

edited

Loading

hackykitty commented Feb 9, 2023 •

edited

Loading

tienifr commented Feb 9, 2023 •

edited

Loading

eh2077 commented Feb 9, 2023 •

edited

Loading

MelvinBot commented Feb 22, 2023 •

edited by dylanexpensify

Loading