From 4c338a37e91c0e1d19f6fb2a0bd3ededa6ebcb93 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 01:13:09 -0400 Subject: [PATCH] fix(web): take the enlarge button out of the tab order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every string field in a tool or elicitation form carries an enlarge button in its right section, so tabbing through a form stopped on one between each pair of fields. Give it tabIndex={-1}, as #1487 did for the clear button. That button was deliberately left in the tab order by #2042, on the grounds that clearing has a keyboard equivalent and enlarging has none. So give it one: Enter on the focused single-line field enlarges it and enters the newline. Nothing was listening for that key there — no consumer renders SchemaForm inside a
— and it is the key a user presses trying to type the newline the input swallows, which is the failure #2042 exists to fix. Enter enters the newline rather than only reshaping the field, since consuming the keystroke leaves the next word running on from the last. A field already at its maxLength is enlarged without one. Shift+Enter enlarges too; the Ctrl/Cmd/Alt chords are left free for a consumer to bind to running the tool. Clicking the button still only enlarges. Signed-off-by: cliffhall --- .../EnlargeButton/EnlargeButton.stories.tsx | 7 +- .../EnlargeButton/EnlargeButton.test.tsx | 23 +- .../elements/EnlargeButton/EnlargeButton.tsx | 17 +- .../groups/SchemaForm/SchemaForm.test.tsx | 213 +++++++++++++++++- .../groups/SchemaForm/SchemaForm.tsx | 55 ++++- 5 files changed, 297 insertions(+), 18 deletions(-) diff --git a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.stories.tsx b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.stories.tsx index 9d77efd7..f43dc11a 100644 --- a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.stories.tsx +++ b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.stories.tsx @@ -11,12 +11,15 @@ const meta: Meta = { export default meta; type Story = StoryObj; -// Default: labelled "Enlarge", reachable by keyboard, and clickable. +// Default: labelled "Enlarge", clickable, and out of the tab order (#2138) — +// a form's string fields each carry one, so leaving them in doubled the tab +// stops between adjacent fields. SchemaForm binds Enter on the field itself as +// the keyboard route in. export const Default: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); const button = await canvas.findByRole("button", { name: "Enlarge" }); - await expect(button).not.toHaveAttribute("tabindex", "-1"); + await expect(button).toHaveAttribute("tabindex", "-1"); await userEvent.click(button); }, }; diff --git a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.test.tsx b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.test.tsx index ad6fb88f..f27905a6 100644 --- a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.test.tsx +++ b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.test.tsx @@ -9,14 +9,33 @@ describe("EnlargeButton", () => { expect(screen.getByRole("button", { name: "Enlarge" })).toBeInTheDocument(); }); - it("stays in the keyboard tab order", () => { + // Out of the tab order like ClearButton (#2138) — a form's string fields each + // carry one, so leaving them in doubled the stops between adjacent fields. + it("is out of the keyboard tab order", () => { renderWithMantine(); - expect(screen.getByRole("button", { name: "Enlarge" })).not.toHaveAttribute( + expect(screen.getByRole("button", { name: "Enlarge" })).toHaveAttribute( "tabindex", "-1", ); }); + // The property that matters is skipped-by-Tab, not the attribute that + // implements it — asserted by driving a real Tab past a neighbouring input. + it("is skipped when tabbing through the surrounding form", async () => { + const user = userEvent.setup(); + renderWithMantine( + <> + + + + , + ); + + screen.getByRole("textbox", { name: "Before" }).focus(); + await user.tab(); + expect(screen.getByRole("textbox", { name: "After" })).toHaveFocus(); + }); + it("invokes onClick when clicked", async () => { const user = userEvent.setup(); const onClick = vi.fn(); diff --git a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.tsx b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.tsx index 3980ab68..38c46a95 100644 --- a/clients/web/src/components/elements/EnlargeButton/EnlargeButton.tsx +++ b/clients/web/src/components/elements/EnlargeButton/EnlargeButton.tsx @@ -23,6 +23,7 @@ const EnlargeActionIcon = ActionIcon.withProps({ variant: "subtle", color: "gray", size: "sm", + tabIndex: -1, }); /** @@ -35,10 +36,18 @@ const EnlargeActionIcon = ActionIcon.withProps({ * the newlines already typed, and either answer (silently discard them, or keep * a value the field can no longer display) is worse than staying enlarged. * - * Unlike ClearButton it stays in the keyboard tab order. Clearing a field has a - * keyboard equivalent (select-all, delete), so #1487 could take that button out - * of the tab order without cost; entering multiline mode has none, so removing - * this one would put the feature out of reach of keyboard users entirely. + * `tabIndex={-1}` keeps it clickable but out of the keyboard tab order, the same + * as ClearButton (#1487) — a form's string fields each carry one, so leaving + * them in doubled the tab stops between one field and the next (#2138). + * + * That is only affordable because the keyboard keeps its own way in: SchemaForm + * binds Enter on the single-line field, which enlarges it and enters the + * newline in one go. Nothing else is listening for that key there, and it is + * the one a user presses trying to type the newline the field cannot hold — so + * the gesture that fails is the gesture that fixes it. Do not take this button + * out of the tab order anywhere that binding is absent; unlike clearing + * (select-all, delete) there is no built-in equivalent, and multiline mode + * would simply be unreachable by keyboard. */ export function EnlargeButton({ onClick, diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index d0ec70a8..db3f7bbb 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1932,22 +1932,26 @@ describe("SchemaForm multiline strings (#2042)", () => { ).toBeInTheDocument(); }); - // The button unmounts in the same commit that mounts the text area, so - // without an explicit hand-off a keyboard user is left focused on nothing. + // The button unmounts in the same commit that mounts the text area, taking + // the focused element with it, so without an explicit hand-off the user is + // left focused on nothing. (The keyboard route hands off from the field + // instead — see the #2138 suite.) it("moves focus into the text area, caret last, when activated", async () => { const user = userEvent.setup(); renderWithMantine(); await user.type(screen.getByRole("textbox", { name: /Note/ }), "typed"); - await user.tab(); - expect(screen.getByRole("button", { name: "Enlarge Note" })).toHaveFocus(); - await user.keyboard("{Enter}"); + await user.click(screen.getByRole("button", { name: "Enlarge Note" })); const textarea = screen.getByRole("textbox", { name: /Note/, }) as HTMLTextAreaElement; expect(textarea.tagName).toBe("TEXTAREA"); expect(textarea).toHaveFocus(); + // Clicking asks for a bigger box and nothing else: the value is carried + // over untouched, and only Enter — the key that means "new line" — enters + // one (#2138). + expect(textarea.value).toBe("typed"); expect(textarea.selectionStart).toBe("typed".length); // And the caret really is at the end: typing appends rather than prepends. @@ -1970,3 +1974,202 @@ describe("SchemaForm multiline strings (#2042)", () => { expect(screen.getByRole("button", { name: "Enlarge Note" })).toBeDisabled(); }); }); + +// The enlarge button is clickable but out of the tab order, so tabbing runs +// field to field; Enter in a single-line string field takes its place as the +// keyboard route into multiline mode (#2138). +describe("SchemaForm enlarge keyboard access (#2138)", () => { + const twoStringSchema: InspectorFormSchema = { + type: "object", + properties: { + note: { type: "string", title: "Note" }, + summary: { type: "string", title: "Summary" }, + }, + }; + + function TwoStringHarness({ disabled }: { disabled?: boolean }) { + const [values, setValues] = useState>({}); + return ( + + ); + } + + const noteField = () => + screen.getByRole("textbox", { name: /Note/ }) as HTMLTextAreaElement; + + // The defect the issue was filed for: an extra stop per field, on every + // string field of every tool form. + it("tabs from one string field to the next, skipping the enlarge buttons", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.tab(); + + expect(screen.getByRole("textbox", { name: /Summary/ })).toHaveFocus(); + }); + + // A populated field renders the clear button too, so this is the widest the + // right section ever gets — and still must not add a stop. + it("skips both right-section buttons when the field holds a value", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.type(noteField(), "typed"); + expect(screen.getByRole("button", { name: "Clear" })).toBeInTheDocument(); + await user.tab(); + + expect(screen.getByRole("textbox", { name: /Summary/ })).toHaveFocus(); + }); + + it("enlarges the focused field when Enter is pressed", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + expect(noteField().tagName).toBe("INPUT"); + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(noteField().tagName).toBe("TEXTAREA"); + }); + + // Enlarging is per field: the key must not reach the neighbour. + it("enlarges only the focused field", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(screen.getByRole("textbox", { name: /Summary/ }).tagName).toBe( + "INPUT", + ); + }); + + // The other gesture a user reaches for when an input will not take a newline. + it("enlarges on Shift+Enter too", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.keyboard("{Shift>}{Enter}{/Shift}"); + + expect(noteField().tagName).toBe("TEXTAREA"); + }); + + // Those chords read as "submit" and stay free for a consumer to bind to + // running the tool; claiming them would silently enlarge a field instead. + it.each([ + ["Ctrl", "{Control>}{Enter}{/Control}"], + ["Meta", "{Meta>}{Enter}{/Meta}"], + ["Alt", "{Alt>}{Enter}{/Alt}"], + ])("leaves %s+Enter alone", async (_name, keys) => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.keyboard(keys); + + expect(noteField().tagName).toBe("INPUT"); + }); + + it("leaves the field alone for any other key", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.type(noteField(), "text "); + await user.keyboard("{Escape}{ArrowDown} "); + + expect(noteField().tagName).toBe("INPUT"); + }); + + // The keystroke has to mean what it says. Enlarging without entering the + // newline consumes the key and leaves the next word running on from the last + // one — the user pressed "new line" and got a reshaped box. + it("enters the newline it was asked for, not just the text area", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.type(noteField(), "before"); + await user.keyboard("{Enter}"); + expect(noteField().value).toBe("before\n"); + + await user.keyboard("after"); + expect(noteField().value).toBe("before\nafter"); + }); + + // The caret follows the newline, so what is typed next lands under it rather + // than back on the first line. + it("leaves the caret after the newline", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.type(noteField(), "before"); + await user.keyboard("{Enter}"); + + expect(noteField().selectionStart).toBe("before\n".length); + }); + + // A newline is a character, so a field with no room for one is enlarged + // without it rather than pushed past a constraint its schema states. + it("enlarges without a newline when the field is at its maxLength", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(noteField().tagName).toBe("TEXTAREA"); + expect(noteField().value).toBe("abc"); + }); + + // An empty field still has room, so the newline is entered there too. + it("enters a newline in an empty field", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(noteField().value).toBe("\n"); + }); + + // The binding is the keyboard's only route in now, so announce that the + // field carries one rather than leaving it undiscoverable. + it("advertises the shortcut on the single-line field, not the text area", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + expect(noteField()).toHaveAttribute("aria-keyshortcuts", "Enter"); + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(noteField()).not.toHaveAttribute("aria-keyshortcuts"); + }); + + // Same reasoning as the disabled button: a text area mounting disabled cannot + // take focus, so it would drop focus to the document. + it("cannot be enlarged by keyboard while the form is disabled", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + noteField().focus(); + await user.keyboard("{Enter}"); + + expect(noteField().tagName).toBe("INPUT"); + }); +}); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 7b39c5dd..f5acc68d 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -718,22 +718,67 @@ export function SchemaForm({ ); } + const enlarge = () => + setEnlargedFields((previous) => new Set([...previous, fieldName])); + + // Enter both enlarges the field and enters the newline that was asked + // for. Enlarging alone would consume the keystroke and leave the caret + // where it was, so the next thing typed runs on from the last word — the + // user pressed the key that means "new line" and got a reshaped box. + // + // The newline lands at the end rather than at the caret because that is + // where EnlargedStringField puts the caret regardless (see its comment); + // going anywhere else would separate the newline from the text that + // follows it. A field already at its maxLength is enlarged without one, + // since the alternative is breaching a constraint the schema states. + const enlargeWithNewline = () => { + const current = (rawValue as string) ?? ""; + const room = + fieldSchema.maxLength === undefined || + current.length < fieldSchema.maxLength; + if (room) handleFieldChange(fieldName, `${current}\n`); + enlarge(); + }; + return ( `, so there is no + // implicit submission to displace — and it is the very key a user + // presses trying to enter the newline a single-line input swallows, + // which is what #2042 exists to fix. So the gesture that fails is the + // one that enlarges, rather than a shortcut nobody would guess. + onKeyDown={(event) => { + // Shift+Enter is the other newline gesture, so it enlarges too. The + // Ctrl/Cmd/Alt chords are deliberately left alone: those read as + // "submit" in a form, and a consumer binding one (run the tool) + // must not be overridden into enlarging a field instead. + if ( + event.key !== "Enter" || + event.ctrlKey || + event.metaKey || + event.altKey + ) { + return; + } + event.preventDefault(); + enlargeWithNewline(); + }} + // Announces that the field carries a shortcut at all. A keyboard user + // no longer meets the button by tabbing, so without this the binding + // is undiscoverable rather than merely unlabelled. + aria-keyshortcuts="Enter" rightSection={ - setEnlargedFields( - (previous) => new Set([...previous, fieldName]), - ) - } + onClick={enlarge} /> {clearButton}