diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 188c78a..40ad8c7 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4350,3 +4350,82 @@ 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. + +## `contain: paint` is why a Web Awesome popup is clipped on the device (read 2026-08-20, applied 2026-08-21) + +Recorded here because it outlives #57 and #60 both, and because the +next person to reach for a floating surface will reach for `wa-popup`. + +`wa-popup` renders `
` and feature-detects the +Popover API, falling back to `strategy: "fixed"` where there is none. +The reference device is Chrome 113 and `popover` is Chrome 114, so +every popup in the app takes the fallback there. `position: fixed` +escapes ancestor *overflow* but not `contain: paint`, which makes an +element a containing block for fixed descendants **and clips them** — +and `index.css` puts `contain: layout style paint` on `.main-panel` +and on `div.sidebar`. + +So the rule is: **a floating surface opened from inside the main panel +must be a `wa-dialog`, not a `wa-popup`,** because `` / +`showModal()` is Chrome 37 and uses the real top layer. #57's search +modal is one on that ground alone; #60 is the same finding applied to +the six context menus. + +The half that costs time is the second one. **No tier here can +reproduce the clip.** CI's Chromium and WebKit both have the Popover +API, so a popup is top-layered and correct, and a spec asserting "the +surface is not clipped" is green on the broken build. Assert the +*mechanism* — that there is a native `` in the tree at phone +width — which is the one form of the question a browser here answers +honestly. + +## Removing the phone's top bar cost the page header its count (measured 2026-08-21) + +#57 deletes the `top-bar` grid row below 600px and puts a 40px search +button in `page-header` instead. That button is 43px more than the row +has at 320px, which is a width the app promises (WCAG 1.4.10 reflow, +and `header-action-overflow.spec.ts` asks about it). + +Measured on Playlists at 320px, after the fit pass had already +collapsed all three actions into "More actions" and truncated the title +to nothing: title 0, count 50, sort 143, search 40, More 38, five 12px +gaps, 32px of gutters — **363 in 320**, with the More button ending +27px past the edge. So an *action* was clipped, which is the exact +defect #69 exists to prevent. + +What yields is the **count**, last, after everything else. It is the +only item on that row that is neither an identity (the title, which the +navigation repeats) nor an action (the sort control and the buttons, +each the only place they are said). With it gone the header is 304 in +304 and the title even comes back to 19px. + +Two things worth keeping: + +- **The failure was found by the suite, not by the spec.** `make + ui-test` (964), `tsc` in both packages, `make lint`, `make test` and + the new `phone-search.spec.ts` were all green; what failed was + `header-action-overflow.spec.ts` at 320×600, which has nothing to do + with search. That is #62's lesson holding for a second change in a + row: anything that adds to or reflows the shell has a blast radius + the spec you wrote cannot see. +- **A collapsed thing has to still be in the DOM.** Returning `nothing` + from `renderCount()` would have taken the count away for the rest of + the session the first time a 320px window appeared, because + `measureFit` starts every pass from all-visible and needs a node to + un-hide. Same shape as the action buttons, which is where the pattern + was already written down. + +## The e2e app is long-lived, so a staged job outlives the spec that staged it (measured 2026-08-21) + +`make dev-headless` runs one app across every `make e2e` invocation, and +`/__test/emit` writes to a store that nothing clears. A first draft of +`phone-search.spec.ts` asserted the content starts at y=0 with the top +bar gone; it passed alone and failed in a suite run, because +`top-bar-fit.spec.ts` had staged a long-titled scan and `` is +a real grid row whenever work is in flight. + +The fix is not `beforeEach` cleanup — it is measuring the right thing: +the content starts where the **row above it** ends, which is true with a +job running and without one. An assertion against an absolute +coordinate was quietly also asserting "and no background job exists", +which is not something that spec is about or can arrange. diff --git a/CLAUDE.md b/CLAUDE.md index ed31b8f..4a22267 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1559,8 +1559,8 @@ still permits *programmatic* scrolling, so a probe that sets **Below 600px it reflows instead, and that is the phone.** The sideways scroll above was the concession available while the shell had one layout; plan 016 B2 gives it a second. Under 600px the grid drops its -sidebar column, `` takes over as the primary navigation, -the header's controls shrink or stand down, and the shell measures +sidebar column *and* (since #57) its top-bar row, `` takes +over as the primary navigation, and the shell measures exactly 320px in a 320px viewport — so `layout-overflow.spec.ts` now asserts *nothing needs scrolling to*, which is what WCAG 1.4.10 wanted all along. 600 rather than the sidebar's 900 because 900 is a laptop: @@ -1601,9 +1601,36 @@ three, *no action is ever unreachable at any supported size*. The bands themselves already existed; what was new is that they are a promise and that the queue panel is inside it. -**The top bar decides what it can afford, and what it gives up is never -an action.** Its five children do not fit at the bottom of the Compact -band: the bar was 611px inside a 600px viewport idle and **862px while +**And below 600px there is no top bar at all** (#57). The row is gone +from the phone's grid template — not the header hidden, the row deleted +— which is 3.25em of a 439 CSS px viewport, the single biggest vertical +win the reference device has to give. Each of its five children has +somewhere else to be there: `nav-history` is the platform's own back +gesture (already gone from 899 down), the job indicator is `` +(#62, which is why this was blocked on it), the search box is a +`wa-dialog` opened from the view's own header, the library filter is +Settings → Libraries (#148), and the wordmark stays exactly where it is. + +Three things about it are load-bearing. **The header is visually hidden +rather than `display: none`**, because that `h1` is the document's +top-level heading and several pages have no other one — `page-header` +renders no `h1` when `heading` is `''`, and Settings has no +`page-header` at all. Its four *controls* are `display: none` inside it, +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. **The fit pass stands down**, from the +bar's computed `position` rather than from a width — with the bar out of +flow there is no content box to measure children against, and a pass +that ran would collapse the wordmark every time and report success about +a 1px box. And **`top-bar-fit.spec.ts` keeps 390 in its list and asserts +the stronger property there**: "nothing hangs out of the bar" is +trivially true of a bar with no row, and would have passed on a build +that merely broke it, so what that width asks now is that the content +starts where the row above it ends. + +**Above 600px the top bar decides what it can afford, and what it gives +up is never an action.** Its five children do not fit at the bottom of +the Compact band: the bar was 611px inside a 600px viewport idle and **862px while a scan ran**, because `job-indicator` is `hidden` when idle and 235px wide showing a real library's scan title (#143). So `services/ top-bar-fit.ts` is `page-header`'s treatment one bar up — a @@ -1624,12 +1651,17 @@ fixed whichever case happened to be idle when it was measured. **What yields is decided by the promise above, which rules out the two cheapest answers.** Hiding the library filter takes away an action — -`library-filter` is the only control in the app that calls -`setSelectedLibrary` — so it trades this promise for the same promise -(#148 is the phone already doing that). Collapsing the search box to an -icon is what #57 wants and #57 is blocked behind #62, so building it -here is building it without the thing that blocks it. The two that -yield are the two that are **not** actions: the wordmark, which the +`library-filter` was the only control in the app that called +`setSelectedLibrary` — so it trades this promise for the same promise. +That is #148, and #57 fixed it by giving the selection a *second +placement* rather than a second definition: the same component, in +Settings → Libraries under a "Showing" label, at every width. A +phone-only copy was the obvious cheaper answer and is the fault, not the +fix — "where do I change which library I am browsing" having two answers +by viewport is exactly what one control in two places avoids. +Collapsing the search box to an icon is what #57 wanted and #57 was +blocked behind #62, so building it here would have been building it +without the thing that blocked it. The two that yield are the two that are **not** actions: the wordmark, which the window's own title bar repeats and which #48 wants down to "YJ" at every width anyway, and then the job indicator's *label*, leaving the ring — which is not a new judgement, since the component already drops @@ -2403,6 +2435,23 @@ Six things about it are load-bearing: without that half it would pass vacuously on a build that renders no actions at all. +**The count is the last thing to yield, and only at 320px.** Four +things compete for that row and three of them cannot go: the title +yields first and is allowed to ellipsis away entirely, because the +navigation also says which page you are on; the sort control and the +actions are each the only place they are said, which is what the +overflow menu exists for. That leaves the count, which is the one +purely informational item there — an empty page says so in its empty +state and a full one is being looked at. It became reachable rather +than theoretical with #57, since below 600px this header also carries +the phone's search button: measured on Playlists at 320px, title 0, +count 50, sort 143, search 40, "More actions" 38, five 12px gaps and +32px of gutters — 363 in 320, with the More button ending 27px past +the edge. It is rendered and hidden with an attribute rather than +returned as `nothing`, for the reason the action buttons are: every +pass starts from all-visible and needs a node to un-hide, or the first +320px window costs the count for the rest of the session. + One thing it deliberately does **not** grow is a phone mode for the actions. `PHONE_COLUMN_IDS` is the precedent for "what is drawn and what can be sorted are different questions", but it exists because the @@ -2430,6 +2479,54 @@ term belongs in that map**, detail views included — placeholder saying there was nothing to search here, because its sibling was in the map and it was not. +**On a phone the box is a modal, and the map is what decides who gets +one** (#57). There is no header to hold it below 600px, so +`` is a button in the row that already says which page +you are on and `` is where the box goes — and both ask +`searchStore.isSearchableView()` rather than being told, which is the +whole reason the trigger is an element and not a `PageAction`. Seven +hosts each declaring a search action would be a second list of +searchable views, and putting the decision inside `page-header` would +be the phone mode for actions that component documents its refusal to +grow. + +Four things about it are load-bearing. + +**It is a `wa-dialog`, and that is a mechanism rather than a taste.** +#60 read out of the Web Awesome source that `wa-popup` renders +`
` and feature-detects the Popover API, falling +back to `strategy: "fixed"` where there is none — which is Chrome 113, +the reference device, since `popover` is Chrome 114. `position: fixed` +escapes ancestor overflow but **not** `contain: paint`, which +`.main-panel` carries, so a popup-shaped search panel opened from a +view's header is structurally clipped on that device. `` / +`showModal()` is Chrome 37 and uses the real top layer. **No tier here +can see the difference** — CI's Chromium and WebKit both have the +Popover API, so the popup would be top-layered and correct and a spec +asserting "not clipped" would pass on the broken build. The component +tier asserts the *mechanism* instead: that there is a native `` +in the tree. + +**It carries the real ``**, not a second input, which is +what keeps one debounce, one clear button and one view-scoped +placeholder. `--yj-search-max-width` is the one thing the modal changes +about it: 360px is a cap for a header, not for a control that has the +whole of a 424px screen. + +**The results are the page, not a list in the modal.** The term is +view-scoped and the view behind already filters on it and says +"Showing tracks matching …", so Enter closes and hands the screen back. +Rendering results in the dialog would be a second implementation of +every view's filtering, and one that could not offer the row actions +the view does. + +**Escape closes and keeps the term.** `search-bar`'s own input treats +Escape as *clear the search*, which is right in a header where the box +is on screen either way; in a modal it would mean dismissing the search +surface silently discarded the search. The dialog takes the key in the +capture phase on its own host, which is the only listener that runs +before the input inside `search-bar`'s shadow root. + **The window's minimum is measured, not aspirational.** `MinWidth`/ `MinHeight` are 800×600 because that is where the shell was checked to still work: below ~780 the header subtitle wraps and pushes the title 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/index.html b/frontend/index.html index 61688df..991def6 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -14,6 +14,13 @@ user is not walked through the header, the library filter, the search box and eleven nav items on every navigation. --> +

YellowJacket

@@ -84,6 +91,12 @@ + + diff --git a/frontend/index.ts b/frontend/index.ts index a31f1c5..cdaf1c9 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -28,6 +28,11 @@ import '@components/bottom-nav/bottom-nav.ts'; import '@components/queue-panel/queue-panel.ts'; import '@components/nav-history/nav-history.ts'; import '@components/search-bar/search-bar.ts'; +// The phone's search surface (#57). Eager, because below 600px it is +// the *only* way to search and a modal that has to fetch a chunk before +// it can take a keystroke is late by exactly the interval it exists to +// remove. It renders nothing until asked. +import '@components/search-dialog/search-dialog.ts'; import '@components/library-filter/library-filter.ts'; import '@components/first-run-wizard/first-run-wizard.ts'; import '@components/notifications/notification-host.ts'; diff --git a/frontend/src/components/config-page/config-page.ts b/frontend/src/components/config-page/config-page.ts index 37caa5a..6604419 100644 --- a/frontend/src/components/config-page/config-page.ts +++ b/frontend/src/components/config-page/config-page.ts @@ -56,6 +56,10 @@ import { import './config-field'; import './config-section'; +// The view filter's home (#148). The same component the top bar +// carries, placed a second time rather than reimplemented -- two +// definitions of "which library am I browsing" is what this is for. +import '@components/library-filter/library-filter'; import './download-clients'; import './shortcut-capture'; import { confirmAction } from '../confirm-dialog/confirm-dialog'; @@ -231,6 +235,42 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { flex-wrap: wrap; } + /* #148, and the second half of #57. + + library-filter is the only control in the app that calls + setSelectedLibrary, and it lived in the top bar -- which + #57 takes out of the layout on a phone, and which #143 + already refused to hide as a fit step precisely because + hiding it takes away an action. So the selection gets a home + that does not depend on that bar existing. + + At every width, not below 600px: a phone-only copy would be + a second place the control lives, and "where do I change + which library I am browsing" having two answers by size is + the fault, not the fix. */ + .library-scope { + display: flex; + align-items: center; + justify-content: space-between; + gap: 1em; + flex-wrap: wrap; + margin-bottom: 1em; + } + + .library-scope .scope-label { + font-weight: 600; + font-size: 0.85em; + color: var(--yj-text-primary, #fff); + display: block; + } + + .library-scope .scope-description { + font-size: 0.75em; + color: var(--yj-text-tertiary, #888); + margin: 0.35em 0 0; + max-width: 40em; + } + .save-row { display: flex; gap: 0.5em; @@ -2370,6 +2410,21 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { for new and changed files." .open=${true} > +
+
+ Showing +

+ Which library the Albums, Artists and Genres + views show. This is a view filter, not a + setting about the libraries themselves — the + list below is where they are added, renamed + and scanned. +

+
+ + +
+
@@ -516,12 +540,6 @@ export class PageHeader extends LitElement { if (!header) return; - if (this.actions.length === 0) { - this.commitCollapsed(new Set()); - - return; - } - const buttons = new Map(); for (const el of this.renderRoot.querySelectorAll( @@ -534,6 +552,7 @@ export class PageHeader extends LitElement { const more = this.moreButton; const title = this.renderRoot.querySelector('h1'); + const count = this.renderRoot.querySelector('.count'); /** * Nothing is clipped — which is not the same as the header not @@ -555,6 +574,8 @@ export class PageHeader extends LitElement { if (more) more.hidden = true; + if (count) count.hidden = false; + const collapsed = new Set(); if (!fits()) { @@ -571,7 +592,42 @@ export class PageHeader extends LitElement { } } - this.commitCollapsed(collapsed); + this.commitCollapsed(collapsed, this.collapseCount(count, fits)); + } + + /** + * The last thing to give way, after every action is in the menu and + * the title has already run out. + * + * There are four things competing for this row and three of them + * cannot go. The **title** yields first and is allowed to ellipsis + * away entirely at 320px, because the navigation also says which + * page you are on. The **sort** control and the **actions** are + * each the only place they are said, so an action collapses into + * the menu rather than disappearing and the sort control stays. + * That leaves the **count**, which is the one purely informational + * item on the row — an empty page says so in its empty state, and a + * full one is being looked at. + * + * It became reachable rather than theoretical with #57: below 600px + * the header also carries the phone's search button, and on + * Playlists at 320px that is 43px more than the row has. Measured + * there: title 0, count 50, sort 143, search 40, "More actions" 38, + * five 12px gaps and 32px of gutters — 363 in 320, with the More + * button ending 27px past the edge. Something has to go, and this + * is the only candidate that is not an action. + * + * @returns whether the count was given up. + */ + private collapseCount( + count: HTMLElement | null, + fits: () => boolean, + ): boolean { + if (count === null || fits()) return false; + + count.hidden = true; + + return true; } /** Lowest priority first; ties broken from the right. */ @@ -586,7 +642,9 @@ export class PageHeader extends LitElement { .map(({ action }) => action); } - private commitCollapsed(next: Set): void { + private commitCollapsed(next: Set, countHidden: boolean): void { + this.countCollapsed = countHidden; + const same = next.size === this.collapsed.size && [...next].every((id) => this.collapsed.has(id)); @@ -754,7 +812,16 @@ export class PageHeader extends LitElement { const noun = this.count === 1 ? this.countNoun : plural; - return html``; } diff --git a/frontend/src/components/playlist-details/playlist-details.ts b/frontend/src/components/playlist-details/playlist-details.ts index c2e5d75..c90e7ed 100644 --- a/frontend/src/components/playlist-details/playlist-details.ts +++ b/frontend/src/components/playlist-details/playlist-details.ts @@ -27,6 +27,7 @@ import { queueStore } from '@store/queue-store'; import { creditStore } from '@store/credit-store'; import { PlayerController } from '@store/controllers/player-controller'; import { SearchController } from '@store/controllers/search-controller'; +import '../search-dialog/search-trigger'; import { SelectionController } from '@utils/selection-controller'; import type { SelectionHost } from '@utils/selection-controller'; import { @@ -1039,6 +1040,16 @@ export class PlaylistDetails min-width: 0; } + /* #57. This view is in search-store's map and filters on the + term, but it is a detail view and so has no page-header to + carry the phone's search button. Pushed to the end of the + header row, which is where page-header puts it too. */ + .header-end { + margin-left: auto; + display: flex; + align-items: center; + } + .playlist-title { font-size: 24px; font-weight: 700; @@ -1383,6 +1394,9 @@ export class PlaylistDetails ` : ''}
+
+ +
${searchBar}
` and feature-detects the Popover API, falling + * back to `strategy: "fixed"` where there is none — which is the + * reference device, Chrome 113, since `popover` is Chrome 114. And + * `position: fixed` escapes ancestor *overflow* but not `contain: + * paint`, which makes an element a containing block for fixed + * descendants **and clips them**; `index.css` puts `contain: layout + * style paint` on `.main-panel`, which is the ancestor of every view. + * A popup-shaped search panel opened from a view's header would + * therefore be structurally clipped on the one device this issue is + * about, and **no tier here could see it** — CI's Chromium and WebKit + * both have the Popover API, so the popup is top-layered and correct. + * ``/`showModal()` is Chrome 37 and uses the real top layer, so + * this is immune by construction. + * + * **It carries the real ``**, not a second input. That is + * what keeps one debounce, one clear button, one accessible name and + * one view-scoped placeholder — and it is why `store/search-store.ts` + * is still the only statement of which views can search and what they + * search. The modal is a presentation of the control, not a copy of it. + * + * **The results are the view, not a list in here.** The Direction says + * "the box and live results"; the live results already exist, because + * the term is view-scoped and the page behind this dialog filters on it + * and says so in `page-header`'s "Showing albums matching …" line. + * Rendering results in the dialog would be a second implementation of + * every view's own filtering, and a worse one — it could not offer the + * row actions the view does. So Enter closes and hands the screen back. + * + * A singleton in `index.html` for the reason `shortcuts-overlay` is: + * one instance, one `data-testid`, one document listener, and no + * `data-testid="search-input"` resolving to two elements while it is + * shut. + */ +import { LitElement, css, html, nothing } from 'lit'; +import { customElement, query, state } from 'lit/decorators.js'; +import '@awesome.me/webawesome/dist/components/dialog/dialog.js'; + +import { designTokens } from '../../styles/tokens.css'; +import { nameDialogsIn } from '@utils/name-dialog'; +import { SearchController } from '@store/controllers/search-controller'; +import type { SearchBar } from '../search-bar/search-bar'; +import '../search-bar/search-bar'; + +/** The event any trigger dispatches to open this. */ +export const OPEN_SEARCH_EVENT = 'open-search'; + +@customElement('search-dialog') +export class SearchDialog extends LitElement { + private searchCtrl = new SearchController(this); + + @query('wa-dialog') private dialog?: HTMLElement & { open: boolean }; + + @query('search-bar') private bar?: SearchBar; + + @state() private isOpen = false; + + static override styles = [ + designTokens, + css` + :host { + display: contents; + } + + wa-dialog::part(dialog) { + background: var(--yj-bg-surface, #212529); + color: var(--yj-text-primary, #fff); + } + + /* The box is the whole content, so it gets the whole width + rather than the 360px cap it wears in a header. */ + search-bar { + display: block; + width: 100%; + --yj-search-max-width: none; + } + + .hint { + margin: 0.75em 0 0; + font-size: var(--yj-text-sm, 0.8125rem); + color: var(--yj-text-secondary, #b3b3b3); + } + `, + ]; + + override connectedCallback(): void { + super.connectedCallback(); + document.addEventListener(OPEN_SEARCH_EVENT, this.open); + // Capture, on the host: the path runs document -> host -> + // shadow root -> the input inside `search-bar`, so a capture + // listener here is the only one that gets the key *before* the + // input's own handler. A `@keydown` in the template is a + // bubbling listener and would run after the term was cleared, + // and there is nowhere to put a `firstUpdated` hook -- the + // first render of this element produces no content at all. + this.addEventListener('keydown', this.onKeydown, true); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + document.removeEventListener(OPEN_SEARCH_EVENT, this.open); + this.removeEventListener('keydown', this.onKeydown, true); + } + + /** + * Not a toggle, for `shortcuts-overlay`'s reason: a dialog owns + * every unmodified key while it is up, so a second press of the + * shortcut that opened it never reaches the shortcut service. + */ + private open = (): void => { + if (this.isOpen) return; + + // Nothing to search here is not an error; it is the state the + // trigger already declines to render in. Guarding here too is + // what makes the keyboard route (Ctrl+F on a phone) agree with + // the button. + if (!this.searchCtrl.isSearchableView) return; + + this.isOpen = true; + + void this.updateComplete.then(() => { + if (this.dialog) this.dialog.open = true; + + // `wa-dialog` positions and shows in its own update, and + // `search-bar` populates its own shadow root in one more — + // the same lifecycle trap `name-dialog.ts` documents. One + // more frame, and the box has an input to focus. + requestAnimationFrame(() => this.bar?.focusInput()); + }); + }; + + private close(): void { + if (this.dialog) this.dialog.open = false; + + this.isOpen = false; + } + + /** + * Escape closes and **keeps the term**; Enter closes and shows the + * results. + * + * Escape is the one worth stating. `search-bar`'s input treats it + * as *clear the search*, which is right in a header — the box is on + * screen either way, so clearing is the only thing left for the key + * to mean. Here it would make dismissing the search surface + * silently discard the search, and discarding is what the clear + * button inside it is for. So this runs first and closes; the term + * survives, and the page behind is still filtered by it. + */ + private onKeydown = (e: KeyboardEvent): void => { + if (!this.isOpen) return; + + if (e.key === 'Escape') { + e.stopPropagation(); + this.close(); + + return; + } + + if (e.key === 'Enter') { + e.stopPropagation(); + e.preventDefault(); + this.close(); + } + }; + + /** + * Web Awesome renders `label` into a heading it never points the + * `` at. See `utils/name-dialog.ts`. + */ + override updated(): void { + nameDialogsIn(this.shadowRoot); + } + + override render() { + if (!this.isOpen) return nothing; + + const scope = this.searchCtrl.scopeLabel; + + return html` + this.close()} + > + +

+ Results appear on the page behind this. Press Enter + or close to see them. +

+ + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'search-dialog': SearchDialog; + } +} diff --git a/frontend/src/components/search-dialog/search-trigger.ts b/frontend/src/components/search-dialog/search-trigger.ts new file mode 100644 index 0000000..16d0b71 --- /dev/null +++ b/frontend/src/components/search-dialog/search-trigger.ts @@ -0,0 +1,147 @@ +/** + * The phone's way into search (#57): one button, in the row that + * already says which page you are on. + * + * **Which views show it is not a decision this component makes.** + * `store/search-store.ts` has held the map of what each view searches + * since plan 007, and #57's own Findings say so — "that is exactly the + * condition for showing the button". So this asks `isSearchableView` + * and renders nothing otherwise, and no second list of searchable views + * exists to fall out of step with the first. + * + * **It is an element rather than a `PageAction`**, and that is the + * whole reason it is a component at all. Two of the seven searchable + * views — `playlist-details` and `smart-playlist-details` — have no + * `page-header`; they filter on the term and say so in their own + * headers. Declaring search as an action would mean seven hosts each + * writing it out, which is the second list again, and it would put a + * *phone mode for actions* inside `page-header`, which that component + * documents its refusal to grow. An element three headers place is one + * statement of the rule, placed three times. + * + * It does not participate in `page-header`'s overflow measurement, for + * the reason the count and the sort control do not: it is 32px, it is + * `flex-shrink: 0`, and the header's `fits()` sees its width like any + * other child. What it must never do is collapse into the overflow + * menu — on a phone that menu is the only home for the page's actions + * already, and search would be two taps behind an ellipsis. + */ +import { LitElement, css, html, nothing } from 'lit'; +import { customElement, state } from 'lit/decorators.js'; +import '@awesome.me/webawesome/dist/components/icon/icon.js'; + +import { designTokens } from '../../styles/tokens.css'; +import { PHONE_QUERY } from '@utils/breakpoints'; +import { SearchController } from '@store/controllers/search-controller'; +import { ICON_SEARCH } from '@utils/icon-language'; +import { OPEN_SEARCH_EVENT } from './search-dialog'; + +@customElement('search-trigger') +export class SearchTrigger extends LitElement { + private searchCtrl = new SearchController(this); + + /** + * From `matchMedia` rather than a media query, because this decides + * whether the button *exists* — `job-band`'s rule, and for the same + * consequence: a header that renders it at every width puts a + * second search affordance beside the desktop's own box. + */ + @state() private phone = false; + + private media?: MediaQueryList; + + static override styles = [ + designTokens, + css` + :host { + display: contents; + } + + button { + display: inline-flex; + align-items: center; + justify-content: center; + /* The smallest a touch target should be. The header's + own action buttons are smaller because they carry a + label; this one is a glyph. */ + min-width: 40px; + min-height: 40px; + padding: 0; + background: none; + border: 1px solid var(--yj-border-subtle, #555); + border-radius: 4px; + color: var(--yj-text-primary, #fff); + cursor: pointer; + flex-shrink: 0; + } + + button:focus-visible { + outline: 2px solid var(--yj-accent, #ffd43b); + outline-offset: -1px; + } + + /* A search that is *on* says so without a second control: + the page already carries "Showing albums matching ...", + and this is the button that reopens the box to change or + clear it. */ + button.filtering { + border-color: var(--yj-accent, #ffd43b); + color: var(--yj-accent-text, #ffd43b); + } + `, + ]; + + override connectedCallback(): void { + super.connectedCallback(); + + this.media = window.matchMedia(PHONE_QUERY); + this.phone = this.media.matches; + this.media.addEventListener('change', this.onMedia); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + this.media?.removeEventListener('change', this.onMedia); + } + + private onMedia = (e: MediaQueryListEvent): void => { + this.phone = e.matches; + }; + + private onClick = (): void => { + document.dispatchEvent(new CustomEvent(OPEN_SEARCH_EVENT)); + }; + + override render() { + if (!this.phone || !this.searchCtrl.isSearchableView) return nothing; + + const scope = this.searchCtrl.scopeLabel; + const term = this.searchCtrl.term; + + // The name carries the state, because the colour cannot: a + // control that is a different colour and the same word is a + // control that says nothing to anyone not seeing it. Same rule + // `library-status.ts` states for a partial badge. + const label = term + ? `Search ${scope}, showing matches for ${term}` + : `Search ${scope}`; + + return html` + + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'search-trigger': SearchTrigger; + } +} diff --git a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts index d86101b..83adde0 100644 --- a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts +++ b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts @@ -18,6 +18,7 @@ import { queueStore } from '@store/queue-store'; import { creditStore } from '@store/credit-store'; import { PlayerController } from '@store/controllers/player-controller'; import { SearchController } from '@store/controllers/search-controller'; +import '../search-dialog/search-trigger'; import { SelectionController } from '@utils/selection-controller'; import type { SelectionHost } from '@utils/selection-controller'; import { @@ -359,6 +360,15 @@ export class SmartPlaylistDetails flex-shrink: 0; } + /* #57. Like playlist-details, this view filters on the search + term and has no page-header to carry the phone's search + button, so the action row does. */ + .actions-end { + margin-left: auto; + display: flex; + align-items: center; + } + .action-button { background: none; border: 1px solid var(--yj-border-subtle, #555); @@ -1300,6 +1310,9 @@ export class SmartPlaylistDetails Edit Rules `} +
+ +
${this.editing ? html` diff --git a/frontend/src/services/keyboard-shortcut-service.ts b/frontend/src/services/keyboard-shortcut-service.ts index 12689cf..43ac955 100644 --- a/frontend/src/services/keyboard-shortcut-service.ts +++ b/frontend/src/services/keyboard-shortcut-service.ts @@ -16,6 +16,7 @@ import { playerStore } from '@store/player-store'; import { queueStore } from '@store/queue-store'; import * as Player from '@go/player/player.js'; import type { SearchBar } from '@components/search-bar/search-bar'; +import { OPEN_SEARCH_EVENT } from '@components/search-dialog/search-dialog'; // =================================================================== // KEY STRING UTILITIES @@ -387,14 +388,24 @@ async function dispatch(action: string): Promise { break; // Navigation + // The key has one meaning -- *let me search this page* -- and + // two surfaces since #57. The header box is gone below 600px, + // so scoping the query to the bar is not tidiness: an unscoped + // `search-bar` also matches the one inside `search-dialog` + // while that is open, and would focus a box the user is + // already typing in while leaving the phone with nothing at + // all. The dialog declines to open on a view with nothing to + // search, which is the same condition the trigger renders on. case 'nav.search': case 'nav.searchAlt': { const bar = document.querySelector( - 'search-bar', + 'header.top-bar search-bar', ) as SearchBar | null; - if (bar && !bar.hasAttribute('hidden')) { + if (bar && bar.checkVisibility()) { bar.focusInput(); + } else { + document.dispatchEvent(new CustomEvent(OPEN_SEARCH_EVENT)); } break; 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(); diff --git a/frontend/src/utils/icon-language.ts b/frontend/src/utils/icon-language.ts index 6efe84d..9315b1f 100644 --- a/frontend/src/utils/icon-language.ts +++ b/frontend/src/utils/icon-language.ts @@ -124,6 +124,19 @@ export const ICON_DOWNLOADING = 'download'; */ export const ICON_MORE_ACTIONS = 'ellipsis'; +/** + * Look for something. + * + * Deliberately **not** governed by the sweep in + * `icon-language.test.ts`: `magnifying-glass` has only ever meant this, + * in the header box and in Explore's own catalog search alike, so + * governing it would force a rename on two call sites that are already + * right. It is written down because #57 gave the meaning a *button* as + * well as a box, and a second surface for the same verb is exactly the + * point at which two spellings start. + */ +export const ICON_SEARCH = 'magnifying-glass'; + /** * Take this away. * diff --git a/frontend/test/components/search-dialog.test.ts b/frontend/test/components/search-dialog.test.ts new file mode 100644 index 0000000..03f83de --- /dev/null +++ b/frontend/test/components/search-dialog.test.ts @@ -0,0 +1,248 @@ +/** + * The phone's search surface (#57). + * + * Two things are asserted here that the e2e tier cannot reach, and one + * that it deliberately must not be trusted with. + * + * **Which views show the trigger is `search-store`'s answer**, so this + * walks the map rather than sampling a view: the fault the issue guards + * against is a second list of searchable views, and a spec that checks + * Albums checks nothing about Playlists. + * + * **The dialog is a ``, not a popup.** #60 established from the + * Web Awesome source that `wa-popup` falls back to `position: fixed` + * without the Popover API — Chrome 113, the reference device — and that + * `.main-panel`'s `contain: paint` clips a fixed descendant. Every tier + * available here has the Popover API, so a popup renders perfectly in + * CI and is clipped on the device: **an assertion that the surface is + * not clipped passes on the broken build.** So the assertion is the + * *mechanism* — a real `` in the tree — which is the one form + * of this that a browser here can answer honestly. + * + * The breakpoint is stubbed rather than emulated, for the reason + * `now-playing-phone.test.ts` gives: the runner's viewport is fixed at + * 1280x800, and the component reads `matchMedia` in `connectedCallback` + * precisely so a test can answer it first. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; + +import '@components/search-dialog/search-dialog'; +import '@components/search-dialog/search-trigger'; +import { searchStore } from '@store/search-store'; +import { fixture, shadow, deepShadow } from '@test/support/render'; +import { flush } from '@test/support/harness'; + +/** Views the store says can be searched, and what they search. */ +const SEARCHABLE: [string, string][] = [ + ['tracks', 'tracks'], + ['albums', 'albums'], + ['artists', 'artists'], + ['genres', 'genres'], + ['playlists', 'playlists'], + ['playlist-details', 'tracks in this playlist'], + ['smart-playlist-details', 'tracks in this smart playlist'], +]; + +/** Views with nothing of their own to search, or a search of their own. */ +const UNSEARCHABLE = ['home', 'explore', 'settings', 'downloads', 'autotag']; + +let restoreMedia: (() => void) | null = null; + +/** Answer the shell's phone query with `phone` until restored. */ +function stubPhone(phone: boolean): void { + const real = window.matchMedia.bind(window); + + window.matchMedia = ((q: string) => + q.includes('max-width: 599px') + ? { + matches: phone, + media: q, + addEventListener() {}, + removeEventListener() {}, + } + : real(q)) as typeof window.matchMedia; + + restoreMedia = () => { + window.matchMedia = real; + }; +} + +beforeEach(() => { + searchStore.setTerm(''); + searchStore.setCurrentView('tracks'); +}); + +afterEach(() => { + restoreMedia?.(); + restoreMedia = null; + searchStore.setTerm(''); + searchStore.setCurrentView('tracks'); +}); + +describe('', () => { + it('is offered on every view the store says can be searched', async () => { + stubPhone(true); + + // One element, walked across the views: the trigger reads the store + // on every render, so remounting per view would test mounting + // rather than the condition. + const el = await fixture('search-trigger'); + + for (const [view] of SEARCHABLE) { + searchStore.setCurrentView(view); + await el.updateComplete; + + expect( + shadow(el, '[data-testid="search-trigger"]'), + `no trigger on ${view}`, + ).not.toBeNull(); + } + }); + + it('names what the button will search', async () => { + stubPhone(true); + + const el = await fixture('search-trigger'); + + for (const [view, scope] of SEARCHABLE) { + searchStore.setCurrentView(view); + await el.updateComplete; + + expect( + shadow(el, '[data-testid="search-trigger"]')?.getAttribute( + 'aria-label', + ), + ).toBe(`Search ${scope}`); + } + }); + + it('is absent where there is nothing to search', async () => { + stubPhone(true); + + const el = await fixture('search-trigger'); + + for (const view of UNSEARCHABLE) { + searchStore.setCurrentView(view); + await el.updateComplete; + + expect( + shadow(el, '[data-testid="search-trigger"]'), + `a trigger appeared on ${view}`, + ).toBeNull(); + } + }); + + it('is absent above the phone breakpoint, where the header has a box', async () => { + stubPhone(false); + + const el = await fixture('search-trigger'); + + expect(shadow(el, '[data-testid="search-trigger"]')).toBeNull(); + }); + + /** + * A colour is not a signal on its own. The button is the only thing + * on screen that reopens a filtered search, so the state it is in has + * to reach someone who cannot see the accent border. + */ + it('says in its name that a search is applied', async () => { + stubPhone(true); + + const el = await fixture('search-trigger'); + + searchStore.setTerm('aurora'); + await el.updateComplete; + + const button = shadow(el, '[data-testid="search-trigger"]'); + + expect(button?.getAttribute('aria-label')).toContain('aurora'); + expect(button?.className).toContain('filtering'); + }); +}); + +describe('', () => { + it('opens on the event the trigger dispatches, as a real dialog', async () => { + stubPhone(true); + + const el = await fixture('search-dialog'); + const trigger = await fixture('search-trigger'); + + shadow(trigger, '[data-testid="search-trigger"]')?.click(); + await flush(); + await el.updateComplete; + + expect(shadow(el, '[data-testid="search-dialog"]')).not.toBeNull(); + + // The mechanism, not the appearance: a native is what + // reaches the top layer on Chrome 113, and a wa-popup would look + // identical in this browser while being clipped on the device. + expect(deepShadow(el, 'dialog')).not.toBeNull(); + }); + + /** + * It carries the real box rather than a second input, which is what + * keeps one debounce, one clear button and one view-scoped + * placeholder — and what keeps `search-store` the only statement of + * what a view searches. + */ + it('carries the header search box itself', async () => { + const el = await fixture('search-dialog'); + + document.dispatchEvent(new CustomEvent('open-search')); + await flush(); + await el.updateComplete; + + expect(shadow(el, 'search-bar')).not.toBeNull(); + }); + + /** + * The one place the shortcut route and the button could disagree. + * Ctrl+F on a view with nothing to search dispatches the same event + * the button would, and the button is not there to be pressed. + */ + it('declines to open where there is nothing to search', async () => { + const el = await fixture('search-dialog'); + + searchStore.setCurrentView('home'); + document.dispatchEvent(new CustomEvent('open-search')); + await flush(); + await el.updateComplete; + + expect(shadow(el, '[data-testid="search-dialog"]')).toBeNull(); + }); + + /** + * Escape closes and **keeps the term**. + * + * `search-bar`'s own input treats Escape as "clear the search", which + * is right in a header where the box stays on screen either way. Here + * it would mean dismissing the surface silently discarded the search, + * and the page behind would refill without being asked to. + */ + it('keeps the search when it is dismissed', async () => { + const el = await fixture('search-dialog'); + + document.dispatchEvent(new CustomEvent('open-search')); + await flush(); + await el.updateComplete; + + searchStore.setTerm('aurora'); + + const input = deepShadow(el, 'input'); + + expect(input).not.toBeNull(); + + input!.dispatchEvent( + new KeyboardEvent('keydown', { + key: 'Escape', + bubbles: true, + composed: true, + }), + ); + await flush(); + await el.updateComplete; + + expect(searchStore.getTerm()).toBe('aurora'); + expect(shadow(el, '[data-testid="search-dialog"]')).toBeNull(); + }); +});