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

[$1000] [HOLD for payment 2023-07-24] [HOLD for payment 2023-07-21] Chat - Copy to clipboard does not copy with format #22833

Closed
3 of 6 tasks
kbecciv opened this issue Jul 13, 2023 · 43 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kbecciv
Copy link

kbecciv commented Jul 13, 2023

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 the app
  2. Open any report
  3. Send message with styling characters eg: # testing
  4. Hover on click on copy to clipboard
  5. Paste in compose box and observe that it does not copy text with format i.e. if same message send as step 3 then it will paste 'testing' in place of '# testing'

Expected Result:

App should copy text with formatting on click of copy to clipboard

Actual Result:

App does not copy text with formatting on click of copy to clipboard

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.40-4
Reproducible in staging?: y
Reproducible in production?: n
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: Any additional supporting documentation

copy.to.clipboard.without.format.mp4
Recording.3604.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0109720246c50a4eba
  • Upwork Job ID: 1683456836334284800
  • Last Price Increase: 2023-07-24
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jul 13, 2023
@kbecciv kbecciv changed the title Web - Copy to clipboard does not copy with format Chat - Copy to clipboard does not copy with format Jul 13, 2023
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

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

@francoisl
Copy link
Contributor

francoisl commented Jul 13, 2023

I can reproduce on staging as well. @robertKozik I think the issue is coming from this PR, any idea how to fix it?

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jul 13, 2023

@francoisl It's not related to that, as it never hit production NVM Will wait @robertKozik

@francoisl
Copy link
Contributor

Yeah, removing this new block solves the issue for me:

if (['INPUT', 'TEXTAREA'].includes(event.target.nodeName)) {
    return;
}

... but it also reintroduces this other issue 🙃

@mountiny
Copy link
Contributor

I think the previous issue is less serious than this one so reverting would make sense to me

@Santhosh-Sellavel
Copy link
Collaborator

Yeah, Let's revert it!

@robertKozik
Copy link
Contributor

Sorry I was unavailable for a while. Yeah let's revert this if needed. I wasn't aware of such a consequences of that fix

@ahmdshrif
Copy link
Contributor

hello, I've got a solution ready that addresses both problems. If you're open to Proposals, I'm available to assist
@francoisl @mountiny

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Jul 13, 2023

Proposal

Problem:

Issue #22803: The first problem occurs when we past in the edit comment composer, it gets pasted into the main composer.
the fix of this issue causes the pasted text to lose its formatting .

Root Cause:

We changed the event listener from the text input to the document, resulting in any paste action focusing on the main composer and adding text to it. This change was made to fulfill certain requirements mentioned in this PR: #22817.

To address the first issue, we implemented a check to determine if any textarea or text input is currently focused. If so, we stop our handler and allow the default paste action to occur without formatting. However, this solution introduced a current issue .

Additionally, checking isFocused as mentioned in issue #22845 will exacerbate the main issue discussed in PR #22817.

Solution:

We will add two paste listeners:

  1. One on the text input, which will handle pasting into the text input field.
  2. Another on the document, where we will exit early if any text input is currently focused.

With these changes, when pasting without any focused text input, the document's textInput event will function properly. However, if there is a focused text input, only the specific paste event for that input will work, and the document listener will be skipped.

Here are the proposed changes: Ready to merge

Result :

Screen.Recording.2023-07-14.at.12.40.32.AM.mov

cc: @francoisl

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 13, 2023
@Santhosh-Sellavel
Copy link
Collaborator

Didn't work and multiple event listeners for the same event in a component. Bad!

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Jul 13, 2023

hello @Santhosh-Sellavel do you test it ?? I test it and attach a video if you don't.

multiple event listeners for the same event in a component. Bad!

it's the same event and we return early so only one will work.

I hope you confirm which case does not work for you.

@Santhosh-Sellavel
Copy link
Collaborator

Seems you updated the changes after posting. We tested it earlier

@Santhosh-Sellavel
Copy link
Collaborator

But it has another issue cursor gets misplaced and still not opting due
"multiple event listeners for the same event in a component"

Screen.Recording.2023-07-14.at.3.29.43.AM.mov

@robertKozik
Copy link
Contributor

robertKozik commented Jul 13, 2023

@Santhosh-Sellavel I think the cursor misplacement is whole new issue not related with this. The ReportActionMessageEdit's composer doesn't have shouldCalculateCaretPosition prop set up (it's present in ReportActionCompose's Composer). After adding this prop, caret is placed correctly NVM, something cached on my side

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Jul 18, 2023
@melvin-bot

This comment was marked as outdated.

@francoisl
Copy link
Contributor

Let's close this issue and keep the discussion in #22803

@joekaufmanexpensify we'll just need to compensate @Santhosh-Sellavel for reviewing the internal fix PR #22845

@joekaufmanexpensify
Copy link
Contributor

@francoisl Sounds good! Just to clarify, you mean don't complete the BZ checklist here (as this was a regression of the other issue). And then just pay @Santhosh-Sellavel for reviewing the PR fix here (once the 7 day regression period is up on 2023-07-21?

If so, that sounds good to me!

@francoisl
Copy link
Contributor

Correct, no need to complete the BZ checklist.

@joekaufmanexpensify
Copy link
Contributor

Cool cool, thanks! Going to set this back to weekly for now. And then we'll pay Santhosh $1,000 for reviewing on 2023-07-01!

@joekaufmanexpensify
Copy link
Contributor

@Santhosh-Sellavel when you have a sec, could you please request $1,000 for this issue, and leave a comment here once you've submitted your request?

@Santhosh-Sellavel
Copy link
Collaborator

Requested on ND

@anmurali
Copy link

Approved

@dhanashree-sawant
Copy link

Hi @francoisl, @joekaufmanexpensify is it eligible for reporting bonus?

@melvin-bot melvin-bot bot added the Overdue label Jul 24, 2023
@joekaufmanexpensify
Copy link
Contributor

Ah, yep thanks for pointing that out @dhanashree-sawant . This is eligible for a reporting bonus.

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2023
@joekaufmanexpensify
Copy link
Contributor

Adding external label to create upwork job

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Jul 24, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-07-24] [HOLD for payment 2023-07-21] Chat - Copy to clipboard does not copy with format [$1000] [HOLD for payment 2023-07-24] [HOLD for payment 2023-07-21] Chat - Copy to clipboard does not copy with format Jul 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0109720246c50a4eba

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

Current assignee @Santhosh-Sellavel is eligible for the External assigner, not assigning anyone new.

@joekaufmanexpensify
Copy link
Contributor

@dhanashree-sawant offer sent for $250!

@joekaufmanexpensify
Copy link
Contributor

@dhanashree-sawant $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

All payment is issued, closing as this is all set. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests