Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for #54759] [$500] Description field doesn't autofocus correctly in submit expense flow #52414

Open
1 of 8 tasks
m-natarajan opened this issue Nov 12, 2024 · 67 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Nov 12, 2024

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.60-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation (hyperlinked to channel name): ts_external_expensify_bugs

Action Performed:

  1. Create an expense
  2. Choose a recipient
  3. Tap description field

Expected Result:

We should auto-focus any text field that is the only text field on the page.

Actual Result:

The description field is the only text field on the page, but the field doesn't quite auto-focus. The border turns green as if it was auto-focused, but you have to tap again to get the cursor to focus in the field.

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
RPReplay_Final1731409905.MP4
Auto.focus.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021856445363759517675
  • Upwork Job ID: 1856445363759517675
  • Last Price Increase: 2025-01-20
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

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

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Nov 12, 2024
@melvin-bot melvin-bot bot changed the title Description field doesn't autofocus correctly in submit expense flow [$250] Description field doesn't autofocus correctly in submit expense flow Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

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

@Anaslancer
Copy link
Contributor

Proposal

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

Description field doesn't autofocus correctly in submit expense flow

What is the root cause of that problem?

We are trying to set a focus in this code.
And it is an animated text input box.
So we should wait and set focus until the animation is ended.

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

We can use the useEffect instead of useFocusEffect in this below code.

What alternative solutions did you explore? (Optional)

We can increase more milliseconds in setTimeout.

}, CONST.ANIMATED_TRANSITION);

Contributor details

Your Expensify account email: anasup1995@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

Copy link

melvin-bot bot commented Nov 12, 2024

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

@ikevin127
Copy link
Contributor

ikevin127 commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-20 00:20:18 UTC.

Proposal

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

The field doesn't quite auto-focus. The border turns green as if it was auto-focused, but then in the end you have to tap again to get the cursor to focus into the field.

What is the root cause of that problem?

The root cause is this IOURequestStepDescription.tsx block of logic which does not work as intended, instead only turning the input's border green without actualy focusing the input and showing the cursor / keyboard.

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

  1. Remove the mentioned block of logic completely as it doesn't work as intended and on top of that adds extra timeout and logic that only slows down the UI.
  2. Pass the autoFocus (boolean) prop to the TextInput (InputWrapperWithRef) here.

More context on the TextInput autoFocus logic here.

What alternative solutions did you explore? (Optional)

Add autoFocus={Browser.isMobileWebKit()}prop to the TextInput (InputWrapperWithRef) here. This ensures we keep the current IOURequestStepDescription.tsx autofocus logic while doubling down on autofocus only for WebKit based browsers (more context in #52414 (comment)).

Note

I tested the first proposal's suggested solutions and none of them fixed the issue for me.

Result videos (before / after)

iOS: mWeb Safari
Before After
before.MP4
after.MP4

@huult
Copy link
Contributor

huult commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-13 03:01:54 UTC.

Proposal

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

Description field doesn't autofocus correctly in submit expense flow

What is the root cause of that problem?

We set a 300ms timeout in this block to delay the inputRef focus until after animations or screen transitions. However, 300ms was not enough for inputRef to focus properly, so this issue occurred.

useFocusEffect(
useCallback(() => {
focusTimeoutRef.current = setTimeout(() => {
if (inputRef.current) {
inputRef.current.focus();
}
return () => {
if (!focusTimeoutRef.current) {
return;
}
clearTimeout(focusTimeoutRef.current);
};
}, CONST.ANIMATED_TRANSITION);
}, []),
);

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

To resolve this issue, we should use InteractionManager.runAfterInteractions instead of setTimeout, as this React Native utility runs code only after animations and interactions have finished, ensuring that the component is ready. Here’s an example:

https://github.com/Expensify/App/blob/main/src/pages/iou/request/step/IOURequestStepDescription.tsx#L79-L93

    useFocusEffect(
        useCallback(() => {
-            focusTimeoutRef.current = setTimeout(() => {
+            focusTimeoutRef.current = InteractionManager.runAfterInteractions(() => {
                if (inputRef.current) {
                    inputRef.current.focus();
-                }
+                });
                return () => {
                    if (!focusTimeoutRef.current) {
                        return;
                    }
                    clearTimeout(focusTimeoutRef.current);
                };
-            }, CONST.ANIMATED_TRANSITION);
        }, []),
    );

What alternative solutions did you explore? (Optional)

Alternatively, we can remove this block of code and add autoFocus and shouldDelayFocus props to InputWrapperWithRef. We can use shouldDelayFocus to ensure the keyboard opens smoothly on Android. Without shouldDelayFocus, the keyboard may open during screen transitions on Android, which is the main difference from the second proposal. Here’s an example:

// src/pages/iou/request/step/IOURequestStepDescription.tsx#L171
     <InputWrapperWithRef
        InputComponent={TextInput}
        inputID={INPUT_IDS.MONEY_REQUEST_COMMENT}
        name={INPUT_IDS.MONEY_REQUEST_COMMENT}
        defaultValue={currentDescription}
        label={translate('moneyRequestConfirmationList.whatsItFor')}
        accessibilityLabel={translate('moneyRequestConfirmationList.whatsItFor')}
        role={CONST.ROLE.PRESENTATION}
        ref={(el) => {
            if (!el) {
                return;
            }
            if (!inputRef.current) {
                updateMultilineInputRange(el);
            }
            inputRef.current = el;
        }}
        autoGrowHeight
        maxAutoGrowHeight={variables.textInputAutoGrowMaxHeight}
        shouldSubmitForm
        isMarkdownEnabled
        excludedMarkdownStyles={!isReportInGroupPolicy ? ['mentionReport'] : []}
+        autoFocus
+        shouldDelayFocus
    />

Test branch

POC FOR MAIN SOLUTION
Screen.Recording.2024-11-13.at.09.59.48.mov
POC FOR MAIN ALTERNATIVE SOLUTION
Screen.Recording.2024-11-13.at.10.24.02.mov

Alternative solutions 2

We found that in the app, when there is an InputWrapper with only a single input on the screen, it should automatically focus. If there is more than one input, we shouldn't focus automatically, or we should only focus on the first input.

const {inputCallbackRef} = useAutoFocusInput();
<InputWrapper
    ...
    ref={inputCallbackRef}
    ...
/>

@truph01
Copy link
Contributor

truph01 commented Nov 13, 2024

Proposal

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

The field doesn't quite auto-focus. The border turns green as if it was auto-focused, but then in the end you have to tap again to get the cursor to focus into the field.

What is the root cause of that problem?

  • We already have a logic to call focus input manually in:

useFocusEffect(
useCallback(() => {
focusTimeoutRef.current = setTimeout(() => {
if (inputRef.current) {
inputRef.current.focus();
}
return () => {
if (!focusTimeoutRef.current) {
return;
}
clearTimeout(focusTimeoutRef.current);
};
}, CONST.ANIMATED_TRANSITION);
}, []),
);

It works well in all platform except IOS Safari because of the following:

We (Apple) like the current behavior and do not want programmatic focus to bring up the keyboard when you do not have a hardware keyboard attached

Source

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

  • We can add autoFocus={Browser.isSafari()} in:

to make sure the bug is fixed in safari and does not introduce regression in other platform.

What alternative solutions did you explore? (Optional)

Optional:

  • Beside the main solution, we can update:

to:

ref={inputCallbackRef}

with:

    const {inputCallbackRef} = useAutoFocusInput(true)

and remove this useEffect

@sakluger
Copy link
Contributor

Hey @getusha, looks like we have several proposals. Could you please take a look?

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2024
Copy link

melvin-bot bot commented Nov 18, 2024

@sakluger, @getusha Huh... This is 4 days overdue. Who can take care of this?

@sakluger
Copy link
Contributor

Im' going to assign a new C+ to make sure we keep the urgency on this issue.

@sakluger sakluger added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Nov 18, 2024
Copy link

melvin-bot bot commented Nov 18, 2024

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

@sobitneupane
Copy link
Contributor

Thanks for the proposal everyone.

I believe this issue isn’t limited to the description field. We’ve encountered similar issues in the app before, and I recall we planned to batch them into a single issue. However, it seems they’re still unresolved.

Copy link

melvin-bot bot commented Nov 19, 2024

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

@sobitneupane
Copy link
Contributor

I found the original issue, but it seems we closed it without resolving. @parasharrajat, since you were reviewing the issue, did we close it in favor of another one? Could you please share some details?

@parasharrajat
Copy link
Member

It was closed after many attempts to fix it. As, I mentioned here #10414 (comment) we should work on fresh issue if needed.

@sakluger
Copy link
Contributor

sakluger commented Jan 6, 2025

I've raised the bounty to $500. Hopefully we'll get a few new or updated proposals!

@shubham1206agra
Copy link
Contributor

@hannojg Can you look into this? Maybe you can figure this out. As you are working on similar thing #54759

@hannojg
Copy link
Contributor

hannojg commented Jan 7, 2025

I think on web it makes sense to handle the autofocus ourselves, as the autofocus prop on input or text input only works on page load, but not afterwards.

So I actually agree with this proposal. @ikevin127 mentioned that there is a bug to why the focus isn't working reliably. While I believe this could be true, I would give it a try and see if there isn't anything else going on that might prevents it from working (so I am trying to say: maybe we can make it work 100% of the times, maybe its a bug - I'd have to double check myself).

The issue ticket #54759 is primarily for mobile (however it would be nice if wee remove shouldDelayFocus on mobile to also unifiable remove this on web as well.).
So yes, as part of #54759 I could look into this issue as well!

@melvin-bot melvin-bot bot added the Overdue label Jan 9, 2025
@sakluger
Copy link
Contributor

sakluger commented Jan 9, 2025

@hannojg I believe the issue was that @ikevin127's proposal only addressed the description field - we want to solve this issue for any field that uses auto-focus.

Where we got stuck was in figuring out if there was a single component that would fix all auto-focus fields, or if we would need to identify each field and fix it separately. Do you know if there is a single component that would address all auto-focus fields?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 9, 2025
Copy link

melvin-bot bot commented Jan 13, 2025

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

Copy link

melvin-bot bot commented Jan 13, 2025

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

@sakluger
Copy link
Contributor

@hannojg checking in again - Do you know if there is a single component that would address all auto-focus fields?

@melvin-bot melvin-bot bot removed the Overdue label Jan 13, 2025
@hannojg
Copy link
Contributor

hannojg commented Jan 14, 2025

Here is what I think:

There should only be one autoFocus prop for our TextInput implementation, and it should just work ™ (talking about our BaseTextInput component).

Right now additionally we have:

  • a shouldDelayFocus prop which we use in conjunction with autoFocus on the BaseTextInput to work around some bugs (e.g. the bug we are discovering here, we would just need to use this prop to workaround those bugs)
  • additionally logic in some screens / components to basically work around the same bugs that shouldDelayFocus aims to workaround

So my proposal is, to remove all those workarounds in different places, remove the shouldDelayFocus prop, and simply make sure we fix all underlying bugs that prevent autoFocus from working 100%.

Thats basically my proposal here:

(note: on web this will still require to use some kind of mechanism like setTimeout as autoFocus on web doesn't work like we need it to. However, when reimplementing this I would argue that this can be automatically handled correctly when autoFocus is true)

Do you know if there is a single component that would address all auto-focus fields?

So, my tl;dr: yes, it should be BaseTextInputs autoFocus prop that we should fix and make sure it's always working for all cases.

@melvin-bot melvin-bot bot added the Overdue label Jan 16, 2025
@sakluger
Copy link
Contributor

Cool, thanks for the feedback! That's super helpful.

@sobitneupane @ikevin127 is that helpful for you?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 17, 2025
Copy link

melvin-bot bot commented Jan 20, 2025

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

@sobitneupane
Copy link
Contributor

@sakluger I believe we should hold off on #54759, as it will significantly affect the autofocus logic.

@hannojg This issue is reproducible only on iOS/mWeb and occurs across almost every component. The text field appears focused but lacks a caret and keyboard. It has been a long-standing issue.

Auto.focus.mp4

@melvin-bot melvin-bot bot removed the Overdue label Jan 20, 2025
Copy link

melvin-bot bot commented Jan 20, 2025

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

@sakluger sakluger removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 22, 2025
@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2025
@sakluger sakluger changed the title [$500] Description field doesn't autofocus correctly in submit expense flow [HOLD for #54759] [$500] Description field doesn't autofocus correctly in submit expense flow Jan 22, 2025
@sakluger sakluger added Weekly KSv2 and removed Daily KSv2 labels Jan 22, 2025
@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2025
@sakluger
Copy link
Contributor

As recommended by @sobitneupane, I've put this on hold for #54759 and set it to weekly to align with the priority of the hold issue.

@melvin-bot melvin-bot bot added the Overdue label Jan 31, 2025
@sakluger
Copy link
Contributor

No new updates in the hold issue.

@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests