App [$250] Submit expense - Tabs disappear when changing from Scan to Distance via SHIFT+TAB

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: 1.4.83-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4626993 Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to FAB > Submit expense
Go to Scan
Click on empty area on the RHP (like the empty header area)
Press SHIFT+TAB

Expected Result:

Nothing will happen

Actual Result:

RHP changes to Distance flow, and tabs (Manual, Scan, Distance) are missing

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/d86b3e9c-7d57-4722-b6ae-547be8e690f1

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0112bb5e5d8d723e7f
Upwork Job ID: 1801340710232481793
Last Price Increase: 2024-07-11

Issue Owner

Current Issue Owner: @rayane-djouah

Jun 13 '24 18:06 lanitochka17

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

Jun 13 '24 18:06 melvin-bot[bot]

:wave: 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

Jun 13 '24 18:06 github-actions[bot]

@tgolen FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

Jun 13 '24 18:06 lanitochka17

We think that this bug might be related to #vip-vsp

Jun 13 '24 18:06 lanitochka17

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

Jun 13 '24 19:06 melvin-bot[bot]

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

Jun 13 '24 19:06 melvin-bot[bot]

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

Jun 13 '24 19:06 melvin-bot[bot]

I'm demoting this one and assigning to external.

Jun 13 '24 19:06 tgolen

cannot reproduce in production release. Offending PR: https://github.com/Expensify/App/pull/39520

Jun 13 '24 23:06 tsa321

Not produceable on Production and local

Jun 14 '24 10:06 studentofcoding

Proposal

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

Pressing SHIFT+TAB on the top section of the RHP updates the contents of the RHP.

What is the root cause of that problem?

The current implementation has a common FocusTrap wrapper (ScreenWrapper) for all three screens (Manual, Scan & Distance).

{* Common wrapper for all the three screens *}
<ScreenWrapper
    includeSafeAreaPaddingBottom={false}
    shouldEnableKeyboardAvoidingView={false}
    shouldEnableMinHeight={DeviceCapabilities.canUseTouchScreen()}
    headerGapStyles={isDraggingOver ? [styles.receiptDropHeaderGap] : []}
    testID={IOURequestStartPage.displayName}
>
    {({safeAreaPaddingBottomStyle}) => (
        <DragAndDropProvider
            setIsDraggingOver={setIsDraggingOver}
            isDisabled={selectedTab !== CONST.TAB_REQUEST.SCAN}
        >
            <View style={[styles.flex1, safeAreaPaddingBottomStyle]}>
                <HeaderWithBackButton
                    title={tabTitles[iouType]}
                    onBackButtonPress={navigateBack}
                />
                {iouType !== CONST.IOU.TYPE.SEND && iouType !== CONST.IOU.TYPE.PAY && iouType !== CONST.IOU.TYPE.INVOICE ? (
                    <OnyxTabNavigator
                        id={CONST.TAB.IOU_REQUEST_TYPE}
                        onTabSelected={resetIOUTypeIfChanged}
                        tabBar={TabSelector}
                    >
                        <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
                            {() => (
                                <IOURequestStepAmount
                                    shouldKeepUserInput
                                    route={route}
                                />
                            )}
                        </TopTab.Screen>
                        <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>{() => <IOURequestStepScan route={route} />}</TopTab.Screen>
                        {shouldDisplayDistanceRequest && <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>{() => <IOURequestStepDistance route={route} />}</TopTab.Screen>}
                    </OnyxTabNavigator>
                ) : (
                    <IOURequestStepAmount route={route} />
                )}
            </View>
        </DragAndDropProvider>
    )}
</ScreenWrapper>

When a click is made on the top empty area on the RHP (like the empty header area), the focus gets initiated on the common FocusTrap wrapper. Pressing SHIFT+TAB focuses the last element on the DOM tree which is the Next button of the Distance screen. (In the issue repro video, we can observe that the Next button of the Distance screen is focused)
Because of the translating nature of the Tabs component OnyxTabNavigator, the tabs (Manual, Scan, Distance) are translated to the left by 750px when the Next button of the Distance screen is focused. This is the reason behind missing of the tabs.

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

Since the TAB navigation focus is already handled by focus-trap library, we can remove the below code block in src/pages/iou/request/IOURequestStartPage.tsx.

