App icon indicating copy to clipboard operation
App copied to clipboard

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

Open lanitochka17 opened this issue 1 year ago • 47 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

  1. Open the app and log in
  2. Navigate to any chat
  3. Tap + > Assign task
  4. Add a tittle and proceed to the confirmation
  5. 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-12-05
Issue OwnerCurrent Issue Owner: @hoangzinh

lanitochka17 avatar Oct 30 '24 12:10 lanitochka17

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

melvin-bot[bot] avatar Oct 30 '24 12:10 melvin-bot[bot]

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] avatar Oct 30 '24 12:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 30 '24 12:10 melvin-bot[bot]

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

github-actions[bot] avatar Oct 30 '24 12:10 github-actions[bot]

Production:

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

lanitochka17 avatar Oct 30 '24 12:10 lanitochka17

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;
      }

Nodebrute avatar Oct 30 '24 14:10 Nodebrute

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

melvin-bot[bot] avatar Oct 31 '24 07:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 31 '24 07:10 melvin-bot[bot]

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

Beamanator avatar Oct 31 '24 07:10 Beamanator

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)

truph01 avatar Oct 31 '24 12:10 truph01

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

hoangzinh avatar Nov 01 '24 08:11 hoangzinh

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

truph01 avatar Nov 01 '24 08:11 truph01

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

hoangzinh avatar Nov 01 '24 08:11 hoangzinh

@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

Nodebrute avatar Nov 01 '24 13:11 Nodebrute

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

hoangzinh avatar Nov 04 '24 14:11 hoangzinh

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

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

Nodebrute avatar Nov 04 '24 14:11 Nodebrute

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

Nodebrute avatar Nov 04 '24 14:11 Nodebrute

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

hoangzinh avatar Nov 04 '24 16:11 hoangzinh

@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

Nodebrute avatar Nov 05 '24 04:11 Nodebrute

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 avatar Nov 06 '24 10:11 hoangzinh

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

hoangzinh avatar Nov 06 '24 10:11 hoangzinh

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 avatar Nov 06 '24 12:11 Nodebrute

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

Nodebrute avatar Nov 06 '24 13:11 Nodebrute

@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

hoangzinh avatar Nov 07 '24 11:11 hoangzinh

📣 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] avatar Nov 07 '24 16:11 melvin-bot[bot]

@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] avatar Nov 13 '24 09:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 13 '24 09:11 melvin-bot[bot]

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

hoangzinh avatar Nov 13 '24 10:11 hoangzinh

📣 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] avatar Nov 14 '24 16:11 melvin-bot[bot]

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?

  1. 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);
    }
  1. 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.

QichenZhu avatar Nov 15 '24 04:11 QichenZhu