-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: Add a WAI-ARIA compliant Combobox. #19657
Conversation
Is this related to the work on LinkControl? Love the use of downshift. |
Could you add before/after visuals? I often review from a mobile device and without visuals it’s much harder to do a design review. |
No, but it can and should be leveraged there.
This PR adds an entirely new component. You need to run the storybook to check all of its states and interactions. |
I was testing this component because we might want to use it in WooCommerce Blocks and found a visual glitch when hovering the arrow button (notice the box shadow in the top-left corner): I could reproduce it with Firefox and Chrome both in Linux. Do you see it too @epiqueras? |
Yeah, good catch! Fixed 😄 |
Thanks for the GIF, that helps a lot. My only point of visual feedback is to somehow connect the results to the input. Right now, the options/results are just kind of "floating" below and feel disconnected. Here's what it could look like it we add a shape around the results/options: I added the placeholder (when the input has focus) which shows the currently selected option before you start typing, along with a visual indicator for the selected option in the list (the checkmark). The highlight on the first option is meant to indicate keyboard focus — using the arrows would move it up/down, and hitting return would select it. I also explored a "connected" variation along with the "disconnected" option shown above: |
It wasn't clear in the GIF, because of the size of the shape, but it already has an outline from the focus styles:
I like that a lot, but is it OK from an a11y standpoint? cc @MarcoZehe
That should/was already working. Something has regressed in Downshift where
I don't have a strong opinion on this. I think I like the connected one more. cc @jasmussen |
@epiqueras can you describe / codesandbox? In the normal useSelect codesandbox selection still works on 4.0.6. https://codesandbox.io/s/useselect-usage-53qfj |
That seems like an OS X system-level focus style. I've only seen it with the current font-size select UI and this new component. It makes me question where the focus actually is in this component; Is the input (where you type) focused, or is the list of options (where you can navigate/select with the keyboard) focused? I think the focus should be on the input. I'd expect the input to get a 2px blue border, and the list of options to have a standard 1px grey border. |
3cab8ea
to
cc3d3eb
Compare
@silviuavram It has to do with how you detect un/controlled modes: https://codesandbox.io/s/useselect-usage-1glc2 It doesn't differentiate between falsy or |
@shaunandrews Focus should move between the two depending on what the user is doing, according to the spec. I think it's working correctly right now, but it needs a proper a11y team review. |
Found it, 3.3.2 - prevState[key] = props[key] === undefined ? state[key] : props[key]
+ prevState[key] = key in props ? props[key] : state[key] We should fix this and test if it's still working with the changes done to fix our useReducer logic. |
Nice catch! Let us know when a new version is released :) |
Hey @epiqueras. As @getdave mentions it'd be great to explore using this for the I don't necessarily want to increase the scope of this PR, but though it'd be worth discussing how they might fit together. One of the fairly unique things about |
In @epiqueras I want to try to keep the focus on the button even when the list is open, in the case of . Focusing the list at open already revealed a problem if you use React portal for the list. Also my colleagues think screen readers will have issues with this scenario as well. I will update you about both the controlled fix and the focus thing. The select should still be accessible even if the focus is kept on the button at all times, it's what I'm aiming for. |
@talldan Yes, the @silviuavram This uses |
menuProps[ 'aria-activedescendant' ].slice( 0, 'downshift-null'.length ) === | ||
'downshift-null' | ||
) { | ||
delete menuProps[ 'aria-activedescendant' ]; |
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.
this looks like it should be a downshift bug.
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.
@epiqueras can you point out to me the use case when this happens? I want to fix it in v5. Thanks!
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.
When mounting in uncontrolled mode.
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 am confused, sorry :)). Do we have an example somewhere?
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.
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.
@epiqueras downshift-js/downshift#945 is this fixing your use case?
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 should, yeah.
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.
Can we remove this code if it's fixed in Downshift?
@silviuaavram the expected behaviour is described in the ARIA spec. Quoting the relevant section:
The current implementation fits into none of these conditions. I realise that this list is not all-inclusive and there might be other valid approaches, but having the empty input state act in one way when the input is newly focused, and in a different way when it is emptied of content, seems buggy. The other thing to consider, in the WP case specifically, is that existing auto-suggests don't have that behaviour. They either show all options by default (Categories in post editor), or they show nothing when the input is empty (Tags in post editor). It would be good to not introduce yet a third behaviour to confuse things further 😅 |
@epiqueras downshift-js/downshift#1058 is for the a11y issue, it should fix it. Will probably be available soon in 5.4.3. As for the menu open it makes sense to implement one of those behaviors by default. I will create an issue on our side, but feel free to use state reducer to apply your specific use case in the meantime. Related to that codesandbox, a couple of comments: no need for initialSelectedItem, since you also fully control the selectedItem. It won't do anything, and is expected. Also the aria-activedescendant bug should be fixed, and you can remove that Thank you for the feedback everyone! |
Thank you for the quick fix!
It's been a while since I looked at the code, but I think the idea is also to support an uncontrolled mode. |
Howdy, y'all! I'm diving in here for an a11y review. I'm by no means an a11y expert, but doing my best to sift through specs and recommendations from people who know more than I do :) Here's my initial take to kick things off:
Hopefully none of these are major wrenches in the system :) I can drill down into a more specific spec code-review after we confirm which spec (or hybrid of specs) we're trying to conform to. To help in this decision, Sarah Higley wrote an excellent article that I think will be enormously helpful for us in deciding our direction: |
Hi @jeryj ! Thanks for your feedback and willingness to dive in and help with a11y work 😺
Currently, 1.2 is in Working Draft status, so it is still subject to change. Though it seems promising, we shouldn't try to implement it until it has reached at least Candidate Recommendation status, which means browsers/AT will start to implement it, and from that point onward, it's unlikely to change much.
This combobox is not meant to behave like a select, because we already have CustomSelectControl for that. The storybook example isn't awfully clear in that respect, because it reproduces the example from
For these reasons, it's better to not show all the options as soon as the input is focused, but to only show filtered options as the user starts typing.
If what you type isn't a valid option, nothing happens. This is expected behaviour; there is no requirement in the specs for this type of component to show an error state. I hope this addresses all your concerns! |
@tellthemachines Thank you for the detailed response! Which Spec to go with?
Even though it's in working draft, because it lines up so much to 1.0 and how OS-level comboboxes work, it appears that the 1.2 proposed spec is more supported out-of-the-box right now than 1.1. From Sarah Higley's article on their testing the editable combobox:
IMO, since hardly any screen readers have supported 1.1, it seems better to go with either 1.0 or the proposed 1.2 spec, since it's already supported better than 1.1 just due to it working more inline with what screen readers expect. My instinct is to go with 1.2 since it appears to be supported by screen readers already. I'm happy to test this with NVDA and VoiceOver and report back to see if this implementation suffers the same results. In the article linked above though, Higley says:
Expected behavior of
|
Thanks again for your feedback @jeryj ! This PR is quite close to a mergeable state; it's been extensively tested with VoiceOver and NVDA, and is working well on both. I'm not too fussed about the spec question, but we haven't tested an implementation based on 1.2, so to go back and rebuild this would require a bunch of extra work that's hard to justify, given that what we have here is working well. As for whether to show all items on open or not, I'd be more inclined to not, for the reasons I already stated, and because it is already possible to show all options by either clicking the button on the right hand side, or pressing Down Arrow. But for that, as well as for your other suggestions, I think the best course would be to create a separate issue for further iteration/discussion. |
To be clear, my intention here wasn't to throw a wrench in the PR. Sorry if it came across that way! I know y'all have done a lot of work and testing to get it here, and that is so appreciated 🙌 I've created all the spin-off issues for further discussion. 👍 |
@epiqueras downshift-js/downshift#1058 has been closed. Should be available in 5.4.3. |
Is it ok if we merge this now and work on polishing it up in follow-ups? This is much better than what's currently broken on |
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 close enough, let's merge what we have and work on further improvements 🚀
From my side, the biggest remaining issues are announcing the number of filtered options and having consistent behaviour when input is empty (whether it's just been opened or the content has been deleted). I'm very reluctant to have all items show on open by default, but we could consider making it an option.
35ed682
to
20b2cbf
Compare
@silviuaavram Those things are fixed in the next version of Downshift, right? |
Seems this component hasn't been exported from the package (#24443)? Is it still WIP? If so, it might be worth removing the docs from the handbook. |
Description
This PR implements a fully accessible and customizable Combobox component with individually style-able items in the
@wordpress/components
package.It uses Downshift, a library that is already used by our custom select control and that will be very useful for any of our other complex a11y input needs.
It has some modifications hooked on top of it to make it fully compliant with the spec. These modifications might soon make it into a future Downshift release, and at that point, we should remove them from our implementations.
How has this been tested?
Test the new component in its Storybook story.
Types of Changes
New Feature:
@wordpress/components
now has a fully WAI-ARIA compliant Combobox.Checklist: