From 218e4f5e991def8c6ecea7a7b8ab887d5671faec Mon Sep 17 00:00:00 2001 From: Logan Date: Thu, 20 Aug 2026 23:42:15 -0400 Subject: [PATCH 1/5] feat(player): give the transport a context, and thumb-sized controls Measured at the reference device's 424x439, every button here was 33x21px -- in the bottom bar and on the full-screen view alike. #56 reports them as "the most important thing in the mobile app and they are tiny", and that is the number behind it. The context is a **property, not a media query**, and that is the whole design. Everywhere else in this app a component states what it drops at phone width itself, because a media query inside a shadow root is answered by the viewport and that is the honest signal. Here the two hosts want different answers at the *same* viewport: on a phone the bar wants three controls sized for a thumb and now-playing-view wants five, larger still. So the host says which context and the viewport says which size band, and neither alone can express it. Play/pause alone goes above the 44px floor. A row of five identical squares says every action is equally likely, which is not true of play -- "large play/pause, adequate prev/next" is the Direction, and a spec caught that the first version had sized all three the same. Two things that fail silently: The desktop bar must not move, and a ` + `; + } - const shuffleClass = this.shuffleMode ? 'active' : ''; + /** Repeat, whose label spells the mode out because one icon covers + * three states. */ + private renderRepeat() { const repeatMode = this.repeatMode; const repeatClasses = [ repeatMode !== 'off' ? 'active' : '', repeatMode === 'one' ? 'repeat-one' : '', ].filter(Boolean).join(' '); + return html` + + `; + } + + /** Previous, play/pause, next — the three that are always drawn, in + * every context and at every width. Only play/pause takes the large + * size: the Direction asks for "large play/pause, adequate + * prev/next", and a row of identical squares says every action here + * is equally likely, which is not true of play. */ + private renderPrimary() { + const playOrPauseIcon = this.player.isPlaying ? 'pause' : 'play'; + const playOrPauseHandler = this.player.isPlaying + ? this.handlePauseClick + : this.handlePlayClick; + + return html` + + + + `; + } + + /** + * Two arrangements, not two components. + * + * `bar` keeps the order it has always had — shuffle, prev, play, + * next, repeat, one row — so nothing about the desktop bar moves. + * `full` puts the primary three on their own row with the secondary + * pair beneath, which the Direction asks for. + * + * **The phone's bar draws three buttons rather than hiding two.** A + * `display: none` control is still in the component's shadow root, + * still in the accessibility tree's markup, and still something a + * `shadowAll('button')[4]` finds — so "the phone has three controls" + * would be true of the pixels and false of the element. They are + * reachable on the full-screen view, which the mini player's art + * opens, and through the global shortcuts. + */ + override render() { + if (this.context === 'full') { + return html` +
${this.renderPrimary()}
+
+ ${this.renderShuffle()}${this.renderRepeat()} +
+ `; + } + return html`
- - - - - + ${this.slim ? nothing : this.renderShuffle()} + ${this.renderPrimary()} + ${this.slim ? nothing : this.renderRepeat()}
`; } diff --git a/frontend/src/components/now-playing-view/now-playing-view.ts b/frontend/src/components/now-playing-view/now-playing-view.ts index d426dd3..ab71ded 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -117,8 +117,27 @@ export class NowPlayingView extends LitElement { .art .placeholder { /* Square, and never taller than the room left over: the art is the one thing here that would happily push the - transport off the bottom of a short phone. */ + transport off the bottom of a short phone. + + **max-height is what actually keeps that promise**, and + it was missing. With a definite width and + a 1:1 aspect-ratio the height is *derived from the width* + and is bounded by nothing: at the reference device's + 424x439 that is a 263px square (60vh) in a box with far + less than 263px left, so the art overflowed its own + centred flex item and drew over the header above and the + title below it. The comment claimed this was handled; + 60vh is a bound on the *viewport*, not on the room left + over, and those differ by however much chrome is above + and below. + + Pre-existing -- screenshotted on main -- and made acute + by #56, which gives the transport 95px more than it had. + Found by reading a screenshot, which is the only tier + that can see it: nothing fails, nothing overflows the + *shell*, and every control is still hittable. */ width: min(100%, 60vh); + max-height: 100%; aspect-ratio: 1; object-fit: cover; border-radius: 12px; @@ -317,7 +336,13 @@ export class NowPlayingView extends LitElement {
- + +
`; diff --git a/frontend/test/components/transport-context.test.ts b/frontend/test/components/transport-context.test.ts new file mode 100644 index 0000000..72aa089 --- /dev/null +++ b/frontend/test/components/transport-context.test.ts @@ -0,0 +1,181 @@ +/** + * The transport in its two contexts (#59, #56). + * + * `player-controls` is one component in two places, and what each place + * wants differs *at the same viewport*: on a phone the bottom bar wants + * three controls sized for a thumb, and `now-playing-view` wants five, + * larger still. So the host states the context and the viewport states + * the size band, and this file pins the half a media query cannot + * express. + * + * **What this tier can and cannot see.** It can see which buttons + * exist, because that is `matchMedia` and a render — and existence is + * the whole of #59. It cannot see the *sizes*: those come from the + * context's custom properties, and a component-tier render has no shell + * around it, so the measurements live in `e2e/specs/phone-transport.spec.ts` + * where there is a real bar in a real viewport. Asserting a pixel here + * would be asserting the fallbacks, which is `ui-visual`'s documented + * blind spot one tier over. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; + +import '@components/audio-player/controls/player-controls'; +import { Events } from '../../src/events'; +import { emit, flush, calls } from '@test/support/harness'; +import { fixture, shadowAll, click } from '@test/support/render'; + +/** Reset the backend-owned state the component reads from. */ +function idle(): void { + emit(Events.TrackChanged, null); + emit(Events.PlaybackStateChanged, { state: 'stopped' }); + emit(Events.QueueModeChanged, { shuffleMode: false, repeatMode: 'off' }); +} + +const names = (el: Element): Array => + shadowAll(el, 'button').map((b) => b.getAttribute('aria-label')); + +/** + * Answer `matchMedia` for the phone query, since the test runner's own + * window is whatever size the browser provider gives it. + * + * It is stubbed rather than resized because what is under test is the + * component's *reaction* to the answer, and a resize would additionally + * be asserting that this runner's viewport can get below 600px. + */ +const realMatchMedia = window.matchMedia; + +function pretendPhone(phone: boolean): void { + window.matchMedia = ((query: string) => ({ + matches: phone && query.includes('599'), + media: query, + addEventListener: () => {}, + removeEventListener: () => {}, + })) as unknown as typeof window.matchMedia; +} + +afterEach(() => { + window.matchMedia = realMatchMedia; +}); + +describe(' in the bar', () => { + beforeEach(() => { + idle(); + }); + + it('keeps all five on a desktop, in the order it always had', async () => { + pretendPhone(false); + + const el = await fixture('player-controls'); + + // Unchanged from before #59, deliberately: this is the desktop bar + // and nothing about it was reported. + expect(names(el)).toEqual([ + 'Shuffle', + 'Previous track', + 'Play', + 'Next track', + 'Repeat: off', + ]); + }); + + it('draws three on a phone, and does not merely hide the other two', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + expect(names(el)).toEqual(['Previous track', 'Play', 'Next track']); + + // The distinction this asserts is the point. A `display: none` + // control is still in the shadow root, still something a positional + // query finds, and still a thing the component claims to have -- + // so "the phone has three controls" would have been true of the + // pixels and false of the element. + expect(shadowAll(el, 'button')).toHaveLength(3); + }); + + it('follows the viewport when it changes, not just at construction', async () => { + pretendPhone(false); + + const el = await fixture('player-controls'); + + expect(names(el)).toHaveLength(5); + + // A desktop window dragged narrow is the phone layout, per plan + // 018's decision 4 -- so this is a real transition and not a + // hypothetical. + (el as unknown as { phone: boolean }).phone = true; + await flush(); + await el.updateComplete; + + expect(names(el)).toEqual(['Previous track', 'Play', 'Next track']); + }); +}); + +describe(' full-screen', () => { + beforeEach(() => { + idle(); + }); + + it('keeps all five on a phone, where the bar keeps three', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // The same viewport, the other answer: this is why the context is a + // property and cannot be a media query. + expect(names(el)).toHaveLength(5); + }); + + it('puts the secondary pair after the primary three, in the DOM', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // Order, not just membership: the secondary controls are drawn on a + // second row, and this is asserted in the DOM because visual order + // and focus order have to agree. A CSS `order` property would move + // them on screen and leave Tab walking the old sequence. + expect(names(el)).toEqual([ + 'Previous track', + 'Play', + 'Next track', + 'Shuffle', + 'Repeat: off', + ]); + }); + + it('still routes every button to the backend', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // Two arrangements, one set of handlers. The regression this + // guards is the reason a second *component* was refused: a second + // template renders buttons wired to nothing, which looks perfect + // in a screenshot and does nothing at all. + for (const name of [ + 'Previous track', + 'Next track', + 'Shuffle', + 'Repeat: off', + ]) { + await click(el, `button[aria-label="${name}"]`); + } + + expect(calls().map((c) => c.path)).toEqual([ + 'queue.Queue.Previous', + 'queue.Queue.Next', + 'queue.Queue.ToggleShuffle', + 'queue.Queue.CycleRepeat', + ]); + }); +}); From 32d4dc2c82178c915d0c2bc3399d90e3a676fb7c Mon Sep 17 00:00:00 2001 From: Logan Date: Thu, 20 Aug 2026 23:42:34 -0400 Subject: [PATCH 2/5] feat(player): slim the phone's mini player to three controls Shuffle, repeat and the queue button leave the phone's bottom bar. They are not gone: all three are on the full-screen Now Playing view, one tap away through the mini player's art, which is the "reachable only from Now Playing" this issue asks for. #55 is what makes the queue half safe -- it is a screen with an entry in the back stack now, rather than a panel with no way out but the button being removed here. Removing a control is only allowed because it is still reachable, which is plan 018's matrix promise, so that is what the spec walks rather than counting buttons. It found that the route did not exist in the state that matters: `now-playing` renders two branches and the no-track one had no `.expand` button on its placeholder, so with nothing loaded there was no way to the full-screen view at all -- and once the queue button left the bar, no way to the queue. The queue is persisted across restarts, so "tracks queued, nothing playing" is a state the app launches into, not a corner. The favourite stays on the bar and was 18x14px, the smallest control in the app, against the 48x48 art beside it. One CSS trap, because it failed silently. The phone block is last in index.css on purpose -- a media query adds no specificity -- but the rule it overrides here is written *nested* inside `.bottom-bar`, so it builds to a descendant selector one class more specific and a bare `#queue-button` lost to it. Being last is not enough when the thing above is more specific. Closes #59 --- frontend/index.css | 24 ++++++++++++ .../src/components/now-playing/now-playing.ts | 37 ++++++++++++++++++- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/frontend/index.css b/frontend/index.css index 2875913..a42aa45 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -535,6 +535,30 @@ body div.sidebar { .bottom-bar volume-control { display: none; } + + /* The queue leaves the phone's bar (#59), because #55 made it a + screen with an entry in the back stack and Now Playing already + carries its own button for it. The route is the mini player's + art -> Now Playing -> the queue, which is the "reachable only + from Now Playing" this issue asks for. + + This is allowed to remove a control only because the control is + still reachable: plan 018's matrix promises that no action is + ever unreachable at any supported size, and that promise is what + `phone-transport.spec.ts` asserts rather than the button count. + + **`.bottom-bar #queue-button`, not `#queue-button`**, and that is + not decoration. The rule this overrides is written *nested* + inside `.bottom-bar`, so it builds to a descendant selector one + class more specific than it looks in the source -- and a bare + `#queue-button` here loses to it, media query or not. Being last + in the file is not enough when the thing above is more specific, + which is the same lesson as this section's own header one level + down: nesting adds specificity the source does not show, and the + failure is silent (the button simply stayed). */ + .bottom-bar #queue-button { + display: none; + } } /* Out of the desktop grid entirely. `job-band` renders nothing above diff --git a/frontend/src/components/now-playing/now-playing.ts b/frontend/src/components/now-playing/now-playing.ts index 860a7a2..8e335a5 100644 --- a/frontend/src/components/now-playing/now-playing.ts +++ b/frontend/src/components/now-playing/now-playing.ts @@ -215,6 +215,17 @@ export class NowPlaying extends LitElement { outline: 2px solid var(--yj-accent, #ffd43b); outline-offset: 2px; } + + /* The favourite is one of the three controls #59 keeps on the + phone's bar, and it was the **smallest control in the app**: + measured at 424x439, 18x14px, against the 48x48 art beside it. + Zero padding around an icon-sized glyph is a reasonable mouse + target and is not a thumb target at all. */ + .fav-btn { + min-width: 44px; + min-height: 44px; + font-size: var(--yj-icon-md); + } } .cover-preview-panel { @@ -446,8 +457,30 @@ export class NowPlaying extends LitElement { return html`
${announcement}
-
-
+ +
+ +
+
+
Date: Thu, 20 Aug 2026 23:42:35 -0400 Subject: [PATCH 3/5] test(player): pin the phone transport, and the desktop bar not moving Ten tests, five of which fail on the build before this. The desktop guard is meant to pass there -- that is its job, and it is the one that caught a three-pixel regression nothing else could see. `openTheQueue` moves to the fixtures, because hiding one button failed ten tests in four files about the back stack and about layout: every one of them opened the queue by clicking `#queue-button`, and so was quietly asserting *which* route exists as well as what the queue does. The route differs by width now and that is the feature. Two smaller things. The play button is named for its action, so an exact 'Play' waits out a fixture track -- 11.1s per test, passing by luck, and it would have failed outright against LONG_TRACK. And the "nothing playing" case clears the queue itself rather than trusting the app not to have played anything: `make e2e` runs one long-lived app across every spec (#168), which is how a deterministic bug first showed up as a flake. --- e2e/specs/phone-transport.spec.ts | 276 ++++++++++++++++++++++++++++ e2e/specs/queue-as-a-screen.spec.ts | 150 +++++++++++---- e2e/specs/queue-overlay.spec.ts | 20 +- e2e/support/fixtures.ts | 44 +++++ 4 files changed, 443 insertions(+), 47 deletions(-) create mode 100644 e2e/specs/phone-transport.spec.ts diff --git a/e2e/specs/phone-transport.spec.ts b/e2e/specs/phone-transport.spec.ts new file mode 100644 index 0000000..a5d56dc --- /dev/null +++ b/e2e/specs/phone-transport.spec.ts @@ -0,0 +1,276 @@ +import { test, expect } from '../support/fixtures.js'; + +/** + * The phone's transport (#59, #56). + * + * #56 reports that "the playback controls are the most important thing + * in the mobile app and they are tiny". Measured at the reference + * device's 424x439 before this, every one of them was **33x21px**, and + * the favourite beside them — which #59 keeps on the bar — was + * **18x14px**, the smallest control in the app. + * + * #59 is what makes the sizes affordable: five controls plus a queue + * button at 44px does not fit 424 CSS px, so the bar carries three and + * the rest are on the full-screen view. + * + * **The assertion that matters is not the pixel count.** Plan 018's + * matrix promises that *no action is ever unreachable at any supported + * size*, and #59 removes three controls from the phone's bar — so the + * first thing this file checks is that all three are still reachable, + * by walking the route a user would. A spec that only measured the + * survivors would be green on a build that had made shuffle + * unreachable, which is the failure mode this pair of issues is one + * mistake away from. + */ +type Page = import('@playwright/test').Page; + +/** The reference device's real viewport. */ +const DEVICE = { width: 424, height: 439 }; +const PHONE = { width: 390, height: 780 }; +const DESKTOP = { width: 1280, height: 800 }; + +/** + * The touch-target floor. 44px is what #56's Findings name and what + * #55's queue header was sized to, so the app has one number. + */ +const TARGET = 44; + +/** The play button is named for its action, not its identity. */ +const PLAY_PAUSE = /^(Play|Pause)$/; + +const barControls = (page: Page) => + page.locator('audio-player player-controls'); + +/** + * `name` may be a regex, and for play/pause it must be: that button is + * named for the *action*, so it is "Pause" while a track runs and + * "Play" when it stops. An exact 'Play' made these tests wait out a + * fixture track (11.1s each, passing by luck) and would have failed + * outright against `LONG_TRACK`. A test about a control's size does not + * care what the transport is doing. + */ +async function sizeOf( + page: Page, + name: string | RegExp, +): Promise<[number, number]> { + const box = await page + .getByRole('button', { name, exact: typeof name === 'string' }) + .boundingBox(); + + expect(box, `no button named ${name}`).not.toBeNull(); + + return [box!.width, box!.height]; +} + +/** Put something in the queue, so the transport has a track to act on. */ +async function stageATrack(page: Page): Promise { + await page.evaluate(async () => { + const tracks = (await window.__yjEvents.call( + 'library.Library.GetTracks', + [0], + 10_000, + )) as { FilePath: string }[]; + + await window.__yjEvents.call( + 'queue.Queue.SetQueue', + [tracks.slice(0, 4).map((t) => t.FilePath), 0, false, { type: '', id: 0, label: '' }], + 10_000, + ); + }); +} + +test.describe('the phone bar carries three controls', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(DEVICE); + await stageATrack(app); + }); + + test('drops shuffle, repeat and the queue from the bar', async ({ app }) => { + const bar = barControls(app); + + await expect(bar.getByRole('button', { name: 'Previous track' })).toBeVisible(); + await expect(bar.getByRole('button', { name: 'Next track' })).toBeVisible(); + + // Not in the bar's own subtree. Asserted against the bar rather + // than the page, because the whole point is that they moved rather + // than went away -- a page-wide `not.toBeVisible()` would fail the + // moment Now Playing is open and would be asserting the wrong + // thing besides. + await expect(bar.getByRole('button', { name: 'Shuffle' })).toHaveCount(0); + await expect(bar.getByRole('button', { name: /^Repeat/ })).toHaveCount(0); + await expect(app.locator('#queue-button')).toBeHidden(); + }); + + /** + * The promise, walked. Every control #59 takes off the bar is + * reachable from the mini player's art in one tap. + */ + test('leaves every removed control reachable from Now Playing', async ({ + app, + }) => { + await app.getByTestId('open-now-playing').click(); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'now-playing', + ); + + await expect(app.getByRole('button', { name: 'Shuffle' })).toBeVisible(); + await expect(app.getByRole('button', { name: /^Repeat/ })).toBeVisible(); + await expect(app.getByRole('button', { name: 'Show the queue' })).toBeVisible(); + }); + + test('sizes what is left for a thumb', async ({ app }) => { + for (const name of ['Previous track', 'Next track']) { + const [w, h] = await sizeOf(app, name); + + expect(w, `${name} width`).toBeGreaterThanOrEqual(TARGET); + expect(h, `${name} height`).toBeGreaterThanOrEqual(TARGET); + } + + // Play is deliberately bigger than its neighbours: a row of + // identical squares says every action is equally likely, which is + // not true of play. + const [pw, ph] = await sizeOf(app, PLAY_PAUSE); + const [nw] = await sizeOf(app, 'Next track'); + + expect(ph).toBeGreaterThanOrEqual(TARGET); + expect(pw).toBeGreaterThan(nw); + }); + + /** + * The favourite was 18x14 and is one of the three controls #59 + * keeps, so it is part of this issue rather than a nicety. + */ + test('sizes the favourite, which was the smallest control in the app', async ({ + app, + }) => { + const fav = app + .locator('now-playing') + .getByRole('button', { name: /Favorites$/ }); + + const box = await fav.boundingBox(); + + expect(box).not.toBeNull(); + expect(box!.width).toBeGreaterThanOrEqual(TARGET); + expect(box!.height).toBeGreaterThanOrEqual(TARGET); + }); + + /** + * **The route to the queue must not depend on what is playing.** + * + * `now-playing` renders two branches, and the no-track one had no + * `.expand` button on its placeholder — so with nothing loaded there + * was no way to Now Playing, and once #59 takes the queue button off + * the bar that makes the *queue* unreachable. The queue is persisted + * across restarts, so "tracks queued, nothing playing" is a state the + * app launches into. + * + * This is asserted with the queue explicitly emptied rather than by + * relying on the app not having played anything: `make e2e` runs one + * long-lived app across every spec file (#168), so "no track loaded" + * is otherwise whatever the file before this one left behind — which + * is how the underlying fault first showed up as a flake in a spec + * about something else. + */ + test('reaches the queue with nothing playing', async ({ app }) => { + await app.evaluate(async () => { + await window.__yjEvents.call('queue.Queue.Clear', [], 10_000); + }); + + await expect(app.getByTestId('open-now-playing')).toBeVisible(); + + await app.getByTestId('open-now-playing').click(); + await app.getByTestId('npv-queue').click(); + + await expect(app.locator('#queue-panel')).toHaveAttribute('open', ''); + }); + + test('still fits, with nothing to scroll sideways to', async ({ app }) => { + const fit = await app.evaluate(() => ({ + scroll: document.body.scrollWidth, + client: document.body.clientWidth, + })); + + expect(fit.scroll).toBe(fit.client); + }); +}); + +test.describe('the full-screen transport is the page', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(DEVICE); + await stageATrack(app); + await app.getByTestId('open-now-playing').click(); + }); + + test('draws all five, larger than the bar draws any', async ({ app }) => { + const [pw, ph] = await sizeOf(app, PLAY_PAUSE); + + expect(pw).toBeGreaterThanOrEqual(56); + expect(ph).toBeGreaterThanOrEqual(56); + + for (const name of ['Shuffle', 'Previous track', 'Next track']) { + const [w, h] = await sizeOf(app, name); + + expect(w, `${name} width`).toBeGreaterThanOrEqual(TARGET); + expect(h, `${name} height`).toBeGreaterThanOrEqual(TARGET); + } + }); + + test('fits at both phone widths', async ({ app }) => { + for (const size of [DEVICE, PHONE]) { + await app.setViewportSize(size); + + const fit = await app.evaluate(() => ({ + scroll: document.body.scrollWidth, + client: document.body.clientWidth, + })); + + expect(fit.scroll, `${size.width}px`).toBe(fit.client); + } + }); +}); + +/** + * **The desktop bar is not what either issue is about, and must not + * move.** Both are `Platform/Android`; this is the guard that says so + * in a way a build can check. + * + * It caught a real regression while it was being written: a generic + * `font-size` on the buttons took them from the UA stylesheet's 13.3px + * to the shell's 16px and grew every one from 33x21 to 36x24 — a + * change nobody asked for, invisible to every other assertion here. + */ +test.describe('the desktop bar is untouched', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(DESKTOP); + await stageATrack(app); + }); + + test('keeps all five controls and the queue button', async ({ app }) => { + const bar = barControls(app); + + for (const name of ['Shuffle', 'Previous track', 'Next track']) { + await expect(bar.getByRole('button', { name })).toBeVisible(); + } + + await expect(bar.getByRole('button', { name: /^Repeat/ })).toBeVisible(); + await expect(app.locator('#queue-button')).toBeVisible(); + }); + + test('keeps them exactly the size they were', async ({ app }) => { + const sizes = await barControls(app).evaluate((el) => + [...el.shadowRoot!.querySelectorAll('button')].map((b) => { + const r = b.getBoundingClientRect(); + + return `${Math.round(r.width)}x${Math.round(r.height)}`; + }), + ); + + // Measured on `main` before this change, at 1280x800 and at 424x439 + // alike. Written down as a literal rather than as "not bigger", + // because the regression was three pixels and a range would have + // swallowed it. + expect(sizes).toEqual(['33x21', '33x21', '33x21', '33x21', '33x21']); + }); +}); diff --git a/e2e/specs/queue-as-a-screen.spec.ts b/e2e/specs/queue-as-a-screen.spec.ts index f6de2fd..5767a91 100644 --- a/e2e/specs/queue-as-a-screen.spec.ts +++ b/e2e/specs/queue-as-a-screen.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, openTheQueue } from '../support/fixtures.js'; /** * #55 — the queue is a *place* while it covers the content, and a @@ -37,15 +37,37 @@ const DEVICE = { width: 424, height: 439 }; /** Wide enough that the queue is a column: 1280 − 200 − 320 ≥ 480. */ const DESKTOP = { width: 1280, height: 800 }; +/** + * The Compact band, where the queue is a *screen* (644 − 320 < 480) and + * the bottom bar still carries its button. + * + * Two of these tests need both facts at once and only this band has + * them: below 600px #59 takes the button off the bar, so there is no + * toggle to re-press and the queue is opened from Now Playing — which + * is itself a detail view, so "the destination stays lit" is vacuously + * true there rather than tested. + */ +const COMPACT = { width: 700, height: 600 }; + const activeView = (page: Page) => page.getByTestId('main-content'); const queue = (page: Page) => page.locator('#queue-panel'); const toggle = (page: Page) => page.locator('#queue-button'); +/** + * Whether the queue is up. + * + * The panel's own attribute rather than the toggle's `aria-expanded`, + * because below 600px there is no toggle to ask (#59) — and the panel + * is the one fact both of them reflect anyway. + */ async function expectQueue(page: Page, open: boolean): Promise { - await expect(toggle(page)).toHaveAttribute( - 'aria-expanded', - String(open), - ); + const panel = queue(page); + + if (open) { + await expect(panel).toHaveAttribute('open', ''); + } else { + await expect(panel).not.toHaveAttribute('open', ''); + } } test.describe('the queue is a screen where it covers the content', () => { @@ -55,12 +77,17 @@ test.describe('the queue is a screen where it covers the content', () => { await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); }); + // On a phone the queue is opened from Now Playing (#59), so the page + // *underneath* it is `now-playing` and the journey is two entries + // deep: albums -> now-playing -> queue. That is the real route a user + // takes, which is why these do not reach for the shortcut. + test('back closes the queue and leaves the page where it was', async ({ app, }) => { await expect(queue(app)).toHaveAttribute('overlay', ''); - await toggle(app).click(); + await openTheQueue(app); await expectQueue(app, true); await app.goBack(); @@ -69,27 +96,31 @@ test.describe('the queue is a screen where it covers the content', () => { // The page underneath is untouched. Before #55 this was the // *previous* view, because the queue was not in the stack at all // and back spent an entry navigating something nobody could see. - await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); + await expect(activeView(app)).toHaveAttribute( + 'data-active-view', + 'now-playing', + ); }); test('costs exactly one entry, so the next press navigates', async ({ app, }) => { - await toggle(app).click(); + await openTheQueue(app); await expectQueue(app, true); await app.goBack(); await expectQueue(app, false); + await expect(activeView(app)).toHaveAttribute( + 'data-active-view', + 'now-playing', + ); await app.goBack(); - // Whatever the launch page is, it is not Albums — the point is that - // this press moved the app rather than being swallowed by a queue - // that had already closed. - await expect(activeView(app)).not.toHaveAttribute( - 'data-active-view', - 'albums', - ); + // Exactly one entry each: the second press leaves Now Playing for + // the page it was opened from, rather than being swallowed by a + // queue that had already closed. + await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); }); /** @@ -116,15 +147,9 @@ test.describe('the queue is a screen where it covers the content', () => { await app.keyboard.press('Escape'); }, ], - [ - 'the toggle it was opened from', - async (app: Page) => { - await toggle(app).click(); - }, - ], ] as Array<[string, (app: Page) => Promise]>) { test(`${name} leaves no entry behind`, async ({ app }) => { - await toggle(app).click(); + await openTheQueue(app); await expectQueue(app, true); await dismiss(app); @@ -132,7 +157,10 @@ test.describe('the queue is a screen where it covers the content', () => { await app.goBack(); - await expect(activeView(app)).not.toHaveAttribute( + // One press, one screen: Now Playing is what the queue was opened + // from, so leaving it lands on Albums. An orphaned entry would + // have spent this press on nothing and left it here. + await expect(activeView(app)).toHaveAttribute( 'data-active-view', 'albums', ); @@ -149,18 +177,6 @@ test.describe('the queue is a screen where it covers the content', () => { * `back-navigation.spec.ts` gives: the class was right throughout the * bug that rule exists for. */ - test('leaves the tab it was opened from highlighted', async ({ app }) => { - await expect( - app.getByRole('button', { name: 'Albums', exact: true }), - ).toHaveAttribute('aria-current', 'page'); - - await toggle(app).click(); - await expectQueue(app, true); - - await expect( - app.getByRole('button', { name: 'Albums', exact: true }), - ).toHaveAttribute('aria-current', 'page'); - }); /** * With the panel spanning the whole width the scrim has no uncovered @@ -168,7 +184,7 @@ test.describe('the queue is a screen where it covers the content', () => { * full-screen surface. Measured at 424×439 before #55: **25×21px**. */ test('offers a way out a thumb can hit', async ({ app }) => { - await toggle(app).click(); + await openTheQueue(app); const box = await app .getByRole('button', { name: 'Close queue' }) @@ -204,7 +220,7 @@ test('the panel stays out of the paint-contained region', async ({ app }) => { // from it — and because the host drops `paint` from its own // containment deliberately in overlay mode, so a closed panel answers // a different question. - await toggle(app).click(); + await openTheQueue(app); await expectQueue(app, true); const ancestry = await app.evaluate(() => { @@ -235,6 +251,66 @@ test('the panel stays out of the paint-contained region', async ({ app }) => { } }); +/** + * Two properties need the queue to be a *screen* and the bar to still + * have its button, and only the Compact band has both — below 600px #59 + * takes the button off the bar. + */ +test.describe('a screen opened from the bar', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(COMPACT); + await app.getByTestId('nav-albums').click(); + await expect(activeView(app)).toHaveAttribute('data-active-view', 'albums'); + await expect(queue(app)).toHaveAttribute('overlay', ''); + }); + + /** + * A detail view leaves the destination it was opened from lit + * (`active-view-store`, #72), and the queue inherits that — it is + * published with `isPrimary: false`, so `isActive('albums')` is still + * true underneath it. + * + * `aria-current` rather than a class, for the reason + * `back-navigation.spec.ts` gives: the class was right throughout the + * bug that rule exists for. + */ + test('leaves the destination it was opened from highlighted', async ({ + app, + }) => { + // By testid, not by role: at 700px the sidebar is in icon mode, so + // what the item is *named* is a different question from which item + // it is. The assertion is still `aria-current`, which is the + // accessible fact. + const albums = app.getByTestId('nav-albums'); + + await expect(albums).toHaveAttribute('aria-current', 'page'); + + await toggle(app).click(); + await expectQueue(app, true); + + await expect(albums).toHaveAttribute('aria-current', 'page'); + }); + + /** The toggle is a fourth way out, and it unwinds the entry like the + * other three — through the panel's attribute, not its own handler. */ + test('closes from the same toggle, leaving no entry behind', async ({ + app, + }) => { + await toggle(app).click(); + await expectQueue(app, true); + + await toggle(app).click(); + await expectQueue(app, false); + + await app.goBack(); + + await expect(activeView(app)).not.toHaveAttribute( + 'data-active-view', + 'albums', + ); + }); +}); + /** * The column is not a place. Somebody docked it; back must not undock * it, and navigating to another view must not take it away. diff --git a/e2e/specs/queue-overlay.spec.ts b/e2e/specs/queue-overlay.spec.ts index 1000936..3c52d48 100644 --- a/e2e/specs/queue-overlay.spec.ts +++ b/e2e/specs/queue-overlay.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, openTheQueue } from '../support/fixtures.js'; /** * #24 — the queue panel does not take the page's width away from it. @@ -54,15 +54,15 @@ const shellGeometry = (page: import('@playwright/test').Page) => }; }); -async function openQueue(page: import('@playwright/test').Page) { - const toggle = page.locator('#queue-button'); - - if ((await toggle.getAttribute('aria-expanded')) !== 'true') { - await toggle.click(); - } - - await expect(toggle).toHaveAttribute('aria-expanded', 'true'); -} +/** + * Opening the queue is `openTheQueue`, which takes the route this + * viewport offers. It used to be a local helper that clicked + * `#queue-button` unconditionally, and #59 hid that button below + * 600px -- so the two phone bands here failed on a build where the + * queue was working perfectly, having been asserting *how* it opens as + * much as what it does. + */ +const openQueue = openTheQueue; test.describe('an open queue leaves the content its width', () => { for (const band of BANDS) { diff --git a/e2e/support/fixtures.ts b/e2e/support/fixtures.ts index 72a0327..e495687 100644 --- a/e2e/support/fixtures.ts +++ b/e2e/support/fixtures.ts @@ -140,6 +140,50 @@ export async function navigateTo(page: Page, view: string): Promise { .waitFor({ state: 'attached' }); } +/** + * Open the queue the way a user at this viewport would. + * + * **The route differs by width and that is the feature, not an + * inconvenience.** Above 600px the bottom bar carries a queue button. + * Below it that button is gone (#59) and the queue is reached from the + * full-screen Now Playing view, which the mini player's art opens — + * "reachable only from Now Playing", which is what the issue asks for. + * + * It is here rather than in one spec because four files need it, and + * because a spec that hard-codes `#queue-button` is quietly asserting + * *which* route exists as well as what the queue does. Four of them + * were, which is how hiding one button failed ten tests about + * something else. + * + * The width is read from the page rather than passed, so a caller that + * resizes and then opens does not have to say so twice. + */ +export async function openTheQueue(page: Page): Promise { + const toggle = page.locator('#queue-button'); + + if (await toggle.isVisible()) { + if ((await toggle.getAttribute('aria-expanded')) !== 'true') { + await toggle.click(); + } + + await expect(toggle).toHaveAttribute('aria-expanded', 'true'); + + return; + } + + // The phone: through Now Playing. `open-now-playing` is the mini + // player's art, which is a button only below 600px. + if ( + (await page.getByTestId('main-content').getAttribute('data-active-view')) !== + 'now-playing' + ) { + await page.getByTestId('open-now-playing').click(); + } + + await page.getByTestId('npv-queue').click(); + await expect(page.locator('#queue-panel')).toHaveAttribute('open', ''); +} + /** Thin client for the dev-only /__test/ surface (backend/testctl). */ export class TestCtl { constructor(private readonly baseURL: string) {} From c7e5a4f086c9051c0d699c9cb8e59c88db82acb5 Mon Sep 17 00:00:00 2001 From: Logan Date: Thu, 20 Aug 2026 23:42:36 -0400 Subject: [PATCH 4/5] docs(player): record the phone transport, and four silent failures The model in CLAUDE.md beside the volume rule it qualifies; the measurements and the four things that cost a cycle each in NOTES.md, dated. Three of the four are invisible to every assertion in the repo: a button not inheriting its font, a nested rule out-specifying a later one, and art whose height is bounded by nothing. --- .planning/NOTES.md | 72 ++++++++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 49 +++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index dcc33c0..6dd7de0 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4487,3 +4487,75 @@ is not orphaned" and "a docked column is not in the stack" are both vacuously true of a build that pushes no entry at all. Reverting the source and re-running is what established which were which, and the file says so in its header rather than implying all nine reproduce. + +## The phone's transport, and three things that only a screenshot or a stash could see (measured 2026-08-21) + +#59 and #56 were done as one PR — argued on #73 first — because they are +the same row of pixels: one removes controls from the phone's bar and +the other enlarges what is left, and both are one property on +`player-controls`. Measured at 424x439 before: + +| control | before | after | +|---|---|---| +| bar: shuffle / prev / play / next / repeat | 33x21 each | prev/next 44, play 56, shuffle+repeat moved | +| bar: favourite | **18x14** | 44x44 | +| bar: queue button | 33x29 | gone (#59) | +| Now Playing: all five | 33x21 each | 44, play 64 | +| desktop bar: all five | 33x21 | **33x21** | + +Four things cost a cycle each and are worth keeping. + +**A `