diff --git a/docs/designs/822-mui-revamp.md b/docs/designs/822-mui-revamp.md index 9122abdd..da66edb1 100644 --- a/docs/designs/822-mui-revamp.md +++ b/docs/designs/822-mui-revamp.md @@ -194,6 +194,19 @@ All from `@mui/material` 9.4.0 (`package-lock.json`). No `@mui/x-*` package; no | 21 | `DayStrip`, `StockBar`, `.meter`, `.meter-stack`, `GradingChip`, `BrandSplash`, `ProvenanceCell`, `FarmDate` | **kept as they are** | bespoke data marks and interactions (#654, #777, F134, #179); their CSS stays (§2.2). `StockBar` is a multi-grade stacked bar and `LinearProgress` is single-value; `GradingChip` is a drag source (§2.1). No alternative meets the functionality, so these are not owner-review rows. | none | | 22 | `usePagedList` "Load more" | `Button` | the hook (381 lines, ticket discipline) is not presentation and stays; there is no offset `Pagination` to adopt. | none | +**Amendment on pair 1 (#826, landed 2026-09-17).** Six places where what shipped differs from the row above, each found by running the rewritten suite against the real installed `@mui/material@9.4.0` rather than assumed from the API surface, plus three presentation changes the owner made after the first captures (mid-implementation, and two more in a #898 review round): + +- **`Autocomplete`'s own Escape handling calls `event.stopPropagation()`**, which the row's plan did not anticipate and which broke a real caller: a picker nested inside a Dialog (Sales' new-order customer picker) used to let a single Escape press both cancel exploration AND bubble to close the dialog. `onClose`'s `reason === "escape"` branch never fires that stop; the fix moves Escape handling into the picker's own root `onKeyDown` (which `useAutocomplete.js`'s `getRootProps` calls BEFORE its internal switch) and sets `event.defaultMuiPrevented = true` so `Autocomplete`'s own Escape case — and its `stopPropagation` — never runs. Caught by `SalesPage.test.tsx`'s existing "lets the dialog own Escape" test, not by the four picker test files. +- **`useAutocomplete.js`'s `handleValue` bails out of calling `onChange` when the newly selected option is `===` the current controlled `value`** — reference equality, not `isOptionEqualToValue`. A picker whose committed entity and discovery window share the same object reference (FR-037's page-level default customer, fetched once and handed to both `controlledCommitted` and the picker's own discovery request) silently no-ops on re-clicking that already-committed option. Fixed by passing a shallow clone as `value`, so it is never reference-equal to a discovery row. Also caught by `SalesPage.test.tsx`, not the four picker files — the row's plan had no reason to anticipate a same-reference collision. +- **`getOptionKey` needed explicit wiring, by id.** Without it, `useAutocomplete.js` falls back to `getOptionLabel(option)` as the React list key, and two rows can share a name (the discovery contract explicitly allows it — the duplicate-name pair test in `NamedEntityPicker.test.tsx`) — a real key collision the row's plan did not name. +- **The listbox's own AX status element could not be reused for loading/no-results text**, contrary to what "`noOptionsText`/`loadingText`… all five come from `t()`" implied. `Autocomplete`'s `StatusSlot` (role="status") mounts UNCONDITIONALLY whenever the popper is open — even with both its loading and no-options children null — because `hasPopupContent` is unconditionally true whenever `freeSolo` is false. A second, usually-empty `role="status"` node coexisting with the engine's own would make `getByRole("status")` ambiguous, and `Autocomplete`'s own text has no way to express "loading while retained rows are still shown" (Load more in flight), which the engine's own status markup must keep handling regardless. Fixed by nulling `slots.status` and keeping the engine's own status/alert spans verbatim; `noOptionsText`/`loadingText`/`clearText`/`closeText`/`openText` are still supplied from `t()` (the five icons/text they would otherwise leak into are also nulled or hidden), so the letter of "no baked-in English" holds even though the visible path never exercises them. +- **`disablePortal` is required**, not just elevation. Without it, `Autocomplete`'s popper renders in a React portal to `document.body`, which defeats the engine's own outside-click `mousedown` listener (`containerRef.current.contains(event.target)` — a portalled option is never a DOM descendant of the container) and would have broken US2's outside-click cancellation. +- **Presentation change (owner, 2026-09-17), beyond the row's scope.** The committed/closed state no longer renders the page-supplied `trigger` element's own markup (a `} /> + ); + const { rerender } = render(renderPicker(true)); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + let input = screen.getByRole("combobox"); + // Navigate to the TRUE last option (50 items, no wrap). + for (let i = 0; i < 50; i++) { + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + } + expect(input.getAttribute("aria-activedescendant")).toBeTruthy(); + // Close, then reopen — same discovery window retained (FR-018), but + // `` is a fresh mount. + rerender(renderPicker(false)); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + rerender(renderPicker(true)); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + input = screen.getByRole("combobox"); + const callsBeforeReopenArrow = mockListFlocks.mock.calls.length; + // First ArrowDown after reopening must move the highlight normally, not + // request another page — the stale-ref bug fired loadMore here instead. + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + expect(mockListFlocks.mock.calls.length).toBe(callsBeforeReopenArrow); + }); + + // `highlightedIdRef` clears on three more transitions besides reopen + // (typing, Escape, outside-click); each sibling below reuses T023-7c's + // technique — an intentional id collision between the row highlighted + // before the transition and a row in the window after it — so a stale ref + // falsely satisfies `atEnd` on the very next ArrowDown. + it("T023-7e: a stale highlightedIdRef does not falsely trigger Load More after typing replaces the discovery window", async () => { + const page0 = Array.from({ length: 50 }, (_, i) => ({ ...fiveFlocks[0], id: `p0-${i}`, name: `Page0 Flock ${i}` })); + // The typed-query's own result window reuses "p0-49" — the id + // highlighted just before typing — as one of ITS rows (not necessarily + // its own last one; the id merely has to exist for a stale ref to + // "find" it). 50 rows, so `hasMore` is true for this window too. + const searchResults = Array.from({ length: 50 }, (_, i) => + i === 49 ? { ...fiveFlocks[0], id: "p0-49", name: "Search Match 49" } + : { ...fiveFlocks[0], id: `s-${i}`, name: `Search Match ${i}` }); + mockListFlocks.mockImplementation(async (params) => { + if (params?.search) return searchResults; + const offset = params?.offset ?? 0; + if (offset === 0) return page0; + return []; + }); + const { input } = openPicker(); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + for (let i = 0; i < 50; i++) { + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + } + expect(input.getAttribute("aria-activedescendant")).toBeTruthy(); + // Type: a brand new discovery generation replaces `items` entirely. + fireEvent.change(input, { target: { value: "Search" } }); + await act(async () => { await vi.advanceTimersByTimeAsync(300); }); + expect(screen.getAllByRole("option")).toHaveLength(50); + const callsBeforeArrow = mockListFlocks.mock.calls.length; + // First ArrowDown after typing must move the highlight onto the new + // window's first option, not request another page — the stale-ref bug + // would falsely read "already at the end" via the id-49 collision. + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + expect(mockListFlocks.mock.calls.length).toBe(callsBeforeArrow); + }); + + it("T023-7f: a stale highlightedIdRef does not falsely trigger Load More after Escape", async () => { + const page0 = Array.from({ length: 50 }, (_, i) => ({ ...fiveFlocks[0], id: `p0-${i}`, name: `Page0 Flock ${i}` })); + mockListFlocks.mockImplementation(async (params) => { + const offset = params?.offset ?? 0; + if (offset === 0) return page0; + return []; + }); + const { input } = openPicker(); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + for (let i = 0; i < 50; i++) { + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + } + expect(input.getAttribute("aria-activedescendant")).toBeTruthy(); + fireEvent.keyDown(input, { key: "Escape" }); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + const callsBeforeArrow = mockListFlocks.mock.calls.length; + // First ArrowDown after Escape must move the highlight normally (the + // discovery window is retained, same 50 items, same true last id + // "p0-49" — a CORRECTLY cleared ref moves to option 0 first; a stale + // one still reading "p0-49" falsely fires loadMore immediately). + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + expect(mockListFlocks.mock.calls.length).toBe(callsBeforeArrow); + }); + + it("T023-7g: a stale highlightedIdRef does not falsely trigger Load More after an outside click", async () => { + const page0 = Array.from({ length: 50 }, (_, i) => ({ ...fiveFlocks[0], id: `p0-${i}`, name: `Page0 Flock ${i}` })); + mockListFlocks.mockImplementation(async (params) => { + const offset = params?.offset ?? 0; + if (offset === 0) return page0; + return []; + }); + const { input } = openPicker(); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + for (let i = 0; i < 50; i++) { + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(1); }); + } + expect(input.getAttribute("aria-activedescendant")).toBeTruthy(); + // A genuine outside click: `document.body` is outside the picker's own + // `containerRef`. + fireEvent.mouseDown(document.body); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + const callsBeforeArrow = mockListFlocks.mock.calls.length; + fireEvent.keyDown(input, { key: "ArrowDown" }); + await act(async () => { await vi.advanceTimersByTimeAsync(50); }); + expect(mockListFlocks.mock.calls.length).toBe(callsBeforeArrow); + }); + + // `useAutocomplete.js`'s `syncHighlightedIndex` closes over `value` by + // reference and re-runs whenever that reference changes; `clonedSelectedValue` + // (`useMemo` on id/name, below) holds it stable so an unrelated render — + // including one mid page-load — cannot resync the highlight to the + // committed row. The mechanism is real in `useAutocomplete.js`, but its + // consequence was not observable across any construction tried, in jsdom + // or Chromium: + // - jsdom, a full Load-More round trip on a synchronous mock + // - jsdom, a forced `rerender()` with unchanged props + // - jsdom, a deferred extension request held open by hand + // - Chromium (`named-entity-picker.spec.ts`), the same deferred-request + // construction, mutation-checked against a rebuilt image + // No test pins it. + it("T023-8: optional clear button commits blank and fires onClear", async () => { const onClear = vi.fn(); const onSnapshot = vi.fn(); @@ -990,47 +1133,63 @@ describe("T023: ARIA contract and interaction semantics", () => { expect(screen.getByRole("combobox").getAttribute("aria-controls")).toBe(listboxId); }); - it("T023-12: closed state: label associated to trigger via htmlFor/id; open swaps trigger for combobox", () => { + it("T023-12 (#826): closed state: label associated to a read-only field carrying the trigger's value and open contract; open swaps it for a combobox", () => { render( My Trigger} /> ); - // The label is programmatically associated: getByLabelText returns the ACTUAL trigger - const labeledTrigger = screen.getByLabelText("Pick Flock"); - expect(labeledTrigger.tagName).toBe("BUTTON"); - expect(labeledTrigger).toHaveTextContent("My Trigger"); - // The association is the cloned trigger's stable id — the label's htmlFor - // points at it, and the trigger carries that exact id. - const labelEl = screen.getByText("Pick Flock"); - expect(labelEl).toHaveAttribute("for", labeledTrigger.id); - expect(labeledTrigger.id).toBeTruthy(); - // The trigger's accessible name is label + current value via aria-labelledby - // referencing [label-id, value-id] (the value child wrapped in a stable span). - const labelledby = labeledTrigger.getAttribute("aria-labelledby")!.split(" "); - expect(labelledby).toHaveLength(2); - expect(labelledby[0]).toBe(labelEl.id); - expect(document.getElementById(labelledby[1])?.textContent).toBe("My Trigger"); - expect(labeledTrigger.getAttribute("aria-labelledby")).toBe(`${labelledby[0]} ${labelledby[1]}`); - // Accessible role name includes BOTH the label and the current value - expect(screen.getByRole("button", { name: /Pick Flock/ })).toBe(labeledTrigger); - expect(screen.getByRole("button", { name: /My Trigger/ })).toBe(labeledTrigger); - // Exactly one control (the trigger) - const buttons = screen.getAllByRole("button"); - expect(buttons.length).toBe(1); - // No combobox or listbox in closed state + // The label is programmatically associated with the closed-state field — + // an MUI TextField now, not the caller's own } /> ); - expect(screen.getByText("Pick Flock")).toBeInTheDocument(); const combo = screen.getByRole("combobox"); expect(combo).toBeInTheDocument(); - // The label is now associated to the combobox (htmlFor = input id) - expect(screen.getByLabelText("Pick Flock")).toBe(combo); - // The trigger button is ABSENT in open state (swapped, not duplicated) - expect(screen.queryByRole("button", { name: "My Trigger" })).toBeNull(); + // The label is now associated to the combobox. + expect(screen.getByLabelText(/^Pick Flock/)).toBe(combo); + // The closed-state read-only field is ABSENT in open state (swapped, not duplicated). + expect(screen.queryByRole("textbox", { name: "Pick Flock" })).toBeNull(); + }); + + // T023-12 asserts the closed field's static open contract but never + // presses a key, so an `onKeyDown` handler removed or narrowed to the + // wrong keys would still pass. A spy on the trigger's own `onClick` is + // what proves the key handler actually reaches it. + it("T023-12b: Enter and Space on the closed field both activate the trigger (Enter and Space, not just click)", () => { + const onTriggerClick = vi.fn(); + render( + My Trigger} /> + ); + const closedField = screen.getByLabelText(/^Pick Flock/); + fireEvent.keyDown(closedField, { key: "Enter" }); + expect(onTriggerClick).toHaveBeenCalledTimes(1); + fireEvent.keyDown(closedField, { key: " " }); + expect(onTriggerClick).toHaveBeenCalledTimes(2); + // Any OTHER key must not activate it (a handler that fires on every + // keydown would pass the two assertions above too). + fireEvent.keyDown(closedField, { key: "a" }); + expect(onTriggerClick).toHaveBeenCalledTimes(2); }); it("T023-13: closed state with no trigger element renders no orphan htmlFor", () => { diff --git a/web/src/components/NamedEntityPicker.tsx b/web/src/components/NamedEntityPicker.tsx index a1e5a8a8..f16944dc 100644 --- a/web/src/components/NamedEntityPicker.tsx +++ b/web/src/components/NamedEntityPicker.tsx @@ -15,9 +15,17 @@ // generations, unavailable states, Escape/clear, Retry) arrives in T026–T034 // on top of the same state. -import React, { useCallback, useEffect, useId, useRef, useState } from "react"; -import type { ChangeEvent, KeyboardEvent, ReactNode } from "react"; +import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import type { KeyboardEvent, ReactNode } from "react"; import { useTranslation } from "react-i18next"; +import Autocomplete from "@mui/material/Autocomplete"; +import TextField from "@mui/material/TextField"; +import Paper from "@mui/material/Paper"; +import InputAdornment from "@mui/material/InputAdornment"; +import Box from "@mui/material/Box"; +import type { PaperProps } from "@mui/material/Paper"; +import type { AutocompleteRenderInputParams } from "@mui/material/Autocomplete"; +import { ChevronDown } from "lucide-react"; // --- Eligibility policy ------------------------------------------------------ @@ -81,8 +89,6 @@ export interface DiscoveryState { eligibilityKey: FlockEligibilityKey | null; /** Rows owned by the current generation only. */ items: T[]; - /** The keyboard-active option; never implies a committed selection. */ - activeId: string | null; /** Offset cursor, advanced by raw server row counts. */ cursor: number; /** Whether an extension (Load more) may be requested. */ @@ -175,7 +181,6 @@ function initialState(eligibilityKey: FlockEligibilityKey normalizedQuery: null, eligibilityKey, items: [], - activeId: null, cursor: 0, hasMore: false, phase: "closed", @@ -277,8 +282,6 @@ export function NamedEntityPickerEngine({ id, label, trig const onClearRef = useRef(onClear); onClearRef.current = onClear; const { t } = useTranslation("namedEntityPicker"); - const ids = useId(); - const listboxId = `${ids}-listbox`; const [state, setState] = useState>(() => initialState(eligibilityKey)); const stateRef = useRef(state); stateRef.current = state; @@ -330,7 +333,6 @@ export function NamedEntityPickerEngine({ id, label, trig selection: { entity, requestedId: null, phase: "committed", transitionGeneration: selectionGenRef.current }, discovery: { ...prev.discovery, - activeId: entity.id, rawQuery: entity.name, }, })); @@ -348,7 +350,7 @@ export function NamedEntityPickerEngine({ id, label, trig // dropped. setState((prev) => ({ ...prev, - discovery: { ...prev.discovery, phase: "replacing", items: [], error: null, activeId: null, discoveryGeneration: gen }, + discovery: { ...prev.discovery, phase: "replacing", items: [], error: null, discoveryGeneration: gen }, })); let page: T[]; try { @@ -482,7 +484,7 @@ export function NamedEntityPickerEngine({ id, label, trig setState((prev) => ({ ...prev, selection: { entity: resolved, requestedId: null, phase: "committed", transitionGeneration: gen }, - discovery: { ...prev.discovery, rawQuery: resolved.name, activeId: resolved.id }, + discovery: { ...prev.discovery, rawQuery: resolved.name }, })); } catch { if (selectionGenRef.current !== gen) return; @@ -498,12 +500,18 @@ export function NamedEntityPickerEngine({ id, label, trig // exact 250 ms pause (FR-008) owns whether a replacement goes out. The // debounce timer is the newest-intent check: each keystroke cancels the // previous one, so only the final text ever requests. - const onQueryChange = useCallback((e: ChangeEvent) => { + // + // Takes the raw string directly (not a ChangeEvent): `Autocomplete`'s own + // `onInputChange(event, value, reason)` is the only caller (#826 — the + // combobox is `Autocomplete`'s, not a hand-rolled ``), and + // it already hands over the string. + const onQueryChange = useCallback((raw: string) => { if (disabled) return; - const raw = e.target.value; const trimmed = raw.trim(); const gen = ++discoveryGenRef.current; if (debounceRef.current !== null) window.clearTimeout(debounceRef.current); + // Typing replaces `items`, so the highlighted option is gone with them. + highlightedIdRef.current = null; setState((prev) => ({ ...prev, discovery: { @@ -512,7 +520,6 @@ export function NamedEntityPickerEngine({ id, label, trig normalizedQuery: trimmed === "" ? null : trimmed, items: [], error: null, - activeId: null, cursor: 0, hasMore: false, phase: "debouncing", @@ -561,7 +568,6 @@ export function NamedEntityPickerEngine({ id, label, trig ...prev.discovery, eligibilityKey, items: [], - activeId: null, cursor: 0, hasMore: false, phase: "closed", @@ -586,7 +592,6 @@ export function NamedEntityPickerEngine({ id, label, trig eligibilityKey, items: [], error: null, - activeId: null, cursor: 0, hasMore: false, phase: "debouncing", @@ -617,6 +622,11 @@ export function NamedEntityPickerEngine({ id, label, trig // by the eligibility effect) handles the request under the new key. useEffect(() => { if (!open) { + // `` remounts on every open with a clean highlight, but + // this ref outlives it. Left stale, a reopen onto a retained window + // (FR-018) whose end had been reached would treat the first ArrowDown + // as `atEnd` and load a page instead of moving the highlight. + highlightedIdRef.current = null; if (debounceRef.current !== null) { window.clearTimeout(debounceRef.current); debounceRef.current = null; @@ -630,7 +640,7 @@ export function NamedEntityPickerEngine({ id, label, trig discoveryGenRef.current += 1; setState((prev) => ({ ...prev, - discovery: { ...prev.discovery, phase: "closed", items: [], activeId: null, hasMore: false, cursor: 0 }, + discovery: { ...prev.discovery, phase: "closed", items: [], hasMore: false, cursor: 0 }, })); } return; @@ -651,93 +661,107 @@ export function NamedEntityPickerEngine({ id, label, trig // it never runs before the input exists in the same commit. The Retry paths // above still call focus() themselves: those run from a button that has // already taken focus, so this effect (keyed on `open`) does not fire. + // + // Select-all stays (typing must replace the name, #735), but a selection + // wider than the field scrolls the browser to its active end, showing the + // TAIL of a long name. `"backward"` puts the active end at index 0, and + // `scrollLeft = 0` forces the head into view, which the direction alone + // did not reliably do in Chromium. useEffect(() => { if (!open || disabled) return; const inputEl = document.getElementById(id); if (inputEl instanceof HTMLInputElement) { inputEl.focus(); - inputEl.select(); + inputEl.setSelectionRange(0, inputEl.value.length, "backward"); + inputEl.scrollLeft = 0; } }, [open, disabled, id]); - // Arrow navigation: activation only — committing is Enter/pointer (FR-030). - // Down Arrow at the loaded end with hasMore requests the next page (FR-032). - const onArrow = useCallback((delta: number) => { - if (disabled) return; - const { items, activeId, hasMore, phase } = stateRef.current.discovery; - if (items.length === 0) return; - const idx = activeId ? items.findIndex((x) => x.id === activeId) : -1; - // Down Arrow at the loaded end: request extension if more is available. - if (delta === 1 && idx === items.length - 1 && hasMore && phase === "ready") { - void loadMore(); - return; + // #826 — `Autocomplete` now owns arrow/Enter navigation and highlight + // tracking (the engine no longer hand-rolls a `role=combobox`/`listbox` + // pair). `handleHomeEndKeys={false}` on the element below keeps Home/End + // native (FR-031). Two things `Autocomplete` does NOT know about stay here: + // FR-032 (Down Arrow at the loaded end with `hasMore` requests the next + // page) and the Escape cancellation contract (US2: restore committed/blank + // text, retain the discovery window, call the page's `onEscape`). + const highlightedIdRef = useRef(null); + + // US2: cancel exploration and restore committed/blank text — shared by + // Escape (`Autocomplete`'s `onClose` with reason "escape", below). The + // discovery window (items, cursor, hasMore, phase) is retained, never + // wiped, and the discovery generation is untouched: an in-flight request + // settles into the retained window under its own generation. + const cancelExploration = useCallback(() => { + if (debounceRef.current !== null) { + window.clearTimeout(debounceRef.current); + debounceRef.current = null; + } + // `highlightedIdRef` is set only by `onHighlightChange`; clear it here so + // a reopen's first ArrowDown cannot read a stale `atEnd` (FR-032). + highlightedIdRef.current = null; + const selection = stateRef.current.selection; + // A fixed requested-ID read belongs to the page's selection intent and + // must still be allowed to settle. + if (selection.phase !== "resolving" || selection.requestedId === null) { + selectionGenRef.current += 1; } - const next = Math.min(items.length - 1, Math.max(0, idx + delta)); + const committed = selection.entity; setState((prev) => ({ ...prev, - discovery: { ...prev.discovery, activeId: prev.discovery.items[next]?.id ?? prev.discovery.activeId }, + discovery: { ...prev.discovery, rawQuery: committed?.name ?? "" }, })); - }, [disabled, loadMore]); + setCommittedText(committed?.name ?? null); + }, []); - const onKey = useCallback((e: KeyboardEvent) => { + // FR-032: Down Arrow at the loaded end with `hasMore` requests the next + // page instead of moving the highlight (there is nowhere further to move + // it). Wired onto `Autocomplete`'s own `onKeyDown` prop, which + // `useAutocomplete.js`'s `getRootProps` calls BEFORE its own switch, so + // `highlightedIdRef` still holds the option the PREVIOUS keypress left + // highlighted — the same "already at the end" check the hand-rolled + // `onArrow` used to make against `activeId`. + // + // Escape is ALSO handled here rather than through `Autocomplete`'s own + // `onClose` callback: `useAutocomplete.js`'s own Escape case calls + // `event.stopPropagation()` after closing the popup, which — because + // `FlockPicker`/`CustomerPicker` render inside dialogs whose own + // Escape-to-close listens on an ancestor — swallowed a SINGLE Escape press + // that used to close both the exploration AND the dialog at once + // (SalesPage's new-order picker test, #826 round 1). Setting + // `defaultMuiPrevented` tells `getRootProps`'s `handleKeyDown` (which calls + // our handler FIRST, then checks that flag before its own switch) to skip + // its Escape case entirely, so only OUR `preventDefault` runs and the event + // keeps bubbling — restoring the pre-#826 behavior exactly. + const handleRootKeyDown = useCallback((e: KeyboardEvent & { defaultMuiPrevented?: boolean }) => { if (disabled) return; - const items = stateRef.current.discovery.items; - if (e.key === "ArrowDown") { - e.preventDefault(); - onArrow(1); - return; - } - if (e.key === "ArrowUp") { - e.preventDefault(); - onArrow(-1); - return; - } - if (e.key === "Enter") { - const active = items.find((x) => x.id === stateRef.current.discovery.activeId); - if (active) { - e.preventDefault(); - commit(active); - } - return; - } if (e.key === "Escape") { - // US2: cancel exploration, restore committed/blank text, cancel any - // pending debounce (so a stale request never fires after restore), - // and close the picker via the page's onEscape callback. e.preventDefault(); - // Cancel debounce: a pending replacement must not fire after restore. - // Cancel the debounce (a pending replacement must not fire after - // restore) and claim a SELECTION-transition generation. Escape must - // NOT bump the discovery generation or wipe the discovery window: the - // page may keep the picker open (onEscape is page-controlled), and the - // retained rows/cursor stay usable. The rawQuery/activeId restore below - // is the cancellation; an in-flight discovery settles into the retained - // window under its own generation. - if (debounceRef.current !== null) { - window.clearTimeout(debounceRef.current); - debounceRef.current = null; - } - const selection = stateRef.current.selection; - // Closing only cancels exploration. A fixed requested-ID read belongs - // to the page's selection intent and must still be allowed to settle. - if (selection.phase !== "resolving" || selection.requestedId === null) { - selectionGenRef.current += 1; - } - const committed = selection.entity; - setState((prev) => ({ - ...prev, - discovery: { - ...prev.discovery, - rawQuery: committed?.name ?? "", - activeId: null, - }, - })); - setCommittedText(committed?.name ?? null); + e.defaultMuiPrevented = true; + cancelExploration(); onEscapeRef.current?.(); return; } - // Home/End: native input behavior, not intercepted (FR-031). - }, [commit, onArrow, disabled]); + if (e.key !== "ArrowDown") return; + const d = stateRef.current.discovery; + if (d.items.length === 0) return; + const atEnd = highlightedIdRef.current === d.items[d.items.length - 1].id; + if (atEnd && d.hasMore && d.phase === "ready") { + // Also suppress `Autocomplete`'s own ArrowDown handling for THIS + // press: `options` has not grown yet (the extension is async), so its + // `changeHighlightedIndex` would move against the STALE, still-short + // list — and because `disableListWrap` defaults to `false`, moving + // past the last valid index WRAPS to the first option instead of + // clamping. Left alone, every boundary press would silently snap the + // highlight back to the top of a growing list instead of holding + // still for the extension to land — found via + // `named-entity-picker.spec.ts`'s real-browser keyboard-paging test, + // not reasoned about. The next ArrowDown (after `options` grows) again + // gets no special handling and moves forward normally. + e.preventDefault(); + e.defaultMuiPrevented = true; + void loadMore(); + } + }, [disabled, loadMore, cancelExploration]); // Exploration (FR-020): the visible text differs from the committed label. // A committed picker with an untouched field is not exploring. @@ -821,7 +845,7 @@ export function NamedEntityPickerEngine({ id, label, trig selection: { entity, requestedId: null, phase: entity ? "committed" : "blank", transitionGeneration: prev.selection.transitionGeneration }, discovery: sameEntity ? prev.discovery - : { ...prev.discovery, rawQuery: entity?.name ?? "", activeId: entity?.id ?? null, items: [], hasMore: false, cursor: 0 }, + : { ...prev.discovery, rawQuery: entity?.name ?? "", items: [], hasMore: false, cursor: 0 }, }; }); // Exact-identity validation: only when an exact read is provided AND the @@ -859,7 +883,7 @@ export function NamedEntityPickerEngine({ id, label, trig setState((prev) => ({ ...prev, selection: { entity: null, requestedId: null, phase: "blank", transitionGeneration: gen }, - discovery: { ...prev.discovery, rawQuery: "", activeId: null }, + discovery: { ...prev.discovery, rawQuery: "" }, })); return; } @@ -886,7 +910,7 @@ export function NamedEntityPickerEngine({ id, label, trig setState((prev) => ({ ...prev, selection: { entity: resolved, requestedId: null, phase: "committed", transitionGeneration: gen }, - discovery: { ...prev.discovery, rawQuery: resolved.name, activeId: resolved.id }, + discovery: { ...prev.discovery, rawQuery: resolved.name }, })); }).catch(() => { if (selectionGenRef.current !== gen) return; @@ -921,6 +945,8 @@ export function NamedEntityPickerEngine({ id, label, trig window.clearTimeout(debounceRef.current); debounceRef.current = null; } + // Same stale-ref hazard as `cancelExploration`. + highlightedIdRef.current = null; const selection = stateRef.current.selection; // Outside-click closes exploration, not the page-owned requested-ID // intent. Keep that exact read live while the picker is closed. @@ -933,7 +959,6 @@ export function NamedEntityPickerEngine({ id, label, trig discovery: { ...prev.discovery, rawQuery: committed?.name ?? "", - activeId: null, }, })); setCommittedText(committed?.name ?? null); @@ -945,12 +970,17 @@ export function NamedEntityPickerEngine({ id, label, trig }, [open]); const d = state.discovery; + // The clone (see `value` below) is memoised by id/name: `useAutocomplete`'s + // `syncHighlightedIndex` closes over `value` by reference and re-runs when + // it changes, so a fresh clone every render re-synced the highlight to the + // committed row on unrelated renders, including one mid page-load. + const selectedEntity = state.selection.entity; + const clonedSelectedValue = useMemo( + () => (selectedEntity ? { ...selectedEntity } : null), + // eslint-disable-next-line react-hooks/exhaustive-deps + [selectedEntity?.id, selectedEntity?.name], + ); const showLoading = d.phase === "replacing" || d.phase === "extending" || d.phase === "debouncing"; - const loading = showLoading; - const activeId = d.activeId; - // aria-controls only when the listbox is actually rendered (open state). - const ariaControls = open ? listboxId : undefined; - const ariaActivedescendant = activeId ? `${ids}-opt-${activeId}` : undefined; // The input value IS the discovery's raw text — no committed-text fallback. // After a commit the field shows the committed name (rawQuery was set to it); // typing explores and the committed entity survives in `selection`. @@ -975,40 +1005,194 @@ export function NamedEntityPickerEngine({ id, label, trig ? t("results", { count: d.items.length }) : ""; + // US2: optional clear — commits blank. A selection transition only: the + // discovery window is retained and its generation is untouched. + const clearSelection = useCallback(() => { + selectionGenRef.current += 1; + if (debounceRef.current !== null) { + window.clearTimeout(debounceRef.current); + debounceRef.current = null; + } + setCommittedText(null); + setState((prev) => ({ + ...prev, + selection: { entity: null, requestedId: null, phase: "blank", transitionGeneration: selectionGenRef.current }, + discovery: { ...prev.discovery, rawQuery: "" }, + })); + onClearRef.current?.(); + }, []); + + // #826 — the status/alert/Load-more/Clear footer renders INSIDE + // `Autocomplete`'s floating Paper, as a sibling of the `
    ` + // it supplies as `children` — never through `slotProps.listbox`, which + // would put a ` + )} + {f.d.phase === "extension-error" && ( + {f.t("loadMoreFailed")} + )} + {f.d.phase === "extension-error" && ( + + )} + {f.d.hasMore && ( + + )} + {/* US3 (T034/T038): an unavailable exact identity (scoped 404 / + transport failure on the exact GET) renders the translated + unavailable label and a keyboard-reachable Retry — never a raw + ID or a first-result substitution. The Retry re-runs the exact + read; focus returns to the input on success. */} + {f.unavailable && ( + {f.t("unavailable")} — {f.t("unavailableExplanation")} + )} + {/* Deliberately NOT gated on `disabled` (US3 remediation): Retry + re-resolves a FIXED identity via the exact GET, not ordinary + discovery/selection, which stays disabled above. */} + {f.unavailable && ( + + )} + {!f.required && f.committedEntity && !f.disabled && ( + + )} + + + ); + }; + } + // US2: closed state renders exactly ONE trigger in the normal form slot. // The combobox/listbox are absent. Open state: no trigger, just the // searchable combobox in the same position. if (!open) { - // Closed state: visible label + exactly one field-sized trigger. - // Programmatic association: the label has an id; the trigger is cloned - // with a trigger id and its children wrapped in a value span. The - // trigger's aria-labelledby references [labelId, valueId] so the - // accessible name is "