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

[Feature] Support to html pasting on web #4009

Merged
merged 7 commits into from
Jul 14, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 13, 2021

Details

Fixed Issues

$ Fixes #3790

Tests | QA Steps

  1. Copy the following text:

Bold text
italic text
Strikethrough text

  1. Paste the text in e.cash

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

paste-w.mp4

Mobile Web

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner July 13, 2021 17:49
@MelvinBot MelvinBot requested review from Jag96 and removed request for a team July 13, 2021 17:49
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitpicks but it's working well!

const {files, types} = event.clipboardData;
const TEXT_HTML = 'text/html';
const TEXT_PLAIN = 'text/plain';
const pastedHTML = event.clipboardData.getData(TEXT_HTML);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this inside the types.includes(TEXT_HTML) check since it's only used in that context

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks

const domparser = new DOMParser();
const embededImages = domparser.parseFromString(event.clipboardData.getData(TEXT_HTML), TEXT_HTML).images;
const pastedText = event.clipboardData.getData(TEXT_PLAIN);
const embededImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const embededImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;
const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It was already there but I will update.

@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 14, 2021

@Jag96 While handling all this I found a couple of issues.

  1. If we edited the formatted text I see MD syntax but clicking save does not show formatting anymore.
    image
  2. There is an extra Line Break added on paste. I debugged the main cause and found we are getting newlines via HTMLparser.
  3. When I copy the existing sent message which is formatted and pastes it. it does not convert to formatted MD strings. The main issue here is that we don't use HTML semantic tags for rendering Every type of element is made with div and styling rules. Which we can't really interpret ATM.

Do you suggest they need solving and if so, How can I help?

@parasharrajat
Copy link
Member Author

Updated.

@Jag96
Copy link
Contributor

Jag96 commented Jul 14, 2021

Good finds!

  • 1 sounds like a regression to me, so I think that would be a blocker in this case and something we should look into.
  • For 2, I tested on Firefox and Chrome and it looks like Firefox has extra line breaks in the HTML whereas Chrome doesn't. Could that be the issue there? If our code is adding unnecessary line breaks I think that'd be a blocker, but if it's just the way the HTML is copied from the browser I don't think that should block this PR. Looking into this a bit more, I do see that there are <br> tags in the HTML when pasting on chrome (using the same text in the issue description), so perhaps there is an issue w/ the newline regex causing it to fail in this case?
  • For 3 that sounds like a different issue and something to discuss in #expensify-open-source since I think that will be a bit more involved, so I wouldn't block this PR on that.

Thoughts?

@parasharrajat
Copy link
Member Author

Ok. I agree. Should I add those fixes in this PR?

@Jag96
Copy link
Contributor

Jag96 commented Jul 14, 2021

I think fixes for 1 and 2 should be added to this PR, assuming 2 requires a fix.

@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 14, 2021

@Jag96 Just opened a new PR for 2. Expensify/expensify-common#393. Could be merged quickly and we can move forward here.
1's fix is done here.

@parasharrajat
Copy link
Member Author

Okay, I have updated the E-common here. and 1 & 2 is fixed

@parasharrajat
Copy link
Member Author

Please let me know if anything else is needed here.

@Jag96
Copy link
Contributor

Jag96 commented Jul 14, 2021

1 looks good, for 2 it looks like when I copy HTML that contains a <br> tag it doesn't add the line breaks if the <br> tag has styles. For example, copying the following from #3790:

image

Results in the following html value inside handlePastedHTML

"<meta charset='utf-8'><strong style=\"box-sizing: border-box; font-weight: 600; color: rgb(36, 41, 46); font-family: -apple-system, system-ui, &quot;Segoe UI&quot;, Helvetica, Arial, sans-serif, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;; font-size: 14px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;\">Bold text</strong><br style=\"box-sizing: border-box; color: rgb(36, 41, 46); font-family: -apple-system, system-ui, &quot;Segoe UI&quot;, Helvetica, Arial, sans-serif, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;; font-size: 14px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;\"><em style=\"box-sizing: border-box; color: rgb(36, 41, 46); font-family: -apple-system, system-ui, &quot;Segoe UI&quot;, Helvetica, Arial, sans-serif, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;; font-size: 14px; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;\">italic text</em><br style=\"box-sizing: border-box; color: rgb(36, 41, 46); font-family: -apple-system, system-ui, &quot;Segoe UI&quot;, Helvetica, Arial, sans-serif, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;; font-size: 14px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;\"><del style=\"box-sizing: border-box; color: rgb(36, 41, 46); font-family: -apple-system, system-ui, &quot;Segoe UI&quot;, Helvetica, Arial, sans-serif, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;; font-size: 14px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255);\">Strikethrough text</del>"

Before this PR, pasting the content would include these line breaks, but now it strips them:

Before After
image image

However, this just looks like a bug in the regex and isn't really tied to the core of this PR, so I won't block on it and I'll open a new issue for this.

@Jag96 Jag96 merged commit f1ea424 into Expensify:main Jul 14, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@parasharrajat
Copy link
Member Author

Yeah @Jag96 Do you want me to correct the Br tag regex

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Jag96
Copy link
Contributor

Jag96 commented Jul 14, 2021

@parasharrajat sounds good, feel free to leave a comment on #4052 so I can assign you, I'll invite you to the Upwork job in the meantime

@isagoico
Copy link

isagoico commented Jul 16, 2021

@Jag96 Since this PR is failing because of the issue above, should we block prod deploy on this? or can we check it off the PR list and test this again when the fix is merged? Ignore me on this, I was setting up QA and had not reached #4067 in the list yet 🤦

@Jag96
Copy link
Contributor

Jag96 commented Jul 16, 2021

No worries! Now that #4067 is on staging that'll be the latest fix

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@sonialiap
Copy link
Contributor

An error was identified that was linked to this PR - #9505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants