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 2022-12-08] [$1000] Incorrect pasted text on copied HTML snippet reported by @kerupuksambel #12271

Closed
kavimuru opened this issue Oct 28, 2022 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Oct 28, 2022

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. Open staging.new.expensify.com
  2. Send this HTML snippet to someone : <a href="takia.html">asd</a>
  3. Right click on the message and select the option "Copy to clip board" and paste it to either an external text editor or Expensify textbox

Expected Result:

The pasted text is the same as the copied text

Actual Result:

The pasted text on the external text editor is only asd while the pasted text on the Expensify textbox is [asd](https://staging.new.expensify.com/r/takia.html)

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.21-4

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:

Wrong.Snippet.mp4
Recording.812.mp4

Expensify/Expensify Issue URL:

Issue reported by: @kerupuksambel

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666969406840619

View all open jobs on GitHub

Upwork Automation - Do Not Edit

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 28, 2022

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Overdue label Oct 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 1, 2022

@Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Christinadobrzyn
Copy link
Contributor

Sorry I was ooo, I can't replicate this. Here's what I tried:

  1. Copy/paste <a href="takia.html">asd</a> in a chat in staging
  2. Click the Copy to Clipboard icon
  3. Open TextEdit and paste

2022-11-02_09-33-19 (1)

Side note, I can see this message in my notifications

image

@melvin-bot melvin-bot bot removed the Overdue label Nov 2, 2022
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 2, 2022

@kavimuru can you try to reproduce this again to see what we are doing differently? Feel free to assign this back to me if you can reproduce again.

@kerupuksambel
Copy link

kerupuksambel commented Nov 2, 2022

@Christinadobrzyn May I try to elaborate? My step to generate the error is :

  1. Type <a href="takia.html">asd</a> in a chat in staging
  2. Copy the code with highlighting the code and pressing Ctrl+C / Cmd+C
  3. Paste it with Ctrl+V / Cmd + V on your text editor.

The bug is remained the same in my side. I also just realized the difference between copying the code from Copy To Clipboard button and using Ctrl/Cmd + C.

wronggg

@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

@kavimuru Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Nov 7, 2022
@kavimuru kavimuru removed their assignment Nov 7, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 7, 2022
@Christinadobrzyn
Copy link
Contributor

Hi @kerupuksambel! Thank you so much for the insight - testing this:

Using Copy to Clipboard

  1. Type <a href="takia.html">asd</a> in a chat in staging
  2. Copy the code by clicking the Copy to Clipboard button
  3. Paste it with Ctrl+V / Cmd + V on your text editor.
  4. You get <a href="takia.html">asd</a> in text editor

Using Ctrl+C / Cmd+C

  1. Type <a href="takia.html">asd</a> in a chat in staging
  2. Copy the code with highlighting the code and pressing Ctrl+C / Cmd+C
  3. Paste it with Ctrl+V / Cmd + V on your text editor.
  4. You get asd in text editor

So the Copy to Clipboard is pasting the <a href="takia.html">asd</a> code into text edit whereas the Ctrl+C / Cmd+C & Ctrl+V / Cmd + V is pasting the result of the code asd. Interesting!

I think this might be similar to this issue #10262 - @parasharrajat would you be able to take a peek at this and see if you think it will be fixed with the changes you're working on in #10262

@parasharrajat
Copy link
Member

parasharrajat commented Nov 10, 2022

Investigated this one, it seems like root cause might be same. We are tackling the SetHTML function in the other issue. It is good to know one more bug caused by the same navigator.clipboard.write.

@b1tjoy This might be related to #10262.

@Christinadobrzyn
Copy link
Contributor

Thanks @parasharrajat, I think we should probably hold this until #10262 is resolved to see if that fixes this issue. Let me know if there's a better option.

@Christinadobrzyn Christinadobrzyn changed the title Incorrect pasted text on copied HTML snippet reported by @kerupuksambel [HOLD] Incorrect pasted text on copied HTML snippet reported by @kerupuksambel Nov 10, 2022
@Christinadobrzyn Christinadobrzyn added Weekly KSv2 and removed Daily KSv2 labels Nov 10, 2022
@parasharrajat
Copy link
Member

Good call @Christinadobrzyn.

@b1tjoy
Copy link
Contributor

b1tjoy commented Nov 12, 2022

This one is a regression of PR #11905, ExpensiMark definitely expects encoded html string as parameter.

https://github.com/Expensify/expensify-common/blob/490d695c8ceb54bcff736785b54f06057c32fc9b/lib/ExpensiMark.js#L479

Solution fix both current issue and #11693

Remove the code which encode the selection html.

const newHtml = Str.htmlDecode(render(domRepresentation));

-    const newHtml = Str.htmlDecode(render(domRepresentation));
+    const newHtml = render(domRepresentation);

Set decoded html to clipboard as text/plain type.

Clipboard.setHtml(selection, parser.htmlToText(selection));

-        Clipboard.setHtml(selection, parser.htmlToText(selection));
+        Clipboard.setHtml(selection, Str.htmlDecode(parser.htmlToText(selection)));

Clipboard.setHtml(content, parser.htmlToText(content));

-                         Clipboard.setHtml(content, parser.htmlToText(content));
+                         Clipboard.setHtml(content, Str.htmlDecode(parser.htmlToText(content)));

You may noticed that we found incorrect use of htmlToText() in #10262 (comment), I suggest we fix both #11693 and #12271 in solution of #10262.

Screenshot

macOS-Chrome-2022-11-12-122510.mp4

@deetergp deetergp removed their assignment Nov 28, 2022
@JmillsExpensify
Copy link

@thesahindia Can you also please assign @b1tjoy to this issue. Thanks!

@b1tjoy
Copy link
Contributor

b1tjoy commented Nov 28, 2022

Please review PR #13108, thanks!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

📣 @b1tjoy You have been assigned to this job by @Christinadobrzyn!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 29, 2022

Thanks @thesahindia! I hired:

job posting here - https://www.upwork.com/jobs/~015b92dc642f298147

Looks like @b1tjoy is eligible for the 50% merge bonus!

@Christinadobrzyn Christinadobrzyn added Weekly KSv2 and removed Daily KSv2 labels Nov 29, 2022
@JmillsExpensify
Copy link

2 more days before we exit the 50% bonus period for merged PRs!

@thesahindia
Copy link
Member

The PR got merged.

@JmillsExpensify
Copy link

Oh whoops, I'm flying through too many issues. Thanks!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 1, 2022
@melvin-bot melvin-bot bot changed the title [$1000] Incorrect pasted text on copied HTML snippet reported by @kerupuksambel [HOLD for payment 2022-12-08] [$1000] Incorrect pasted text on copied HTML snippet reported by @kerupuksambel Dec 1, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 1, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.34-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-12-08. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Dec 1, 2022

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:

  • [@thesahindia / @danieldoglas] The PR that introduced the bug has been identified. Link to the PR:
  • [@thesahindia / @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:
  • [@thesahindia / @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:
  • [@Christinadobrzyn] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here Slack chat about this here

@thesahindia
Copy link
Member

The PR that introduced the bug has been identified. Link to the PR:

As mentioned by @b1tjoy, it was a regression from #11905

@thesahindia
Copy link
Member

@Christinadobrzyn, this is ready for payment

@Christinadobrzyn
Copy link
Contributor

So sorry for the payment delay, I wasn't sure if there was a regression and I was ooo for a bit. Paid and closed the job.

@b1tjoy $1000 for the fix & $500 bonus (PR in 3 days)
@thesahindia $1000 as C+ & $500 bonus (PR in 3 days)
@kerupuksambel $250 for reporting

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Dec 19, 2022

@thesahindia @danieldoglas can you please complete the checkboxes here and feel close this GH when you're done!

#12271 (comment)

@Christinadobrzyn Christinadobrzyn removed their assignment Dec 19, 2022
@Christinadobrzyn Christinadobrzyn removed the Awaiting Payment Auto-added when associated PR is deployed to production label Dec 19, 2022
@thesahindia
Copy link
Member

The PR that introduced the bug has been identified. Link to the PR:

#11905

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:

#11905 (comment)

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:

https://expensify.slack.com/archives/C049HHMV9SM/p1671474832197629

A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here Slack chat about this here

Need help with this one @danieldoglas

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2022
@danieldoglas
Copy link
Contributor

Closing this since regression test was created already

@melvin-bot melvin-bot bot removed the Overdue label Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests