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; + } +}