Edit comment - Spaces are not displayed in edit box

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Expected Result:

Spaces should be visible in edit comment box

Actual Result:

Spaces are not visible in edit comment box.

Action Performed:

1. Navigate to a conversation
2. Send a message with the following text

Hello 
Space
Test

3. Right click on the message and click edit comment.

Workaround:

N/a

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.43-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Expensify/Expensify Issue URL: https://www.upwork.com/jobs/~01aab2d470107fe14c

View all open jobs on Upwork

---

From @roryabraham https://expensify.slack.com/archives/C01GTK53T8Q/p1620849222131700

When editing a comment, newlines do not appear in the text box.

[MelvinBot]

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

[MelvinBot]

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

[stitesExpensify]

@roryabraham this can probably be external unless you're working on it already, right?

[roryabraham]

Yep I'm not working on this, so let's get this on Upwork!

[MelvinBot]

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

[NicMendonca]

Created job in Upwork: https://www.upwork.com/jobs/~01aab2d470107fe14c

[MelvinBot]

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

[pranshuchittora]

Proposal 📄

Spaces should be visible in edit comment box

Investigation 🕵🏻‍♂️

• I started by debugging the edit flow. And found out that the text key was incorrect as compared to the html.

• After some more debugging I came to the conclusion that the error is in the submit / add message flow. You can see that the two keys are different as shown in the below screenshot.

• This narrowed down the investigation a lot and I found that the culprit is this.

htmlForNewComment.replace(/<[^>]*>?/gm, '')

Approach 👨🏼‍💻

File of concern : src/libs/actions/Report.js

1. One of the easiest way is to replace the regex with something that works.
2. We can also create a rule engine that performs various transformations.

Discussion with the team might be required to pick the best possible approach

Best Practices 💃🏼

• No hard-coded regexes
• No inline styling
• Accessibility
• Follow React Native best practices

Testing Strategy 🧪

• Unit tests for the underlying modules

Expected Delivery Time 📦

Approx 1-3 days.

Previous Experience 🙅🏼‍♂️

• [Hold - for payment on Wed 26th] Add an About and App download links page to Expensify.cash #2811

[roryabraham]

@pranshuchittora The regex you're referring to actually lives here. It's not that it "doesn't work" per-se. It does exactly what it intends to – strips HTML tags from a string. However, we will need to investigate the full context of why we have separate reportAction fields for text v.s. html. I believe it has to do with preventing XSS attacks, but I'm really not sure cc @yuwenmemon @tgolen do you know why we chose to do have separate reportAction values for the message text & html?

Without knowing the full answer to that question – I think the better of your two proposed approaches might be 2, i.e: take a peek at the message HTML, transform certain tags to text, and strip others that we don't handle yet. So for example, a <br /> would just become \n. This would probably be the easiest way to solve just this current problem of displaying newlines in the text composer. However, it doesn't scale well for messages containing other HTML elements such as <ul> or <ol>, or if we ever wanted to implement "live previews" of code blocks or something.

So a better long-term solution will probably be to build a custom input that can enable us to display and easily edit certain HTML like text. Similar to how Slack works. But I think that would be a bold undertaking and would require a broader conversation.

Yet another alternate solution would be to store not only the transformed html of a message, but also the markdown that the user wrote to generate that html. Then we could display that markdown string (always plain text) in the compose box and allow the user to edit that. That might help us achieve a UX more similar to GitHub than Slack.

Thoughts?

[pranshuchittora]

@roryabraham I truly agree with you. As per my experience regexes doesn’t scale well in terms of maintainability in the long run.

Considering the budget for this fix, the possible way is to replace the HTML string sanitization with native sanatizer new Sanitizer().sanitizeToString(stringToClean) MDN Reference which apparently not supported by any browser. Or we can go with some popular community packages like this https://www.npmjs.com/package/sanitize-html.

In the long run, I prefer the HTML way as mentioned in the above comment. But it totally depends on the budget for this ticket. If possible then I would love to implement this :)

[BayCo]

Details

