docs: record the second Phase 5 pass and what it corrected

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.
2026-08-12 11:44:58 -04:00
parent 862e8a0468
commit 63d11c3f9c
3 changed files with 314 additions and 1 deletions
@@ -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.
@@ -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 `<wa-dialog>`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 `<div @click>`,
 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.
@@ -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 `<wa-dialog>` 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