diff --git a/frontend/src/components/audio-player/controls/player-controls.ts b/frontend/src/components/audio-player/controls/player-controls.ts index 6353c8b..6a8030a 100644 --- a/frontend/src/components/audio-player/controls/player-controls.ts +++ b/frontend/src/components/audio-player/controls/player-controls.ts @@ -1,22 +1,77 @@ -import { LitElement, html, css } from 'lit'; -import { customElement, state } from 'lit/decorators.js'; +import { LitElement, html, css, nothing } from 'lit'; +import { customElement, property, state } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import { PlayerController } from '@store/controllers/player-controller'; import { queueStore } from '@store/queue-store'; import type { RepeatMode } from '@store/queue-store'; import { designTokens } from '../../../styles/tokens.css'; +import { PHONE_QUERY } from '../../../utils/breakpoints'; + +/** + * The transport, in the two places it appears. + * + * **The context is a property and cannot be a media query**, which is + * the whole reason this exists (#56). 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 bottom 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 one alone can express it. + * + * Measured at the reference device's 424x439 before this: every button + * here was **33x21px**, in both places, which is what #56 reports as + * "the most important thing in the mobile app and they are tiny". + */ +export type ControlsContext = 'bar' | 'full'; @customElement('player-controls') export class PlayerControls extends LitElement { private player = new PlayerController(this); private unsubscribeQueue?: () => void; + /** + * Where these controls are drawn. `bar` is the bottom bar in both + * bands; `full` is the full-screen transport. + * + * Reflected so a spec can read it and so the stylesheet keys off one + * fact rather than a class the host has to remember to set. + */ + @property({ type: String, reflect: true }) + context: ControlsContext = 'bar'; + @state() private shuffleMode = false; @state() private repeatMode: RepeatMode = 'off'; + /** + * Phone width, from `matchMedia` rather than from a media query, + * because what it decides is whether shuffle and repeat *exist* here + * — and a stylesheet can only decide whether they are painted. + * `job-band` and `search-trigger` are the same pattern for the same + * reason. + */ + @state() private phone = false; + + private media?: MediaQueryList; + + private onMedia = (e: MediaQueryListEvent) => { + this.phone = e.matches; + }; + + /** Whether this is the phone's bottom bar, which carries three + * controls rather than five. */ + private get slim(): boolean { + return this.context === 'bar' && this.phone; + } + override connectedCallback(): void { super.connectedCallback(); + this.media = window.matchMedia(PHONE_QUERY); + this.phone = this.media.matches; + this.media.addEventListener('change', this.onMedia); + const s = queueStore.getState(); this.shuffleMode = s.shuffleMode; this.repeatMode = s.repeatMode; @@ -37,6 +92,7 @@ export class PlayerControls extends LitElement { override disconnectedCallback(): void { super.disconnectedCallback(); this.unsubscribeQueue?.(); + this.media?.removeEventListener('change', this.onMedia); } static override styles = [designTokens, css` @@ -58,6 +114,99 @@ export class PlayerControls extends LitElement { justify-content: center; } + /* --------------------------------------------------------------- + Sizes (#56). + + 44px is the floor everything here is sized to, and play/pause + alone goes above it -- "large play/pause, adequate prev/next" is + the Direction, and it is the one control the report calls "front + and centre". + + They are stated as custom properties rather than on each button + so a context sets two numbers instead of five rules, and so the + icon scales with its target: a 44px box around a 16px glyph is a + big hit area that still looks tiny, which is half of what the + report is about. + + **The desktop bar sets none of them and must not change at all.** + #56 is an Android issue; the desktop's buttons are 33x21 before + this and are 33x21 after it. + + That is why the box rules take a zero fallback and the *font-size* + rules are scoped to the two contexts instead of sharing them. A + button does not inherit its font from its parent -- the UA + stylesheet gives it one -- so a generic font-size: inherit is + not the no-op it reads as: it moved the desktop's buttons from + 33x21 to 36x24, silently, by taking them from the UA's 13.3px to + the shell's 16px. Measured before and after by stashing this + file, which is the only way that particular 3px shows up. + --------------------------------------------------------------- */ + button { + min-width: var(--yj-control-target, 0); + min-height: var(--yj-control-target, 0); + } + + button.play { + min-width: var(--yj-control-play-target, 0); + min-height: var(--yj-control-play-target, 0); + } + + /* The phone's bottom bar: three controls, sized for a thumb. + Shuffle and repeat are not here -- see the render method, which + does not draw them rather than hiding them, because a control + that is display:none is still a thing the component claims to + have. They are on the full-screen view, which is one tap away + through the mini player's art (#59). */ + @media (max-width: 599px) { + :host([context='bar']) { + --yj-control-target: 44px; + --yj-control-icon: 18px; + --yj-control-play-target: 56px; + --yj-control-play-icon: 24px; + } + + :host([context='bar']) button { + font-size: var(--yj-control-icon); + } + + :host([context='bar']) button.play { + font-size: var(--yj-control-play-icon); + } + } + + /* The full-screen transport, at every width: this view *is* the + player, so the controls are the page rather than a strip of it. */ + :host([context='full']) { + --yj-control-target: 44px; + --yj-control-icon: 20px; + --yj-control-play-target: 64px; + --yj-control-play-icon: 28px; + } + + :host([context='full']) button { + font-size: var(--yj-control-icon); + } + + :host([context='full']) button.play { + font-size: var(--yj-control-play-icon); + } + + :host([context='full']) #player-control-buttons { + gap: 12px; + } + + /* Secondary controls sit below the primary row rather than beside + it, which is the Direction's shape and is why this is a second + group in the DOM instead of a CSS order property: visual order + and focus order have to agree. */ + .secondary { + display: flex; + justify-content: center; + align-items: center; + gap: 24px; + margin-top: 8px; + } + button:hover { color: var(--yj-accent-text, #ffd43b); } @@ -104,46 +253,107 @@ export class PlayerControls extends LitElement { queueStore.cycleRepeat(); }; - override render() { - const playOrPauseIcon = this.player.isPlaying ? 'pause' : 'play'; - const playOrPauseHandler = this.player.isPlaying - ? this.handlePauseClick - : this.handlePlayClick; + /** Shuffle. Secondary: it changes how the queue behaves rather than + * what is playing now. */ + private renderShuffle() { + return html` + + `; + } - 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', + ]); + }); +});