Add new TreeGrid and RovingTabIndex components #19799
Conversation
talldan
added
[Type] Enhancement
talldan
changed the title
There was a problem hiding this comment.
Thanks for working on this, @talldan!
I tested the Block Navigator modal with VoiceOver + Safari. It works well! It announces that I am in a table (which is probably an effect of role="treegrid"), and I can navigate through the items using arrow keys (or VO + arrow keys). One thing I missed though was knowing my current position in the table, which is something you get for free when using native table elements. I guess this can be achieved by using attributes like aria-setsize and aria-posinset.
Regarding the API, when I read NavigableTreeGrid, I expect that it will render a role="treegrid" element that already manages focus for me, including the roving tabindex method, as the WAI-ARIA practices recommend. It also sounds a bit redundant since a TreeGrid should be navigable already.
I would expect the usage to be something like this:
<TreeGrid>
<TreeGridRow expanded>
<TreeGridCell>...</TreeGridCell>
<TreeGridCell>...</TreeGridCell>
<TreeGridCell>...</TreeGridCell>
</TreeGridRow>
<TreeGridRow level={2}>
<TreeGridCell>...</TreeGridCell>
<TreeGridCell>...</TreeGridCell>
<TreeGridCell>...</TreeGridCell>
</TreeGridRow>
</TreeGrid>That would render a markup like this:
<table role="treegrid">
<tbody>
<tr role="row" aria-level="1" aria-posinset="1" aria-setsize="1" aria-expanded="true" tabindex="0">
<td role="gridcell">...</td>
<td role="gridcell">...</td>
<td role="gridcell">...</td>
</tr>
<tr role="row" aria-level="2" aria-posinset="1" aria-setsize="1" tabindex="-1">
<td role="gridcell">...</td>
<td role="gridcell">...</td>
<td role="gridcell">...</td>
</tr>
</tbody>
</table>(Even though this example uses native table elements, we still need those aria-posinset and aria-setsize attributes to represent the hierarchy in the table. Otherwise, in this example, browsers would guess aria-setsize="2" by default since it has two rows)
This kind of component would require much more effort! As a low hanging fruit, I would try to merge both NavigableTreeGrid and RovingTabIndex into a sort of bidimensional NavigableContainer. I would try to avoid the name TreeGrid as this could be misleading.
<NavigableContainer2D>
<div role="treegrid">
...
</div>
</NavigableContainer2D>This would require replacing the role="treeitem" selectors in the NavigableTreeGrid code by something more generic (e.g. a NavigableRow2D component that could be translated into role="treeitem" by the component consumer).
EDIT: Looks like aria-posinset and aria-setsize aren't allowed on role="row". See #19799 (comment)
packages/block-editor/src/components/roving-tab-index/container.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/navigable-tree-grid/index.js
Outdated
Show resolved
Hide resolved
|
Also, this is a good candidate to be tested with this |
|
Thanks for the feedback @diegohaz. Hmm, at the moment it looks like my implementation is closer to a Treeview in the examples, I should look at changing the markup to a table. Not sure how I missed that. Your idea of making the RovingTabIndex behaviour implicit is good as well, I think I'll do that. Cheers for the pointers! |
There was a problem hiding this comment.
Great work exploring this pattern. Props to @tellthemachines for raising it in the first place. I'll defer the testing part to @diegohaz who is an expert in the field and has the most experience developing similar features. 😄
I wanted to discuss some other aspects and find out what are your thoughts on the following:
This PR adds new components to the block-editor package for implementing a tree grid ,as described at WAI ARIA authoring practices - Tree Grid.
As noted in other comments, similar components exist in @wordpress/components package. Why do you think it would fit better to the @wordpress/block-editor? For me, it doesn't seem it solves anything specific to the block editor.
I shared 2 components that use different mechanisms for the roving pattern. We discussed it with @diegohaz when he worked on the accessible version of Toolbar components. We agreed that eventually, Reakit's Rover should be the only way supported. This PR introduces yet another way because existing solutions seem to be not flexible enough. Do you have any thoughts if this could be consolidated someday? Does it even make sense in the first place?
Those are open questions, and I'm totally fine with landing this PR in the current shape, even if we have a different vision for the future based on the discussion. I wanted to ensure we have a good plan how to coordinate all the accessibility efforts.
Finally, I think the most important part is that we have those components covered with tests. It echos what @diegohaz shared about unit tests. If there are good reasons to move those components to @wordpress/components we should add Storybook stories to make it easier to test them in isolation.
| - [Editor Menu Bar](https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-2/menubar-2.html) | ||
| - [Navigation Menu Bar](https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html) | ||
| - [Radio Group](https://www.w3.org/TR/wai-aria-practices/examples/radio/radio-1/radio-1.html) | ||
| - [Toolbar](https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html) |
There was a problem hiding this comment.
Noting that for the fully accessible version of Toolbar in @wordpress/components we use Rover from Reakit.
Based on the Reakit's documentation only, I don't think it supports grids though.
There was a problem hiding this comment.
Is that in an open PR? I couldn't find it in the codebase.
As you say it doesn't seem to support grids. The RovingTabIndex components in this PR only handle focus management, and they're intended to be composed with a separate component for keyboard navigation, so that's the main difference.
There was a problem hiding this comment.
It's part of the codebase. It still isn't integrated with the block editor because of the required refactor to the SlotFill implementation.
You can test it in Storybook:
https://wordpress.github.io/gutenberg/?path=/story/components-toolbar--default
| Some patterns that implement a roving tab index are: | ||
|
|
||
| - [Layout Grid](https://www.w3.org/TR/wai-aria-practices/examples/grid/LayoutGrids.html) | ||
| - [Editor Menu Bar](https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-2/menubar-2.html) |
There was a problem hiding this comment.
Noting that for the dropdown menus we use NavigableMenu from @wordpress/components which is based on more general NavigableContainer that also implements roving but limited to horizontal or vertical orientation.
There was a problem hiding this comment.
I couldn't see that they do implement the roving tab index in the traditional sense. For example, in the editor more menu it's still possible to tab through the individual menu items. If it were a roving tab index the entire menu would only a single tab stop.
It should be possible to use this component to make it behave with a roving tab index, though. If we wanted to 😄.
There was a problem hiding this comment.
The current tabbing behavior for the dropdown menu is broken, similarly to the legacy version of toolbars :)
It should be all refactored to use roving. This is why I raised it and marked as something to coordinate for the future so we don't maintain 3 different ways to deal with roving.
There was a problem hiding this comment.
I'm not sure we'd want to have roving behaviour on the more menu. It's a one dimensional dropdown that has to be clicked on to open, so I would expect to be able to tab through it.
There was a problem hiding this comment.
@tellthemachines, to be clear. The more menu supports both arrows down/up and tabbing for navigation between menu items as of today. You can also use the arrow down key to open this dropdown.
packages/block-editor/src/components/roving-tab-index/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/roving-tab-index/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| ### `isExpanded: boolean` | ||
|
|
||
| An optional value that designates whether a row is expanded or collapsed. Currently this value only sets the correct aria-expanded property on a row, it has no other built-in behaviour. |
There was a problem hiding this comment.
Maybe I should just use the aria- equivalents of these props rather than aliasing them?
There was a problem hiding this comment.
Having those custom props may be useful for supporting React Native as well in the future.
@gziolo Yep, that's true. I wasn't sure what the best place for them would be and wasn't sure if I'll work on moving them to I'll also look into the testing and give @diegohaz's PR a review, that will probably have to be on Monday, though. |
@youknowriad introduced a new package by extracting components from
@youknowriad, can you help to pick home for new components? :) |
talldan
changed the title
talldan
force-pushed
the
try/grid-accessibility-components
branch
from
81e31fb
to
c8b8ac0
Compare
talldan
requested review from
ajitbohra,
chrisvanpatten and
jorgefilipecosta
as code owners
|
Haven't tested it yet, but after a quick look at the changes the code looks great. Amazing work, @talldan! Things I would add for now:
|
talldan
force-pushed
the
try/grid-accessibility-components
branch
from
7c721a2
to
d11e683
Compare
|
This should be ready for review. I've added storybook stories and tests (though unfortunately the react-test-renderer tests can't cover much of the functionality). |
|
I tested the components on Storybook with VoiceOver + Safari. RovingTabindex seems fine, but TreeGrid has a few issues:
|
|
@tellthemachines Good catch! The WAI-ARIA docs may be misleading. They use
Looks like the alternative for grids are |
|
My understanding is that
That they don't seem to be read by voiceover is weird, but the same happens in the examples. I suppose voiceover doesn't support those attributes. |
@diegohaz I was just noticing that 😅 though VoiceOver works pretty well on that example, so maybe it's not the cause of the bugs I described above. I also notice that in the WAI-ARIA example, when navigating with Ctrl+Opt+Arrow keys VoiceOver reads out the position when changing rows or columns (e.g. "column 3 of 3", or "row 1 of 3") for all the cells except the ones that only have links in them. So now I'm wondering if what causes the buggy behaviour in Storybook is the fact that all the cells contain buttons. If so, this is likely a VoiceOver bug. I've tried searching the internets but it's really hard to find info on VO bugs 😩 |
talldan
force-pushed
the
try/grid-accessibility-components
branch
from
d11e683
to
21a3487
Compare
|
Size Change: +725 B (0%) Total Size: 822 kB
ℹ️ View Unchanged
|
|
What will this be used for? |
|
I'm not quite following. Is this for use in the UI or in blocks? |
|
They're components used for managing focus and keyboard interaction, the idea is to use them in the Block Navigator, which is a tree like structure: |
|
Ah I see. It's not to be used for blocks then :) |
|
@gziolo Quite possibly! I'll take a look tomorrow. |
|
In case it helps, I've made this proof of concept of a |
|
@gziolo @diegohaz Hmm, that seems like quite a huge refactor. I'd be open to exploring it as a separate PR, but I'm hesitant to work it into this PR as I've already been through many iterations, and as we all know it's better to iterate and ship rather than iterate and not ship. The components in this are experimental already, so we should be able to rework them afterwards. An option could be to make them unstable in this PR instead of experimental, as I do agree with the idea of using the |
|
As for the status of this PR, the arrow key navigation should work consistently. The only outstanding issue that I know of is that screen readers don't read the row or level positions, and I'm guessing that's because the focusables in the treegrid are buttons that are children of the table cells. When the cells themselves can be focused position seems to be read correctly. This seems to be a shortcoming of the screenreader implementation. In the main PR that uses the TreeGrid (#18014) I've gotten around this by adding descriptions to all the buttons in the grid using
|
|
Merged as part of #18014 |
|
Related to the comments above from @gziolo and @diegohaz It seems like we should avoid introducing stable components right now in Gutenberg (RovingIndex and GridTree) that we may remove/refactor later to rely on reakit. How complex would that refactor be, can we mark them unstable? It's particularly important to not end up with a solution for the BlockNavigator and another one for Alignment. |
|
@youknowriad They should be experimental at the moment, so not stable. |
Yes, it would be awesome to handle it. I think it's confusing that the docs are exposed in the manifest file. Given that, we are using WordPress 5.4 branch it's fine, but we should take care of it one way or another before the 5.5 release cycle starts. We should either refactor to use |
|
@gziolo One of the underlying problems is that if the only option is to not write docs when creating an experimental component, then that becomes a task that has to be done later if the component becomes non-experimental, and it's easy to miss that task. A nice little feature would be if we could add a text snippet like And something more concise for headings greater than first level: |
|
Here's the PR to mark them unstable - #22373 |
Let me clarify it, I never meant to avoid writing docs or stories in the first place 😬 The idea is to improve our tooling to detect experimental and unstable components and never expose them at https://developer.wordpress.org/block-editor/components/. This is where the files are collected: gutenberg/docs/tool/manifest.js Lines 9 to 12 in 7427369 It might be enough, as you mentioned, if we read the first row and detect whether "experimental" or "unstable" keyword is included. Based on that we could filter out some files from the list. |
Description
Extracted from PR #18014
This PR adds new components to the
block-editorpackage for implementing a tree grid as described at WAI ARIA authoring practices - Tree Grid.I've created this as a separate PR as I think the implementation should get some scrutiny.
The new components:
TreeGrid,TreeGridRow,TreeGridCell- implements keyboard navigation as described in the above link. up/down arrow keys navigate between grid rows, left/right arrows navigate between focusable elements in a row.RovingTabIndexa component that wraps a UI control that requires a roving tab index. This component keeps track of the last focused element in the structure it wraps.RovingTabIndexItema component that wraps an individual element/component that is focusable and requires a roving tab index. This component sets the correcttabIndexon the element/component.How has this been tested?
Testing can be carried out on PR #18014. Testing instructions:
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: