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 {