diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 4fb55b8..188c78a 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4302,3 +4302,51 @@ compiles against HEAD with a single shim (`SetPlaybackFinishedHandler` gained a `srcErr error` parameter), which makes "did the backend fix cause this" a ten-minute question instead of a full checkout. + +## An overlay band is not a notification, it is a lid (measured 2026-08-20) + +#62 asks for background jobs to become "a notification" on the phone, +and the app has exactly one notification surface, so the first version +of the fix put `` in `notification-host`'s band — which is +`position: fixed` under the header. It renders correctly, it is on top, +it is inside the viewport, and it is unusable. + +At the device's 424x439 viewport a **compact** panel showing two active +jobs is ~216px — half the screen — drawn over the content, with +`pointer-events: auto` so it swallows every tap underneath. Nothing in +the component tier could see it. The e2e suite could: four specs failed, +and *none* of them was about jobs — two `phone-shell` journeys into the +full-screen Now Playing and `header-action-overflow`'s phone case, all +three because the band was intercepting taps meant for the app. + +`` is in the shell's grid instead, as a row between the top +bar and the main panel, so it **pushes**. That is #24's one sentence +("no action is ever unreachable at any supported size") deciding a +layout question: a band that hides the app in order to say the app is +busy has traded the popover's fault for a worse one. + +Two things fell out of it worth keeping: + +- **A finished row in flow is furniture.** The overlay could afford to + keep terminal jobs around; a row that holds the content down after + the work is done cannot. `job-panel` grew `active-only` for the band, + and Settings keeps finished rows because that is where "did the last + scan work" is asked. +- **`job-row` already had the right density.** `variant="compact"` is + described in its own source as "the popover density", which is + exactly what the band is replacing — 216px against 259px for the + same two jobs, and no per-job statistics that a phone has no room + for. + +## The e2e suite is the tier that sees a shell regression (2026-08-20) + +Worth stating because it decided how #62 was verified. The change is +one component, one stylesheet and one line of `index.html`; `make +ui-test` (955 tests) passed on the broken overlay version and so did +`tsc`, `lint` and the whole Go suite. The failure was three specs that +have nothing to do with jobs, failing on `click()` timeouts. + +The corollary for anything that draws over the shell: **run the whole +e2e suite, not the spec you wrote.** A spec written for a feature +asserts the feature works; what a new overlay breaks is everything +else, and only the suite is looking at that. diff --git a/CLAUDE.md b/CLAUDE.md index 43b4d8b..ed31b8f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -593,11 +593,38 @@ rather than renaming them. `ClearFinishedJobs` is global — a Clear under Libraries would discard the index build's history too; a finished row dismisses itself. - The header `job-indicator` is untouched and is still the one view of - everything at once, from every page. One consequence worth knowing - before writing a spec: a section holding a `job-panel` also holds a - `job-details-drawer`, whose own header carries `.header` — so + The header `job-indicator` is still the one view of everything at + once, from every page — **on a desktop.** One consequence worth + knowing before writing a spec: a section holding a `job-panel` also + holds a `job-details-drawer`, whose own header carries `.header` — so `config-section .header` is ambiguous the moment a job exists. + + **Below 600px that indicator stands down and `` takes + over** (#62), because a popover is a *disclosure* and background work + is the one thing a phone should not make you open something to see — + and because #57 deletes the bar it is anchored to and is blocked on + it having somewhere else to live. The band is the same `job-panel`, + so `applyJobControl` and its index-build confirmation come along + rather than being reimplemented; `kinds="*"` is how it says "every + kind", which is what the indicator was for. + + Three things about it are load-bearing. **It is in the layout, not + over it**, as its own grid row above the main panel: the first + version put it in `notification-host`'s fixed band, which reads fine + in a screenshot and is unusable — at 424×439 a compact panel is + ~200px of a 439px screen and it *covers* what is under it, which four + e2e specs caught by failing on taps it was intercepting. **It shows + active work only** (`active-only`), because in flow a finished row is + furniture that keeps the content pushed down after the work is done; + finished rows stay where the work was started, which is #27's rule. + And **it renders nothing above 600px**, from `matchMedia` rather than + a media query, because that decides whether the element *exists* — + Settings already holds four `job-panel`s and a fifth answering for + every kind is `bottom-nav`'s "resolved to 2 elements" trap again. + `index.css` keeps it `display: none` outside the phone for a second + reason: an in-flow grid child with no named area is auto-placed into + one of the shell's rows, which is what the skip link is absolutely + positioned to avoid. - `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments. - `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists. - `mediacontrols` — OS media controls behind one `Handler`: MPRIS over diff --git a/e2e/specs/jobs-on-a-phone.spec.ts b/e2e/specs/jobs-on-a-phone.spec.ts new file mode 100644 index 0000000..f76b639 --- /dev/null +++ b/e2e/specs/jobs-on-a-phone.spec.ts @@ -0,0 +1,204 @@ +import { test, expect } from '../support/fixtures.js'; + +/** + * #62. On a phone, background work is shown in the notification band + * and the header indicator stands down. + * + * The report was that the indicator's popover "is obscured by other UI, + * so it cannot be read while jobs run". Worth saying plainly: **that + * symptom did not reproduce in this tier.** Measured at the device's + * own 424x439 viewport, the popover was neither clipped nor covered — + * `elementFromPoint` at its centre returned the indicator at every + * width tried. So this is not a fix for a stacking bug, and a spec + * asserting one would be a spec asserting something that was never + * true here. + * + * What is true regardless, and is what these assert: + * + * - a popover is a **disclosure**, and it is anchored to a bar 3.25em + * tall on a screen 439px tall. Background work is the one thing a + * phone should not make you open something to see. + * - #57 deletes that bar and is *blocked on this issue*, because the + * indicator needs somewhere else to live first. Somewhere else is + * the band, and the test that matters for #57 is that the bar no + * longer holds the indicator at all. + * + * This is the media-query tier by necessity: a query inside a shadow + * root is answered by the viewport, and `notification-host` decides + * whether the panel *exists* from `matchMedia`. The component tier + * cannot set either. + */ + +type Page = import('@playwright/test').Page; + +const JOBS = [ + { + id: 'phone:scan', + kind: 'library-scan', + state: 'running', + title: 'Scanning Music', + current: 40, + total: 100, + caps: { pausable: true, cancellable: true }, + }, + { + id: 'phone:idx', + kind: 'index-build', + state: 'running', + title: 'Building the search index', + current: 2, + total: 9, + caps: { pausable: true, cancellable: true }, + }, +]; + +/** The panel the band renders. Playwright's CSS engine pierces open + * shadow roots, which is what keeps this one line. */ +const bandPanel = (page: Page) => page.locator('job-band').locator('job-panel'); + +const PHONE = { width: 424, height: 439 }; +const DESKTOP = { width: 1100, height: 800 }; + +test.describe('background jobs on a phone', () => { + test('are shown in the band, without opening anything', async ({ + app, + testctl, + }) => { + await app.setViewportSize(PHONE); + await testctl.emit('JobsChanged', JOBS); + + await expect(bandPanel(app)).toBeVisible(); + + // Both jobs, drawn by real `job-row`s -- asking the rows what they + // hold rather than reading the panel's text, which would pass + // whether or not a row rendered. Playwright's CSS engine pierces + // open shadow roots, which is what makes this one line; + // `querySelectorAll` does not, and stops at `job-panel`. + await expect(bandPanel(app).locator('job-row')).toHaveCount(2); + + await expect( + bandPanel(app).locator('job-row').first(), + ).toContainText('Scanning Music'); + }); + + /** + * The #57 assertion. Not "the indicator is invisible" — that could be + * true because the bar overflowed — but that the shell's own rule + * puts it away at this width. + */ + test('leave the top bar, which is what #57 is waiting for', async ({ + app, + testctl, + }) => { + await app.setViewportSize(PHONE); + await testctl.emit('JobsChanged', JOBS); + await expect(bandPanel(app)).toBeVisible(); + + await expect(app.locator('job-indicator')).toBeHidden(); + }); + + /** + * The property the first attempt at this got wrong, so it is the one + * worth pinning: the band is **in the layout**, not over it. + * + * A fixed band reads fine in a screenshot and is unusable -- at + * 424x439 a compact panel is ~200px of a 439px screen and it covers + * what is under it. Four specs failed on that version, two + * phone-shell journeys and the header's action menu, because the + * panel was intercepting the taps. So: nothing of the app is + * underneath it, and the main panel starts below it. + */ + test('push the content down rather than covering it', async ({ + app, + testctl, + }) => { + await app.setViewportSize(PHONE); + + const before = await app + .getByTestId('main-content') + .evaluate((el) => el.getBoundingClientRect().top); + + await testctl.emit('JobsChanged', JOBS); + await expect(bandPanel(app)).toBeVisible(); + + const after = await app.evaluate(() => { + const band = document.querySelector('job-band') as HTMLElement; + const main = document.querySelector( + '[data-testid="main-content"]', + ) as HTMLElement; + const b = band.getBoundingClientRect(); + const m = main.getBoundingClientRect(); + + // What the browser reports at the band's own centre. If this is + // anything but the band, the band is sitting on top of it. + const hit = document.elementFromPoint( + Math.round(b.x + b.width / 2), + Math.round(b.y + b.height / 2), + ); + + return { + mainTop: m.top, + bandBottom: b.bottom, + withinViewport: b.bottom <= window.innerHeight + 0.5, + hit: hit?.tagName.toLowerCase() ?? null, + }; + }); + + expect({ + pushed: after.mainTop > before, + mainClearsBand: after.mainTop >= after.bandBottom - 0.5, + withinViewport: after.withinViewport, + hit: after.hit, + }).toEqual({ + pushed: true, + mainClearsBand: true, + withinViewport: true, + hit: 'job-band', + }); + }); + + /** + * A running job repaints several times a second. The stack it sits + * beside is `role="status" aria-live="polite"`, and a progress bar + * inside a live region is a screen reader reading a number out over + * and over — so the two are siblings in the band rather than one + * list, and this is what says so. + */ + test('are not inside the live region they sit beside', async ({ + app, + testctl, + }) => { + await app.setViewportSize(PHONE); + await testctl.emit('JobsChanged', JOBS); + await expect(bandPanel(app)).toBeVisible(); + + const insideLiveRegion = await app.evaluate(() => { + const band = document.querySelector('job-band'); + + // Neither the band itself nor anything it is nested in may be a + // live region -- `closest` answers both at once. + return !!band?.closest('[aria-live]') || band?.hasAttribute('aria-live'); + }); + + expect(insideLiveRegion).toBe(false); + }); + + /** + * `bottom-nav` rendering its duplicate `` unconditionally + * broke 30 specs with "resolved to 2 elements" on a viewport where it + * was not even visible. Settings already holds four `job-panel`s, so + * a fifth that answers for *every* kind is the same trap — which is + * why the band decides from `matchMedia` whether the element exists + * rather than hiding it with CSS. + */ + test('do not leave a second panel behind on a desktop', async ({ + app, + testctl, + }) => { + await app.setViewportSize(DESKTOP); + await testctl.emit('JobsChanged', JOBS); + + await expect(app.locator('job-indicator')).toBeVisible(); + await expect(bandPanel(app)).toHaveCount(0); + }); +}); diff --git a/e2e/specs/top-bar-fit.spec.ts b/e2e/specs/top-bar-fit.spec.ts index 747fdd4..8f8423f 100644 --- a/e2e/specs/top-bar-fit.spec.ts +++ b/e2e/specs/top-bar-fit.spec.ts @@ -108,7 +108,23 @@ test.describe('the top bar fits the window', () => { // The indicator has to actually be up, or this test passes by // measuring the idle case under another name. - await expect(app.locator('job-indicator')).toBeVisible(); + // + // Below 600px there is deliberately no indicator to measure: + // #62 stands it down and puts the rows in `` instead, + // in the layout under the bar. So at 390 the assertion is that + // it *is* away and the bar still fits -- which is the same + // property (the bar has nothing hanging out of it) reached by the + // other branch of the same rule, rather than a width quietly + // dropped from the list. + const phone = width < 600; + + await expect(app.locator('job-indicator'))[ + phone ? 'toBeHidden' : 'toBeVisible' + ](); + + if (phone) { + await expect(app.locator('job-band').locator('job-row')).toHaveCount(1); + } await expect.poll(() => overflowingChildren(app)).toEqual([]); }); diff --git a/frontend/index.css b/frontend/index.css index 968cfc4..714699b 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -414,6 +414,7 @@ body div.sidebar { body { grid-template: "top-bar" 3.25em + "jobs-band" auto "main-panel" 1fr "bottom-bar" auto "bottom-nav" auto @@ -522,3 +523,45 @@ body div.sidebar { display: none; } } + +/* Out of the desktop grid entirely. `job-band` renders nothing above + 600px anyway, but an in-flow grid child with no named area is + auto-placed into a row of the shell -- the same trap the skip link is + absolutely positioned to avoid. */ +body job-band { + display: none; +} + +/* #62. The job indicator stands down on the phone, and its work is + shown in the notification band instead (notification-host). + + Three reasons, and the first is the report: its popover is anchored + to the top bar, which is 3.25em here on a viewport 439 CSS px tall, + and it was reported as unreadable behind other UI. The second is + that a popover is a disclosure, and background work is the one thing + a phone should not make you disclose. The third is #57, which + deletes this bar entirely and is blocked on the indicator having + somewhere else to live -- this is that somewhere. + + `display: none` rather than a fit step: `services/top-bar-fit.ts` + already skips children whose computed display is none, so the bar's + measurement simply sees one fewer child, and `[compact]` toggling on + a hidden element costs nothing. */ +@media (max-width: 599px) { + .top-bar job-indicator { + display: none; + } + + /* ...and its rows appear here, in the grid row above the content. + In flow rather than over it: a fixed band reads fine in a + screenshot and is unusable, because at 424x439 a compact panel + is ~200px of a 439px screen and it *covers* what is under it. + Measured, not assumed -- four e2e specs failed on that version, + two phone-shell journeys and the header's action menu, because + the panel was intercepting the taps. */ + body job-band { + display: block; + grid-area: jobs-band; + background-color: var(--yj-bg-elevated, #343a40); + } +} diff --git a/frontend/index.html b/frontend/index.html index 12cfb5e..61688df 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -30,6 +30,14 @@ + +
diff --git a/frontend/index.ts b/frontend/index.ts index ef8145e..a31f1c5 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -38,6 +38,11 @@ import '@components/confirm-dialog/confirm-dialog.ts'; // not know what is going on. It costs a dialog and a table. import '@components/shortcuts-overlay/shortcuts-overlay.ts'; import '@components/jobs/job-indicator.ts'; +// The phone's half of the same thing (#62). Eager because it is part +// of the shell's first paint below 600px, and because a band that has +// to fetch a chunk before it can say the app is busy is late by +// exactly the interval it exists to explain. +import '@components/jobs/job-band.ts'; import '@awesome.me/webawesome/dist/styles/themes/default.css'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import { setBasePath } from '@awesome.me/webawesome/dist/webawesome.js'; diff --git a/frontend/src/components/jobs/job-band.ts b/frontend/src/components/jobs/job-band.ts new file mode 100644 index 0000000..e8a946f --- /dev/null +++ b/frontend/src/components/jobs/job-band.ts @@ -0,0 +1,132 @@ +/** + * The phone's view of background work (#62). + * + * The header `job-indicator` is a *popover*, anchored to a bar 3.25em + * tall on a screen 439 CSS px tall, and it was reported as unreadable + * behind other UI. Two things are wrong with it there regardless of + * that symptom: a popover is a **disclosure**, and background work is + * the one thing a phone should not make you open something to see; and + * #57 deletes the bar it is anchored to, and is blocked on this issue + * precisely because the indicator needs somewhere else to live first. + * + * This is that somewhere. Below 600px the indicator stands down + * (`index.css`) and its work appears here instead. + * + * Four things about it are load-bearing. + * + * **It is the existing `job-panel`, not a second job UI.** Pause, + * cancel, Details and the log all come along — and, more to the point, + * so does `applyJobControl`, which is what carries the "you will + * discard hours of downloading" confirmation for an index build. A + * host drawing its own buttons drops that silently, which is the trap + * #27 already named. + * + * **It is in the layout, not over it**, and that was measured rather + * than assumed. The first version of this put the panel in + * `notification-host`'s fixed band, which reads fine in a screenshot + * and is unusable: at 424x439 a compact panel is ~200px of a 439px + * screen, and it *covers* what is under it. Four e2e specs failed — + * two phone-shell journeys and the header's action menu — because the + * panel was intercepting the taps. A band that hides the app to tell + * you the app is busy is worse than the popover it replaced. In flow + * it pushes instead, so nothing is covered and nothing is unreachable, + * which is #24's one sentence across all three bands. + * + * **It shows active work only.** A finished row that lingers is a + * banner that stays after the work is done, which is the opposite of + * what #62 asks for ("dismissed automatically on completion") and, in + * flow, is furniture that keeps the content pushed down. Finished jobs + * are still shown where the work was started, which is #27's rule and + * unaffected. + * + * **It renders nothing at all above 600px**, from `matchMedia` rather + * than a media query, because this decides whether the element + * *exists*. `bottom-nav` learned that the expensive way: rendering its + * duplicate `` unconditionally put a second copy of every + * `nav-*` testid in the DOM and broke 30 specs on a viewport where it + * was not even visible. Settings already holds four `job-panel`s, so a + * fifth answering for *every* kind is the same trap. + */ +import { LitElement, html, css, nothing } from 'lit'; +import { customElement, state } from 'lit/decorators.js'; + +import { jobStore } from '@store/job-store'; +import { isTerminal } from '@store/job-store'; +import { designTokens } from '../../styles/tokens.css'; +import { PHONE_QUERY } from '../../utils/breakpoints'; +import './job-panel'; + +@customElement('job-band') +export class JobBand extends LitElement { + @state() private phone = false; + + @state() private active = 0; + + private media?: MediaQueryList; + + private unsubscribe?: () => void; + + static override styles = [ + designTokens, + css` + :host { + display: block; + min-width: 0; + } + + /* The panel's own margin is for a settings section; here the + band owns the spacing. */ + job-panel { + margin-top: 0; + padding: 0 0.5em 0.5em; + } + `, + ]; + + private onMedia = (e: MediaQueryListEvent | MediaQueryList) => { + this.phone = e.matches; + }; + + private onJobs = () => { + this.active = jobStore.jobs.filter((job) => !isTerminal(job)).length; + }; + + override connectedCallback(): void { + super.connectedCallback(); + + this.media = window.matchMedia(PHONE_QUERY); + this.phone = this.media.matches; + this.media.addEventListener('change', this.onMedia); + + // The band decides whether to render *at all*, and a panel that + // hides itself cannot tell its host that. + this.unsubscribe = jobStore.subscribe(this.onJobs); + this.onJobs(); + void jobStore.init(); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + this.unsubscribe?.(); + this.media?.removeEventListener('change', this.onMedia); + } + + override render() { + // `hidden` rather than an empty render, so the grid row this + // sits in costs nothing at all while there is no work -- the + // rule `job-panel` already follows one layer down. + this.hidden = !(this.phone && this.active > 0); + + if (this.hidden) return nothing; + + return html` + + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'job-band': JobBand; + } +} diff --git a/frontend/src/components/jobs/job-panel.ts b/frontend/src/components/jobs/job-panel.ts index 5bc5808..25470bc 100644 --- a/frontend/src/components/jobs/job-panel.ts +++ b/frontend/src/components/jobs/job-panel.ts @@ -51,6 +51,14 @@ export class JobPanel extends LitElement { * literal in a template, and one of them is inside an HTMX-adjacent * settings page where a property binding would be one more thing to * remember. + * + * **`*` means every kind**, which is the phone's band (#62) and + * nothing else: there, this panel is standing in for the header + * indicator, whose whole job was to be the one view of everything + * at once. It is spelled `*` rather than taken as the meaning of an + * empty attribute, because empty is what a typo and a missing + * binding both produce and "show everything" is the wrong thing to + * do by accident. Empty still shows nothing. */ @property({ type: String }) kinds = ''; @@ -59,6 +67,31 @@ export class JobPanel extends LitElement { @property({ type: String }) heading = ''; + /** + * Row density, passed to `job-row`. + * + * `full` adds elapsed time and per-job statistics and is what a + * settings section wants, so it stays the default and the four + * existing call sites are unchanged. `compact` is what `job-row` + * itself calls "the popover density", and it is what the phone's + * band uses (#62) — there this panel *is* the popover, on a screen + * 439 CSS px tall, and the full density spent 259 of them. + */ + @property({ type: String }) + density: 'compact' | 'full' = 'full'; + + /** + * Drop finished rows. + * + * For the phone's band (#62), which is *in the layout*: a finished + * row there is a banner that stays after the work is done and keeps + * the content pushed down. Settings keeps them, because that is + * where "did the last scan work" is asked, and a finished row there + * dismisses itself. + */ + @property({ type: Boolean, attribute: 'active-only' }) + activeOnly = false; + @state() private jobs: Job[] = []; @@ -162,9 +195,14 @@ export class JobPanel extends LitElement { } private get mine(): Job[] { - const wanted = this.wanted; + const ofKind = + this.kinds.trim() === '*' + ? this.jobs + : this.jobs.filter((job) => + this.wanted.has(job.kind as JobKind), + ); - return this.jobs.filter((job) => wanted.has(job.kind as JobKind)); + return this.activeOnly ? ofKind.filter((job) => !isTerminal(job)) : ofKind; } private openDetails(id: string) { @@ -196,7 +234,7 @@ export class JobPanel extends LitElement {