From 63d11c3f9c4b33bc1f4af321798ba2991c34f9c7 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 12 Aug 2026 11:44:58 -0400 Subject: [PATCH] docs: record the second Phase 5 pass and what it corrected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of a11y.md's findings describe a build that no longer exists — one fixed by a phase that was not about it, one whose stated mechanism stopped being true when Phase 4 bundled the icons, and one that reproduces as a different shape. The generalisation is that a finding has a date as well as a magnitude and a mechanism. Also records the two bad versions of the duplicate-shelf rule that the *existing* tests caught, the eleven e2e specs that landing on Home broke and the one of them that was a real bug, and the second CI e2e failure on a commit that changed no application code. --- .planning/NOTES.md | 97 +++++++++++++ .../plans/active/007-ui-reconciliation.md | 130 ++++++++++++++++++ CLAUDE.md | 88 +++++++++++- 3 files changed, 314 insertions(+), 1 deletion(-) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index b7fcf04..ceeaf9a 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -1329,3 +1329,100 @@ Seven things worth keeping: header now renders during load and omits the count until there is an answer — `null` meaning "no answer yet", which is a different thing from zero and has to be a different value. + +## An audit ages against the code, and the oldest claims are the least checked + +Plan 007 phase 5, second pass: the five hand-rolled dialogs, the context +menu's keyboard model, the ARIA tail, and landing on Home. `a11y.md` was +the least verified material in the repo — three of its 34 findings had +been touched before this pass — and treating each one as a hypothesis +was worth it three times over. + +The generalisation the pass adds to "an audit's magnitude and its +mechanism are two claims": **a finding also has a date, and the code has +moved since.** Three of the findings here describe a build that no +longer exists, in three different ways: + +- **Fixed by a phase that was not about it.** `a11y.12` lists five + silent async surfaces. The first is `config-page`'s private toast, + which Phase 3 *deleted* — and the surface that replaced it has had + `role="status" aria-live="polite"` since the day it was written. Two + of five bullets were already closed. +- **Half-fixed, so the stated mechanism is now wrong.** `H-9` says the + Home card's missing-art placeholder "has no background". It has one; + it is `--yj-bg-surface`, which is almost exactly the page colour, and + it holds a `wa-icon` — which has rendered at all only since Phase 4 + bundled the icons. "Renders as nothing" was *literally* true offline + when the audit was written and is now merely nearly true. Fixing the + stated cause would have changed one line and nothing visible. +- **Reproduces differently.** `H-9`'s other half says "all three shelves + show the same seven albums". There are five shelves and the + duplication is one adjacent *pair*. The fix is still right; a rule + written from the sentence rather than from the page would have been + aimed at three shelves that do not exist. + +Seven more things worth keeping: + +- **The existing tests caught two bad versions of a new rule; the new + test caught neither.** "Suppress a shelf that repeats the one above" + is one line of intent and three of policy. Version one collapsed a + four-album library to a single shelf. Version two, guarded by a fixed + shelf size, let an 11-album library keep three identical shelves while + a 13-album one lost them. The rule that survives is **"a repeat is a + fault only if a different row was possible"** — the shelf must not be + showing the whole library — and the reason it is right is that it is + about the library rather than about a constant. A test written for a + change tests the change; the tests already there are what test the + system. +- **A default that is a behaviour has to be changed in two places, and + one of them is a test suite.** Landing on Home broke **eleven** e2e + specs. Nine assumed the track list is on screen at startup. One failed + because every primary view stays in the DOM and Home names the same + artists, so an unscoped `getByText().first()` matched a card on a + `.view-hidden` page. And one was a real bug: `getByRole('button', + {name: 'Shuffle'})` resolved to *two* elements, because Home's + page-header action and the transport's shuffle mode had the same + accessible name and had never been on screen together. A cached view + is in the accessibility tree from the first paint, so "these two + controls are on different pages" stopped being true the moment the app + started on one of them. +- **A component test against hand-built markup cannot see a web + component's own lifecycle.** Two of the three things that made the + menu keyboard model work are invisible to it: `wa-dropdown-item` sets + its `role` in its *own* first update, so a query at the host's + `updateComplete` finds no items at all; and `focus()` on a `wa-popup` + that has not positioned itself is a silent no-op. Both produce a menu + that opens and refuses to take focus. Both were found by driving the + real app, and the e2e spec exists because the component test passes + either way. +- **The rule for a live region is about ordering, not markup.** Most + screen readers announce a *change* to a region they are already + watching and ignore one that appears with its content already in it — + which is why `catalog-scope-notice` had a `role="status"` that + announced nothing. So the regions render unconditionally and empty + and only their text changes, and `now-playing`'s is in **both** render + branches, because the branch with no track is the one that has to be + mounted before the first track arrives. +- **`aria-selected` on `role="button"` is not useless, it is dropped.** + Four grids whose entire ctrl/shift interaction exists to produce a + selection were publishing it into a void. The fix is not an attribute + but a role: `listbox`/`option`. +- **A backtick in a comment inside a `css` tagged template literal ends + the literal.** The skill has warned about this for two plans. I did it + twice in one session — once in a component, where `tsc` pointed at the + line, and once in `tokens.css.ts`, where **every test file in the + suite failed to import** and the output reads like a broken test + runner. If the whole tier dies at once, suspect the shared module. +- **Migrating a dialog can delete feedback nobody listed.** + `config-page`'s remove-library spinner lived *in* the hand-rolled + overlay, so moving the confirmation to `confirmAction()` left a + backend call of unknown length with no indication it had started. The + state field it used had no reader afterwards, which is the tell: + `removingLibraryId` now means "which row is busy" and the row says so. + +And one on the harness: **`e2e` has now failed in CI on two consecutive +commits that changed no application code** — `9e92721` (docs only) and +`9f03b3f` (a workflow and a shell script). `check` passed on both. That +is not proof the container's audio clock is the whole story, but a +failure on a commit which cannot have caused it is worth more than a +green run, and it is two of them. diff --git a/.planning/plans/active/007-ui-reconciliation.md b/.planning/plans/active/007-ui-reconciliation.md index a1e08a6..3eab922 100644 --- a/.planning/plans/active/007-ui-reconciliation.md +++ b/.planning/plans/active/007-ui-reconciliation.md @@ -1687,6 +1687,136 @@ And two things about CI, neither mine and both pre-existing on --- +### Phase 5 — the second pass: the dialogs, the menu, the ARIA tail, and Home + +Item 3 of the four as one landing, plus the first of the smaller ones. +Every finding was reproduced in the running app before it was fixed, +and two of them changed shape when it was. + +- **`a11y.4` / `a11y.16` — five hand-rolled dialogs are `wa-dialog`s.** + Split by shape rather than by owner: the three that only ask a + question (the autotag warning, the leave-as-is confirmation, the + remove-library confirmation) are `confirmAction()` calls, and the two + carrying input (paste URL, MusicBrainz search) are ``s in + place. Verified in the app: the native dialog reports `:modal`, focus + lands in the first field, Escape closes it and the view state follows + through `@wa-hide`. **`autotag-view`'s last document keydown listener + died with them** — it existed only for Escape, because its dialogs + could not close themselves. +- **`a11y.3` — the context menu has a keyboard model.** `MenuKeyboard` + in `context-menu-controller.ts`: focus the first item, Arrow/Home/End + (wrapping), Enter/Space, Escape/Tab, focus restored to the row. + Shift+F10 and the ContextMenu key open it from a focused row in all + six hosts. It is standalone rather than part of the controller + because `playlist-view` renders a menu without the controller. +- **Three lists had no focused row to open it from** — the queue panel + and both playlist detail views — and gained a roving tab stop + (`utils/roving-rows.ts`). `track-list` keeps its own. +- **The ARIA tail**: `aria-sort` on the column headers (`a11y.9`), + `role=listbox`/`option` on the four selectable grids with the invalid + `aria-selected`-on-`button` dropped (`a11y.13`), live regions on the + four silent async surfaces (`a11y.12`), the `rem` type scale + (`a11y.19`), `job-indicator`'s unmanaged `role="dialog"` (`a11y.17`) + and its colour-only failure dot (`a11y.23`). +- **`H-8` / `H-9` — the app lands on Home, and Home is worth landing + on.** The missing-art placeholder draws the letter tile the other two + grids draw, and a shelf that repeats the one above it is suppressed + in `backend/home` — the same rule as omitting an empty one. + +#### Where the plan was wrong — the second pass + +Eight things, and three of them are the audit describing a build that +had already moved: + +- **`a11y.12`'s first bullet was fixed two phases ago, twice.** It names + `config-page`'s private toast as having no `role="status"`. Phase 3 + deleted that toast, and the surface that replaced it — + `notification-host` and `inline-notice` — has had `role="status" + aria-live="polite"` from the day it was written. Two of the finding's + five bullets were closed by a phase that was not about accessibility. +- **`H-9`'s stated mechanism is not why the card is invisible.** The + audit says the placeholder "has no background". It has one: + `--yj-bg-surface`, which is *almost exactly the page colour*, holding + a `wa-icon` at `--yj-text-tertiary`. And the icon has rendered at all + only since Phase 4 bundled the icon set — before that the fallback + was a CDN fetch, so "renders as nothing" was literally true offline + and is now merely nearly true. The fix is the same; the reason it was + worth checking is that "no background" would have been fixed by one + line that changed nothing visible. +- **`H-9`'s duplicate-shelf half does not reproduce as stated, and the + obvious rule breaks a small library.** The audit says "all three + shelves show the same seven albums". On the fixture library there are + five shelves and the duplication is one *pair* — "On repeat" is + "Pick up where you left off" reordered. The first rule I wrote + (suppress at two-thirds overlap with the shelf above) **collapsed a + four-album library to a single shelf**, and the second (guarded by a + fixed shelf size) let an 11-album library keep three identical + shelves while a 13-album one lost them. Both were caught by the + *existing* Go tests, not by the one written for the change. The rule + that survives is "a repeat is a fault only if a different row was + possible" — the shelf must not be showing the whole library. +- **Landing on Home broke eleven e2e specs, and one of them was an app + bug.** Nine assumed the track list is the first thing on screen. One + failed because Home's shelves name the same artists as + `artists-view`, and every primary view stays in the DOM — so an + unscoped `getByText().first()` matched a card on a `.view-hidden` + page. And one was real: **`getByRole('button', {name: 'Shuffle'})` + resolved to two elements**, because Home's page-header action and the + transport's shuffle mode had the same accessible name. They were + never on screen together before; a cached Home is in the + accessibility tree from the first paint. It is "Shuffle suggestions" + now. +- **`a11y.19` and `a11y.20` are one finding, and the second one wins.** + Converting the type scale to `rem` works — verified at a 24px root, a + track cell goes 12px to 18px — and does *not* reach the four + virtualized lists, whose rows are a hardcoded px height duplicated as + the layout's `_itemSize` and carry `contain: strict`. Measured: the + row stays 33px while its text grows to 18px, so larger text crops it. + Left unfixed and documented in the token file, which is what a11y.20 + itself asks for: deriving `_itemSize` from a measured row is a change + to the scroll maths of four lists, not to a type scale. +- **Two of the three things that made the menu work are invisible to a + component test.** `wa-dropdown-item` sets its `role` in its own first + update, so querying by role at the host's `updateComplete` finds no + items; and `focus()` on a `wa-popup` that has not positioned itself + is a silent no-op. Both produced a menu that opened and refused to + take focus, and both were found by driving the real app — a component + test against hand-built markup passes either way, which is why the + e2e spec exists. +- **A backtick in a comment inside a `css` template literal ends the + literal.** The skill warns about this. I did it twice in one session + anyway, and the second time every test file in the suite failed to + import, which reads like anything except a stray backtick. +- **The `wa-dialog` migration removed state that had a use.** + `config-page`'s `isRemoving` had no reader once the dialog owned the + spinner. Rather than delete it, `removingLibraryId` now means "which + row is busy" and the row says "Removing…" — the removal is a backend + call of unknown length, and moving the confirmation out of the page + had quietly removed the only feedback that it had started. + +#### Not done, and still worth doing (after the second pass) + +Item 4's remaining smaller items, each independently landable: the +album page's primary action and its unexplained ✓ badges (`H-13`), the +`?` shortcuts overlay (and with it keyboard seeking from a focused +row), Settings reordered with a Playback section (`H-22`) together with +`a11y.1` (`config-section`'s disclosure header is a bare `
`, +so every setting is behind a control that cannot be tabbed to) and +`a11y.2` (the Downloads tabs), an Album column in the track list +(`H-15`), and `cover-grid`'s dead album dropdown (`perf.p2`). + +Three inherited items are unchanged: `tracklist.delete` (below), the +header search box on `smart-playlist-details`, and keyboard seeking +from a focused row. + +**`tracklist.delete` was deliberately not built.** It needs a "remove +from library" that does not exist *and* a decision about what it +removes. Neither answer is currently right: removing the row is a lie +unless it also excludes the path, since the next scan brings it back, +and removing the file is a delete-your-music button one keystroke from +a focused row. The honest interim is to stop advertising the binding in +Settings; that is not done yet either. + ## Phase 6 — Explore starts the conversation The only phase that adds rather than repairs. diff --git a/CLAUDE.md b/CLAUDE.md index d9b2280..10f6e8d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -237,7 +237,24 @@ See `.planning/plans/active/005-agent-development-harness.md`. says so. Its queries (`sql/queries/home.sql`) return album ids only and are joined back to `GetAllAlbumsWithDetails` in Go, so the album projection has one definition. A shelf with nothing behind it is - omitted, never rendered empty. + omitted, never rendered empty — **and so is a shelf that repeats the + one above it**, which is the same rule one step further: "On repeat" + was "Pick up where you left off" reordered, because a small library + has one signal and answers several questions with the same albums. + Two guards make that safe, and both were arrived at by breaking the + existing tests: only shelves of three or more albums are judged (two + rows of one overlap by 100% whenever they agree at all), and only + when the shelf is **not showing the whole library** — a repeat is a + fault only if a different row was possible. Measured against a fixed + shelf size instead, an 11-album library kept three identical shelves + while a 13-album one lost them. + + **The app lands here**, from `index.ts` after the stores are wired. + `index.html` still renders the track list eagerly and it is still what + paints first — it is the cached `tracks` view, so the navigation is a + class toggle plus one chunk rather than a second render of the shell. + `app-sidebar`'s default `activeView` is `home` to match, because the + sidebar does not hear a `navigate` it did not send. - `profiling` — pprof server on `:6060`, compiled out in non-dev builds via build tags (`internal/dev/`). **Explore catalog** (`backend/explore/`): the searchable MusicBrainz/ @@ -347,6 +364,75 @@ Destructive actions ask once, through `confirmAction()` (`components/confirm-dialog/`), which is a `wa-dialog` and so brings the focus trap and Escape the hand-rolled overlays do not have. +**Every dialog in the app is a `wa-dialog`, and there is no sixth +pattern.** The four hand-rolled autotag overlays and the remove-library +confirmation had no `role`, no `aria-modal`, no focus trap and no focus +restore — including the two gating an irreversible on-disk metadata +rewrite. The split is by *shape*, not by owner: a dialog that only asks +a question is a `confirmAction()` call (title, message, impact, +confirm/cancel), and a dialog carrying **input** is a `` in +the host's own template. Both remaining autotag dialogs render +unconditionally with `?open` deciding which is up — mounting one on +demand puts the element and its `showModal()` in the same update. +`autotag-view`'s last document keydown listener died with them; it +existed only because its dialogs could not close themselves. + +**A menu has a keyboard model, and it is one model.** +`utils/context-menu-controller.ts` exports **`MenuKeyboard`** — focus +the first item on open, Arrow/Home/End to move (wrapping, as a menu +does and a listbox does not), Enter/Space to activate, Escape or Tab to +close, and focus back to the element it opened from. It is standalone +rather than part of `ContextMenuController` because `playlist-view` +renders a menu without that controller, and two menus with two keyboard +models is exactly what this is for. `isContextMenuKey()` is the +Shift+F10 / ContextMenu-key test, and `openFrom(el)` is the keyboard +open: anchored to the element, restoring focus to it. + +Four things in it are load-bearing, and two of them are only visible +against the real components: + +- **The items are not items yet when the host finishes updating.** + `wa-dropdown-item` sets its `role` in its *own* first update, so a + `[role^="menuitem"]` query at `updateComplete` finds nothing — which + reads exactly like a menu that opened and refused to take focus. +- **`focus()` on a popup that has not positioned itself is a silent + no-op**, so the first focus is retried across a few frames. +- **Focus is only taken back if the menu had it.** A click elsewhere + closes the menu too, and pulling focus to the row the user + right-clicked a moment ago is worse than leaving it. +- **Web Awesome keys an item's tabindex and highlight off `active`**, so + moving focus without setting it leaves the highlight on whichever + item the mouse last touched. + +Three lists had no focused row to open a menu *from* — the queue panel +and both playlist detail views — and gained a roving tab stop through +`utils/roving-rows.ts`. **`track-list` deliberately does not use it**: +its equivalent predates this, carries selection semantics (shift-extend, +ctrl-toggle) the other three do not have, and is pinned by its own +tests. + +**Async surfaces say what they are doing.** `styles/sr-only.css.ts` +carries the visually-hidden class and the rule that comes with it: a +live region must be **in the DOM before the text it announces is**, +because most screen readers announce a change to a region they are +already watching and ignore one that appears with its content already +in it. So these regions render unconditionally and empty, and only +their text changes. Four surfaces have one — the track list (loading, +failed, and how many rows a search matched), Explore's search, +`now-playing` (in **both** render branches, so it exists before the +first track arrives) and `job-indicator`, whose label swings between +"Scanning Music", "3 background jobs" and "Finished". The notification +surface already had one from Phase 3. + +**A selectable grid is a listbox.** The four grids that ctrl/shift-select +(`artists-view`, `genres-view`, `cover-grid`, and the queue) are +`role="listbox" aria-multiselectable` over `role="option"` cards, not +rows of `role="button"`: `aria-selected` on a button is *invalid* and is +dropped outright, so the state the whole ctrl/shift interaction exists +to produce was invisible to anything but a sighted user. `track-list`'s +column headers carry `aria-sort` (Phase 1 added `role="columnheader"` +without it) and are activated by Enter/Space as well as by a click. + **One keyboard authority.** No component owns a document keydown listener for its own shortcuts; it registers *panel-scoped* bindings (`autotag.*`, `tracklist.*` in `backend/shortcuts/config.go`) and