Catalog: give the facet "Sort by" control an accessible name (5217 stack 3/9)

[nl0]

Description

The search sidebar's "Sort by" control has no accessible name. Its
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. A
keyboard or screen-reader user browsing package metadata facets hears an unnamed
button where a sighted user reads "Sort by".

Accessible name only. The visibility of the same control — it disappearing while
you type in "Find metadata" — is a different concern in a different file, and it
is PR 4.

Review findings addressed

• f28 — the new spec reinvented the empty facet tree as a local
  KTree.Tree([]) when the model already exports EMPTY_FACET_TREE and uses it
  at four call sites.

Recorded:

• f18 — the same MUI v4 accessible-name problem is fixed twice in this
  stack, in two different ways: SelectDisplayProps here, labelId in
  PR 2. Both are correct for their
  call site; the inconsistency is the finding, and a third case is still unfixed at
  Queries/Athena/Workgroups.tsx:64-66. Unifying this inside the shared Select
  wrapper is a follow-up — neither PR should reach across into the other's file to
  do it.

Verification

cd catalog && npx vitest run app/containers/Search/Layout/PackageFilters.spec.tsx

The assertion is getByRole('button', { name: /Sort by/ }), which fails against
the inputProps version — the name has to be on the focusable node for the query
to resolve.

Position in the stack

PR 3 of 9, based on
stack/5217-2-query-select-accessible-name.

Part of the split of #5217 asked for in
f27. This
unit and PR 2 are the two uncontested accessibility fixes and sit early
deliberately. PR 4 stacks directly above this one because its own fix corrects a
stale comment in PackageFilters.tsx, which this PR owns.

TODO

• Unit tests
• Security: Confirm that this change meets security best practices and does not violate the security model
• Open: Confirm that this change doesn't break the Open variant
• Changelog entry

🤖 Generated with Claude Code

Greptile Summary

This PR gives the search sidebar’s facet ordering control an accessible name by applying aria-labelledby to MUI’s focusable Select display node.

• Moves the label reference from the hidden native input to SelectDisplayProps.
• Adds focused regression coverage that queries the control by role and accessible name.
• Documents the accessibility fix in the Catalog changelog.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The shared Select wrapper forwards SelectDisplayProps to MUI, the referenced label exists, and the regression test targets the actual focusable button by its accessible name.

Important Files Changed

Filename	Overview
catalog/app/containers/Search/Layout/PackageFilters.tsx	Correctly forwards the existing label to the focusable MUI Select display node and exports the component for render-level testing.
catalog/app/containers/Search/Layout/PackageFilters.spec.tsx	Adds focused coverage verifying that assistive technologies can identify the ordering button by its “Sort by” label.
catalog/CHANGELOG.md	Accurately records the user-facing accessibility correction.

