Repository navigation
Catalog UI 5/5: keyboard and screen-reader accessibility pass (#19) - #36
Conversation
- Visible focus: a :focus-visible outline scoped to the plugin-manager panel, so keyboard travel has a position the dark theme cannot eat. - Screen-reader labels: the install plan note is a labelled live region (selection state, required-by explanations, pruned entries, download size); install outcome, browse status, filter state and download progress are polite live regions; the progress bar and both placeholder-only inputs get accessible names; the install list is a labelled group. - Disabled entries expose their reason to AT, not only visually: every disabled checkbox carries a title, incompatibility reasons and resolver locks are visible lines wired through aria-describedby, and a lock says who requires the plugin and how to release it. - Tests: describedby/lock-line/title behaviour per row state, and static pins on the fragment's a11y markup (no inline handlers, no positive tabindex).
# Conflicts: # src/renderer/plugin-manager/screen.js
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two findings strike at the goals this pass sets for itself: the new live regions are populated while still display:none, so the first — and every re-revealed — announcement is dropped by NVDA, JAWS and VoiceOver; and the unconditional aria-describedby both re-announces text the wrapping <label> already puts in the accessible name and, per accname precedence, suppresses title as the description. Details inline.
Reviewed changes the full keyboard and screen-reader pass for the catalog UI — focus visibility, live-region wiring and disabled-state explanations across the fragment and screen script, with new pins in the view suite.
- Visible focus — a
#plugin-manager-panel-scoped:focus-visibleoutline rule, so keyboard travel through the dark panel has a visible position. - Live regions and labels (
screen.html) —aria-live/roleon the install-plan note, install outcome, browse status, filter state and download progress text;aria-labelon<progress>and the git URL input;role="group"on the install list. - Disabled-reason exposure (
screen.js) —titleplusaria-describedbyon every disabled checkbox; the incompatibility line gains a stable id; a new lock element rewritten on every resolve names who required the entry and how to release it. - Tests — four new a11y tests (disabled reason, lock explanation, static-reason titles, markup wiring, no inline handlers or positive
tabindex) plussetAttributestubs in both DOM shims.
ℹ️ Nitpicks
- The map-shape comment at
src/renderer/plugin-manager/screen.js:184still documents{box, entry, staticDisabled, incompatible, incompatibleReason}; this PR changed the stored shape tostaticReasonandlockEl. tests/renderer-html-escaping.test.js— this PR had to extend its DOM shim, but noci.ymlstep runs that suite (only thenpm testglob does, and no workflow runsnpm test), so it never executes in CI. Pre-existing gap, worth closing while the file is touched.
big-pickle (free) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The delta since my previous review is clean — no new findings. The two open findings from that review (live-region reveal timing and the
aria-describedby/titleinteraction on the wrapping<label>) remain outstanding and are not repeated here.
Reviewed changes what changed since the prior pullfrog review (b417ce2 → e13e4cb) — the merge of the base branch's reworked resolver mirror, with this branch's accessibility edits re-applied on top:
- Lock-line rendering moved into the reworked branches (
mirrorPlanOntoRows) — the pruned and unlocked branches now clear the lock element and restorebox.titleto the static reason, and a lock line renders only for entries the adoption loop pulls into the selection; on-disk entries (bundled/held) are deliberately left without a "Locked into the plan" line. - Stale-lock-line comment resolved — the third inline comment from my previous review no longer applies: a visible lock line now implies its entry was adopted into the selection, so the empty-selection early return can no longer strand it.
- Tests — the a11y tests are unchanged against this head and pass:
node --test tests/plugin-catalog-view.test.js48/48,node --test tests/renderer-html-escaping.test.js7/7.
ℹ️ Nitpicks
- The branch that suppresses the lock line for already-on-disk entries (
src/renderer/plugin-manager/screen.js:572-578) has no test pinning it — the a11y suite only exercises the adoptable-dependency case, so a regression there would silently restore a "Locked into the plan" line over a checkbox that was never selectable.
big-pickle (free) ｜ 𝕏
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit ee70357)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ee70357)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit e13e4cb)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Note: reviewed against head Reviewed by free · Input: 173.8K · Output: 6.7K · Cached: 103.7K |
|
Reviewed review #5448998607 on PR #36. The two open findings from the previous review (live-region reveal timing and aria-describedby/title interaction) remain as noted in the follow-up review; the stale lock-line case is addressed by calling Task list (5/5 completed)
|
…sible, aria-describedby only for visible lines, stale lock cleared on empty selection
There was a problem hiding this comment.
Important
The aria-describedby scoping and the stale-lock clearing from the first review are fixed cleanly, but the live-region fix only covers the very first announcement. The plan note is re-hidden when the selection empties, and the progress text lives in a wrapper that is re-hidden between runs; both are then repopulated in the same task that unhides them again, so those announcements are still dropped.
Reviewed changes the ee70357 address-review delta — live-region visibility, aria-describedby scoping, and the empty-selection lock cleanup.
- Live-region visibility —
hiddendropped from#pm-catalog-depsand#pm-catalog-msg, so their first content lands in an already-rendered region. aria-describedbyscoping — set only where it points at real text (pm-row-reason-*for an incompatible row,pm-row-lock-*while locked), removed on release, sotitleregains its role as the fallback description.- Empty-selection cleanup — the empty branch now calls
mirrorPlanOntoRows(null), clearing every lock line and lock-derivedtitle. - Test shim —
removeAttributeadded to both DOM shims.
⚠️ The live-region fix only covers the first reveal
#pm-catalog-deps is re-hidden whenever the last plugin is unticked (screen.js:601), and #pm-catalog-progress-text lives inside #pm-catalog-progress-wrap, which is re-hidden after every install (screen.js:726). Both then get their text written in the same task that removes hidden again (screen.js:647-650, screen.js:197-201), so the next announcement is once more a populate-then-unhide reveal that NVDA, JAWS and VoiceOver drop. Only #pm-catalog-msg — never re-hidden — is fully fixed. The first-review thread on this stays open.
Technical details
# Live-region fix is partial: re-reveals still take the display:none path
## Affected sites
- src/renderer/plugin-manager/screen.js:600-606 — empty selection re-adds `hidden` to `#pm-catalog-deps` and clears it. The `mirrorPlanOntoRows(null)` addition here is correct and clears the rows.
- src/renderer/plugin-manager/screen.js:647-650 — for a non-empty selection the text is written before `hidden` is removed, so the first announcement after the region was hidden is a populate-then-unhide reveal.
- src/renderer/plugin-manager/screen.js:195-201 — `renderCatalogProgress` removes `hidden` from `#pm-catalog-progress-wrap` and writes `#pm-catalog-progress-text` in the same task; the wrapper is re-hidden after each install (screen.js:726), so the first progress update of every run is dropped.
- src/renderer/plugin-manager/screen.html:78,85 — the two regions involved.
## Required outcome
- Every announcement the install-plan and progress regions make must reach assistive tech, not just the first one after page load.
## Suggested approach
- Keep `#pm-catalog-deps` and `#pm-catalog-progress-text` rendered at all times (never re-add `hidden`); clear their text when there is nothing to say. Or unhide the region while empty, then write the text in a later task (`setTimeout(..., 0)`).
## Open questions for the human
- Is the progress bar's visual presence tied to the wrapper's `hidden`? If so, move the live-region text out of the wrapper so the bar can hide independently.big-pickle (free) ｜ 𝕏
…e progress text out of hidden wrapper
There was a problem hiding this comment.
Important
The commit drops the last classList.remove('hidden') on the install-plan region but leaves both re-adding sites standing, so after the first successful install — or a resolver error — the plan note is stuck display:none and its live-region announcements are dead for the rest of the session. The progress-text move out of the wrapper likewise strands the last status line once the bar hides.
Reviewed changes what changed since the prior pullfrog review (ee70357 → d1d3923):
- Progress text leaves the hidden wrapper (
screen.html) —#pm-catalog-progress-textmoved out of#pm-catalog-progress-wrap, so the live region is always rendered and only the bar is toggled. - Plan note stops re-hiding — and stops un-hiding (
screen.js) — thehiddenclass toggles were removed from the empty-selection and lines/no-lines branches ofupdateDependencyInfo, andrenderCatalogProgressgained a null guard on the progress-text write.
Tests still pass on this head: node --test tests/plugin-catalog-view.test.js 48/48, tests/renderer-html-escaping.test.js 7/7.
big-pickle (free) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes the single commit since the prior pullfrog review (d1d3923 → 8d347a6):
- Removed the last two plan-note hides (
screen.js) — the resolver-error catch and the post-install branch now clear#pm-catalog-depsinstead of addinghidden, so the region stays rendered for the rest of the session and every later plan write is both visible and announced; the catch comment was reworded to match the new contract. - Cleared the progress text with the bar (
screen.js) — the installfinallynow empties#pm-catalog-progress-textnext to the wrapper hide, so a cancelled or failed run no longer strands a stale "downloading… 42%" line under a hidden bar.
Audited every write site for the five live regions (#pm-catalog-deps, #pm-catalog-msg, #pm-catalog-status, #pm-catalog-active, #pm-catalog-progress-text) against this head: none is populated while it or an ancestor is display:none, and no hidden toggle on them survives anywhere in screen.js or screen.html. Suites pass on this head: node --test tests/plugin-catalog-view.test.js 48/48, node --test tests/renderer-html-escaping.test.js 7/7.
big-pickle (free) ｜ 𝕏
Part of #4. Closes #19 — Catalog UI 5/5.
Stacked on #35 (Catalog UI 4/5): this branch is based on
kilo/18-catalog-install-action, so the diff here is only the accessibility pass. Merge #35 first, then retarget this PR tomain(GitHub does that automatically once the base branch merges).Changes
:focus-visiblestyling, so keyboard travel through this dark panel had no visible position. The fragment now brings its own outline rule, scoped to#plugin-manager-panelso it cannot repaint focus rings elsewhere in the app.screen.html):#pm-catalog-deps) is a labelled region (Installation plan) witharia-live="polite"— selection state, "Required by the selection: Core (required by Practice Alpha)", pruned entries, download size and the network requirement are announced as they change;#pm-catalog-msg), browse status (#pm-catalog-status), filter state (#pm-catalog-active) and download progress text are polite live regions;<progress>element and both placeholder-only inputs (catalog search, git URL) get accessible names; the install list is a labelled group.screen.js):titlenaming the reason and the control that undoes it (Unpin / Enable / Downgrade / "ships with the app");aria-describedby;input/buttonin document order. Pinned that: no inline event handlers and no positivetabindexanywhere in the fragment or screen script.Manual checklist (recorded per the issue)
Keyboard-only walkthrough (Tab / Shift+Tab / Space / Enter, no mouse):
<label>), Enter activates every button.:focus-visiblerule); disabled checkboxes are skipped by Tab as expected, with their reason exposed viaaria-describedby/titlewhen browsed.Screen-reader pass:
Verification
npm test— 311 passed, 6 skipped; the only failure is the JUCE-submodule suite, which a clean checkout cannot populate (pre-existing).npm run typecheck— clean (no TS changes in this PR).aria-describedbywiring, the lock-line text, or the:focus-visiblerule each fails the corresponding test.