diff --git a/e2e/specs/phone-search.spec.ts b/e2e/specs/phone-search.spec.ts new file mode 100644 index 0000000..cdd7c9d --- /dev/null +++ b/e2e/specs/phone-search.spec.ts @@ -0,0 +1,322 @@ +import { test, expect } from '../support/fixtures.js'; + +/** + * #57. Below 600px the top bar is not in the layout, and search is a + * button that opens a modal on the pages where searching means + * anything. + * + * **This is the tier that can answer it, with one honest exception.** + * The shell's breakpoints are media queries, which the component tier + * cannot set — so whether the bar is a grid row, and whether a header + * grows a search button, is a question for a real viewport. What this + * tier *cannot* answer is the reason the surface is a `wa-dialog`: + * #60 read out of the Web Awesome source that `wa-popup` falls back to + * `position: fixed` where there is no Popover API (Chrome 113, the + * reference device) and that `.main-panel`'s `contain: paint` clips a + * fixed descendant. Chromium and WebKit here both have the Popover API, + * so a popup is top-layered and correct, and **an assertion that the + * modal is not clipped would pass on the broken build.** The mechanism + * is asserted in `frontend/test/components/search-dialog.test.ts` + * instead, where "is there a native " is a question a browser + * can answer without lying. + * + * **And it is measured per element.** `layout-overflow.spec.ts` asks + * whether the *shell* needs sideways scrolling and was green throughout + * the defect it is named for; the win this issue is for is vertical and + * belongs to one element, so it is that element's box that is read. + */ +type Page = import('@playwright/test').Page; + +/** The reference device's own viewport, and a common small phone. */ +const DEVICE = { width: 424, height: 439 }; +const PHONE = { width: 390, height: 780 }; + +/** + * Where the top bar is, and how much of the screen it costs. + * + * `contentTop` is measured against the *jobs band* rather than against + * the window, because that band is a real grid row whenever work is in + * flight (#62) and the app under these specs is long-lived — a job + * staged by another file is still in the store. Measuring against zero + * makes this assertion say "and no background job is running", which is + * not what it is for and is not something it can arrange. + */ +const barBox = (page: Page) => + page.evaluate(() => { + const bar = document.querySelector('header.top-bar')!; + const main = document.querySelector('.main-panel')!; + const band = document.querySelector('job-band'); + const cs = getComputedStyle(bar); + + return { + position: cs.position, + height: Math.round(bar.getBoundingClientRect().height), + /** Where the content starts, and where the row above it ends. */ + contentTop: Math.round(main.getBoundingClientRect().top), + aboveBottom: Math.round(band?.getBoundingClientRect().bottom ?? 0), + }; + }); + +test.describe('the phone has no top bar', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(DEVICE); + }); + + test.afterEach(async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + }); + + /** + * The vertical win, measured rather than asserted by the absence of + * an element: `display: none` on the header would satisfy "the bar is + * hidden" while leaving a 3.25em grid row exactly where it was. + */ + test('gives the row back to the content', async ({ app }) => { + const box = await barBox(app); + + // Out of flow, so it takes no row — and 1px rather than 0, because + // it still carries the document's h1. + expect(box.position).toBe('absolute'); + expect(box.height).toBeLessThanOrEqual(1); + + // The content starts where the row above it ends, and there is no + // row above it but the jobs band. On `main` at the time of writing + // the content started 52px down from that point. + expect(box.contentTop).toBe(box.aboveBottom); + }); + + /** + * The wordmark yields its width and not its existence, which is the + * rule `top-bar-fit.ts` already lives by one band up: with the bar + * gone, `display: none` would take this document from one top-level + * heading to none on every page whose own header has no h1 — + * Settings has no `page-header` at all. + */ + test('still has a top-level heading', async ({ app }) => { + await expect( + app.getByRole('heading', { name: 'YellowJacket', level: 1 }), + ).toHaveCount(1); + }); + + /** + * And its four controls are gone from the tab order, not merely from + * sight. A visually-hidden container is still focusable, and tabbing + * into a search box nobody can see is worse than not having one. + */ + test('leaves nothing in the bar to tab into', async ({ app }) => { + for (const tag of [ + 'nav-history', + 'library-filter', + 'search-bar', + 'job-indicator', + ]) { + await expect(app.locator(`header.top-bar ${tag}`)).toBeHidden(); + } + + const focusable = await app.evaluate( + () => + document + .querySelector('header.top-bar')! + .querySelectorAll('input, select, button, a[href]').length, + ); + + // Nothing in the bar is *rendered*, so nothing in it can be + // focused; the controls are display:none, which takes their own + // shadow content with them. + expect(focusable).toBe(0); + }); +}); + +test.describe('search on a phone', () => { + test.beforeEach(async ({ app }) => { + await app.setViewportSize(PHONE); + }); + + test.afterEach(async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + }); + + test('is a button in the view that can be searched', async ({ app }) => { + await app.getByTestId('tab-tracks').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'tracks', + ); + + // Scoped to the view: every cached primary view holds a + // `page-header`, and an unscoped testid is `bottom-nav`'s + // "resolved to 2 elements" trap again. + const trigger = app.locator('track-list page-header search-trigger button'); + + await expect(trigger).toBeVisible(); + await expect(trigger).toHaveAttribute('aria-label', 'Search tracks'); + }); + + /** + * The whole journey, which is the thing the issue asks for: a button, + * a modal, and the results on the page behind it saying what they are + * showing. + */ + test('opens a modal, filters the page, and says so', async ({ app }) => { + await app.getByTestId('tab-tracks').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'tracks', + ); + + await app.locator('track-list page-header search-trigger button').click(); + + const dialog = app.getByTestId('search-dialog'); + + // Attached, not visible: `wa-dialog`'s host is `display: contents`, + // so the element carrying the testid always reports hidden — what + // is visible is the native `` inside it. That awkwardness + // is written down in CLAUDE.md and is why the assertion that this + // is really up is the role query below. + await expect(dialog).toBeAttached(); + + // Named, which `getByRole` can answer and the a11y snapshot cannot + // — the snapshot never prints a dialog's name, named or not. This + // is also the assertion that the dialog is genuinely showing. + await expect( + app.getByRole('dialog', { name: 'Search tracks' }), + ).toBeVisible(); + + // Scoped: the header's own box is still in the document, hidden. + // This is the one moment there are two `search-input`s. + await dialog.getByTestId('search-input').fill('aurora'); + + // Enter hands the screen back, because the results are the page. + await app.keyboard.press('Enter'); + await expect(dialog).not.toBeAttached(); + + // Polled: the box debounces by 150ms, so reading the page once + // straight after closing the dialog can capture the state before + // the term ever reached the store. + await expect + .poll(() => + app.evaluate( + () => + document + .querySelector('[data-testid="main-content"] track-list') + ?.shadowRoot?.querySelector('page-header') + ?.shadowRoot?.querySelector('[data-testid="page-search-scope"]') + ?.textContent?.trim() ?? '', + ), + ) + .toMatch(/matching.*aurora/); + + // And the button says the search is on, in its name rather than + // only in its colour. + await expect( + app.locator('track-list page-header search-trigger button'), + ).toHaveAttribute('aria-label', /aurora/); + + // Leave the app as the next spec expects to find it. + await app.locator('track-list page-header search-trigger button').click(); + await app.getByTestId('search-dialog').getByTestId('search-input').fill(''); + await app.keyboard.press('Escape'); + }); + + /** + * Two of the seven searchable views have no `page-header` — they are + * detail views that filter on the term and say so in their own + * headers. A trigger placed only in `page-header` would leave them + * with a search they can show and no way to set it, which is #24's + * sentence broken in the band it was written for. + */ + test('reaches the playlist detail view too', async ({ app }) => { + await app.getByTestId('tab-playlists').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'playlists', + ); + + // `.playlist-item`, which is what the list renders. Asserted to + // exist rather than skipped on: the seed has a playlist, and a + // spec that quietly skips when its selector stops matching is a + // spec that reports success for a renamed class. + const first = app.locator('playlist-view .playlist-item').first(); + + await expect(first).toBeVisible(); + await first.dblclick(); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'playlist-details', + ); + + await expect( + app.locator('playlist-details search-trigger button'), + ).toBeVisible(); + }); + + /** + * A button that cannot do anything is worse than none — the rule + * `library-status-indicator` was rewritten on. Home has nothing of + * its own to search and is not in the store's map. + */ + test('offers no button where there is nothing to search', async ({ app }) => { + await app.getByTestId('tab-home').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'home', + ); + + await expect( + app.locator('home-view page-header search-trigger button'), + ).toHaveCount(0); + }); + + test('offers no button on a desktop, where the header has a box', async ({ + app, + }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + await app.getByTestId('nav-tracks').click(); + + await expect( + app.locator('track-list page-header search-trigger button'), + ).toHaveCount(0); + await expect(app.locator('header.top-bar search-bar')).toBeVisible(); + }); +}); + +/** + * #148, which #57 inherits: `library-filter` is the only control in the + * app that calls `setSelectedLibrary`, and the bar it lived in is gone + * on a phone. #143 refused to hide it as a fit step for exactly this + * reason, so dropping it here would have been the same trade. + */ +test.describe('the library filter has a home that is not the bar', () => { + test.afterEach(async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + }); + + test('is in Settings, and is reachable from a phone', async ({ app }) => { + await app.setViewportSize(PHONE); + + await app.getByTestId('tab-more').click(); + await app.getByTestId('nav-drawer').getByTestId('nav-settings').click(); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'settings', + ); + + const filter = app.getByTestId('settings-library-filter'); + + await expect(filter).toBeVisible(); + await expect(filter.locator('select')).toBeVisible(); + }); + + test('and it is the same control at every width', async ({ app }) => { + // Not a phone-only copy: "where do I change which library I am + // browsing" having two answers by viewport is the fault, not the + // fix. + await app.setViewportSize({ width: 1440, height: 900 }); + await app.getByTestId('nav-settings').click(); + + await expect(app.getByTestId('settings-library-filter')).toBeVisible(); + await expect(app.locator('header.top-bar library-filter')).toBeVisible(); + }); +}); diff --git a/e2e/specs/top-bar-fit.spec.ts b/e2e/specs/top-bar-fit.spec.ts index 8f8423f..badd8d0 100644 --- a/e2e/specs/top-bar-fit.spec.ts +++ b/e2e/specs/top-bar-fit.spec.ts @@ -26,10 +26,22 @@ type Page = import('@playwright/test').Page; * 600 is the bottom of the Compact band (#24) and where the defect * lands; 899 and 900 straddle `nav-history` appearing (68px more to * find, at the width that just gained the sidebar's labels); 800 is the - * enforced minimum; 390 is a phone, where the answer must be that - * nothing collapses because the media queries already did the work. + * enforced minimum. + * + * **390 is kept, and what it asks changed with #57.** There is no bar + * to fit below 600px any more — it is out of the grid and visually + * hidden — so "nothing hangs out of it" is a claim about an element + * with no row, and would pass on a build that had merely broken the + * bar. Dropping the width would be dropping the one place this file + * can still say something true about a phone, so it asserts the + * *stronger* property instead, below: the bar is out of the layout + * altogether, which is the thing #57 wanted and the thing that makes + * fitting moot. */ -const WIDTHS = [390, 600, 800, 899, 900, 1440]; +const WIDTHS = [600, 800, 899, 900, 1440]; + +/** Where #57 leaves the bar, and where the desktop still has one. */ +const PHONE_WIDTH = 390; /** * A scan whose title is as long as a real one gets. The label is capped @@ -90,6 +102,56 @@ const collapsed = (page: Page) => })); test.describe('the top bar fits the window', () => { + /** + * The phone's answer, which is not "it fits" (#57). + * + * The bar has no grid row below 600px, so measuring its children + * against its content box is measuring a 1px box that is already + * invisible — a fit pass would collapse the wordmark every time and + * report success about nothing, which is why `measureTopBarFit` + * declines to run at all when the bar is out of flow. What is worth + * asserting here is that the fit pass has not quietly started + * *undoing* that: a rule that put the bar back in the layout would + * pass every assertion in this file and cost a 439px screen 12% of + * its height. + */ + test(`the bar is out of the layout at ${PHONE_WIDTH}px, with a job running`, async ({ + app, + testctl, + }) => { + await app.setViewportSize({ width: PHONE_WIDTH, height: 600 }); + await testctl.emit('JobsChanged', [LONG_JOB]); + + // Not merely hidden: `display: none` on the header would satisfy + // "invisible" and leave the 3.25em row exactly where it was. So + // the assertion is that the content starts where the row above it + // ends -- and with a job staged, the row above it is the jobs + // band, which is the whole reason this row could go. + await expect + .poll(() => + app.evaluate(() => { + const bar = document.querySelector('header.top-bar')!; + const main = document.querySelector('.main-panel')!; + const band = document.querySelector('job-band')!; + + return { + position: getComputedStyle(bar).position, + gap: + Math.round(main.getBoundingClientRect().top) - + Math.round(band.getBoundingClientRect().bottom), + }; + }), + ) + .toEqual({ position: 'absolute', gap: 0 }); + + // And the work is still visible, in the band that replaced the + // indicator (#62) — which is what made this row removable at all. + await expect(app.locator('job-indicator')).toBeHidden(); + await expect(app.locator('job-band').locator('job-row')).toHaveCount(1); + + await app.setViewportSize({ width: 1440, height: 900 }); + }); + for (const width of WIDTHS) { test(`no control sits outside the bar at ${width}px, idle`, async ({ app, @@ -108,23 +170,7 @@ 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. - // - // 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(app.locator('job-indicator')).toBeVisible(); await expect.poll(() => overflowingChildren(app)).toEqual([]); }); diff --git a/frontend/index.css b/frontend/index.css index 714699b..2875913 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -412,8 +412,17 @@ body div.sidebar { =================================================================== */ @media (max-width: 599px) { body { + /* **There is no top-bar row here (#57).** Every one of the five + things that bar held has somewhere else to be below 600px: + `nav-history` is the platform's own gesture (gone from 899 + down), the job indicator is `` (#62), the search + box is a modal opened from the view's own header + (`search-trigger`), the library filter is Settings -> + Libraries (#148), and the wordmark is below. That is 3.25em + of a 439 CSS px viewport -- the single biggest vertical win + available on the reference device, which is why #57 asks for + the row rather than for a smaller bar. */ grid-template: - "top-bar" 3.25em "jobs-band" auto "main-panel" 1fr "bottom-bar" auto @@ -433,46 +442,55 @@ body div.sidebar { grid-area: bottom-nav; } - /* The 2em gutters are half a thumb each at this width, and the - subtitle is already gone from 900 down. + /* The bar is out of the layout, and out of it the way the *wordmark* + already goes at desktop widths: visually hidden rather than + `display: none`, because that `h1` is the document's top-level + heading and this app would otherwise have none on the pages whose + own header is empty by design (`page-header` renders no `h1` when + `heading` is '', and Settings has no `page-header` at all). - `min-width: 0` is the load-bearing half. A grid item's implicit - minimum is `auto` -- its content -- so a header whose children - ask for 580px makes the *body* 580px wide inside a 360px - viewport, and `overflow-x: hidden` then hides the right-hand - third of the app rather than fitting it. Every box between the - viewport and the content that must shrink needs this. */ + Its four *controls* are `display: none` below, which is what + keeps them out of the tab order -- a visually-hidden container is + still focusable, and tabbing into a search box nobody can see is + worse than not having one. + + This is `styles/sr-only.css.ts`'s recipe again, written out + because that one is a `CSSResult` for shadow roots and this is + the light DOM. `position: absolute` is also what tells + `services/top-bar-fit.ts` there is no row to fit into. */ .top-bar { - padding-left: 0.75em; - padding-right: 0.75em; - gap: 0.5em; - min-width: 0; + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; overflow: hidden; + clip-path: inset(50%); + white-space: nowrap; + border: 0; + gap: 0; + min-width: 0; } + .top-bar nav-history, + .top-bar library-filter, + .top-bar search-bar, + .top-bar job-indicator { + display: none; + } + + /* `min-width: 0` is load-bearing wherever a box sits between the + viewport and content that must shrink. A grid item's implicit + minimum is `auto` -- its content -- so one child insisting on + 580px makes the *body* 580px wide inside a 360px viewport, and + `overflow-x: hidden` then hides the right-hand third of the app + rather than fitting it. */ .content-area, .main-panel, .bottom-bar { min-width: 0; } - .title { - font-size: 1.1em; - } - - /* The search box is the one header control worth its width; the - library filter is a rarely-changed setting and reachable from - the drawer's Settings. - - `nav-history` is already gone from 899 down. It would belong - here anyway and for a stronger reason than width: the phone has - Back as a gesture or a button the OS owns, and this app hooks it - (`popstate`), so a second Back in the chrome duplicates a - control the platform provides. */ - .top-bar library-filter { - display: none; - } - /* The full-screen now-playing view *is* the transport, so the bar repeating it underneath is 4em of a small screen spent saying the same thing twice -- visible in a screenshot, invisible to @@ -486,11 +504,6 @@ body div.sidebar { body:has(#main-content[data-active-view="now-playing"]) .bottom-bar { display: none; } - - .top-bar search-bar { - flex: 1 1 auto; - min-width: 0; - } } @media (max-width: 599px) { @@ -543,16 +556,12 @@ body job-band { 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. */ + #57 has since done exactly that, so the indicator's own rule now + lives with the other three in the phone block above, where the bar + goes out of the layout in one statement rather than four. What stays + here is the band, and the argument for it. */ @media (max-width: 599px) { - .top-bar job-indicator { - display: none; - } - - /* ...and its rows appear here, in the grid row above the content. + /* The indicator's 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. diff --git a/frontend/src/services/top-bar-fit.ts b/frontend/src/services/top-bar-fit.ts index d253b46..24acaf9 100644 --- a/frontend/src/services/top-bar-fit.ts +++ b/frontend/src/services/top-bar-fit.ts @@ -119,6 +119,16 @@ export const FIT_STEPS: readonly FitStep[] = [ * @returns the ids collapsed, in the order they were given up. */ export function measureTopBarFit(bar: HTMLElement): string[] { + // Below 600px there is no bar to fit (#57): `index.css` takes it + // out of the grid and leaves it visually hidden at 1px, carrying + // nothing but the document's `h1`. Measuring that reports the + // wordmark as overflowing 1px of content box and collapses it every + // time -- true, and about nothing, since the whole bar is already + // invisible. Asking the *computed position* rather than the + // viewport width is what keeps this file free of a breakpoint the + // stylesheet already owns. + if (getComputedStyle(bar).position === 'absolute') return []; + const fits = () => { const style = getComputedStyle(bar); const box = bar.getBoundingClientRect();