_{Reviews (1): Last reviewed commit: "docs(changelog): entry for the facet ord..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 35.16%. Comparing base (aee6db6) to head (0dcf150).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5261      +/-   ##
==========================================
+ Coverage   34.92%   35.16%   +0.23%     
==========================================
  Files         741      741              
  Lines       23804    23806       +2     
  Branches     6429     6429              
==========================================
+ Hits         8314     8371      +57     
+ Misses      13746    13692      -54     
+ Partials     1744     1743       -1     

Flag	Coverage Δ
catalog	35.16% <100.00%> (+0.23%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nl0]

Local code review — findings and dispositions

Reviewed as this layer's diff (stack/5217-2-query-select-accessible-name..stack/5217-3-search-ordering-accessible-name), not against master. Four findings accepted, one recorded, two declined.

Greptile

Its summary sits in the PR description (that is where it delivers here), confidence 5/5, "no actionable defects identified", on commit aaefcdf1. That commit's three files are byte-identical to the state the description describes, so its verdict — and its file notes, which say the label is forwarded "to the focusable MUI Select display node" via SelectDisplayProps — predate finding 1 below and stand on code that is no longer on the branch. It left no reviews and no inline comments, so there is nothing to accept, decline or reply to.

It has not re-run since the commits below were pushed, and going by the rest of the stack it will not: PR 1, PR 2 and PR 4 all had commits land after their review — PR 1 by fast-forward, the other two by force-push — and all three still report Reviews (1) against the commit they were reviewed at. So the wiring that actually ships here has not been through it.

1. Accepted — the first fix named the control by taking the ordering away (731f4685)

SelectDisplayProps is spread after MUI's computed aria-labelledby, so the label id landed as the attribute's whole value; and because aria-labelledby wins over an element's contents, the name became the label alone. The branch traded one omission for the other.

labelId + id is the wiring QuerySelect uses one layer down. MUI builds the attribute as [labelId, buttonId] and only emits buttonId when the Select is given an id (or a name) — buttonId = SelectDisplayProps.id || (name ? 'mui-component-select-' + name : undefined) — so labelId on its own would have repeated the same omission, and the two together give Sort by: Name A → Z. labelId also names the popup listbox and wires the caption's click through to the control, neither reachable through SelectDisplayProps. Both ids now come from useId rather than the hardcoded meta-order-label, which nothing else in the tree referenced.

Read off the role="button" node with the default ordering selected, each wiring installed in turn:

wiring	accessible name
inputProps (before the branch)	Name A → Z — label missing
SelectDisplayProps	Sort by: — ordering missing
labelId + id	Sort by: Name A → Z

One consequence worth noting: this is now the same fix as PR 2, so f18 no longer holds inside the stack. The third case at Queries/Athena/Workgroups.tsx:60-61 is still unfixed, and unifying this inside the shared Select wrapper is still the follow-up.

2. Accepted — the changelog entry mis-stated the before-state (d53518df)

A div[role="button"] takes its name from its contents, so the control was never unnamed: it announced the selected ordering and nothing about what the control does. "Has an accessible name" also overstated what the first commit shipped, since the name it gained had dropped that ordering.

3. Accepted — the spec withheld the one thing its failure needed to say (8e6ce6b2)

queryByRole(...).not.toBeNull() fails with expected null not to be null, which for an accessible-name regression is precisely the information the failure should not withhold. getByRole prints every role and accessible name in the tree, so against the pre-fix wiring the same failure now names the defect outright — button / Name "Sort by:" — instead of leaving the next reader to go find out what the control is actually called. It is also the house pattern (screen.getByRole('button', { name })).

4. Accepted — comment sweep (9cfe326d)

Six lines above labelId/id where four carry a constraint. The inputProps clause documented what the 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 there. What is left is what someone has to know before touching the two props, stated the way QuerySelect states the same trap. The spec comment retelling that mechanism went too, keeping only the guard on the assertion: the caption alone matches /Sort by/, so loosening the name to a substring passes on wiring that drops the ordering.

The CHANGELOG entry was read in the same pass and left alone. d53518df had already corrected its premise, and the mechanism clause it keeps — "its label used to land on a hidden input" — is the register this file already writes in: neighbouring entries name bundles, adapters and compatibility corpora. What it says about before and after is accurate as it stands.

5. Recorded, not changed — two components named AvailablePackagesMetaFilters

The presentational one in Layout/PackageFilters.tsx and the render-prop data wrapper in Search/model.ts; the new spec has both in scope, one module-qualified. The name is pre-existing — exporting made it reachable, not ambiguous — and renaming either reaches outside this layer.

6. Declined — the listbox test's unnamed getByRole('button')

Only one button renders in this fixture (MoreButton needs hidden facets), and naming it there would re-assert what the first test exists to assert. If a second button ever renders unconditionally, found multiple elements points straight at it.

7. Declined — the spec's local afterEach(cleanup) is redundant

True: setup-vitest.ts registers it globally and documents why. But 47 specs under app/ register it locally as well, so dropping it here buys agreement with the setup file at the price of agreement with every sibling. Not this layer's to change.

The PR description

Still describes the SelectDisplayProps approach and records f18 as an open inconsistency inside the stack. Both are superseded by the commits above, so it needs a pass.

Verification

npx vitest run (1686 pass, 1 skipped, across 178 files), npx vitest run app/containers/Search (109 pass), npx tsc --noEmit, and oxfmt --check / oxlint on the touched files. Reverting only the component to f974f5d6 fails both new tests, with the accessible name the control actually has printed in the failure.

Layers 4–9 were rebased onto the updated chain in order and force-pushed with --force-with-lease; chain ancestry re-verified, and every PR in the stack still shows only its own layer. Each later layer's own added and removed lines are byte-identical after the rebase — only blob ids and hunk line numbers move, since this layer edits lines above them in the same files. The one conflict, in CHANGELOG.md against PR 4's entry, was resolved by keeping PR 4's own line above this layer's corrected one, leaving PR 4's diff a single added line as before.