[Awaiting Payment 26th Sept] [$250] Selecting a filter and using keyboard return on the filter confirmation screen ends up clearing filters.

[m-natarajan]

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.25-7
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
Expensify/Expensify Issue URL:
Issue reported by: @JmillsExpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1724762642354929

Action Performed:

1. Go to Search
2. Click on Filters
3. Click on a specific filter
4. Enter your query, use return on your keyboard to confirm
5. You’re taken back to the main filters page, click on another filter
6. Enter your query, use return on your keyboard to confirm
7. Now enter the return key again on the filter summary/confirmation screen

Expected Result:

No filters are cleared

Actual Result:

All filters are cleared, nothing happens.

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

Recording.496.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e1f39a3a6d7c0f4a
• Upwork Job ID: 1828501186366247316
• Last Price Increase: 2024-09-03
• Automatic offers:
• alitoshmatov | Reviewer | 103916367
• suneox | Contributor | 103916369

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[luacmartins]

This is related to focus trap. It seems like the top back button is selected when Enter is pressed, which navigates the user back. This also seems to happen in other flows, e.g. Submit expense

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-08-27 18:38:39 UTC.

Proposal

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

Focus is set on the back button when navigating to the filter page using keyboard

What is the root cause of that problem?

when the filter page is first rendered, the focus trap keeps the focus on the back button and doesn't move to the confirm button.

we solved this issue in the submit report confirmation page by bluring the active element so the focus moves from the back header to the button after the timeout:

App/src/components/MoneyRequestConfirmationList.tsx

Lines 815 to 825 in 735101a

	const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
	useFocusEffect(
	useCallback(() => {
	focusTimeoutRef.current = setTimeout(() => {
	InteractionManager.runAfterInteractions(() => {
	blurActiveElement();
	});
	}, CONST.ANIMATED_TRANSITION);
	return () => focusTimeoutRef.current && clearTimeout(focusTimeoutRef.current);
	}, []),
	);

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

we should do the same in the filter page to keep both flows consistent and don't introduce breaking changes to the focus trap logic.

Optional

this logic can be moved to a hook and used when needed to avoid code duplication

result

Screen.Recording.2024-08-27.at.8.47.38.PM.mov

[Krishna2323]

Proposal

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

• Selecting a filter and using keyboard return on the filter confirmation screen ends up clearing filters.

• This problem was faced in the submit expense flow, to solve that issue we manually moved removed the focus from the back button.

App/src/components/MoneyRequestConfirmationList.tsx

Lines 815 to 825 in e5b6cef

	const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
	useFocusEffect(
	useCallback(() => {
	focusTimeoutRef.current = setTimeout(() => {
	InteractionManager.runAfterInteractions(() => {
	blurActiveElement();
	});
	}, CONST.ANIMATED_TRANSITION);
	return () => focusTimeoutRef.current && clearTimeout(focusTimeoutRef.current);
	}, []),
	);

What is the root cause of that problem?

Due to the focus trap the focus is set to the first focus element instead of the submit button.

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

• Instead of calling the blurActiveElement on every page. I suggest to add the function in the button component.
  

  App/src/components/Button/index.tsx

  Lines 142 to 156 in e5b6cef

  	function KeyboardShortcutComponent({isDisabled = false, isLoading = false, onPress = () => {}, pressOnEnter, allowBubble, enterKeyEventListenerPriority}: KeyboardShortcutComponentProps) {
  	const isFocused = useIsFocused();
  	const activeElementRole = useActiveElementRole();
  	const shouldDisableEnterShortcut = useMemo(() => accessibilityRoles.includes(activeElementRole ?? '') && activeElementRole !== CONST.ROLE.PRESENTATION, [activeElementRole]);
  	const keyboardShortcutCallback = useCallback(
  	(event?: GestureResponderEvent | KeyboardEvent) => {
  	if (!validateSubmitShortcut(isDisabled, isLoading, event)) {
  	return;
  	}
  	onPress();
  	},
  	[isDisabled, isLoading, onPress],
  	);

• When subscribing to the keyboard short, we can check if pressOnEnter is true or not and based on that we will call blurActiveElement. After making this change we can remove all the instances which is used in a component to remove the focus from the back button.

    const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);

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

