From fa491641438c070d1719602241c7910104ac8c6a Mon Sep 17 00:00:00 2001 From: Tudor Date: Fri, 2 Oct 2026 09:40:29 +0100 Subject: [PATCH] fix(search): keep the filter sheet usable while a change lands The sheet's controls were disabled while a filter change navigated, and a control disabled under focus drops it to , out of the dialog. They now stay enabled, with aria-busy on the sheet instead. The disabling had also been covering a race: updateURL built from useSearchParams, which only catches up once a navigation lands, so a second change made before then undid the first. It now builds on the URL the navigation in flight is heading to. The sheet also closes if the screen widens past phone width while it is open, so its selects and the desktop row's are never both showing. Co-Authored-By: Claude Opus 5.5 --- .../components/FilterSheetPending.test.tsx | 80 +++++++++++++++++++ nextjs-app/components/FilterBar.tsx | 42 ++++++++-- nextjs-app/components/FilterSheet.module.css | 4 - nextjs-app/components/FilterSheet.tsx | 6 +- 4 files changed, 118 insertions(+), 14 deletions(-) create mode 100644 nextjs-app/__tests__/components/FilterSheetPending.test.tsx diff --git a/nextjs-app/__tests__/components/FilterSheetPending.test.tsx b/nextjs-app/__tests__/components/FilterSheetPending.test.tsx new file mode 100644 index 0000000..19dd2e7 --- /dev/null +++ b/nextjs-app/__tests__/components/FilterSheetPending.test.tsx @@ -0,0 +1,80 @@ +import { act, fireEvent, render, screen, within } from '@testing-library/react'; +import { FilterBar } from '@/components/FilterBar'; + +/* + * While a filter change is navigating, the sheet's controls stay enabled: a + * control disabled under the user's focus drops it to , and a keyboard or + * screen-reader user is thrown out of the sheet after every change. A second + * change made before the first lands must build on the first, not on the URL + * useSearchParams still reports. + */ + +// Every transition stays pending, as a slow server render would. +jest.mock('react', () => ({ + ...jest.requireActual('react'), + useTransition: () => [true, (fn: () => void) => fn()], +})); + +const params = new URLSearchParams('postcode=SW196AR&radius=1'); +const push = jest.fn(); +jest.mock('next/navigation', () => ({ + useSearchParams: () => params, + usePathname: () => '/', + useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }), +})); +jest.mock('@/lib/analytics', () => ({ track: jest.fn() })); + +const filters = { + local_authorities: ['Wandsworth'], school_types: ['Community school'], years: [], + phases: ['Primary', 'Secondary'], genders: [], admissions_policies: [], +}; + +const pushedParams = () => new URLSearchParams(push.mock.calls.at(-1)![0].split('?')[1]); + +const openSheet = () => { + fireEvent.click(screen.getByRole('button', { name: /^Filters/ })); + return screen.getByRole('dialog', { name: 'Filters' }); +}; + +beforeEach(() => push.mockClear()); + +it('keeps the sheet usable, and says it is busy, while a change lands', () => { + render(); + const sheet = openSheet(); + for (const name of ['Phase', 'School type', 'Local authority']) { + expect(within(sheet).getByRole('combobox', { name })).toBeEnabled(); + } + expect(within(sheet).getByRole('radio', { name: 'Within 3 miles' })).toBeEnabled(); + expect(sheet.querySelector('[aria-busy="true"]')).not.toBeNull(); +}); + +it('builds a second change on the first, not on the URL it has not reached', () => { + render(); + const sheet = openSheet(); + fireEvent.change(within(sheet).getByRole('combobox', { name: 'Phase' }), { target: { value: 'primary' } }); + fireEvent.change(within(sheet).getByRole('combobox', { name: 'School type' }), + { target: { value: 'Community school' } }); + const next = pushedParams(); + expect(next.get('phase')).toBe('primary'); + expect(next.get('school_type')).toBe('Community school'); + expect(next.get('postcode')).toBe('SW196AR'); +}); + +describe('a screen that widens past phone width', () => { + let listeners: ((e: { matches: boolean }) => void)[] = []; + beforeEach(() => { + listeners = []; + window.matchMedia = jest.fn().mockImplementation((query: string) => ({ + matches: true, media: query, + addEventListener: (_: string, l: (e: { matches: boolean }) => void) => listeners.push(l), + removeEventListener: jest.fn(), + })); + }); + + it('closes the sheet, leaving the desktop row as the only filters', () => { + render(); + openSheet(); + act(() => listeners.forEach((l) => l({ matches: false }))); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/components/FilterBar.tsx b/nextjs-app/components/FilterBar.tsx index f10ab28..036b7fa 100644 --- a/nextjs-app/components/FilterBar.tsx +++ b/nextjs-app/components/FilterBar.tsx @@ -247,9 +247,21 @@ export function FilterBar({ return () => document.removeEventListener("keydown", handleKeyDown); }, []); + /* + * Where the navigation in flight is headed. useSearchParams only catches up + * once a navigation lands, so a second change made before then (the phone + * sheet stays usable while one lands) would otherwise be built on the old + * URL and undo the first. Dropped once nothing is pending, when + * searchParams is current again. + */ + const pendingQueryRef = useRef(null); + useEffect(() => { + if (!isPending) pendingQueryRef.current = null; + }, [isPending]); + const updateURL = useCallback( (updates: Record) => { - const params = new URLSearchParams(searchParams); + const params = new URLSearchParams(pendingQueryRef.current ?? searchParams); Object.entries(updates).forEach(([key, value]) => { if (value && value !== "") { @@ -260,6 +272,7 @@ export function FilterBar({ }); params.delete("page"); + pendingQueryRef.current = params.toString(); startTransition(() => { router.push(`${pathname}?${params.toString()}`); @@ -324,6 +337,7 @@ export function FilterBar({ const handleClearFilters = () => { setOmniValue(""); + pendingQueryRef.current = ""; startTransition(() => { router.push(pathname); }); @@ -367,6 +381,18 @@ export function FilterBar({ * "More filters" opened did not contain either of them. */ const [sheetOpen, setSheetOpen] = useState(false); + // The sheet is phone furniture: if the screen widens past phone width while + // it is open (rotation, a resized window), close it, or its selects and the + // desktop row's would both be showing. + useEffect(() => { + if (!sheetOpen || typeof window.matchMedia !== "function") return; + const phone = window.matchMedia("(max-width: 640px)"); + const onChange = (e: { matches: boolean }) => { + if (!e.matches) setSheetOpen(false); + }; + phone.addEventListener("change", onChange); + return () => phone.removeEventListener("change", onChange); + }, [sheetOpen]); const values: Record = { phase: currentPhase, school_type: currentType, @@ -401,6 +427,8 @@ export function FilterBar({ * Each select is built here once and drawn twice: as a pill in the desktop * row or panel, and as a full-width labelled field in the phone sheet. Only * one copy is ever showing; the sheet's is mounted only while it is open. + * The sheet's stay enabled while a change lands: disabling the control + * under the user's focus would drop focus to , out of the dialog. */ type Look = "pill" | "panel" | "sheet"; const selectClass = (look: Look, value: string) => @@ -413,7 +441,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("phase", e.target.value)} className={selectClass(look, currentPhase)} aria-label="Phase" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {phaseOptions.map((p) => ( @@ -432,7 +460,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("school_type", e.target.value)} className={selectClass(look, currentType)} aria-label="School type" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {typeOptions.map((type) => ( @@ -451,7 +479,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("local_authority", e.target.value)} className={selectClass(look, currentLA)} aria-label="Local authority" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {laOptions.map((la) => ( @@ -470,7 +498,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("gender", e.target.value)} className={selectClass(look, currentGender)} aria-label="Gender" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {genderOptions.map((g) => ( @@ -489,7 +517,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("has_sixth_form", e.target.value)} className={selectClass(look, currentHasSixthForm)} aria-label="Sixth form" - disabled={isPending} + disabled={isPending && look !== "sheet"} > @@ -505,7 +533,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("admissions_policy", e.target.value)} className={selectClass(look, currentAdmissionsPolicy)} aria-label="Admissions" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {admissionsPolicyOptions.map((p) => ( diff --git a/nextjs-app/components/FilterSheet.module.css b/nextjs-app/components/FilterSheet.module.css index 73491be..95c24db 100644 --- a/nextjs-app/components/FilterSheet.module.css +++ b/nextjs-app/components/FilterSheet.module.css @@ -74,10 +74,6 @@ box-shadow: inset 0 0 0 2px var(--brand); } -.segment input:disabled { - cursor: progress; -} - .footer { display: flex; align-items: center; diff --git a/nextjs-app/components/FilterSheet.tsx b/nextjs-app/components/FilterSheet.tsx index 6229a09..df72421 100644 --- a/nextjs-app/components/FilterSheet.tsx +++ b/nextjs-app/components/FilterSheet.tsx @@ -61,7 +61,6 @@ export function FilterSheet({ type="button" className={`btn btn-tertiary ${styles.clearAll}`} onClick={onClearAll} - disabled={isPending} > Clear all @@ -76,7 +75,9 @@ export function FilterSheet({ } > -
+ {/* Controls stay enabled while a change lands, so focus is never + dropped out of the dialog; aria-busy says the results are updating. */} +
{radius !== null && (
@@ -95,7 +96,6 @@ export function FilterSheet({ value={r} checked={r === radius} onChange={() => onRadiusChange(r)} - disabled={isPending} aria-label={`Within ${radiusLabel(r)}`} /> {r} mi