diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 63eee4deb..57e6f497d 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -164,7 +164,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | -| `spa.library-pickers-resolve-by-search` | A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), so editing an existing record can never lose or fail to name its selection. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change. | 2026-07-26 | [link](records/spa/library-pickers-resolve-by-search.md) | +| `spa.library-pickers-resolve-by-search` | A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit-load refresh MERGES rather than replaces so a response that fails to NAME a selection can never CLEAR its id; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable ``. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change.' +signals: 'library picker typeahead, SearchPicker, searchLibraryPickerOptions, titleContainsQuery, LIBRARY_PICKER_RESULTS, LIBRARY_PICKER_MIN_QUERY, LIBRARY_PICKER_LUCENE_SPECIALS, compile typed text not raw Lucene, Lucene && || escaping, picker truncation hint removed, loadAllPages Class A, LuceneSearchIndex.Search hitsLimit, useIsMountedRef, aria-activedescendant combobox keyboard, refresh must not clear an unnamed id · paths: `web/src/api/libraryBrowse.ts`, `web/src/schedules/pickers.tsx`, `web/src/hooks.ts`, `web/src/screens/RerunCollectionsScreen.tsx`, `web/src/screens/PlaylistsScreen.tsx`, `web/src/screens/FillerPresetsScreen.tsx`, `web/src/api/paging.ts`, `docs/spa-conventions.md` §3b · issues: #651, #644, #578, #440' mechanics: '`docs/spa-conventions.md` §3b' --- @@ -50,6 +50,24 @@ replaces: the old picker could only name a selection that happened to fall insid browse rows, and rendered a bare `#9999` otherwise. A failed resolution degrades to `#id`; it never clears the id. +A cold cross-family review found that "renders from the record" was not by itself enough, because +the *record* can arrive without its selection. `RerunCollectionsController.ProjectToResponseModel` +derives both `selectedId` and `selectedName` from the same eager-loaded navigation, and +`GetRerunCollectionByIdHandler` loads media metadata only for Show/Season/Artist/Movie while +`MediaCollections/Mapper` maps RemoteStream through `_ => null` — so opening a RemoteStream rerun +collection returned HTTP 200 with a null selection and the edit-load refresh *cleared a stored id*, +leaving Save permanently disabled. The rule is therefore stated as a prohibition on the client: +**no code path may clear a stored id it merely failed to name.** The refresh merges +(`refreshed.selectedId ?? current.selectedId`) instead of replacing, and every affected type +(RemoteStream, Episode, MusicVideo, Song, OtherVideo, Image) is covered by its own test. The +underlying read-model gaps are server-side and tracked separately; this branch is web-only. + +Symmetrically, a name resolved asynchronously is **keyed to the id it was resolved for** and refuses +to overwrite a label that already names a different id — otherwise a slow edit-load read landing +after the user picked something else labels the new selection with the old item's title while the +saved id says otherwise. Keying the render alone stops the mislabelling but still throws away the +newer, correct label, so both halves are needed. + **Scope: Lucene-backed types only.** `GetLibraryBrowseItemsHandler` applies `query` two different ways — as a Lucene clause for media items, and as a plain SQL `LIKE` on `Name` for the collection-family types (Collection / SmartCollection / MultiCollection / RerunCollection / @@ -59,9 +77,27 @@ collection-family types keep the bounded single-page load and the truncation hin `loadAllPages` paths in the other two screens are untouched. The `api.search-allitems-paging` precedent holds: the client bounds itself, the server cap is not raised. +**Replacing a native control means owing its keyboard behaviour.** A `` is +fully keyboard-operable; swapping in a listbox-and-input is an accessibility *regression* unless it +implements the ARIA combobox pattern — `role="combobox"` + `aria-expanded`/`aria-controls`/ +`aria-autocomplete` on the input, ArrowUp/ArrowDown to move a virtual cursor exposed via +`aria-activedescendant`, Enter to commit, Escape to dismiss, options as non-tab-stops +(`tabIndex={-1}`) marked with `aria-selected`. Reset the cursor whenever a new result set lands — +index 2 of the old list is not index 2 of the new one. Note this changes what +`getAllByRole('combobox')` matches in tests: count ` was fully keyboard-operable; replacing it with a custom widget must not + // regress that. Standard ARIA combobox: focus stays on the input, Arrow keys move a virtual + // cursor exposed via aria-activedescendant, Enter commits, Escape dismisses. + describe('keyboard navigation', () => { + const OPTIONS: SearchPickerOption[] = [ + { id: 1, name: 'Alpha' }, + { id: 2, name: 'Beta' }, + { id: 3, name: 'Gamma' } + ]; + + async function openWithResults(onSelect = vi.fn()) { + const search = vi.fn<(query: string) => Promise>().mockResolvedValue(OPTIONS); + renderPicker({ onSelect, search }); + const input = screen.getByLabelText('Movie search') as HTMLInputElement; + fireEvent.focus(input); + fireEvent.change(input, { target: { value: 'a' } }); + await screen.findByRole('listbox', { name: 'Movie results' }); + return { input, onSelect }; + } + + const activeOptionName = (input: HTMLInputElement) => { + const id = input.getAttribute('aria-activedescendant'); + return id === null ? null : document.getElementById(id)?.textContent ?? null; + }; + + it('exposes the combobox contract on the input', async () => { + const { input } = await openWithResults(); + expect(input).toHaveAttribute('role', 'combobox'); + expect(input).toHaveAttribute('aria-expanded', 'true'); + expect(input).toHaveAttribute('aria-autocomplete', 'list'); + expect(input.getAttribute('aria-controls')).toBe( + screen.getByRole('listbox', { name: 'Movie results' }).getAttribute('id') + ); + // Nothing highlighted until the user navigates. + expect(input).not.toHaveAttribute('aria-activedescendant'); + }); + + it('ArrowDown/ArrowUp move the active option and wrap at both ends', async () => { + const { input } = await openWithResults(); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(activeOptionName(input)).toBe('Alpha'); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(activeOptionName(input)).toBe('Beta'); + fireEvent.keyDown(input, { key: 'ArrowUp' }); + expect(activeOptionName(input)).toBe('Alpha'); + // Wrap backwards past the first option... + fireEvent.keyDown(input, { key: 'ArrowUp' }); + expect(activeOptionName(input)).toBe('Gamma'); + // ...and forwards past the last. + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(activeOptionName(input)).toBe('Alpha'); + }); + + it('ArrowUp from nothing highlighted starts at the LAST option', async () => { + const { input } = await openWithResults(); + fireEvent.keyDown(input, { key: 'ArrowUp' }); + expect(activeOptionName(input)).toBe('Gamma'); + }); + + it('Home/End jump to the first and last option', async () => { + const { input } = await openWithResults(); + fireEvent.keyDown(input, { key: 'End' }); + expect(activeOptionName(input)).toBe('Gamma'); + fireEvent.keyDown(input, { key: 'Home' }); + expect(activeOptionName(input)).toBe('Alpha'); + }); + + it('marks the active option with aria-selected', async () => { + const { input } = await openWithResults(); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + const options = screen.getAllByRole('option'); + expect(options.map((option) => option.getAttribute('aria-selected'))).toEqual(['false', 'true', 'false']); + }); + + it('Enter commits the highlighted option and closes the list', async () => { + const { input, onSelect } = await openWithResults(); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'Enter' }); + + expect(onSelect).toHaveBeenCalledWith(2, 'Beta'); + await waitFor(() => expect(screen.queryByRole('listbox', { name: 'Movie results' })).not.toBeInTheDocument()); + }); + + it('Enter with nothing highlighted selects nothing (a form submit is not hijacked)', async () => { + const { input, onSelect } = await openWithResults(); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(onSelect).not.toHaveBeenCalled(); + expect(screen.getByRole('listbox', { name: 'Movie results' })).toBeInTheDocument(); + }); + + it('Escape dismisses the list but keeps the typed text', async () => { + const { input, onSelect } = await openWithResults(); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'Escape' }); + + await waitFor(() => expect(screen.queryByRole('listbox', { name: 'Movie results' })).not.toBeInTheDocument()); + expect(input.value).toBe('a'); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it('drops the highlight when a NEW result set arrives', async () => { + const search = vi + .fn<(query: string) => Promise>() + .mockResolvedValueOnce(OPTIONS) + .mockResolvedValue([{ id: 9, name: 'Zeta' }]); + renderPicker({ search }); + + const input = screen.getByLabelText('Movie search') as HTMLInputElement; + fireEvent.focus(input); + fireEvent.change(input, { target: { value: 'a' } }); + await screen.findByRole('listbox', { name: 'Movie results' }); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(activeOptionName(input)).toBe('Alpha'); + + // Index 0 of the OLD list is not index 0 of the new one — the cursor must reset, not carry + // over onto whatever now happens to sit at that position. + fireEvent.change(input, { target: { value: 'ab' } }); + await waitFor(() => expect(screen.getByRole('option', { name: 'Zeta' })).toBeInTheDocument()); + expect(input).not.toHaveAttribute('aria-activedescendant'); + }); + }); }); diff --git a/web/src/schedules/pickers.tsx b/web/src/schedules/pickers.tsx index a9f405f3a..f2c2451c7 100644 --- a/web/src/schedules/pickers.tsx +++ b/web/src/schedules/pickers.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react'; +import { useEffect, useId, useRef, useState, type KeyboardEvent as ReactKeyboardEvent } from 'react'; import { Check, Search, X } from 'lucide-react'; import { Spinner } from '../components'; import { useIsMountedRef } from '../hooks'; @@ -52,6 +52,9 @@ export function SearchPicker({ // searched yet" — without it the no-matches copy flashes between a keystroke and its debounce. const [resultsQuery, setResultsQuery] = useState(null); const [loading, setLoading] = useState(false); + // -1 = no option highlighted (the ARIA combobox "virtual cursor" is parked). + const [activeIndex, setActiveIndex] = useState(-1); + const listboxId = useId(); const seqRef = useRef(0); // #578: the seq guard drops a stale response but not a post-unmount one. Both are needed. const mountedRef = useIsMountedRef(); @@ -70,6 +73,7 @@ export function SearchPicker({ if (mountedRef.current && seqRef.current === seq) { setResults([]); setResultsQuery(null); + setActiveIndex(-1); setLoading(false); } return; @@ -80,6 +84,9 @@ export function SearchPicker({ if (mountedRef.current && seqRef.current === seq) { setResults(items); setResultsQuery(trimmed); + // A new result set invalidates the highlight — index 2 of the old list is not index 2 + // of the new one. + setActiveIndex(-1); setLoading(false); } }) @@ -87,6 +94,7 @@ export function SearchPicker({ if (mountedRef.current && seqRef.current === seq) { setResults([]); setResultsQuery(trimmed); + setActiveIndex(-1); setLoading(false); } }); @@ -97,6 +105,59 @@ export function SearchPicker({ const trimmedQuery = query.trim(); const noMatches = open && !loading && results.length === 0 && resultsQuery !== null && resultsQuery === trimmedQuery; + const listboxOpen = open && results.length > 0; + + // Keyboard navigation (#651 F6). Replacing a native Response | null; @@ -183,9 +195,11 @@ describe('RerunCollectionsScreen', () => { const typeSelect = (await screen.findAllByRole('combobox'))[0]; fireEvent.change(typeSelect, { target: { value: 'Movie' } }); - // The picker is a typeahead, not a : only the three type/order selects remain as + // native