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

[$500] IOS - The app flashes after selecting 'Copy to Clipboard' #30706

Closed
6 tasks
lanitochka17 opened this issue Nov 1, 2023 · 28 comments
Closed
6 tasks

[$500] IOS - The app flashes after selecting 'Copy to Clipboard' #30706

lanitochka17 opened this issue Nov 1, 2023 · 28 comments
Assignees
Labels
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

@lanitochka17
Copy link

lanitochka17 commented Nov 1, 2023

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


Version Number: 1.3.94.0
**Reproducible in staging?:**Y
**Reproducible in production?:**n/a
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
Expensify/Expensify Issue URL:
**Issue reported by:**Applause - Internal Team
Slack conversation:

Issue found when executing PR #30085

Action Performed:

  1. Open the app in either MacOS Safari v17 and above or Safari on iPhone 15 and above
  2. Go to any chat and right click/Long press on a chat message to view the context menu
  3. Click/Press on Copy to clipboard

Expected Result:

The app does not flash after selecting 'Copy to Clipboard'

Actual Result:

The app flashes after selecting 'Copy to Clipboard'

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6259491_1698849945657.Screen_Recording_20231101_102829_New_Expensify__2_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d6cd889f3d0aa3a9
  • Upwork Job ID: 1719740024722116608
  • Last Price Increase: 2023-11-22
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 1, 2023
@melvin-bot melvin-bot bot changed the title IOS - The app flashes after selecting 'Copy to Clipboard' [$500] IOS - The app flashes after selecting 'Copy to Clipboard' Nov 1, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01d6cd889f3d0aa3a9

Copy link

melvin-bot bot commented Nov 1, 2023

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

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

melvin-bot bot commented Nov 1, 2023

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

Copy link

melvin-bot bot commented Nov 1, 2023

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

@yh-0218
Copy link
Contributor

yh-0218 commented Nov 1, 2023

Proposal

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

IOS - The app flashes after selecting 'Copy to Clipboard'

What is the root cause of that problem?

This is visible effect of Modal disappearing.
Now we have Modal hide time issue on iOS and Android.
This make above issue

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

We need to fix Mobile animation issue when close Modal.

Then that effect will not appear flashes
Slow hide of Modal
This works well on iOS and Android

What alternative solutions did you explore? (Optional)

Screen.Recording.2023-11-01.at.9.27.34.PM.mov

@ikevin127
Copy link
Contributor

I was not able to reproduce this on the latest iPhone 15 models or simulators, this might only be happening on older devices due to performance.

Proposal

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

After selecting 'Copy to Clipboard' on a chat message via the context menu, there's a flash when the popup closes.

What is the root cause of that problem?

This was an issue of react-native-modal: react-native-modal/react-native-modal#92 for when using the modal prop useNativeDriver={true} on native which we currently do.

The solution was they added the hideModalContentWhileAnimating prop which was the chosen workaround implementation for this issue.

This is already used within our codebase, for example in the YearPickerModal to address similar performance issues:

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

Add the hideModalContentWhileAnimating (true) prop to PopoverReportActionContextMenu -> PopoverWithMeasuredContent component.

Videos

iOS: Native
Screen.Recording.2023-11-03.at.14.59.45.mov

@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
Copy link

melvin-bot bot commented Nov 7, 2023

@bfitzexpensify, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

@bfitzexpensify
Copy link
Contributor

Either of those proposals look good @aimane-chnaif?

@melvin-bot melvin-bot bot removed the Overdue label Nov 7, 2023
@aimane-chnaif
Copy link
Contributor

@bfitzexpensify I'd like to confirm the bug here. "flash" means "no animation"?
So Expected result is: close modal with animation?

@bfitzexpensify
Copy link
Contributor

@aimane-chnaif - yes, that sounds correct to me

@ikevin127
Copy link
Contributor

I have to disagree, if we look at the OP video frame by frame we can see that the context menu closes before the backdrop does which cause that "flash" visual:

iOS: Native - Frame by frame "flash"
Screen.Recording.2023-11-08.at.16.07.49.mov

This is what I think the issue refers to, otherwise it would've said something along the lines of: "no animation when context menu closes".

Otherwise, allowing the context menu to animate-out does fix the visual issue but that will also add the same animation upon clicking on any other context menu item when the menu closes: Reply in thread, Subscribe to thread, Copy link, Mark as unread and Flag as offensive.

Which I'm not sure we want to do here because some of the items in the menu have different closing behaviour: in ContextMenuActions some delay the hideContextMenu and some don't.

If something changed and a transition-out animation is required when closing the menu no matter the menu item pressed, then dismiss this comment.

Copy link

melvin-bot bot commented Nov 8, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@ishpaul777
Copy link
Contributor

ishpaul777 commented Nov 8, 2023

Proposal

Problem

The app flashes after selecting 'Copy to Clipboard'

Root Cause

The flash occurs because we intentionally set animationOutTiming={1} to have an animation out timing of only 1 millisecond. This change was made as part of this PR approximately two years ago, with the aim of optimizing chat switching performance.

Proposed Change

It would be more appropriate to retain animationOutTiming={1} only for large screens. Chat switching performance is not a concern when the popover is open for full-screen modals, as no user interactions can take place during this time. However, instead of a flash, which is detrimental to the user experience, we should consider using a smoother animation to improve the overall UX.

Screen.Recording.2023-11-08.at.10.14.29.PM.mov

@bfitzexpensify
Copy link
Contributor

I have to disagree, if we look at the OP video frame by frame we can see that the context menu closes before the backdrop does which cause that "flash" visual:

This is what I think the issue refers to, otherwise it would've said something along the lines of: "no animation when context menu closes".

Good call @ikevin127 - I went frame-by-frame and I agree with this

Otherwise, allowing the context menu to animate-out does fix the visual issue but that will also add the same animation upon clicking on any other context menu item when the menu closes: Reply in thread, Subscribe to thread, Copy link, Mark as unread and Flag as offensive.

Which I'm not sure we want to do here because some of the items in the menu have different closing behaviour: in ContextMenuActions some delay the hideContextMenu and some don't.

Also agree, I don't think we want to have the same animation across all actions

@melvin-bot melvin-bot bot added the Overdue label Nov 13, 2023
Copy link

melvin-bot bot commented Nov 13, 2023

@bfitzexpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Nov 13, 2023

@bfitzexpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@bfitzexpensify
Copy link
Contributor

How do these proposals look @aimane-chnaif?

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2023
Copy link

melvin-bot bot commented Nov 15, 2023

@bfitzexpensify @aimane-chnaif this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@aimane-chnaif
Copy link
Contributor

reviewing now

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Nov 15, 2023

@lanitochka17 can you please clarify what "flash" means here?
And I see that "IOS" is stated in issue title but attached video is android. Does it happen on native only or mWeb as well?

Copy link

melvin-bot bot commented Nov 15, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@bfitzexpensify
Copy link
Contributor

Bump on the questions in #30706 (comment) @lanitochka17 - thank you!

@yh-0218
Copy link
Contributor

yh-0218 commented Nov 17, 2023

hi, @bfitzexpensify how do you think about my proposal?

@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2023
@bfitzexpensify
Copy link
Contributor

Hi @lanitochka17, clarifying questions:

  1. can you please clarify what "flash" means here?
  2. "IOS" is stated in issue title but attached video is android. Does it happen on native only or mWeb as well?

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

@bfitzexpensify @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

Copy link

melvin-bot bot commented Nov 22, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added the Overdue label Nov 23, 2023
@bfitzexpensify
Copy link
Contributor

OK, considering this has been identifed as not reproducible, I think we can close this for the moment.

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

7 participants