[HOLD for payment 2023-11-09] [$500] mWeb/Safari- Chat - After tapping on "Copy to clipboard", options menu does not close

[lanitochka17]

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

---

Action Performed:

1. Go to any chat
2. Long press on any message
3. Tap on "Copy to clipboard"

Expected Result:

There should be a message indicating the message has been copied and then the options menu should close

Actual Result:

There is no message indicating that the message has been copied and options menu does not close

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.77-2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6224644_1696427243168.Tppt3240_1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0153aad7aa52da20d4
• Upwork Job ID: 1710335830550470656
• Last Price Increase: 2023-10-13
• Automatic offers:
• cubuspl42 | Reviewer | 27277511
• AmjedNazzal | Contributor | 27277514

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[paultsimura]

Not reproducible on Staging and latest main

[garrettmknight]

I was able to reproduce on staging and prod - It's not the "+" button, it's a long press on any message in a DM that gets you to the options menu for the chat on mWeb.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0153aad7aa52da20d4

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External)

[ChrisCates]

The best option would be to update the menu to use touch events.
More specifically the touchend event.

It's also possible that there may be conditional statements not capturing mobile properly. I will take a deeper dive and verify the source code in:

src/components/CopyTextToClipboard.js
src/components/Pressable/PressableWithDelayToggle.js

Would be more than happy to submit a PR. Let me know if I can work on the project.

[cubuspl42]

@ChrisCates

You're welcome to submit a proposal! Please read the contributing guidelines.

Also, take a quick look at any other external issue to see how the proposal selection process looks in Expensify.

[ChrisCates]

@cubuspl42 I'll prepare a proposal shortly.
I've also not been invited to Slack yet.

Should I wait for that? Or can I submit PRs and everything will be cool?

[ChrisCates]

Proposal

Please re-state the problem that we are trying to solve in this issue

There is an issue with pointer events on mobile for Safari with the chat menu. More specifically the copy to clipboard button is not working properly. We should verify and test all buttons though.

What is the root cause of that problem?

After further review, we should do regression testing on all menu items that utilize:

src/components/CopyTextToClipboard.js
src/components/Pressable/PressableWithDelayToggle.js
src/components/Pressable/GenericPressable/...

What changes do you think we should make in order to solve the problem?

We should investigate an implementation that uses Touch events. We will need to implement this in tandem with existing Pressable This way, if there is a long press, it will still register with the touchend event.

However, this should be only if using regular pointer events does not resolve the issue on Safari. It may be possible to get pointer events to work properly. Will verify if possible.

The following is Mozilla Documentation on Touch Events and how to implement them: https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Using_Touch_Events

We would also need to verify on all web platforms that the new touch events work as intended and do not have any potential negative side effects. We should verify all major web browsers on mobile and web, as well as React Native and Desktop App too.

[cubuspl42]

@ChrisCates

Should I wait for that? Or can I submit PRs and everything will be cool?

From CONTRIBUTING.md:

Refrain from leaving additional comments until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet)

First, precisely 1 PR is expected for most issues, if the proposal is selected.

Second, the PR should be created after the proposal is selected.

You are welcome to create a branch on your fork, though. It's very typical to create a branch that contains a draft of the solution when preparing a proposal. In many cases, such a draft is later a starting point of the PR (if the proposal is selected), which is then reviewed, and we iterate on it.

[cubuspl42]

@ChrisCates Congrats on writing your first proposal! As is often the case with first tries, there's a big room for improvement here. Both crucial sections of the proposal (the root cause analysis and the solution description) are not up to Expensify's standards.

What is the root cause of that problem?

After further review, we should do regression testing

How does "After further review, we should do regression testing..." answer the question "What is the root cause of that problem?"?

What changes do you think we should make in order to solve the problem?

We should investigate...

The investigation is what we typically do when we prepare a proposal. The proposal itself should contain a description of the solution itself. Depending on the scope of the issue, the level of detail varies. For relatively small issues (the majority), it is typical to have the full solution prepared. The proposal then contains a description of that solution.

Please take a look at a dozen other issues to get a feeling of how the Expensify process works. The crucial labels are "External" and "Help wanted". Issues with "Help wanted" are open for new proposals, while the issues without "Help wanted" are typically already being handled.

Here are the "External" issues without "Help wanted" (i.e. mostly the ones that are being handled). Check out the proposals that were selected for inspiration.

[ishpaul777]

Proposal

Please re-state the problem that we are trying to solve in this issue

After tapping on "Copy to clipboard", options menu does not close and "Copy to Clipboard" isn't changed to "Copied"

