diff --git a/src/libs/Navigation/Navigation.ts b/src/libs/Navigation/Navigation.ts index 8cd9ac3da7b7..ad356bac40ab 100644 --- a/src/libs/Navigation/Navigation.ts +++ b/src/libs/Navigation/Navigation.ts @@ -6,7 +6,7 @@ import clearSelectedTextIfComposerBlurred from '@libs/clearSelectedTextIfCompose import getIsNarrowLayout from '@libs/getIsNarrowLayout'; import {setupHadTabNavigation} from '@libs/hadTabNavigation'; import Log from '@libs/Log'; -import {skipNextFocusRestore} from '@libs/NavigationFocusReturn'; +import {cancelSkipNextFocusRestore, skipNextFocusRestore} from '@libs/NavigationFocusReturn'; import {shallowCompare} from '@libs/ObjectUtils'; import {getSpan, startSpan} from '@libs/telemetry/activeSpans'; @@ -528,18 +528,36 @@ function goUp(backToRoute: Route, options?: GoBackOptions): boolean { return false; } - // Arms the one-shot inline with each dispatch — no window between "set flag" and dispatch for an early-return to leak it. - const dispatch = (actionToDispatch: NavigationAction) => { + // Arms the focus-restore skip before dispatch (PUSH_PARAMS consumes it inside the router) and disarms it if the + // action was dropped, so a cancelled prompt can't leak it. Returns false when the action was dropped: prevented by a + // `beforeRemove` guard or a no-op, both reported as a `noop` `__unsafe_action__`. Pinned by the tests in `GoBackTests.tsx`. + const dispatch = (actionToDispatch: NavigationAction): boolean => { if (options?.shouldSkipFocusRestore) { skipNextFocusRestore(); } - navigationRef.current?.dispatch(actionToDispatch); + let wasApplied = true; + const unsubscribe = navigationContainer.addListener('__unsafe_action__', (event) => { + if (event.data.action !== actionToDispatch) { + return; + } + wasApplied = !event.data.noop; + }); + navigationContainer.dispatch(actionToDispatch); + unsubscribe(); + if (!wasApplied && options?.shouldSkipFocusRestore) { + cancelSkipNextFocusRestore(); + } + return wasApplied; }; // Once these pops are out, going back has happened, so nothing below may report a failure to go up. const popsToNavigator = getPopsToNavigatorWithBackToRoute(navigationContainer.getRootState(), action, compareParams); for (const popToNavigator of popsToNavigator) { - dispatch(popToNavigator); + // A prevented pop hands control to its guard: going on would change screens under its prompt or re-send the pop, + // which closes the prompt. On confirm the guard replays only this pop, which usually is the whole way back. + if (!dispatch(popToNavigator)) { + return true; + } } const didPopToNavigator = popsToNavigator.length > 0; @@ -563,7 +581,10 @@ function goUp(backToRoute: Route, options?: GoBackOptions): boolean { ); const jumpParams = 'params' in payload ? payload.params : undefined; if (underlyingTabNavIndex !== -1) { - dispatch(StackActions.pop(topRootIndex - underlyingTabNavIndex)); + // A prevented pop leaves the tab navigator covered, so jumping in it would change the tab under the prompt. + if (!dispatch(StackActions.pop(topRootIndex - underlyingTabNavIndex))) { + return true; + } // The uncovered tab navigator has the right tab active, but not necessarily the requested screen inside // it, so the jump still has to be applied there rather than to the tab navigator that was popped. const underlyingTabStateKey = rootState.routes.at(underlyingTabNavIndex)?.state?.key; diff --git a/src/libs/NavigationFocusReturn/index.native.ts b/src/libs/NavigationFocusReturn/index.native.ts index 6dce299bef0b..7db71f9dd4e5 100644 --- a/src/libs/NavigationFocusReturn/index.native.ts +++ b/src/libs/NavigationFocusReturn/index.native.ts @@ -49,6 +49,11 @@ function skipNextFocusRestore(): void { skipNextRestore = true; } +/** Undo `skipNextFocusRestore` when its goBack never happened. */ +function cancelSkipNextFocusRestore(): void { + skipNextRestore = false; +} + function registerPressable(routeKey: string, identifier: string, ref: RefObject | null>): () => void { let routeMap = pressableRegistry.get(routeKey); if (!routeMap) { @@ -352,6 +357,7 @@ export { notifyPushParamsBackward, cancelPendingFocusRestore, skipNextFocusRestore, + cancelSkipNextFocusRestore, isFocusRestoreInProgress, shouldSkipAutoFocusDueToExistingFocus, resetForTests, diff --git a/src/libs/NavigationFocusReturn/index.ts b/src/libs/NavigationFocusReturn/index.ts index 8f14ba35cd80..ba7b9046d8dc 100644 --- a/src/libs/NavigationFocusReturn/index.ts +++ b/src/libs/NavigationFocusReturn/index.ts @@ -169,6 +169,11 @@ function skipNextFocusRestore(): void { skipNextRestore = true; } +/** Undo `skipNextFocusRestore` when its goBack never happened. */ +function cancelSkipNextFocusRestore(): void { + skipNextRestore = false; +} + /** Native-only. Web captures via `focusin` so this stub exists only to keep the import cross-platform. */ // eslint-disable-next-line @typescript-eslint/no-unused-vars function notifyPressedTrigger(_ref: RefObject | null> | null, _identifier?: string): void {} @@ -690,6 +695,7 @@ export { notifyPushParamsBackward, cancelPendingFocusRestore, skipNextFocusRestore, + cancelSkipNextFocusRestore, notifyPressedTrigger, registerPressable, isFocusRestoreInProgress, diff --git a/tests/navigation/GoBackTests.tsx b/tests/navigation/GoBackTests.tsx index d77ac2ddd264..553d2895a8c3 100644 --- a/tests/navigation/GoBackTests.tsx +++ b/tests/navigation/GoBackTests.tsx @@ -6,13 +6,14 @@ import getIsNarrowLayout from '@libs/getIsNarrowLayout'; import {getPreservedNavigatorState} from '@libs/Navigation/AppNavigator/createSplitNavigator/usePreserveNavigatorState'; import Navigation from '@libs/Navigation/Navigation'; import navigationRef from '@libs/Navigation/navigationRef'; +import * as NavigationFocusReturn from '@libs/NavigationFocusReturn'; import CONST from '@src/CONST'; import NAVIGATORS from '@src/NAVIGATORS'; import ROUTES from '@src/ROUTES'; import SCREENS from '@src/SCREENS'; -import type {InitialState} from '@react-navigation/native'; +import type {EventArg, InitialState, NavigationAction} from '@react-navigation/native'; import React, {createContext, useContext} from 'react'; @@ -760,6 +761,22 @@ describe('Go back on the narrow layout', () => { const policyA = 'policy-a'; const policyB = 'policy-b'; + type BeforeRemoveEvent = EventArg<'beforeRemove', true, {action: NavigationAction}>; + + function preventRemove(event: BeforeRemoveEvent) { + event.preventDefault(); + } + + /** Stands in for a guard on the routes named `routeName`, and lets every other removal through */ + function guardOn(routeName: string, guard: (event: BeforeRemoveEvent) => void) { + return (event: BeforeRemoveEvent, route: {name: string}) => { + if (route.name !== routeName) { + return; + } + guard(event); + }; + } + function buildStateWithModalOverWorkspaces(...workspaceSplits: WorkspaceScopeRoute[]): InitialState { return buildStateWithModalOverTab(WORKSPACES_TAB_INDEX, ...workspaceSplits); } @@ -801,6 +818,155 @@ describe('Go back on the narrow layout', () => { expect(dispatchSpy).not.toHaveBeenCalledWith(expect.objectContaining({type: CONST.NAVIGATION.ACTION_TYPE.PUSH})); }); + it('Should stop at a pop prevented by a beforeRemove listener', () => { + // Given a modal over two workspace splits, whose closing a discard-changes guard prevents + const guard = jest.fn(preventRemove); + render( + , + ); + const dispatchSpy = jest.spyOn(requireNavigationContainer(), 'dispatch'); + + // When going back to a screen that needs the modal closed and the splits under it popped too + act(() => { + Navigation.goBack(ROUTES.WORKSPACE_OVERVIEW.getRoute(policyA)); + }); + + // Then only the prevented pop goes out and the guard sees it once - issue #102010: re-dispatching it reached + // the guard over its own open prompt, which closed that prompt, and popping the splits would change the + // screens under the form + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(guard).toHaveBeenCalledTimes(1); + const rootState = navigationRef.current?.getRootState(); + expect(rootState?.routes.map((route) => route.name)).toEqual([NAVIGATORS.TAB_NAVIGATOR, NAVIGATORS.RIGHT_MODAL_NAVIGATOR]); + const workspaceState = rootState?.routes.at(0)?.state?.routes.find((route) => route.name === NAVIGATORS.WORKSPACE_NAVIGATOR)?.state; + expect(workspaceState?.routes).toHaveLength(2); + }); + + it('Should disarm the focus-restore skip when the pop it was armed for is prevented', () => { + // Given a modal over a workspace split, whose closing a discard-changes guard prevents + render( + , + ); + const skipSpy = jest.spyOn(NavigationFocusReturn, 'skipNextFocusRestore'); + const cancelSkipSpy = jest.spyOn(NavigationFocusReturn, 'cancelSkipNextFocusRestore'); + + // When a form-submit goBack arms the skip for a pop the guard then prevents + act(() => { + Navigation.goBack(ROUTES.WORKSPACE_OVERVIEW.getRoute(policyA), {shouldSkipFocusRestore: true}); + }); + + // Then the skip is disarmed, so a cancelled prompt can't leave it to eat the next Back's focus restore + expect(skipSpy).toHaveBeenCalledTimes(1); + expect(cancelSkipSpy).toHaveBeenCalledTimes(1); + }); + + it('Should stop at a prevented pop after an earlier pop went through', () => { + // Given a modal over two workspace splits, where closing the modal goes through but a guard on the covering + // split prevents popping it + const guard = jest.fn(preventRemove); + render( + , + ); + const dispatchSpy = jest.spyOn(requireNavigationContainer(), 'dispatch'); + + // When going back to a screen in the covered split + act(() => { + Navigation.goBack(ROUTES.WORKSPACE_OVERVIEW.getRoute(policyA)); + }); + + // Then nothing is dispatched past the prevented pop, and the splits it would have popped stay as they were + expect(dispatchSpy).toHaveBeenCalledTimes(2); + expect(guard).toHaveBeenCalledTimes(1); + const rootState = navigationRef.current?.getRootState(); + expect(rootState?.routes.map((route) => route.name)).toEqual([NAVIGATORS.TAB_NAVIGATOR]); + const workspaceState = rootState?.routes.at(0)?.state?.routes.find((route) => route.name === NAVIGATORS.WORKSPACE_NAVIGATOR)?.state; + expect(workspaceState?.routes).toHaveLength(2); + }); + + it('Should take only the prevented pop once the guard lets the user leave', () => { + // Given a modal over two workspace splits, whose closing a guard prevents until the user confirms leaving + let hasConfirmed = false; + const guard = jest.fn((event: BeforeRemoveEvent) => { + if (hasConfirmed) { + return; + } + event.preventDefault(); + }); + render( + , + ); + act(() => { + Navigation.goBack(ROUTES.WORKSPACE_OVERVIEW.getRoute(policyA)); + }); + const blockedAction = guard.mock.calls.at(0)?.at(0)?.data.action; + expect(blockedAction).toBeDefined(); + + // When the user confirms, and the guard replays the pop it blocked, as useDiscardChangesConfirmation does + hasConfirmed = true; + act(() => { + if (!blockedAction) { + return; + } + navigationRef.current?.dispatch(blockedAction); + }); + + // Then the modal closes onto split B: the pops to split A that goUp stopped before are not taken. This pins + // the known limit, so a change that resumes goUp after the replay shows up here + const rootState = navigationRef.current?.getRootState(); + expect(rootState?.routes.map((route) => route.name)).toEqual([NAVIGATORS.TAB_NAVIGATOR]); + const workspaceState = rootState?.routes.at(0)?.state?.routes.find((route) => route.name === NAVIGATORS.WORKSPACE_NAVIGATOR)?.state; + expect(workspaceState?.routes).toHaveLength(2); + const activeSplit = workspaceState?.routes.at(workspaceState.index ?? 0); + expect(activeSplit?.state?.routes.at(0)?.params).toMatchObject({policyID: policyB}); + }); + + it('Should not jump tabs when a guard prevents uncovering the underlying tab navigator', () => { + // Given a modal over a pushed tab navigator, over one that already has the target tab active, where a guard + // on the pushed tab navigator prevents popping it + const initialState: InitialState = { + index: 2, + routes: [ + ...buildWorkspaceNavigationState(buildWorkspaceSplitRoute(policyA)).routes, + ...buildWorkspaceNavigationStateWithActiveTab(1, buildWorkspaceSplitRoute(policyB)).routes, + {name: NAVIGATORS.RIGHT_MODAL_NAVIGATOR, state: {index: 0, routes: [{name: SCREENS.RIGHT_MODAL.SETTINGS}]}}, + ], + }; + const guard = jest.fn(preventRemove); + render( + , + ); + const pushedTabStateBefore = navigationRef.current?.getRootState().routes.at(1)?.state; + const dispatchSpy = jest.spyOn(requireNavigationContainer(), 'dispatch'); + + // When going back to a screen in the underlying tab navigator + act(() => { + Navigation.goBack(ROUTES.WORKSPACE_OVERVIEW.getRoute(policyA)); + }); + + // Then the modal closes, but nothing goes out past the prevented pop: jumping tabs in the underlying tab + // navigator would change the screens under the guard's prompt + expect(guard).toHaveBeenCalledTimes(1); + expect(dispatchSpy).not.toHaveBeenCalledWith(expect.objectContaining({type: 'JUMP_TO'})); + const rootState = navigationRef.current?.getRootState(); + expect(rootState?.routes.map((route) => route.name)).toEqual([NAVIGATORS.TAB_NAVIGATOR, NAVIGATORS.TAB_NAVIGATOR]); + expect(rootState?.routes.at(1)?.state?.index).toBe(pushedTabStateBefore?.index); + }); + it('Should restore the tab the fallback route belongs to when the tab navigator is focused elsewhere', () => { // Issue #89006: the root stack's state slicing leaves the tab navigator's index on Home, so popping the // modal alone lands there instead of the tab holding the back target. The walk down plus jumpTo fixes it. diff --git a/tests/utils/TestNavigationContainer.tsx b/tests/utils/TestNavigationContainer.tsx index aad0452981b2..ec4183408b09 100644 --- a/tests/utils/TestNavigationContainer.tsx +++ b/tests/utils/TestNavigationContainer.tsx @@ -20,11 +20,11 @@ import CONST from '@src/CONST'; import NAVIGATORS from '@src/NAVIGATORS'; import SCREENS from '@src/SCREENS'; -import type {InitialState, NavigatorScreenParams, RouteProp} from '@react-navigation/native'; +import type {EventArg, InitialState, NavigationAction, NavigatorScreenParams, ParamListBase, RouteProp} from '@react-navigation/native'; import {createBottomTabNavigator} from '@react-navigation/bottom-tabs'; -import {NavigationContainer} from '@react-navigation/native'; -import React from 'react'; +import {NavigationContainer, useNavigation, useRoute} from '@react-navigation/native'; +import React, {createContext, useContext, useEffect} from 'react'; /** Test-specific param list with split navigators at root level for simplified test setup */ type TestRootParamList = AuthScreensParamList & { @@ -48,11 +48,36 @@ const RightModalNavigatorStack = createSplitNavigator jest.fn(); -type TestNavigationContainerProps = {initialState: InitialState}; +type BeforeRemoveEvent = EventArg<'beforeRemove', true, {action: NavigationAction}>; + +type BeforeRemoveListener = (event: BeforeRemoveEvent, route: RouteProp) => void; + +type TestNavigationContainerProps = { + initialState: InitialState; + + /** Called on `beforeRemove` of every tab navigator, the right modal and every workspace split, so a test can stand in for a guard */ + onBeforeRemove?: BeforeRemoveListener; +}; + +const BeforeRemoveListenerContext = createContext(undefined); + +function useTestBeforeRemoveListener() { + const navigation = useNavigation(); + const route = useRoute(); + const onBeforeRemove = useContext(BeforeRemoveListenerContext); + + useEffect(() => { + if (!onBeforeRemove) { + return undefined; + } + return navigation.addListener('beforeRemove', (event) => onBeforeRemove(event, route)); + }, [navigation, route, onBeforeRemove]); +} // `usePreserveNavigatorState` keys off `parentRoute.key`, so the workspace and domain splits get their real route: // a shared placeholder key would make sibling splits of different scopes share one preserved state. function TestWorkspaceSplitNavigator({route}: {route: RouteProp}) { + useTestBeforeRemoveListener(); return ( - - - - - - + + + + + + + + + ); } export default TestNavigationContainer;