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<void> {
+  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']);
+  });
+});

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<void> {
-  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<void>]>) {
     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.

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

e2e/support/fixtures.ts

@@ -140,6 +140,50 @@ export async function navigateTo(page: Page, view: string): Promise<void> {
     .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<void> {
+  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) {}