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
4.02k stars 3.01k forks source link

[$250] Hybrid - Android - Attachments - Keyboard overlaps composer after sending a password protected PDF. #56358

Open IuliiaHerets opened 5 days ago

IuliiaHerets commented 5 days 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.94-0 Reproducible in staging?: Yes Reproducible in production?: No If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: No, reproducible on hybrid only Email or phone of affected tester (no customers): ibellicotest+231@gmail.com Issue reported by: Applause Internal Team Device used: Motorola MotoG60 - Android 12 App Component: Chat Report View

Action Performed:

Prerequisite: Last message before sending the PDF, has to be an image.

  1. Open the Expensify app.
  2. Open any chat.
  3. Tap on the "+" button and select "Add Attachment"
  4. Tap on "Choose File"
  5. Select a password protected PDF.
  6. On PDF preview, tap on "Enter Password"
  7. Enter the correct password and tap on "Confirm"
  8. Once redirected to PDF, tap on "Send"
  9. Check keyboard behaviour after sending the PDF to chat.

Expected Result:

Keyboard shouldn´t be opened above the composer after sending the PDF to chat.

Actual Result:

Keyboard is displayed overlapping the composer after sending a Password protected PDF to chat.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/376dab44-bbbb-4fbb-b60a-c77428eab917

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021886894165219110202
  • Upwork Job ID: 1886894165219110202
  • Last Price Increase: 2025-02-04
Issue OwnerCurrent Issue Owner: @shubham1206agra
melvin-bot[bot] commented 5 days ago

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

melvin-bot[bot] commented 5 days ago

