Components: ComboboxControl supports disabled items #61294
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
retrofox
force-pushed
the
update/combobox-control-support-disabled-items
branch
from
237e3fb
to
d06d3f3
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +825 B (+0.05%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d06d3f3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8913331768
|
There was a problem hiding this comment.
@retrofox Thanks for the PR. One accessibility note, you should not be restricting arrow navigation for disabled options. By doing that, you are taking the ability away from keyboard-only users to hear if an option is disabled or not. I see you are already using the aria-disabled attribute on the disabled options, this is good, but users would likely never hear this announced since you are skipping any index that is disabled on arrow key down.
|
Hi, @alexstine. Thanks for your feedback.
Good point. This is something that I've been thinking about, too. However, I followed the behavior of the native element. It isn't possible to select the disabled option by using the keyboard. Anyway, we can make them accessible if we consider this to be the proper behavior. |
|
@retrofox I think at minimum, the option should at least be discoverable even if it is announced as disabled. Thoughts @afercia @joedolson? So many bad examples around the web. I fail to understand how people see the experience as fair or equitable when we make things visible in the UI visually but not readable allowed for VI/blind users. |
💯 sgtm |
The following shows the current style based on
I decided to apply the |
|
Again, will defer to others. To me this is not a design issue, this is a question of: what are the constraints we are reaching for as to how this component should behave in an accessible way. If the gray works, go for it. An alternative is to keep the blue selection style, but instead show a semi-transparent white text-color for the disabled item when it's selected — my instincts would be that this would be less jumpy as you navigate. |
|
Thanks, Joen
I do agree. |
Screen.Recording.2024-05-07.at.09.17.01.mov |
retrofox
force-pushed
the
update/combobox-control-support-disabled-items
branch
from
d32e49e
to
a648fe5
Compare
packages/components/src/combobox-control/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
retrofox
force-pushed
the
update/combobox-control-support-disabled-items
branch
from
a648fe5
to
5cd34a5
Compare
|
In a lot of Combobox examples I see, disabled options do tend to lighten the background as well as the text. I presume this is to create adequate visual contrast between disabled/enabled options. Perhaps it would work a bit better if the background was a lighter shade of the admin theme color rather than gray? 
|
|
Hi, James. Thanks for your input.
Could you share a few UI design Combobox examples where disabled options lighten the background and text? I did the same, and considering there are many options... it doesn't help to define which approach we should take :-( 
|
|
That example is a The example I looked at earlier was this one. There are more here. Generally disabled options don't share a background treatment with enabled ones. Oftentimes disabled options have no hover effect at all, which could also work here imo. |
👍
having no effect, I think, makes sense for |
Yes that's a good point. For sighted keyboard users it does make sense for there to be a background change when an aria-disabled option is highlighted 👍 One small tweak to the mockup above, we might apply 
|
The mouse pointer is not captured by my screenshots 😅 , but it looks similar to what you shared (believe me :-D ) , with an exception: The style is not applied to the disabled option when the mouse hovers. This is because it applies |
Oh is that necessary? I think I'd expect the hover and highlight (via keyboard) visuals to match. I'm still seeing the solid blue background on disabled options: 
For the disabled option background we could potentially use |
This approach, in my view, is not just good but possibly the best way to handle the disabled state. It's a change proposed by our development team and is also a prevalent practice in other libraries like Ariaakit.
👍 |
|
Thanks for your help. Super valuable. Let's wait for the other 👍s! |
Same :-D Merging... |
What?
This PR supports disabled items for the
<ComboboxControl / >component.Why?
Because the
disableoption for a Combobox component is a common requirement when using it for the UI.How?
The
<ComboboxControl />now accepts thedisabledproperty, which is propagated to theSuggestionsListone.The implementation checks if the option is disabled. If so, it does not allow selection in the respective event handler functions.
Let's remark that the disable options are not selectable (by clicking on it or pressing the
enterkey), but they are discoverable and can make the implementation a11y-friendly.Testing Instructions
This PR adds a new story to test the
disableitems:ENTERkeyScreen.Recording.2024-05-03.at.07.09.32.mov
Testing Instructions for Keyboard
Screenshots or screencast