What alternative solutions did you explore? (Optional)



Result

[tsa321]

Edited by proposal-police: This proposal was edited at 2024-08-28 08:54:02 UTC.

Proposal

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

When entering a search filter on the advanced search page, pressing Enter and then pressing Enter again does not proceed with the search request and ends up clearing the search filter.

What is the root cause of the problem?

When the screen is mounted, the focus trap sets the initialFocus to the first focusable element:

App/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx

Lines 49 to 58 in ac5ee2e

	initialFocus: (focusTrapContainers) => {
	if (!canFocusInputOnScreenFocus()) {
	return false;
	}
	const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
	if (isFocusedElementInsideContainer) {
	return false;
	}
	return undefined;

Since the document's activeElement is outside of the screen (the view result button is not focused), the initialFocus will return undefined and will focus the first element (in this case, the header back button). When the user presses Enter, the back button is pressed instead of the View result button.

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

The focus trap already listens for the focus event and checks whether the focused element is within the element to which it is attached (in this case, within the screen). It will automatically move the focus inside the screen if it is outside. For example, when the user presses the Tab key, the focus trap will focus the first element of the screen if the current active element is outside the screen. Therefore, we do not need the focus trap to auto-focus when the screen is mounted. The auto-focus behavior of the focus trap will also interfere with the auto-focus applied to elements inside the screen.

We can disable the initialFocus. In the FocusTrapForScreen options:

App/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx

Lines 44 to 45 in 59741d8

	focusTrapOptions={{
	trapStack: sharedTrapStack,

we could add an onActivate property with the function: () => document?.activeElement?.blur(). Then, in initialFocus:

App/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx

Lines 49 to 59 in 59741d8

	initialFocus: (focusTrapContainers) => {
	if (!canFocusInputOnScreenFocus()) {
	return false;
	}
	const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
	if (isFocusedElementInsideContainer) {
	return false;
	}
	return undefined;
	},

we should always set it to false or return false.

The focus trap already listens for the focus event and will check whether the focused element is inside the element to which it is attached (in this case, inside the screen) and will automatically move the focus inside the screen if it is outside.

The code change (in legacy mode, to test set USE_REACT_STRICT_MODE to false) could be:

focusTrapOptions={{
+                onActivate: () => {
+                    document?.activeElement?.blur();
+               },
                trapStack: sharedTrapStack,
                allowOutsideClick: true,
                fallbackFocus: document.body,
                delayInitialFocus: CONST.ANIMATED_TRANSITION,
                initialFocus: (focusTrapContainers) => {
-                     if (!canFocusInputOnScreenFocus()) {
-                        return false;
-                     }

-                     const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
-                     if (isFocusedElementInsideContainer) {
-                        return false;
-                     }
-                    return undefined;
+                    return false;
                },
                setReturnFocus: (element) => {
+                    return false;
-                    if (document.activeElement && document.activeElement !== document.body) {
-                        return false;
-                    }
-                    return element;
                },
                ...(focusTrapSettings?.focusTrapOptions ?? {}),
            }}

What alternative solutions did you explore? (2)

On other pages, we use useAutoFocusInput(); as shown in:

App/src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx

Line 43 in 3e5da78

const {inputCallbackRef} = useAutoFocusInput();

App/src/pages/OnboardingWork/BaseOnboardingWork.tsx

Line 35 in 3e5da78

const {inputCallbackRef} = useAutoFocusInput();

App/src/pages/iou/request/step/IOURequestStepMerchant.tsx

Line 58 in 3e5da78

const {inputCallbackRef} = useAutoFocusInput();

etc..

We could also use it for AdvancedSearchFilters. To do this, we can add:

const { inputCallbackRef } = useAutoFocusInput();

Then, in:

App/src/pages/Search/AdvancedSearchFilters.tsx

Lines 316 to 317 in 3e5da78

	<FormAlertWithSubmitButton
	buttonText={translate('search.viewResults')}

we add buttonRef={inputCallbackRef}. Additionally, since the hook is general and not limited to text input, we can rename it if necessary.

[melvin-bot]

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

[alitoshmatov]

@abzokhattab Thank you for your proposal, your RCA is correct and solution does solve the issue

[alitoshmatov]

@Krishna2323 Thank you for your proposal, I think you solution is great since it addresses all the instances of this issue

[alitoshmatov]

@tsa321 Thank you for your proposal, I think we should not change current logic of focus trap

[abzokhattab]

I agree that the other proposal addresses the issue in other components as well, but I believe it could be a risky change since it's used extensively so we should be careful when modifying the focus.

I'm concerned that this could introduce regressions in the enterKeyEventListenerPriority logic, as the button will always be focused during rendering, causing the listener priority to be ignored.

I think a safer option would be to create a hook and call it when needed. However, I'm open to hearing your thoughts on this, @alitoshmatov, @trjExpensify, @luacmartins.

[suneox]

Proposal

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

Avoid initial focus trap element while container includes button has an enter event listener

What is the root cause of that problem?

We only have condition isFocusedElementInsideContainer to prevent set initial focus but at AdvancedSearchFilters screen we don't have any focused element so the initial focus will be set to the first focusable element in the screen container.

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

To prevent this issue from occurring in other places, we should add a new condition that prevents setting the initial focus when the container includes a button with an enter event listener.

      const isFocusedElementInsideContainer = !!focusTrapContainers?.some((container) => container.contains(document.activeElement));
+     const isButtonHasEnterListener = !!focusTrapContainers?.some((container) => !!container.querySelector('button[data-listener="ENTER"]'));
+     if (isFocusedElementInsideContainer || isButtonHasEnterListener) {
-     if (isFocusedElementInsideContainer) {
          return false;
      }

Update Button component to add a new attribute data-listener

    <PressableWithFeedback
+       dataSet={{
+           listener: !!pressOnEnter ? 'ENTER' : undefined,
+       }}
        ref={ref}
        onPress={(event) => {

What alternative solutions did you explore? (Optional)

[tsa321]

Proposal

updated
@alitoshmatov I have updated my proposal on alternative solution(2), to use useAutoFocusInput like in other pages

[alitoshmatov]

I think we should go with @abzokhattab 's proposal and solve it for current issue.

C+ reviewed 🎀 👀 🎀

[melvin-bot]

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

[Krishna2323]

@alitoshmatov, any comments on my proposal? It will fix all the pages with pressOnEnter buttons. We have many pages with same issue.

Monosnap.screencast.2024-09-04.23-47-07.mp4

[suneox]

I think we should find an overall solution to prevent initialFocus early when the screen has a focused element or a button listening for the enter event, instead of relying on trigger initialFocus and then using blurActiveElement (note: initialFocus doesn’t disable the focus trap functionality).

[luacmartins]

I agree that we should look at a holistic solution for other pages as well, so we don't end up with 42 different ways to solve the same problem. Let's fix the root cause.

[melvin-bot]

@trjExpensify, @luacmartins, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

[trjExpensify]

I agree that we should look at a holistic solution for other pages as well, so we don't end up with 42 different ways to solve the same problem. Let's fix the root cause.

💯 agree, anyone got thoughts on a proposal for that?

[suneox]

@alitoshmatov How about my proposal? It can prevent the initial focus early, rather than deactivating the focused element.

[melvin-bot]

@trjExpensify @luacmartins @alitoshmatov 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]

@trjExpensify, @luacmartins, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

[alitoshmatov]

I think @suneox 's proposal makes sense now. It goes to source of the problem, and will make sure focus is not reset into first focusable element when there is a button with pressOnEnter attribute.

@luacmartins What do you think?

[luacmartins]

Agreed, let's do it.

[melvin-bot]

📣 @alitoshmatov 🎉 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

[melvin-bot]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[trjExpensify]

PR on prod, updated the title to reflect the payment hold.

[melvin-bot]

@suneox @trjExpensify @luacmartins @alitoshmatov this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[trjExpensify]

Go away, melvin.

[trjExpensify]

Payment summary as follows:

• $250 to @suneox for the fix
• $250 to @alitoshmatov for the C+ review

Paid, closing!