-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for Payment 2024-08-22] [$250] Submit expense - Tabs disappear when changing from Scan to Distance via SHIFT+TAB #43719
Comments
Triggered auto assignment to @tgolen ( |
👋 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:
|
@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 |
We think that this bug might be related to #vip-vsp |
Job added to Upwork: https://www.upwork.com/jobs/~0112bb5e5d8d723e7f |
Triggered auto assignment to @OfstadC ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah ( |
I'm demoting this one and assigning to external. |
cannot reproduce in production release. Offending PR: #39520 |
Not produceable on Production and local |
ProposalPlease 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?
{* 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>
What changes do you think we should make in order to solve the problem?
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. Update the below in <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 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 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 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 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)
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.
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);
}, []),
);
Update the below in <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 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 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 Line no 314:
// shouldShowWrapper={!!backTo || isEditing}
shouldShowWrapper
hideHeader Update the below in Line no 314:
// shouldShowWrapper={!isCreatingNewRequest}
shouldShowWrapper
hideHeader Update the below in Line no 314:
// shouldShowWrapper={!!backTo}
shouldShowWrapper
hideHeader Update the below in 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 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 |
📣 @vishnu-karuppusamy! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
Proposal updated. |
As I mentioned here: #43719 (comment) This solution will cause a performance issue (re-render delays when tab switching, especially for the map in the distance tab) |
@tgolen, Summary: @dominictb's proposal looks good to me. 🎀👀🎀 C+ reviewed |
Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
I would like to re-assist the solutions proposed as both the solutions proposed such as hiding the in-active tabs and the use of @rayane-djouah could you please confirm the above statement? cc: @OfstadC |
I think we should try to have tab navigation anywhere we can, so if it's
possible to re-enable tab navigation (it sounds like it), then I think we
should do it. I'll wait for ^ question to be resolved before doing anything
here though.
…On Sun, Jul 21, 2024 at 2:34 AM vishnu-karuppusamy ***@***.***> wrote:
I would like to re-assist the solutions proposed as both the solutions
proposed such as hiding the in-active tabs and the use of
containerElements prop in the focus-trap-react were proposed initially by
me.
@rayane-djouah <https://github.com/rayane-djouah> could you please
confirm the above statement?
cc: @OfstadC <https://github.com/OfstadC>
—
Reply to this email directly, view it on GitHub
<#43719 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMABZ5WK76ZMN3YCTKTDLZNNXAJAVCNFSM6AAAAABJI5OZFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGUZDONRSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@vishnu-karuppusamy - Thank you for your initial suggestion regarding the use of the I tried implementing the solution you suggested, as seen in the test branch, but unfortunately, it did not resolve the bug. In striving for the clearest and highest-quality proposal, with a fully working solution, I've decided to proceed with @dominictb's proposal that meets these criteria more fully. Thank you again for your efforts and understanding. |
@tgolen - Thank you for confirming the way to move forward! I agree that enabling tab navigation wherever possible will improve usability. Let's go ahead with @dominictb's solution. |
📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Any update here @dominictb @rayane-djouah ? 😃 |
PR under review |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
This ^ is a false positive #47381 (comment) |
Note The production deploy automation failed: This should be on [HOLD for Payment 2024-08-22] according to #47356 prod deploy checklist, confirmed in #45982 (comment). cc @OfstadC |
Thanks @rayane-djouah - since the automation failed. I'll posted the checklist below 😄
|
Checklist
Regression Test Proposal
Do we agree 👍 or 👎 cc @OfstadC |
Payment Summary:
|
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:
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?
Screenshots/Videos
Add any screenshot/video evidence
Bug6512234_1718299693563.20240614_012437.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @rayane-djouahThe text was updated successfully, but these errors were encountered: