From 218e4f5e991def8c6ecea7a7b8ab887d5671faec Mon Sep 17 00:00:00 2001 From: Logan Date: Thu, 20 Aug 2026 23:42:15 -0400 Subject: [PATCH] feat(player): give the transport a context, and thumb-sized controls Measured at the reference device's 424x439, every button here was 33x21px -- in the bottom bar and on the full-screen view alike. #56 reports them as "the most important thing in the mobile app and they are tiny", and that is the number behind it. The context is a **property, not a media query**, and that is the whole design. Everywhere else in this app a component states what it drops at phone width itself, because a media query inside a shadow root is answered by the viewport and that is the honest signal. Here the two hosts want different answers at the *same* viewport: on a phone the bar wants three controls sized for a thumb and now-playing-view wants five, larger still. So the host says which context and the viewport says which size band, and neither alone can express it. Play/pause alone goes above the 44px floor. A row of five identical squares says every action is equally likely, which is not true of play -- "large play/pause, adequate prev/next" is the Direction, and a spec caught that the first version had sized all three the same. Two things that fail silently: The desktop bar must not move, and a ` + `; + } - const shuffleClass = this.shuffleMode ? 'active' : ''; + /** Repeat, whose label spells the mode out because one icon covers + * three states. */ + private renderRepeat() { const repeatMode = this.repeatMode; const repeatClasses = [ repeatMode !== 'off' ? 'active' : '', repeatMode === 'one' ? 'repeat-one' : '', ].filter(Boolean).join(' '); + return html` + + `; + } + + /** Previous, play/pause, next — the three that are always drawn, in + * every context and at every width. Only play/pause takes the large + * size: the Direction asks for "large play/pause, adequate + * prev/next", and a row of identical squares says every action here + * is equally likely, which is not true of play. */ + private renderPrimary() { + const playOrPauseIcon = this.player.isPlaying ? 'pause' : 'play'; + const playOrPauseHandler = this.player.isPlaying + ? this.handlePauseClick + : this.handlePlayClick; + + return html` + + + + `; + } + + /** + * Two arrangements, not two components. + * + * `bar` keeps the order it has always had — shuffle, prev, play, + * next, repeat, one row — so nothing about the desktop bar moves. + * `full` puts the primary three on their own row with the secondary + * pair beneath, which the Direction asks for. + * + * **The phone's bar draws three buttons rather than hiding two.** A + * `display: none` control is still in the component's shadow root, + * still in the accessibility tree's markup, and still something a + * `shadowAll('button')[4]` finds — so "the phone has three controls" + * would be true of the pixels and false of the element. They are + * reachable on the full-screen view, which the mini player's art + * opens, and through the global shortcuts. + */ + override render() { + if (this.context === 'full') { + return html` +
${this.renderPrimary()}
+
+ ${this.renderShuffle()}${this.renderRepeat()} +
+ `; + } + return html`
- - - - - + ${this.slim ? nothing : this.renderShuffle()} + ${this.renderPrimary()} + ${this.slim ? nothing : this.renderRepeat()}
`; } 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 d426dd3..ab71ded 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -117,8 +117,27 @@ export class NowPlayingView extends LitElement { .art .placeholder { /* Square, and never taller than the room left over: the art is the one thing here that would happily push the - transport off the bottom of a short phone. */ + transport off the bottom of a short phone. + + **max-height is what actually keeps that promise**, and + it was missing. With a definite width and + a 1:1 aspect-ratio the height is *derived from the width* + and is bounded by nothing: at the reference device's + 424x439 that is a 263px square (60vh) in a box with far + less than 263px left, so the art overflowed its own + centred flex item and drew over the header above and the + title below it. The comment claimed this was handled; + 60vh is a bound on the *viewport*, not on the room left + over, and those differ by however much chrome is above + and below. + + Pre-existing -- screenshotted on main -- and made acute + by #56, which gives the transport 95px more than it had. + Found by reading a screenshot, which is the only tier + that can see it: nothing fails, nothing overflows the + *shell*, and every control is still hittable. */ width: min(100%, 60vh); + max-height: 100%; aspect-ratio: 1; object-fit: cover; border-radius: 12px; @@ -317,7 +336,13 @@ export class NowPlayingView extends LitElement {
- + +
`; diff --git a/frontend/test/components/transport-context.test.ts b/frontend/test/components/transport-context.test.ts new file mode 100644 index 0000000..72aa089 --- /dev/null +++ b/frontend/test/components/transport-context.test.ts @@ -0,0 +1,181 @@ +/** + * The transport in its two contexts (#59, #56). + * + * `player-controls` is one component in two places, and what each place + * wants differs *at the same viewport*: on a phone the bottom bar wants + * three controls sized for a thumb, and `now-playing-view` wants five, + * larger still. So the host states the context and the viewport states + * the size band, and this file pins the half a media query cannot + * express. + * + * **What this tier can and cannot see.** It can see which buttons + * exist, because that is `matchMedia` and a render — and existence is + * the whole of #59. It cannot see the *sizes*: those come from the + * context's custom properties, and a component-tier render has no shell + * around it, so the measurements live in `e2e/specs/phone-transport.spec.ts` + * where there is a real bar in a real viewport. Asserting a pixel here + * would be asserting the fallbacks, which is `ui-visual`'s documented + * blind spot one tier over. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; + +import '@components/audio-player/controls/player-controls'; +import { Events } from '../../src/events'; +import { emit, flush, calls } from '@test/support/harness'; +import { fixture, shadowAll, click } from '@test/support/render'; + +/** Reset the backend-owned state the component reads from. */ +function idle(): void { + emit(Events.TrackChanged, null); + emit(Events.PlaybackStateChanged, { state: 'stopped' }); + emit(Events.QueueModeChanged, { shuffleMode: false, repeatMode: 'off' }); +} + +const names = (el: Element): Array => + shadowAll(el, 'button').map((b) => b.getAttribute('aria-label')); + +/** + * Answer `matchMedia` for the phone query, since the test runner's own + * window is whatever size the browser provider gives it. + * + * It is stubbed rather than resized because what is under test is the + * component's *reaction* to the answer, and a resize would additionally + * be asserting that this runner's viewport can get below 600px. + */ +const realMatchMedia = window.matchMedia; + +function pretendPhone(phone: boolean): void { + window.matchMedia = ((query: string) => ({ + matches: phone && query.includes('599'), + media: query, + addEventListener: () => {}, + removeEventListener: () => {}, + })) as unknown as typeof window.matchMedia; +} + +afterEach(() => { + window.matchMedia = realMatchMedia; +}); + +describe(' in the bar', () => { + beforeEach(() => { + idle(); + }); + + it('keeps all five on a desktop, in the order it always had', async () => { + pretendPhone(false); + + const el = await fixture('player-controls'); + + // Unchanged from before #59, deliberately: this is the desktop bar + // and nothing about it was reported. + expect(names(el)).toEqual([ + 'Shuffle', + 'Previous track', + 'Play', + 'Next track', + 'Repeat: off', + ]); + }); + + it('draws three on a phone, and does not merely hide the other two', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + expect(names(el)).toEqual(['Previous track', 'Play', 'Next track']); + + // The distinction this asserts is the point. A `display: none` + // control is still in the shadow root, still something a positional + // query finds, and still a thing the component claims to have -- + // so "the phone has three controls" would have been true of the + // pixels and false of the element. + expect(shadowAll(el, 'button')).toHaveLength(3); + }); + + it('follows the viewport when it changes, not just at construction', async () => { + pretendPhone(false); + + const el = await fixture('player-controls'); + + expect(names(el)).toHaveLength(5); + + // A desktop window dragged narrow is the phone layout, per plan + // 018's decision 4 -- so this is a real transition and not a + // hypothetical. + (el as unknown as { phone: boolean }).phone = true; + await flush(); + await el.updateComplete; + + expect(names(el)).toEqual(['Previous track', 'Play', 'Next track']); + }); +}); + +describe(' full-screen', () => { + beforeEach(() => { + idle(); + }); + + it('keeps all five on a phone, where the bar keeps three', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // The same viewport, the other answer: this is why the context is a + // property and cannot be a media query. + expect(names(el)).toHaveLength(5); + }); + + it('puts the secondary pair after the primary three, in the DOM', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // Order, not just membership: the secondary controls are drawn on a + // second row, and this is asserted in the DOM because visual order + // and focus order have to agree. A CSS `order` property would move + // them on screen and leave Tab walking the old sequence. + expect(names(el)).toEqual([ + 'Previous track', + 'Play', + 'Next track', + 'Shuffle', + 'Repeat: off', + ]); + }); + + it('still routes every button to the backend', async () => { + pretendPhone(true); + + const el = await fixture('player-controls'); + + el.setAttribute('context', 'full'); + await el.updateComplete; + + // Two arrangements, one set of handlers. The regression this + // guards is the reason a second *component* was refused: a second + // template renders buttons wired to nothing, which looks perfect + // in a screenshot and does nothing at all. + for (const name of [ + 'Previous track', + 'Next track', + 'Shuffle', + 'Repeat: off', + ]) { + await click(el, `button[aria-label="${name}"]`); + } + + expect(calls().map((c) => c.path)).toEqual([ + 'queue.Queue.Previous', + 'queue.Queue.Next', + 'queue.Queue.ToggleShuffle', + 'queue.Queue.CycleRepeat', + ]); + }); +});