Start chat - Transition between Room & Chat tab is not smooth

[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: 1.4.24-4
Reproducible in staging?: Y
Reproducible in production?: N
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: Applause - Internal Team
Slack conversation:

Action Performed:

1. Click on FAB > Start chat > Switch between Room & Chat tab multiple times

Expected Result:

Transition between tabs should be smooth

Actual Result:

Transition between tabs is not smooth

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

Bug6339068_1704989190844.Screen_Recording_2024-01-11_at_6.19.28_in_the_evening.mp4

View all open jobs on GitHub

[github-actions]

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

[melvin-bot]

Triggered auto assignment to @robertjchen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[shubham1206agra]

@kacper-mikolajczak Can you check this problem?
Offending PR #33841

[situchan]

Seems like bug here is "no transition animation"
But we intentionally disabled animation since it's better than freezing

[kacper-mikolajczak]

Seems like bug here is "no transition animation"
But we intentionally disabled animation since it's better than freezing

As @situchan mentioned, animations were purposely disabled here #31190

Let me know if the issue raised here involves only animations or something extra - if yes, then I am happy to look into that 👍

CC @lanitochka17 @shubham1206agra

[shubham1206agra]

@kacper-mikolajczak As per video, I can notice some stutter while transitioning. Can you please look into that?

[kacper-mikolajczak]

@shubham1206agra Definitely 🔧

[kacper-mikolajczak]

Hi! After inspecting here and there, it looks like a nasty race condition caused by the input autofocus. I need more time to establish with certainty what is causing race condition, so if you could assign the issue to me. Thanks! 👍

Here is the recording that shows the reproduction of the bug whenever I time it right (first transition is too fast). After waiting until autofocus kicks in, the transition seems to start working fine:

before.mp4

And here is the sample with disabled autofocus for RoomName input in Workspace screen that shows no shakiness even with right timing:

after.mp4

CC: @shubham1206agra @lanitochka17

[robertjchen]

Great point, let us know what you find!

[kacper-mikolajczak]

Hi everyone! 👋

While I was investigating the issue from technical side of things, I realised that the autofocus in current form seems a bit off. Thus, I wanted to discuss with you a reason behind having an autofocus in this case and how we can improve it. Thanks for your insight!

Imho, If user already visited screen (e.g. filled some inputs), then we should not bring back focus to the first input - it should stay where it was. Current behaviour should be preferred only on first visit or when form is still in clean state.

As an example, here is a recording of undesired behaviour where change of the focus breaks UX heavily:

unwanted-focus.mov

For this (and other minor technical concerns) I keep PR in draft. There are more details in PR itself, so feel free to check this out 👍

CC: @robertjchen

[bernhardoj]

What if we just cancel the focus to happen when the tab is unfocused?

Currently, we only clear the timeout when out of focus,

App/src/hooks/useAutoFocusInput.ts

Lines 33 to 44 in 4da7fda

	useCallback(() => {
	focusTimeoutRef.current = setTimeout(() => {
	setIsScreenTransitionEnded(true);
	}, CONST.ANIMATED_TRANSITION);
	return () => {
	if (!focusTimeoutRef.current) {
	return;
	}
	clearTimeout(focusTimeoutRef.current);
	};
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []),

but the interaction manager is still firing.

App/src/hooks/useAutoFocusInput.ts

Lines 26 to 29 in 4da7fda

	InteractionManager.runAfterInteractions(() => {
	inputRef.current?.focus();
	setIsScreenTransitionEnded(false);
	});

So, we just need to cancel it too.

return () => {
+   interactionRef.current?.cancel()
+   setIsScreenTransitionEnded(false);
    if (!focusTimeoutRef.current) {
        return;
    }
    clearTimeout(focusTimeoutRef.current);
};

[kacper-mikolajczak]

Hi @bernhardoj 👋

Thanks a lot for your feedback! 👍

While the suggested changes are sufficient to fix original issue, I'd recommend we discuss autofocus behaviour in general. Just for the quick recap, here are the points that I am worried about:

• The timeout in useAutoFocusInput is set to fixed duration of CONST.ANIMATED_TRANSITION regardless of the hook usage. Are we sure the useAutoFocusInput should always fire after that time? In case where there is no animation (as the one we are talking about) we could trigger focus straight away instead of waiting which is bad for UX
• Setting focus after each transition to the same input is again bad from UX standpoint - more details in the Start chat - Transition between Room & Chat tab is not smooth #34377 (comment)

Having that said, we can also move the discussion into separate issue so it does not block the actual fix 🔧

In that case, your code is a great way of achieving that - thanks! I would just move the cancellation of the InteractionManager task into useEffect cleanup function. Like this:

        const focusTaskHandle = InteractionManager.runAfterInteractions(() => {
            inputRef.current?.focus();
            setIsScreenTransitionEnded(false);
        });

        return () => {
            focusTaskHandle.cancel();
        };

Thus we don't need to create a ref for that as it is running in the same scope. We will trigger the cleanup in the useFocusEffect cleanup as well via setting the isScreenTransitionEnded state to false as you suggested 👍

I am wrapping up the PR and we are good to go :)

[melvin-bot]

This issue has not been updated in over 15 days. @robertjchen, @kacper-mikolajczak eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[shubham1206agra]

@robertjchen You may close this. This has been fixed already.