useFocusEffect(
    useCallback(() => {
        const handler = (event: KeyboardEvent) => {
            if (event.code !== CONST.KEYBOARD_SHORTCUTS.TAB.shortcutKey) {
                return;
            }
            event.preventDefault();
            event.stopPropagation();
        };
        KeyDownPressListener.addKeyDownPressListener(handler);

        return () => KeyDownPressListener.removeKeyDownPressListener(handler);
    }, []),
);

The fix for the issue is to only include the interactive elements of the focussed tab in the TAB navigation behavior order i.e. when the Manual tab is selected, the interactive elements of the Split & Distance tab should be removed from the TAB navigation tree by visually hiding them. FOR PROPOSAL PURPOSES, ONLY A POC-LEVEL CODE IS ADDED BELOW. WILL ADD THE STANDARDIZED CODE WHEN THE SOLUTION IS ACCEPTED. The major update from the last proposal is that we are using the containerElements (https://www.npmjs.com/package/focus-trap-react#containerelements) prop of the focus-trap-react library.

Update the below in src/pages/iou/request/IOURequestStartPage.tsx:

<ScreenWrapper
    includeSafeAreaPaddingBottom={false}
    shouldEnableKeyboardAvoidingView={false}
    shouldEnableMinHeight={DeviceCapabilities.canUseTouchScreen()}
    headerGapStyles={isDraggingOver ? [styles.receiptDropHeaderGap] : []}
    testID={IOURequestStartPage.displayName}
    selectedTab={selectedTab}
>
    {({safeAreaPaddingBottomStyle}) => (
        <DragAndDropProvider
            setIsDraggingOver={setIsDraggingOver}
            isDisabled={selectedTab !== CONST.TAB_REQUEST.SCAN}
        >
            <View style={[styles.flex1, safeAreaPaddingBottomStyle]}>
                <View id={`${CONST.TAB.IOU_REQUEST_TYPE}_header`}>
                    <HeaderWithBackButton
                        title={tabTitles[iouType]}
                        onBackButtonPress={navigateBack}
                    />
                </View>
                {iouType !== CONST.IOU.TYPE.SEND && iouType !== CONST.IOU.TYPE.PAY && iouType !== CONST.IOU.TYPE.INVOICE ? (
                    <OnyxTabNavigator
                        id={CONST.TAB.IOU_REQUEST_TYPE}
                        onTabSelected={resetIOUTypeIfChanged}
                        tabBar={(props) => (
                            <TabSelector
                                {...props}
                                id={`${CONST.TAB.IOU_REQUEST_TYPE}_tabsHeader`}
                            />
                        )}
                    >
                        <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
                            {() => (
                                <View
                                    id={CONST.TAB_REQUEST.MANUAL}
                                    style={{flex: 1}}
                                >
                                    <IOURequestStepAmount
                                        shouldKeepUserInput
                                        route={route}
                                    />
                                </View>
                            )}
                        </TopTab.Screen>
                        <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>
                            {() => (
                                <View
                                    id={CONST.TAB_REQUEST.SCAN}
                                    style={{flex: 1}}
                                >
                                    <IOURequestStepScan route={route} />
                                </View>
                            )}
                        </TopTab.Screen>
                        {shouldDisplayDistanceRequest && (
                            <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>
                                {() => (
                                    <View
                                        id={CONST.TAB_REQUEST.DISTANCE}
                                        style={{flex: 1}}
                                    >
                                        <IOURequestStepDistance route={route} />
                                    </View>
                                )}
                            </TopTab.Screen>
                        )}
                    </OnyxTabNavigator>
                ) : (
                    <IOURequestStepAmount route={route} />
                )}
            </View>
        </DragAndDropProvider>
    )}
</ScreenWrapper>

Update the below in src/components/ScreenWrapper.tsx:

Line 7: import type {OnyxEntry} from 'react-native-onyx';
Line 22: import type {SelectedTabRequest} from '@src/types/onyx';
Line 105:
/** The tab to select by default (whatever the user visited last) */
selectedTab?: OnyxEntry<SelectedTabRequest>;
Line 134: selectedTab,
Line 251: <FocusTrapForScreens selectedTab={selectedTab}>

Update the below in src/components/FocusTrap/FocusTrapForScreen/FocusTrapProps.ts:

import type { SelectedTabRequest } from "@src/types/onyx";
import type { OnyxEntry } from "react-native-onyx";

type FocusTrapForScreenProps = {
    children: React.ReactNode;
    selectedTab?: OnyxEntry<SelectedTabRequest>;
};

export default FocusTrapForScreenProps;

Update the below in src/components/FocusTrap/FocusTrapForScreen/index.web.tsx:

Line 12: function FocusTrapForScreen({children, selectedTab}: FocusTrapProps) {
Line 16: const [containerElements, setContainerElements] = useState<HTMLElement[]>([]);
Line 36: 
useEffect(() => {
    if (selectedTab) {
        const elements = [
            document.getElementById(`${CONST.TAB.IOU_REQUEST_TYPE}_header`),
            document.getElementById(`${CONST.TAB.IOU_REQUEST_TYPE}_tabsHeader`),
            document.getElementById(selectedTab),
        ];
        setContainerElements(() => elements.filter((el) => Boolean(el)));
    }
}, [selectedTab]);
Line 70: containerElements={isActive && containerElements?.length ? containerElements : undefined}

Update the below in src/components/TabSelector/TabSelector.tsx:

Line 17: id?: string;
Line 57: function TabSelector({state, navigation, onTabPress = () => {}, position, id}: TabSelectorProps) {
Line 87:
<View
    style={styles.tabSelector}
    id={id}
>

What alternative solutions did you explore? (Optional)

We can fix this issue also by not rendering the contents of other screens i.e. when the Manual screen is in view, the contents of the Split & Distance screens shouldn't be rendered.

Since the TAB navigation focus is already handled by focus-trap library, we can remove the below code block.

useFocusEffect(
    useCallback(() => {
        const handler = (event: KeyboardEvent) => {
            if (event.code !== CONST.KEYBOARD_SHORTCUTS.TAB.shortcutKey) {
                return;
            }
            event.preventDefault();
            event.stopPropagation();
        };
        KeyDownPressListener.addKeyDownPressListener(handler);

        return () => KeyDownPressListener.removeKeyDownPressListener(handler);
    }, []),
);

The fix for the issue is to only include the interactive elements of the focussed tab in the TAB navigation behavior order i.e. when the Manual tab is selected, the interactive elements of the Split & Distance tab should be removed from the TAB navigation tree by visually hiding them.

<OnyxTabNavigator
    id={CONST.TAB.IOU_REQUEST_TYPE}
    onTabSelected={resetIOUTypeIfChanged}
    tabBar={TabSelector}
>
    <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
        {(props) => (
            <View style={{visibility: props?.navigation?.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                <IOURequestStepAmount
                    shouldKeepUserInput
                    route={route}
                />
            </View>
        )}
    </TopTab.Screen>
    <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>
        {(props) => (
            <View style={{visibility: props?.navigation?.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                <IOURequestStepScan route={route} />
            </View>
        )}
    </TopTab.Screen>
    {shouldDisplayDistanceRequest && (
        <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>
            {(props) => (
                <View style={{visibility: props.navigation.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                    <IOURequestStepDistance route={route} />
                </View>
            )}
        </TopTab.Screen>
    )}
</OnyxTabNavigator>

With the above update, the contents of all the tabs are never re-rendered and only the visible elements are included in the TAB navigation tree.

Since the TAB navigation focus is already handled by focus-trap library, we can remove the below code block in src/pages/iou/request/IOURequestStartPage.tsx.

useFocusEffect(
    useCallback(() => {
        const handler = (event: KeyboardEvent) => {
            if (event.code !== CONST.KEYBOARD_SHORTCUTS.TAB.shortcutKey) {
                return;
            }
            event.preventDefault();
            event.stopPropagation();
        };
        KeyDownPressListener.addKeyDownPressListener(handler);

        return () => KeyDownPressListener.removeKeyDownPressListener(handler);
    }, []),
);

Since each screen is individually wrapped by FocusTrap by the component ScreenWrapper, we can remove the common ScreenWrapper for all the screens. By this approach, only the active tab's FocusTrap will be active at once. FOR PROPOSAL PURPOSES, ONLY A POC-LEVEL CODE IS ADDED BELOW. WILL ADD THE STANDARDIZED CODE WHEN THE SOLUTION IS ACCEPTED.

Update the below in src/pages/iou/request/IOURequestStartPage.tsx:

<AccessOrNotFoundWrapper
    reportID={reportID}
    iouType={iouType}
    policyID={policy?.id}
    accessVariants={[CONST.IOU.ACCESS_VARIANTS.CREATE]}
    allPolicies={allPolicies}
>
    <DragAndDropProvider
        setIsDraggingOver={setIsDraggingOver}
        isDisabled={selectedTab !== CONST.TAB_REQUEST.SCAN}
    >
        <View style={[styles.flex1]}>
            <View id={`${CONST.TAB.IOU_REQUEST_TYPE}_header`}>
                <HeaderWithBackButton
                    title={tabTitles[iouType]}
                    onBackButtonPress={navigateBack}
                />
            </View>
            {iouType !== CONST.IOU.TYPE.SEND && iouType !== CONST.IOU.TYPE.PAY && iouType !== CONST.IOU.TYPE.INVOICE ? (
                <OnyxTabNavigator
                    id={CONST.TAB.IOU_REQUEST_TYPE}
                    onTabSelected={resetIOUTypeIfChanged}
                    tabBar={(props) => (
                        <TabSelector
                            {...props}
                            id={`${CONST.TAB.IOU_REQUEST_TYPE}_tabsHeader`}
                        />
                    )}
                >
                    <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
                        {() => (
                            <View
                                id={CONST.TAB_REQUEST.MANUAL}
                                style={{flex: 1}}
                            >
                                <IOURequestStepAmount
                                    shouldKeepUserInput
                                    route={route}
                                />
                            </View>
                        )}
                    </TopTab.Screen>
                    <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>
                        {() => (
                            <View
                                id={CONST.TAB_REQUEST.SCAN}
                                style={{flex: 1}}
                            >
                                <IOURequestStepScan route={route} />
                            </View>
                        )}
                    </TopTab.Screen>
                    {shouldDisplayDistanceRequest && (
                        <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>
                            {() => (
                                <View
                                    id={CONST.TAB_REQUEST.DISTANCE}
                                    style={{flex: 1}}
                                >
                                    <IOURequestStepDistance route={route} />
                                </View>
                            )}
                        </TopTab.Screen>
                    )}
                </OnyxTabNavigator>
            ) : (
                <IOURequestStepAmount route={route} />
            )}
        </View>
    </DragAndDropProvider>
</AccessOrNotFoundWrapper>

Update the below in src/components/FocusTrap/FocusTrapForScreen/index.web.tsx:

Line no 10: import CONST from '@src/CONST';
Line no: 65:
containerElements={
    isActive && !!TOP_TAB_SCREENS.find((screen) => screen === route.name)
        ? [
                document.getElementById(`${CONST.TAB.IOU_REQUEST_TYPE}_header`),
                document.getElementById(`${CONST.TAB.IOU_REQUEST_TYPE}_tabsHeader`),
                document.getElementById(route.name),
            ]
        : undefined
}

Update the below in src/components/TabSelector/TabSelector.tsx:

Line no 17: id?: string;
Line no 57: function TabSelector({state, navigation, onTabPress = () => {}, position, id}: TabSelectorProps) {
Line no 87: 
<View
    style={styles.tabSelector}
    id={id}
>

Update the below in src/pages/iou/request/step/IOURequestStepAmount.tsx:

Line no 314: 
// shouldShowWrapper={!!backTo || isEditing}
shouldShowWrapper
hideHeader

Update the below in src/pages/iou/request/step/IOURequestStepDistance.tsx:

Line no 314: 
// shouldShowWrapper={!isCreatingNewRequest}
shouldShowWrapper
hideHeader

Update the below in src/pages/iou/request/step/IOURequestStepScan/index.tsx:

Line no 314: 
// shouldShowWrapper={!!backTo}
shouldShowWrapper
hideHeader

Update the below in src/pages/iou/request/step/StepScreenDragAndDropWrapper.tsx:

Line no 29: 
/** Hide the header when the wrapper is shown */
hideHeader?: boolean;
Line no 34:
function StepScreenDragAndDropWrapper({testID, hideHeader, headerTitle, onBackButtonPress, onEntryTransitionEnd, children, shouldShowWrapper}: StepScreenDragAndDropWrapperProps) {
Line no 55:
{!hideHeader ? (
    <HeaderWithBackButton
        title={headerTitle}
        onBackButtonPress={onBackButtonPress}
    />
) : null}

Update the below in src/pages/iou/request/step/StepScreenWrapper.tsx:

Line no 36: 
/** Hide the header when the wrapper is shown */
hideHeader?: boolean;
Line no 50: hideHeader,
Line no 68:
{!hideHeader ? (
    <HeaderWithBackButton
        title={headerTitle}
        onBackButtonPress={onBackButtonPress}
    />
) : null}

The id & other props are passed directly in the above changes for the purpose of creating a PoC. The additional ScreenWrapper in the IOURequestStartPage page is also removed with the above approach.

Jun 14 '24 13:06 vishnu-karuppusamy

📣 @vishnu-karuppusamy! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

Jun 14 '24 13:06 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01fded0233eaf38db6

Jun 14 '24 13:06 vishnu-karuppusamy

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

Jun 14 '24 13:06 melvin-bot[bot]

@rayane-djouah could you please provide an update here by EOD?

Jun 17 '24 18:06 OfstadC

I'm reviewing now

Jun 17 '24 18:06 rayane-d

I've locally reverted the changes from PR 39520 and confirmed that it is responsible for the bug. @vishnu-karuppusamy - Thank you for your proposal. Can you identify which specific change in PR 39520 caused this bug?

I also have a few questions:

When a click is made on the top empty area on the RHP (like the empty header area), the focus gets initiated on the common FocusTrap wrapper. Pressing SHIFT+TAB focuses the last element on the DOM tree which is the Next button of the Distance screen. (In the issue repro video, we can observe that the Next button of the Distance screen is focused)

Could you clarify the cause of the current incorrect TAB navigation behavior on these "request start" pages? When the TAB key is pressed on the Manual tab, the focus is stuck on the "Next" button, and it does not shift to the tab switching buttons, currency switcher, or the back button. Additionally, when clicking on another tab and then pressing the TAB key, the focus is stuck on that tab button (e.g., the Distance button).

Video

https://github.com/Expensify/App/assets/77965000/6f4b88b8-7afa-4373-b17b-8e2850aa7861

In contrast, the tab navigation works correctly in the assign task flow as seen here:

Video

https://github.com/Expensify/App/assets/77965000/c5c36b31-9641-4b50-9581-169ee70a5456

Because of the translating nature of the Tabs component OnyxTabNavigator, the tabs (Manual, Scan, Distance) are translated to the left by 750px when the Next button of the Distance screen is focused. This is the reason behind missing of the tabs.

Could you elaborate more on this translation behavior?

Although your proposal addresses the "Tabs disappear when changing from Scan to Distance via SHIFT+TAB" bug, I believe we should also correct the tab navigation functionality on these pages as part of this issue.

After applying your changes (wrapping the tab screens with ScreenWrapper), the focus is trapped on a single button of the tab screen, as shown here:

Video

https://github.com/Expensify/App/assets/77965000/faecd4ce-a343-4154-911a-b28cfe6c6ebd

Could you update your proposal to ensure that the tab navigation functions correctly on these screens? Thank you!

Jun 17 '24 18:06 rayane-d

@tgolen, @OfstadC, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Jun 17 '24 18:06 melvin-bot[bot]

Not overdue Melvin

Jun 17 '24 18:06 rayane-d

Proposal

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

tab key navigation is broken/disabled in create IOU request RHP

What is the root cause of that problem?

In IOURequestStartPage we disable tab key navigation by this code:

https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/pages/iou/request/IOURequestStartPage.tsx#L79-L92

After removing the code, when iouerqueststartpage rendered, all tab screen content are also rendered, which make tab key navigation jump to other tab screen.

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

We must remove the code above that preventing tab key navigation.

Then in here:

https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/pages/iou/request/IOURequestStartPage.tsx#L154-L163

We set the content of tab screen to be null if selectedTab is difference than the tab screen names.

The code could be:

<TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
        {() => (selectedTab === CONST.TAB_REQUEST.MANUAL) ? (
            <IOURequestStepAmount
                shouldKeepUserInput
                route={route}
            />
        ) : null}
    </TopTab.Screen>
    <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>{() => (selectedTab === CONST.TAB_REQUEST.SCAN) ? <IOURequestStepScan route={route} /> : null}</TopTab.Screen>
    {shouldDisplayDistanceRequest && <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>{() => selectedTab === CONST.TAB_REQUEST.DISTANCE ? <IOURequestStepDistance route={route} /> : null}</TopTab.Screen>}

To prevent re-rendering instead of returning null, we can use css styling to set the visibility of the content.

Jun 18 '24 03:06 tsa321

@tsa321 - Thank you for your proposal. Your proposal focuses on the disabled tab navigation but doesn't explain the cause of this bug. The code that you suggest to remove was added in this PR: https://github.com/Expensify/App/pull/35154 to fix this bug: https://github.com/Expensify/App/issues/36513. We need to ensure that we don't reintroduce the bug.

We set the content of the tab screen to be null if the selectedTab is different from the tab screen names.

I don't think that this solution is good for app performance because the tab screens' content will be rerendered on each tab switch.

When I tested your solution, the app crashed with a console error:

Screenshot

Jun 18 '24 22:06 rayane-d

@rayane-djouah I am sorry there must be an error when I beautify the code.

please try this code:

<TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
        {() => (selectedTab === CONST.TAB_REQUEST.MANUAL) ? (
            <IOURequestStepAmount
                shouldKeepUserInput
                route={route}
            />
        ) : null}
    </TopTab.Screen>
    <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>{() => (selectedTab === CONST.TAB_REQUEST.SCAN) ? <IOURequestStepScan route={route} /> : null}</TopTab.Screen>
    {shouldDisplayDistanceRequest && <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>{() => selectedTab === CONST.TAB_REQUEST.DISTANCE ? <IOURequestStepDistance route={route} /> : null}</TopTab.Screen>}

Your proposal focuses on the disabled tab navigation but doesn't explain the cause of this bug.

I have explained briefly in RCA:

After removing the code, when iouerqueststartpage rendered, all tab screen content are also rendered, which make tab key navigation jump to other tab screen. I should mention about tab bar buttons disappears too.

The code that you suggest to remove was added in this PR: https://github.com/Expensify/App/pull/35154 to fix this bug: https://github.com/Expensify/App/issues/36513. We need to ensure that we don't reintroduce the bug.

I think what you mean is: to fix https://github.com/Expensify/App/issues/26326 and not #36513 , right?

It is working fine now using my proposal:

https://github.com/Expensify/App/assets/114975543/de1f1644-bc29-4766-b47d-bf8a05545edf

Jun 18 '24 23:06 tsa321

@rayane-djouah thanks for the reply. Please find the below explanation for the questions raised.

Could you clarify the cause of the current incorrect TAB navigation behavior on these "request start" pages? When the TAB key is pressed on the Manual tab, the focus is stuck on the "Next" button, and it does not shift to the tab switching buttons, currency switcher, or the back button. Additionally, when clicking on another tab and then pressing the TAB key, the focus is stuck on that tab button (e.g., the Distance button).

In contrast, the tab navigation works correctly in the assign task flow as seen here:

Irrespective of the pages, the TAB navigation behavior remains the same i.e. pressing TAB will move focus to the next interactive element in the DOM tree and SHIFT+TAB will move focus to the previous interactive element. In assign task flow, all the interactive elements (back button, title & description input and next button) are in the same DOM tree. Hence the TAB navigation behaviour works as expected.

In the request start pages, all the tabs content (Manuel, Split & Distance) are rendered simultaneously in the same DOM tree. So the TAB navigation behavior follows the below order.

RHP's header back button
All the tabBars buttons (Manual, Split & Distance)
Interactive elements in the Manual tab such as currency selection dropdown, amount input field & Next button.
Interactive elements in the Split tab such as choose file button & so on.
Interactive elements in the Distance tab such as Start & Stop menu items, Mapbox & Next button.

Issue root cause explanation:

Clicking in the RHP header area activates the focus-trap within the RHP.
Pressing TAB focuses the first element in the above order i.e. RHP's header back button.
Pressing SHIFT+TAB moves focus to the previous interactive element in the above order i.e. from the first interactive element (RHP's header back button) to the last interactive element (Next button of the Distance tab).

In-order to have correct TAB navigation behavior, I've updated the initial proposal as follows.

Since the TAB navigation focus is already handled by focus-trap library, we can remove the below code block.

useFocusEffect(
    useCallback(() => {
        const handler = (event: KeyboardEvent) => {
            if (event.code !== CONST.KEYBOARD_SHORTCUTS.TAB.shortcutKey) {
                return;
            }
            event.preventDefault();
            event.stopPropagation();
        };
        KeyDownPressListener.addKeyDownPressListener(handler);

        return () => KeyDownPressListener.removeKeyDownPressListener(handler);
    }, []),
);

The fix for the issue is to only include the interactive elements of the focussed tab in the TAB navigation behavior order i.e. when the Manual tab is selected, the interactive elements of the Split & Distance tab should be removed from the TAB navigation tree by visually hiding them.

<OnyxTabNavigator
    id={CONST.TAB.IOU_REQUEST_TYPE}
    onTabSelected={resetIOUTypeIfChanged}
    tabBar={TabSelector}
>
    <TopTab.Screen name={CONST.TAB_REQUEST.MANUAL}>
        {(props) => (
            <View style={{visibility: props?.navigation?.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                <IOURequestStepAmount
                    shouldKeepUserInput
                    route={route}
                />
            </View>
        )}
    </TopTab.Screen>
    <TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>
        {(props) => (
            <View style={{visibility: props?.navigation?.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                <IOURequestStepScan route={route} />
            </View>
        )}
    </TopTab.Screen>
    {shouldDisplayDistanceRequest && (
        <TopTab.Screen name={CONST.TAB_REQUEST.DISTANCE}>
            {(props) => (
                <View style={{visibility: props.navigation.isFocused() ? 'visible' : 'hidden', flex: 1}}>
                    <IOURequestStepDistance route={route} />
                </View>
            )}
        </TopTab.Screen>
    )}
</OnyxTabNavigator>

With the above update, the contents of all the tabs are never re-rendered and only the visible elements are included in the TAB navigation tree.

Jun 19 '24 11:06 vishnu-karuppusamy

@vishnu-karuppusamy I think what you suggested above is similar to my proposal and it is really different with your original proposal.

Jun 19 '24 12:06 tsa321

@vishnu-karuppusamy, @tsa321, please follow the rule from CONTRIBUTING.md:

If you want to make an entirely new proposal or update an existing proposal, please go back and edit your original proposal, then post a new comment to the issue in this format to alert everyone that it has been updated:
## Proposal
[Updated](link to proposal)

And include brief details of what has been updated.

Jun 19 '24 12:06 rayane-d

@tsa321 - Setting the content of tab screen to null if selectedTab is different than the tab screen names cause the tab screen to re-render on every tab change causing performance issue:

Video

https://github.com/Expensify/App/assets/77965000/87671bb6-1e41-4517-8220-883ee8134c48

Jun 19 '24 12:06 rayane-d

I think what you mean is: to fix https://github.com/Expensify/App/issues/26326 and not https://github.com/Expensify/App/issues/36513 , right?

Correct. @vishnu-karuppusamy, @tsa321, I recommend you to read through https://github.com/Expensify/App/issues/26326 which is a similar bug.

Jun 19 '24 12:06 rayane-d

@vishnu-karuppusamy - The idea of setting the hidden style to tab screen content if it's not focused is working correctly, but I think that there is a better solution than that. I think that the a better solution is to use focus trap functionality to fix this issue.

@vishnu-karuppusamy - I dont't understand something in your proposal: IOURequestStepAmount, IOURequestStepScan, and IOURequestStepDistance all have a ScreenWrapper under the hood, I don't see why we need to wrap them with a ScreenWrapper.

Jun 19 '24 12:06 rayane-d

@vishnu-karuppusamy, @tsa321, upon investigating I found that we have already this condition to activate focus trap for currently shown tab only, but why it's not working?

https://github.com/Expensify/App/blob/362953d307fb97ff56e90cb5458f887b4b4132b1/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx#L24-L27

Jun 19 '24 12:06 rayane-d

@vishnu-karuppusamy, @tsa321, I'm looking for a proposal that: 1- Thoroughly explains the root cause of this bug. 2- Removes this code to ensure that the tab navigation functions correctly. 3- Use focus trap to prevent focusing on elements of tab screens that are not visible.

Note: This bug is also happening in NewChatSelectorPage.

Jun 19 '24 12:06 rayane-d