Use composite pattern to improve keyboard navigation on the inserter #23610
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
force-pushed
the
update/use-composite-in-inserter
branch
from
afc0fad to
e5bc4ec
Compare
|
Size Change: +9.05 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @youknowriad! :)
Made some suggestions (that will probably fix CI as well). The code looks good and https://gutenberg.run/23610 is working great! As you said, it should be better with the grid pattern, but that would require more work, and I agree that a listbox is already an improvement over what we have on master.
| <ul role="list" className="block-editor-block-types-list"> | ||
| <Composite | ||
| as="ul" | ||
| role="list" |
There was a problem hiding this comment.
list is not a valid composite role. Screen reader users won't have a hint that they should use arrow keys to navigate through the items, so this would be probably less accessible for some of them. But we can use listbox here.
| role="list" | |
| role="listbox" |
Also, this components requires either aria-label or aria-labelledby prop. Since the category title element is already in the document, the best option is to pass its ID to the aria-labelledby prop here.
There was a problem hiding this comment.
I've used an aria-label as the alternative would have required a bigger refactor (separate components)
packages/block-editor/src/components/inserter-list-item/index.js
Outdated
Show resolved
Hide resolved
|
Found a bug:
This is better explained here. Re-mounting (updating the There's another solution here, but it's still unstable, and for some reason it's not working well on Firefox yet. |
|
Thanks for the feedback @diegohaz I pushed some tweaks, let me know what you think. |
| /** | ||
| * External dependencies | ||
| */ | ||
| import { Composite, useCompositeState } from 'reakit'; |
There was a problem hiding this comment.
It adds nearly 10kB to the module.
There was a problem hiding this comment.
yes, I was wondering about that. the solution could be to make it part of the @wordpress/components API but then it becomes something we need to ensure BC for. Always a struggle to decide what's best in these situations.
There was a problem hiding this comment.
Let's improve the experience first and see how the performance might get improved later then.
There was a problem hiding this comment.
I'm hoping we can balance these kbits loss by refactoring some of our components to rely on reakit like NavigableToolbar/NavigableContainer...
There was a problem hiding this comment.
Yes, it's essentially what Composite does 👍
|
|
||
| return ( | ||
| /* | ||
| * Disable reason: The `list` ARIA role is redundant but | ||
| * Safari+VoiceOver won't announce the list otherwise. | ||
| */ | ||
| /* eslint-disable jsx-a11y/no-redundant-roles */ | ||
| <ul role="list" className="block-editor-block-types-list"> | ||
| <Composite |
There was a problem hiding this comment.
Do you think we should expose Composite or maybe create a Listbox component in @wordpress/components? We can live with 10kB extra for now, but I'm sure there is going to be more cases where Reakit can be useful :)
|
So the e2e tests are failing here and here's what I see:
For whatever reason, the changes on this PR prevent the popover from closing. That said, this is the behavior I see when doing @diegohaz Do you have any idea what might cause this? Do we prevent event bubbling anywhere in composite components (blur events specifically). Thanks. |
It shouldn't. I'll take a look into that. |
youknowriad
force-pushed
the
update/use-composite-in-inserter
branch
from
a0ae203 to
c682202
Compare
There was a problem hiding this comment.
After some research, I learned that, according to the WAI-ARIA spec, interactive roles like button aren't allowed inside option elements:
the interaction model conveyed by the listbox role to assistive technologies does not support interacting with elements inside of an option
Having a button inside of an option doesn't seem to affect anything when using a screen reader though. In fact, the button role is just ignored and only its text content is announced. But in order to have a valid accessibility tree, we would have to change the structure to something like this:
<Composite role="listbox"> {/* not a ul */}
<div className="block-editor-block-types-list__list-item"> {/* not an li, no role="option" */}
<CompositeItem as={Button} role="option" />
</div>
</Composite>Alternatively, as the spec suggests, we could use the grid pattern, but this would require additional work with styling here.
Regarding the e2e test failure, it seems that it's happening because of the key={ orderId }. But it looks like the list of blocks aren't being re-ordered anymore. Is this correct? If so, key isn't needed anymore.
They are if you enable "most used blocks" in the editor options. |
|
Just pushed 506de60. I've added the HTML structural changes that I'd proposed, and also a different approach to re-order items using the Tried to run e2e tests locally, but got some errors. I think it might be some configuration here, but let's see. Feel free to revert it if it doesn't work out. |
|
Oh great! Thanks for the update Diego, It looks great. It seems that one of the end2end tests is fixed. I'll be looking at the last failure a bit later. |
|
The tests should be fixed now 🎉 |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
I'm not sure listbox is an appropriate pattern. Re: the grid/gridcell pattern, as far as I can tell actual support is very scarce. |
|
@afercia Is a |
|
I don't know. This needs some more thinking and research. Aside: I'd really like the grid/gridcell pattern was better supported. It is possible that things have changed with recent progress of assistive technologies. I'd greatly appreciate some help in testing the grid/gridcell pattern across different combinations of browsers and screen readers. The grid/gridcell pattern would greatly help (if it worked) also in the Media views "grid", where we're looking for a decent, accessible, solution since a few years... |
|
While I'm working on this I think that the inserter would benefit from the grid pattern as I suggested on #21080 (comment), but I've also seen some pushback in the community on using the grid pattern on things that aren't tabular data. Most notoriously, this recent article (ARIA Grid As an Anti-Pattern), where the author arguments against the use of the layout grid pattern present in the Authoring Practices Guide. In the comments, we've discussed about how we would communicate to screen reader users this kind of composite interaction without using a composite role like I still think that the grid pattern plus a visual instruction for keyboard-only users is the safest option here based on the design we currently have. But, ultimately, this whole discussion seems very opinion based and lacks real user feedback. I wonder if we couldn't make it easier for users to give feedback on accessibility directly from the Gutenberg interface. |
|
@diegohaz thanks for pointing me to yet another interesting article from Adrian Roselli and the conversation in the comments there. I see it's a case about data-grids vs. layout-grids To me, the definition in the ARIA spec is a bit confusing in the first place, to start with. While it states the Specifically, I'm not sure I understand how a collection of checkboxes may represent "information whose structure is characterized by a two-dimensional relationship". A collection of checkboxes is just a group of logically related items. Not sure how it could fit in a two-dimensional relationship model. That said, I'm not that interested in a high-level academic discussion and I'm more interested in problem solving 🙂 I'd argue that Adrian recommends to use other mechanisms instead of grid such as:
Which is exactly was what removed from the Inserter 🙂 It used to have accordions. Now they're gone. Now there are tons of tab stops to go through the groups of blocks. I'd argue that also for mouse users, having to scroll through a list of more than 70 blocks isn't ideal. And plugins can add more block types to the default ones, making the list even longer. In my opinion, this part of the design change isn't a great improvement, to be fair. Moving on, I'd like we make an effort to consider the introduction of new patterns in WordPress more holistically. WordPress is not just the block editor. When introducing a new pattern, we should make sure it's good also for other existing use cases in the admin. I think we all agree it's best to provide users with consistent, predictable, patterns instead of forcing them to learn a new, different, pattern each time. In WordPress there's at least one more case where we still have to solve the two-dimensional navigation issue. I'd call it "spatial navigation" for the sake of brevity and because I liked the old Opera browser versions, where spatial navigation was a remarkable feature. So how do we enable keyboard and screen reader users to use spatial navigation in the block editor inserter and in the Media library "grid"? Screenshot for reference:
The Media grid does have a "spatial navigation" mechanism but it has some issues. It always worked this way and we haven't been able to fix it in the last 7 (maybe 8?) years.
However, the spatial navigation does not work for screen reader users. At least, doesn't work by default and isn't communciated in any way. Worth reminding that Windows screen readers have to main "modes" (their names but that's not relevant). In "browse mode", key presses are not passed to the browser. Spatial navigation won't work. In "focus mode" key presses are passed to the browser. There are also some ARIA roles that trigger a sort of focus mode that would make spatial navigation work. Same applies to the Inserter: spatial navigation won't work by default for Windows screen reader users. Users can manually switch between modes but they would need to know they have to do that to be able to navigate spatially. Ideally, the switch should be automatic, as screen readers do when they encounter forms or some specific ARIA roles. In short: for both the Inserter and the Media grid we need an ARIA role to trigger the switch and to make screen readers pass key presses to the browser. If the grid/gridcell is not appropriate, then I'm totally fine with not using it 🙂 However, given we can't change the design, we still need an appropriate ARIA role, excluding I don't know what other ARIA role would be appropriate in our two cases. I'm also wondering if this is a case where a "layout-grid" would be acceptable. Pinging @aardrian to bring him into the conversation 👋 (when you have a chance!) There's also one more thing that concerns me: excessive use of ARIA. We should really strive to keep things simple instead of inventing complex UIs and then try to remediate with ARIA to make them accessible. Also, and not a minor concern, we need to remind ourselves that some ARIA patterns aren't well known. Many screen reader users don't get trained in using uncommon ARIA widgets, and some of these patterns are rarely taught. Aside: in my testing: |
…23610) Co-authored-by: Haz <[email protected]>
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
|
Coming in way late (too late)... From @afercia:
I was.
This would be a custom widget, likely a combination of other patterns (for example, the emoji picker referenced in comments on my post) such as modals, accordions, tabs, etc. I encourage you to read Sarah Higley's post Grids Part 1: To grid or not to grid and then test with real users. |
|
As I understand, the biggest issue with not using a composite role such as grid here is that screen readers will not automatically change to focus mode. Given the current design, I think it's still worth giving grid a try (with a visual hint for keyboard-only users) and testing with real users. |
Changing modes is one problem. Since grids are by default interactive you need to make a read-only grid (if I understand your use case correctly), and then you as the author need to handle all keyboard interaction. A challenge is when screen readers do not behave as expected by users nor consistently across pairings.
By all means, give it a shot. Test it with real users (real AT users), but don't deploy anything until you do. Also, to conform to 3.3.2 Labels or Instructions your visual hint cannot just be for sighted keyboard users. Touch, voice, and even SR users will need those instructions. Until the pattern is established (outside of Wordpress, not just in this UI given the constant onboarding of new authors) I recommend it is always visible. |
The idea is to tab directly between categories and use arrow keys to navigate inside a given category. We could consider "Grid" navigation but I think that's already an improvement over master.
That said, I don't what I'm doing with reakit :) mostly trying to follow the docs, let me know what you think :)