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) {}