Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment July 20] Markdown - Copied text (with markdown) does not show the formatting when pasted in e.cash #3790

Closed
isagoico opened this issue Jun 29, 2021 · 31 comments · Fixed by #4009
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jun 29, 2021

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

Upwork job: https://www.upwork.com/jobs/~01784929033238eacd


Action Performed:

  1. Copy the following text:

Bold text
italic text
Strikethrough text

  • Bullet points test

  • Another Bullet point

    Indents test

  1. Paste the text in e.cash

Expected Result:

Formatting should be displayed in e.cash.

Actual Result:

Text is pasted without any formatting. Only line breaks are displayed.

Workaround:

User has to edit the text in e.cash compose box and add the markdown manually.

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.74-0

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

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1617291564288600

When copying text (gmail draft in this example) and pasting into e.cash, formatting is being stripped. I can create an issue for it, wondering if we should attempt to fix parts of it first, like line breaks, then markdown in a separate issue? Thoughts/ideas?

@MelvinBot
Copy link

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

@mallenexpensify
Copy link
Contributor

@tylerkaraszewski this can be worked on by a contributor, right? If so, can you add the External label, thanks

@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Jun 30, 2021
@MelvinBot
Copy link

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

@tylerkaraszewski
Copy link
Contributor

Seems straightforward for an external contributor to work on.

@parasharrajat
Copy link
Member

It is not straightforward but looks like so.

  1. We need to convert the copied HTML to MD first. and then show the MD in composer.
  2. We do not support lists. So we have to decide what can be done about that.

@rdjuric
Copy link
Contributor

rdjuric commented Jun 30, 2021

Feels like this one should hold until #3229?

@mallenexpensify
Copy link
Contributor

Reassigning @Jag96

@Jag96 Jag96 added the Exported label Jul 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

I agree that this doesn't have to be held on #3229. Also, if we don't currently support lists I don't think we need to add that in this issue.

We need to convert the copied HTML to MD first. and then show the MD in composer.

Just to confirm, we shouldn't have to do anything special to show the markdown in the composer right? We'd just have to ensure that when text is copied, the markdown version is copied to the clipboard.

@parasharrajat
Copy link
Member

But when you copy only HTML flavour is available. You can quickly check it on Gmail. Web browser.

@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

Ah ok, so it sounds like #3047 will handle that conversion. Looping in @roryabraham here.

@NikkiWines NikkiWines added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 2, 2021
@NikkiWines
Copy link
Contributor

Adding the Help Wanted label, as it looks like we're still looking for proposals here (please remove the label if we're not!)

@Jag96
Copy link
Contributor

Jag96 commented Jul 6, 2021

@roryabraham just bumping here, should this issue be held on #3047?

@parasharrajat
Copy link
Member

I still think it's better to save the markdown and directly render it to HTML when needed. react-native-render-htmls author has also put this idea https://expensify.slack.com/archives/C01GTK53T8Q/p1625507632443500.

@roryabraham
Copy link
Contributor

@Jag96 #3047 is about taking HTML that we've saved, and converting it to markdown for editing. I think this issue is different. I don't know for sure how copying formatted text works, but my guess is that both the plain-text and formatted (html) versions of the text are copied to the clipboard. If that's correct, I think we just need to make sure that the formatted html is pasted in E.cash, instead of the plain-text version. It will be a bit wonky because our main report action composer doesn't support HTML. So the raw HTML would be displayed in the composer, but then would display correctly formatted once sent. Does that make sense?

@Jag96
Copy link
Contributor

Jag96 commented Jul 7, 2021

From this comment it sounds like it's just HTML that is being copied at the moment, and after copying formatted HTML from e.cash into Notepad on mac it looks like some of the HTML is pasted.

E.cash (copied) Notepad (pasted)
image image

So the raw HTML would be displayed in the composer, but then would display correctly formatted once sent.

From testing we don't convert raw HTML comments (ex <b>Test</b>) into formatted HTML after posting, do you mean that we'll convert the raw HTML we post into markdown? It sounds like @parasharrajat's idea is what we'd have to do instead to ensure it'd display properly after posting.

I still think it's better to save the markdown and directly render it to HTML when needed. react-native-render-htmls author has also put this idea https://expensify.slack.com/archives/C01GTK53T8Q/p1625507632443500.

The investigation for saving markdown instead will take some time so for now I don't think that's an option in the short term.

@roryabraham
Copy link
Contributor

From testing we don't convert raw HTML comments (ex Test) into formatted HTML after posting

Ah, you're right! Imo this is a problem. We should be able to type html and have it displayed as formatted html. Just like how markdown and plain HTML work in GitHub.

@quinthar
Copy link
Contributor

quinthar commented Jul 7, 2021 via email

@parasharrajat
Copy link
Member

As per Rory's suggestion.

