Navigator: remove location history, simplify internal logic (#64675)

* Remove location history and only use current location * Deprecate `options.replace` * Make sure that two screens with the same path cannot be added * Re-implement focus restoration relying on a map with paths as keys * Re-implement the isInitial flag by relying on the initialPath * Update README * Comment map deletion out * CHANGELOG * Fix broken focus restoration by updating focusSelectors in the navigator reducer * create a copy of focusSelectors map only when necessary * Set isInitial inside the reducer, instead of on the context value --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
WordPress · Aug 23, 2024 · 1e3efbb · 1e3efbb
1 parent 903f2f4
commit 1e3efbb
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 87 deletions.
diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Deprecations
+
+-   Deprecate `replace` from the options accepted by `useNavigator().goTo()` ([#64675](https://github.com/WordPress/gutenberg/pull/64675)).
+
 ### Enhancements
 
 -   `ColorPicker`: Adopt radius scale ([#64693](https://github.com/WordPress/gutenberg/pull/64693)).
@@ -16,6 +20,7 @@
 -   `TabPanel`: Remove radius applied to panel focus style ([#64693](https://github.com/WordPress/gutenberg/pull/64693)).
 -   `Tabs`: Remove radius applied to panel focus style ([#64693](https://github.com/WordPress/gutenberg/pull/64693)).
 -   `UnitControl`: Update unit select styles ([#64712](https://github.com/WordPress/gutenberg/pull/64712)).
+-   `Navigator`: remove location history, simplify internal logic ([#64675](https://github.com/WordPress/gutenberg/pull/64675)).
 -   Decrease horizontal padding from 16px to 12px on the following components, when in the 40px default size ([#64708](https://github.com/WordPress/gutenberg/pull/64708)).
     -   `AnglePickerControl`
     -   `ColorPicker` (on the inputs)

diff --git a/packages/components/src/navigator/navigator-provider/README.md b/packages/components/src/navigator/navigator-provider/README.md
@@ -72,7 +72,6 @@ The available options are:
 -   `focusTargetSelector`: `string`. An optional property used to specify the CSS selector used to restore focus on the matching element when navigating back;
 -   `isBack`: `boolean`. An optional property used to specify whether the navigation should be considered as backwards (thus enabling focus restoration when possible, and causing the animation to be backwards too);
 -   `skipFocus`: `boolean`. An optional property used to opt out of `Navigator`'s focus management, useful when the consumer of the component wants to manage focus themselves;
--   `replace`: `boolean`. An optional property used to cause the new location to replace the current location in the stack.
 
 ### `goBack`: `( path: string, options: NavigateOptions ) => void`
 
@@ -87,8 +86,8 @@ The available options are the same as for the `goTo` method, except for the `isB
 The `location` object represent the current location, and has a few properties:
 
 -   `path`: `string`. The path associated to the location.
--   `isBack`: `boolean`. A flag that is `true` when the current location was reached by navigating backwards in the location history.
--   `isInitial`: `boolean`. A flag that is `true` only for the first (root) location in the location history.
+-   `isBack`: `boolean`. A flag that is `true` when the current location was reached by navigating backwards.
+-   `isInitial`: `boolean`. A flag that is `true` only for the initial location.
 
 ### `params`: `Record< string, string | string[] >`
 

diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx
@@ -37,110 +37,97 @@ type RouterAction =
 	| { type: 'gotoparent'; options?: NavigateToParentOptions };
 
 type RouterState = {
+	initialPath: string;
 	screens: Screen[];
-	locationHistory: NavigatorLocation[];
+	currentLocation: NavigatorLocation;
 	matchedPath: MatchedPath;
+	focusSelectors: Map< string, string >;
 };
 
-const MAX_HISTORY_LENGTH = 50;
-
 function addScreen( { screens }: RouterState, screen: Screen ) {
+	if ( screens.some( ( s ) => s.path === screen.path ) ) {
+		// eslint-disable-next-line no-console
+		console.warn(
+			`Navigator: a screen with path ${ screen.path } already exists.
+The screen with id ${ screen.id } will not be added.`
+		);
+		return screens;
+	}
 	return [ ...screens, screen ];
 }
 
 function removeScreen( { screens }: RouterState, screen: Screen ) {
 	return screens.filter( ( s ) => s.id !== screen.id );
 }
 
-function goBack( { locationHistory }: RouterState ) {
-	if ( locationHistory.length <= 1 ) {
-		return locationHistory;
-	}
-	return [
-		...locationHistory.slice( 0, -2 ),
-		{
-			...locationHistory[ locationHistory.length - 2 ],
-			isBack: true,
-			hasRestoredFocus: false,
-		},
-	];
-}
-
 function goTo(
 	state: RouterState,
 	path: string,
 	options: NavigateOptions = {}
 ) {
-	const { locationHistory } = state;
+	const { currentLocation, focusSelectors } = state;
+
 	const {
-		focusTargetSelector,
+		// Default assignments
 		isBack = false,
 		skipFocus = false,
-		replace = false,
+		// Extract to avoid forwarding
+		replace,
+		focusTargetSelector,
+		// Rest
 		...restOptions
 	} = options;
 
-	const isNavigatingToSamePath =
-		locationHistory.length > 0 &&
-		locationHistory[ locationHistory.length - 1 ].path === path;
-	if ( isNavigatingToSamePath ) {
-		return locationHistory;
-	}
-
-	const isNavigatingToPreviousPath =
-		isBack &&
-		locationHistory.length > 1 &&
-		locationHistory[ locationHistory.length - 2 ].path === path;
-
-	if ( isNavigatingToPreviousPath ) {
-		return goBack( state );
+	if ( currentLocation?.path === path ) {
+		return { currentLocation, focusSelectors };
 	}
 
-	const newLocation = {
-		...restOptions,
-		path,
-		isBack,
-		hasRestoredFocus: false,
-		skipFocus,
-	};
+	let focusSelectorsCopy;
 
-	if ( locationHistory.length === 0 ) {
-		return replace ? [] : [ newLocation ];
+	// Set a focus selector that will be used when navigating
+	// back to the current location.
+	if ( focusTargetSelector && currentLocation?.path ) {
+		if ( ! focusSelectorsCopy ) {
+			focusSelectorsCopy = new Map( state.focusSelectors );
+		}
+		focusSelectorsCopy.set( currentLocation.path, focusTargetSelector );
 	}
 
-	const newLocationHistory = locationHistory.slice(
-		locationHistory.length > MAX_HISTORY_LENGTH - 1 ? 1 : 0,
-		-1
-	);
-
-	if ( ! replace ) {
-		newLocationHistory.push(
-			// Assign `focusTargetSelector` to the previous location in history
-			// (the one we just navigated from).
-			{
-				...locationHistory[ locationHistory.length - 1 ],
-				focusTargetSelector,
-			}
-		);
+	// Get the focus selector for the new location.
+	let currentFocusSelector;
+	if ( isBack ) {
+		if ( ! focusSelectorsCopy ) {
+			focusSelectorsCopy = new Map( state.focusSelectors );
+		}
+		currentFocusSelector = focusSelectorsCopy.get( path );
+		focusSelectorsCopy.delete( path );
 	}
 
-	newLocationHistory.push( newLocation );
-
-	return newLocationHistory;
+	return {
+		currentLocation: {
+			...restOptions,
+			path,
+			isBack,
+			hasRestoredFocus: false,
+			focusTargetSelector: currentFocusSelector,
+			skipFocus,
+		},
+		focusSelectors: focusSelectorsCopy ?? focusSelectors,
+	};
 }
 
 function goToParent(
 	state: RouterState,
 	options: NavigateToParentOptions = {}
 ) {
-	const { locationHistory, screens } = state;
-	const currentPath = locationHistory[ locationHistory.length - 1 ].path;
+	const { currentLocation, screens, focusSelectors } = state;
+	const currentPath = currentLocation?.path;
 	if ( currentPath === undefined ) {
-		return locationHistory;
+		return { currentLocation, focusSelectors };
 	}
 	const parentPath = findParent( currentPath, screens );
 	if ( parentPath === undefined ) {
-		return locationHistory;
+		return { currentLocation, focusSelectors };
 	}
 	return goTo( state, parentPath, {
 		...options,
@@ -152,7 +139,13 @@ function routerReducer(
 	state: RouterState,
 	action: RouterAction
 ): RouterState {
-	let { screens, locationHistory, matchedPath } = state;
+	let {
+		screens,
+		currentLocation,
+		matchedPath,
+		focusSelectors,
+		...restState
+	} = state;
 	switch ( action.type ) {
 		case 'add':
 			screens = addScreen( state, action.screen );
@@ -161,26 +154,31 @@ function routerReducer(
 			screens = removeScreen( state, action.screen );
 			break;
 		case 'goto':
-			locationHistory = goTo( state, action.path, action.options );
+			const goToNewState = goTo( state, action.path, action.options );
+			currentLocation = goToNewState.currentLocation;
+			focusSelectors = goToNewState.focusSelectors;
 			break;
 		case 'gotoparent':
-			locationHistory = goToParent( state, action.options );
+			const goToParentNewState = goToParent( state, action.options );
+			currentLocation = goToParentNewState.currentLocation;
+			focusSelectors = goToParentNewState.focusSelectors;
 			break;
 	}
 
+	if ( currentLocation?.path === state.initialPath ) {
+		currentLocation = { ...currentLocation, isInitial: true };
+	}
+
 	// Return early in case there is no change
 	if (
 		screens === state.screens &&
-		locationHistory === state.locationHistory
+		currentLocation === state.currentLocation
 	) {
 		return state;
 	}
 
 	// Compute the matchedPath
-	const currentPath =
-		locationHistory.length > 0
-			? locationHistory[ locationHistory.length - 1 ].path
-			: undefined;
+	const currentPath = currentLocation?.path;
 	matchedPath =
 		currentPath !== undefined
 			? patternMatch( currentPath, screens )
@@ -197,23 +195,35 @@ function routerReducer(
 		matchedPath = state.matchedPath;
 	}
 
-	return { screens, locationHistory, matchedPath };
+	return {
+		...restState,
+		screens,
+		currentLocation,
+		matchedPath,
+		focusSelectors,
+	};
 }
 
 function UnconnectedNavigatorProvider(
 	props: WordPressComponentProps< NavigatorProviderProps, 'div' >,
 	forwardedRef: ForwardedRef< any >
 ) {
-	const { initialPath, children, className, ...otherProps } =
-		useContextSystem( props, 'NavigatorProvider' );
+	const {
+		initialPath: initialPathProp,
+		children,
+		className,
+		...otherProps
+	} = useContextSystem( props, 'NavigatorProvider' );
 
 	const [ routerState, dispatch ] = useReducer(
 		routerReducer,
-		initialPath,
+		initialPathProp,
 		( path ) => ( {
 			screens: [],
-			locationHistory: [ { path } ],
+			currentLocation: { path },
 			matchedPath: undefined,
+			focusSelectors: new Map(),
+			initialPath: initialPathProp,
 		} )
 	);
 
@@ -242,19 +252,16 @@ function UnconnectedNavigatorProvider(
 		[]
 	);
 
-	const { locationHistory, matchedPath } = routerState;
+	const { currentLocation, matchedPath } = routerState;
 
 	const navigatorContextValue: NavigatorContextType = useMemo(
 		() => ( {
-			location: {
-				...locationHistory[ locationHistory.length - 1 ],
-				isInitial: locationHistory.length === 1,
-			},
+			location: currentLocation,
 			params: matchedPath?.params ?? {},
 			match: matchedPath?.id,
 			...methods,
 		} ),
-		[ locationHistory, matchedPath, methods ]
+		[ currentLocation, matchedPath, methods ]
 	);
 
 	const cx = useCx();

diff --git a/packages/components/src/navigator/types.ts b/packages/components/src/navigator/types.ts
@@ -28,7 +28,9 @@ export type NavigateOptions = {
 	 */
 	skipFocus?: boolean;
 	/**
-	 * Whether the navigation should replace the current location in the stack.
+	 * Note: this option is deprecated and currently doesn't have any effect.
+	 * @deprecated
+	 * @ignore
 	 */
 	replace?: boolean;
 };