Skip to content
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.

Sign up for GitHub

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

Navigator: restore focus only once per location #44972

Merged
merged 8 commits into from
Oct 17, 2022
Merged

Navigator: restore focus only once per location #44972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8178ce4
Create new focus management sub-describe
ciampo Oct 14, 2022
31682f6
Add a second, outer input to tests
ciampo Oct 14, 2022
0cd2f43
Simplify existing focus tests
ciampo Oct 14, 2022
b660bd3
Add test to check focus management with elements outside of navigator
ciampo Oct 14, 2022
4d24642
Add hasRestoredFocus prop on location to avoid focusing more than onc…
ciampo Oct 14, 2022
465311c
CHANGELOG
ciampo Oct 14, 2022
2c05f41
Typo
ciampo Oct 14, 2022
f6e5f33
Update packages/components/src/navigator/test/index.tsx
ciampo Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Bug Fix

- `FontSizePicker`: Ensure that fluid font size presets appear correctly in the UI controls ([#44791](https://github.com/WordPress/gutenberg/pull/44791)).
- `Navigator`: restore focus only once per location ([#44972](https://github.com/WordPress/gutenberg/pull/44972)).

### Documentation

Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/navigator/navigator-provider/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function NavigatorProvider(
...options,
path,
isBack: false,
hasRestoredFocus: false,
},
] );
},
Expand All @@ -62,6 +63,7 @@ function NavigatorProvider(
{
...locationHistory[ locationHistory.length - 2 ],
isBack: true,
hasRestoredFocus: false,
},
] );
}
Expand Down
12 changes: 10 additions & 2 deletions packages/components/src/navigator/navigator-screen/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,20 @@ function NavigatorScreen( props: Props, forwardedRef: ForwardedRef< any > ) {
// - if the current location is not the initial one (to avoid moving focus on page load)
// - when the screen becomes visible
// - if the wrapper ref has been assigned
if ( isInitialLocation || ! isMatch || ! wrapperRef.current ) {
// - if focus hasn't already been restored for the current location
if (
isInitialLocation ||
! isMatch ||
! wrapperRef.current ||
location.hasRestoredFocus
) {
return;
}

const activeElement = wrapperRef.current.ownerDocument.activeElement;

// If an element is already focused within the wrapper do not focus the
// element. This prevents inputs or buttons from losing focus unecessarily.
// element. This prevents inputs or buttons from losing focus unnecessarily.
if ( wrapperRef.current.contains( activeElement ) ) {
return;
}
Expand All @@ -93,10 +99,12 @@ function NavigatorScreen( props: Props, forwardedRef: ForwardedRef< any > ) {
elementToFocus = firstTabbable ?? wrapperRef.current;
}

location.hasRestoredFocus = true;
elementToFocus.focus();
}, [
isInitialLocation,
isMatch,
location.hasRestoredFocus,
location.isBack,
previousLocation?.focusTargetSelector,
] );
Expand Down
261 changes: 150 additions & 111 deletions packages/components/src/navigator/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,71 +141,91 @@ const MyNavigation = ( {
initialPath?: string;
onNavigatorButtonClick?: CustomTestOnClickHandler;
} ) => {
const [ inputValue, setInputValue ] = useState( '' );
const [ innerInputValue, setInnerInputValue ] = useState( '' );
const [ outerInputValue, setOuterInputValue ] = useState( '' );
return (
<NavigatorProvider initialPath={ initialPath }>
<NavigatorScreen path={ PATHS.HOME }>
<p>{ SCREEN_TEXT.home }</p>
<CustomNavigatorButton
path={ PATHS.NOT_FOUND }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toNonExistingScreen }
</CustomNavigatorButton>
<CustomNavigatorButton
path={ PATHS.CHILD }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toChildScreen }
</CustomNavigatorButton>
<CustomNavigatorButton
path={ PATHS.INVALID_HTML_ATTRIBUTE }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toInvalidHtmlPathScreen }
</CustomNavigatorButton>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.CHILD }>
<p>{ SCREEN_TEXT.child }</p>
<CustomNavigatorButtonWithFocusRestoration
path={ PATHS.NESTED }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toNestedScreen }
</CustomNavigatorButtonWithFocusRestoration>
<CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>

<label htmlFor="test-input">This is a test input</label>
<input
name="test-input"
// eslint-disable-next-line no-restricted-syntax
id="test-input"
onChange={ ( e ) => {
setInputValue( e.target.value );
} }
value={ inputValue }
/>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.NESTED }>
<p>{ SCREEN_TEXT.nested }</p>
<CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.INVALID_HTML_ATTRIBUTE }>
<p>{ SCREEN_TEXT.invalidHtmlPath }</p>
<CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>
</NavigatorScreen>

{ /* A `NavigatorScreen` with `path={ PATHS.NOT_FOUND }` is purposefully not included. */ }
</NavigatorProvider>
<>
<NavigatorProvider initialPath={ initialPath }>
<NavigatorScreen path={ PATHS.HOME }>
<p>{ SCREEN_TEXT.home }</p>
<CustomNavigatorButton
path={ PATHS.NOT_FOUND }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toNonExistingScreen }
</CustomNavigatorButton>
<CustomNavigatorButton
path={ PATHS.CHILD }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toChildScreen }
</CustomNavigatorButton>
<CustomNavigatorButton
path={ PATHS.INVALID_HTML_ATTRIBUTE }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toInvalidHtmlPathScreen }
</CustomNavigatorButton>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.CHILD }>
<p>{ SCREEN_TEXT.child }</p>
<CustomNavigatorButtonWithFocusRestoration
path={ PATHS.NESTED }
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.toNestedScreen }
</CustomNavigatorButtonWithFocusRestoration>
<CustomNavigatorBackButton
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>

<label htmlFor="test-input-inner">Inner input</label>
<input
name="test-input-inner"
// eslint-disable-next-line no-restricted-syntax
id="test-input-inner"
onChange={ ( e ) => {
setInnerInputValue( e.target.value );
} }
value={ innerInputValue }
/>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.NESTED }>
<p>{ SCREEN_TEXT.nested }</p>
<CustomNavigatorBackButton
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>
</NavigatorScreen>

<NavigatorScreen path={ PATHS.INVALID_HTML_ATTRIBUTE }>
<p>{ SCREEN_TEXT.invalidHtmlPath }</p>
<CustomNavigatorBackButton
onClick={ onNavigatorButtonClick }
>
{ BUTTON_TEXT.back }
</CustomNavigatorBackButton>
</NavigatorScreen>

{ /* A `NavigatorScreen` with `path={ PATHS.NOT_FOUND }` is purposefully not included. */ }
</NavigatorProvider>

<label htmlFor="test-input-outer">Outer input</label>
<input
name="test-input-outer"
// eslint-disable-next-line no-restricted-syntax
id="test-input-outer"
onChange={ ( e ) => {
setOuterInputValue( e.target.value );
} }
value={ outerInputValue }
/>
</>
);
};

Expand Down Expand Up @@ -379,38 +399,6 @@ describe( 'Navigator', () => {
} );
} );

it( 'should restore focus correctly', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <MyNavigation /> );

expect( getScreen( 'home' ) ).toBeInTheDocument();

// Navigate to child screen.
await user.click( getNavigationButton( 'toChildScreen' ) );

expect( getScreen( 'child' ) ).toBeInTheDocument();

// Navigate to nested screen.
await user.click( getNavigationButton( 'toNestedScreen' ) );

expect( getScreen( 'nested' ) ).toBeInTheDocument();

// Navigate back to child screen, check that focus was correctly restored.
await user.click( getNavigationButton( 'back' ) );

expect( getScreen( 'child' ) ).toBeInTheDocument();
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();

// Navigate back to home screen, check that focus was correctly restored.
await user.click( getNavigationButton( 'back' ) );

expect( getScreen( 'home' ) ).toBeInTheDocument();
expect( getNavigationButton( 'toChildScreen' ) ).toHaveFocus();
} );

it( 'should escape the value of the `path` prop', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
Expand Down Expand Up @@ -445,26 +433,77 @@ describe( 'Navigator', () => {
).toHaveFocus();
} );

it( 'should keep focus on the element that is being interacted with, while re-rendering', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
describe( 'focus management', () => {
Copy link
Contributor Author

@ciampo ciampo Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are:

  • moving tests around (into the new describe for focus management)
  • removing unnecessary assertions (those assertions are already made in other tests in the same file)
  • adding a new test specifically for the bug that this PR is fixing

Reviewing the PR commit-by-commit should make it easier to review them

it( 'should restore focus correctly', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <MyNavigation /> );

// Navigate to child screen.
await user.click( getNavigationButton( 'toChildScreen' ) );

// The first tabbable element receives focus.
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();

// Navigate to nested screen.
await user.click( getNavigationButton( 'toNestedScreen' ) );

// The first tabbable element receives focus.
expect( getNavigationButton( 'back' ) ).toHaveFocus();

// Navigate back to child screen.
await user.click( getNavigationButton( 'back' ) );

// The first tabbable element receives focus.
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();

// Navigate back to home screen, check that focus was correctly restored.
await user.click( getNavigationButton( 'back' ) );

// The first tabbable element receives focus.
expect( getNavigationButton( 'toChildScreen' ) ).toHaveFocus();
} );

render( <MyNavigation /> );
it( 'should keep focus on an active element inside navigator, while re-rendering', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

expect( getScreen( 'home' ) ).toBeInTheDocument();
expect( getNavigationButton( 'toChildScreen' ) ).toBeInTheDocument();
render( <MyNavigation /> );

// Navigate to child screen.
await user.click( getNavigationButton( 'toChildScreen' ) );
// Navigate to child screen.
await user.click( getNavigationButton( 'toChildScreen' ) );

expect( getScreen( 'child' ) ).toBeInTheDocument();
expect( getNavigationButton( 'back' ) ).toBeInTheDocument();
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();
// The first tabbable element receives focus.
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();

// Interact with the input, the focus should stay on the input element.
const input = screen.getByLabelText( 'This is a test input' );
await user.type( input, 'd' );
expect( input ).toHaveFocus();
// Interact with the inner input.
// The focus should stay on the input element.
const innerInput = screen.getByLabelText( 'Inner input' );
await user.type( innerInput, 'd' );
expect( innerInput ).toHaveFocus();
} );

it( 'should keep focus on an active element outside navigator, while re-rendering', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <MyNavigation /> );

// Navigate to child screen.
await user.click( getNavigationButton( 'toChildScreen' ) );

// The first tabbable element receives focus.
expect( getNavigationButton( 'toNestedScreen' ) ).toHaveFocus();

// Interact with the outer input.
// The focus should stay on the input element.
const outerInput = screen.getByLabelText( 'Outer input' );
await user.type( outerInput, 'd' );
expect( outerInput ).toHaveFocus();
} );
} );
} );
1 change: 1 addition & 0 deletions packages/components/src/navigator/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export type NavigatorLocation = NavigateOptions & {
isInitial?: boolean;
isBack?: boolean;
path?: string;
hasRestoredFocus?: boolean;
};

export type NavigatorContext = {
Expand Down