Style Book: Focus the Style Book when opened, and enable ESCAPE key to close#48151
Merged
andrewserong merged 4 commits intoFeb 21, 2023
Merged
Conversation
|
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
Contributor
apeatling
left a comment
There was a problem hiding this comment.
Initial testing of this seemed to work correctly, but I did have some troubles with the tabbing and keyboard navigation. The stylebook did open and close as expected though. I will record a screen grab on Monday and re-test.
Contributor
Author
|
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). |
Contributor
|
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. |
Contributor
Author
|
Thanks for confirming @joedolson! I'll leave the tab order as-is for now then 👍 |
kevin940726
approved these changes
Feb 20, 2023
Member
kevin940726
left a comment
There was a problem hiding this comment.
Tested in both Brave and Safari and it worked as expected 👍 .
Contributor
Author
|
@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? |
8594232 to
8260102
Compare
kevin940726
reviewed
Feb 21, 2023
| 'role=region[name="Style Book"i] >> role=button[name="Close Style Book"i]' | ||
| ); | ||
| await expect( | ||
| page.locator( 'role=region[name="Style Book"i]' ), |
Member
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 😅 .
Contributor
Author
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!
Member
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
Contributor
apeatling
left a comment
There was a problem hiding this comment.
Tested again and this is working well for me now! 👍
Contributor
|
Noting this worked in both Safari and Chrome for me. |
Contributor
Author
|
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. |
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
Contributor
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: c0bbc76 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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