From 3c1af656445aa4fb3dcd380f81320fef57c0ec50 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:32:18 +0200 Subject: [PATCH 1/7] fix(search): give the facet ordering control an accessible name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `aria-labelledby` was passed through `inputProps`, which MUI v4 spreads onto the aria-hidden native input — where a label names nothing. The focusable node is the `role="button"` display div, reachable only through `SelectDisplayProps`, so the "Sort by" control read as an unnamed button to assistive tech. Same trap `QuerySelect` documents, fixed at this call site. Co-Authored-By: Claude Opus 5 --- .../Search/Layout/PackageFilters.spec.tsx | 43 +++++++++++++++++++ .../Search/Layout/PackageFilters.tsx | 8 +++- 2 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 catalog/app/containers/Search/Layout/PackageFilters.spec.tsx diff --git a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx new file mode 100644 index 00000000000..8ff225b3018 --- /dev/null +++ b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx @@ -0,0 +1,43 @@ +import * as React from 'react' +import { render, cleanup, screen } from '@testing-library/react' +import { describe, it, expect, vi, afterEach } from 'vitest' + +import * as KTree from 'utils/KeyedTree' + +import * as SearchUIModel from '../model' + +import { AvailablePackagesMetaFilters } from './PackageFilters' + +vi.mock('constants/config', () => ({ default: { registryUrl: '' } })) + +const EMPTY_TREE: SearchUIModel.FacetTree = KTree.Tree([]) + +function renderFilters() { + return render( + , + ) +} + +describe('containers/Search/Layout/PackageFilters', () => { + afterEach(cleanup) + + describe('the ordering control', () => { + // MUI v4's Select spreads `inputProps` onto the aria-hidden native input, + // not the focusable display node -- so an aria-labelledby placed there + // names nothing, and the control reads as a bare "button" to a screen + // reader. Same trap QuerySelect documents. + it('names its focusable node', () => { + renderFilters() + expect(screen.getByRole('button', { name: /Sort by/ })).toBeDefined() + }) + }) +}) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.tsx b/catalog/app/containers/Search/Layout/PackageFilters.tsx index d7f884c93d6..38de3fcfaa1 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.tsx @@ -189,7 +189,9 @@ interface AvailablePackagesMetaFiltersProps { fetching: boolean } -function AvailablePackagesMetaFilters({ +// Exported for testing: the ordering control's accessible name lives in how +// props land on MUI's Select internals, which only a render can assert. +export function AvailablePackagesMetaFilters({ className, filtering, facets, @@ -246,7 +248,9 @@ function AvailablePackagesMetaFilters({ disabled={fetching} extents={FACET_ORDERING_VALUES} getOptionLabel={(value) => FACET_ORDERING_LABELS[value]} - inputProps={{ 'aria-labelledby': 'meta-order-label' }} + // On the display node, not `inputProps`: MUI spreads `inputProps` + // onto the aria-hidden native input, where a label names nothing. + SelectDisplayProps={{ 'aria-labelledby': 'meta-order-label' }} onChange={(value) => ordering.set(SearchUIModel.parseFacetOrdering(value, ordering.value)) } From 2cb3f5dafa9ed058621bf65cf2528ad17375d3b9 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:32:57 +0200 Subject: [PATCH 2/7] test(search): use the model's empty facet tree in the spec addresses review finding f28 The new spec built its own `KTree.Tree([])` for the empty facet tree, which the model already exports as `EMPTY_FACET_TREE` and uses at four call sites. A second definition of the same value in a test is a second thing to update. Co-Authored-By: Claude Opus 5 --- .../containers/Search/Layout/PackageFilters.spec.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx index 8ff225b3018..d1c8a6586aa 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx @@ -2,21 +2,21 @@ import * as React from 'react' import { render, cleanup, screen } from '@testing-library/react' import { describe, it, expect, vi, afterEach } from 'vitest' -import * as KTree from 'utils/KeyedTree' - import * as SearchUIModel from '../model' import { AvailablePackagesMetaFilters } from './PackageFilters' vi.mock('constants/config', () => ({ default: { registryUrl: '' } })) -const EMPTY_TREE: SearchUIModel.FacetTree = KTree.Tree([]) - function renderFilters() { return render( Date: Mon, 31 Aug 2026 14:33:27 +0200 Subject: [PATCH 3/7] docs(changelog): entry for the facet ordering accessible name addresses review findings f44, f27 The omnibus entry covered the accessible name and two visibility changes in one line citing one PR. This is the accessible-name half, citing the PR that ships it; the visibility half lands with its own unit. Co-Authored-By: Claude Opus 5 --- catalog/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/catalog/CHANGELOG.md b/catalog/CHANGELOG.md index 99878fca7f5..e2c9295a475 100644 --- a/catalog/CHANGELOG.md +++ b/catalog/CHANGELOG.md @@ -21,6 +21,7 @@ complete sentence without it. ## Changes +- [Fixed] Search sidebar: the facet "Sort by" control has an accessible name — its label used to land on a hidden input, reading as an unnamed button to assistive tech ([#5261](https://github.com/quiltdata/quilt/pull/5261)) - [Fixed] Queries: the query selector announces its label to assistive tech, and no longer claims "Custom" is loaded while its helper text reports the query failed to load ([#5260](https://github.com/quiltdata/quilt/pull/5260)) - [Changed] The `data-products` demo fixture data no longer ships in the bundles a browser downloads on the volumes landing; it loads only when the preview is on ([#5259](https://github.com/quiltdata/quilt/pull/5259)) - [Fixed] Quilt+ URI parsing retains the informational `catalog` field, reads a raw `+` in an unencoded path as a `+` rather than a space, and shares its compatibility corpus with quilt3 ([#5255](https://github.com/quiltdata/quilt/pull/5255)) From ea1dc8f872edfd9ca22829d47ea8f8378adfe3c2 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 17:50:36 +0200 Subject: [PATCH 4/7] fix(search): keep the selected ordering in the control's accessible name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review findings f1, f2, f3, f6, f10, f13, f15 `SelectDisplayProps` is spread over MUI's computed `aria-labelledby` rather than merged into it, and with no `id` on the Select the join collapses to the label alone. Since `aria-labelledby` overrides the display node's contents, the name went from "Name A → Z" to "Sort by:" — the branch traded the ordering for the label instead of getting both. `labelId` + `id` is what `QuerySelect` uses one layer down, and MUI joins them into "Sort by: Name A → Z". It also names the popup listbox and wires the caption's click through to the control, neither of which `SelectDisplayProps` can reach. Both ids now come from `useId`, so the control's name no longer depends on a hardcoded document-global IDREF. The spec asserted `name: /Sort by/`, which matched the broken and the correct wiring alike; it now pins the exact name and the listbox's, and both new tests fail on the previous commit. Co-Authored-By: Claude Opus 5 --- .../Search/Layout/PackageFilters.spec.tsx | 23 +++++++++++-------- .../Search/Layout/PackageFilters.tsx | 16 +++++++++---- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx index d1c8a6586aa..3712383a511 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx @@ -1,12 +1,12 @@ import * as React from 'react' -import { render, cleanup, screen } from '@testing-library/react' +import { render, cleanup, fireEvent, screen } from '@testing-library/react' import { describe, it, expect, vi, afterEach } from 'vitest' import * as SearchUIModel from '../model' import { AvailablePackagesMetaFilters } from './PackageFilters' -vi.mock('constants/config', () => ({ default: { registryUrl: '' } })) +vi.mock('constants/config', () => ({ default: {} })) function renderFilters() { return render( @@ -30,14 +30,19 @@ function renderFilters() { describe('containers/Search/Layout/PackageFilters', () => { afterEach(cleanup) - describe('the ordering control', () => { - // MUI v4's Select spreads `inputProps` onto the aria-hidden native input, - // not the focusable display node -- so an aria-labelledby placed there - // names nothing, and the control reads as a bare "button" to a screen - // reader. Same trap QuerySelect documents. - it('names its focusable node', () => { + describe('the accessible name', () => { + // Both halves, the way `QuerySelect` asserts them: an `aria-labelledby` + // reaching the focusable display node overrides that node's contents, so + // naming it after the caption alone is as lossy as not naming it at all. + it('carries the label and the selected ordering', () => { renderFilters() - expect(screen.getByRole('button', { name: /Sort by/ })).toBeDefined() + expect(screen.queryByRole('button', { name: 'Sort by: Name A → Z' })).not.toBeNull() + }) + + it('names the popup listbox too', () => { + renderFilters() + fireEvent.mouseDown(screen.getByRole('button')) + expect(screen.queryByRole('listbox', { name: 'Sort by:' })).not.toBeNull() }) }) }) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.tsx b/catalog/app/containers/Search/Layout/PackageFilters.tsx index 38de3fcfaa1..008fde78115 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.tsx @@ -8,6 +8,7 @@ import * as FiltersUI from 'components/Filters' import Skeleton from 'components/Skeleton' import * as JSONPointer from 'utils/JSONPointer' import * as NamedRoutes from 'utils/NamedRoutes' +import useId from 'utils/useId' import FilterWidget from '../FilterWidget' import { PACKAGE_FILTER_LABELS } from '../i18n' @@ -199,6 +200,8 @@ export function AvailablePackagesMetaFilters({ fetching, }: AvailablePackagesMetaFiltersProps) { const classes = useAvailablePackagesMetaFiltersStyles() + const orderLabelId = useId() + const orderButtonId = useId() const [expanded, setExpanded] = React.useState(false) const toggleExpanded = React.useCallback(() => setExpanded((x) => !x), []) @@ -239,7 +242,7 @@ export function AvailablePackagesMetaFilters({ on the client-filter path. */} {ordering.offered && (
- + Sort by: @@ -248,9 +251,14 @@ export function AvailablePackagesMetaFilters({ disabled={fetching} extents={FACET_ORDERING_VALUES} getOptionLabel={(value) => FACET_ORDERING_LABELS[value]} - // On the display node, not `inputProps`: MUI spreads `inputProps` - // onto the aria-hidden native input, where a label names nothing. - SelectDisplayProps={{ 'aria-labelledby': 'meta-order-label' }} + // Both ids: MUI joins them into the display div's `aria-labelledby`, + // and the label id alone would override the div's contents, dropping + // the ordering a sighted user reads off the control. `labelId` also + // names the popup listbox and makes the caption click into the + // control -- neither is reachable through `SelectDisplayProps`, and + // `inputProps` lands on the aria-hidden native input. + labelId={orderLabelId} + id={orderButtonId} onChange={(value) => ordering.set(SearchUIModel.parseFacetOrdering(value, ordering.value)) } From 5062b191845e884cdbad94fbfac981185aa8acb5 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 17:50:42 +0200 Subject: [PATCH 5/7] docs(changelog): correct the premise of the ordering entry addresses review finding f5 The control was not unnamed before the fix: a `div[role=button]` takes its name from its contents, so it announced the selected ordering and nothing about what the control does. "Has an accessible name" also overstated what shipped, since the name it gained dropped that ordering. Co-Authored-By: Claude Opus 5 --- catalog/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/catalog/CHANGELOG.md b/catalog/CHANGELOG.md index e2c9295a475..2cbb430deb3 100644 --- a/catalog/CHANGELOG.md +++ b/catalog/CHANGELOG.md @@ -21,7 +21,7 @@ complete sentence without it. ## Changes -- [Fixed] Search sidebar: the facet "Sort by" control has an accessible name — its label used to land on a hidden input, reading as an unnamed button to assistive tech ([#5261](https://github.com/quiltdata/quilt/pull/5261)) +- [Fixed] Search sidebar: the facet "Sort by" control announces what it is — its label used to land on a hidden input, leaving assistive tech to read the control as its current ordering and nothing more ([#5261](https://github.com/quiltdata/quilt/pull/5261)) - [Fixed] Queries: the query selector announces its label to assistive tech, and no longer claims "Custom" is loaded while its helper text reports the query failed to load ([#5260](https://github.com/quiltdata/quilt/pull/5260)) - [Changed] The `data-products` demo fixture data no longer ships in the bundles a browser downloads on the volumes landing; it loads only when the preview is on ([#5259](https://github.com/quiltdata/quilt/pull/5259)) - [Fixed] Quilt+ URI parsing retains the informational `catalog` field, reads a raw `+` in an unencoded path as a `+` rather than a space, and shares its compatibility corpus with quilt3 ([#5255](https://github.com/quiltdata/quilt/pull/5255)) From 209f78acf93b74809e88aa9ecf5c62739fc2541d Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 18:07:50 +0200 Subject: [PATCH 6/7] test(search): fail with the name the control actually has `queryByRole(...).not.toBeNull()` reports "expected null not to be null" and nothing else, which for an accessible-name regression is the one thing the failure should not withhold. `getByRole` prints every role and accessible name in the tree: against the pre-fix wiring it now names the defect outright -- `button` / `Name "Sort by:"` -- instead of leaving the reader to go find out what the control is called. The comment keeps only the constraint the assertion depends on: the caption alone matches `/Sort by/`, so loosening the name to a substring passes on wiring that drops the ordering. The mechanism behind that is stated where a reader lands on it, in the component. Co-Authored-By: Claude Opus 5 --- .../app/containers/Search/Layout/PackageFilters.spec.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx index 3712383a511..0e951aea976 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.spec.tsx @@ -31,18 +31,17 @@ describe('containers/Search/Layout/PackageFilters', () => { afterEach(cleanup) describe('the accessible name', () => { - // Both halves, the way `QuerySelect` asserts them: an `aria-labelledby` - // reaching the focusable display node overrides that node's contents, so - // naming it after the caption alone is as lossy as not naming it at all. + // The exact name, not a substring: the caption alone matches /Sort by/ too, + // so a loosened assertion passes on wiring that drops the ordering. it('carries the label and the selected ordering', () => { renderFilters() - expect(screen.queryByRole('button', { name: 'Sort by: Name A → Z' })).not.toBeNull() + expect(screen.getByRole('button', { name: 'Sort by: Name A → Z' })).toBeTruthy() }) it('names the popup listbox too', () => { renderFilters() fireEvent.mouseDown(screen.getByRole('button')) - expect(screen.queryByRole('listbox', { name: 'Sort by:' })).not.toBeNull() + expect(screen.getByRole('listbox', { name: 'Sort by:' })).toBeTruthy() }) }) }) From 0dcf150d72417e2a28056996b5d16f7430539328 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 18:07:57 +0200 Subject: [PATCH 7/7] refactor(search): trim the ordering comment to the constraint Six lines where four carry a constraint. The `inputProps` clause documented what this branch moved away from -- `inputProps` appears nowhere in the file now, so it constrains nothing a reader can act on -- and the caption-click behaviour is not why either prop is here. What is left is what a reader has to know before touching the two props: `labelId` reaches the focusable node and names the popup, and `id` cannot be dropped, matching how `QuerySelect` states the same trap. Co-Authored-By: Claude Opus 5 --- .../app/containers/Search/Layout/PackageFilters.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.tsx b/catalog/app/containers/Search/Layout/PackageFilters.tsx index 008fde78115..17d16e8c0f6 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.tsx @@ -251,12 +251,10 @@ export function AvailablePackagesMetaFilters({ disabled={fetching} extents={FACET_ORDERING_VALUES} getOptionLabel={(value) => FACET_ORDERING_LABELS[value]} - // Both ids: MUI joins them into the display div's `aria-labelledby`, - // and the label id alone would override the div's contents, dropping - // the ordering a sighted user reads off the control. `labelId` also - // names the popup listbox and makes the caption click into the - // control -- neither is reachable through `SelectDisplayProps`, and - // `inputProps` lands on the aria-hidden native input. + // `labelId` reaches the focusable display div and names the popup + // listbox; `id` must come with it, since MUI joins the two into + // `aria-labelledby="labelId buttonId"` and `labelId` alone would + // override the div's contents, dropping the ordering from the name. labelId={orderLabelId} id={orderButtonId} onChange={(value) =>