diff --git a/app/ui/lib/NumberInput.browser.spec.tsx b/app/ui/lib/NumberInput.browser.spec.tsx new file mode 100644 index 000000000..7cde8182e --- /dev/null +++ b/app/ui/lib/NumberInput.browser.spec.tsx @@ -0,0 +1,109 @@ +/* + * 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 { useState } from 'react' +import * as R from 'remeda' +import { expect, test } from 'vitest' +import { render } from 'vitest-browser-react' +import { userEvent } from 'vitest/browser' + +import { NumberInput } from './NumberInput' + +type Props = Omit, 'onChange'> & { + recordChanges?: boolean +} + +function NumberInputHarness({ recordChanges = true, ...props }: Props) { + const [value, setValue] = useState(props.value ?? NaN) + const [changes, setChanges] = useState([]) + return ( + <> + { + setValue(nextValue) + if (recordChanges) setChanges((values) => [...values, nextValue]) + }} + /> + Changes: {changes.length ? changes.map(String).join(', ') : '(none)'} + + ) +} + +test('fires onChange per keystroke with the parsed number', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await userEvent.type(input, '1') + await expect.element(screen.getByText('Changes: 1')).toBeVisible() + await userEvent.type(input, '2') + + await expect.element(screen.getByText('Changes: 1, 12')).toBeVisible() +}) + +test('fires onChange with NaN when the input is cleared', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await input.clear() + + await expect.element(screen.getByText('Changes: NaN')).toBeVisible() +}) + +test('clamps typed values above maxValue', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await input.fill('150') + + await expect.element(screen.getByText('Changes: 100')).toBeVisible() + await expect.element(input).toHaveValue('100') +}) + +test('clamps typed values below minValue', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await input.fill('0') + + await expect.element(screen.getByText('Changes: 1')).toBeVisible() + await expect.element(input).toHaveValue('1') +}) + +test('only simplifies numbers on blur', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await input.fill('0') + await expect.element(screen.getByText('Changes: 0')).toBeVisible() + + for (const precision of R.range(0, 6)) { + const value = `1.${'0'.repeat(precision)}` // 1., 1.0, etc. + await input.fill(value) + await expect.element(screen.getByText('Changes: 0')).toBeVisible() + await expect.element(input).toHaveValue(value) + } + + await userEvent.tab() + + await expect.element(screen.getByText('Changes: 0, 1')).toBeVisible() + await expect.element(input).toHaveValue('1') +}) + +test('still controls the displayed value when onChange causes no re-render', async () => { + const screen = await render() + const input = screen.getByRole('textbox', { name: 'Test number' }) + + await input.fill('1099') + await expect.element(input).toHaveValue('1023') + + await input.fill('10239') + await expect.element(input).toHaveValue('1023') +}) diff --git a/app/ui/lib/NumberInput.spec.tsx b/app/ui/lib/NumberInput.spec.tsx new file mode 100644 index 000000000..2350e80b5 --- /dev/null +++ b/app/ui/lib/NumberInput.spec.tsx @@ -0,0 +1,25 @@ +/* + * 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 { describe, expect, it } from 'vitest' + +import { isCanonicalNumberString } from './NumberInput' + +describe('isCanonicalNumberString', () => { + it.each([ + ['0', true], + ['-1', true], + ['1.5', true], + ['', false], + ['01', false], + ['1.', false], + ['1.0', false], + ['-0', false], + ])('%j => %j', (value, expected) => { + expect(isCanonicalNumberString(value)).toBe(expected) + }) +}) diff --git a/app/ui/lib/NumberInput.tsx b/app/ui/lib/NumberInput.tsx index 3792654b7..8ebe0c032 100644 --- a/app/ui/lib/NumberInput.tsx +++ b/app/ui/lib/NumberInput.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ import cn from 'classnames' -import { useRef, type Ref } from 'react' +import { useEffect, useRef, type Ref } from 'react' import { useButton, useLocale, @@ -23,6 +23,8 @@ type NumberInputProps = AriaNumberFieldProps & { ref?: Ref } +export const isCanonicalNumberString = (value: string) => String(Number(value)) === value + export function NumberInput(props: NumberInputProps) { const { locale } = useLocale() const state = useNumberFieldState({ ...props, locale }) @@ -31,6 +33,20 @@ export function NumberInput(props: NumberInputProps) { const { groupProps, inputProps, incrementButtonProps, decrementButtonProps } = useNumberField(props, state, inputRef) + // react-aria only fires props.onChange on commit (blur / Enter / stepper), + // but we want form state to update as soon as it would produce a different + // field value. Committing whenever state.inputValue changes to an + // unambiguous number lets react-aria keep controlling parsing, clamping + // etc., but forces it to be more eager. + // + // Context: https://github.com/adobe/react-spectrum/issues/7984 + useEffect(() => { + if (isCanonicalNumberString(state.inputValue) || state.inputValue === '') { + state.commit() + } + // eslint-disable-next-line exhaustive-deps + }, [state.inputValue]) + return (