Site Editor: Fix bug where focus moved erroneously in navigation screens. #44239
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
jasmussen
approved these changes
Sep 19, 2022
There was a problem hiding this comment.
Nice, this fixes it for me! Thank you!
I would love a quick sanity check on the code, CC: @noisysocks maybe? But this looks good.
I'll take a look at this, since I wrote the component in the first place |
ciampo
added
[Type] Bug
ciampo
reviewed
Sep 19, 2022
There was a problem hiding this comment.
Thank you for working on this fix, @BE-Webdesign !
Code changes LGTM. Before merging, could you please add an entry to the package's CHANGELOG ? Thank you 🙏
For more context, it looks like this bug had always been there, but was "exposed" by the changes #43876 (cc @chad1008 )
It would be great if we could add a unit test to Navigator so that we can catch this specific regression in the future. Given that we may want to merge this fix ASAP, we could add the test in a follow-up PR — would you be able to do that, @BE-Webdesign ? If not, maybe @chad1008 could help.
|
I will be able to add the test case and changelog changes this weekend. |
Thank you! And no rush at all. You will likely want to rebase this PR on top of latest |
BE-Webdesign
force-pushed
the
fix/global-styles-focus-issue
branch
from
d594c1d to
b6fb35d
Compare
|
Tests added and changelog comment added. I went with a e2e test, because the unit tests were not tracking focus correctly as it does in the actual live environment. |
|
Size Change: +7 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 26, 2022
There was a problem hiding this comment.
I went with a e2e test, because the unit tests were not tracking focus correctly as it does in the actual live environment.
I'd prefer to have RTL unit/integration tests where possible — I had a look at Navigator's unit tests and realized that the reason why they didn't track focus correctly was because of the getClientRects() call in the isVisible utility function not working as expected in jest-dom, causing this call in NavigatorScreen to be unreliable during unit tests.
That can be fixed by patching the getClientRects function in the unit tests, and therefore the regression can be tested by adding a simple controlled input field that causes the NavigatorScreen to re-render when its value changes.
I managed to put together a working unit/integration test with the following changes (this test should also fail when un-doing the fix from this PR):
Click to expand
diff --git a/packages/components/src/navigator/test/index.js b/packages/components/src/navigator/test/index.js
index 497e1c2612..7374ede686 100644
--- a/packages/components/src/navigator/test/index.js
+++ b/packages/components/src/navigator/test/index.js
@@ -2,6 +2,12 @@
* External dependencies
*/
import { render, screen, fireEvent } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+
+/**
+ * WordPress dependencies
+ */
+import { useState } from '@wordpress/element';
/**
* Internal dependencies
@@ -86,60 +92,74 @@ function CustomNavigatorBackButton( { onClick, ...props } ) {
const MyNavigation = ( {
initialPath = PATHS.HOME,
onNavigatorButtonClick,
-} ) => (
- <NavigatorProvider initialPath={ initialPath }>
- <NavigatorScreen path={ PATHS.HOME }>
- <p>This is the home screen.</p>
- <CustomNavigatorButton
- path={ PATHS.NOT_FOUND }
- onClick={ onNavigatorButtonClick }
- >
- Navigate to non-existing screen.
- </CustomNavigatorButton>
- <CustomNavigatorButton
- path={ PATHS.CHILD }
- onClick={ onNavigatorButtonClick }
- >
- Navigate to child screen.
- </CustomNavigatorButton>
- <CustomNavigatorButton
- path={ PATHS.INVALID_HTML_ATTRIBUTE }
- onClick={ onNavigatorButtonClick }
- >
- Navigate to screen with an invalid HTML value as a path.
- </CustomNavigatorButton>
- </NavigatorScreen>
-
- <NavigatorScreen path={ PATHS.CHILD }>
- <p>This is the child screen.</p>
- <CustomNavigatorButtonWithFocusRestoration
- path={ PATHS.NESTED }
- onClick={ onNavigatorButtonClick }
- >
- Navigate to nested screen.
- </CustomNavigatorButtonWithFocusRestoration>
- <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
- Go back
- </CustomNavigatorBackButton>
- </NavigatorScreen>
-
- <NavigatorScreen path={ PATHS.NESTED }>
- <p>This is the nested screen.</p>
- <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
- Go back
- </CustomNavigatorBackButton>
- </NavigatorScreen>
-
- <NavigatorScreen path={ PATHS.INVALID_HTML_ATTRIBUTE }>
- <p>This is the screen with an invalid HTML value as a path.</p>
- <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
- Go back
- </CustomNavigatorBackButton>
- </NavigatorScreen>
-
- { /* A `NavigatorScreen` with `path={ PATHS.NOT_FOUND }` is purposefully not included. */ }
- </NavigatorProvider>
-);
+} ) => {
+ const [ inputValue, setInputValue ] = useState( '' );
+ return (
+ <NavigatorProvider initialPath={ initialPath }>
+ <NavigatorScreen path={ PATHS.HOME }>
+ <p>This is the home screen.</p>
+ <CustomNavigatorButton
+ path={ PATHS.NOT_FOUND }
+ onClick={ onNavigatorButtonClick }
+ >
+ Navigate to non-existing screen.
+ </CustomNavigatorButton>
+ <CustomNavigatorButton
+ path={ PATHS.CHILD }
+ onClick={ onNavigatorButtonClick }
+ >
+ Navigate to child screen.
+ </CustomNavigatorButton>
+ <CustomNavigatorButton
+ path={ PATHS.INVALID_HTML_ATTRIBUTE }
+ onClick={ onNavigatorButtonClick }
+ >
+ Navigate to screen with an invalid HTML value as a path.
+ </CustomNavigatorButton>
+ </NavigatorScreen>
+
+ <NavigatorScreen path={ PATHS.CHILD }>
+ <p>This is the child screen.</p>
+ <CustomNavigatorButtonWithFocusRestoration
+ path={ PATHS.NESTED }
+ onClick={ onNavigatorButtonClick }
+ >
+ Navigate to nested screen.
+ </CustomNavigatorButtonWithFocusRestoration>
+ <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
+ Go 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>This is the nested screen.</p>
+ <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
+ Go back
+ </CustomNavigatorBackButton>
+ </NavigatorScreen>
+
+ <NavigatorScreen path={ PATHS.INVALID_HTML_ATTRIBUTE }>
+ <p>This is the screen with an invalid HTML value as a path.</p>
+ <CustomNavigatorBackButton onClick={ onNavigatorButtonClick }>
+ Go back
+ </CustomNavigatorBackButton>
+ </NavigatorScreen>
+
+ { /* A `NavigatorScreen` with `path={ PATHS.NOT_FOUND }` is purposefully not included. */ }
+ </NavigatorProvider>
+ );
+};
const getNavigationScreenByText = ( text, { throwIfNotFound = true } = {} ) => {
const fnName = throwIfNotFound ? 'getByText' : 'queryByText';
@@ -194,6 +214,28 @@ const getBackButton = ( { throwIfNotFound } = {} ) =>
} );
describe( 'Navigator', () => {
+ const originalGetClientRects = window.Element.prototype.getClientRects;
+
+ // `getClientRects` needs to be mocked so that `isVisible` from the `@wordpress/dom`
+ // `focusable` module can pass, in a JSDOM env where the DOM elements have no width/height.
+ const mockedGetClientRects = jest.fn( () => [
+ {
+ x: 0,
+ y: 0,
+ width: 100,
+ height: 100,
+ },
+ ] );
+
+ beforeAll( () => {
+ window.Element.prototype.getClientRects =
+ jest.fn( mockedGetClientRects );
+ } );
+
+ afterAll( () => {
+ window.Element.prototype.getClientRects = originalGetClientRects;
+ } );
+
it( 'should render', () => {
render( <MyNavigation /> );
@@ -404,4 +446,27 @@ describe( 'Navigator', () => {
expect( getHomeScreen() ).toBeInTheDocument();
expect( getToInvalidHTMLPathScreenButton() ).toHaveFocus();
} );
+
+ it( 'should keep focus on the element that is being interacted with, while re-rendering', async () => {
+ const user = userEvent.setup( {
+ advanceTimers: jest.advanceTimersByTime,
+ } );
+
+ render( <MyNavigation /> );
+
+ expect( getHomeScreen() ).toBeInTheDocument();
+ expect( getToChildScreenButton() ).toBeInTheDocument();
+
+ // Navigate to child screen.
+ await user.click( getToChildScreenButton() );
+
+ expect( getChildScreen() ).toBeInTheDocument();
+ expect( getBackButton() ).toBeInTheDocument();
+ expect( getToNestedScreenButton() ).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();
+ } );
} );While we're at it, we could also edit the Storybook example to feature an input element:
Click to expand
diff --git a/packages/components/src/navigator/stories/index.js b/packages/components/src/navigator/stories/index.js
index cbc710a28a..d92fa6e643 100644
--- a/packages/components/src/navigator/stories/index.js
+++ b/packages/components/src/navigator/stories/index.js
@@ -3,6 +3,11 @@
*/
import { css } from '@emotion/react';
+/**
+ * WordPress dependencies
+ */
+import { useState } from '@wordpress/element';
+
/**
* Internal dependencies
*/
@@ -24,6 +29,7 @@ export default {
};
const MyNavigation = () => {
+ const [ inputValue, setInputValue ] = useState( '' );
const cx = useCx();
return (
<NavigatorProvider
@@ -83,6 +89,13 @@ const MyNavigation = () => {
<NavigatorBackButton variant="secondary">
Go back
</NavigatorBackButton>
+ <input
+ name="sample-input"
+ onChange={ ( e ) =>
+ setInputValue( e.target.value )
+ }
+ value={ inputValue }
+ />
</CardBody>
</Card>
</NavigatorScreen>With these changes applied, we shouldn't need an e2e test anymore.
|
@ciampo, thank you for diving into that. I will be able to implement those fixes and remove the e2e test this weekend, then we should be good to ship it. I will also resolve the conflicts in the changelog. |
… to the Navigation Back Button after an input onChange event fired. Now focus remains with the focused input.
…NavigatorScreen component.
…ut elements within NavigatorScreens. Adds mock of getClientRects, so that elements are focusable programatically.
…ts are in testing same functionality.
BE-Webdesign
force-pushed
the
fix/global-styles-focus-issue
branch
from
ec790b5 to
d155462
Compare
|
@ciampo Okay, all changes have been made. I think we are good to go on this. |
|
Thank you for fixing this one! |
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.
Fixes #44191.
What?
Site Editor: Fix bug where focus moved back erroneously to the Navigation Back Button after an input onChange event fired. Now focus remains with the focused input.
Why?
Fixes a bug that interrupts user input, when focus moves incorrectly.
How?
Added a check to see if the navigation screen already has a focused element within it, and prevent the item from getting focused
Testing Instructions
Screenshots or screencast