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

[Due for payment 2025-02-13] [$250] mWeb - Workspace - Input is not auto-focused and keyboard doesn't open when creating a tag #53208

Open IuliiaHerets opened 2 months 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.67-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): htad26+tttyyu@gmail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Login to new dot with an account
  2. Create a new workspace
  3. Go to more features > Tags > Enable tags
  4. Navigate to tags page > Add tag

Expected Result:

Tag creation page is opened, the name input is automatically focused and keyboard is opened

Actual Result:

The name input isn't automatically focused and the keyboard is not opened. Sometimes the focus and the keyboard appear with a delay

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/e3fab1af-c01b-44f4-9776-13616a26d0a0

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021863569158386608990
  • Upwork Job ID: 1863569158386608990
  • Last Price Increase: 2024-12-09
  • Automatic offers:
    • linhvovan29546 | Contributor | 105879100
Issue OwnerCurrent Issue Owner: @sonialiap
melvin-bot[bot] commented 2 months ago

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

Themoonalsofall commented 2 months ago

Proposal

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

mWeb - Workspace - Input is not auto-focused and keyboard doesn't open when creating a tag

What is the root cause of that problem?

We are missing autoFocus props in this input

https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L92-L100

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

Add autoFocus to this input

What alternative solutions did you explore? (Optional)

Shahidullah-Muffakir commented 2 months ago

Proposal

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

When user opens the Tags creating page, the input is not focused and the keyboard is not opened.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L92-L100 here we did not pass : autoFocus shouldDelayFocus

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

pass these

autoFocus shouldDelayFocu

  1. autoFocus: it will focus the input
  2. shouldDelayFocus: it will open the keyboard.
<InputWrapper 
     InputComponent={TextInput} 
+     autoFocus
+    shouldDelayFocus
CyberAndrii commented 2 months ago

It's the same issue as https://github.com/Expensify/App/issues/53314 and will be fixed by proposals from there. @sonialiap can we put this on hold and retest later?

Shahidullah-Muffakir commented 2 months ago

It's the same issue as #53314 and will be fixed by proposals from there. @sonialiap can we put this on hold and retest later?

@CyberAndrii These issues seem different, but if you believe they are the same, please share your proposal here. Since this issue was created first, any duplicate would typically be closed in favor of this one.

CyberAndrii commented 2 months ago

I don't think your proposals have the correct RCA and solution. We already use useAutoFocusInput() here which should handle the focus. However, there's a bug that prevents InteractionManager.runAfterInteractions callbacks from executing. Also, useAutoFocusInput() is preferred over autoFocus.

CyberAndrii commented 2 months ago

I did some more testing and it appears that specifically in this case InteractionManager callbacks are being blocked by a playing lottie animation on the previous page. If I disable it here by setting autoPlay={false} then autofocus works again.

cc @Themoonalsofall @Shahidullah-Muffakir if you want to investigate this further and update your proposals.

I wasn't able to reproduce this issue on a Pixel 8 Pro emulator so I tried various Browserstack devices and the results are as follows

Reproducible

Not reproducible

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

sonialiap commented 2 months ago

@mollfpr we have a few proposals for your review :)

mollfpr commented 2 months ago

I don't think your proposals have the correct RCA and solution. We already use useAutoFocusInput() here which should handle the focus.

I agree with @CyberAndrii. Both @Themoonalsofall @Shahidullah-Muffakir proposals don't solve the main issue.

melvin-bot[bot] commented 2 months ago

@sonialiap, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

mollfpr commented 2 months ago

Not overdue πŸ‘€

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 1 month ago

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

melvin-bot[bot] commented 1 month ago

@sonialiap @mollfpr 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!

melvin-bot[bot] commented 1 month ago

@sonialiap, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

sonialiap commented 1 month ago

I think this one might be like https://github.com/Expensify/App/issues/53346 and waiting on React Native 0.76 upgrade

mvtglobally commented 1 month ago

Issue not reproducible during KI retests. (First week)

linhvovan29546 commented 1 month ago

I think this one might be like #53346 and waiting on React Native 0.76 upgrade

I think this is not correct. React Native 0.76 upgrade only applies to native, while this issue appears on mWeb.

linhvovan29546 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2025-01-06 14:03:34 UTC.

Proposal

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

mWeb - Workspace - Input is not auto-focused and keyboard doesn't open when creating a tag

What is the root cause of that problem?

On the "Tags" page, a Lottie animation is displayed when the tags is empty https://github.com/Expensify/App/blob/8ca44dac9fbd0db0d8c57694f0a46a003b3245df/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L407-L418 Then, when navigating to the "Add Tag" page, we have a hook is used to auto-focus the input, the focus is triggered inside InteractionManager.runAfterInteractions: https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L33 However, the Lottie animation continues running in the background, preventing runAfterInteractions from executing. https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/hooks/useAutoFocusInput.ts#L28-L34

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

We have logic to pause the animation when the screen blurs, but this does not apply to isSideModalNavigator. Because, on desktop or larger screens, a backdrop is present, and the previous screen remains visible. https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/components/Lottie/index.tsx#L76-L77 We should pause the Lottie animation on blur for mWeb or smallScreen. Because, on mWeb, the previous page is not visible as the sidebar renders in full-page mode https://github.com/Expensify/App/blob/c62ab78b34dbc993c241277b4ee525ec6277b471/src/components/Lottie/index.tsx#L76-L77

            if (!isSideModalNavigator(targetRouteName) || isMobile()) {
                setHasNavigatedAway(true);
            }
POC
Before After

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

N/A

What alternative solutions did you explore? (Optional)

N/A

linhvovan29546 commented 1 month ago

I think this is not correct. React Native 0.76 upgrade only applies to native, while this issue appears on mWeb.

Pinging @sonialiap and @mollfpr, in case you may have missed my comment

mollfpr commented 1 month ago

@linhvovan29546 I am unable to reproduce the issue on my Samsung phone or with the simulator. What might be causing this?

https://github.com/user-attachments/assets/4eb1e472-26e9-4c23-85b0-d707ebcea1de

linhvovan29546 commented 1 month ago

@linhvovan29546 I am unable to reproduce the issue on my Samsung phone or with the simulator. What might be causing this?

Screen_Recording_20250107_220516_Chrome.mp4

I can reproduce on my Samsung phone with same step in the OP

mollfpr commented 1 month ago

It will be hard if we can't constantly reproduce the issue on the affected platform and I can't confirm if the solution works.

linhvovan29546 commented 3 weeks ago

It will be hard if we can't constantly reproduce the issue on the affected platform and I can't confirm if the solution works.

Honestly, I can still reproduce this issue on staging with my device, but it seems challenging since you are unable to reproduce it on your device.

melvin-bot[bot] commented 3 weeks ago

@sonialiap, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

linhvovan29546 commented 3 weeks ago

@mollfpr I looked into the steps to reproduce the issue. You can test it under high CPU usage conditions.

melvin-bot[bot] commented 3 weeks ago

@sonialiap, @mollfpr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 3 weeks ago

@sonialiap, @mollfpr Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 2 weeks ago

@sonialiap, @mollfpr 12 days overdue now... This issue's end is nigh!

mollfpr commented 2 weeks ago

@linhvovan29546 How can I emulate the high CPU usage on the Android emulator? I try to create an AVD with CPU core of 1 and RAM 512MB, but can't reproduce the issue.

rayane-d commented 2 weeks ago

@mollfpr I'm able to reproduce on my physical device if you want me to take over the C+ review

https://github.com/user-attachments/assets/366f2507-434c-47d4-8687-8b41c160ff11

mollfpr commented 2 weeks ago

@rayane-djouah Yup, please take over the issue πŸ™

rayane-d commented 2 weeks ago

@linhvovan29546's proposal looks good to me

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

rayane-d commented 2 weeks ago

@MonilBhavsar please assign me as well. Thanks!

rayane-d commented 2 weeks ago

We don't need [Hold for React Native 0.76] in the title

MonilBhavsar commented 2 weeks ago

Is this on HOLD?

rayane-d commented 2 weeks ago

This is not on hold

MonilBhavsar commented 2 weeks ago

Okay thanks! Took it off HOLD. I will review this tomorrow

rayane-d commented 1 week ago

@MonilBhavsar kind reminder :)

MonilBhavsar commented 1 week ago

However, the Lottie animation continues running in the background, preventing runAfterInteractions from executing.

Curious to know why it only happens on specific devices, and is this the only place where this issue happens, I don't think so. As we use animations at many places in the App

linhvovan29546 commented 1 week ago

Curious to know why it only happens on specific devices.

I'm not sure why this happens on specific devices. I think it may be related to the device's hardware as some of the affected devices can be found here: https://github.com/Expensify/App/issues/53208#issuecomment-2509803139 (Thank you to @CyberAndrii for investigating!)

and is this the only place where this issue happens.

The previous screen of the create tag page is the workspace settings tag page, which includes a Lottie animation. And on the create tag page, we use useAutoFocusInput to focus the input field, as mentioned in my proposal.

As we use animations at many places in the App.

For many places in the app, I’ve searched thoroughly and noticed that they are not the same as this case.

In summary, my proposal covers the missing case in mWeb https://github.com/Expensify/App/pull/48444, where the LEFT_MODAL_NAVIGATOR and RIGHT_MODAL_NAVIGATOR are displayed as full-screen views. For better performance, we should pause the lottie animation when a full-screen view is displayed

cc @MonilBhavsar

MonilBhavsar commented 1 week ago

Thanks for clarifying πŸ‘

melvin-bot[bot] commented 1 week ago

πŸ“£ @linhvovan29546 πŸŽ‰ 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 πŸ“–

linhvovan29546 commented 1 week ago

PR is ready for review!

melvin-bot[bot] commented 20 hours ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 20 hours ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.94-25 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-02-13. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 20 hours ago

@rayane-d @sonialiap @rayane-d The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]