Headless UI listbox #1563

benjaminleonard · 2023-06-09T11:01:58Z

The current listbox has issues when used within a modal, it is absolutely positioned and clipped by the parent container. Notably when you open a listbox inside a modal towards the end of a form or in a non-side modal.

We are presently using downshift's useSelect hook. My first attempt was to use the react-aria select and combobox implementation since we are using that lib elsewhere. #1558

This has a few issues, there's a bug that prevents the user from selecting an option after a form has validated or errored that field. We can fix this by adding the error as a key to the component, but not an ideal solution. Also ran into a few other focus errors.

The other little roadblock is that radix breaks scrolling on a listbox since it prevents scrolling outside of the dialog. This seems to be a bug, presently using this workaround.

I then figured we might be able to get the original select working with floating-ui and its FloatingPortal — unfortunately it expects the dropdown to be in the same part of the DOM as the button and get's a little upset if you use a portal.

This brought me to the headless-ui listbox. It doesn't have the portal functionality out of the box, but it had an example using floating-ui and FloatingPortal that I used as a reference.

This does seem to work well and I have the tests passing. Hopefully this is an easier path to getting our present listbox issues resolved. It's much simpler than the implementation on #1558.

benjaminleonard · 2023-06-09T11:06:09Z

app/components/form/fields/ListboxField.tsx

  return (
    <div className={cn('max-w-lg', className)}>
-      <div className="mb-2">
-        <FieldLabel id={`${id}-label`} tip={description} optional={!required}>


Headless UI has it's on aria-labelledby solution but you need to have the label in the same parent component, so I moved this directly into the Listbox and made it optional.

I was going to make a PR for getting aria-describedby into headless UI listbox if this works out. They have it in some other components.

If I put aria-describedby="test" on <Select.Button> or <Select.Options> it passes through fine — is that what you're looking for?

Ok great, I must have missed that

benjaminleonard · 2023-06-09T11:07:24Z

app/test/e2e/firewall-rules.e2e.ts


  // add host filter instance "host-filter-instance"
-  await page.locator('role=button[name="Host type"]').click()
+  await page.locator('role=button[name*="Host type"]').click()


So the accessible name is a combination of the label and contents of the field. Previously I guess it was just the label. This does mean that we need to use * since it's not an exact match.

react-aria does the same thing so I figured it was a reasonable accessible behaviour.

benjaminleonard · 2023-06-09T11:07:51Z

app/test/e2e/image-upload.e2e.ts

 import { expectNotVisible, expectRowVisible, expectVisible } from './utils'

-async function chooseFile(page: Page, size = 5 * MiB) {
+async function chooseFile(page: Page, size = 10 * MiB) {


The upload kept finishing before the test had finished

benjaminleonard · 2023-06-09T11:08:29Z

app/test/e2e/image-upload.e2e.ts

  // states right away we won't catch them in time
  await expectVisible(page, ['role=heading[name="Image upload progress"]'])
-  const done = page.locator('role=dialog >> role=button[name="Done"]')
+  const done = page.locator('css=.ox-modal >> role=button[name="Done"]')


With modal={false} the underlying modal is still seen by playwright, this stops it getting mixed up with the other "done" button.

benjaminleonard · 2023-06-09T11:09:32Z

libs/ui/lib/tooltip/Tooltip.tsx

-      placement: finalPlacement,
-      middlewareData,
-    } = useFloating({
+    const { refs, floatingStyles, context } = useFloating({


Had to update this to the latest useFloating implementation

That's good, I was hoping we could upgrade for the new underlying floating-ui/react-dom API
https://github.com/floating-ui/floating-ui/releases/tag/%40floating-ui%2Freact-dom%402.0.0

oh and it looks like a big improvement. much less noise. nice!

benjaminleonard · 2023-06-09T11:11:05Z

app/components/form/fields/ListboxField.tsx

              }}
              // required to get required error to trigger on blur
-              onBlur={field.onBlur}
+              // onBlur={field.onBlur}


Headless UI doesn't have a onBlur but I'm wondering if it's even necessary?

The present behaviour where it kicks out a required error when you click off seems a bit premature and the user can only select from items in the listbox.

Yeah, that error is annoying anyway.

benjaminleonard · 2023-06-09T11:52:22Z

app/forms/disk-create.tsx

+  const imagesQuery = useApiQuery('imageList', {
+    query: { includeSiloImages: true, ...projectSelector },
+  })
+
+  const images = imagesQuery.data?.items || []

  return (
    <ListboxField
      control={control}
      name="diskSource.imageId"
      label="Source image"
      placeholder="Select an image"
+      isLoading={imagesQuery.isLoading}


Maybe there's a neater way to reuse this — but this is passing query isLoading and then the listbox uses that to decide whether to show a spinner or "No items" since the two both manifest as an empty array

Nope, this is perfect

benjaminleonard · 2023-06-09T11:53:48Z

libs/ui/lib/spinner/Spinner.tsx

  )
 }
+
+export const SpinnerLoader = ({


Fun little component, better version of the one I added into utilization.

Is a spinner that you can set a minimum time it appears for — and if the loading is quick (aka cached) it doesn't show at all.

david-crespo · 2023-06-09T15:34:58Z

This only adds 6kb min / 2kb gzipped so that's nice.

benjaminleonard · 2023-06-09T16:17:56Z

This only adds 6kb min / 2kb gzipped so that's nice.

I should also remove downshift if we're not using it elsewhere

david-crespo · 2023-06-12T22:16:05Z

It's currently only used on MultiSelect:

which is not used anywhere, so we should probably get rid of it. But it won't affect the bundle size because it's already being tree-shaken out.

david-crespo · 2023-06-12T22:22:05Z

libs/ui/lib/listbox/Listbox.tsx

+import { FieldLabel, SpinnerLoader, TextInputHint } from '@oxide/ui'
 import { SelectArrows6Icon } from '@oxide/ui'

 export type ListboxItem = {


wonder if we can do something less weird than labelString since we're not using downshift. looking into it

david-crespo · 2023-06-13T15:49:14Z

app/components/form/fields/DateTimeRangePicker.tsx

-            const newPreset = item.value as RangeKeyAll
-            setPreset(newPreset)
-            onRangeChange(newPreset)
-          }


cast no longer necessary because Listbox understands that the values are RangeKey. if (item) no longer necessary because it's done inside Listbox

david-crespo · 2023-06-13T15:59:39Z

libs/ui/lib/spinner/Spinner.tsx

+      // took longer than the load time.
+      hideTimerRef.current = setTimeout(() => {
+        setIsVisible(false)
+      }, Math.max(0, loadTime - elapsedLoadTime))


This is a lot of fun. Is the logic in the else here meant to prevent the spinner from flashing too briefly, e.g., if the thing we're waiting for takes 27ms, don't let the spinner pop in at 25ms and out after 2ms?

So it'll take the loadTime as a minimum. If it loads in 1000ms you'll see the loader for exactly that long, if it loads in 500ms the loader will still show for a minimum of 750ms. If it loads in 23ms you won't see it at all. A bit of a head scratcher but I think that's the best behaviour.

david-crespo · 2023-06-13T16:02:27Z

libs/ui/lib/modal/Modal.tsx

+      document.body.classList.remove('pointer-events-none')
+    }
+  }, [isOpen])
+


Yikes, what bug is this fixing? Definitely needs a comment.

So this relates to this

The other little roadblock is that radix breaks scrolling on a listbox since it prevents scrolling outside of the dialog. This seems to be a bug, presently using radix-ui/primitives#1159 (comment).

With modal on, the listbox cannot be scrolled. A package that is a little overzealous with preventing mouse events I think.

With modal={false} like they suggest, it seems to work without the pointer events useEffect.

Yeah it might be fine without, I wasn’t sure if it would catch any stray clicks but I suppose the overlay does that

david-crespo · 2023-06-13T20:56:01Z

app/forms/image-upload.tsx

      />
      {file && modalOpen && (
-        <Modal isOpen onDismiss={closeModal}>
-          <Modal.Title>Image upload progress</Modal.Title>


I noticed that Modal takes a title prop that hooks up the aria-labelledby properly. I made a note on my to-do list to look at the other uses of Modal and change them to use the title prop instead of Modal.Title. I'm thinking we shouldn't even export Modal.Title, instead requiring that it be done through the title prop to guarantee the ARIA thing is good.

david-crespo · 2023-06-13T20:57:25Z

libs/ui/lib/modal/Modal.tsx


 export function Modal({ children, onDismiss, title, isOpen }: ModalProps) {
-  const titleId = 'modal-title'
+  const titleId = useId()


This should be unique. Justin pointed me at this, and I changed the existing spots that use our useUuid to useId and deleted useUuid.

david-crespo

Works great for me. Functionality improvements but still net -80, love it. If you wanna do the aria-describedby thing go ahead, otherwise merge away.

benjaminleonard added 3 commits June 8, 2023 18:58

Listbox first pass

b8b3784

Fix tests

980e54e

Make listbox controlled

7d82bd6

benjaminleonard commented Jun 9, 2023

View reviewed changes

benjaminleonard mentioned this pull request Jun 9, 2023

React aria select combobox #1558

Closed

benjaminleonard requested review from david-crespo and just-be-dev June 9, 2023 11:11

Listbox async loading state

b0d1083

benjaminleonard commented Jun 9, 2023

View reviewed changes

Fix test error

3627e91

delete MultiSelect and get rid of downshift

05d42ca

david-crespo reviewed Jun 12, 2023

View reviewed changes

david-crespo added 3 commits June 13, 2023 10:08

centralize null checks for listbox onChange

6546b40

generic listbox value so we can use string unions to our heart's content

ce3beef

clean up some stuff around label and labelString

6cff84a

david-crespo reviewed Jun 13, 2023

View reviewed changes

david-crespo added 3 commits June 13, 2023 12:00

remove 25ms delay from SpinnerLoader

94a2ab6

fix aria label on image progress modal, use it in e2e tests

6903fcb

use useId everywhere instead of useUuid

d0f0e94

david-crespo reviewed Jun 13, 2023

View reviewed changes

don't do the pointer-events-none thing

8e4247b

david-crespo approved these changes Jun 13, 2023

View reviewed changes

link to github discussion on modal={false}

5777f6e

david-crespo enabled auto-merge (squash) June 13, 2023 22:20

david-crespo merged commit 89b58a1 into main Jun 13, 2023

david-crespo deleted the headless-listbox branch June 13, 2023 22:23

david-crespo mentioned this pull request Jun 13, 2023

Only use modal title prop #1572

Merged

benjaminleonard mentioned this pull request Jul 24, 2025

Combobox optimizations #2861

Draft

Headless UI listbox #1563

Headless UI listbox #1563

Uh oh!

Conversation

benjaminleonard commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Jun 9, 2023

Uh oh!

benjaminleonard commented Jun 9, 2023

Uh oh!

david-crespo commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benjaminleonard commented Jun 9, 2023 •

edited

Loading

david-crespo Jun 12, 2023 •

edited

Loading