Virtual list entry for arbitrary values in comboboxes (#2518)

* Update how allowArbitraryValue functions in comboboxes

* Update vertical position, to reduce peek-through of button on lower layer

* update test to work with new approach

* Update tests, though Safari is still a bit touchy

* Select via Enter

* Update test

* Change UX for allowArbitraryOption

* Update test

* More test updating

* new copy: 'Use custom value:'

* gray out "custom:" in virtual item, fix query clearing onClose, e2e test

* use key to nuke combobox on host filter value type change

* explicit useEffect for clearing query whenever value is cleared

---------

Co-authored-by: David Crespo <[email protected]>

Author: charliepark

.eslintrc.cjs

@@ -116,7 +116,7 @@ module.exports = {
       rules: {
         'playwright/expect-expect': [
           'warn',
-          { assertFunctionNames: ['expectVisible', 'expectRowVisible'] },
+          { assertFunctionNames: ['expectVisible', 'expectRowVisible', 'expectOptions'] },
         ],
       },
     },

app/ui/lib/Combobox.tsx

@@ -15,7 +15,7 @@ import {
 } from '@headlessui/react'
 import cn from 'classnames'
 import { matchSorter } from 'match-sorter'
-import { useId, useState, type ReactNode, type Ref } from 'react'
+import { useEffect, useId, useState, type ReactNode, type Ref } from 'react'
 
 import { SelectArrows6Icon } from '@oxide/design-system/icons/react'
 
@@ -97,6 +97,43 @@ export const Combobox = ({
     keys: ['selectedLabel', 'label'],
     sorter: (items) => items, // preserve original order, don't sort by match
   })
+
+  // In the arbitraryValues case, clear the query whenever the value is cleared.
+  // this is necessary, e.g., for the firewall rules form when you submit the
+  // targets subform and clear the field. Two possible changes we might want to make
+  // here if we run into issues:
+  //
+  //   1. do it all the time, not just in the arbitraryValues case
+  //   2. do it on all value changes, not just on clear
+  //
+  // Currently, I don't think there are any arbitraryValues=false cases where we
+  // set the value from outside. There is an arbitraryvalues=true case where we
+  // setValue to something other than empty string, but we don't need the
+  // sync because that setValue is done in onInputChange and we already are
+  // doing setQuery in here along with it.
+  useEffect(() => {
+    if (allowArbitraryValues && !selectedItemValue) {
+      setQuery('')
+    }
+  }, [allowArbitraryValues, selectedItemValue])
+
+  // If the user has typed in a value that isn't in the list,
+  // add it as an option if `allowArbitraryValues` is true
+  if (
+    allowArbitraryValues &&
+    query.length > 0 &&
+    !filteredItems.some((i) => i.selectedLabel === query)
+  ) {
+    filteredItems.push({
+      value: query,
+      label: (
+        <>
+          <span className="text-secondary">Custom:</span> {query}
+        </>
+      ),
+      selectedLabel: query,
+    })
+  }
   const zIndex = usePopoverZIndex()
   const id = useId()
   return (
@@ -106,7 +143,8 @@ export const Combobox = ({
         value={selectedItemValue}
         // fallback to '' allows clearing field to work
         onChange={(val) => onChange(val || '')}
-        onClose={() => setQuery('')}
+        // we only want to keep the query on close when arbitrary values are allowed
+        onClose={allowArbitraryValues ? undefined : () => setQuery('')}
         disabled={disabled || isLoading}
         immediate
         {...props}
@@ -177,18 +215,13 @@ export const Combobox = ({
             </ComboboxButton>
           )}
         </div>
-        {items.length > 0 && (
+        {(items.length > 0 || allowArbitraryValues) && (
           <ComboboxOptions
             anchor="bottom start"
             // 14px gap is presumably because it's measured from inside the outline or something
-            className={`ox-menu pointer-events-auto ${zIndex} relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border !outline-none border-secondary [--anchor-gap:14px] empty:hidden`}
+            className={`ox-menu pointer-events-auto ${zIndex} relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border !outline-none border-secondary [--anchor-gap:13px] empty:hidden`}
             modal={false}
           >
-            {!allowArbitraryValues && filteredItems.length === 0 && (
-              <ComboboxOption disabled value="no-matches" className="relative">
-                <div className="ox-menu-item !text-disabled">No items match</div>
-              </ComboboxOption>
-            )}
             {filteredItems.map((item) => (
               <ComboboxOption
                 key={item.value}
@@ -202,7 +235,7 @@ export const Combobox = ({
                   // of those rules one by one. Better to rely on the shared classes.
                   <div
                     className={cn('ox-menu-item', {
-                      'is-selected': selected,
+                      'is-selected': selected && query !== item.value,
                       'is-highlighted': focus,
                     })}
                   >
@@ -211,6 +244,11 @@ export const Combobox = ({
                 )}
               </ComboboxOption>
             ))}
+            {!allowArbitraryValues && filteredItems.length === 0 && (
+              <ComboboxOption disabled value="no-matches" className="relative">
+                <div className="ox-menu-item !text-disabled">No items match</div>
+              </ComboboxOption>
+            )}
           </ComboboxOptions>
         )}
       </HCombobox>

test/e2e/firewall-rules.e2e.ts

@@ -6,14 +6,9 @@
  * Copyright Oxide Computer Company
  */
 
-import {
-  clickRowAction,
-  expect,
-  expectRowVisible,
-  selectOption,
-  test,
-  type Locator,
-} from './utils'
+import { expect, test, type Locator, type Page } from '@playwright/test'
+
+import { clickRowAction, expectRowVisible, selectOption } from './utils'
 
 const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp']
 
@@ -53,6 +48,7 @@ test('can create firewall rule', async ({ page }) => {
   // add host filter instance "host-filter-instance"
   await selectOption(page, 'Host type', 'Instance')
   await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance')
+  await page.getByText('host-filter-instance').click()
   await page.getByRole('button', { name: 'Add host filter' }).click()
 
   // host is added to hosts table
@@ -167,11 +163,13 @@ test('firewall rule form targets table', async ({ page }) => {
   // add targets with overlapping names and types to test delete
 
   await targetVpcNameField.fill('abc')
+  await targetVpcNameField.press('Enter')
   await addButton.click()
   await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' })
 
   // enter a VPC called 'mock-subnet', even if that doesn't make sense here, to test dropdown later
   await targetVpcNameField.fill('mock-subnet')
+  await targetVpcNameField.press('Enter')
   await addButton.click()
   await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-subnet' })
 
@@ -188,6 +186,7 @@ test('firewall rule form targets table', async ({ page }) => {
   // now add a subnet by entering text
   await selectOption(page, 'Target type', 'VPC subnet')
   await subnetNameField.fill('abc')
+  await page.getByText('abc').first().click()
   await addButton.click()
   await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' })
 
@@ -221,6 +220,7 @@ test('firewall rule form target validation', async ({ page }) => {
   // Enter invalid VPC name
   const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first()
   await vpcNameField.fill('ab-')
+  await vpcNameField.press('Enter')
   await addButton.click()
   await expect(nameError).toBeVisible()
 
@@ -246,6 +246,7 @@ test('firewall rule form target validation', async ({ page }) => {
   await expect(ipError).toBeHidden()
   await expect(nameError).toBeHidden()
   await vpcNameField.fill('abc')
+  await page.getByText('abc').click()
   await addButton.click()
   await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' })
 
@@ -284,6 +285,7 @@ test('firewall rule form host validation', async ({ page }) => {
   // Enter invalid VPC name
   const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1)
   await vpcNameField.fill('ab-')
+  await vpcNameField.press('Enter')
   await addButton.click()
   await expect(nameError).toBeVisible()
 
@@ -309,6 +311,7 @@ test('firewall rule form host validation', async ({ page }) => {
   await expect(ipError).toBeHidden()
   await expect(nameError).toBeHidden()
   await vpcNameField.fill('abc')
+  await page.getByText('abc').click()
   await addButton.click()
   await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })
 
@@ -350,10 +353,12 @@ test('firewall rule form hosts table', async ({ page }) => {
   // add hosts with overlapping names and types to test delete
 
   await hostFiltersVpcNameField.fill('abc')
+  await hostFiltersVpcNameField.press('Enter')
   await addButton.click()
   await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })
 
   await hostFiltersVpcNameField.fill('def')
+  await hostFiltersVpcNameField.press('Enter')
   await addButton.click()
   await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' })
 
@@ -364,6 +369,7 @@ test('firewall rule form hosts table', async ({ page }) => {
 
   await selectOption(page, 'Host type', 'VPC subnet')
   await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc')
+  await page.getByRole('combobox', { name: 'Subnet name' }).press('Enter')
   await addButton.click()
   await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' })
 
@@ -427,6 +433,7 @@ test('can update firewall rule', async ({ page }) => {
   // add host filter
   await selectOption(page, 'Host type', 'VPC subnet')
   await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet')
+  await page.getByText('edit-filter-subnet').click()
   await page.getByRole('button', { name: 'Add host filter' }).click()
 
   // new host is added to hosts table
@@ -561,3 +568,49 @@ test('name conflict error on edit', async ({ page }) => {
   await page.getByRole('button', { name: 'Update rule' }).click()
   await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' })
 })
+
+async function expectOptions(page: Page, options: string[]) {
+  const selector = page.getByRole('option')
+  await expect(selector).toHaveCount(options.length)
+  for (const option of options) {
+    await expect(page.getByRole('option', { name: option })).toBeVisible()
+  }
+}
+
+test('arbitrary values combobox', async ({ page }) => {
+  await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')
+
+  // test for bug where we'd persist the d after add and only show 'Custom: d'
+  const vpcInput = page.getByRole('combobox', { name: 'VPC name' }).first()
+  await vpcInput.focus()
+  await expectOptions(page, ['mock-vpc'])
+
+  await vpcInput.fill('d')
+  await expectOptions(page, ['Custom: d'])
+
+  await vpcInput.blur()
+  page.getByRole('button', { name: 'Add target' }).click()
+  await expect(vpcInput).toHaveValue('')
+
+  await vpcInput.focus()
+  await expectOptions(page, ['mock-vpc']) // bug cause failure here
+
+  // test keeping query around on blur
+  await selectOption(page, 'Target type', 'Instance')
+  const input = page.getByRole('combobox', { name: 'Instance name' })
+
+  await input.focus()
+  await expectOptions(page, ['db1', 'you-fail', 'not-there-yet'])
+
+  await input.fill('d')
+  await expectOptions(page, ['db1', 'Custom: d'])
+
+  await input.blur()
+  await expect(page.getByRole('option')).toBeHidden()
+
+  await expect(input).toHaveValue('d')
+  await input.focus()
+
+  // same options show up after blur (there was a bug around this)
+  await expectOptions(page, ['db1', 'Custom: d'])
+})