diff --git a/CLAUDE.md b/CLAUDE.md index be86733..894630d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -952,6 +952,50 @@ kept beside it, because two stacks is precisely how a view's own back button and the phone's gesture come to disagree about what one press means. +**And there is one statement of which view is active**, for the same +reason: `popstate` calls `handleNavigate()` directly and dispatches no +`navigate`, so the two nav components — which learned the active view +from that event — kept highlighting the view the user had just *left*. +`store/active-view-store.ts` is the shell saying where the user is, and +both navs read it through `ActiveViewController` rather than holding an +`activeView` of their own. + +Four things about it are load-bearing. + +**"Please go to X" and "the active view is now X" are different +statements**, and only the first existed — dispatched from 28 call +sites across 18 files. A re-dispatch from inside `handleNavigate` is +not the fix and cannot be: that function is the `document` listener for +`navigate`, so it is an infinite loop. + +**It is a store rather than an event, because a component that mounts +after a navigation still has to know.** `bottom-nav`'s "More" drawer +creates its `` on open, and that copy had heard no +`navigate` at all — standing on Albums, the drawer opened highlighting +Home. An event has no answer for a listener that was not there. + +**A detail view is not a view here**, so the destination it was opened +from stays lit. `app-sidebar` did that by accident (it guarded on +`navItems.some(...)`, so an unmatched name left its highlight alone) +and `bottom-nav` had no such guard and so lit *nothing* — which is why +one looked right and the other looked broken on the same screen. +Whether a view is primary is the shell's fact: `view in VIEW_TAGS` is +passed to `setView`, never re-derived, because a second copy of that +list is a second thing to forget. + +**Nothing is lit until the shell has navigated.** The store starts +empty rather than defaulting to `home`, which is what `app-sidebar`'s +field used to do to match the landing view — a default that is correct +only while `GetDefaultPage()` agrees with it. + +The assertion is `aria-current="page"`, in +`e2e/specs/back-navigation.spec.ts`. That file existed throughout the +bug, covered exactly these journeys, and asserted only +`data-active-view` — the shell's own bookkeeping, which was right the +whole way through — so it was green on the broken build. Same trap as +`layout-overflow.spec.ts` and `page-header`: a spec named for the +behaviour, measuring the plumbing. + **A primary view is cached, not unmounted.** `index.ts` keeps every primary view in the DOM and toggles a `.view-hidden` class, because that is what preserves `scrollTop` across navigation — so diff --git a/e2e/specs/back-navigation.spec.ts b/e2e/specs/back-navigation.spec.ts index 7d16e3b..e82402d 100644 --- a/e2e/specs/back-navigation.spec.ts +++ b/e2e/specs/back-navigation.spec.ts @@ -15,12 +15,49 @@ import { test, expect } from '../support/fixtures.js'; * * What it cannot answer is whether Android's *gesture* reaches the * WebView, which is between the OS and the scaffold. + * + * **And `data-active-view` is not the behaviour.** Every assertion here + * used to be that attribute, which the shell sets on every path + * including `_isBack` — so this file was green throughout #72, in + * which both navs highlighted the view the user had just *left*. The + * shell's own bookkeeping was the one thing that was already right; + * what a person sees is `aria-current`, and that is asserted below as + * well. This is the same trap `layout-overflow.spec.ts` set for #69: a + * spec named for the behaviour, measuring the plumbing. */ type Page = import('@playwright/test').Page; const activeView = (page: Page) => page.getByTestId('main-content'); +/** A common phone, where the bottom bar is the primary navigation. */ +const PHONE = { width: 390, height: 844 }; + +/** + * The nav item for a destination, in whichever navigation is on screen. + * + * Both navs carry a button named `Albums`, and only one of them is ever + * in the accessibility tree — the other is `display: none` — so the + * role query resolves to the one the user can see at this viewport. + * That is the point: the highlight has to be right in both, and #72 was + * two different-looking symptoms of one cause. + */ +const navItem = (page: Page, label: string) => + page.getByRole('button', { name: label, exact: true }); + +/** + * `aria-current="page"` is the accessible fact and the assertion worth + * making; `.active` is a class and could be restyled without breaking + * anything real. + */ +async function expectHighlighted(page: Page, label: string): Promise { + await expect(navItem(page, label)).toHaveAttribute('aria-current', 'page'); +} + +async function expectNotHighlighted(page: Page, label: string): Promise { + await expect(navItem(page, label)).toHaveAttribute('aria-current', 'false'); +} + /** * Open an artist's detail view, which is the deepest ordinary route. * @@ -71,6 +108,105 @@ test.describe('the back gesture', () => { await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); }); + test('leaves the nav highlighting the view it landed on, not the one it left', async ({ + app, + }) => { + await app.getByTestId('nav-albums').click(); + await expectHighlighted(app, 'Albums'); + + await app.getByTestId('nav-tracks').click(); + await expectHighlighted(app, 'Tracks'); + + await app.goBack(); + + // #72, and the half of it the report did not describe: this is + // desktop, and before the shell published the active view *both* + // navs stayed on Tracks. An absent highlight reads as a glitch; a + // confident wrong one is worse, and any back across two primary + // views produced it. + await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); + await expectHighlighted(app, 'Albums'); + await expectNotHighlighted(app, 'Tracks'); + }); + + test('keeps the parent destination lit while a detail view is open', async ({ + app, + }) => { + await app.getByTestId('nav-artists').click(); + await expectHighlighted(app, 'Artists'); + + await openAnArtist(app); + + // A detail view is not a destination in either nav, and the user is + // still inside Artists. `app-sidebar` did this by accident -- it + // guarded on its own item list, so an unmatched name left the + // highlight alone -- and that accident is why the sidebar looked + // right on a detail view while the tab bar lit nothing. This test + // therefore passed before the fix and is here to keep the rule from + // being lost while the others are made to pass; the *tab bar's* + // half of it is the phone test below, which did not. + await expectHighlighted(app, 'Artists'); + + await app.goBack(); + + await expectHighlighted(app, 'Artists'); + }); + + test('the tab bar survives the same journey on a phone', async ({ app }) => { + await app.setViewportSize(PHONE); + + // The reported shape: Albums, open an album, press back. The tab + // bar had a highlight, then no highlight at all, and never got it + // back — `bottom-nav` took the detail view's name, matched it + // against no tab, and lit nothing. + await navItem(app, 'Albums').click(); + await expectHighlighted(app, 'Albums'); + + await app.locator('cover-grid').getByText('Glass Harbour').first().click(); + await expect(activeView(app)).toHaveAttribute( + 'data-active-view', + 'explore-album-details', + ); + await expectHighlighted(app, 'Albums'); + + await app.goBack(); + + await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); + await expectHighlighted(app, 'Albums'); + }); + + test('the drawer sidebar opens on the page you are standing on', async ({ + app, + }) => { + await app.setViewportSize(PHONE); + + await navItem(app, 'Tracks').click(); + await expectHighlighted(app, 'Tracks'); + + // A third symptom of the same cause, found while measuring #72 and + // not in the report: `bottom-nav` mounts its `` when + // the drawer opens, so that copy had heard no `navigate` at all and + // showed its own default — Home, from any page in the app. An event + // has no answer for a listener that was not there; a store does. + await navItem(app, 'More').click(); + + // The element carrying the testid is the `wa-drawer` host, which + // always reports hidden -- what is visible is the `` in its + // shadow root -- so the drawer being open is asserted of the + // sidebar it holds rather than of itself. + const drawer = app.getByTestId('nav-drawer'); + + await expect(drawer.locator('app-sidebar')).toBeVisible(); + await expect(drawer.getByTestId('nav-tracks')).toHaveAttribute( + 'aria-current', + 'page', + ); + await expect(drawer.getByTestId('nav-home')).toHaveAttribute( + 'aria-current', + 'false', + ); + }); + test('an in-app back button consumes exactly one entry', async ({ app }) => { await app.getByTestId('nav-tracks').click(); await openAnArtist(app); diff --git a/frontend/index.ts b/frontend/index.ts index fee2280..cb7396a 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -40,6 +40,7 @@ import { setBasePath } from '@awesome.me/webawesome/dist/webawesome.js'; import { registerBundledIcons } from './src/icons'; import { queueStore } from '@store/queue-store'; import { searchStore } from '@store/search-store'; +import { activeViewStore } from '@store/active-view-store'; import * as Player from '@go/player/player.js'; import * as Queue from '@go/queue/queue.js'; import { GetDefaultPage } from '@go/config/config.js'; @@ -279,6 +280,20 @@ async function handleNavigate( // attribute keeps e2e selectors semantic instead of structural. mainContent.dataset.activeView = view; + // And publishing it as a *value* is what the nav components read. + // They used to learn the active view from the `navigate` event, + // which only the outbound path dispatches -- so a back-navigation + // left both of them highlighting the view it had just left (#72). + // Re-dispatching `navigate` here is not the fix: this file is a + // document listener for it, so that is an infinite loop, and + // "please go to X" is not the statement being made. + // + // `view in VIEW_TAGS` is the primary/detail split, and it is passed + // rather than re-derived because this table is where it is written + // down. A detail view therefore leaves the tab it was opened from + // lit, which is what the report asks for. + activeViewStore.setView(view, view in VIEW_TAGS); + // --- Primary (cacheable) views ---------------------------------------- if (view in VIEW_TAGS) { // Remove any active detail view first diff --git a/frontend/src/components/bottom-nav/bottom-nav.ts b/frontend/src/components/bottom-nav/bottom-nav.ts index 4cae8f2..24f6d7c 100644 --- a/frontend/src/components/bottom-nav/bottom-nav.ts +++ b/frontend/src/components/bottom-nav/bottom-nav.ts @@ -7,6 +7,7 @@ import { designTokens } from '../../styles/tokens.css'; import '../sidebar/app-sidebar.js'; import { nameDialog } from '@utils/name-dialog'; import { ICON_PLAYLIST } from '@utils/icon-language'; +import { ActiveViewController } from '@store/controllers/active-view-controller'; type View = 'home' | 'albums' | 'tracks' | 'playlists'; @@ -114,8 +115,19 @@ export class BottomNav extends LitElement { } `]; - @state() - private activeView = 'home'; + /** + * Which tab is lit, read from the shell rather than tracked here. + * + * This was a `@state()` field set from the `navigate` event, which + * only the outbound path dispatches -- so backing out of a detail + * view left the highlight wherever it had been (#72). It had no + * equivalent of `app-sidebar`'s `navItems.some(...)` guard either, + * so a detail view set it to a name matching no tab and *nothing* + * was lit; that asymmetry is why one nav looked broken and the + * other looked fine. The store answers both: a detail view leaves + * the tab it was opened from lit, in both components. + */ + private activeCtrl = new ActiveViewController(this); /** * Whether the drawer has been asked for. @@ -167,12 +179,9 @@ export class BottomNav extends LitElement { nameDialog(this.drawer); } - private onGlobalNavigate = (e: Event) => { - const detail = (e as CustomEvent<{ view?: string }>).detail; - - if (detail?.view) this.activeView = detail.view; - + private onGlobalNavigate = () => { // A navigation from inside the drawer is the drawer's job done. + // The highlight is not this listener's business any more. this.drawerOpen = false; }; @@ -206,9 +215,11 @@ export class BottomNav extends LitElement {