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] iOS - Split bill - Keyboard displayed abruptly when split bill animation slides in #21394

Closed
1 of 6 tasks
lanitochka17 opened this issue Jun 23, 2023 · 37 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 23, 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!


Issue found when executing PR #21310

Action Performed:

  1. Open iOS app
  2. Open group DM
  3. Set focus to message composer (so keyboard is displayed)
  4. Tap on "+" near composer
  5. Tap on "Split bill"

Expected Result:

Split bill screen animation slides in smoothly with no unnecessary elements displayed

Actual Result:

Keyboard is still displayed for a couple of seconds when Split bill screen animation slides in

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

Bug6103623_IMG_8200.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/~01fd7b79c5ae42e842
  • Upwork Job ID: 1673323311936536576
  • Last Price Increase: 2023-06-26
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 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

@melvin-bot melvin-bot bot added the Overdue label Jun 26, 2023
@CortneyOfstad
Copy link
Contributor

Was able to recreate, as the keyboard lingered. Going to get eyes on this.

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2023
@CortneyOfstad CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Jun 26, 2023
@melvin-bot melvin-bot bot changed the title iOS - Split bill - Keyboard displayed abruptly when split bill animation slides in [$1000] iOS - Split bill - Keyboard displayed abruptly when split bill animation slides in Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

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

melvin-bot bot commented Jun 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

@waterim
Copy link
Contributor

waterim commented Jun 26, 2023

Hi!
I'm Artem from Callstack - expert contributor group
Maybe I can help with this one?

@s77rt
Copy link
Contributor

s77rt commented Jun 26, 2023

@CortneyOfstad Can you please assign @waterim here

@CortneyOfstad
Copy link
Contributor

@s77rt Do you want me to remove your assignment in favor of @waterim, or have you both assigned?

@s77rt
Copy link
Contributor

s77rt commented Jun 26, 2023

@CortneyOfstad Both. I will be the C+ here for review

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

📣 @waterim You have been assigned to this job by @CortneyOfstad!
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 📖

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

📣 @aabantariqmurtaza! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@aabantariqmurtaza
Copy link

Contributor details
Your Expensify account email: aban.softengr@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01ecb1d62beec30401

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@s77rt
Copy link
Contributor

s77rt commented Jun 26, 2023

@aabantariqmurtaza You don't need to access the SO. Just run npm install; cd ios; pod install; cd ..; npm run ios. BTW this issue is not open for proposals (does not have Help Wanted label). If you have any further questions please ask on Slack. If you don't have Slack please read the contributing guide.

@melvin-bot melvin-bot bot added the Overdue label Jun 28, 2023
@s77rt
Copy link
Contributor

s77rt commented Jun 29, 2023

@waterim Any updates here?

@melvin-bot melvin-bot bot removed the Overdue label Jun 29, 2023
@waterim
Copy link
Contributor

waterim commented Jun 29, 2023

@s77rt Found a root of the issue, will add proposal today.

@waterim
Copy link
Contributor

waterim commented Jun 29, 2023

@s77rt Will send it tomorrow, found one issue with random black blink after the navigation

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt I was reviewing Popover animation, everything was correct there. After I decided to try to restart IOS simulator and this black blink just disappeared.

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

Proposal

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

Keyboard displayed abruptly after navigation to split bill screen

What is the root cause of that problem?

The main issue here is a behaviour of Native IOS keyboard. If the keyboard dismissal occurs after the navigation has started, it may result in a momentary blink as the new screen is displayed.
File we are working with: ReportActionCompose.js

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

The best way to fix this issue is to hide Keyboard with Keyboard.dismiss() before the actual Popover with (Split Bill and Add Attachment) was opened. This will add a natural feeling of the application - Keyboard will disappear with a native animation before Popover is opened.

What alternative solutions did you explore?

N/A

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

@waterim Thanks for the proposal. I don't think your RCA is correct/complete. Your provided little info on why the bug is occurring in the first place. For instance why the keyboard is even getting pulled up? Is it because we are re-focusing the input after closing the modal or something else? I think the point that I'm trying to make here is that instead of pulling the keyboard up and then dismiss it, we should not even pull it in the first place.

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt okay, I will add more information to the Proposal.

here is an answer to your question:
original behavior - we are not closing the keyboard itself when we are opening Popover, and after the navigation to the split bill screen we are dismissing the keyboard, thats why it still blinks for a second, because we are closing it only on the split bill screen.
In my proposal Im adding dismiss before even popover opens, with this solution we are getting smooth keyboard close before opening popover(like telegram for example has) and with this solution when we are navigating to a new screen we don’t have any keyboard pulled up before.

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

@waterim After we click on the plus grey button, the keyboard is already dismissed. Why after we click on "Split bill" it appears again?

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt After additional investigation:
"Why after we click on "Split bill" it appears again?"

  1. "grey button" opens a modal from react-native-modal which handles automatically the keyboard. It means it dismisses the keyboard on open and shows the keyboard after it close(if it was opened before we opened the modal itself)
  2. After the click on the button "split bill" we are making a navigation and closing the modal and it means that our keyboard will show up. But on the page Split bill we have a logic to dismiss a keyboard and thats why keyboard will be opened just for a second.
  3. With my solution we are closing the keyboard manually before modal is showing thats why it will not dismiss the keyboard automatically and will not open it again after modal is closed.(because it was closed before we opened the modal)

@CortneyOfstad
Copy link
Contributor

Heading OoO for the week (back July 10) so re-assigning this in the meantime 👍

@CortneyOfstad CortneyOfstad removed their assignment Jun 30, 2023
@CortneyOfstad CortneyOfstad added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jun 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 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

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

@waterim Thanks for the follow up.

"grey button" opens a modal from react-native-modal which handles automatically the keyboard. It means it dismisses the keyboard on open and shows the keyboard after it close(if it was opened before we opened the modal itself)

Can you please back this up with link to code? I know this behaviour exists in RNW for accessibility reasons as part of the FocusTrap implementation. But I don't see any relevant code in react-native-modal. Perhaps this is something coming from RN's <Modal />?

Also, how come this issue is only reproducible in iOS and not in Android?


Feedback: Everytime you make a statement it's really recommended that you link to code supporting that statement e.g. in "But on the page Split bill we have a logic to dismiss a keyboard", it would have been great and much helpful if you linked to that logic. This makes the review process much faster.

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

Sorry @waterim I just realised that this issue may be a dupe of #13922.
@lschurr I think we can close this one.

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt actually with my solution add attachment works perfectly, without any keyboard blinks

And without this abrupt closing of the keyboard

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt thank you for your feedback, will add more details, more relevant code to future proposals

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

@waterim What I meant to say is that we may already be working on fixing this issue. I see we have an open PR #15337 for that.

@waterim
Copy link
Contributor

waterim commented Jun 30, 2023

@s77rt okay, sure)
Just one suggestion to add a native animation for a keyboard dismiss after clicking on the “+” button, it will feel more natural and native

@s77rt
Copy link
Contributor

s77rt commented Jun 30, 2023

@waterim Thanks, this feels like an improvement. I think we should let the assignees handle the bug fix first then we can discuss such animations in Slack.

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@lschurr
Copy link
Contributor

lschurr commented Jul 3, 2023

Just to confirm - should this one be closed for now @s77rt?

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@s77rt
Copy link
Contributor

s77rt commented Jul 3, 2023

@lschurr Yes

@lschurr lschurr closed this as completed Jul 5, 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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

6 participants