-
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
Add new TreeGrid and RovingTabIndex components #19799
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should just use the aria-
equivalents of these props rather than aliasing them?
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.
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? :) |
81e31fb
to
c8b8ac0
Compare
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:
|
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 😩 |
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? |
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-editor
package 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.RovingTabIndex
a 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.RovingTabIndexItem
a component that wraps an individual element/component that is focusable and requires a roving tab index. This component sets the correcttabIndex
on 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: