-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Style Book: Focus the Style Book when opened, and enable ESCAPE key to close #48151
Style Book: Focus the Style Book when opened, and enable ESCAPE key to close #48151
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in both Brave and Safari and it worked as expected 👍 .
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 !
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and this is working well for me now! 👍
Noting this worked in both Safari and Chrome for me. |
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. |
…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 |
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?
useFocusOnMount
anduseFocusReturn
hooks 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