Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.58k stars 2.92k forks source link

[HOLD for payment 2024-11-21] [$250] iOS Invoices - Blank area under composer after returning from workspace page #48677

Closed IuliiaHerets closed 4 days ago

IuliiaHerets commented 2 months ago

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.30-7 Reproducible in staging?: Y Reproducible in production?: N Issue was found when executing this PR: https://github.com/Expensify/App/pull/48014 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  1. Launch New Expensify app.
  2. Open invoice chat as the invoice sender.
  3. Tap on the invoice preview.
  4. Tap on the composer to bring up keyboard.
  5. Tap Add bank account.
  6. Return to the invoice report.

Expected Result:

There will not be a blank area under the composer if keyboard is not up.

Actual Result:

There is a blank area under the composer when keyboard is not up after returning from workspace page.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/46c266b9-7430-47b7-9c46-087a1f54f797

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021831977574854649069
  • Upwork Job ID: 1831977574854649069
  • Last Price Increase: 2024-10-17
  • Automatic offers:
    • QichenZhu | Contributor | 104516887
Issue OwnerCurrent Issue Owner: @muttmuure
melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

github-actions[bot] commented 2 months ago

:wave: 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.
mjasikowski commented 2 months ago

on it

mjasikowski commented 2 months ago

per conversation with @mountiny, demoting this one since it's minor UI issue which can be worked around (the user can tap on the composer field to bring the keyboard back up) and this occurs on a single platform only

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $125

ryalinikhil commented 2 months ago

Modify the view lifecycle methods to handle layout recalculations. Listen to keyboard notifications (UIKeyboardWillShowNotification, UIKeyboardWillHideNotification) to adjust the composer’s frame.

melvin-bot[bot] commented 2 months ago

📣 @ryalinikhil! 📣 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. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. 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>
ryalinikhil commented 2 months ago

Contributor details Your Expensify account email: sameernikhil123@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01af3c66ed0d7d2779?mp

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mjasikowski commented 2 months ago

@ryalinikhil thank you for your submission! Please make you're familiar with our contributor guidelines and submit your proposals with the the proposal template

fedirjh commented 2 months ago

Update: Awaiting on proposals.

melvin-bot[bot] commented 2 months ago

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

mjasikowski commented 2 months ago

Still waiting for proposals

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

fedirjh commented 2 months ago

Update: Awaiting on proposals.

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

fedirjh commented 2 months ago

Same Update

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@mjasikowski @fedirjh @muttmuure 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!

wildan-m commented 2 months ago

Hi, I've got forbidden error when sending invoice.

Asking in slack:

https://expensify.slack.com/archives/C01GTK53T8Q/p1727076209941879

Please inform me if anyone knows how to solve it.

mjasikowski commented 2 months ago

Checking if still reproducable

kavimuru commented 2 months ago

Tester is still able to reproduce.

https://github.com/user-attachments/assets/278d526a-6417-4cf6-8cd9-17cf847ccff4

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

fedirjh commented 2 months ago

Still waiting for proposals

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $250

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

QichenZhu commented 2 months ago

Proposal

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

The keyboard avoiding padding area is visible even when the keyboard is closed.

What is the root cause of that problem?

The KeyboardAvoidingView component is not aware of the keyboard changes while it is unmounted after navigating two screens away.

Here’s what happens:

  1. The KeyboardAvoidingView component subscribes to the keyboard events in componentDidMount.
Screenshot 2024-09-29 at 4 33 05 AM
  1. The keyboard opens.

  2. The component receives the event and updates the padding size.

  3. The component unsubscribes from the keyboard events when it is unmounted after navigating two screens away.

Screenshot 2024-09-29 at 4 32 47 AM
  1. The keyboard closes.

  2. The component doesn’t receive the event because it unsubscribed from it in step 4.

  3. When navigating back, the component is remounted, but the padding size remains unchanged as if the keyboard is still open.

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

There is a related upstream issue https://github.com/facebook/react-native/issues/36287 where the padding area should show but doesn’t, which is the opposite of our issue.

