Refactor SelectNext into [role=combobox] to improve a11y #1330
Conversation
I don't think this is a "weird behavior" but rather a case where the expected behavior is not clearly defined. A common native form control has several aspects to it which get read out when a screen reader describes it. In the order they are read out, these are:
The issue with When using a combobox however, there is now a clear separation between the value and the label, and both get read out in the expected order. |
My bad. Probably not the best choice of words, as my intention was actually to say it was not the "desired" behavior for the use cases in which we want to use Sorry for the confusion. |
|
This is now ready to review, as I have built this branch and tested it in LMS. With these changes we can go back to use |
| 'w-full flex justify-between', | ||
| className={classnames( | ||
| 'focus-visible-ring transition-colors whitespace-nowrap', | ||
| 'w-full flex items-center justify-between gap-x-2 p-2', |
There was a problem hiding this comment.
These classes were automatically applied by our Button component, but we have moved to the regular button because our own only allows to set aria-label via title.
There was a problem hiding this comment.
The change to be a combobox is good. When looking at this I noticed that the SelectNext documentation structure needs refinement:
- At the top the component is described as a presentational component. I don't think that's really accurate any more. It isn't just styling around a native
<select>. - There is an example near the top that shows what it looks like, but there is no code view
- Underneath that is a "Working with SelectNext" section that references
SelectNext.Optionbut at this point of reading through the page, there has been no explanation or demonstration of what anOptionis or what the code structure looks like.
| @@ -228,6 +246,44 @@ export default function SelectNextPage() { | |||
| </Library.Demo> | |||
| </Library.Example> | |||
|
|
|||
| <Library.Example title="Labelling SelectNext"> | |||
There was a problem hiding this comment.
In US English, "labeling" as one "l" in the middle. See eg. https://www.w3.org/WAI/tutorials/forms/labels/.
| @@ -149,6 +149,9 @@ export type SelectProps<T> = PresentationalProps & { | |||
| */ | |||
| buttonId?: string; | |||
|
|
|||
| 'aria-label'?: string; | |||
| 'aria-labelledby'?: string; | |||
There was a problem hiding this comment.
An issue I noticed when reading the docs, but that I don't think we should try to address in this PR:
The documentation creates some slight confusion here because at the top of the "Component API" section it links to http://localhost:4001/using-components#presentational-components-api, which states that HTML attributes are accepted. For this particular control, those attributes are not available however. I'm not sure that accepting HTML attributes as rest props makes sense because it might be unclear which element they are applied to (the container? the button? the listbox?)
There was a problem hiding this comment.
I think this was a confusion on my part. SelectNextProps extends PresentationalProps, but that type does not mention anything about HTML attributes.
I have noticed though, that all other components extending PresentationalProps also extend some form of JSX.HTMLAttributes<...>, which is not the case for SelectNext.
So I don't know if PresentationalProps should actually implicitly extend JSX.HTMLAttributes and we should have some other type with a different name for what is currently PresentationalProps.
acelaya
force-pushed
the
select-next-combobox
branch
from
7c89d48
to
eaeaf4d
Compare
|
I have created an issue to address the needed documentation improvements #1331 |
Codecov Report
@@ Coverage Diff @@
## main #1330 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 62 62
Lines 926 926
Branches 354 354
=========================================
Hits 926 926
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR refactors the
SelectNextso that the toggle button has[role="combobox"], as we have seen this is the best way to make sure screen readers announce its content and the label linked to it, if any.Without this, we have seen weird behaviors in which a label linked to the button will "replace" the content of the button itself when announced by screen readers, instead of combining both.
Additionally, and in order to make sure
SelectNextis always labelled, this PR addsaria-labelandaria-labelledbyas optional props.This allows for the control to be labelled even in contexts where adding a
<label />is not possible or not desired.In order to test these changes, all examples in the pattern library are now labelled, and there's a new section explaining the different ways to label
SelectNext.