[$250] iOS - Task - Description field not focused and keyboard not opened automatically

lanitochka17 commented 1 month 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.55.6 Reproducible in staging?: Y Reproducible in production?: N If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5143269 Issue reported by: Applause - Internal Team

Action Performed:

Open the app and log in
Navigate to any chat
Tap + > Assign task
Add a tittle and proceed to the confirmation
Select the description

Expected Result:

The description field is focused and the keyboard is opened automatically

Actual Result:

The description field is not focused and the keyboard is not opened. It takes a few taps to get the keyboard, and it can not be dismissed

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Standalone
[ ] Android: HybridApp
[ ] Android: mWeb Chrome
[x] iOS: Standalone
[x] iOS: HybridApp
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/17335050-4ad8-42a2-8e39-9d6d22aeee50

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021851889241444905343
Upwork Job ID: 1851889241444905343
Last Price Increase: 2024-11-28

Issue Owner

Current Issue Owner: @hoangzinh

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month 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.

melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

lanitochka17 commented 1 month ago

Production:

https://github.com/user-attachments/assets/00ec1a57-0922-43b9-9c20-e7d139684084

Nodebrute commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-30 15:03:43 UTC.

Proposal

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

Description field not focused and keyboard not opened automatically

What is the root cause of that problem?

We recently duplicated the logic of updateMultilineInputRange into useAutoFocusInput and began passing the isMultiline parameter into useAutoFocusInput to enable this logic. While we made these changes in ExitSurveyResponsePage.tsx, we haven't yet updated NewTaskDescriptionPage.tsx to reflect the latest changes.

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

remove updateMultilineInputRange usage from here https://github.com/Expensify/App/blob/803821640f120e5674a8b307e701b4799c1897c0/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

       ref={inputCallbackRef}

and update this line here to pass true

    const {inputCallbackRef} = useAutoFocusInput(true);

What alternative solutions did you explore? (Optional)

Or we can add here This will also fix the issue

   if (!el) {
  return;
      }

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

Beamanator commented 1 month ago

Calling this NAB as it's pretty easy to work around and I am going to try to get the deploy out early today

truph01 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-31 13:16:24 UTC.

Proposal

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

The description field is not focused and the keyboard is not opened. It takes a few taps to get the keyboard, and it can not be dismissed

What is the root cause of that problem?

On the page, we have a text input and a ref which calls updateMultilineInputRange:

https://github.com/Expensify/App/blob/803821640f120e5674a8b307e701b4799c1897c0/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

In updateMultilineInputRange, with IOS native, we immediately focus on the input. With other platforms, we do 2 things, setting the selection to the last character and scroll the input to the last line:

https://github.com/Expensify/App/blob/780e09e10fbd33f9806e5e1b30a22bd3703de742/src/libs/updateMultilineInputRange/index.ios.ts#L23

Because we focus the input too early, the input is not focused properly which I think is already a known issue which is why we delay an input focus. The similar RCA is mentioned here.
Also, the issue can be reproduced in prod as well, not only staging, so it is not related to the recently changes in this PR.

What changes do you think we should make in order to solve the problem?
As I said above, in updateMultilineInputRange, we do 2 things, setting the selection to the last character and scroll the input to the last line. In PR, we scroll the input immediately and only setting the selection to the last character if the transition ended. It will make sure we only call focus function to focus on input if the transition ended. So we can make use of it to fix this bug.
To do it, just need to update this:

const {inputCallbackRef} = useAutoFocusInput(true);

and this:

const {inputCallbackRef} = useAutoFocusInput(true);

What alternative solutions did you explore? (Optional)

hoangzinh commented 1 month ago

Thanks for the proposals, everyone. Does anyone know why it has only happened recently? I believe it worked before.

truph01 commented 1 month ago

@hoangzinh As I mentioned, the bug appears in prod as well

hoangzinh commented 1 month ago

I didn't mean to find a deploy blocker, I mean why did it happen recently? Or when the existing focus logic we have here doesn't work anymore

https://github.com/Expensify/App/blob/dfe8e950cbbfd22adb32c622110a45e6c9b12dd0/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

Nodebrute commented 1 month ago

@hoangzinh I believe this issue has the same root cause as this one. Yes, this issue also exists in production, but we’re only using useAutoFocus along with updateMultilineInputRange in two components.

https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L55 https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/pages/tasks/NewTaskDescriptionPage.tsx#L40

The problem only occurs on NewTaskDescriptionPage.tsx and not on WorkspaceInviteMessagePage. It works on WorkspaceInviteMessagePage due to the code below the return statement. If we add this code to NewTaskDescriptionPage.tsx, it will fix the problem (as mentioned in my alternate proposal) https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L200-L202

Another working solution (also mentioned in my main proposal) Another solution will be what we did in ExitSurveyResponsePage by passing true to useAutoFocusInput and removing updateMultilineInputRange https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L41

hoangzinh commented 4 weeks ago

Yeah, but the root cause here https://github.com/Expensify/App/issues/50962#issuecomment-2418541617 is not clear to me, it doesn't explain why those don't logic work anymore. It used to work a few months ago https://github.com/Expensify/App/pull/46172#discussion_r1691114699 https://github.com/Expensify/App/blob/a3a270d918f2130baab4ddc0ddc00a6ddfe7388f/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

Moreover, I tried to apply useAutoFocusInput(true) on the latest main branch, but it didn't work to me 👀

https://github.com/user-attachments/assets/17ef21bc-769b-4de9-a454-8fcc4cab39af

Nodebrute commented 4 weeks ago

@hoangzinh It's working for me, I am on the latest main.

https://github.com/user-attachments/assets/50f933a7-d767-4144-b47d-dda7d4793c21

Nodebrute commented 4 weeks ago

@hoangzinh Are you sure the changes are fully loaded on your iOS device? I noticed the back button is also missing(description page) in your video.

hoangzinh commented 4 weeks ago

After restarting everything, confirmed that applying useAutoFocusInput(true). Now, we need to understand why those logic doesn't work anymore

https://github.com/Expensify/App/blob/a3a270d918f2130baab4ddc0ddc00a6ddfe7388f/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

Nodebrute commented 4 weeks ago

@hoangzinh I’ve spent more time investigating this today, but I haven’t yet pinpointed the exact recent change that caused this error. However, I believe the root cause is related to this code: https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/libs/updateMultilineInputRange/index.ios.ts#L22-L24

This issue only occurs on iOS, and the file linked is iOS-specific. Here, shouldAutoFocus = true, which is why the field is being auto-focused and this issue only happens when we use useAutoFocusInput with updateMultilineInputRange https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/libs/updateMultilineInputRange/index.ios.ts#L12

Another way to test the rca is by passing false here as a second param https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/pages/tasks/NewTaskDescriptionPage.tsx#L87

hoangzinh commented 3 weeks ago

However, I believe the root cause is related to this code:

It doesn't seem to explain why your alternative solution works. Because we already checked if it's null here for the IOS platform before executing that LOC.

https://github.com/Expensify/App/blob/9177e2ac3177e7832f83c0d4f514695bfae189c9/src/libs/updateMultilineInputRange/index.ios.ts#L13-L15

hoangzinh commented 3 weeks ago

I suspect this function work is not working correctly on the New Task Description page

https://github.com/Expensify/App/blob/9177e2ac3177e7832f83c0d4f514695bfae189c9/src/hooks/useAutoFocusInput.ts#L56-L65

Nodebrute commented 3 weeks ago

Yes, adding the code below in inputCallbackRef works. However, I'm still unable to pinpoint the exact recent change that introduced this bug

   if(!ref){
            return;
        }

Nodebrute commented 3 weeks ago

@hoangzinh I still believe we should remove updateMultilineInputRange from here and stick with useAutoFocus, as we've already incorporated the same logic from updateMultilineInputRange into useAutoFocus. It would be more logical to use just useAutoFocus.

hoangzinh commented 3 weeks ago

@Nodebrute I agree that the solution makes sense currently. Yet because we haven't found the root cause yet, therefore I'm unsure if it's a workaround solution or if we may hide a real bug behind

melvin-bot[bot] commented 3 weeks 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 weeks ago

@nkuoch @hoangzinh @sonialiap 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 2 weeks ago

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

hoangzinh commented 2 weeks ago

Still looking for a RCA that helps us to understand why this issue occures.

melvin-bot[bot] commented 2 weeks ago

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

QichenZhu commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-11-20 01:39:10 UTC.

Proposal

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

Task description field is not focused automatically on iOS.

What is the root cause of that problem?

The root cause is in https://github.com/facebook/react-native/issues/47576.

In the new architecture, native view commands are dispatched immediately to the UI thread

But mounting operations are only flushed at the end of the current task in the event loop / runtime scheduler

That causes problems when invoking native view commands on views that were just created

On Android, this works correctly because we queue the native view command in the same queue as mount operations, so they're always processed in the right order.

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

There is a potential upstream fix https://github.com/facebook/react-native/pull/47604. I tried partially backporting it to our repo and it worked. So we can put this on hold until the fix is released and then upgrade RN, or request the upstream to cherry-pick the fix into old versions.

