diff --git a/.planning/NOTES.md b/.planning/NOTES.md index caab6bf..13c0469 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3961,3 +3961,71 @@ The general rule for this repo's fixture library: it is deliberately full of edge cases (untagged, unicode, duplicates, extremes), so a spec that wants an *ordinary* track has to **say so** — filter on the property it depends on rather than slicing. +## A nested rule starting with an element name is dropped on the phone (2026-08-20) + +`CLAUDE.md` records that the device renders in **Chrome 113**, which +does not have relaxed CSS nesting (Chrome 120). The consequence is +sharper than "some syntax is unavailable": a nested rule whose selector +begins with a bare identifier is not a parse error you would notice, it +is **silently dropped**. + +Three such rules were live in `frontend/index.css`, all inside +`.bottom-bar`, and all therefore dead on the phone and only on the +phone: + +```css +.bottom-bar { + #track-info { p { … } } /* the metadata's ellipsis */ + now-playing { overflow: hidden; } + audio-player { margin: 0.5em 1em; } +} +``` + +The first is the interesting one: it is the *ellipsis* on the bottom +bar's track title and artist, so on the device that text has never +truncated — the same class of fault as `now-playing`'s marquee, whose +`text-overflow` sat on the wrong box and had never produced an ellipsis +in any mode. Both are invisible to every assertion and visible in a +screenshot. + +`& p`, `& now-playing`, `& audio-player` are valid in both, so the fix +is one character per rule. What is worth keeping is the rule of thumb: +**inside a nested block, always write `&`** — and note that a rule +inside `@media` is *not* nested, so `@media … { bottom-nav { … } }` +elsewhere in that file is fine and needs nothing. + +`make css-check` does not catch this (it looks for backticks that end a +tagged template early). Filed as an issue: the check is the natural +place for it, being the same shape of trap — a silent, phone-only, +screenshot-only failure. + +## Centring a bar costs the control in the middle of it (measured 2026-08-20) + +#23 asks for the transport centred in the bottom bar. The obvious +implementation — make the outer two grid tracks the same width, so the +middle is centred by construction — is right, and the first cut of it +was a regression, because "the same width" was taken to mean *the +metadata's* width on both sides. + +Measured at 800px, with the seek bar's own track: + +| layout | seek track | transport column | +|---|---|---| +| `320px 1fr auto` (before) | 257 | 407 | +| both sides `--now-playing-width` | **61** | 179 | +| both sides `min(--now-playing-width, 25%)` | 246 | 364 | + +At 200% text the middle row is worse still: 130 before, **0** with the +uncapped sides. Centring is free at 1440 and expensive at 800, so a +change checked only at a comfortable width looks perfect. + +The general form: **a symmetric layout reserves space on the side that +does not need it.** The right-hand group here is ~141px (volume plus +the queue button) and was being given 320 to keep the arithmetic +symmetric. Cap the side tracks against the *bar*, not against their +content, and the middle gets the difference. + +The spec that pins this is two assertions, not one, and that split is +deliberate: an uncapped build is *perfectly centred* and fails only the +seek-bar width, so a spec asserting centring alone would have passed +the regression. diff --git a/CLAUDE.md b/CLAUDE.md index 860b7cf..a63e2c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1572,6 +1572,50 @@ And **the bar does not resize when a job starts**, which is the case the whole thing is for — a ResizeObserver on the header alone never fires, so every element child is observed too. +**The bottom bar is three columns whose outer two are the same width, +and that is what "centred" means.** It was `320px 1fr auto`, so the +transport sat in the middle of the space the metadata and the queue +button did not use — its centre was ~140px right of the window's at +every size (#23). The outer tracks are now the same expression, so the +middle one is centred by construction rather than by arithmetic that +has to be redone whenever a control joins the bar. + +Four things about it are load-bearing. + +**The side width is the metadata's, capped at a quarter of the bar**, +and the cap is not tidiness — it was measured as a regression first. +Reserving the full `--now-playing-width` on *both* sides costs the +transport twice: at 800px the outer pair wanted 640 of 800 and the seek +bar's track fell from **257px to 61px**, and to 0 at 200% text. The +control you drag was being squeezed to centre the buttons above it. +With the cap it is 246px at 800, which is parity with the uncentred +layout. + +**The cap is a `min()` rather than a breakpoint** because +`--now-playing-width` is *user state* — the metadata panel has a drag +handle — and the same reasoning the queue panel's overlay mode uses +applies: a rule that assumed the default 320 would be wrong by whatever +the user dragged. Tying both sides to that variable is also what keeps +the handle meaningful; a plain `1fr … 1fr` would centre the transport +just as well and silently make dragging a no-op. + +**The volume moved out of `audio-player` and into the bar** (#42), +because the transport column has to hold the transport and nothing +else or "centred" means centred with a slider bolted to one side. It +lives in `.bar-end` with the queue button — one cell, not two columns, +since the centring compares *columns* and a separate volume track would +make the outer pair unequal by whatever the slider measures. + +And **the slider is inline by default, with the popup as a setting** +whose stored flag names the *popup*: `backend/config`'s polarity rule, +where the zero value has to be the intended answer, so an existing +`config.toml` with no key gets the new default without a migration. +Inline, the icon becomes the mute toggle and is named after that action +rather than after the state, because with the slider beside it there is +nothing left to disclose. It stands down below 600px whatever the +setting says — that is about the platform rather than preference, and +is why `mediacontrols`' Android handler implements no volume callback. + **900 is the worst desktop width, not the 800×600 minimum.** The sidebar collapses to icons *below* 900, so the main panel is 843px at 899 and 700px at 900 — the narrowest content area any desktop width diff --git a/backend/config/config.go b/backend/config/config.go index 3e5b7b0..e116553 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -666,6 +666,49 @@ func (c *Config) SetAllowMeteredCatalogDownload(allow bool) error { return nil } +// GetPopupVolume reports whether the bottom bar's volume control is a +// click-to-open popup rather than an inline slider (#42). +func (c *Config) GetPopupVolume() bool { + if c.General == nil { + return false + } + + return c.General.PopupVolume +} + +// SetPopupVolume saves the volume control's presentation. +// +// Nothing to validate: both values are legal at every width, and the +// frontend additionally stands the inline slider down below the phone +// breakpoint whatever this says, because that is about room rather than +// about preference. +func (c *Config) SetPopupVolume(popup bool) error { + if c.General == nil { + c.General = &GeneralConfig{} + c.General.ApplyDefaults() + } + + c.General.PopupVolume = popup + + if err := c.Save(); err != nil { + return fmt.Errorf( + "could not save config: %w", err, + ) + } + + events.Emit( + c.ctx, + events.GeneralConfigChanged, + map[string]any{ + "PopupVolume": popup, + }, + ) + + c.logger.Info("volume control presentation updated", "popup", popup) + + return nil +} + // GetViewVisibility reports which primary views the sidebar should // show, answered for every known view rather than only the ones the // config mentions -- so the frontend filters on a value and never has diff --git a/backend/config/emit_test.go b/backend/config/emit_test.go index 8aeb5e0..b554ff5 100644 --- a/backend/config/emit_test.go +++ b/backend/config/emit_test.go @@ -188,3 +188,43 @@ func TestEmit_FavoritesChangeCarriesFullConfig(t *testing.T) { } } } + +// TestEmit_PopupVolumeRoundTripsAndDefaultsToInline pins both halves of +// #42's storage decision. +// +// The **default** is the load-bearing one: inline is what a fresh +// install and an existing `config.toml` with no such key must both +// produce, which is why the field names the popup rather than the +// inline slider. A flag spelled the other way round would default to +// false, hand every existing install the popup this issue exists to +// stop being the only option, and need a migration to say otherwise. +func TestEmit_PopupVolumeRoundTripsAndDefaultsToInline(t *testing.T) { + t.Parallel() + + conf, rec := setupRecordedConfig(t) + + if conf.GetPopupVolume() { + t.Error("a config with no PopupVolume key wants the popup, want inline") + } + + if err := conf.SetPopupVolume(true); err != nil { + t.Fatalf("SetPopupVolume: %v", err) + } + + if !conf.GetPopupVolume() { + t.Error("GetPopupVolume = false after setting it true") + } + + data := payloadMap(t, rec, events.GeneralConfigChanged) + if data["PopupVolume"] != true { + t.Errorf("PopupVolume = %v, want true", data["PopupVolume"]) + } + + if err := conf.SetPopupVolume(false); err != nil { + t.Fatalf("SetPopupVolume(false): %v", err) + } + + if conf.GetPopupVolume() { + t.Error("GetPopupVolume = true after setting it false") + } +} diff --git a/backend/config/general.go b/backend/config/general.go index 5e6609e..70a5c5d 100644 --- a/backend/config/general.go +++ b/backend/config/general.go @@ -54,6 +54,16 @@ type GeneralConfig struct { // so an existing config with no such key refuses by default rather // than needing a migration to become careful. AllowMeteredCatalogDownload bool `toml:"AllowMeteredCatalogDownload"` + // PopupVolume draws the bottom bar's volume as a click-to-open popup + // instead of a slider that is always there (#42). + // + // The polarity is the rule this file already states twice: **the + // zero value is the intended answer**. Inline is the new default, so + // the flag has to name the *other* choice — an `InlineVolume bool` + // would default to false and give every existing install the popup + // this issue exists to stop being the only option, and would need a + // migration to say otherwise. + PopupVolume bool `toml:"PopupVolume"` } // ApplyDefaults fills zero-value fields with sensible defaults. diff --git a/e2e/specs/bottom-bar.spec.ts b/e2e/specs/bottom-bar.spec.ts new file mode 100644 index 0000000..734b4a9 --- /dev/null +++ b/e2e/specs/bottom-bar.spec.ts @@ -0,0 +1,147 @@ +import { test, expect, callBinding, NO_QUEUE_SOURCE } from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * The bottom bar's two promises (#23, #42): the transport is centred in + * the window, and the volume is a slider rather than a popup. + * + * **"Centred" is measured against the window, not against the space + * left over**, which is the whole of #23. The bar was + * `320px 1fr auto`, so the transport sat in the middle of what the + * metadata and the queue button did not use — its centre was ~140px + * right of the window's at every size, which reads as an alignment + * mistake rather than as a layout choice. + * + * The mechanism is that the outer two columns are the same width, so + * this asserts the *outcome* (centre lines up) rather than the CSS. A + * spec that checked `grid-template-columns` would pass on any build + * that kept the declaration and broke the result. + */ + +/** Where the transport sits, against where the window's centre is. */ +const geometry = (app: Page) => + app.evaluate(() => { + const bar = document.querySelector('.bottom-bar')!; + const player = document.querySelector('audio-player')!; + const b = bar.getBoundingClientRect(); + const p = player.getBoundingClientRect(); + + const seek = player.shadowRoot + ?.querySelector('seek-bar') + ?.shadowRoot?.querySelector('wa-slider'); + + return { + offset: Math.round(p.left + p.width / 2 - (b.left + b.width / 2)), + barHeight: Math.round(b.height), + seekWidth: seek ? Math.round(seek.getBoundingClientRect().width) : -1, + }; + }); + +/** Something has to be playing before the transport draws a seek bar. */ +async function play(app: Page): Promise { + const paths = await app.evaluate(async () => { + const tracks = (await window.__yjEvents.call( + 'library.Library.GetTracks', + [0], + 10_000, + )) as { FilePath: string }[]; + + return tracks.slice(0, 3).map((t) => t.FilePath); + }); + + await callBinding(app, 'queue.Queue.SetQueue', [ + paths, + 0, + false, + NO_QUEUE_SOURCE, + ]); + await callBinding(app, 'queue.Queue.Play'); + await expect(app.getByTestId('now-playing-title')).not.toBeEmpty(); +} + +test.describe('the bottom bar', () => { + test.afterEach(async ({ app }) => { + await callBinding(app, 'queue.Queue.Clear').catch(() => { + /* already empty */ + }); + await app.setViewportSize({ width: 1440, height: 900 }); + }); + + /** + * Four widths, because a centring bug is a function of width: the old + * layout was off by half the difference between the two outer + * columns, so it was wrong by a different amount at each one and + * exactly right at none. + */ + for (const width of [800, 900, 1100, 1440]) { + test(`centres the transport in the window at ${width}px`, async ({ + app, + }) => { + await app.setViewportSize({ width, height: 700 }); + await play(app); + + await expect.poll(() => geometry(app).then((g) => g.offset)).toBe(0); + }); + } + + /** + * The seek bar is what the centring is *paid for* with, so it is + * asserted rather than assumed. + * + * Reserving the metadata's full width on both sides centres the + * transport perfectly and squeezes the control you drag: measured + * during this work at **61px of track at 800px**, against 257 before + * the change. The side columns are capped at a quarter of the bar for + * that reason, and this is the number that says so — 246 at 800px, + * which is parity with the uncentred layout. + */ + test('does not pay for the centring with the seek bar', async ({ app }) => { + await app.setViewportSize({ width: 800, height: 700 }); + await play(app); + + await expect + .poll(() => geometry(app).then((g) => g.seekWidth)) + .toBeGreaterThan(200); + }); + + /** + * #42: the slider is simply there. Three gestures — click open, drag, + * click closed — is what a bottom bar has room not to ask for. + */ + test('shows the volume slider without a click', async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + + const volume = app.locator('.bottom-bar volume-control'); + + await expect(volume).toBeVisible(); + await expect(volume.locator('wa-slider')).toBeVisible(); + }); + + /** + * And the inline icon is the mute toggle, because with the slider + * beside it there is nothing left to disclose. The name follows the + * action rather than the state for the same reason. + */ + test('names the inline icon after what it does', async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + + await expect( + app.locator('.bottom-bar volume-control').getByRole('button', { + name: 'Mute', + }), + ).toBeVisible(); + }); + + /** + * The bar is a fixed 4em row and the transport sits in it. A slider + * with a label grows `#slider` by 8px unless `wa-slider-label.css` + * suppresses it, which moved the whole bar the last time — so the + * height is pinned here rather than left to a screenshot. + */ + test('stays 4em tall', async ({ app }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + await play(app); + + await expect.poll(() => geometry(app).then((g) => g.barHeight)).toBe(64); + }); +}); diff --git a/e2e/specs/control-names.spec.ts b/e2e/specs/control-names.spec.ts index f51b71f..900a024 100644 --- a/e2e/specs/control-names.spec.ts +++ b/e2e/specs/control-names.spec.ts @@ -29,18 +29,13 @@ test.describe('a control says what it controls', () => { }); test('the volume slider is announced as Volume', async ({ app }) => { - // The popup renders no slider at all while closed, the same way the - // queue panel renders no list — so this has to open it first. - await app.getByRole('button', { name: /volume/i }).click(); - + // No disclosure to open first, and no state to put back afterwards: + // #42 made the slider inline, so it is simply there. The assertion + // is unchanged — the *name* is the subject here, and the route to + // the control got shorter rather than different. await expect( app.getByRole('slider', { name: 'Volume' }), ).toBeVisible(); - - // Leave the transport as it was found: the specs share one page in - // file order, and an open popup covers the buttons beneath it. - await app.keyboard.press('Escape'); - await app.locator('body').click({ position: { x: 5, y: 5 } }); }); test('naming the slider did not move the transport', async ({ app }) => { diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index e6aadf9..192084b 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -154,9 +154,26 @@ test.describe('the shell on a phone', () => { await expect(app.locator('now-playing')).toBeVisible(); // Volume is the hardware keys' job on a phone, and a 4px seek bar - // is not a thumb target -- both belong to a later phase's - // full-screen now-playing view. - await expect(app.locator('audio-player volume-control')).toBeHidden(); + // is not a thumb target -- both belong to the full-screen + // now-playing view. + // + // `.bottom-bar volume-control`, not `audio-player volume-control`: + // #42 moved the control out of that component and into the bar, and + // **the old locator would have kept passing** — `toBeHidden()` is + // satisfied by an element that does not exist, so this assertion + // would have gone on reporting success about nothing. Its partner + // below is what makes this one mean something. + await expect(app.locator('.bottom-bar volume-control')).toBeHidden(); + + // The element is there and hidden, rather than absent: the check + // above cannot tell those apart on its own. + await expect(app.locator('.bottom-bar volume-control')).toHaveCount(1); + + // And the seek bar is still inside the transport, where it stands + // down by its own media query. + await expect( + app.locator('audio-player').locator('seek-bar'), + ).toBeHidden(); }); }); diff --git a/frontend/bindings/yellowjacket/backend/config/config.ts b/frontend/bindings/yellowjacket/backend/config/config.ts index 169ae09..8a38637 100644 --- a/frontend/bindings/yellowjacket/backend/config/config.ts +++ b/frontend/bindings/yellowjacket/backend/config/config.ts @@ -69,6 +69,14 @@ export function GetPinDefaultPlaylist(): $CancellablePromise { return $Call.ByID(3818283301); } +/** + * GetPopupVolume reports whether the bottom bar's volume control is a + * click-to-open popup rather than an inline slider (#42). + */ +export function GetPopupVolume(): $CancellablePromise { + return $Call.ByID(2885777); +} + /** * GetQueueFallback returns what plays, if anything, once the queue * runs out. @@ -207,6 +215,18 @@ export function SetPinDefaultPlaylist(pin: boolean): $CancellablePromise { return $Call.ByID(372446849, pin); } +/** + * SetPopupVolume saves the volume control's presentation. + * + * Nothing to validate: both values are legal at every width, and the + * frontend additionally stands the inline slider down below the phone + * breakpoint whatever this says, because that is about room rather than + * about preference. + */ +export function SetPopupVolume(popup: boolean): $CancellablePromise { + return $Call.ByID(1430308453, popup); +} + /** * SetQueueFallback validates and saves a new queue-fallback mode. */ diff --git a/frontend/index.css b/frontend/index.css index 4431617..968cfc4 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -211,7 +211,39 @@ body div.sidebar { padding: 0.25em; background-color: var(--yj-bg-elevated, #343a40); display: grid; - grid-template-columns: var(--now-playing-width, 320px) 1fr auto; + /* Three columns whose outer two are the *same* width, which is what + centres the middle one (#23). It was `var(--now-playing-width) 1fr + auto`, so the transport's centre sat at `W/2 + 140px` — in the + middle of the space left over, which is not the same thing and + reads as an alignment mistake at every window size. + + The outer width is still `--now-playing-width`, so **the metadata + panel's drag handle keeps meaning something**: widening it takes + room from the transport on both sides at once, symmetrically. An + `1fr … 1fr` pair would have centred the transport just as well and + silently made that handle a no-op. + + **The cap is what stops that being a regression**, and it was + measured as one first. Reserving the full metadata width on both + sides costs the transport twice: at 800px the outer pair wanted + 640 of 800, and the seek bar's track went from 257px to 61px + (and to 0 at 200% text) — the control you drag, squeezed out to + centre the buttons above it. So the side tracks are the metadata + width *or a quarter of the bar*, whichever is smaller, which + leaves the drag handle meaningful everywhere it has room to be + and hands the difference to the transport where it does not. + + `minmax(0, …)` on the outer tracks and `min-content` on the middle + decide who yields when even that is not enough: the metadata and + the end group shrink (both truncate; neither loses an action), and + the transport keeps at least its buttons. Without the `min-content` + floor the middle collapses first, because a `1fr` track's minimum + is `auto` only until something else insists. */ + --bar-side: min(var(--now-playing-width, 320px), 25%); + grid-template-columns: + minmax(0, var(--bar-side)) + minmax(min-content, 1fr) + minmax(0, var(--bar-side)); align-items: center; contain: layout style; @@ -239,23 +271,44 @@ body div.sidebar { text-wrap-mode: nowrap; overflow: hidden; - p { + /* `& p`, not `p`. **A nested rule that begins with a bare + element selector is silently dropped before Chrome 120** + (relaxed nesting), and the phone this app runs on renders + in Chrome 113 -- so this ellipsis, and the two rules + below, have never applied on the device. Nothing fails; + the text simply overflows there. The `&` form is valid in + both, which is why it is used for every element selector + in this file's nested blocks. */ + & p { overflow: hidden; text-overflow: ellipsis; } } } - now-playing { + & now-playing { overflow: hidden; } - audio-player { + & audio-player { margin: 0.5em 1em; + min-width: 0; + } + + /* The right-hand group, and the thing the left column is matched + against. It is one grid cell rather than two columns because the + centring rule above compares *columns*: volume and the queue + button in separate tracks would make the outer pair unequal by + whatever the volume happens to measure. */ + .bar-end { + justify-self: end; + display: flex; + align-items: center; + gap: 0.25em; + min-width: 0; } #queue-button { - justify-self: end; background: none; border: none; color: inherit; @@ -440,6 +493,11 @@ body div.sidebar { } @media (max-width: 599px) { + /* The phone keeps the two-part bar it had: metadata, then the + transport and the queue button. There is no third column to + balance because the centring the desktop does is a luxury of + having room — at 360px the metadata needs all of the space the + controls do not. */ .bottom-bar { grid-template-columns: minmax(0, 1fr) auto auto; gap: 0.25em; @@ -448,4 +506,19 @@ body div.sidebar { .bottom-bar audio-player { margin: 0.25em; } + + /* Volume stands down here whatever the setting says, because this + is about room and about the platform rather than about + preference: the hardware keys own volume on a phone, which is + also why mediacontrols' Android handler implements no volume + callback. It moved from `audio-player`'s own media query when + #42 moved the control into the bar — same rule, and now stated + where the element actually is. + + `.bottom-bar volume-control`, not the one in + `now-playing-view`: that view is the phone's transport and is + where a slider does belong. */ + .bottom-bar volume-control { + display: none; + } } diff --git a/frontend/index.html b/frontend/index.html index dbfa05c..12cfb5e 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -40,16 +40,31 @@ +
- +
+ + +