If we turn this line into this:
const commentText = parser.replace(text).replace(/<br>/gm, '\n');
problem is solved.

However, there's one more problem in editing the message that new message doesn't use ExpensiMark (parser).

Proposal

1. As I mentioned above current issue will be solved by reversing the parsers newline rule
2. I can add the ExpensiMark (parser) to message editing

[Viacheslav80]

Proposal

We modify getActionText() in /src/pages/home/report/ReportActionContextMenu.js

147:      const message = _.last(lodashGet(this.props.reportAction, 'message', null));
+         const text =  message.html.replace(/<br \/>/g,'\n');
-         return lodashGet(message, 'text', '');
+         return text || lodashGet(message, 'text', '');

[yuwenmemon]

However, we will need to investigate the full context of why we have separate reportAction fields for text v.s. HTML.

@roryabraham I believe the text field is for exactly this:

Yet another alternate solution would be to store not only the transformed html of a message, but also the markdown that the user wrote to generate that html. Then we could display that markdown string (always plain text) in the compose box and allow the user to edit that

It also is useful for contexts where we can use HTML (like push notifications), but I think if we're not using the text field in the "alternate solution" you're describing we certainly should be 😅

[marcaaron]

Ok based on the conversations in Slack I think that how we should handle this is:

1. Stop referencing the text field altogether as we are discussing deprecating it and removing it
2. Implement a reverse parser in ExpensiMark that will convert HTML back into our own custom flavor of markdown. Something like:

const parser = new ExpensiMark();
const markdownTextForEditor = parser.HTMLtoMarkdown(/* html string */)

3. Include automated tests for ExpensiMark()
4. Finally, update this code so that we are converting the html to markdown before setting the draft message for edit instead of grabbing the text value

https://github.com/Expensify/Expensify.cash/blob/087205d81ed0bf3b072d85e09c20bf2af1c91262/src/pages/home/report/ReportActionContextMenu.js#L141-L149

[marcaaron]

Also, it might be fine for this work to be split up into several issues - but we can start by handling the line breaks at least?

[marcaaron]

This is pretty much just waiting on a contributor to raise their ✋ and agree to do this

@pranshuchittora thoughts?

[pranshuchittora]

Hi, @marcaaron pardon the delayed reply.
I agree with the changes in the parser. I have a couple of questions

• Can you explain what the custom flavour of markdown is? Pls, provide an example of HTML -> Markdown. (Because few links mentioned in the slack thread are private)
• Before that do we need to create a spec of the possible transformations?

[pranshuchittora]

Let me elaborate

• As we are deprecating text and will be showing markdown in the text input is that correct?
• As a solution to this issue we are required to write a parser for HTML -> Markdown ?

Example 👇🏼

Input HTML

<a href="https://expensify.cash/">Expensify</a>

OutputMarkdown

[Expensify](https://expensify.cash/)

[roryabraham]

@pranshuchittora That looks correct to me! Generally speaking, we try to match GitHub-flavored markdown.

[pranshuchittora]

@roryabraham& @marcaaron I think #2965 is related to this. If we fix #2965 now then we need to fix it again as we are deprecating text
Can you pls confirm?

[parasharrajat]

@pranshuchittora AFAIK that is a different issue. We would want to show the text as the last message text preview in LHN. This issue requires converting Html to MD and vice-versa as while editing a message, it would be best if the user can see the original message created(in MD). We would drop the text from storage and Create the text variant of the message on LHN at runtime.

Hope it clears your doubt.

[marcaaron]

@roryabraham @pranshuchittora I don't think we need to implement a perfect conversion to markdown and can start with a simple line breaks to new lines conversion for now then add other features later. @roryabraham not sure if you feel the same way or not.

@pranshuchittora no that's not correct as @parasharrajat has pointed out. One situation calls for:

HTML -> markdown with line breaks converted to \n
HTML -> plain-text with line breaks converted to spaces

[roryabraham]

start with a simple line breaks to new lines conversion for now then add other features later

Seems like a great place to start, and keeps the scope of this issue nice and tidy. A couple questions:

1. For features not yet implemented, should we just display plain HTML?
2. Should we consider making a tracking issue for the greater endeavor of making Expensimark "bi-directional" ?

[marcaaron]

For features not yet implemented, should we just display plain HTML?

I think they'd be plain text for now (so strip all tags but replace
with new lines) otherwise we are showing raw HTML in the editor which could be strange.

