diff --git a/.planning/NOTES.md b/.planning/NOTES.md index dcc33c0..dd1a3b0 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4487,3 +4487,89 @@ is not orphaned" and "a docked column is not in the stack" are both vacuously true of a build that pushes no entry at all. Reverting the source and re-running is what established which were which, and the file says so in its header rather than implying all nine reproduce. + +## The phone's transport, and three things that only a screenshot or a stash could see (measured 2026-08-21) + +#59 and #56 were done as one PR — argued on #73 first — because they are +the same row of pixels: one removes controls from the phone's bar and +the other enlarges what is left, and both are one property on +`player-controls`. Measured at 424x439 before: + +| control | before | after | +|---|---|---| +| bar: shuffle / prev / play / next / repeat | 33x21 each | prev/next 44, play 56, shuffle+repeat moved | +| bar: favourite | **18x14** | 44x44 | +| bar: queue button | 33x29 | gone (#59) | +| Now Playing: all five | 33x21 each | 44, play 64 | +| desktop bar: all five | 33x21 | **33x21** | + +Four things cost a cycle each and are worth keeping. + +**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/src/components/now-playing/now-playing.ts b/frontend/src/components/now-playing/now-playing.ts index 860a7a2..8e335a5 100644 --- a/frontend/src/components/now-playing/now-playing.ts +++ b/frontend/src/components/now-playing/now-playing.ts @@ -215,6 +215,17 @@ export class NowPlaying extends LitElement { outline: 2px solid var(--yj-accent, #ffd43b); outline-offset: 2px; } + + /* The favourite is one of the three controls #59 keeps on the + phone's bar, and it was the **smallest control in the app**: + measured at 424x439, 18x14px, against the 48x48 art beside it. + Zero padding around an icon-sized glyph is a reasonable mouse + target and is not a thumb target at all. */ + .fav-btn { + min-width: 44px; + min-height: 44px; + font-size: var(--yj-icon-md); + } } .cover-preview-panel { @@ -446,8 +457,30 @@ export class NowPlaying extends LitElement { return html`
${announcement}
-
-
+ +
+ +
+
+
=> + 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', + ]); + }); +});