[$500] iOS - Room - Room name field in Room tab is not auto-focused when transitioning from Chat

[lanitochka17]

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:
Reproducible in staging?:
Reproducible in production?:
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:
Slack conversation:

Action Performed:

Break down in numbered steps1. Launch New Expensify app
2. Tap + > New chat
3. Tap Room

Expected Result:

Room name field is auto-focused

Actual Result:

Room name field is not auto-focused

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6278459_1700099720216.RPReplay_Final1700091647.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d7206b7c5e42ad01
• Upwork Job ID: 1724981456441212928
• Last Price Increase: 2023-11-23
• Automatic offers:
• shubham1206agra | Reviewer | 27955731

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[bernhardoj]

Proposal

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

The auto focus of the new room page is lost when switching tabs.

What is the root cause of that problem?

This kind of issue was previously fixed in #28646 where they introduced a 600ms delay when auto-focusing.

However, this PR #29203 maybe unintentionally change the delay to 300ms that is coming from useAutoFocusInput hook.

App/src/hooks/useAutoFocusInput.js

Lines 28 to 30 in 5e01376

	focusTimeoutRef.current = setTimeout(() => {
	setIsScreenTransitionEnded(true);
	}, CONST.ANIMATED_TRANSITION);

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

Have a new param to useAutoFocusInput called delay that has a default value of CONST.ANIMATED_TRANSITION. For the new room page, we will pass a custom 600ms delay.

-const {inputCallbackRef} = useAutoFocusInput();
+const {inputCallbackRef} = useAutoFocusInput(600);

[shubham1206agra]

@bernhardoj Thanks for the proposal. Your RCA is right. However, the proposed solution might introduce a delay in the initial mounting of components. Kindly update your proposal to correct this.

[bernhardoj]

might introduce a delay in the initial mounting of components.

@shubham1206agra The solution is just increasing the auto-focus delay, I don't think it would delay the component mounting.

[shubham1206agra]

I mean focus in the initial mounting.

[bernhardoj]

But that's the delay we had before which works fine. Do you expect a solution without a delay?

[tienifr]

Proposal

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

As we can see in the video, the room name was actually autofocused correctly, but it's dismissed right after that, causing the blur to occur and the error show.

This not only happens for the RoomNameInput but also any inputs that are inside a TabNavigator, including the input in NewChatPage.

What is the root cause of that problem?

This is a known issue with react-navigation, when the transition ends, sometimes it will dismiss the keyboard immediately. A similar issue was this one for Android, but the fix for that doesn't seem to work for this case.

And the tab navigator transition time is longer than the stack navigator transition time (which is set to 300ms) now, so the timeout in useAutoFocusInput is not enough for the tab navigator.

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

Pass a focusDelay param to useAutoFocusInput to customize the delay time. And pass it as 350 for all inputs inside a TabNavigator, including the input in NewChatPage and WorkspaceNewRoomPage.

From my testing, 350 is a reasonable delay for tab navigation animation and works reliably for this case. This will also not impact the user experience because it's only slightly delayed 50ms more compared to current implementation.

What alternative solutions did you explore? (Optional)

Disable animation for the TabNavigator

350 works fine in all instances of my testing, but we can adjust it a bit based on multiple devices testing during PR process.

[manlaig]

Proposal

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

Room name input is not auto-focused.

What is the root cause of that problem?

For the RoomNameInput component, auto-focusing is decided by this condition:

App/src/components/RoomNameInput/index.native.js

Line 45 in 47c7819

autoFocus={isFocused && autoFocus}

But, the autoFocus property is not passed as part of RoomNameInput here:

App/src/pages/settings/Report/RoomNamePage.js

Lines 102 to 107 in 47c7819

	<RoomNameInput
	ref={(ref) => (roomNameInputRef.current = ref)}
	inputID="roomName"
	defaultValue={report.reportName}
	isFocused={isFocused}
	/>

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

Set the autoFocus field for RoomNameInput as follows:

<RoomNameInput
    ref={(ref) => (roomNameInputRef.current = ref)}
    inputID="roomName"
    defaultValue={report.reportName}
    isFocused={isFocused}
    autoFocus
/>

What alternative solutions did you explore? (Optional)

[shubham1206agra]

But that's the delay we had before which works fine. Do you expect a solution without a delay?

I want 300ms for initial mounting and 600ms afterwards.

[bernhardoj]

Ah, I see. IMO, that would bloat the useAutoFocusInput hook and I just feel it's weird to see that logic.

The new logic would probably look like this:

useAutoFocusInput(onMountDelay = CONST.ANIMATED_TRANSITION, onFocusDelay = CONST.ANIMATED_TRANSITION) {
    const isFirstTimeRender = useRef(true);
    const delay = isFirstTimeRender.current ? onMountDelay : onFocusDelay;
    useEffect(() => {
        isFirstTimeRender.current = false;
    }, []);

    useFocusEffect(() => setTimeout(..., delay))
}

I think it's fine to use 600ms for both cases as we don't have any issue with that when we are using it instead of trying to introduce something fancy that doesn't really add a value. Let me know your opinion.

[shubham1206agra]

Ah, I see. IMO, that would bloat the useAutoFocusInput hook and I just feel it's weird to see that logic.

The new logic would probably look like this:

useAutoFocusInput(onMountDelay = CONST.ANIMATED_TRANSITION, onFocusDelay = CONST.ANIMATED_TRANSITION) {
    const isFirstTimeRender = useRef(true);
    const delay = isFirstTimeRender.current ? onMountDelay : onFocusDelay;
    useEffect(() => {
        isFirstTimeRender.current = false;
    }, []);

    useFocusEffect(() => setTimeout(..., delay))
}

I think it's fine to use 600ms for both cases as we don't have any issue with that when we are using it instead of trying to introduce something fancy that doesn't really add a value. Let me know your opinion.

@bernhardoj I don't think this will work exactly.

[bernhardoj]

@shubham1206agra it actually works, but an ugly one (the other option is to use either useFocusEffect or focus listener based on a flag passed to useAutoFocusInput). Make sure to put delay into the useCallback of useFocusEffect. But I still don't like the idea of the different delay. The only case I see the new logic will be used (the new room page tab is shown first) is when you switch to the room tab, close the app, and open it back.

Btw, I just noticed that we set autoFocus to the room name input, so when mounted, it will use the 300ms delay, but we should disable it because we already handle the auto focus using useAutoFocusInput.

App/src/pages/workspace/WorkspaceNewRoomPage.js

Lines 199 to 205 in 9cfe182

	<RoomNameInput
	ref={inputCallbackRef}
	inputID="roomName"
	isFocused={props.isFocused}
	shouldDelayFocus
	autoFocus
	/>

So, let's focus on the original issue and ignore the different delay feature.

[tienifr]

@shubham1206agra do you have any feedback on my proposal? Thanks!

[shubham1206agra]

@shubham1206agra do you have any feedback on my proposal? Thanks!

@tienifr Your proposal looks the same as @bernhardoj proposal, and since @bernhardoj's proposal came first. I am going to prefer @bernhardoj on this.

[mallenexpensify]

Is this a regression? Seems like it.

[tienifr]

Your proposal looks the same as @bernhardoj proposal, and since @bernhardoj's proposal came first. I am going to prefer @bernhardoj on this.

@shubham1206agra sorry but I'm not sure which part of it is the same. The RCA is different, the solution is also different:

• @bernhardoj 's proposal reverts the long timeout (600ms) in one of the input
• My proposal will apply a short timeout (350ms) to all the inputs under TabNavigator, since I was able to identified the root cause on TabNavigator rather than just applying a timeout arbitrarily on one input and hope it will fix the issue

1 big difference is if we apply the 600ms on the RoomNameInput, all other inputs under TabNavigator will still have the issue. We couldn't say it's "PR-level change" since we don't know the root cause in TabNavigator (and don't know what other inputs are impacted) at all until my proposal comes.

If somehow we end up deciding to apply the 600ms timeout on only the RoomNameInput, @bernhardoj is indeed first. But my proposal is not the same as that proposal.

[melvin-bot]

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

[shubham1206agra]

@marcochavezf Can you drop this issue to SWM to see if anyone picks this up?

[melvin-bot]

@marcochavezf @CortneyOfstad @shubham1206agra 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]

@marcochavezf @CortneyOfstad @shubham1206agra 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]

@marcochavezf @CortneyOfstad @shubham1206agra 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!

[marcochavezf]

Expert agency might help. Maybe SWM since this is related to navigation

Sounds good, asking them

[CortneyOfstad]

Not overdue!

[kowczarz]

Hello! I'm an expert contributor from Software Mansion and I would like to work on this task.

[shubham1206agra]

@marcochavezf Please assign @kowczarz

[melvin-bot]

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[kowczarz]

Hello! After some research I'm confident to write that since Material Top Tab Navigator under the hood utilise react-native-pager-view there is no simple way to overcome the issue with unfocusing the keyboard after the animation. Pager view wrapped in Material Top Tab Navigator doesn't expose any api that would notify us, that the screen is fully visible and the animation is finished. It also doesn't work with InteractionManager.runAfterInteractions, because in the opposite to navigators based on the react-native-screens library, the animations are done on the native side instead of Animated/reanimated api. I could deep-dive into the pager view code and try to patch it to expose a callback, that would run on animation finished, but it's hard to estimate how long would it take since I don't know the code of that library.

Other solution could be an addition of check if the useAutoFocusInput is being used in native navigator or the one that doesn't support InteractionManager and in the second case we can add some more time to setTimeout, because currently it the focus is called in almost the samem moment as the animation finish and that cause the race condition.

[kowczarz]

Should I proceed with the native changes, or the solution with setTimeout is fine for now?

[melvin-bot]

@marcochavezf @CortneyOfstad @shubham1206agra @kowczarz this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[shubham1206agra]

@kowczarz Please proceed with the native one for now

[kowczarz]

Ok, I will work on it next week

[CortneyOfstad]

Thanks @kowczarz!

[CortneyOfstad]

Hey! Sorry for the confusion here, but we just got an internal update. Per this post, we can close any bugs that are not related to Waves, so closing this out 👍 We will re-address once the waves are completed.

[situchan]

We will re-address once the waves are complete

Isn't it better to hold and move to Monthly? I believe closed issues won't be reopened again.

[CortneyOfstad]

@situchan our plan is to manually reopen the GHs that were closed during this period, so no worries there 👍

[situchan]

Ah ok. Thanks

[shubham1206agra]

@CortneyOfstad Is there any list being maintained for this?