Style Book: Focus the Style Book when opened, and enable ESCAPE key to close #48151
+52
−6
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
andrewserong
added
[Type] Bug
|
Size Change: +57 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d302f48. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4229314649
|
apeatling
reviewed
Feb 17, 2023
|
Thanks for taking this for a spin @apeatling!
One of the things to watch out for is that the close button is the first element in the DOM within the style book, so if you tab forward from the close button it then goes to the tabs in the stylebook, whereas visually you'd expect SHIFT-TAB to go there. It looks like the close button was intentionally placed as the first element there, so I'm not too sure what would be best in terms of accessibility, but it's probably a fairly easy change to move things around as needed (either in this PR or in another one). |
|
It's more standard practice to focus a close button, as a quick escape if the user realizes this isn't something they want and to orient them to a way to exit. But yes, the design does end up leading to an issue with a mismatch between the visual focus order and the actual order. This doesn't impact where the initial focus should be placed; the focus order mismatch exists no matter where the initial focus lands. Practically, if the design had the tabs at a lower vertical level from the close button the order would match. The close button should be first in the container, so I think that there aren't a lot of options available. |
|
Thanks for confirming @joedolson! I'll leave the tab order as-is for now then 👍 |
kevin940726
approved these changes
Feb 20, 2023
|
@joedolson now that there's an approving review from the code side of things, does this PR look okay to you as an incremental accessibility improvement? |
andrewserong
force-pushed
the
fix/style-book-focus-on-mount-and-escape-key-behavior
branch
from
8594232
to
8260102
Compare
kevin940726
reviewed
Feb 21, 2023
| await page.click( | ||
| 'role=region[name="Style Book"i] >> role=button[name="Close Style Book"i]' | ||
| ); | ||
| await expect( | ||
| page.locator( 'role=region[name="Style Book"i]' ), |
There was a problem hiding this comment.
Nit: We can hoist this locator and reuse it in the test:
const styleBook = page.getByRole( 'region', { name: 'Style Book' } );
await expect( styleBook ).toBeVisible();
await styleBook.getByRole( 'button', { name: 'Close Style Book' } ).click();Also it's now recommended to use getByRole rather than role-selector after a recent change of Playwright's doc 😅 .
There was a problem hiding this comment.
Oh, thanks for the tip! In d302f48 I've named the const styleBookRegion to distinguish between it and the styleBook instance of the StyleBook class that's available to each of the tests.
I've left the other tests untouched, though I see they could be switched to page.getByRole in a follow-up. Happy to keep tweaking this test, though!
There was a problem hiding this comment.
This is perfect! Thanks for continuing to follow the best practices 💯 .
FWIW, I'm also trying to build a codemod to automate the transformation from role-selector to getByRole, so no need to manually refactor the existing ones just yet. 👍 Just keep in mind that from now on, getByRole is the preferred way to select elements. It's also updated in the doc too.
apeatling
approved these changes
Feb 21, 2023
|
Noting this worked in both Safari and Chrome for me. |
andrewserong
added
the
Backport to WP 6.7 Beta/RC
|
Thanks for confirming @apeatling! Since I've got a couple of approving reviews, I'll merge this in now. @joedolson, let me know if you run into any issues, though, and I can follow up in another PR. |
andrewserong
deleted the
fix/style-book-focus-on-mount-and-escape-key-behavior
branch
ntsekouras
pushed a commit
that referenced
this pull request
Feb 27, 2023
…o close (#48151) * Style Book: Focus the Style Book when opened, and enable ESCAPE key to close Style Book * Add e2e test for Escape key * Combine e2e tests * Store style book locator in a const and reuse
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: c0bbc76 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
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.
What?
This PR addresses part of #48054 by attempting to improve the keyboard navigation for opening and closing the Style Book. It does not address the problems of returning to the Style Book from other parts of global styles.
The features in this PR are:
Why?
#48054 flagged a number of keyboard navigation challenges with the Style Book. This PR seeks to improve the keyboard navigation incrementally by focusing on one particular area — opening and closing the Style Book via keyboard.
How?
useFocusOnMountanduseFocusReturnhooks to allow focusing to the first tabbable element when the Style Book is mounted, and to return focus to the button in the sidebar once the Style Book is closed. This follows the same logic as used by the List View sidebar.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
The below screengrab demonstrates the following:
style-book-keyboard-navigation.mp4