What is the root cause of that problem?

selection.setBaseAndExtent throws a type error when "Copy to Clipboard" is clicked.
Arguments are passed in setBaseAndExtent are (null, 0, null, 0), I am unsure why this is only producable on production and staging and what changed
On dev enviornment on physical device (both on main branch and production branch) There is no issue everything is working as expected. The issue can only be spotted on production and staging and on emulator(dev) for me

What changes do you think we should make in order to solve the problem?

We should call setBaseAndExtent with valid arguments only , we can have a check for anchorNode focusNode to be not null.
Or we use try, catch like below

            try {
                selection.setBaseAndExtent(originalSelection.anchorNode, originalSelection.anchorOffset, originalSelection.focusNode, originalSelection.focusOffset);
            }
            catch (e) {
               
            }

[ChrisCates]

Hey @cubuspl42 thanks for the feedback. Just came here.
I'll make sure to just add comments and feedback next time. Also I think @ishpaul777 has the better solution. Since, to be honest, I was struggling to reproduce on LAN (iPhone 11 Pro). Why I proposed a migration to Touch Events (assuming artifacts there).

I have a question about this issue. Is this more in line with what follows Expensify's guidelines. And if not? What should change: #28078

[cubuspl42]

@ishpaul777 Please stick to the proposal template. The headers are not meant to be modified. Thank you!

[ishpaul777]

Oh Okay! Updated proposal.

[cubuspl42]

@lanitochka17 @garrettmknight I can't reproduce on staging

[ishpaul777]

Hii @cubuspl42 , I am able to reproduce on staging

trim.045354BB-9842-4811-A1B4-422978BB6A9E.MOV

[garrettmknight]

@cubuspl42 I just reproduced it on staging as well on 1.3.81-6

[cubuspl42]

@ishpaul777

selection.setBaseAndExtent throws a type error

Are there any details available for this error?

we can have a check for anchorNode focusNode to be not null.

Have you analyzed the surrounding code? Do we understand when the mentioned values could be null?

[AmjedNazzal]

@cubuspl42 PR is ready for review. Sorry it took so long, I had some pc issues.

[cubuspl42]

@tgolen Ouch, is it the GitHub autoclosing based on the commit message? 😳

[tgolen]

Oh, hahaha, yes. I'll reopen this.

[cubuspl42]

@tgolen Do we actually use that behavior? If we don't, maybe it should be disabled in the settings?

[tgolen]

I don't think we use it in this repo, no. Do you know where the setting might be? I'm looking at another repo I own and I don't know where to find it.

[cubuspl42]

@tgolen I might've confused it with GitLab. It appears that on GitHub it cannot be disabled 😕

It could be worth adding a PR-level check to prevent close|closes|closed|fixes|fixed #\d+

[tgolen]

Ah, I agree. I confirmed with our infra team that they don't know of a setting like that either. I think we're OK for now. It doesn't happen very often.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix - mWeb/Safari- Chat - After tapping on "Copy to clipboard", options menu does not close #28820 #30085

If no regressions arise, payment will be issued on 2023-11-09. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

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

For reference, here are some details about the assignees on this issue:

• @cubuspl42 requires payment offer (Reviewer)
• @AmjedNazzal requires payment offer (Contributor)

[melvin-bot]

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:

• [@cubuspl42] The PR that introduced the bug has been identified. Link to the PR:
• [@cubuspl42] 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:
• [@cubuspl42] 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:
• [@cubuspl42] Determine if we should create a regression test for this bug.
• [@cubuspl42] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[cubuspl42]

⚠️ Looks like this issue was linked to a Deploy Blocker here

False alarm.

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • N/A; this is not a regression, but an adjustment to the changed behavior of the new Safari version
• 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:
  • N/A
• 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:
  • No need for additional discussion
• Determine if we should create a regression test for this bug.
  • Up to the QA team
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • Launch the application on macOS Safari 17 or higher or Safari on iPhone 15 or higher.
  • Navigate to any chat and right-click or long-press on a chat message to display the context menu.
  • Select 'Copy to clipboard'.
  • Ensure the text is copied successfully (verify this by pasting it somewhere) and the context menu closes after the action.

[AmjedNazzal]

@garrettmknight Gentle nudge for the payment. Thanks! :)

[garrettmknight]

Summary of payments:

• Urgency bonus: No
• Contributor: @AmjedNazzal $500 via Upwork
• Reviewer: @cubuspl42 $500 via Upwork

[garrettmknight]

Since this is really version specific and a pretty edge case, I don't think we need a regression test.