Skip to content

Update id for combobox and listbox #2470

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.

Sign up for GitHub

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

Closed
charliepark wants to merge 2 commits into from
Closed

Update id for combobox and listbox #2470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fb8404
Update id for Listbox and Combobox
charliepark Sep 24, 2024
fcda8ec
Add ariaLabel back in, to reduce VoiceOver complaints
charliepark Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions app/ui/lib/Combobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
} from '@headlessui/react'
import cn from 'classnames'
import { matchSorter } from 'match-sorter'
import { useState } from 'react'
import { useId, useState } from 'react'

import { SelectArrows6Icon } from '@oxide/design-system/icons/react'

Expand Down Expand Up @@ -81,7 +81,7 @@ export const Combobox = ({
})

const zIndex = usePopoverZIndex()

const id = useId()
return (
<>
<HCombobox
Expand All @@ -93,17 +93,18 @@ export const Combobox = ({
disabled={disabled || isLoading}
>
{label && (
// TODO: FieldLabel needs a real ID
<div className="mb-2">
<FieldLabel
id="FieldLabel"
id={`${id}-label`}
Copy link
Collaborator

@david-crespo david-crespo Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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.

as="div"
tip={tooltipText}
optional={!required && !hideOptionalTag}
>
<Label>{label}</Label>
</FieldLabel>
{description && <TextInputHint id="TextInputHint">{description}</TextInputHint>}
{description && (
<TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
Copy link
Collaborator

@david-crespo david-crespo Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

image 
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>
   )
 }

)}
</div>
)}
<ComboboxButton
Expand Down
10 changes: 6 additions & 4 deletions app/ui/lib/Listbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
ListboxOptions,
} from '@headlessui/react'
import cn from 'classnames'
import { type ReactNode, type Ref } from 'react'
import { useId, type ReactNode, type Ref } from 'react'

import { SelectArrows6Icon } from '@oxide/design-system/icons/react'

Expand Down Expand Up @@ -70,7 +70,7 @@ export const Listbox = <Value extends string = string>({
const noItems = !isLoading && items.length === 0
const isDisabled = disabled || noItems
const zIndex = usePopoverZIndex()

const id = useId()
return (
<div className={cn('relative', className)}>
<HListbox
Expand All @@ -86,14 +86,16 @@ export const Listbox = <Value extends string = string>({
{label && (
<div className="mb-2">
<FieldLabel
id={``}
id={`${id}-label`}
as="div"
tip={tooltipText}
optional={!required && !hideOptionalTag}
>
<Label>{label}</Label>
</FieldLabel>
{description && <TextInputHint id={``}>{description}</TextInputHint>}
{description && (
<TextInputHint id={`${id}-help-text`}>{description}</TextInputHint>
)}
</div>
)}
<ListboxButton
Expand Down