Navigator: add focus restoration
#38149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Feature] Component System
ciampo
commented
Jan 21, 2022
| elementToFocus.focus(); | ||
| }, [ isInitialLocation, isMatch ] ); | ||
|
|
||
| const mergedWrapperRef = useMergeRefs( [ forwardedRef, wrapperRef ] ); |
There was a problem hiding this comment.
Merging the two refs, so that they are correctly assigned to the wrapper element regardless of the value of prefersReducedMotion
ciampo
commented
Jan 21, 2022
Comment on lines
+92
to
+102
| const firstTabbable = ( focus.tabbable.find( | ||
| wrapperRef.current | ||
| ) as HTMLElement[] )[ 0 ]; | ||
|
|
||
| elementToFocus = firstTabbable ?? wrapperRef.current; |
There was a problem hiding this comment.
This code effectively replicates the same logic as in the useFocusOnMount's logic
|
Size Change: +218 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/navigator-focus-restoration
branch
from
e0a2250
to
67d68df
Compare
mirka
approved these changes
Feb 1, 2022
There was a problem hiding this comment.
Tests out very well! 👍 There was a styling issue (#38414), but that was unrelated to this PR.
To be honest, it isn't immediately clear to me why there is a complicated system to manipulate isBack and isInitial flags in the stack. As a person who hasn't worked on this 😄, I would think that we could rely on simple stack mechanics instead of tinkering with these flags in the history array, if the current location were set explicitly in the push/pop functions instead of just peeking the stack.
const [ locationHistory, setLocationHistory ] = useState();
const [ location, setLocation ] = useState();
const push = path => {
setLocation({path, isBack: false}); // any flags are set here, instead of mucking with them in the history stack
setLocationHistory([...locationHistory, {path}]);
};
const pop = () => {
const newLocationHistory = [...locationHistory];
setLocation({...newLocationHistory.pop(), isBack: true});
setLocationHistory(newLocationHistory);
};^ This kind of way better matches my mental model of how a history stack works, but maybe there are other complications to consider. Non-blocking, just a thought!
|
Thank you @mirka for the review! I'm glad you took some time to look into the code and post some in-depth feedback with fresh eyes! Looking at your code, these are the major differences that stand out:
To summarise, I see points 1 and 2 more about personal preference (explicit vs derived location state, array "surgery" with mutable/immutable methods). Regarding the complexity of the Here's an example of what code be easily removeddiff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx
index c20e7aaa5c..69685e4de0 100644
--- a/packages/components/src/navigator/navigator-provider/component.tsx
+++ b/packages/components/src/navigator/navigator-provider/component.tsx
@@ -42,7 +42,6 @@ function NavigatorProvider(
>( [
{
path: initialPath,
- isBack: false,
isInitial: true,
},
] );
@@ -51,26 +50,18 @@ function NavigatorProvider(
( path, options = {} ) => {
const { focusTargetSelector, ...restOptions } = options;
- // Notes:
- // - the `isBack` flag is set to `false` when navigating forwards on both
- // the previous and the new location.
- // - the `isInitial` flag is set to `false` for the new location, to make
- // sure it doesn't get overridden by mistake.
- // - the `focusTargetSelector` prop is set on the current (soon previous)
- // location, as it is used to restore focus in NavigatorScreen. The
- // remaining options are instead set on the new location being pushed.
+ // `focusTargetSelector` is set on the current (soon to be previous)
+ // location, as it is used to restore focus in NavigatorScreen. The
+ // remaining options are instead set on the new location being pushed.
setLocationHistory( [
...locationHistory.slice( 0, -1 ),
{
...locationHistory[ locationHistory.length - 1 ],
- isBack: false,
focusTargetSelector,
},
{
...restOptions,
path,
- isBack: false,
- isInitial: false,
},
] );
},What do you think about the proposed changes? Do you still think we should change the code further? |
|
Thanks for the thoughtful reply! I trust your gut so I defer to your decision either way 🙂 It's just that in my experience, undo functionality like this was usually implemented with standard stack operation equivalents, without having to alter a previous item in the history stack. I don't really mind whether the "stack operation" is achieved by Could the const push = (path, focusTargetSelector) => {
setLocationHistory([...locationHistory, {path: location.path, focusTargetSelector}]); // push *current* path to history
setLocation({path, isBack: false}); // go to *new* path
};I'm also starting to wonder if API-wise it would be more friendly to name it something like |
This is a fair point. I though about a way to simplify this, and I came up with a potential solution (committed in 4bfccc7):
What do you think?
I personally like |
Much simplified, and I like the use of |
I don't have a strong preference. I guess the argument of push/pop is to make it closer to the browser history API? |
…focusTargetSelector`
ciampo
force-pushed
the
feat/navigator-focus-restoration
branch
from
4bfccc7
to
b3e5ede
Compare
Awesome! I'll give this PR one final check, rebase on top of
That is also a fair point, although the History API's public functions are Personally, I'm not sure it's a good idea to name |
This was referenced Feb 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Following the conversation in #37063 and the exploratory work done in #37223, this PR is the first step towards adding support for advanced focus restoration in
Navigator. The plan is to have the work split across 3 PRs:Navigator: add basic location history #37416)NavigationLinkandNavigationBackLinkcomponents (future PR)This PR adds focus restoration to the
Navigator:focusTargetSelectorto thepushfunction's optionsNavigatorProviderto store thefocusTargetSelectorwith the correct locationNavigatorScreenHow has this been tested?
Types of changes
New feature (technically not breaking, since the component is experimental)
Checklist:
*.native.jsfiles for terms that need renaming or removal).