diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index 897f998..8969511 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -130,6 +130,25 @@ test.describe('the shell on a phone', () => { // are here, and they are the *same* components -- this view // composes the transport rather than reimplementing it. await expect(app.locator('now-playing-view seek-bar')).toBeVisible(); + + // Volume is here **because the player says there is one** (#64), + // not because this is a phone. This tier is the platform that owns + // its own volume, so what it can assert is that the control's + // presence follows that answer -- an inverted polarity in + // `volume-style-store` fails here and in `bottom-bar.spec.ts`, and + // the *absent* branch is checked in the component tier, where the + // binding can be stubbed. Nothing here can reach the Android side. + const systemOwns = await app.evaluate( + async () => + (await window.__yjEvents.call( + 'player.Player.SystemOwnsVolume', + [], + 5_000, + )) as boolean, + ); + + expect(systemOwns, 'this platform should own its own volume').toBe(false); + await expect(app.locator('now-playing-view volume-control')).toBeVisible(); // Back goes where the user came from, through the nav stack. diff --git a/frontend/index.css b/frontend/index.css index a42aa45..30172ae 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -522,16 +522,20 @@ body div.sidebar { } /* 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. + is about room: five controls and a slider do not fit a 360px + bar, and the full-screen now-playing view is where seeking and + volume go on a phone. 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. */ + **This rule used to carry the platform argument too, and no + longer does** (#64). "The hardware keys own the volume" is not a + width: it is false of a narrow desktop window and true of an + Android tablet, which this selector gets backwards both ways. + The player answers it now — `SystemOwnsVolume` — and + `volume-control` renders nothing when it is true, at every + width and in both of its mount points. What is left here is the + question a stylesheet can actually answer. */ .bottom-bar volume-control { display: none; } diff --git a/frontend/src/components/audio-player/volume-control/volume-control.ts b/frontend/src/components/audio-player/volume-control/volume-control.ts index 1ad0ada..7dd070d 100644 --- a/frontend/src/components/audio-player/volume-control/volume-control.ts +++ b/frontend/src/components/audio-player/volume-control/volume-control.ts @@ -1,4 +1,4 @@ -import { LitElement, html, css } from 'lit'; +import { LitElement, html, css, nothing } from 'lit'; import { customElement, state } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/slider/slider.js'; @@ -27,6 +27,26 @@ export class VolumeControl extends LitElement { @state() private popup = volumeStyleStore.popup; + /** + * Whether there is a volume of ours to control at all (#64). + * + * The decision is made here rather than at either mount point, + * because there are two -- the bottom bar's copy lives in + * `index.html`, which has no module scope to make it conditional -- + * and one of them is a control the shell cannot un-render. So the + * control answers for itself, and the bar and the phone's + * full-screen transport get the same answer without either knowing + * the question exists. + * + * It renders `nothing` *and* hides the host: an empty shadow root is + * what stops a positional or role query finding a button that cannot + * act, and `:host([hidden])` is what stops the element occupying a + * flex item's worth of the transport -- the `:host` display above + * outranks the UA's `[hidden]` rule, so it has to be said. + */ + @state() + private available = volumeStyleStore.available; + private unsubscribeStyle?: () => void; // Locally-tracked volume while the user is actively dragging or scrolling. @@ -43,6 +63,13 @@ export class VolumeControl extends LitElement { align-items: center; } + /* See the available field. A gap is only drawn between boxes, + so a hidden host costs its parent nothing -- which is where the + 29px this gives back to Now Playing comes from (#172). */ + :host([hidden]) { + display: none; + } + button { background: none; border: none; @@ -154,12 +181,15 @@ export class VolumeControl extends LitElement { this.unsubscribeStyle = volumeStyleStore.subscribe(() => { this.popup = volumeStyleStore.popup; + this.setAvailable(volumeStyleStore.available); // Switching to the slider while the popup is open would leave the // document listener installed for a popup that no longer renders. if (!this.popup) this.closeSlider(); }); + this.setAvailable(volumeStyleStore.available); + void volumeStyleStore.init(); } @@ -233,7 +263,25 @@ export class VolumeControl extends LitElement { // RENDER // =================================================================== + /** + * `hidden` is set imperatively rather than reflected from the state, + * because it has to be on the *host* and a `@state` does not reflect. + * It is the right attribute besides: it takes the element out of the + * accessibility tree as well as out of the layout. + */ + private setAvailable(available: boolean) { + this.available = available; + this.hidden = !available; + + // A popup left open when the control goes away would keep its + // document click listener installed for markup that no longer + // renders. + if (!available) this.closeSlider(); + } + override render() { + if (!this.available) return nothing; + const muted = this.player.muted; // Inline, the icon is the mute toggle rather than a disclosure: diff --git a/frontend/src/components/now-playing-view/now-playing-view.ts b/frontend/src/components/now-playing-view/now-playing-view.ts index ab71ded..cbf1d1d 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -343,6 +343,12 @@ export class NowPlayingView extends LitElement { a media query because the bottom bar wants a different answer at this same viewport. --> + `; diff --git a/frontend/src/store/volume-style-store.ts b/frontend/src/store/volume-style-store.ts index 88ee68a..89ef98a 100644 --- a/frontend/src/store/volume-style-store.ts +++ b/frontend/src/store/volume-style-store.ts @@ -1,5 +1,6 @@ import { EventsOn } from '@runtime/runtime'; import { GetPopupVolume } from '@go/config/config.js'; +import { SystemOwnsVolume } from '@go/player/player.js'; import { Events } from '../events'; type Subscriber = () => void; @@ -28,10 +29,37 @@ type Subscriber = () => void; * becomes one. An install that has chosen the popup sees it swap once * on load, which is the cheaper of the two wrong first frames: the * inline slider occupies the space the popup's button would have. + * + * **`available` is the question one step earlier — whether there is a + * volume of ours to draw at all (#64).** On Android the hardware keys + * are the volume control and the backend pins its own level at + * maximum, so a slider here would move nothing. + * + * It is asked of the *player* rather than of the viewport, and that is + * the whole design decision. Every other stand-down rule in this app + * is a width, because a width is what a browser can answer and what + * every tier can test — but this one is a property of the build. Keyed + * on width instead, an Android tablet at 600px or more would draw the + * bottom bar's slider over a pinned level: a control that cannot act, + * which `library-status-indicator` settled is worse than none. + * + * It lives beside `popup` because both answer "what presentation does + * the volume control get", both readers are the same two components, + * and "none" is a presentation. A second store would be a second + * subscription in the same `connectedCallback` saying the same thing. + * + * The initial value is `true` on the same first-frame rule: there is a + * volume on every platform but one, and the platform that pins it sees + * the control once at boot and never again in the session — the answer + * cannot change while the app runs, so by the time the lazily-mounted + * now-playing view exists it has long been settled by the bar's own + * copy. */ class VolumeStyleStore { private value = false; + private hasVolume = true; + private loaded = false; private subscribers = new Set(); @@ -47,13 +75,21 @@ class VolumeStyleStore { return this.value; } + /** + * Whether this app has a volume of its own to control. False where + * the device owns it; see the class comment. + */ + get available(): boolean { + return this.hasVolume; + } + /** Reads the setting once. Safe to call from every mount. */ async init(): Promise { if (this.loaded) return; this.loaded = true; - await this.refresh(); + await Promise.all([this.refreshAvailability(), this.refresh()]); } subscribe(fn: Subscriber): () => void { @@ -77,6 +113,28 @@ class VolumeStyleStore { } } + /** + * Asked once, not on `GeneralConfigChanged`: this is a property of + * the platform the binary was built for and cannot change while + * the app is running. + */ + private async refreshAvailability(): Promise { + try { + const owned = await SystemOwnsVolume(); + + if (owned === !this.hasVolume) return; + + this.hasVolume = !owned; + this.notify(); + } catch (err) { + // The control renders, which is the answer on every + // platform but one and is the recoverable way to be wrong: + // a working control nobody needs, rather than a missing one + // somebody does. + console.error('failed to ask who owns the volume', err); + } + } + private notify(): void { for (const fn of this.subscribers) fn(); } diff --git a/frontend/test/components/volume-ownership.test.ts b/frontend/test/components/volume-ownership.test.ts new file mode 100644 index 0000000..00fb6a9 --- /dev/null +++ b/frontend/test/components/volume-ownership.test.ts @@ -0,0 +1,93 @@ +/** + * Who owns the volume, and what the control does when it is not us + * (#64). + * + * **This is the tier that can exercise the Android branch**, and it is + * the reason the predicate is a backend answer rather than a build tag + * the frontend cannot see: `SystemOwnsVolume` is a stub here, so the + * "no volume" rendering is checked on an ordinary Linux CI runner with + * no device anywhere. What no tier here can check is the *constant* + * behind it, which `TestPlatformVolumeOwnershipIsDeclaredOncePerPlatform` + * sweeps the Go source for instead. + * + * It is a file of its own because `volumeStyleStore` asks once and + * latches — the answer is a property of the binary and cannot change + * while the app runs, so there is deliberately no event that refreshes + * it. Vitest gives each file its own module registry, which is what + * lets the stub be in place before the singleton is first touched. + * The *available* case is the rest of `transport.test.ts`, which mounts + * the same element under the default stub. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import '@components/audio-player/volume-control/volume-control'; +import '@components/now-playing-view/now-playing-view'; +import { Events } from '../../src/events'; +import { emit, resetHarness, stub } from '@test/support/harness'; +import { fixture, shadow, shadowAll } from '@test/support/render'; +import type { TrackInfo } from '@store/player-store'; + +const TRACK: TrackInfo = { + fileName: 'tideline.mp3', + filePath: '/music/tideline.mp3', + trackLength: 245, + seekPosition: 0, + state: 'playing', + title: 'Tideline', + artist: 'Sea Change', + album: 'Ebb', + coverArt: '', + coverArtSmall: '', + coverArtMedium: '', + coverArtLarge: '', + trackChangeId: 1, + artistMbid: '', + releaseGroupMbid: '', + recordingMbid: '', +}; + +describe('a platform whose volume we do not own', () => { + beforeEach(async () => { + resetHarness(); + stub('player.Player.SystemOwnsVolume', true); + stub('config.Config.GetPopupVolume', false); + + // The store latches on the first mount; do it here so every test + // below sees a settled answer rather than the first frame. + const warm = await fixture('volume-control'); + + await warm.updateComplete; + }); + + it('renders no control at all, and no empty shadow root to find', async () => { + const el = await fixture('volume-control'); + + await el.updateComplete; + + // Both halves matter. An empty shadow root is what stops a + // positional or by-role query finding a button that cannot act; + // `hidden` is what stops the host taking a flex item's worth of + // space in the transport it sits in. + expect(shadowAll(el, 'button')).toHaveLength(0); + expect(shadowAll(el, 'wa-slider')).toHaveLength(0); + expect(el.hidden, 'the host is not hidden').toBe(true); + }); + + it('leaves the rest of the phone transport alone', async () => { + emit(Events.TrackChanged, TRACK); + + const view = await fixture('now-playing-view'); + + await view.updateComplete; + + // Seeking and the transport buttons are not volume, and #64 is + // allowed to remove one control, not to thin the screen out. + expect(shadow(view, 'seek-bar')).not.toBeNull(); + expect(shadow(view, 'player-controls')).not.toBeNull(); + + const volume = shadow(view, 'volume-control') as HTMLElement | null; + + expect(volume, 'the element is still mounted').not.toBeNull(); + expect(volume!.hidden, 'a mounted volume-control is not hidden').toBe(true); + }); +});