back-navigation.spec.ts asserts aria-current, not data-active-view
f34777d
—
CLAUDE.md: the rule, beside the two that already keep the back stack honest
What was actually wrong
handleNavigate() sets #main-content's data-active-view on every
path, _isBack included. The nav components learned the active view
from the navigate CustomEvent, which only the outbound path
dispatches — the popstate listener calls handleNavigate directly.
So the shell knew and never told anyone on the back path.
Measured on make dev-headless SEED=default, reading the lit item out
of each nav's shadow root:
steps
data-active-view
sidebar lit
bottom-nav lit
Albums (390px)
albums
nav-albums
tab-albums
→ open an album
explore-album-details
nav-albums
(nothing)
→ back
albums
nav-albums
(nothing)
Albums → Tracks (1280px)
tracks
nav-tracks
tab-tracks
→ back (lands on Albums)
albums
nav-tracks
tab-tracks
Two things there are not in the report. It is not Android-only —
the last row is desktop, and both navs highlight the view just left,
which is worse than an absent highlight and happens on any back across
two primary views. And the two symptoms are one cause: app-sidebar.onGlobalNavigate guarded on navItems.some(...), so a
detail view left its highlight alone, while bottom-nav had no such
guard and lit nothing. Neither was deliberate; the sidebar was
stale-but-lucky.
A third symptom turned up while measuring, and it decided the
shape: standing on Albums, opening the phone's "More" drawer showed Home highlighted, because bottom-nav builds that <app-sidebar>
when the drawer opens and the fresh copy had heard no navigate at
all.
The shape, and why it is not the obvious one
Not a re-dispatch of navigate: index.ts is itself the document
listener for it, so that is an infinite loop — and the two statements
differ. "Please go to X" is said from 28 call sites across 18 files; "the active view is now X" is said by the shell, once per
navigation. Only the second is what a highlight wants.
Not an event either, because of that drawer: an event has no answer
for a listener that was not there, and a value does. So a store, read
through a ReactiveController like every other store here, with no @state() copy in either component — which is #72's Direction, one
source, no per-component tracking.
Four decisions worth naming:
A detail view leaves the parent destination lit, deliberately and
in both navs. setView(view, isPrimary) takes view in VIEW_TAGS
from the call site rather than re-deriving it, because that table is
where the primary/detail split is already written down.
The store starts empty, so nothing is lit until the shell's first
navigation. app-sidebar's activeView defaulted to home to match
the landing view; that default is right only while GetDefaultPage()
agrees with it, and the fossil comment saying so is gone.
No optimistic highlight on click. The sidebar used to set its own activeView before dispatching; that is the second opinion this
removes. handleNavigate answers synchronously, before it awaits a
chunk.
#6 is not folded in. It shares handleNavigate and its
groundwork (pushedEntries) is already in the tree, but it is a
feature with its own design questions — where the controls live,
what disables them — and this is a Priority/High bug. It also
inherits the fix rather than needing it: forward reaches the same
state by a second route. The argument for taking them in this order,
against #73's stated one, is a comment on #73.
The spec that should have caught this
e2e/specs/back-navigation.spec.ts already covered these journeys and
asserted onlydata-active-view — the one thing correct on the
back path — so it was green throughout. Same trap as layout-overflow.spec.ts for #69. The new assertions went into that
file rather than a new one, or it would carry on passing vacuously;
they are aria-current="page" via getByRole, and the role query
resolves to whichever nav is in the accessibility tree at that
viewport, so one helper covers both.
Checked negatively: with the three source files stashed and the specs
kept, 3 of the 4 new tests fail. The fourth (parent lit while a detail
view is open) passed before by accident and says so in a comment; the
tab bar's half of that rule is the phone test, which did not.
Verification
make ui-test — 928 passed (82 files)
tsc --noEmit in frontend/ and in e2e/ — clean
make e2e — 141 passed
make css-check — 130 files, no broken literals
No Go changed, so no make lint / make test
Reproduced by hand at 1280×800 and 390×780 before and after, plus a
screenshot read of the phone detail view: the Albums tab is lit
behind an open album page
Two things filed rather than fixed here:
#140 — folder-picker and explore-track-details time out in
full-suite make ui-test runs. Reproduced on pristine main with a
stashed tree, twice; passes in isolation. Distinct from #138 (an
assertion, not a timeout). If CI's check fails on either, it is not
this branch.
#66 — the album page's header actions clip on a phone (found in
the screenshot above; "Shuffle album" cut by 53px, "Add to queue" by
50px, neither reachable). Already filed as the width half of #66, so
it got the measurement as a comment rather than a duplicate issue.
Fixes the nav highlight by giving the shell one way to say which view
is active, and asserting the thing a person can see.
## Commits
| commit | issue | what |
|---|---|---|
| `9d65d9a` | #72 | `activeViewStore` + `ActiveViewController`; `handleNavigate` publishes on every path, both navs read it and hold no `activeView` of their own |
| `fef8c8b` | #72 | `back-navigation.spec.ts` asserts `aria-current`, not `data-active-view` |
| `f34777d` | — | `CLAUDE.md`: the rule, beside the two that already keep the back stack honest |
## What was actually wrong
`handleNavigate()` sets `#main-content`'s `data-active-view` on every
path, `_isBack` included. The nav components learned the active view
from the `navigate` CustomEvent, which only the *outbound* path
dispatches — the `popstate` listener calls `handleNavigate` directly.
So the shell knew and never told anyone on the back path.
Measured on `make dev-headless SEED=default`, reading the lit item out
of each nav's shadow root:
| steps | `data-active-view` | sidebar lit | bottom-nav lit |
|---|---|---|---|
| Albums (390px) | `albums` | `nav-albums` | `tab-albums` |
| → open an album | `explore-album-details` | `nav-albums` | **(nothing)** |
| → back | `albums` | `nav-albums` | **(nothing)** |
| Albums → Tracks (1280px) | `tracks` | `nav-tracks` | `tab-tracks` |
| → back (lands on Albums) | `albums` | **`nav-tracks`** | **`tab-tracks`** |
Two things there are not in the report. It is **not Android-only** —
the last row is desktop, and both navs highlight the view just *left*,
which is worse than an absent highlight and happens on any back across
two primary views. And the two symptoms are **one cause**:
`app-sidebar.onGlobalNavigate` guarded on `navItems.some(...)`, so a
detail view left its highlight alone, while `bottom-nav` had no such
guard and lit nothing. Neither was deliberate; the sidebar was
stale-but-lucky.
A **third** symptom turned up while measuring, and it decided the
shape: standing on Albums, opening the phone's "More" drawer showed
**Home** highlighted, because `bottom-nav` builds that `<app-sidebar>`
when the drawer opens and the fresh copy had heard no `navigate` at
all.
## The shape, and why it is not the obvious one
Not a re-dispatch of `navigate`: `index.ts` is itself the document
listener for it, so that is an infinite loop — and the two statements
differ. *"Please go to X"* is said from 28 call sites across 18 files;
*"the active view is now X"* is said by the shell, once per
navigation. Only the second is what a highlight wants.
Not an event either, because of that drawer: an event has no answer
for a listener that was not there, and a value does. So a store, read
through a `ReactiveController` like every other store here, with no
`@state()` copy in either component — which is #72's Direction, *one
source, no per-component tracking*.
Four decisions worth naming:
- **A detail view leaves the parent destination lit**, deliberately and
in both navs. `setView(view, isPrimary)` takes `view in VIEW_TAGS`
from the call site rather than re-deriving it, because that table is
where the primary/detail split is already written down.
- **The store starts empty**, so nothing is lit until the shell's first
navigation. `app-sidebar`'s `activeView` defaulted to `home` to match
the landing view; that default is right only while `GetDefaultPage()`
agrees with it, and the fossil comment saying so is gone.
- **No optimistic highlight on click.** The sidebar used to set its own
`activeView` before dispatching; that is the second opinion this
removes. `handleNavigate` answers synchronously, before it awaits a
chunk.
- **#6 is not folded in.** It shares `handleNavigate` and its
groundwork (`pushedEntries`) is already in the tree, but it is a
feature with its own design questions — where the controls live,
what disables them — and this is a `Priority/High` bug. It also
inherits the fix rather than needing it: forward reaches the same
state by a second route. The argument for taking them in this order,
against #73's stated one, is a comment on #73.
## The spec that should have caught this
`e2e/specs/back-navigation.spec.ts` already covered these journeys and
asserted **only** `data-active-view` — the one thing correct on the
back path — so it was green throughout. Same trap as
`layout-overflow.spec.ts` for #69. The new assertions went into that
file rather than a new one, or it would carry on passing vacuously;
they are `aria-current="page"` via `getByRole`, and the role query
resolves to whichever nav is in the accessibility tree at that
viewport, so one helper covers both.
Checked negatively: with the three source files stashed and the specs
kept, 3 of the 4 new tests fail. The fourth (parent lit while a detail
view is open) passed before by accident and says so in a comment; the
tab bar's half of that rule is the phone test, which did not.
## Verification
- `make ui-test` — 928 passed (82 files)
- `tsc --noEmit` in `frontend/` and in `e2e/` — clean
- `make e2e` — 141 passed
- `make css-check` — 130 files, no broken literals
- No Go changed, so no `make lint` / `make test`
- Reproduced by hand at 1280×800 and 390×780 before and after, plus a
screenshot read of the phone detail view: the Albums tab is lit
behind an open album page
Two things filed rather than fixed here:
- **#140** — `folder-picker` and `explore-track-details` time out in
full-suite `make ui-test` runs. Reproduced on pristine `main` with a
stashed tree, twice; passes in isolation. Distinct from #138 (an
assertion, not a timeout). If CI's `check` fails on either, it is not
this branch.
- **#66** — the album page's header actions clip on a phone (found in
the screenshot above; "Shuffle album" cut by 53px, "Add to queue" by
50px, neither reachable). Already filed as the width half of #66, so
it got the measurement as a comment rather than a duplicate issue.
Closes #72
The nav components learned where the user was from the `navigate`
CustomEvent, which only the outbound path dispatches: `popstate` calls
`handleNavigate()` directly. So a back-navigation left both of them
highlighting the view just left — desktop included, at any width, on
any back across two primary views. Opening a detail view was the same
cause wearing a different symptom: `app-sidebar` guarded on its own
item list and kept its highlight, `bottom-nav` did not and lit nothing.
It cannot be fixed by re-dispatching `navigate` — `index.ts` is that
event's document listener, so that is an infinite loop, and "please go
to X" is not the statement being made. `activeViewStore` is the shell
saying "the active view is now X", once per navigation, `popstate`
included; both navs read it through a controller and hold no
`activeView` of their own.
A store rather than an event because a component that mounts *after* a
navigation still has to know: `bottom-nav`'s drawer builds its
`app-sidebar` on open, and that copy had heard nothing at all, so the
drawer opened on Home from any page in the app.
Closes#72
`back-navigation.spec.ts` covered exactly the journeys #72 breaks and
was green throughout it, because every assertion in it was
`data-active-view` — which the shell sets on every path including the
back one, and which was the one thing already correct. The same trap
`layout-overflow.spec.ts` set for #69: a spec named for the behaviour,
measuring the plumbing.
The assertions go here rather than in a second file, or the first would
carry on passing vacuously. They are `aria-current="page"` through
`getByRole`, which is the accessible fact — `.active` is a class and
could be restyled without breaking anything real — and the role query
resolves to whichever nav is in the accessibility tree at that
viewport, so one helper covers the sidebar and the tab bar.
Three of the four fail on the build before the fix. The fourth, the
parent staying lit while a detail view is open, passed by accident and
says so.
It belongs beside the two rules that already keep the history stack and
the in-app back buttons agreeing, and for the same reason: a second
component-local idea of where the user is, is how they came to
disagree.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes the nav highlight by giving the shell one way to say which view
is active, and asserting the thing a person can see.
Commits
9d65d9aactiveViewStore+ActiveViewController;handleNavigatepublishes on every path, both navs read it and hold noactiveViewof their ownfef8c8bback-navigation.spec.tsassertsaria-current, notdata-active-viewf34777dCLAUDE.md: the rule, beside the two that already keep the back stack honestWhat was actually wrong
handleNavigate()sets#main-content'sdata-active-viewon everypath,
_isBackincluded. The nav components learned the active viewfrom the
navigateCustomEvent, which only the outbound pathdispatches — the
popstatelistener callshandleNavigatedirectly.So the shell knew and never told anyone on the back path.
Measured on
make dev-headless SEED=default, reading the lit item outof each nav's shadow root:
data-active-viewalbumsnav-albumstab-albumsexplore-album-detailsnav-albumsalbumsnav-albumstracksnav-trackstab-tracksalbumsnav-trackstab-tracksTwo things there are not in the report. It is not Android-only —
the last row is desktop, and both navs highlight the view just left,
which is worse than an absent highlight and happens on any back across
two primary views. And the two symptoms are one cause:
app-sidebar.onGlobalNavigateguarded onnavItems.some(...), so adetail view left its highlight alone, while
bottom-navhad no suchguard and lit nothing. Neither was deliberate; the sidebar was
stale-but-lucky.
A third symptom turned up while measuring, and it decided the
shape: standing on Albums, opening the phone's "More" drawer showed
Home highlighted, because
bottom-navbuilds that<app-sidebar>when the drawer opens and the fresh copy had heard no
navigateatall.
The shape, and why it is not the obvious one
Not a re-dispatch of
navigate:index.tsis itself the documentlistener for it, so that is an infinite loop — and the two statements
differ. "Please go to X" is said from 28 call sites across 18 files;
"the active view is now X" is said by the shell, once per
navigation. Only the second is what a highlight wants.
Not an event either, because of that drawer: an event has no answer
for a listener that was not there, and a value does. So a store, read
through a
ReactiveControllerlike every other store here, with no@state()copy in either component — which is #72's Direction, onesource, no per-component tracking.
Four decisions worth naming:
in both navs.
setView(view, isPrimary)takesview in VIEW_TAGSfrom the call site rather than re-deriving it, because that table is
where the primary/detail split is already written down.
navigation.
app-sidebar'sactiveViewdefaulted tohometo matchthe landing view; that default is right only while
GetDefaultPage()agrees with it, and the fossil comment saying so is gone.
activeViewbefore dispatching; that is the second opinion thisremoves.
handleNavigateanswers synchronously, before it awaits achunk.
handleNavigateand itsgroundwork (
pushedEntries) is already in the tree, but it is afeature with its own design questions — where the controls live,
what disables them — and this is a
Priority/Highbug. It alsoinherits the fix rather than needing it: forward reaches the same
state by a second route. The argument for taking them in this order,
against #73's stated one, is a comment on #73.
The spec that should have caught this
e2e/specs/back-navigation.spec.tsalready covered these journeys andasserted only
data-active-view— the one thing correct on theback path — so it was green throughout. Same trap as
layout-overflow.spec.tsfor #69. The new assertions went into thatfile rather than a new one, or it would carry on passing vacuously;
they are
aria-current="page"viagetByRole, and the role queryresolves to whichever nav is in the accessibility tree at that
viewport, so one helper covers both.
Checked negatively: with the three source files stashed and the specs
kept, 3 of the 4 new tests fail. The fourth (parent lit while a detail
view is open) passed before by accident and says so in a comment; the
tab bar's half of that rule is the phone test, which did not.
Verification
make ui-test— 928 passed (82 files)tsc --noEmitinfrontend/and ine2e/— cleanmake e2e— 141 passedmake css-check— 130 files, no broken literalsmake lint/make testscreenshot read of the phone detail view: the Albums tab is lit
behind an open album page
Two things filed rather than fixed here:
folder-pickerandexplore-track-detailstime out infull-suite
make ui-testruns. Reproduced on pristinemainwith astashed tree, twice; passes in isolation. Distinct from #138 (an
assertion, not a timeout). If CI's
checkfails on either, it is notthis branch.
the screenshot above; "Shuffle album" cut by 53px, "Add to queue" by
50px, neither reachable). Already filed as the width half of #66, so
it got the measurement as a comment rather than a duplicate issue.
Closes #72
f34777d3e4tod347809e6e