Update id for combobox and listbox #2470
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| <div className="mb-2"> | ||
| <FieldLabel | ||
| id="FieldLabel" | ||
| id={`${id}-label`} |
There was a problem hiding this comment.
If this doesn't end up getting passed to some other component as aria-labelled-by then it's probably not needed at all, which is nice. I was poking around the HTML to see what's going on and it seems like the Label component from Headless handles this all for you, taking its text contents and making sure they're set as the label for the corresponding input. See the aria-labelledby:
The only obstacle to removing the prop entirely is that it's required on FieldLabel in order to try to help us not forget to do these things in cases where Headless isn't doing it for us. That's probably still desirable, so I'm thinking that rather than making id optional on FieldLabel, the best thing is to leave this as-is but add a comment saying it's not actually used and Headless is taking care of it.
| </FieldLabel> | ||
| {description && <TextInputHint id="TextInputHint">{description}</TextInputHint>} | ||
| {description && ( | ||
| <TextInputHint id={`${id}-help-text`}>{description}</TextInputHint> |
There was a problem hiding this comment.
This one, however, is not being magically handled by Headless. I tried manually passing aria-labelledby and aria-describedby to the ComboboxInput and it doesn't seem to work.
....oh.
https://headlessui.com/react/combobox#adding-a-description
Yep, it works, though you have to wrap the entire pile of Combobox stuff in a <Field>.
diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx
index a9c8a29e..598d4103 100644
--- a/app/ui/lib/Combobox.tsx
+++ b/app/ui/lib/Combobox.tsx
@@ -11,6 +11,8 @@ import {
ComboboxInput,
ComboboxOption,
ComboboxOptions,
+ Description,
+ Field,
Combobox as HCombobox,
Label,
} from '@headlessui/react'
@@ -34,7 +36,6 @@ export type ComboboxBaseProps = {
placeholder?: string
required?: boolean
tooltipText?: string
- ariaLabel?: string
hideOptionalTag?: boolean
/**
* Pass in `allowArbitraryValues` as `true` when the user should be able to
@@ -69,7 +70,6 @@ export const Combobox = ({
onChange,
onInputChange,
allowArbitraryValues = false,
- ariaLabel,
hideOptionalTag,
}: ComboboxProps) => {
const [query, setQuery] = useState(selected || '')
@@ -83,7 +83,7 @@ export const Combobox = ({
const zIndex = usePopoverZIndex()
const id = useId()
return (
- <>
+ <Field>
<HCombobox
value={selected}
// fallback to '' allows clearing field to work
@@ -103,7 +103,9 @@ export const Combobox = ({
<Label>{label}</Label>
</FieldLabel>
{description && (
- <TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
+ <Description>
+ <TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
+ </Description>
)}
</div>
)}
@@ -121,7 +123,6 @@ export const Combobox = ({
)}
>
<ComboboxInput
- aria-label={ariaLabel}
displayValue={() => (selected ? selected : query)}
onChange={(event) => {
setQuery(event.target.value)
@@ -184,6 +185,6 @@ export const Combobox = ({
</ComboboxOptions>
)}
</HCombobox>
- </>
+ </Field>
)
}
|
I moved the work to a new branch / PR: #2492 |
Closes #2417
This follows the pattern for labels and description text in other form fields. I had trouble getting VoiceOver to read out the relevant content when I passed in specific strings for the
ariaLabelprop (not changed in this PR), so I think it would be worth doing a deeper dive down the road. But this at least gets the Listbox and Combobox components to follow a similar pattern as the other form field inputs.