Repository navigation
fix(ui-web): state the model picker's name and checked row to a screen reader - #718
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the model chip must only report expanded for the picker it opened.
I found one introduced ARIA state error, marked inline. I reviewed the full diff, the model store and both composer/settings opener paths, relevant history, backward-compatibility behavior, test changes, and the repository's AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and Web UI architecture terms. I found no weakened tests or other merge-blocking issue.
Verification: npm test (207 files, 2870 tests passed); focused ModelChip/ModelPicker run (2 files, 70 tests passed); npm run type-check; npm run gen:check; npm run --prefix ui-tui lint:i18n; and the source-language target's underlying uv command all passed. The make wrapper itself was unavailable in this environment.
…n reader The picker carried role="dialog" with no accessible name, its rows marked the current model with a tick glyph alone, and #modelChip advertised aria-haspopup without aria-expanded -- so a reader using the markup rather than the pixels was told a dialog had opened, could not hear which of it was chosen, and could not tell whether the chip's popover was up. The popover is named after the slot that opened it, or after the word for a model. Rows become radios carrying aria-checked, one radiogroup per account plus one for Recent, because the same model is listed under both and one set may hold one checked member. The tick is hidden from the accessibility tree now that the state is said, and the show-all row stays out of the set it sits in. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Reverting the source left this one green while the other seven went red: it asserted only that the show-all row has no role and no aria-checked, which is equally true of a picker that marks no row at all. It shares the `row` class with the radios, so the negative half means something only next to the positive one. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Review finding on the first head. `isOpen()` answers "a model picker is up somewhere", and one store serves every opener: the composer chip with no anchor, and each settings role pill against its own button. So opening a role such as Planner model from the settings dialog lit up the composer chip too, handing a screen reader a control and a state that do not belong together. The store already knows which control it was opened from -- it keeps the opener as `at.host` -- so it answers the narrower question itself through `openedFrom`, rather than the chip filtering a global. The chip asks about its own node. Reverting the predicate to `isOpen()` reddens the added case and nothing else. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
c46def8 to
60d9142
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The prior blocker is fixed: the store now answers whether the picker belongs to a specific opener, the chip asks with its own node, and the regression test covers a settings pill opening the shared picker. I reviewed the full PR diff and fix delta, relevant composer/settings callers, store lifecycle, rebase history and backward compatibility, test changes, and the repository and Web UI architecture rules. The two original commits are patch-identical after the rebase, tests were not weakened, and I found no new issue worth raising.
Verification: the focused ModelChip/ModelPicker/Dock/Roles run passed 4 files and 115 tests; npm run type-check, npm run gen:check, and npm run --prefix ui-tui lint:i18n passed. The full UI run passed 2,888 of 2,889 tests but timed out in the unrelated state/session/registry case with a late no gateway installed rejection; rerunning that file alone passed all 28 tests.
Summary
Three accessibility gaps in the model picker, all in what the markup
says rather than in what it draws.
.mpickcarriedrole="dialog"with no accessible name, so a readerwas told something had opened without being told what. Model rows
marked the current model with a tick glyph alone, so which one is
chosen was invisible to a screen reader.
#modelChipadvertisedaria-haspopup="true"with noaria-expanded, while its three siblingchips carried both.
The popover is named after the slot that opened it, or after the word
for a model when nothing did. Rows are radios carrying
aria-checked,grouped one radiogroup per account plus one for Recent. Per account
rather than one set for the whole list: the same model is listed under
Recent and under its account, both are marked, and one set may hold one
checked member. The tick is hidden from the accessibility tree now that
the state is said in the markup, and the show-all row stays out of the
set it sits in, since it reveals more radios rather than being one.
One visual consequence, deliberate and worth knowing before reading a
diff that looks attribute-only.
src/styles/page.cssselects on.chip[aria-expanded="true"]and on.under .chip[aria-expanded], sogiving
#modelChipthe attribute repaints it while its popover is up.Measured in Chrome on the built page:
The chip now looks engaged exactly as
#permChipand#wdChipalreadydid, which is the wanted outcome;
coloris unchanged, pinned by.under .chip.model. No rule matches the addedrole,aria-checkedor
aria-hidden:.mpickis not inside a.menuand the rows are notinside
.model-seg, verified in the same browser run by reading::beforeon a checked row ascontent: none.A third commit answers the review on the first head.
aria-expandedwas derived from
isOpen(), which answers "a model picker is upsomewhere" -- and one store serves every opener: this chip with no
anchor, and each settings role pill against its own button
(
features/settings/providers/Roles.tsx). Opening a role such asPlanner model from the settings dialog therefore lit up the composer
chip as well, handing a screen reader a control and a state that do not
belong together. The store already keeps the opener as
at.host, so itanswers the narrower question itself through
openedFromrather thanthe chip filtering a global, and the chip asks about its own node.
The second commit fixes a test this branch had written. Reverting the
source left seven of the eight new assertions red and one green: the
show-all case asserted only that the row carries no
roleand noaria-checked, which is equally true of a picker that marks no row atall. It is asserted against its siblings now, and the expected set is
read off the source's own
FOLD, so the negative half rests on apositive one.
Type
Verification
Run from
ui-web/unless noted. This box runs node 26, whoseexperimental
globalThis.localStorageshadows happy-dom's and killsevery suite that clears it, so the suite runs need
NODE_OPTIONS=--localstorage-file=<path>and--no-file-parallelism.Neither is a repo change; CI runs node 22 and needs neither.
The 2 failures are
features/desk/store.test.tsandfeatures/desk/palette.test.ts, bothwhen storage refuses, andneither is this branch. They are unevaluable on a node 26 box either
way: with the flag, storage works when the test needs it to throw;
without it, the file dies in
beforeEach. The review of the first headran the same suite on another machine and reported 2870 of 2870 passed,
which settles them as this box's artifact rather than a repository-wide
failure.
Everything above was run with the branch rebased onto
mainas itstood a few commits ago;
mainhas moved since, in commits that touchneither file this branch changes. Left un-rebased on purpose, since the
merge ref is what CI measures and the branch would be stale again
before it was reviewed.
Repository gates, from the repo root:
Risk
User-visible change:
#modelChipgains the open-state paint describedabove. It is one chip matching three siblings, and it is the only pixel
difference in the branch. Everything else is markup a screen reader
reads and a sighted reader does not.
No behaviour change to what the picker offers or what choosing does.
Rollback is reverting the commits; nothing persists and no stored shape
changes.
Related Issues
N/A