[$125] Taxes - BNP input is not auto focused in add rate page

[lanitochka17]

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.54-0
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/5134453
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Launch New Expensify app
2. Go to workspace settings > Taxes
3. Tap Add rate
4. Tap Value

Expected Result:

BNP input will be auto focused

Actual Result:

BNP input is not auto focused

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

Bug6647125_1730008681078.ScreenRecording_10-27-2024_13-52-22_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021850971328621900159
• Upwork Job ID: 1850971328621900159
• Last Price Increase: 2024-11-11

Issue Owner

Current Issue Owner: @Ollyws

[melvin-bot]

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

[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]

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

[lanitochka17]

Production:

RPReplay_Final1730050554.MP4

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-10-28 15:51:43 UTC.

Proposal


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

Taxes - BNP input is not auto focused in add rate page

What is the root cause of that problem?

• I don't think this is caused by any recent PR.
• We aren't focusing the input when the modal is visible.

App/src/components/AmountPicker/AmountSelectorModal.tsx

Lines 42 to 48 in 5103b4b

	<AmountForm
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...rest}
	autoFocus
	value={currentValue}
	onInputChange={setValue}
	/>

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


• To focus on the amount input whenever the modal is set to visible we should use the same method as we do in
  

  App/src/components/TextPicker/TextSelectorModal.tsx

  Lines 27 to 33 in 5103b4b

  	const inputRef = useRef<BaseTextInputRef | null>(null);
  	const inputValueRef = useRef(value);
  	const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
  	const inputCallbackRef = (ref: BaseTextInputRef | null) => {
  	inputRef.current = ref;
  	};

  
  

  App/src/components/TextPicker/TextSelectorModal.tsx

  Lines 56 to 71 in 5103b4b

  	useFocusEffect(
  	useCallback(() => {
  	focusTimeoutRef.current = setTimeout(() => {
  	if (inputRef.current && isVisible) {
  	inputRef.current.focus();
  	(inputRef.current as TextInputType).setSelection?.(inputValueRef.current?.length ?? 0, inputValueRef.current?.length ?? 0);
  	}
  	return () => {
  	if (!focusTimeoutRef.current || !isVisible) {
  	return;
  	}
  	clearTimeout(focusTimeoutRef.current);
  	};
  	}, CONST.ANIMATED_TRANSITION);
  	}, [isVisible]),
  	);

  
  

  App/src/components/TextPicker/TextSelectorModal.tsx

  Lines 106 to 107 in 5103b4b

  	<InputWrapper
  	ref={inputCallbackRef}

• We should add the following in AmountSelectorModal.

    // Store the ref of the input
    const inputRef = useRef<BaseTextInputRef | null>(null);
    const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);

    const inputCallbackRef = (ref: BaseTextInputRef | null) => {
        inputRef.current = ref;
    };

    // use "useFocusEffect" focus on the input whenever "isVisible" state is changed.
    useFocusEffect(
        useCallback(() => {
            focusTimeoutRef.current = setTimeout(() => {
                if (inputRef.current && isVisible) {
                    inputRef.current.focus();
                }
                return () => {
                    if (!focusTimeoutRef.current || !isVisible) {
                        return;
                    }
                    clearTimeout(focusTimeoutRef.current);
                };
            }, CONST.ANIMATED_TRANSITION);
        }, [isVisible, inputRef]),
    );
    
    // Pass the ref to "AmountForm"
    ref={(ref) => inputCallbackRef(ref)}

• We also need to remove the autoFocus prop.

What alternative solutions did you explore? (Optional)

Result

tax_amount_not_focused.mp4

[Beamanator]

@Krishna2323

I don't think this is caused by any recent PR.

But then wouldn't that mean that the issue should be reproducible in prod?

[Beamanator]

I'm going to call this NAB - also I don't know if we need it to look "focused" - pressing numbers works fine

[Krishna2323]

@Beamanator, I tried again to look for a recent PR that might have caused this issue, but I don’t believe any recent PR is responsible—though I may be wrong here. In my opinion, the root cause is the use of the autoFocus prop, which doesn’t always work. In all other components, we use inputCallbackRef from the useAutoFocusInput hook, and the same component is then focused correctly.

App/src/pages/workspace/rules/RulesMaxExpenseAmountPage.tsx

Lines 68 to 75 in 4aa19bf

	<InputWrapper
	label={translate('iou.amount')}
	InputComponent={AmountForm}
	inputID={INPUT_IDS.MAX_EXPENSE_AMOUNT}
	currency={CurrencyUtils.getCurrencySymbol(policy?.outputCurrency ?? CONST.CURRENCY.USD)}
	defaultValue={defaultValue}
	isCurrencyPressable={false}
	ref={inputCallbackRef}

App/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx

Lines 86 to 92 in 4aa19bf

	<InputWrapperWithRef
	InputComponent={AmountForm}
	inputID={INPUT_IDS.RATE}
	fixedDecimals={CONST.MAX_TAX_RATE_DECIMAL_PLACES}
	isCurrencyPressable={false}
	currency={currency}
	ref={inputCallbackRef}

I think we need to fix this to be consistent will all other similar pages:

Inconsistency

tax_amount_not_focused_issue.mp4

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $125

[Ollyws]

Reproducible, but also I agree with:

But then wouldn't that mean that the issue should be reproducible in prod?

What was the cause here?

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[thienlnam]

Let's get a fix up at this point - @Ollyws can you review the proposal?

[QichenZhu]

The root cause is likely from upstream, as this happens only after enabling Bridgeless mode, but adding a delay as @Krishna2323 suggested might be the quickest option for now.

[Ollyws]

Ok if we're already using the proposed approach in TextSelectorModal then let's go with @Krishna2323's proposal.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @thienlnam is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

@garrettmknight, @Ollyws, @thienlnam Huh... This is 4 days overdue. Who can take care of this?

[thienlnam]

Hmm, actually we shouldn't be adding setTimeouts here - I'm not sure how that other approach made it into the codebase. That doesn't address the root cause of the issue

[Krishna2323]

@thienlnam, using setTimeout is very common in cases where we need to focus on the input multiple times when the screen remains open, but a modal is opening and closing (AmountForm is a modal). We even use setTimeout in the useAutoFocusInput hook.

App/src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx

Lines 98 to 133 in 3023d60

	useImperativeHandle(innerRef, () => ({
	focus() {
	inputValidateCodeRef.current?.focus();
	},
	focusLastSelected() {
	if (!inputValidateCodeRef.current) {
	return;
	}
	if (focusTimeoutRef.current) {
	clearTimeout(focusTimeoutRef.current);
	}
	focusTimeoutRef.current = setTimeout(() => {
	inputValidateCodeRef.current?.focusLastSelected();
	}, CONST.ANIMATED_TRANSITION);
	},
	}));
	useFocusEffect(
	useCallback(() => {
	if (!inputValidateCodeRef.current) {
	return;
	}
	if (focusTimeoutRef.current) {
	clearTimeout(focusTimeoutRef.current);
	}
	focusTimeoutRef.current = setTimeout(() => {
	inputValidateCodeRef.current?.focusLastSelected();
	}, CONST.ANIMATED_TRANSITION);
	return () => {
	if (!focusTimeoutRef.current) {
	return;
	}
	clearTimeout(focusTimeoutRef.current);
	};
	}, []),
	);

App/src/hooks/useAutoFocusInput.ts

Lines 41 to 54 in 3023d60

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

[thienlnam]

We can update this form so it's consistent with the rest, but it's still a workaround. Ideally we'd solve this upstream for all of these cases and remove them, however not something we need to do in this issue

[melvin-bot]

@garrettmknight, @Ollyws, @thienlnam 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[Krishna2323]

however not something we need to do in this issue

@thienlnam can get assigned then? This also requires an effect to focus on input every time we close the amount modal, so that's isn't an an upstream issue.