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
|
alexstine
left a comment
alexstine
left a comment
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 |
|
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
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This looks good to me, code-wise.
I've left a few optional suggestions, among which there are a couple that should also satisfy all checks.
I'll let design folks do the final approval since there was some design feedback.
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.
👍 |
|
jameskoster
left a comment
jameskoster
left a comment
There was a problem hiding this comment.
Seems in a good spot to me :)
Thanks for your help. Super valuable. Let's wait for the other 👍s! |
Same :-D Merging... |
|
|
||
| // Define accent color transparent variants. | ||
| $components-color-accent-transparent-40: rgba(var(--wp-components-color-accent--rgb, var(--wp-admin-theme-color--rgb)), 0.04); | ||
|
|
There was a problem hiding this comment.
While reviewing --wp-components-color-* variables, @mirka and I came across this change that was added just before merging the PR and escaped our review.
- the
--wp-components-color-accent--rgbcustom property does not exist (we don't have any--rgbvariants); - the
$components-color-accent-transparent-40SASS variable doesn't need to exist as a common variable, as it can be derived directly where it's needed via thecolor-mix()function - it doesn't work on dark backgrounds (ie dark mode)
We should remove this variable and figure out an alternative solution that only makes use of existing variables and works with all possible theme combinations.
8 participants
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