[!NOTE]
The code below is just a feasibility test, not the final version.

https://github.com/facebook/react-native/blob/2e994fdddb05955ee8dcce1e2151288977dd1c5e/packages/react-native/ReactCommon/react/renderer/scheduler/Scheduler.cpp#L328

    auto weakRuntimeScheduler =
        contextContainer_->find<std::weak_ptr<RuntimeScheduler>>(
            "RuntimeScheduler");
    auto runtimeScheduler = weakRuntimeScheduler.has_value()
        ? weakRuntimeScheduler.value().lock()
        : nullptr;
    if (runtimeScheduler) {
      runtimeScheduler->scheduleRenderingUpdate(
        [delegate = delegate_,
          shadowView = std::move(shadowView),
          commandName,
          args]() {
          delegate->schedulerDidDispatchCommand(shadowView, commandName, args);
        });
    } else {
      delegate_->schedulerDidDispatchCommand(shadowView, commandName, args);
    }

Also remove tons of existing focus-with-delay workarounds in our repo, as they are no longer needed.

What alternative solutions did you explore? (Optional)

The upstream bug only affects Bridgeless mode, so disabling Bridgeless by reverting https://github.com/Expensify/App/commit/3ddc1dd1c1475d65ed4a49e4ebf2cc57b8fa0108 could also resolve this.

hoangzinh commented 2 weeks ago

@QichenZhu Interesting! Could you elaborate more on how this issue affects our current focus logic here? Thank you

https://github.com/Expensify/App/blob/a3a270d918f2130baab4ddc0ddc00a6ddfe7388f/src/pages/tasks/NewTaskDescriptionPage.tsx#L85-L90

QichenZhu commented 2 weeks ago

@hoangzinh On iOS, updateMultilineInputRange() calls input.focus(), which dispatches a view command from JS to the native side but fails silently because componentView is nullptr at that time.

https://github.com/Expensify/App/blob/788939a84704c541b9472b7460206272fb927533/src/libs/updateMultilineInputRange/index.ios.ts#L23

https://github.com/facebook/react-native/blob/1611a5e8a1a9a9cb7d6d9ddb4d7316d8e634e6e3/packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm#L334

Though it can be easily worked around by adding a delay, I suggest fixing the root cause because:

We don’t expect focus() to fail silently.
If it fails silently, the JS side isn’t aware and optimistically sets the internal status as focused, causing future programmatic focus attempts to fail as well.

https://github.com/facebook/react-native/blob/0705eb8b91b6c3688a73f8481c6b067899ac6eec/packages/react-native/Libraries/Components/TextInput/TextInputState.js#L62

https://github.com/facebook/react-native/blob/0705eb8b91b6c3688a73f8481c6b067899ac6eec/packages/react-native/Libraries/Components/TextInput/TextInputState.js#L106

(More details can be found in upstream issues: https://github.com/facebook/react-native/issues/47359 and https://github.com/facebook/react-native/issues/47576.)

hoangzinh commented 1 week ago

Thanks for explanation @QichenZhu. I tried to apply your suggestion here but it doesn't work for me. Do I miss anything else?

https://github.com/user-attachments/assets/be08a2d9-1f6c-4ee7-ac96-54400195d99c

QichenZhu commented 1 week ago

@hoangzinh The code in your video is the same as mine, but the filename in the second line is strange.

Before

https://github.com/user-attachments/assets/c0eff20d-2f4d-48a0-aa94-278f2bc4a914

After

https://github.com/user-attachments/assets/c8f6e204-9452-45f0-9bf0-5e3ca1e77ac0

hoangzinh commented 1 week ago

oh right, it works on my device. @QichenZhu proposal looks good to me. Let's wait internal engineer's decision on either waiting for upgrade React native or making a patch to our app.

Link to proposal https://github.com/Expensify/App/issues/51728#issuecomment-2477914716

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week 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 week ago

@nkuoch, @hoangzinh, @sonialiap Huh... This is 4 days overdue. Who can take care of this?

hoangzinh commented 1 week ago

cc @nkuoch waiting on your next review round

melvin-bot[bot] commented 6 days ago

@nkuoch @hoangzinh @sonialiap this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

nkuoch commented 5 days ago

Let's hold for react native update

melvin-bot[bot] commented 4 days ago

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

sonialiap commented 4 days ago

@nkuoch do you have a link to an issue that I can track for the hold?

nkuoch commented 4 days ago

https://github.com/Expensify/App/issues/48911

melvin-bot[bot] commented 1 day ago

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

Expensify / App