Inspired by PR https://github.com/facebook/react-native/pull/44652, we could add a patch to insert the code below before line 184 of node_modules/react-native/Libraries/Components/Keyboard/KeyboardAvoidingView.js.

if (!Keyboard.isVisible()) {
  this._setBottom(0);
}
Screenshot 2024-10-02 at 11 34 19 PM

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 2 months ago

@mjasikowski, @fedirjh, @muttmuure Still overdue 6 days?! Let's take care of this!

mjasikowski commented 1 month ago

@fedirjh are you available to review this please?

fedirjh commented 1 month ago

Hi @mjasikowski! It's on my list of tasks for today.

fedirjh commented 1 month ago

@QichenZhu Can you please elaborate more on the root cause specifically this point?

  1. The component doesn’t receive the event.

Also, can you replicate this bug in a separate snack?

There is a related upstream https://github.com/facebook/react-native/issues/36287 and a https://github.com/facebook/react-native/pull/44652, but neither has been processed for a long time.

I have applied the patch from that PR, and the bug still persists on our App. Are we sure there aren't any changes required on our side?

QichenZhu commented 1 month ago

@fedirjh Thanks for the review and testing!

Can you please elaborate more on the root cause specifically this point?

The component doesn’t receive the event.

The KeyboardAvoidingView component unsubscribes from the keyboardWillChangeFrame event in the componentWillUnmount hook, so it no longer receives the event.

Screenshot 2024-09-29 at 4 32 47 AM



Also, can you replicate this bug in a separate snack?

I'm trying to create a minimal repro in Snack.


I have applied the patch from that PR, and the bug still persists on our App. Are we sure there aren't any changes required on our side?

https://github.com/facebook/react-native/issues/36287 is related but not the same as our issue, it's actually the opposite (the padding area should show but doesn't). My proposed solution is inspired by but different from the PR https://github.com/facebook/react-native/pull/44652.

QichenZhu commented 1 month ago

Proposal

Updated based on the comments above

QichenZhu commented 1 month ago

@fedirjh Here is a minimal reproducer:

https://github.com/QichenZhu/reproducer-react-native-keyboard-avoiding-view

Reproduction steps

  1. Build and run the iOS app with New Arch enabled.
cd ReproducerApp
yarn
bundle install
cd ios
RCT_NEW_ARCH_ENABLED=1 pod install
cd ..
yarn ios
  1. Click Show Keyboard, and ensure the soft keyboard is visible. If not, enable it in the simulator menu: I/O -> Keyboard -> Toggle Software Keyboard.

  2. Click Freeze, Hide Keyboard, Unfreeze buttons in sequence.

Expected result: No blank area under the grey section.

Actual result: A blank area appears under the grey section.

https://github.com/user-attachments/assets/d6765f8b-e297-4fef-ad4d-c443843663e2

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@mjasikowski @fedirjh @muttmuure this issue is now 4 weeks old, please consider:

Thanks!

mjasikowski commented 1 month ago

@fedirjh over to you again

melvin-bot[bot] commented 1 month ago

@mjasikowski, @fedirjh, @muttmuure Huh... This is 4 days overdue. Who can take care of this?

fedirjh commented 1 month ago

@QichenZhu Your proposal makes sense to me. Can you create an upstream PR to fix it?

QichenZhu commented 1 month ago

@fedirjh Sure thing!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@mjasikowski, @fedirjh, @muttmuure Eep! 4 days overdue now. Issues have feelings too...

fedirjh commented 1 month ago

Not overdue.

@QichenZhu Have you made any progress with the upstream fix? Did you open a pull request for us to follow along?

QichenZhu commented 1 month ago

@fedirjh I opened an upstream issue https://github.com/facebook/react-native/issues/46942 and a PR https://github.com/facebook/react-native/pull/46952. The PR has been approved but not yet merged.

QichenZhu commented 1 month ago

Update: The upstream PR https://github.com/facebook/react-native/pull/46952 was merged.

melvin-bot[bot] commented 1 month ago

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