Should we consider making a tracking issue for the greater endeavor of making Expensimark "bi-directional"

Sounds like a great idea to me.

[NicMendonca]

@pranshuchittora just sent you the contract in Upwork, thanks!

[roryabraham]

Created a tracking issue for other HTML ---> MD conversions here

[NicMendonca]

hey @pranshuchittora! how are you getting on with the PR? Is there anything you need help with?

[pranshuchittora]

hey @pranshuchittora! how are you getting on with the PR? Is there anything you need help with?

Hi @NicMendonca I am working on it but the version of expensify-common on GitHub is 1.0.0 but on npm it's 1.0.1.

https://github.com/Expensify/expensify-common/blob/f9156da06752046c1df8a32461e46fb51684b32d/package.json#L3

https://www.npmjs.com/package/expensify-common

[pranshuchittora]

Thought @roryabraham @marcaaron

Peek.2021-05-27.01-12.mp4

Current Implementation
As of now, there's only one rule for HTML -> Markdown. Which parses and modifies the string

Future Architecture
We can have a set of pre and post rules as we have now. for text parser.

---

Blockers

Version of expensify-common on GitHub is 1.0.0 but on npm it's 1.0.1.

https://github.com/Expensify/expensify-common/blob/f9156da06752046c1df8a32461e46fb51684b32d/package.json#L3

https://www.npmjs.com/package/expensify-common

[roryabraham]

Until recently, we were pointing Expensify.cash at an expensify-common commit hash rather than actually publishing expensify-common on npm. It looks like @jasperhuangg changed that here.

@marcaaron since you wrote the SO that @jasperhuangg references in that PR, do you have any comment on whether or not we should be publishing expensify-common on NPM vs just using a commit hash?

[tgolen]

The SO that Jasper linked to is a very general SO that was written with the context of web-e, web-s, and mobile. It wasn't really meant to cover e.cash. @AndrewGable talked about this a lot in the beginning and I think our takeaway was:

• As long as we can maintain strict control over what gets published to NPM, then it is OK to use semver.

At the time, there were doubts if we could maintain the strict control we needed, which is why we used the hash. Here is the full context behind it: https://github.com/Expensify/Expensify/issues/147306 and it looks like the concern was never resolved.

[tgolen]

I think we should revert Jasper's change until the solution recommended in that linked issue is solved.

[roryabraham]

Agreed @tgolen.

So @pranshuchittora your process should probably be something like this:

1. Create a pull request in expensify-common to update ExpensiMark with your changes.
2. Once that pull request is merged, create an Expensify.cash pull request to update package.json in Expensify.cash to point at the merge commit ref for your expensify-common pull request, instead of the npm package.

[marcaaron]

@marcaaron since you wrote the SO that @jasperhuangg references in that PR, do you have any comment on whether or not we should be publishing expensify-common on NPM vs just using a commit hash?

That SO is out of date and should be deleted. I wrote those instructions thinking we would adopt that plan (we didn't) and then went OOO and it slipped my mind. Amending it now, but we should revert @jasperhuangg's changes.

[marcaaron]

Sorry, I've updated those instructions so we can avoid this in the future.

[jasperhuangg]

Hey! Sorry for missing this. I wasn't aware that there was a difference, apologies for failing to notice this. I guess something felt slightly off when it was startimg off at version 1.0.0, but I didn't think much of it. Glad you were able to revert the change!

[pranshuchittora]

To summarise

1. Successfully merge the PR in expensify-common.
2. Change the version of expensify-common to the merge commit hash in E.cash repo (this repo).

Is this correct?

cc @jasperhuangg @marcaaron