Proposal

  1. Allow pasting of HTML directly into the TextInput on the web. For this, we control the paste event on TextInputFocusable https://github.com/Expensify/Expensify.cash/blob/c956587ba3a27d39308b13d95e6ad9bdd0ed4f89/src/components/TextInputFocusable/index.js#L145.
  2. We create another method here that handles image pasting first via checkAttachment and then pasting of text if there are no images.
  3. I already added logic to manage the cursor position and pasting text here https://github.com/Expensify/Expensify.cash/blob/c956587ba3a27d39308b13d95e6ad9bdd0ed4f89/src/components/TextInputFocusable/index.js#L287-L296
  4. We will reuse the above logic to allow the pasting of HTML into the Input.
  5. When using click submit.
  6. We need to disable escaping of the HTML from Expensimark.
    replace(text) {
        // This ensures that any html the user puts into the comment field shows as raw html
        let replacedText = text || Str.safeEscape(text); //remove this escaping

we can put this escaping behind a flag.

Optional

  1. Currently I am not sure. but I believe we have to remove the attributes from the HTML tags.
  2. Maybe restrict the HTML tags to the one which we support in Expensimark.
  3. Add required attributes to the Special tags, such as A needs target="_blank". etc.

I am looking into htmlparser2 for this as this lib is already used by react-native-render-html. I will update the proposal on how to achieve this.

@roryabraham
Copy link
Contributor

Okay, if we do move forward with this plan, we'll want to be sure to strip <script> and <iframe> tags. Maybe others too.

To clarify, you aren't saying that you can just type formatted HTML
into the compose box and expect it to work, right? I do not think that is
a goal, or even a good feature.

Yeah, so I tend to disagree. I love how in GH we can fallback on regular HTML. And it's basically free to implement – as @parasharrajat suggested we just need to stop escaping the HTML. It feels pretty arbitrary to me that this doesn't work already.

If we don't want to support plain HTML, then this still doesn't need to be on hold on #3047. Instead, we would just take step 2 of @parasharrajat's proposal above, and run the pasted HTML through the HTML -> MD converter. And it would only work for HTML -> MD conversions we have built, which currently is just <br/> ---> \n

@Jag96
Copy link
Contributor

Jag96 commented Jul 8, 2021

When we first created the Expensimark lib, we explicitly decided not to convert HTML to formatted HTML so that we could control everything that gets converted to HTML, you can see we even have a test for this: https://github.com/Expensify/expensify-common/blame/master/__tests__/ExpensiMark-test.js#L74

Updating the parser to not escape the HTML should be a separate discussion in #expensify-open-source since I don't think that is functionality we currently want to change.

@parasharrajat
Copy link
Member

Ok Sure. Please let me know the best path forward. Thanks

@Jag96
Copy link
Contributor

Jag96 commented Jul 8, 2021

I think we've come full circle and the solution here should be what @parasharrajat suggested initially: We need to convert the copied HTML to MD first. and then show the MD in composer.

It sounds like we only have one HTML->MD conversion built out already (#3790 (comment)), so to do this I think we'd need to add some more rules similar to what was done in Expensify/expensify-common#381 to do the other conversions (bold/italic/strikethrough).

@parasharrajat are you interested in taking this on?

@parasharrajat
Copy link
Member

@Jag96 Yes, I am interested. I will link PRs for supported tags to the Tracking issue and update you if I am stuck at something.

@Jag96
Copy link
Contributor

Jag96 commented Jul 9, 2021

Sounds great! I'll assign to you and invite you to the Upwork job, thanks

@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 9, 2021
@parasharrajat
Copy link
Member

@Jag96 Thanks for the invite. But Changes to HTML->MD conversion should be separate issues and looking at the complexity, possibly multiple for the different tags listed here #3047.

@Jag96
Copy link
Contributor

Jag96 commented Jul 9, 2021

Ok, how about we split this up into 2 issues:

  1. Implement one of the conversions in [Tracking Issue] Implement HTML -> Markdown conversions in Expensimark #3047 (maybe italics?) to help confirm this will work
  2. Update e.cash to ensure the HTML copy works, and that it can be converted to MD for the currently existing tags (including the one we add in the step above)

Then the rest of the HTML->MD conversions can happen in #3047. How does that sound @parasharrajat?

@parasharrajat
Copy link
Member

@Jag96 Sounds good.

@Jag96
Copy link
Contributor

Jag96 commented Jul 10, 2021

Created another issue here and invited @parasharrajat to the Upwork job. Used the italics conversion for the title, but feel free to choose any of the items from the tracking issue.

@Jag96 Jag96 added the Reviewing Has a PR in review label Jul 14, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 14, 2021

Reopening to keep track of payment

@Jag96 Jag96 changed the title Markdown - Copied text (with markdown) does not show the formatting when pasted in e.cash [HOLD for payment July 20] Markdown - Copied text (with markdown) does not show the formatting when pasted in e.cash Jul 14, 2021
@Jag96 Jag96 reopened this Jul 14, 2021
@parasharrajat
Copy link
Member

@Jag96 any update here? Thanks

@Jag96
Copy link
Contributor

Jag96 commented Jul 21, 2021

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants