Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d347809e6e | ||
|
|
a5ffcc22e3 | ||
|
|
f18691560d | ||
|
|
c84a9069ef |
@@ -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
|
button and the phone's gesture come to disagree about what one press
|
||||||
means.
|
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 `<app-sidebar>` 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
|
**A primary view is cached, not unmounted.** `index.ts` keeps every
|
||||||
primary view in the DOM and toggles a `.view-hidden` class, because that
|
primary view in the DOM and toggles a `.view-hidden` class, because that
|
||||||
is what preserves `scrollTop` across navigation — so
|
is what preserves `scrollTop` across navigation — so
|
||||||
|
|||||||
@@ -15,12 +15,49 @@ import { test, expect } from '../support/fixtures.js';
|
|||||||
*
|
*
|
||||||
* What it cannot answer is whether Android's *gesture* reaches the
|
* What it cannot answer is whether Android's *gesture* reaches the
|
||||||
* WebView, which is between the OS and the scaffold.
|
* 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;
|
type Page = import('@playwright/test').Page;
|
||||||
|
|
||||||
const activeView = (page: Page) =>
|
const activeView = (page: Page) =>
|
||||||
page.getByTestId('main-content');
|
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<void> {
|
||||||
|
await expect(navItem(page, label)).toHaveAttribute('aria-current', 'page');
|
||||||
|
}
|
||||||
|
|
||||||
|
async function expectNotHighlighted(page: Page, label: string): Promise<void> {
|
||||||
|
await expect(navItem(page, label)).toHaveAttribute('aria-current', 'false');
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Open an artist's detail view, which is the deepest ordinary route.
|
* 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');
|
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 `<app-sidebar>` 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 `<dialog>` 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 }) => {
|
test('an in-app back button consumes exactly one entry', async ({ app }) => {
|
||||||
await app.getByTestId('nav-tracks').click();
|
await app.getByTestId('nav-tracks').click();
|
||||||
await openAnArtist(app);
|
await openAnArtist(app);
|
||||||
|
|||||||
@@ -40,6 +40,7 @@ import { setBasePath } from '@awesome.me/webawesome/dist/webawesome.js';
|
|||||||
import { registerBundledIcons } from './src/icons';
|
import { registerBundledIcons } from './src/icons';
|
||||||
import { queueStore } from '@store/queue-store';
|
import { queueStore } from '@store/queue-store';
|
||||||
import { searchStore } from '@store/search-store';
|
import { searchStore } from '@store/search-store';
|
||||||
|
import { activeViewStore } from '@store/active-view-store';
|
||||||
import * as Player from '@go/player/player.js';
|
import * as Player from '@go/player/player.js';
|
||||||
import * as Queue from '@go/queue/queue.js';
|
import * as Queue from '@go/queue/queue.js';
|
||||||
import { GetDefaultPage } from '@go/config/config.js';
|
import { GetDefaultPage } from '@go/config/config.js';
|
||||||
@@ -279,6 +280,20 @@ async function handleNavigate(
|
|||||||
// attribute keeps e2e selectors semantic instead of structural.
|
// attribute keeps e2e selectors semantic instead of structural.
|
||||||
mainContent.dataset.activeView = view;
|
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 ----------------------------------------
|
// --- Primary (cacheable) views ----------------------------------------
|
||||||
if (view in VIEW_TAGS) {
|
if (view in VIEW_TAGS) {
|
||||||
// Remove any active detail view first
|
// Remove any active detail view first
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import { designTokens } from '../../styles/tokens.css';
|
|||||||
import '../sidebar/app-sidebar.js';
|
import '../sidebar/app-sidebar.js';
|
||||||
import { nameDialog } from '@utils/name-dialog';
|
import { nameDialog } from '@utils/name-dialog';
|
||||||
import { ICON_PLAYLIST } from '@utils/icon-language';
|
import { ICON_PLAYLIST } from '@utils/icon-language';
|
||||||
|
import { ActiveViewController } from '@store/controllers/active-view-controller';
|
||||||
|
|
||||||
type View = 'home' | 'albums' | 'tracks' | 'playlists';
|
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.
|
* Whether the drawer has been asked for.
|
||||||
@@ -167,12 +179,9 @@ export class BottomNav extends LitElement {
|
|||||||
nameDialog(this.drawer);
|
nameDialog(this.drawer);
|
||||||
}
|
}
|
||||||
|
|
||||||
private onGlobalNavigate = (e: Event) => {
|
private onGlobalNavigate = () => {
|
||||||
const detail = (e as CustomEvent<{ view?: string }>).detail;
|
|
||||||
|
|
||||||
if (detail?.view) this.activeView = detail.view;
|
|
||||||
|
|
||||||
// A navigation from inside the drawer is the drawer's job done.
|
// 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;
|
this.drawerOpen = false;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -206,9 +215,11 @@ export class BottomNav extends LitElement {
|
|||||||
<li>
|
<li>
|
||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
class=${this.activeView === tab.id ? 'active' : ''}
|
class=${this.activeCtrl.isActive(tab.id)
|
||||||
|
? 'active'
|
||||||
|
: ''}
|
||||||
data-testid="tab-${tab.id}"
|
data-testid="tab-${tab.id}"
|
||||||
aria-current=${this.activeView === tab.id
|
aria-current=${this.activeCtrl.isActive(tab.id)
|
||||||
? 'page'
|
? 'page'
|
||||||
: 'false'}
|
: 'false'}
|
||||||
@click=${() => this.navigate(tab.id)}
|
@click=${() => this.navigate(tab.id)}
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
|||||||
import { designTokens } from '../../styles/tokens.css';
|
import { designTokens } from '../../styles/tokens.css';
|
||||||
|
|
||||||
import type { DragActiveDetail } from '@utils/drag-controller';
|
import type { DragActiveDetail } from '@utils/drag-controller';
|
||||||
|
import { ActiveViewController } from '@store/controllers/active-view-controller';
|
||||||
import {
|
import {
|
||||||
ICON_PLAYLIST,
|
ICON_PLAYLIST,
|
||||||
ICON_AUTOTAG,
|
ICON_AUTOTAG,
|
||||||
@@ -159,11 +160,20 @@ export class AppSidebar extends LitElement {
|
|||||||
/** Delay in ms before a drag-hover triggers navigation. */
|
/** Delay in ms before a drag-hover triggers navigation. */
|
||||||
private static readonly HOVER_NAV_DELAY = 600;
|
private static readonly HOVER_NAV_DELAY = 600;
|
||||||
|
|
||||||
/** Home, because that is where `index.ts` now navigates on startup
|
/**
|
||||||
* (H-8). The sidebar does not hear a `navigate` it did not send,
|
* Which item is lit, read from the shell rather than tracked here.
|
||||||
* so this default is what keeps `aria-current` honest on arrival. */
|
*
|
||||||
@state()
|
* This used to be a `@state()` field defaulting to `home` -- the
|
||||||
private activeView: View = 'home';
|
* landing view -- because "the sidebar does not hear a `navigate`
|
||||||
|
* it did not send". That default was the only honest moment it
|
||||||
|
* ever had: a back-navigation dispatches no `navigate`, so the
|
||||||
|
* highlight stayed on the view the user had just left (#72), and
|
||||||
|
* the copy of this component that `bottom-nav` mounts inside its
|
||||||
|
* drawer opened on `home` from whatever page you were standing on.
|
||||||
|
* The shell publishes the active view now, so there is nothing to
|
||||||
|
* default and nothing to keep in step.
|
||||||
|
*/
|
||||||
|
private activeCtrl = new ActiveViewController(this);
|
||||||
|
|
||||||
@state()
|
@state()
|
||||||
private isDragging = false;
|
private isDragging = false;
|
||||||
@@ -237,10 +247,6 @@ export class AppSidebar extends LitElement {
|
|||||||
'yj-drag-active',
|
'yj-drag-active',
|
||||||
this.onDragActive as EventListener,
|
this.onDragActive as EventListener,
|
||||||
);
|
);
|
||||||
document.addEventListener(
|
|
||||||
'navigate',
|
|
||||||
this.onGlobalNavigate as EventListener,
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
override disconnectedCallback() {
|
override disconnectedCallback() {
|
||||||
@@ -262,10 +268,6 @@ export class AppSidebar extends LitElement {
|
|||||||
'yj-drag-active',
|
'yj-drag-active',
|
||||||
this.onDragActive as EventListener,
|
this.onDragActive as EventListener,
|
||||||
);
|
);
|
||||||
document.removeEventListener(
|
|
||||||
'navigate',
|
|
||||||
this.onGlobalNavigate as EventListener,
|
|
||||||
);
|
|
||||||
this.clearDragHoverTimer();
|
this.clearDragHoverTimer();
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -282,8 +284,9 @@ export class AppSidebar extends LitElement {
|
|||||||
<nav aria-label="Main">
|
<nav aria-label="Main">
|
||||||
<ul>
|
<ul>
|
||||||
${this.navItems.map((item) => {
|
${this.navItems.map((item) => {
|
||||||
|
const active = this.activeCtrl.isActive(item.id);
|
||||||
const classes = [
|
const classes = [
|
||||||
this.activeView === item.id
|
active
|
||||||
? 'active'
|
? 'active'
|
||||||
: '',
|
: '',
|
||||||
this.dragHoverView === item.id
|
this.dragHoverView === item.id
|
||||||
@@ -299,7 +302,7 @@ export class AppSidebar extends LitElement {
|
|||||||
type="button"
|
type="button"
|
||||||
class=${classes}
|
class=${classes}
|
||||||
data-testid="nav-${item.id}"
|
data-testid="nav-${item.id}"
|
||||||
aria-current=${this.activeView === item.id
|
aria-current=${active
|
||||||
? 'page'
|
? 'page'
|
||||||
: 'false'}
|
: 'false'}
|
||||||
@click=${() =>
|
@click=${() =>
|
||||||
@@ -382,19 +385,6 @@ export class AppSidebar extends LitElement {
|
|||||||
private static readonly DROP_VIEWS: Set<View> =
|
private static readonly DROP_VIEWS: Set<View> =
|
||||||
new Set(['playlists']);
|
new Set(['playlists']);
|
||||||
|
|
||||||
/** Keeps the highlighted nav item in sync with navigation that
|
|
||||||
* originates outside the sidebar itself (e.g. the launch-page
|
|
||||||
* dispatch in index.ts). */
|
|
||||||
private onGlobalNavigate = (
|
|
||||||
e: CustomEvent<{ view?: string }>,
|
|
||||||
) => {
|
|
||||||
const view = e.detail.view;
|
|
||||||
|
|
||||||
if (view && this.navItems.some((item) => item.id === view)) {
|
|
||||||
this.activeView = view as View;
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
private onDragActive = (
|
private onDragActive = (
|
||||||
e: CustomEvent<DragActiveDetail>,
|
e: CustomEvent<DragActiveDetail>,
|
||||||
) => {
|
) => {
|
||||||
@@ -460,7 +450,11 @@ export class AppSidebar extends LitElement {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private navigate(view: View) {
|
private navigate(view: View) {
|
||||||
this.activeView = view;
|
// No optimistic highlight: the shell answers, and it answers
|
||||||
|
// synchronously in `handleNavigate` before it awaits anything.
|
||||||
|
// Setting it here as well is the second opinion this fix
|
||||||
|
// removes -- it is what let a click's highlight survive a
|
||||||
|
// navigation the shell then handled differently.
|
||||||
this.dispatchEvent(new CustomEvent('navigate', {
|
this.dispatchEvent(new CustomEvent('navigate', {
|
||||||
detail: { view },
|
detail: { view },
|
||||||
bubbles: true,
|
bubbles: true,
|
||||||
|
|||||||
@@ -0,0 +1,90 @@
|
|||||||
|
/**
|
||||||
|
* Which primary view the app is showing.
|
||||||
|
*
|
||||||
|
* The shell has always known this -- `handleNavigate()` sets
|
||||||
|
* `#main-content`'s `data-active-view` on every path, `_isBack`
|
||||||
|
* included -- and never told anyone. The nav components learned it
|
||||||
|
* from the `navigate` CustomEvent instead, which only the *outbound*
|
||||||
|
* path dispatches: the `popstate` listener calls `handleNavigate()`
|
||||||
|
* directly. So both navs kept highlighting the view you had just left
|
||||||
|
* (#72).
|
||||||
|
*
|
||||||
|
* The fix cannot be a re-dispatch of `navigate`. `index.ts` is itself a
|
||||||
|
* document listener for it, so emitting one from inside
|
||||||
|
* `handleNavigate` is an infinite loop -- and the two statements are
|
||||||
|
* different anyway: `navigate` means *please go to X*, and 28 call
|
||||||
|
* sites across 18 files say it. This says *the active view is now X*,
|
||||||
|
* which only the shell is in a position to say and only once per
|
||||||
|
* navigation.
|
||||||
|
*
|
||||||
|
* Three things about it are load-bearing.
|
||||||
|
*
|
||||||
|
* **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 `<app-sidebar>` on open, and that copy had heard
|
||||||
|
* no `navigate` at all: standing on Albums, the drawer highlighted
|
||||||
|
* Home -- its `activeView` default, which existed to match the landing
|
||||||
|
* view and matched nothing else ever after. An event has no answer for
|
||||||
|
* a listener that was not there; a value does.
|
||||||
|
*
|
||||||
|
* **A detail view is not a view here.** Opening one leaves the primary
|
||||||
|
* view it was opened from lit, which is what #72 asks for and what
|
||||||
|
* `app-sidebar` used to do by accident -- it guarded on
|
||||||
|
* `navItems.some(...)`, so a name matching no item left its highlight
|
||||||
|
* alone. `bottom-nav` had no such guard and so lit nothing on a detail
|
||||||
|
* view. Neither was correct; the sidebar was stale-but-lucky, and
|
||||||
|
* stating the rule once is what makes the two agree.
|
||||||
|
*
|
||||||
|
* **Whether a view is primary is the shell's fact, not this store's.**
|
||||||
|
* `VIEW_TAGS` in `index.ts` is the list, and a copy of it here is a
|
||||||
|
* second list to forget -- so the caller passes the answer it already
|
||||||
|
* has rather than this file re-deriving it.
|
||||||
|
*/
|
||||||
|
|
||||||
|
type Subscriber = () => void;
|
||||||
|
|
||||||
|
class ActiveViewStore {
|
||||||
|
/** Empty until the shell's first navigation, which happens at
|
||||||
|
* startup from `GetDefaultPage()`. Nothing is highlighted for that
|
||||||
|
* moment, which is honest: the alternative is a written-down
|
||||||
|
* default that is right only when the default page agrees with it. */
|
||||||
|
private activeView = '';
|
||||||
|
|
||||||
|
private subscribers = new Set<Subscriber>();
|
||||||
|
|
||||||
|
/** The active primary view, e.g. `albums`. */
|
||||||
|
get(): string {
|
||||||
|
return this.activeView;
|
||||||
|
}
|
||||||
|
|
||||||
|
isActive(view: string): boolean {
|
||||||
|
return this.activeView !== '' && this.activeView === view;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Called by the shell on every navigation, `popstate` included.
|
||||||
|
*
|
||||||
|
* `isPrimary` is `view in VIEW_TAGS` at the call site: a detail
|
||||||
|
* view reports itself and deliberately changes nothing, so the view
|
||||||
|
* it was opened from stays lit until the user picks another one.
|
||||||
|
*/
|
||||||
|
setView(view: string, isPrimary: boolean): void {
|
||||||
|
if (!isPrimary) return;
|
||||||
|
if (view === this.activeView) return;
|
||||||
|
|
||||||
|
this.activeView = view;
|
||||||
|
this.notify();
|
||||||
|
}
|
||||||
|
|
||||||
|
subscribe(fn: Subscriber): () => void {
|
||||||
|
this.subscribers.add(fn);
|
||||||
|
|
||||||
|
return () => this.subscribers.delete(fn);
|
||||||
|
}
|
||||||
|
|
||||||
|
private notify(): void {
|
||||||
|
this.subscribers.forEach((fn) => fn());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
export const activeViewStore = new ActiveViewStore();
|
||||||
@@ -0,0 +1,59 @@
|
|||||||
|
import type {
|
||||||
|
ReactiveController,
|
||||||
|
ReactiveControllerHost,
|
||||||
|
} from 'lit';
|
||||||
|
import { activeViewStore } from '../active-view-store';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* ActiveViewController connects a Lit component to the
|
||||||
|
* ActiveViewStore.
|
||||||
|
*
|
||||||
|
* Usage in a component:
|
||||||
|
*
|
||||||
|
* private activeCtrl = new ActiveViewController(this);
|
||||||
|
*
|
||||||
|
* render() {
|
||||||
|
* const lit = this.activeCtrl.isActive('albums');
|
||||||
|
* }
|
||||||
|
*
|
||||||
|
* It reads through to the store rather than copying the value into a
|
||||||
|
* `@state()` field, which is the point of #72: two components holding
|
||||||
|
* their own idea of the active view is what let them disagree with the
|
||||||
|
* shell and with each other.
|
||||||
|
*/
|
||||||
|
export class ActiveViewController implements ReactiveController {
|
||||||
|
private host: ReactiveControllerHost;
|
||||||
|
private unsubscribe?: () => void;
|
||||||
|
|
||||||
|
constructor(host: ReactiveControllerHost) {
|
||||||
|
this.host = host;
|
||||||
|
host.addController(this);
|
||||||
|
}
|
||||||
|
|
||||||
|
// ===============================================================
|
||||||
|
// LIFECYCLE HOOKS
|
||||||
|
// ===============================================================
|
||||||
|
|
||||||
|
hostConnected(): void {
|
||||||
|
this.unsubscribe = activeViewStore.subscribe(() => {
|
||||||
|
this.host.requestUpdate();
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
hostDisconnected(): void {
|
||||||
|
this.unsubscribe?.();
|
||||||
|
}
|
||||||
|
|
||||||
|
// ===============================================================
|
||||||
|
// DATA ACCESS
|
||||||
|
// ===============================================================
|
||||||
|
|
||||||
|
/** The active primary view, e.g. `albums`. */
|
||||||
|
get current(): string {
|
||||||
|
return activeViewStore.get();
|
||||||
|
}
|
||||||
|
|
||||||
|
isActive(view: string): boolean {
|
||||||
|
return activeViewStore.isActive(view);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -9,6 +9,8 @@ export type { ThemeState, BackgroundShade } from './theme-store';
|
|||||||
export { ThemeController } from './controllers/theme-controller';
|
export { ThemeController } from './controllers/theme-controller';
|
||||||
export { searchStore } from './search-store';
|
export { searchStore } from './search-store';
|
||||||
export { SearchController } from './controllers/search-controller';
|
export { SearchController } from './controllers/search-controller';
|
||||||
|
export { activeViewStore } from './active-view-store';
|
||||||
|
export { ActiveViewController } from './controllers/active-view-controller';
|
||||||
export { shortcutsStore } from './shortcuts-store';
|
export { shortcutsStore } from './shortcuts-store';
|
||||||
export type { ShortcutsState } from './shortcuts-store';
|
export type { ShortcutsState } from './shortcuts-store';
|
||||||
export { ShortcutsController } from './controllers/shortcuts-controller';
|
export { ShortcutsController } from './controllers/shortcuts-controller';
|
||||||
|
|||||||
@@ -3,15 +3,21 @@
|
|||||||
*
|
*
|
||||||
* Three of these are about the thing that makes a second nav dangerous:
|
* Three of these are about the thing that makes a second nav dangerous:
|
||||||
* it has to agree with the first one. `bottom-nav` emits the same
|
* it has to agree with the first one. `bottom-nav` emits the same
|
||||||
* bubbling, composed `navigate` event `app-sidebar` does and listens
|
* bubbling, composed `navigate` event `app-sidebar` does, and reads
|
||||||
* for that event globally, so a navigation from anywhere — a card, a
|
* which tab is lit from `activeViewStore` — the shell's one statement
|
||||||
* detail view, the drawer's own sidebar — moves its highlight too. A
|
* of where the user is — so it follows a navigation from anywhere: a
|
||||||
* tab bar that only tracks its own clicks looks right until the moment
|
* card, a detail view, the drawer's own sidebar, or the back gesture.
|
||||||
* the user arrives somewhere by another route.
|
*
|
||||||
|
* That last one is why the source is the store and not the `navigate`
|
||||||
|
* event these tests used to dispatch. `popstate` dispatches no
|
||||||
|
* `navigate` (index.ts calls `handleNavigate` directly), so a tab bar
|
||||||
|
* listening for the event looked right until the user pressed back —
|
||||||
|
* #72.
|
||||||
*/
|
*/
|
||||||
import { describe, expect, it, beforeEach } from 'vitest';
|
import { describe, expect, it, beforeEach } from 'vitest';
|
||||||
|
|
||||||
import '@components/bottom-nav/bottom-nav';
|
import '@components/bottom-nav/bottom-nav';
|
||||||
|
import { activeViewStore } from '@store/active-view-store';
|
||||||
import type { BottomNav } from '@components/bottom-nav/bottom-nav';
|
import type { BottomNav } from '@components/bottom-nav/bottom-nav';
|
||||||
import { fixture, shadow, shadowAll, update } from '@test/support/render';
|
import { fixture, shadow, shadowAll, update } from '@test/support/render';
|
||||||
import { resetHarness } from '@test/support/harness';
|
import { resetHarness } from '@test/support/harness';
|
||||||
@@ -21,6 +27,12 @@ type Nav = BottomNav;
|
|||||||
const tabs = (el: HTMLElement) =>
|
const tabs = (el: HTMLElement) =>
|
||||||
shadowAll<HTMLButtonElement>(el, 'nav button');
|
shadowAll<HTMLButtonElement>(el, 'nav button');
|
||||||
|
|
||||||
|
/** The testids of whatever the bar says is the current page. */
|
||||||
|
const current = (el: HTMLElement) =>
|
||||||
|
tabs(el)
|
||||||
|
.filter((b) => b.getAttribute('aria-current') === 'page')
|
||||||
|
.map((b) => b.dataset.testid);
|
||||||
|
|
||||||
/** Resolve on one occurrence of an event, or reject loudly on time. */
|
/** Resolve on one occurrence of an event, or reject loudly on time. */
|
||||||
const once = (el: Element, name: string, timeoutMs = 2000) =>
|
const once = (el: Element, name: string, timeoutMs = 2000) =>
|
||||||
new Promise<void>((resolve, reject) => {
|
new Promise<void>((resolve, reject) => {
|
||||||
@@ -70,36 +82,55 @@ describe('bottom-nav', () => {
|
|||||||
it('follows a navigation it did not send', async () => {
|
it('follows a navigation it did not send', async () => {
|
||||||
const el = await fixture<Nav>('bottom-nav');
|
const el = await fixture<Nav>('bottom-nav');
|
||||||
|
|
||||||
document.dispatchEvent(new CustomEvent('navigate', {
|
activeViewStore.setView('tracks', true);
|
||||||
detail: { view: 'tracks' },
|
|
||||||
bubbles: true,
|
|
||||||
composed: true,
|
|
||||||
}));
|
|
||||||
await update(el, {});
|
await update(el, {});
|
||||||
|
|
||||||
const current = tabs(el)
|
expect(current(el)).toEqual(['tab-tracks']);
|
||||||
.filter((b) => b.getAttribute('aria-current') === 'page')
|
|
||||||
.map((b) => b.dataset.testid);
|
|
||||||
|
|
||||||
expect(current).toEqual(['tab-tracks']);
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it('marks exactly one tab current, and none for a view it has no tab for', async () => {
|
it('marks exactly one tab current, and none for a view it has no tab for', async () => {
|
||||||
const el = await fixture<Nav>('bottom-nav');
|
const el = await fixture<Nav>('bottom-nav');
|
||||||
|
|
||||||
document.dispatchEvent(new CustomEvent('navigate', {
|
activeViewStore.setView('settings', true);
|
||||||
detail: { view: 'settings' },
|
|
||||||
bubbles: true,
|
|
||||||
composed: true,
|
|
||||||
}));
|
|
||||||
await update(el, {});
|
await update(el, {});
|
||||||
|
|
||||||
// Settings lives in the drawer, so nothing in the bar is current.
|
// Settings lives in the drawer, so nothing in the bar is current.
|
||||||
// Leaving Home highlighted would be a tab bar lying about where
|
// Leaving Home highlighted would be a tab bar lying about where
|
||||||
// the user is.
|
// the user is.
|
||||||
expect(
|
expect(current(el)).toEqual([]);
|
||||||
tabs(el).filter((b) => b.getAttribute('aria-current') === 'page'),
|
});
|
||||||
).toHaveLength(0);
|
|
||||||
|
it('keeps the parent tab lit while a detail view is open', async () => {
|
||||||
|
const el = await fixture<Nav>('bottom-nav');
|
||||||
|
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
// A detail view reports itself and is not primary, so it changes
|
||||||
|
// nothing. This is the first half of #72: the bar used to take the
|
||||||
|
// name, match it against no tab, and light nothing at all — while
|
||||||
|
// `app-sidebar`, which guarded on its own item list, kept the
|
||||||
|
// highlight. Neither was deliberate and the two disagreed.
|
||||||
|
activeViewStore.setView('explore-album-details', false);
|
||||||
|
await update(el, {});
|
||||||
|
|
||||||
|
expect(current(el)).toEqual(['tab-albums']);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('follows the back path, which dispatches no navigate event', async () => {
|
||||||
|
const el = await fixture<Nav>('bottom-nav');
|
||||||
|
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
activeViewStore.setView('tracks', true);
|
||||||
|
await update(el, {});
|
||||||
|
expect(current(el)).toEqual(['tab-tracks']);
|
||||||
|
|
||||||
|
// What `popstate` does: the shell replays the entry through
|
||||||
|
// `handleNavigate` without dispatching `navigate`. A bar listening
|
||||||
|
// for the event stayed on Tracks — the view just left, confidently
|
||||||
|
// wrong rather than merely blank.
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
await update(el, {});
|
||||||
|
|
||||||
|
expect(current(el)).toEqual(['tab-albums']);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('closes the drawer when a navigation happens', async () => {
|
it('closes the drawer when a navigation happens', async () => {
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import '@components/sidebar/app-sidebar';
|
|||||||
import '@components/library-filter/library-filter';
|
import '@components/library-filter/library-filter';
|
||||||
import '@components/library-status-indicator/library-status-indicator';
|
import '@components/library-status-indicator/library-status-indicator';
|
||||||
import { Events } from '../../src/events';
|
import { Events } from '../../src/events';
|
||||||
|
import { activeViewStore } from '@store/active-view-store';
|
||||||
import { emit, stub, flush, calls, lastArgs } from '@test/support/harness';
|
import { emit, stub, flush, calls, lastArgs } from '@test/support/harness';
|
||||||
import {
|
import {
|
||||||
fixture,
|
fixture,
|
||||||
@@ -49,6 +50,8 @@ describe('<app-sidebar>', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('marks exactly one item as the current page', async () => {
|
it('marks exactly one item as the current page', async () => {
|
||||||
|
activeViewStore.setView('home', true);
|
||||||
|
|
||||||
const el = await fixture('app-sidebar');
|
const el = await fixture('app-sidebar');
|
||||||
|
|
||||||
const current = shadowAll(el, 'li button').filter(
|
const current = shadowAll(el, 'li button').filter(
|
||||||
@@ -72,17 +75,49 @@ describe('<app-sidebar>', () => {
|
|||||||
expect(seen).toEqual(['artists']);
|
expect(seen).toEqual(['artists']);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('moves aria-current to the clicked destination', async () => {
|
it('moves aria-current with the shell, not with the click', async () => {
|
||||||
|
activeViewStore.setView('home', true);
|
||||||
|
|
||||||
const el = await fixture('app-sidebar');
|
const el = await fixture('app-sidebar');
|
||||||
|
|
||||||
shadow<HTMLElement>(el, '[data-testid="nav-genres"]')?.click();
|
shadow<HTMLElement>(el, '[data-testid="nav-genres"]')?.click();
|
||||||
await el.updateComplete;
|
await el.updateComplete;
|
||||||
|
|
||||||
|
// The click asks; it does not answer. The sidebar used to move its
|
||||||
|
// own highlight optimistically, which is the second opinion #72
|
||||||
|
// removed -- one component deciding where the user is, while the
|
||||||
|
// shell decided separately and `bottom-nav` decided a third way.
|
||||||
|
expect(
|
||||||
|
shadow(el, '[data-testid="nav-genres"]')?.getAttribute('aria-current'),
|
||||||
|
).toBe('false');
|
||||||
|
|
||||||
|
// What the shell does with that event, in one line.
|
||||||
|
activeViewStore.setView('genres', true);
|
||||||
|
await update(el, {});
|
||||||
|
|
||||||
expect(
|
expect(
|
||||||
shadow(el, '[data-testid="nav-genres"]')?.getAttribute('aria-current'),
|
shadow(el, '[data-testid="nav-genres"]')?.getAttribute('aria-current'),
|
||||||
).toBe('page');
|
).toBe('page');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('follows the back path, which dispatches no navigate event', async () => {
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
|
||||||
|
const el = await fixture('app-sidebar');
|
||||||
|
|
||||||
|
// `popstate` replays an entry through `handleNavigate` directly, so
|
||||||
|
// there is no `navigate` event to hear -- which is why the sidebar
|
||||||
|
// stayed on the view the user had just left (#72).
|
||||||
|
activeViewStore.setView('tracks', true);
|
||||||
|
await update(el, {});
|
||||||
|
|
||||||
|
expect(
|
||||||
|
shadowAll(el, 'li button')
|
||||||
|
.filter((item) => item.getAttribute('aria-current') === 'page')
|
||||||
|
.map((item) => item.getAttribute('data-testid')),
|
||||||
|
).toEqual(['nav-tracks']);
|
||||||
|
});
|
||||||
|
|
||||||
it('looks the way it did last time', async () => {
|
it('looks the way it did last time', async () => {
|
||||||
const el = await fixture('app-sidebar');
|
const el = await fixture('app-sidebar');
|
||||||
|
|
||||||
|
|||||||
@@ -1,11 +1,13 @@
|
|||||||
/**
|
/**
|
||||||
* The three small stores behind view chrome: the global search term,
|
* The small stores behind view chrome: the global search term, the
|
||||||
* the track list's column set, and the explore cache that keeps detail
|
* active view both navs highlight, the track list's column set, and
|
||||||
* pages from re-fetching what a search already returned.
|
* the explore cache that keeps detail pages from re-fetching what a
|
||||||
|
* search already returned.
|
||||||
*/
|
*/
|
||||||
import { describe, expect, it, beforeEach } from 'vitest';
|
import { describe, expect, it, beforeEach } from 'vitest';
|
||||||
|
|
||||||
import { searchStore } from '@store/search-store';
|
import { searchStore } from '@store/search-store';
|
||||||
|
import { activeViewStore } from '@store/active-view-store';
|
||||||
import { trackListStore } from '@store/tracklist-store';
|
import { trackListStore } from '@store/tracklist-store';
|
||||||
import { exploreCache, ARTIST_IMAGE_CACHE_LIMIT } from '@store/explore-cache';
|
import { exploreCache, ARTIST_IMAGE_CACHE_LIMIT } from '@store/explore-cache';
|
||||||
import { Events } from '../../src/events';
|
import { Events } from '../../src/events';
|
||||||
@@ -80,6 +82,57 @@ describe('search store', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('active view store', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
activeViewStore.setView('home', true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('holds the primary view the shell navigated to', () => {
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
|
||||||
|
expect(activeViewStore.get()).toBe('albums');
|
||||||
|
expect(activeViewStore.isActive('albums')).toBe(true);
|
||||||
|
expect(activeViewStore.isActive('tracks')).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('leaves the primary view lit while a detail view is open', () => {
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
activeViewStore.setView('explore-album-details', false);
|
||||||
|
|
||||||
|
// #72's third finding, made deliberate: a detail view is not a
|
||||||
|
// destination in either nav, and the tab it was opened from is
|
||||||
|
// where the user still is. `app-sidebar` did this by accident (it
|
||||||
|
// guarded on its own item list) and `bottom-nav` did not do it at
|
||||||
|
// all, which is why one looked right and the other looked broken.
|
||||||
|
expect(activeViewStore.get()).toBe('albums');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not notify when the view is unchanged', () => {
|
||||||
|
let notifications = 0;
|
||||||
|
const off = activeViewStore.subscribe(() => {
|
||||||
|
notifications += 1;
|
||||||
|
});
|
||||||
|
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
activeViewStore.setView('albums', true);
|
||||||
|
activeViewStore.setView('explore-album-details', false);
|
||||||
|
off();
|
||||||
|
|
||||||
|
expect(notifications).toBe(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('lights nothing for a view with no name', () => {
|
||||||
|
// The store starts empty rather than defaulting to a view, because
|
||||||
|
// a written-down default is right only while `GetDefaultPage()`
|
||||||
|
// agrees with it. That is only safe if the empty value matches
|
||||||
|
// nothing: `isActive` compares strings, and a component asking
|
||||||
|
// about an id it does not have must not light up.
|
||||||
|
activeViewStore.setView('', true);
|
||||||
|
|
||||||
|
expect(activeViewStore.isActive('')).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('track list store', () => {
|
describe('track list store', () => {
|
||||||
it('starts from the default column set', () => {
|
it('starts from the default column set', () => {
|
||||||
expect(trackListStore.getState().columnIds.length).toBeGreaterThan(0);
|
expect(trackListStore.getState().columnIds.length).toBeGreaterThan(0);
|
||||||
|
|||||||
Reference in New Issue
Block a user