diff --git a/app/components/form/Subform.browser.spec.tsx b/app/components/form/Subform.browser.spec.tsx
new file mode 100644
index 0000000000..4e3a64ef13
--- /dev/null
+++ b/app/components/form/Subform.browser.spec.tsx
@@ -0,0 +1,132 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * Copyright Oxide Computer Company
+ */
+import { useState } from 'react'
+import { afterEach, beforeEach, expect, test, vi } from 'vitest'
+import { render, type RenderResult } from 'vitest-browser-react'
+import { commands, userEvent } from 'vitest/browser'
+
+import { Combobox, toComboboxItems } from '~/ui/lib/Combobox'
+
+import { Subform } from './Subform'
+
+// Combobox's onClose behaves differently when the document is unfocused, and
+// the test iframe's focus state is nondeterministic. See Combobox.browser.spec.tsx.
+beforeEach(() => {
+ vi.spyOn(document, 'hasFocus').mockReturnValue(true)
+})
+
+afterEach(() => {
+ vi.restoreAllMocks()
+})
+
+const items = toComboboxItems([{ name: 'disk-3' }, { name: 'disk-4' }])
+
+// Filling alone doesn't reliably leave the first option highlighted (Firefox
+// CI pressed Enter with nothing active), so open the list and Home to it
+// explicitly, like Combobox.browser.spec.tsx does
+async function highlightFirstOption(screen: RenderResult, label: string) {
+ const combobox = screen.getByRole('combobox', { name: label })
+ await combobox.fill('disk')
+ await commands.pressComboboxKey(label, 'ArrowDown')
+ const option = screen.getByRole('option', { name: 'disk-3' })
+ await expect.element(option).toBeVisible()
+ await commands.pressComboboxKey(label, 'Home')
+ await expect
+ .element(combobox)
+ .toHaveAttribute('aria-activedescendant', option.element().id)
+}
+
+function Harness() {
+ const [inner, setInner] = useState('')
+ const [outer, setOuter] = useState('')
+ const [formSubmits, setFormSubmits] = useState(0)
+ const [subformSubmits, setSubformSubmits] = useState(0)
+
+ return (
+
+ )
+}
+
+test('Enter selects while open, then submits the subform once closed', async () => {
+ const screen = await render()
+
+ await highlightFirstOption(screen, 'Inner disk')
+ await commands.pressComboboxKey('Inner disk', 'Enter')
+ await expect.element(screen.getByText('Inner: disk-3')).toBeVisible()
+ await expect.element(screen.getByText('Subform submits: 0')).toBeVisible()
+
+ await commands.pressComboboxKey('Inner disk', 'Enter')
+ await expect.element(screen.getByText('Subform submits: 1')).toBeVisible()
+ await expect.element(screen.getByText('Form submits: 0')).toBeVisible()
+})
+
+test('Enter in a text input submits the subform', async () => {
+ const screen = await render()
+
+ await screen.getByRole('textbox', { name: 'Port' }).fill('80')
+ await userEvent.keyboard('{Enter}')
+ await expect.element(screen.getByText('Subform submits: 1')).toBeVisible()
+ await expect.element(screen.getByText('Form submits: 0')).toBeVisible()
+})
+
+test('Enter on the Add button submits the subform once', async () => {
+ const screen = await render()
+
+ const button = screen.getByRole('button', { name: 'Add' })
+ // focus directly because WebKit doesn't Tab to buttons
+ button.element().focus()
+ await expect.element(button).toHaveFocus()
+ await userEvent.keyboard('{Enter}')
+ await expect.element(screen.getByText('Subform submits: 1')).toBeVisible()
+ await expect.element(screen.getByText('Form submits: 0')).toBeVisible()
+})
+
+test('outside a subform, Enter in a closed combobox submits the form', async () => {
+ const screen = await render()
+
+ await highlightFirstOption(screen, 'Outer disk')
+ await commands.pressComboboxKey('Outer disk', 'Enter')
+ await expect.element(screen.getByText('Outer: disk-3')).toBeVisible()
+ await expect.element(screen.getByText('Form submits: 0')).toBeVisible()
+
+ await commands.pressComboboxKey('Outer disk', 'Enter')
+ await expect.element(screen.getByText('Form submits: 1')).toBeVisible()
+ await expect.element(screen.getByText('Subform submits: 0')).toBeVisible()
+})
diff --git a/app/components/form/Subform.tsx b/app/components/form/Subform.tsx
new file mode 100644
index 0000000000..72d720046c
--- /dev/null
+++ b/app/components/form/Subform.tsx
@@ -0,0 +1,75 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * Copyright Oxide Computer Company
+ */
+import cn from 'classnames'
+import type { ReactNode } from 'react'
+
+import { Button } from '~/ui/lib/Button'
+import { KEYS } from '~/ui/util/keys'
+
+type SubformProps = {
+ /** Called on Enter in one of the subform's inputs and on the Add button */
+ onSubmit: () => void
+ onClear: () => void
+ addButtonLabel: string
+ /** Disables the Clear and Add buttons */
+ disabled: boolean
+ className?: string
+ children: ReactNode
+}
+
+/**
+ * Container for the fields of a subform: a nested `useForm` rendered inside an
+ * outer form, like the firewall rule targets or webhook secrets, followed by
+ * Clear and Add buttons for adding the result to a list. A subform is
+ * not a DOM form, so Enter in one of its inputs would submit the outer form.
+ * This catches the Enter as it bubbles and submits the subform instead, so the
+ * fields themselves don't need any Enter handling.
+ *
+ * A combobox with its options open handles Enter itself (selecting the
+ * highlighted option) and stops propagation, so that Enter never gets here.
+ */
+export function Subform({
+ onSubmit,
+ onClear,
+ addButtonLabel,
+ disabled,
+ className,
+ children,
+}: SubformProps) {
+ return (
+ // The div only listens for Enter bubbling up from the inputs inside it
+ // oxlint-disable-next-line jsx-a11y/no-static-element-interactions
+
{
+ if (
+ e.key === KEYS.enter &&
+ // a field already handled this Enter
+ !e.defaultPrevented &&
+ // Enter that commits an IME composition isn't a submit
+ !e.nativeEvent.isComposing &&
+ // buttons (Add, Clear, listbox triggers) handle Enter themselves
+ e.target instanceof HTMLInputElement
+ ) {
+ e.preventDefault() // prevent full form submission
+ onSubmit()
+ }
+ }}
+ >
+
{children}
+
+
+
+
+
+ )
+}
diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index b597bd786d..5c3524ed0f 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -35,6 +35,7 @@ import { NameField, validateName } from '~/components/form/fields/NameField'
import { NumberField } from '~/components/form/fields/NumberField'
import { RadioField } from '~/components/form/fields/RadioField'
import { TextField, TextFieldInner } from '~/components/form/fields/TextField'
+import { Subform } from '~/components/form/Subform'
import { useVpcSelector } from '~/hooks/use-params'
import {
ProtocolCell,
@@ -45,10 +46,9 @@ import { toComboboxItems } from '~/ui/lib/Combobox'
import { FormDivider } from '~/ui/lib/Divider'
import { FieldLabel } from '~/ui/lib/FieldLabel'
import { Message } from '~/ui/lib/Message'
-import { ClearAndAddButtons, MiniTable } from '~/ui/lib/MiniTable'
+import { MiniTable } from '~/ui/lib/MiniTable'
import { SideModal } from '~/ui/lib/SideModal'
import { TextInputHint } from '~/ui/lib/TextInput'
-import { KEYS } from '~/ui/util/keys'
import { ALL_ISH } from '~/util/consts'
import { validateIp, validateIpNet } from '~/util/ip'
import { links } from '~/util/links'
@@ -172,59 +172,53 @@ const TargetAndHostFilterSubform = ({
{nounTitle}
-
- {/* In the firewall rules form, a few types get comboboxes instead of text fields */}
- {valueType === 'vpc' || valueType === 'subnet' || valueType === 'instance' ? (
- subform.reset()}
+ addButtonLabel={`Add ${noun}`}
+ disabled={!value}
+ >
+
- // required: false arg is desirable because otherwise if you put in
- // a bad name and submit, causing it to revalidate onChange, then
- // clear the field you're left with a BS "Target name is required"
- // error message
- validateName(value, `${capitalize(sectionType)} name`, false)
- }
/>
- ) : (
- {
- if (e.key === KEYS.enter) {
- e.preventDefault() // prevent full form submission
- submitSubform(e)
+ {/* In the firewall rules form, a few types get comboboxes instead of text fields */}
+ {valueType === 'vpc' || valueType === 'subnet' || valueType === 'instance' ? (
+
+ // required: false arg is desirable because otherwise if you put in
+ // a bad name and submit, causing it to revalidate onChange, then
+ // clear the field you're left with a BS "Target name is required"
+ // error message
+ validateName(value, `${capitalize(sectionType)} name`, false)
}
- }}
- validate={(value) =>
- (valueType === 'ip' && validateIp(value)) ||
- (valueType === 'ip_net' && validateIpNet(value)) ||
- undefined
- }
- />
- )}
- subform.reset()}
- onSubmit={submitSubform}
- />
+ />
+ ) : (
+
+ (valueType === 'ip' && validateIp(value)) ||
+ (valueType === 'ip_net' && validateIpNet(value)) ||
+ undefined
+ }
+ />
+ )}
+
})
return (
<>
-
-
- {
- protocolForm.setValue('icmpType', '')
- protocolForm.setValue('icmpCode', '')
- protocolForm.clearErrors(['icmpType', 'icmpCode'])
- }}
- />
+ protocolForm.reset()}
+ addButtonLabel="Add protocol filter"
+ disabled={!selectedProtocolType}
+ >
+ {
+ protocolForm.setValue('icmpType', '')
+ protocolForm.setValue('icmpCode', '')
+ protocolForm.clearErrors(['icmpType', 'icmpCode'])
+ }}
+ />
- {(selectedProtocolType === 'icmp' || selectedProtocolType === 'icmp6') && (
- <>
-
+ {
+ const result = parseIcmpType(value)
+ if (!result.success) return result.message
+ }}
+ />
+
+ {selectedIcmpType !== undefined && selectedIcmpType !== '' && (
+
+ Enter a code (0) or range (e.g., 1–3). Leave blank for all traffic
+ of type {selectedIcmpType}.
+ >
+ }
placeholder=""
- allowArbitraryValues
- items={icmpTypeItems[selectedProtocolType]}
- validate={(value) => {
- const result = parseIcmpType(value)
- if (!result.success) return result.message
- }}
+ validate={icmpCodeValidation}
+ transform={normalizeDashes}
/>
-
- {selectedIcmpType !== undefined && selectedIcmpType !== '' && (
-
- Enter a code (0) or range (e.g., 1–3). Leave blank for all
- traffic of type {selectedIcmpType}.
- >
- }
- placeholder=""
- validate={icmpCodeValidation}
- transform={normalizeDashes}
- onKeyDown={(e) => {
- if (e.key === KEYS.enter) {
- e.preventDefault() // prevent full form submission
- submitProtocol(e)
- }
- }}
- />
- )}
- >
- )}
-
+ portRangeForm.reset()}
+ addButtonLabel="Add port filter"
+ disabled={!portValue}
+ >
{/* We have to blow this up instead of using TextField to get better
text styling on the label */}
@@ -692,25 +681,13 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
name="portRange"
required
control={portRangeForm.control}
- onKeyDown={(e) => {
- if (e.key === KEYS.enter) {
- e.preventDefault() // prevent full form submission
- submitPortRange(e)
- }
- }}
validate={(value) => {
if (!parsePortRange(value)) return 'Not a valid port range'
if (ports.value.includes(value.trim())) return 'Port range already added'
}}
/>
- {/* We have to blow this up instead of using TextField for better layout control of field and ClearAndAddButtons */}
+ transitIpsForm.reset()}
+ addButtonLabel="Add Transit IP"
+ disabled={!transitIpValue}
+ >
+ {/* We have to blow this up instead of using TextField for better layout control of field and Clear/Add buttons */}
Transit IPs
@@ -157,12 +162,6 @@ export function EditNetworkInterfaceForm({
id="transitIp"
name="transitIp"
control={transitIpsForm.control}
- onKeyDown={(e) => {
- if (e.key === KEYS.enter) {
- e.preventDefault() // prevent full form submission
- submitTransitIp()
- }
- }}
validate={(value) => {
const error = validateIpNet(value)
if (error) return error
@@ -181,13 +180,7 @@ export function EditNetworkInterfaceForm({
placeholder={`An IP network, e.g., ${exampleIPs}`}
/>
+
void
- /** Fires whenever the Enter key is pressed while Combobox input has focus */
- onEnter?: (event: React.KeyboardEvent) => void
/**
* Optional function to transform the value entered into the input as the user types.
*/
@@ -101,7 +99,6 @@ export const Combobox = ({
disabled,
isLoading,
onChange,
- onEnter,
onInputChange,
allowArbitraryValues = false,
renderArbitraryLabel,
@@ -180,17 +177,6 @@ export const Combobox = ({
const zIndex = usePopoverZIndex()
const id = useId()
- // Tracks whether the dropdown is open so the onKeyDown handler can
- // distinguish Enter-to-select (dropdown open, let HUI handle it) from
- // Enter-to-submit (dropdown closed, fire onEnter). We use a ref instead
- // of HUI's `open` render prop because our handler runs before HUI's
- // (useRender merges user props first) and the render prop can be stale.
- // The ref stays current because onClose sets it synchronously during
- // HUI's own keydown handler. With the stale render prop, the handler
- // could see the combobox as closed one keydown too late, firing onEnter
- // instead of letting HUI select — hard to notice manually but caused
- // consistent e2e flakes in Firefox.
- const isOpenRef = useRef(false)
return (
{
- isOpenRef.current = false
// A window or tab switch also closes the combobox. Keep the active query
// in that case so it is still visible when the user returns.
if (!document.hasFocus()) return
@@ -213,143 +198,126 @@ export const Combobox = ({
immediate
virtual={{ options: virtualOptions, disabled: isNoMatch }}
>
- {({ open }) => {
- // Sync open state to ref on render (handles the opening side)
- if (open) isOpenRef.current = true
- return (
-
- {label && (
- // TODO: FieldLabel needs a real ID
-
- {(items.length > 0 || allowArbitraryValues) && virtualOptions.length > 0 && (
-
- {({ option }: { option: ComboboxItem }) => {
- const noMatch = option === NO_MATCH_ITEM
- return (
- (
+ // This *could* be done with data-[focus] and data-[selected] instead, but
+ // it would be a lot more verbose. those can only be used with TW classes,
+ // not our .is-selected and .is-highlighted, so we'd have to copy the pieces
+ // of those rules one by one. Better to rely on the shared classes.
+
" row in
+ // arbitrary-values mode) so the row doesn't read as
+ // committed mid-keystroke
+ 'is-selected': selected && query !== option.value && !noMatch,
+ 'is-highlighted': focus && !noMatch,
+ 'text-disabled!': noMatch,
+ })}
>
- {({ focus, selected }) => (
- // This *could* be done with data-[focus] and data-[selected] instead, but
- // it would be a lot more verbose. those can only be used with TW classes,
- // not our .is-selected and .is-highlighted, so we'd have to copy the pieces
- // of those rules one by one. Better to rely on the shared classes.
-