[Data Views] Experimental keyboard navigation for table view #56513
Conversation
andrewhayward
added
[Focus] Accessibility (a11y)
andrewhayward
requested review from
mcsf,
oandregal,
jasmussen,
ntsekouras and
a team
andrewhayward
added
[Type] Enhancement
| @@ -42,6 +48,8 @@ const defaultConfigPerViewType = { | |||
| }, | |||
| }; | |||
|
|
|||
| const DefaultWidget = forwardRef( ( { render } ) => render ); | |||
There was a problem hiding this comment.
This is only necessary until other views (i.e. GridView) implement this Widget system.
| &:focus, | ||
| &:focus-visible, | ||
| &:focus-within { | ||
| background-color: #f0f0f0; | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
This gives focused cells, or cells containing focused elements, more prominence in the table. Could do with some design input here.
// @WordPress/gutenberg-design
There was a problem hiding this comment.
From the video demo you shared the gray seems darker in that demo than #f0f0f0 (btw you could use $gray-100 there if you like). Was the video recorded earlier or is it just a visual illusion?
In any case my instinct is that if we are also showing the focus-ring, changing the background at the same time is likely going to be a bit noisy. It also might confuse if we end up adding a full-row background-color change on mouse hover. I'll defer to Jay, though, if he chimes in.
There was a problem hiding this comment.
It'd be good to understand the full requirements here before committing to a design.
I assume a focus style is required for cells that only contain data like the Status column in the video?
As you navigate focus transfers immediately to the first child of each cell where possible. In these cases do parent cells still need to be visually highlighted?
If the answer to these questions is yes then perhaps we need different styles for focus and focus-within.
There was a problem hiding this comment.
@andrewhayward Given that this table should and will be applied to many contexts, I feel like the outline should remain for focus. That particular gray might blend with a page background somewhere.
There was a problem hiding this comment.
Sorry, I'll provide a little more context for clarity.
The request for feedback is more around how we ensure that focal context is clearly communicated, rather than on the way I specifically did it here.
The current background colour is somewhat arbitrary (I think I copied it from the unfocused search field), so could be anything; frankly, the cells don't have to have a background colour at all, as long as focus is clear.
Interactive cell content receives focus as normal, but the cells themselves receive focus when they contain no interactive content. I used a background here to maintain consistency between these two different states, but it's not strictly necessary. To @jeffgolenski's point, the cells themselves could have a focus ring, and it wouldn't be a problem.
Essentially, the problem of how best to communicate the focal context is not straightforward, and doesn't necessarily map onto our existing patterns (though it could).
There was a problem hiding this comment.
We tend to use rings to denote focus throughout the Editor so I'd be inclined to try repeating that pattern here.
| DropdownMenuV2Ariakit: DropdownMenu, | ||
| DropdownMenuGroupV2Ariakit: DropdownMenuGroup, | ||
| DropdownMenuItemV2Ariakit: DropdownMenuItem, | ||
| DropdownMenuSeparatorV2Ariakit: DropdownMenuSeparator, |
There was a problem hiding this comment.
Migrating to the new Ariakit-based dropdown menu, which plays better with Composite.
| const Widget = forwardRef( ( widgetProps, innerRef ) => { | ||
| useEffect( () => setWidgetUsed( true ), [] ); | ||
| return <CompositeItem ref={ innerRef } { ...widgetProps } />; | ||
| } ); |
There was a problem hiding this comment.
This is the bit that we pass through to individual cell content render functions. If they make use of it, we adjust the layout here to not make the cell itself navigable.
| { children } | ||
| </a> | ||
| ); | ||
| } | ||
|
|
||
| export default forwardRef( UnforwardedLink ); |
There was a problem hiding this comment.
This is necessary to allow it to be used by Composite.
There was a problem hiding this comment.
So this is needed in order for title field to work properly, right? What happens if the components used by other fields doesn't forward the ref? In the future these fields could come from extenders. Will they need to always do that too?
If yes, can we absorb this in our internal implementation?
There was a problem hiding this comment.
This is because of how Ariakit requires custom components to be open for extension. I'm not sure how we'd absorb it, to be honest.
ciampo
changed the title
Important
The primary intent of this PR is ensuring tables are easier to navigate with a keyboard. As such, the code changes aren't super focused, and there's plenty of room for improvement! That being said, comments are of course welcome.
(Potentially) resolves #56328.
What?
This PR improves the keyboard navigation in list views, presenting the table as a single tab-stop to be navigated with directional keys (up, down, left, right, etc).
list-view-keyboard-navigation.mov
Why?
As per #56328...
How?
The
Compositecomponent is used to render the various table elements, to provide embedded keyboard navigation.A
Widgetsystem is introduced to allow render functions to indicate where navigable content should replace the wrapper context in the navigation flow.Testing Instructions
Testing Instructions for Keyboard
With a list view open:
To do