Triggered auto assignment to @JmillsExpensify (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.

melvin-bot[bot] commented 5 days ago

💬 A slack conversation has been started in #expensify-open-source

github-actions[bot] commented 5 days 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.
arosiclair commented 5 days ago

Did NOT reproduce this in prod v9.0.91-2

arosiclair commented 5 days ago

Did not reproduce in dev with the latest on main either

arosiclair commented 5 days ago

Couldn't reproduce on staging v9.0.94-1. When I try to attach a password protected PDF I get an error:

arosiclair commented 5 days ago

This is not really reproducible at the moment and relatively minor so I'll just demote this and export to see if we have any good proposals externally.

melvin-bot[bot] commented 5 days ago

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

melvin-bot[bot] commented 5 days ago

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

bernhardoj commented 5 days ago

~I think this won't be an issue anymore after we prevent refocusing the input here.~ I was wrong. The composer is still refocused even after removing the refocus logic.

I can repro

@arosiclair @shubham1206agra I found the root cause of this issue. Do we want to fix this issue first before removing the refocus logic in https://github.com/Expensify/App/issues/55485 or just ignore this issue?

rohit9625 commented 5 days ago

Proposal

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

Keyboard is displayed overlapping the composer after sending a Password protected PDF to chat.

What is the root cause of that problem?

The shouldEnableKeyboardAvoidingView becomes false when performing steps as described in the GH issue. The isComposer={true} and showSoftInputOnFocus={false}, when we navigated back to ReportScreen after sending the attachment, resulting in (!isComposerFocus || showSoftInputOnFocus) becomes false. Hence, shouldEnableKeyboardAvoidingView={false}. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/home/ReportScreen.tsx#L797

https://github.com/user-attachments/assets/c9a4a581-3409-45d8-b059-b9212fad70dd

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

The prop shouldEnableKeyboardAvoidingView shouldn't be calculated based on the isComposerFocus and showSoftInputOnFocus props. Instead, it should be like this:

shouldEnableKeyboardAvoidingView={(isTopMostReportId || isInNarrowPaneModal)}

https://github.com/user-attachments/assets/ea7bd415-44d3-426e-b7cd-709836f8c05e

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None, because it's a UI-related issue.

What alternative solutions did you explore? (Optional)

Alternatively, we can change the showSoftInputOnFocus prop's initial value from false to true, because the keyboard must be shown when focusing the composer on native platforms. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/home/ReportScreen.tsx#L314

rohit9625 commented 5 days ago

What is the root cause of this issue @bernhardoj? Is that is different from the one I specified in my proposal?

shubham1206agra commented 5 days ago

@bernhardoj Can you please write a proposal here?

rohit9625 commented 5 days ago

Proposal

Updated

Added screencasts showing and solving the issue.

github-actions[bot] commented 5 days ago

⚠️ @rohit9625 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

bernhardoj commented 5 days ago

Oh, @rohit9625 already gets the solution, but I would even further remove the whole showSoftInputOnFocus logic.

Not sure if I should write another proposal since @rohit9625 already got it, but the logic was added in https://github.com/Expensify/App/pull/51172. It was added to always auto-focus the composer for the first time, but not showing the keyboard, which is why showSoftInputOnFocus is initially false.

It will only be true when we touch the composer. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L825-L835

This is only reproducible if we send the attachment while never touching the composer yet, so when the composer is refocused programmatically, the keyboard avoiding view is still disabled.

Since we don't want the auto-focus behavior anymore in https://github.com/Expensify/App/issues/55485, I think we can remove the showSoftInputOnFocus logic. (we will basically partly revert https://github.com/Expensify/App/pull/51172)

rohit9625 commented 5 days ago

It will only be true when we touch the composer.

Yes, and that's why we have to tap on composer twice to open the keyboard. It's like a bad UX, right? I've mentioned this issue on this comment.

shubham1206agra commented 5 days ago

@bernhardoj @rohit9625 I am still confused why this was a deploy blocker. Did we changed something that made this bug reproducible?

bernhardoj commented 5 days ago

Oh, I just realized too if showSoftInputOnFocus is false, then the keyboard shouldn't show at all. Removing the KeyboardAvoidingView here will fix it (not sure why though and not sure why it only happens now). https://github.com/Expensify/App/blob/4bf233e7bfed42ec154e7645f935ad93ed1af81b/src/components/PDFView/index.native.tsx#L155-L165

note: I don't think we need the KeyboardAvoidingView anyway since the send button is always hidden when the keyboard is visible, but I still suggest to just remove the showSoftInputOnFocus logic.

shubham1206agra commented 4 days ago

@bernhardoj Would you like to put up a formal proposal here?

bernhardoj commented 4 days ago

Proposal

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

Keyboard doesn't push the content above it after sending a password-protected PDF.

What is the root cause of that problem?

To push content/component above the keyboard, we need a keyboard avoiding view. https://github.com/Expensify/App/blob/a8daefb0570311171c7b5a25f90615e12cb8f3a9/src/pages/home/ReportScreen.tsx#L811

In our case, shouldEnableKeyboardAvoidingView is false because showSoftInputOnFocus is false. The showSoftInputOnFocus logic was added in https://github.com/Expensify/App/pull/51172. It was added to always auto-focus the composer for the first time, but not show the keyboard until the user touches the composer, which is why showSoftInputOnFocus is initially false. https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L825-L835

When sending the PDF, the composer is refocused and at this point, showSoftInputOnFocus is still false, but somehow the keyboard is forced to show. Not sure why, but it happens because of the KeyboardAvoidingView inside the PDFView component. https://github.com/Expensify/App/blob/a8daefb0570311171c7b5a25f90615e12cb8f3a9/src/components/PDFView/index.native.tsx#L155-L165

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

I don't think we need the KeyboardAvoidingView anyway since the send button is always hidden when the keyboard is visible, so nothing to push, so we can remove KeyboardAvoidingView from PDFView.

OR

(Preferrable) Since we don't want the auto-focus behavior anymore in https://github.com/Expensify/App/issues/55485, I think we can remove the showSoftInputOnFocus logic. (we will basically partly revert https://github.com/Expensify/App/pull/51172)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

shubham1206agra commented 1 day ago

@bernhardoj's proposal looks good to me.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 day ago

Current assignee @arosiclair is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.