From f5476f7e3322aa534a6ed60cd8cdf33eba3cf903 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 10:19:38 +0200 Subject: [PATCH 01/11] Remove location history and only use current location --- .../navigator-provider/component.tsx | 102 +++++------------- 1 file changed, 25 insertions(+), 77 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 78bc674c06fbd..fbb34776a9ce6 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -38,12 +38,10 @@ type RouterAction = type RouterState = { screens: Screen[]; - locationHistory: NavigatorLocation[]; + currentLocation?: NavigatorLocation; matchedPath: MatchedPath; }; -const MAX_HISTORY_LENGTH = 50; - function addScreen( { screens }: RouterState, screen: Screen ) { return [ ...screens, screen ]; } @@ -52,95 +50,48 @@ 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 } = 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; } - const newLocation = { + return { ...restOptions, path, isBack, hasRestoredFocus: false, skipFocus, }; - - if ( locationHistory.length === 0 ) { - return replace ? [] : [ newLocation ]; - } - - 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, - } - ); - } - - newLocationHistory.push( newLocation ); - - return newLocationHistory; } function goToParent( state: RouterState, options: NavigateToParentOptions = {} ) { - const { locationHistory, screens } = state; - const currentPath = locationHistory[ locationHistory.length - 1 ].path; + const { currentLocation, screens } = state; + const currentPath = currentLocation?.path; if ( currentPath === undefined ) { - return locationHistory; + return currentLocation; } const parentPath = findParent( currentPath, screens ); if ( parentPath === undefined ) { - return locationHistory; + return currentLocation; } return goTo( state, parentPath, { ...options, @@ -152,7 +103,7 @@ function routerReducer( state: RouterState, action: RouterAction ): RouterState { - let { screens, locationHistory, matchedPath } = state; + let { screens, currentLocation, matchedPath } = state; switch ( action.type ) { case 'add': screens = addScreen( state, action.screen ); @@ -161,26 +112,23 @@ function routerReducer( screens = removeScreen( state, action.screen ); break; case 'goto': - locationHistory = goTo( state, action.path, action.options ); + currentLocation = goTo( state, action.path, action.options ); break; case 'gotoparent': - locationHistory = goToParent( state, action.options ); + currentLocation = goToParent( state, action.options ); break; } // 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,7 +145,7 @@ function routerReducer( matchedPath = state.matchedPath; } - return { screens, locationHistory, matchedPath }; + return { screens, currentLocation, matchedPath }; } function UnconnectedNavigatorProvider( @@ -212,7 +160,7 @@ function UnconnectedNavigatorProvider( initialPath, ( path ) => ( { screens: [], - locationHistory: [ { path } ], + currentLocation: { path }, matchedPath: undefined, } ) ); @@ -242,19 +190,19 @@ function UnconnectedNavigatorProvider( [] ); - const { locationHistory, matchedPath } = routerState; + const { currentLocation, matchedPath } = routerState; const navigatorContextValue: NavigatorContextType = useMemo( () => ( { location: { - ...locationHistory[ locationHistory.length - 1 ], - isInitial: locationHistory.length === 1, + ...currentLocation, + isInitial: false, // TODO: find out an alternative to this }, params: matchedPath?.params ?? {}, match: matchedPath?.id, ...methods, } ), - [ locationHistory, matchedPath, methods ] + [ currentLocation, matchedPath, methods ] ); const cx = useCx(); From 30cebaf7046ff465f06f5610a49c8b6a644523ec Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 10:19:52 +0200 Subject: [PATCH 02/11] Deprecate `options.replace` --- packages/components/src/navigator/types.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/components/src/navigator/types.ts b/packages/components/src/navigator/types.ts index c45762d558af2..855787b4d0a19 100644 --- 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; }; From 819c52a58c4311d616da92254bded2a3af0d02ff Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 10:34:12 +0200 Subject: [PATCH 03/11] Make sure that two screens with the same path cannot be added --- .../src/navigator/navigator-provider/component.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index fbb34776a9ce6..6777d5315912b 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -43,6 +43,14 @@ type RouterState = { }; 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 ]; } From 51c78f5975a467ae85c425d4bb27895a8324fee8 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 10:57:07 +0200 Subject: [PATCH 04/11] Re-implement focus restoration relying on a map with paths as keys --- .../navigator-provider/component.tsx | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 6777d5315912b..412eff8c4b029 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -40,6 +40,7 @@ type RouterState = { screens: Screen[]; currentLocation?: NavigatorLocation; matchedPath: MatchedPath; + focusSelectors: Map< string, string >; }; function addScreen( { screens }: RouterState, screen: Screen ) { @@ -63,7 +64,8 @@ function goTo( path: string, options: NavigateOptions = {} ) { - const { currentLocation } = state; + const { currentLocation, focusSelectors } = state; + const { // Default assignments isBack = false, @@ -79,11 +81,25 @@ function goTo( return currentLocation; } + // Set a focus selector that will be used when navigating + // back to the current location. + if ( focusTargetSelector && currentLocation?.path ) { + focusSelectors.set( currentLocation.path, focusTargetSelector ); + } + + // Get the focus selector for the new location. + let currentFocusSelector; + if ( isBack ) { + currentFocusSelector = focusSelectors.get( path ); + focusSelectors.delete( path ); + } + return { ...restOptions, path, isBack, hasRestoredFocus: false, + focusTargetSelector: currentFocusSelector, skipFocus, }; } @@ -111,7 +127,7 @@ function routerReducer( state: RouterState, action: RouterAction ): RouterState { - let { screens, currentLocation, matchedPath } = state; + let { screens, currentLocation, matchedPath, ...restState } = state; switch ( action.type ) { case 'add': screens = addScreen( state, action.screen ); @@ -153,7 +169,7 @@ function routerReducer( matchedPath = state.matchedPath; } - return { screens, currentLocation, matchedPath }; + return { ...restState, screens, currentLocation, matchedPath }; } function UnconnectedNavigatorProvider( @@ -170,6 +186,7 @@ function UnconnectedNavigatorProvider( screens: [], currentLocation: { path }, matchedPath: undefined, + focusSelectors: new Map(), } ) ); From 04667721e5fafc65a21dcc115da97ba06272e304 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 11:04:11 +0200 Subject: [PATCH 05/11] Re-implement the isInitial flag by relying on the initialPath --- .../navigator/navigator-provider/component.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 412eff8c4b029..1532c0692b92e 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -37,6 +37,7 @@ type RouterAction = | { type: 'gotoparent'; options?: NavigateToParentOptions }; type RouterState = { + initialPath: string; screens: Screen[]; currentLocation?: NavigatorLocation; matchedPath: MatchedPath; @@ -176,17 +177,22 @@ 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: [], currentLocation: { path }, matchedPath: undefined, focusSelectors: new Map(), + initialPath: initialPathProp, } ) ); @@ -215,19 +221,19 @@ function UnconnectedNavigatorProvider( [] ); - const { currentLocation, matchedPath } = routerState; + const { currentLocation, matchedPath, initialPath } = routerState; const navigatorContextValue: NavigatorContextType = useMemo( () => ( { location: { ...currentLocation, - isInitial: false, // TODO: find out an alternative to this + isInitial: currentLocation?.path === initialPath, }, params: matchedPath?.params ?? {}, match: matchedPath?.id, ...methods, } ), - [ currentLocation, matchedPath, methods ] + [ currentLocation, matchedPath, methods, initialPath ] ); const cx = useCx(); From 5a7448c283f630ebbd5f0774930d442341196586 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 11:04:18 +0200 Subject: [PATCH 06/11] Update README --- .../components/src/navigator/navigator-provider/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/README.md b/packages/components/src/navigator/navigator-provider/README.md index 13745fae68a15..35bf7a69720be 100644 --- 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[] >` From b3d460b0cde661ab2f95992f32e16ffd4fab346c Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 12:01:34 +0200 Subject: [PATCH 07/11] Comment map deletion out --- .../components/src/navigator/navigator-provider/component.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 1532c0692b92e..2e5b726035763 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -92,7 +92,7 @@ function goTo( let currentFocusSelector; if ( isBack ) { currentFocusSelector = focusSelectors.get( path ); - focusSelectors.delete( path ); + // focusSelectors.delete( path ); } return { From 89c3f4d8294d6997b8815f723e1418d420a12d9e Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Aug 2024 12:03:54 +0200 Subject: [PATCH 08/11] CHANGELOG --- packages/components/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 542d2b8334ed6..44536df98b581 100644 --- 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) From 77fb59f4baa27133122a0320d4db07067cd3a5b8 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 23 Aug 2024 16:07:26 +0200 Subject: [PATCH 09/11] Fix broken focus restoration by updating focusSelectors in the navigator reducer --- .../navigator-provider/component.tsx | 55 +++++++++++++------ 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 2e5b726035763..2ce466c2351af 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -79,29 +79,34 @@ function goTo( } = options; if ( currentLocation?.path === path ) { - return currentLocation; + return { currentLocation, focusSelectors }; } + const focusSelectorsCopy = new Map( state.focusSelectors ); + // Set a focus selector that will be used when navigating // back to the current location. if ( focusTargetSelector && currentLocation?.path ) { - focusSelectors.set( currentLocation.path, focusTargetSelector ); + focusSelectorsCopy.set( currentLocation.path, focusTargetSelector ); } // Get the focus selector for the new location. let currentFocusSelector; if ( isBack ) { - currentFocusSelector = focusSelectors.get( path ); - // focusSelectors.delete( path ); + currentFocusSelector = focusSelectorsCopy.get( path ); + focusSelectorsCopy.delete( path ); } return { - ...restOptions, - path, - isBack, - hasRestoredFocus: false, - focusTargetSelector: currentFocusSelector, - skipFocus, + currentLocation: { + ...restOptions, + path, + isBack, + hasRestoredFocus: false, + focusTargetSelector: currentFocusSelector, + skipFocus, + }, + focusSelectors: focusSelectorsCopy, }; } @@ -109,14 +114,14 @@ function goToParent( state: RouterState, options: NavigateToParentOptions = {} ) { - const { currentLocation, screens } = state; + const { currentLocation, screens, focusSelectors } = state; const currentPath = currentLocation?.path; if ( currentPath === undefined ) { - return currentLocation; + return { currentLocation, focusSelectors }; } const parentPath = findParent( currentPath, screens ); if ( parentPath === undefined ) { - return currentLocation; + return { currentLocation, focusSelectors }; } return goTo( state, parentPath, { ...options, @@ -128,7 +133,13 @@ function routerReducer( state: RouterState, action: RouterAction ): RouterState { - let { screens, currentLocation, matchedPath, ...restState } = state; + let { + screens, + currentLocation, + matchedPath, + focusSelectors, + ...restState + } = state; switch ( action.type ) { case 'add': screens = addScreen( state, action.screen ); @@ -137,10 +148,14 @@ function routerReducer( screens = removeScreen( state, action.screen ); break; case 'goto': - currentLocation = goTo( state, action.path, action.options ); + const goToNewState = goTo( state, action.path, action.options ); + currentLocation = goToNewState.currentLocation; + focusSelectors = goToNewState.focusSelectors; break; case 'gotoparent': - currentLocation = goToParent( state, action.options ); + const goToParentNewState = goToParent( state, action.options ); + currentLocation = goToParentNewState.currentLocation; + focusSelectors = goToParentNewState.focusSelectors; break; } @@ -170,7 +185,13 @@ function routerReducer( matchedPath = state.matchedPath; } - return { ...restState, screens, currentLocation, matchedPath }; + return { + ...restState, + screens, + currentLocation, + matchedPath, + focusSelectors, + }; } function UnconnectedNavigatorProvider( From 7e159f8a268dab0d69ccec68d6b0b9cb1b6ea34b Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 23 Aug 2024 18:30:50 +0200 Subject: [PATCH 10/11] create a copy of focusSelectors map only when necessary --- .../src/navigator/navigator-provider/component.tsx | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 2ce466c2351af..b2a72cc3ae228 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -82,17 +82,23 @@ function goTo( return { currentLocation, focusSelectors }; } - const focusSelectorsCopy = new Map( state.focusSelectors ); + let focusSelectorsCopy; // 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 ); } // 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 ); } @@ -106,7 +112,7 @@ function goTo( focusTargetSelector: currentFocusSelector, skipFocus, }, - focusSelectors: focusSelectorsCopy, + focusSelectors: focusSelectorsCopy ?? focusSelectors, }; } From 29e0fb2eb74d018b8f52233c8f53a6b803ebdecb Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 23 Aug 2024 18:32:00 +0200 Subject: [PATCH 11/11] Set isInitial inside the reducer, instead of on the context value --- .../navigator/navigator-provider/component.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index b2a72cc3ae228..8e596778c7f21 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -39,7 +39,7 @@ type RouterAction = type RouterState = { initialPath: string; screens: Screen[]; - currentLocation?: NavigatorLocation; + currentLocation: NavigatorLocation; matchedPath: MatchedPath; focusSelectors: Map< string, string >; }; @@ -165,6 +165,10 @@ function routerReducer( break; } + if ( currentLocation?.path === state.initialPath ) { + currentLocation = { ...currentLocation, isInitial: true }; + } + // Return early in case there is no change if ( screens === state.screens && @@ -248,19 +252,16 @@ function UnconnectedNavigatorProvider( [] ); - const { currentLocation, matchedPath, initialPath } = routerState; + const { currentLocation, matchedPath } = routerState; const navigatorContextValue: NavigatorContextType = useMemo( () => ( { - location: { - ...currentLocation, - isInitial: currentLocation?.path === initialPath, - }, + location: currentLocation, params: matchedPath?.params ?? {}, match: matchedPath?.id, ...methods, } ), - [ currentLocation, matchedPath, methods, initialPath ] + [ currentLocation, matchedPath, methods ] ); const cx = useCx();