From ffc9490a3262d6e53e69797930ed5dd1c6b4a6a0 Mon Sep 17 00:00:00 2001 From: Logan Date: Thu, 20 Aug 2026 01:09:07 -0400 Subject: [PATCH] fix(player): put the phone's Now Playing button above the artwork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.expand` is the phone's only route into the full-screen now-playing view. It is absolutely positioned with `z-index: auto` over `.cover-art`, which is a *later* sibling with the same z-index, so the two tie on paint order and the later one wins. An `` costs nothing there; a track with no artwork renders a placeholder `wa-icon`, which takes every click aimed at the button underneath it. So the control did not work whenever the current song had no cover, on the one platform that has no other way in. Nothing to do with the fixture: any library has untagged files. Measured at 390px with elementFromPoint at the button's centre — the icon with a placeholder, the button with an image, and the button either way with the z-index. Chosen over `pointer-events: none` on the art, which would take the cover preview's mouseenter with it, and over reordering the DOM, which leaves the same tie to be won by the same accident in the other direction. This was filed as an e2e flake, and the diagnosis was wrong: it failed on both engines three times across two branches that could not have caused it, and passed on re-run each time, because the spec starts the *first* row of the track list and which track that is depends on the order the scan inserted rows — the same root cause as #156. The new spec picks a track *for* having no artwork, and asserts the placeholder is rendered rather than assuming it, so it cannot quietly go back to measuring the easy case. Two things it has to get right, both already documented traps: the track must be the 90-second one, since a 2-second one finishes before the assertions run; and `library.Track.CoverArt` is empty for all 31 fixture rows, so "the first track with no cover art" selects nothing in particular and picked a short one. Verified by mutation: without the z-index the new spec fails on the click in 30s, and the pre-existing one beside it passes, which is exactly how this survived. Closes #150 --- .planning/NOTES.md | 48 ++++++++++++ e2e/specs/phone-shell.spec.ts | 74 ++++++++++++++++++- .../src/components/now-playing/now-playing.ts | 21 ++++++ 3 files changed, 142 insertions(+), 1 deletion(-) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 13c0469..3db730d 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4029,3 +4029,51 @@ The spec that pins this is two assertions, not one, and that split is deliberate: an uncapped build is *perfectly centred* and fails only the seek-bar width, so a spec asserting centring alone would have passed the regression. + +## The phone's way into Now Playing was under the artwork (measured 2026-08-20) + +`phone-shell.spec.ts`'s "opens the full-screen now playing" failed in CI +on both engines, three times across two branches that could not have +caused it, and passed on re-run each time. It was filed as a flake +(#150). It is not one: **it depends on which track is playing.** + +`.expand` — the phone's only route into `` — is +`position: absolute; inset: 0` inside `.cover-art-wrapper`, and +`.cover-art` is a **later sibling**. Both have `z-index: auto`, so they +tie on paint order and the later one wins. With an `` that costs +nothing; with no artwork the placeholder `wa-icon` renders and takes +every click aimed at the button underneath it. + +Measured at 390px with `elementFromPoint` at the button's centre: + +| playing track | hit test | +|---|---| +| has artwork | `button.expand` | +| no artwork | **`wa-icon`** | +| no artwork, with `z-index: 1` | `button.expand` | + +So on a phone, the only way into the full-screen player stopped working +whenever the current song had no cover — and this has nothing to do with +the fixture: any library has untagged files. + +Three things worth keeping. + +**"Flaky in CI" was the wrong diagnosis and it cost three cycles.** The +spec starts the *first* row of the track list, so which track it plays +is the order the scan inserted rows in — the same root cause as #156, +one spec over. A test whose subject is a hit test has to *choose* the +case that breaks it. + +**The first two hypotheses were both wrong, and both were plausible.** +A custom element's upgrade replacing its own contents, and the cover +preview's `mouseenter` opening a popup under the pointer. Neither +survived contact with `elementFromPoint`, which took a minute and would +have saved the other two cycles. + +**And the spec that pins it needs the 90-second track**, because a +2-second one finishes before the assertions run — the trap +`fixtures.ts` already documents. Note the filter that does *not* work: +`library.Track.CoverArt` is empty for all 31 fixture rows, so "the +first track with no cover art" selects nothing in particular. The +placeholder's presence is asserted instead, which is the property the +test actually depends on. diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index 192084b..897f998 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, LONG_TRACK } from '../support/fixtures.js'; /** * The phone shell (plan 016 B2, phase 1). @@ -138,6 +138,78 @@ test.describe('the shell on a phone', () => { .toHaveAttribute('data-active-view', 'tracks'); }); + /** + * The same journey with a track that has **no cover art** (#150). + * + * The test above starts the *first* row of the track list, so which + * track it plays is the order the scan inserted them in — and the + * answer decided whether it passed. A track with artwork renders an + * ``, which is no obstacle; one without renders a placeholder + * `wa-icon`, which took every click aimed at the button beneath it, + * because that button is absolutely positioned with `z-index: auto` + * and the art is a *later* sibling. They tied, and the later one won. + * + * So this picks a track *for* the property that broke it, which is + * the only way the assertion means anything: the version above passes + * on a broken build roughly two runs in three, which is exactly how + * it came to cost three CI cycles across two branches that could not + * have caused it. + */ + test('opens the full-screen now playing for a track with no art', async ({ + app, + }) => { + // `LONG_TRACK` by name, and not "the first track with no + // CoverArt": the *library* model reports that field empty for + // every row in this fixture (31 of 31), so filtering on it selects + // nothing in particular and picked a 2-second track, which had + // finished before the assertions ran. The placeholder check below + // is what actually holds the property this test needs. + const started = await app.evaluate(async (longTitle) => { + const tracks = (await window.__yjEvents.call( + 'library.Library.GetTracks', + [0], + 10_000, + )) as { FilePath: string; TrackName: string }[]; + + const bare = tracks.find((t) => t.TrackName === longTitle); + + if (!bare) return null; + + await window.__yjEvents.call( + 'queue.Queue.SetQueue', + [[bare.FilePath], 0, false, { type: '', id: 0, label: '' }], + 10_000, + ); + await window.__yjEvents.call('queue.Queue.Play', [], 5_000); + + return bare.TrackName; + }, LONG_TRACK); + + expect(started).toBe(LONG_TRACK); + + await expect(app.getByTestId('now-playing-title')).not.toBeEmpty(); + + // **The placeholder is the whole point**, so it is asserted rather + // than assumed: this test is about the thing that renders when + // there is no artwork. If the fixture ever gives this album a + // cover, this fails and says so instead of passing while measuring + // the easy case. + // + // One selector rather than a chain from the host: Playwright's CSS + // engine pierces an open shadow root, and chaining from the host + // element does not reach into it. + await expect( + app.locator('now-playing .cover-placeholder'), + ).toBeAttached(); + + await app.getByTestId('open-now-playing').click(); + + await expect(app.getByTestId('main-content')) + .toHaveAttribute('data-active-view', 'now-playing'); + + await app.getByTestId('npv-back').click(); + }); + test('offers no way in on a desktop, where the bar is whole', async ({ app }) => { await app.setViewportSize({ width: 1440, height: 900 }); diff --git a/frontend/src/components/now-playing/now-playing.ts b/frontend/src/components/now-playing/now-playing.ts index 0057f1d..860a7a2 100644 --- a/frontend/src/components/now-playing/now-playing.ts +++ b/frontend/src/components/now-playing/now-playing.ts @@ -188,6 +188,27 @@ export class NowPlaying extends LitElement { cursor: pointer; /* The art shows through; this is a target, not a picture. */ color: transparent; + /* **Above the art, or it is not a target at all** (#150). + + This button is absolutely positioned with z-index auto and + the art is a *later* sibling, so the two tie on paint order + and the later one wins. With an that costs nothing -- + an image is not a hit-test obstacle here -- but a track with + no artwork renders a placeholder wa-icon, which is, and it + takes every click aimed at the button underneath it. + + The failure is therefore per *track*, not per build: on a + phone the only way into the full-screen now-playing view + stopped working whenever the current song had no cover. + Measured with elementFromPoint at the button's centre -- + wa-icon with a placeholder, button.expand with an image, and + button.expand either way once this line exists. + + z-index rather than pointer-events: none on the art, which + would take the cover preview's mouseenter with it; and + rather than reordering the DOM, which would leave the same + tie to be won by the same accident in the other direction. */ + z-index: 1; } .expand:focus-visible { -- 2.54.0