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

[$250] Bank account - Date of birth page is not full screen if keyboard is not previously dismissed #46745

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 2, 2024 · 16 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 2, 2024

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: 9.0.16-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4806926
Email or phone of affected tester (no customers): applausetester+vd_ios080224.2@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: user must have created a workspace and have enabled Workflows

  1. Open New Expensify app
  2. Go to Settings > Workspaces > Workspace > Workflows
  3. Tap on "Connect bank account"
  4. Select "Connect manually"
  5. Enter testing routing number and account number
  6. Enter first name
  7. Enter last name, but do not dismiss the keyboard
  8. Tap on "Next" button

Expected Result:

Date of birth page should be displayed in full screen

Actual Result:

Date of birth page is not displayed on full screen. If the user goes back, all of the app screens are not displayed on full screen

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

Bug6560422_1722621917080.Mmcn3431_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01808a7973a2a6bf8a
  • Upwork Job ID: 1819450520815861387
  • Last Price Increase: 2024-08-02
  • Automatic offers:
    • ishpaul777 | Contributor | 103375078
Issue OwnerCurrent Issue Owner: @dukenv0307
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Aug 2, 2024

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

@Beamanator Beamanator added External Added to denote the issue can be worked on by a contributor and removed DeployBlocker Indicates it should block deploying the API labels Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01808a7973a2a6bf8a

@melvin-bot melvin-bot bot changed the title Bank account - Date of birth page is not full screen if keyboard is not previously dismissed [$250] Bank account - Date of birth page is not full screen if keyboard is not previously dismissed Aug 2, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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

@Beamanator
Copy link
Contributor

This could also be related to #46513 maybe 🤔

@Beamanator
Copy link
Contributor

I'll work on a revert PR to see if that fixes 2 blockers

@Beamanator Beamanator removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Aug 2, 2024
@ishpaul777
Copy link
Contributor

Confirmed this is fixed with revert

Screen.Recording.2024-08-03.at.12.52.38.AM.mov

@Beamanator Beamanator assigned ishpaul777 and unassigned dukenv0307 Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@Beamanator
Copy link
Contributor

Amazing, mind checking if #46744 is fixed too @ishpaul777 ? 🙏

@ishpaul777
Copy link
Contributor

Fixed

Screen.Recording.2024-08-03.at.12.57.19.AM.mov

@lanitochka17
Copy link
Author

Hello
The team found another bug, is it the same problem or a new one?

46353.ios.1.mp4

@Beamanator
Copy link
Contributor

@lanitochka17 ooh that sounds veryyyyy similar to #46743, so the fix that is being deployed to staging should hopefully fix that too 🙏

@Beamanator Beamanator removed the DeployBlockerCash This issue or pull request should block deployment label Aug 2, 2024
@Beamanator
Copy link
Contributor

Confirmed fixed in staging!

@kirillzyusko
Copy link
Contributor

Enter testing routing number and account number

@lanitochka17 may I ask you to give me these numbers? 😅 I need them to verify that in a new fix for #44514 that issues doesn't happen anymore 🙈

@ishpaul777
Copy link
Contributor

@kirillzyusko see - https://expensify.slack.com/archives/C01GTK53T8Q/p1711049879962529

@kirillzyusko
Copy link
Contributor

Thank you @ishpaul777 👍

kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this issue Aug 8, 2024
…om space (#539)

## 📜 Description

Fixed a case when `KeyboardAvoidingView` is not getting resized back
when keyboard is hidden instantly.

## 💡 Motivation and Context

The condition:

```tsx
if (e.duration === 0) {
  progress.value = e.progress;
  height.value = e.height;
}
```

prevented view to restore its initial position when keyboard is hidden
instantly. When such thing happens we have next events:

```bash
 LOG  onStart {"duration": 250, "eventName": "onKeyboardMoveStart", "height": 0, "progress": 0, "target": 7054} 1723043957135
 LOG  onEnd {"duration": 250, "eventName": "onKeyboardMoveEnd", "height": 0, "progress": 0, "target": -1} 1723043957142
```

So technically we had to set our `height`/`progress` to `0`. But we skip
it, because `event` actually has non-zero duration (it's 250ms).

We can't simply remove the code that I added before because it'll bring
old bug back, so I was thinking a lot on how to resolve the problem.

### 1️⃣ Add more conditions

The first idea was to modify condition to be like

```tsx
if (e.duration === 0 || e.target === -1) {}
```

However I don't like it because:
- it adds more complexity to the code;
- we have to remember about that case and use this condition in every
hook;
- this code doesn't feel cross-platform;
- I'm not sure if it's a correct fix, because we may have a new
situation, where this condition will lead to unexpected results;

So I started to think about other approaches.

### 2️⃣ Native code modification

At some of point of time I realized, that we expect `onEnd` to report an
actual keyboard height that is currently visible on the screen (at least
in `keyboardDidAppear` method). And if we rely on this fact then we can
remove the condition from `onEnd` handler.

So I decided to try to re-work the code and instead of re-forwarding the
data from a notification I decided to grab a real data from the
`keyboardVIew` (its real coordinates) and send them.

Turned out that this fixes old bug and doesn't introduce new one, so I
think I'll stick with this approach. Also all e2e tests passes without
any modification and I think it's a good sign that I do everything
properly 😅

> [!NOTE]
> It's also safe to read frame inside this particular handler, because
we know, that keyboard stopped its movement and has a final frame, so we
don't need to predict next frame based on a current one etc. - we can
simply read frame data and be sure it's exact number what user sees on
the screen.

A new fix for
#327

Also fixes: Expensify/App#46743 and
Expensify/App#46745

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### JS

- removed `if (e.duration === 0)` condition in `KeyboardAvoidingView`
hook;

### iOS

- added new `framePositionInWindow` extension to `UIView`;
- take an actual keyboard height (based on `keyboardView`) in
`keyboardDidAppear` method.

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro (iOS 17.4).

## 📸 Screenshots (if appropriate):

|Before|After|
|-------|-----|
|<video
src="https://github.com/user-attachments/assets/19b3ac39-bd8e-4d47-bc34-a6ba7f3478b3">|<video
src="https://github.com/user-attachments/assets/f7492e25-ccaa-42f0-870a-60eafe912a7d">|

I also checked that
#327
is not reproducible anymore.

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants