[HOLD #11795][$1000] No transition animation on timezone selector

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. go to settings > profile > timezone
2. now open timezone selector

Expected Result:

there should be a transition animation while it opens

Actual Result:

no transition animation happens

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.50-0
Reproducible in staging?: y
Reproducible in production?: y
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-08-01.at.3.03.12.PM.mov

Recording.1442.mp4

Expensify/Expensify Issue URL:
Issue reported by: @chiragxarora
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690882305493399

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0177d92223610217d2
• Upwork Job ID: 1687469082823692288
• Last Price Increase: 2023-08-11
• Automatic offers:
• chiragxarora | Contributor | 26163977
• chiragxarora | Reporter | 26163980

[melvin-bot]

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

[kavimuru]

Proposal from @chiragxarora

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

no transition anmation on timezone selector

What is the root cause of that problem?

Root cause of the issue is focusing logic in our SelectionListRadio component, as we can see currently timezone page uses a slection radio list.
Now the current logic of focusing the text input here is defined like this

App/src/components/SelectionListRadio/BaseSelectionListRadio.js

Lines 173 to 188 in 52b8ddc

	useEffect(() => {
	if (shouldShowTextInput) {
	if (props.shouldDelayFocus) {
	focusTimeoutRef.current = setTimeout(() => textInputRef.current.focus(), CONST.ANIMATED_TRANSITION);
	} else {
	textInputRef.current.focus();
	}
	}
	return () => {
	if (!focusTimeoutRef.current) {
	return;
	}
	clearTimeout(focusTimeoutRef.current);
	};
	}, [props.shouldDelayFocus, shouldShowTextInput]);

Currently the text input is focused conditionally based on shouldDelayFocus prop and we have not used that prop in our timezone selector, so it instantly focuses the input leading to no animation

App/src/pages/settings/Profile/TimezoneSelectPage.js

Lines 119 to 126 in 52b8ddc

	<SelectionListRadio
	textInputLabel={this.props.translate('timezonePage.timezone')}
	textInputValue={this.state.timezoneInputText}
	onChangeText={this.filterShownTimezones}
	onSelectRow={this.saveSelectedTimezone}
	sections={[{data: this.state.timezoneOptions, indexOffset: 0, isDisabled: this.timezone.automatic}]}
	initiallyFocusedOptionKey={_.get(_.filter(this.state.timezoneOptions, (tz) => tz.text === this.timezone.selected)[0], 'keyForList')}
	/>

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

We need to alter the focusing logic in that page and we shall wait for the transitions to end before we focus. For this, we need to wrap our component in ScreenWrapper and then we can use the onEntryTransitionEnd prop which is called after transitions end, and focus the textinput inside it using the ref we already have in the page.
Edit: Upon further investigation, I found out that some radio lists are rendered inside the modal like year picker, state picker, country picker, and for them, onEntryTransitionEnd is never called, so to make it for all the radio lists, we can add a check state to look for transition end (for the non modal cases) and focus them accordingly

const [didTrasitionEnd, setDidTransitionEnd] = useState(false);
onEntryTransitionEnd={() => setDidTransitionEnd(true)}

and replace this line

App/src/components/SelectionListRadio/BaseSelectionListRadio.js

Line 178 in 52b8ddc

textInputRef.current.focus();

with this

if(didTrasitionEnd) textInputRef.current.focus();

Results

https://github.com/chiragxarora/pdf-download/assets/54641656/8f21b0f8-d3c4-45d8-ba00-2417015fe04d

### What alternative solutions did you explore? (Optional) We can keep the SelectionListRadio component as it is and simply add shouldDelayFocus prop to the selector in timezone page

[jfquevedol2198]

Proposal

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

No transition animation on timezone selector

What is the root cause of that problem?

App/src/components/SelectionListRadio/BaseSelectionListRadio.js

Lines 173 to 180 in 52b8ddc

	useEffect(() => {
	if (shouldShowTextInput) {
	if (props.shouldDelayFocus) {
	focusTimeoutRef.current = setTimeout(() => textInputRef.current.focus(), CONST.ANIMATED_TRANSITION);
	} else {
	textInputRef.current.focus();
	}
	}

Without shouldDelayFocus props, BaseSelectionListRadio component with TextInput element tries to focus on TextInput when its mounted. Focusing on Text in the beginning prevents transition of ScreenWrapper component.

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

Pass shouldDelayFocus props in SelectionListRadio element inside TimezoneSelectPage render method.

<SelectionListRadio
    shouldDelayFocus
    ...
/>

This is happening on Pronouns setting page, too. we need to pass shouldDelayFocus props in SelectionListRadio element inside PronounsPage render method.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[melvin-bot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[b4s36t4]

Proposal

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

No transition animation on timezone selector

What is the root cause of that problem?

RCA for this issue pretty straight forwarded and already observed at many places. Same here in the timezone select page we have an input which is getting autoFocused which causing the cluttered/no-animation for component.

Since we're setting focus to the input before completing the rendering of modal(Transition) this issue happening.

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

I feel this proposal is tricky and long but I feel this should be done like this only.

Steps to solve this.

Step 1:

App/src/components/SelectionListRadio/index.js

Line 29 in 9294e9e

<BaseSelectionListRadio

here we're passing the ref to BaseSelectionListRadio component which is functional component.

But we're not using forwardRef here which is causing an error in console. As part of the solution we have to fix this issue which will let us use the ref inside the BaseSelectionListRadio component.

Step 2:
Write an imperative handler to focus the input. Since BaseSelectionListRadio is a functional component, using useImperativeHandler hook should help here.

Inside the hook we should have a function maybe like focusInput which focuses the input.

    useImperativeHandlers(props.forwardedRef, () => ({
        focusInput: () =>{
            textInputRef.current.focus()
        }
    }))

Step 3:
Create a new prop called autoFocus and set it's default value to true to support the backward compatibility this default value help us to not get regressions and not to raise the bugs where the same component get's used.

Step 4:
Restrict focusing input when the autoFocus prop is false

App/src/components/SelectionListRadio/BaseSelectionListRadio.js

Line 174 in ec7b267

if (shouldShowTextInput) {

     if (shouldShowTextInput && props.autoFocus) {
        // Run the focus logic here.
        // we also have to add `props.autoFocus` to `useEffect` dependency.
     }

which results makes the animation to work perfectly when this false passed here

App/src/pages/settings/Profile/TimezoneSelectPage.js

Line 119 in 9294e9e

<SelectionListRadio

in this component.

<SelectionListRadio
    {...props}
    autoFocus={false}
    />

Step 5:
use onEntryTransitionEnd prop for the ScreenWrapper component and access the ref and call focusInput function which we created using the imperative handler.

create a new ref inside TimezoneSelectPage component. ex: this.selectionListRef = createRef(null)

Assign the selectionListRadio's ref to this.selectionListRef.

 ref={(el) => this.selectionListRef.current = ref}

call the focusInput inside the onEntryTransitionEnd function.

onEntryTransitionEnd={() => {
    this.selectionListRef.current && this.selectionListRef.current.focusInput()
}}

What alternative solutions did you explore? (Optional)

Simply pass the shouldDelayFocus here should also do the work. But previously I proposed same type of solution like adding shouldDelayFocus got rejected because shouldDelayFocus is using a time-out function, so proposing the above solution.

Or if we feel all this is complex logic which might cause regressions later down the line we could simply do conditional rendering inside TimezoneSelectPage component.

We could simple use didScreenTransitionEnd passed by ScreenWrapper component and show the loader while it was false and show the component when it becomes true which does the same work.

The proposed solution is also working with the focusing even after re-fresh whereas other solutions might work or not.

Result

Kapture.2023-08-05.at.01.57.49.mp4

[robertKozik]

Hi all! Thank you all for your proposals!

1. @chiragxarora - I like your proposal, could you provide me where we are displaying the radio list on modal? I want to check this scenario too
2. @jfquevedol2198 - I believe we should't accept the proposal which enables the useTimeout workaround when we have other possible solutions. useTimeout is the last resort

[chiragxarora]

here @robertKozik

App/src/components/StatePicker/StateSelectorModal.js

Lines 67 to 82 in 8639fb6

	<Modal
	type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
	isVisible={isVisible}
	onClose={onClose}
	onModalHide={onClose}
	hideModalContentWhileAnimating
	useNativeDriver
	>
	<HeaderWithBackButton
	title={label || translate('common.state')}
	shouldShowBackButton
	onBackButtonPress={onClose}
	/>
	<SelectionListRadio
	headerMessage={headerMessage}
	textInputLabel={label || translate('common.state')}

[chiragxarora]

same with year picker, country picker

[chiragxarora]

here's the result video, it didn't render properly in the GH comment above

Screen.Recording.2023-08-01.at.3.21.14.PM.mov

[chiragxarora]

did you have a look @robertKozik ?

[b4s36t4]

@robertKozik handling conditional logic inside the base component doesn't make sense to me. I feel since we're having a ref(but not handled) ref can handle this here. And this is the approach at many places we're following i.e accessing the ref to manage the focus if not going with shouldDelayFocus prop. Any thoguhts?

[robertKozik]

I have mixed thoughts for this. The @chiragxarora approach makes sense for me, as it's quite simple and doing it's job. But you pointed out good counter argument. It's better to keep base component with no "outside" logic

@b4s36t4 did you check these places which @chiragxarora pointed out - with timezone selector as modal? Does your solution work for them too?

[b4s36t4]

@robertKozik I think the places which @chiragxarora mentioned are component not screens. As far as I know screens only have animations not components. Although I'll get back to you once I check them.

[chiragxarora]

my proposal is making use of the ref only, its focusing using the input ref, by making sure the transition has ended using onEntryTransitionEnd prop, which is what we have used everywhere in the app. What's foreign/outside logic in this? Could you explain me @b4s36t4 @robertKozik

I'm not sure if you've seen @b4s36t4 's proposal or not but it's the one which brings new foreign hooks useImperativeHandlers in the app which are used nowhere. That solution is just a different yet complex version of mine and idk what drawbacks does mine have which it is overcoming. Also, it wont work for the cases I've mentioned, since onEntryTransitionEnd isn't called for modals

And this is the approach at many places we're following i.e accessing the ref to manage the focus if not going with shouldDelayFocus prop.

This is what I am doing

[chiragxarora]

please explain the drawbacks of my proposal @b4s36t4 @robertKozik

[b4s36t4]

@chiragxarora there are no drawbacks with your issue. the solution which is proposed is having outside logic which is the state you'll be introducing didTrasitionEnd and you're passing that state to the entire base component. Also to handle the focus you need to add that to dependencies of useEffect which might (not sure) can cause regressions.

The main thing I'm worried is passing a state which is derived from a screen to the base component is not what the solution should do.

I feel we have forwardRef in the SelectionListRadio component which is passed down to BaseSelectionListRadio and using imperative handle does make sense to me.

Also the modals which you listed above uses shouldDelayFocus prop and they're working prefect. I'm not against using shouldDelayFocus if @robertKozik feels ok with shouldDelayFocus then I feel that's ok too. All I'm worried about brining the new state all the way to base component which is not good.

Also it doesn't have to be onEntryTransitionEnd to focus the input, for modals we have onModalHide which will only be called after modal is rendered (checked ;)).

Decision is upto @robertKozik. I feel we don't touch the ones which are working fine with shouldDelayFocus or let's just go with shouldDelayFocus.

[melvin-bot]

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

Offer link
Upwork job

[chiragxarora]

PR #25414 is ready for review @robertKozik

[dangrous]

Based on Slack, I believe we have decided to close out this PR - but not the issue - and it will be fixed in the refactor of this component. We should still pay out @robertKozik and @chiragxarora as if the PR went through. Does that sound right to everyone?

[robertKozik]

@dangrous Sure! I'd like to remind that I'm from Software Mansion (an expert agency), so I'm not eligible for payment.

[chiragxarora]

sounds good to me @dangrous

[dangrous]

Okay cool, I'll put this on hold for #22622 for now, and we can test and confirm and pay once that's all complete

[b4s36t4]

#25894 I think the same approach I suggested being followed here to focus the input instead of using passing and it's accepted too :)

[b4s36t4]

Also that PR also solves this issue.

cc: @robertKozik @dangrous

[dangrous]

Thanks @b4s36t4! I've called out in that issue that it'll likely get solved by the SelectionList refactor just like this one.

[dangrous]

Still on hold

[b4s36t4]

@dangrous I believe SelectionList refactor is complete, isn't it?

[dangrous]

not according to that issue, but I can confirm. I believe they were splitting it up into three phases, and this issue would only be solved by phase 2. @thiagobrez do you mind confirming when you have a moment? Thank you! (Re this Slack conversation).

[thiagobrez]

Hey @dangrous! Yep, phase 3 is ongoing, and this was solved in phase 2.

More specifically, the regression was solved by this PR #25921.

And the improvements to the logic we discussed in the slack thread was solved here (on hold because of merge freeze)
#26415

[dangrous]

Oh, sweet! @robertKozik do you mind testing this again to see if we can close out?

[dangrous]

bump for testing @robertKozik when you have time! Should be quick - thanks

[robertKozik]

Sure! Missed that one, on it 👀

[robertKozik]

Can confirm, transition is present and issue is fixed 🚀

[dangrous]

Sweet! So I think we just need payment for @chiragxarora, as both reporter and contributor - even though the PR was not used, we agreed to pay as if it had. Does that seem right to everyone?

[dangrous]

@bfitzexpensify do you think you can sort out payment for @chiragxarora this week? Thanks!

[bfitzexpensify]

All sorted!