Implement roving tabindex on the header toolbar #22354
Conversation
diegohaz
added
[Focus] Accessibility (a11y)
|
Size Change: +567 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
diegohaz
changed the title
There was a problem hiding this comment.
I need to test but it looks very good code wise. I'm surprised that it didn't require too much work to convert the header toolbar. I left some notes related to the implementation where we could consider changes to make code more concise but those aren't blockers.
| @@ -24,11 +27,15 @@ function EditorHistoryRedo( { hasRedo, redo } ) { | |||
| ); | |||
| } | |||
|
|
|||
| export default compose( [ | |||
| const EnhancedEditorHistoryRedo = compose( [ | |||
There was a problem hiding this comment.
Have you considered refactoring this and other similar components (EditorHistoryUndo or TableOfContents) to use hooks? I think it would remove the need to use innerRef and you could apply ref directly on the component.
There was a problem hiding this comment.
I haven't played with @wordpress/data hooks yet, but that's a good idea! Will take a note on that for a follow up PR.
| /> | ||
| ) } | ||
| </ToolbarItem> | ||
| { isLargeViewport && ( |
There was a problem hiding this comment.
I'm flagging this for myself to test what happens with focus when you change viewport of your browser and this button disappears when focused. I know it's an edge case and it might be an issue before but I'm simply curious :)
There was a problem hiding this comment.
I tested it. When one of the blocks were selected before, then focus moves to the selected block 🙃
There was a problem hiding this comment.
I was wrong, the blinking cursor tricked me, so the focus gets removed, and when you tab it gets restored to one of the items in the toolbar, not that bad I guess. Could Reakit move focus automatically to the next/previous item in such case or would be too far in what it should do? :)
There was a problem hiding this comment.
In the first iterations, Reakit's Composite (and therefore Toolbar) automatically moved focus to the next/previous item when the current focused item got unmounted. Later it was changed so the previously focused item would get focus, like when you open and close a new tab on Chrome. But I haven't found enough material to support any of those approaches, and this was making the code even more complex for little gain, so I reverted it.
For now, toolbar.currentId is still set to the previously focused item, but it doesn't automatically move focus. It's up to the user to decide what to do, but I do plan to revisit it in the future.
There was a problem hiding this comment.
I would expect it’s going to be tricky to handle 😅 A similar challenge is when you click a button and it should become disabled, like when triggering undo until there is nothing to undo.
In this particular case, I assume it’s fine to clean up internally Reakit’s state - that is the current value and leave the rest (focus) of handling to the implementer.
packages/edit-post/src/components/header/header-toolbar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/header/header-toolbar/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Everything works as expected 🎉 I tested with both the floating toolbar and the top toolbar and the header toolbar works as expected.
There are a few issues that start to become very annoying that existed before and they are now more prominent. We need to discuss which deserve their own issues to be tackled separately. Some examples:
- You can't use arrow down to open Tools dropdown:
- Sometimes using the arrow up/down moves the focus to the editor and it isn't that hard to be tricked that you should use those arrows when you see movers stacker vertically:
- It feels like the editor intercepts arrow up/down when the block is selected. Actually, I can consistently reproduce it in my testing.
- When you open the inserter, there is no easy way to go back
escdoesn't work and focus is moved to the search by default, cc @youknowriad:
- should this button have
aria-pressedset in that case, or maybearia-expand, it's tricky
- It's time to remove tabbing from dropdown menus and promote using arrow keys consistently :)
diegohaz
requested review from
ajitbohra,
chrisvanpatten and
jorgefilipecosta
as code owners
| @@ -14,24 +14,31 @@ import warning from '@wordpress/warning'; | |||
| */ | |||
| import ToolbarContext from '../toolbar-context'; | |||
|
|
|||
| function ToolbarItem( { children, ...props }, ref ) { | |||
| function ToolbarItem( { children, as: Component, ...props }, ref ) { | |||
There was a problem hiding this comment.
I think at some point we need to introduce consistency to our components. Some of them use "tagName", others "as". We kind maybe move to "as" as a bit nicer while maintaining fallbacks for BC.
This PR is part of #18619, whose main goal is to implement roving tabindex on the
@wordpress/components'Toolbarcomponent and to use it on the header and block toolbars so they become a single tab stop as recommended by the WAI-ARIA Toolbar Pattern. Related issues are #15331 and #3383.This PR continues the work done in #20008, but now for the header toolbar.
The GIF below demonstrates the navigation through the header toolbar with arrow keys and Tab. When using Tab, instead of navigating to the next toolbar button, it moves focus out of the toolbar. And, when pressing Shift+Tab, it moves focus back to the toolbar on the button that had focus previously:
This also works when the "Top toolbar" option is enabled and the block toolbar is appended to the header toolbar:
When the block toolbar doesn't use
ToolbarButtonorToolbarItemas their buttons, which may be the case of some of our blocks and especially third party blocks, the header toolbar will fallback to the old behavior, where pressing Tab moves focus through the toolbar buttons. This is better explained on #20008.