diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 9fb69ca..8283ae8 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4958,3 +4958,39 @@ than on screen.** The first two probes (24px/0.45, then 32px/0.75) were measurably present — 52,58,64 down to 30,33,37 — and invisible in the inline preview. Crop the bottom 70px and scale it up before judging; the pixel values are the honest answer either way. + +## The phone's context sheet is now longer than the phone (measured 2026-08-23, headless at 424x439) + +#67 moves two destinations into every row menu, and the track list's +menu is where that runs out of screen. Measured against the running +app at the reference viewport, one row selected: + +| menu | items | first item top | last item bottom | +|---|---|---|---| +| queue panel | 7 | 95 | 431 | +| track list | 8 | 86 | **470** | + +The viewport is 439. So the track list's last item — "Remove from +Library" — is below the fold. It is **not unreachable**: the sheet is a +`wa-dialog` whose body is `overflow-y: auto`, measured `scrollHeight` +412 against `clientHeight` 373, and scrolling it 39px brings that item +fully into view (383–431). What it has is no *affordance*: nothing on +screen says the list continues. + +Two things worth knowing before adding a ninth item anywhere. + +**The limit was already reached, and this is what crossed it.** Seven +48px rows in a 373px body is 364px — the queue's menu fits with 8px to +spare and the track list's fitted exactly. Any item added to any of the +fourteen menus after #60 was going to be the one that overflowed; the +first one simply happened to be this. + +**The measurement has to be taken with a row selected**, since the +`Go to` items are drawn for a single selection only, and on the *first* +track of the fixture library — which has no album (`01 Tone A`, +`02 Tone B`) — only "Go to Artist" appears. That is the 8 above; an +ordinary track makes it 9. + +Filed as its own issue rather than fixed in #67's diff: it is a +property of the shared sheet (`components/menu-surface/`), not of the +items. diff --git a/CLAUDE.md b/CLAUDE.md index 66c4da2..5122044 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2375,6 +2375,31 @@ a list or a detail view: and dropped if a second click arrives, because the title is the widest thing in a row and double-clicking a row plays it. Rows do not need to know links exist. + + **Below 600px a name is not a link, and the row's menu is where it + went** (#67). Every sentence above is a *desktop* compromise: the + double-click grace means nothing on touch, a few characters of text + is not a touch target, and since #63 a claimed `yj-tap` has its click + swallowed, so the link was unreachable as well as fiddly. The rule is + in the utility rather than at twenty call sites, and + `utils/go-to-menu.ts` is the other half — "Go to Artist" / "Go to + Album", drawn under exactly the condition the link is not, from + `explore-link`'s own exported routing so an untagged artist reaches + the library page by the same lookup. + + Three things about it are load-bearing. **Suppressing a link without + a menu behind it is not a smaller affordance**, it is a destination + the phone cannot reach — so `keepOnPhone` is the documented exception + for the three surfaces with no row menu (`now-playing-view`, + `explore-album-details`' header credit, `top-results-row`), and + nothing else may pass it. **One row or none**: the items are the Play + item's rule one step on, since "go to the album" of five different + albums means nothing. And **there is no "Go to Genre"**, because + there is no genre link anywhere to lose — that would be new + navigation rather than a replacement, and belongs in its own issue. + `track-list` is the one list that gains rather than moves: its phone + column set stacks title over artist as plain text already, so those + names have never been links there. - **``** is how a detail page admits what it is showing: catalog data (silent), a library stand-in while a fetch is in flight, library-only because the entity has no MBID, or a failed/ diff --git a/e2e/specs/phone-entity-links.spec.ts b/e2e/specs/phone-entity-links.spec.ts new file mode 100644 index 0000000..3f0c0d5 --- /dev/null +++ b/e2e/specs/phone-entity-links.spec.ts @@ -0,0 +1,135 @@ +import { + test, + expect, + callBinding, + openTheQueue, + NO_QUEUE_SOURCE, +} from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * #67 — a name is not a link on a phone, and the menu is where it went. + * + * The queue panel is the surface this is visible on: its rows draw a + * track title and an artist credit as `explore-link`s at every width, + * unlike `track-list`, whose phone column set stacks title over artist + * as plain text already. + * + * **The pair is what makes either assertion mean anything.** A link + * that is gone and a menu item that never arrived is not a smaller + * affordance — it is a destination the phone cannot reach, which is + * what plan 018's "no action is unreachable at any supported size" + * refuses. So each test asserts the phone and the desktop in the same + * breath: text *and* an item here, a link *and* no item there. + * + * The desktop half is also the regression guard for the change: menus + * above the breakpoint must be exactly what they were, because the name + * beside them is still a link and a menu that repeats the row is + * furniture. + */ + +/** The reference device's real viewport, not a resized desktop. */ +const DEVICE = { width: 424, height: 439 }; + +/** Wide enough that the queue is a column beside the content. */ +const DESKTOP = { width: 1280, height: 800 }; + +const row = (app: Page, index: number) => + app.locator(`queue-panel .track-item[data-index="${index}"]`); + +/** The queue panel's own context menu, as a list of item labels. */ +async function menuLabels(app: Page): Promise { + return app.evaluate(() => + [ + ...document + .querySelector('queue-panel')! + .shadowRoot!.querySelectorAll('wa-dropdown-item'), + ].map((item) => item.textContent?.replace(/\s+/g, ' ').trim() ?? ''), + ); +} + +/** + * Queue three tracks that have an album, for the reason + * `queue-selection.spec.ts` states at length: `explore-link` routes a + * title to its *album's* page and renders plain text where it cannot + * route, so a track with no album answers this file's question with + * the wrong "no link". + */ +async function queueThree(app: Page): Promise { + const paths = await app.evaluate(async () => { + const tracks = (await window.__yjEvents.call( + 'library.Library.GetTracks', + [0], + 10_000, + )) as { FilePath: string; Album: string; ArtistName: string }[]; + + return tracks + .filter((t) => t.Album !== '' && t.ArtistName !== '') + .slice(0, 3) + .map((t) => t.FilePath); + }); + + await callBinding(app, 'queue.Queue.SetQueue', [ + paths, + 0, + false, + NO_QUEUE_SOURCE, + ]); +} + +/** Open the row's context menu and read the items back. */ +async function openRowMenu(app: Page, index: number): Promise { + await row(app, index).click({ button: 'right' }); + await expect + .poll(async () => (await menuLabels(app)).length) + .toBeGreaterThan(0); + + return menuLabels(app); +} + +test.describe('an inline name and the menu that replaces it', () => { + test.afterEach(async ({ app }) => { + await app.keyboard.press('Escape'); + await callBinding(app, 'queue.Queue.Clear').catch(() => { + /* an empty queue is the state we were asking for */ + }); + await app.setViewportSize(DESKTOP); + }); + + test('a queue row is plain text on a phone and carries the destination', async ({ + app, + }) => { + await app.setViewportSize(DEVICE); + await queueThree(app); + await openTheQueue(app); + await expect(row(app, 0)).toBeVisible(); + + // The name is text: nothing in the row is a link at all. + await expect(app.locator('queue-panel .track-item .explore-link')).toHaveCount( + 0, + ); + + const labels = await openRowMenu(app, 0); + + expect(labels).toContain('Go to Artist'); + expect(labels).toContain('Go to Album'); + }); + + test('the same row on a desktop is a link, and its menu is untouched', async ({ + app, + }) => { + await app.setViewportSize(DESKTOP); + await queueThree(app); + await openTheQueue(app); + await expect(row(app, 0)).toBeVisible(); + + await expect( + row(app, 0).locator('.track-title .explore-link'), + ).toHaveCount(1); + + const labels = await openRowMenu(app, 0); + + expect(labels).not.toContain('Go to Artist'); + expect(labels).not.toContain('Go to Album'); + }); +}); diff --git a/frontend/src/components/cover-grid/cover-grid.ts b/frontend/src/components/cover-grid/cover-grid.ts index c4580c9..af3e986 100644 --- a/frontend/src/components/cover-grid/cover-grid.ts +++ b/frontend/src/components/cover-grid/cover-grid.ts @@ -55,6 +55,8 @@ import { import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { creditLink, exploreLinkStyles } from '../../utils/explore-link'; +import { goToMenuItems } from '../../utils/go-to-menu'; +import type { GoToTarget } from '../../utils/go-to-menu'; import { creditStore } from '@store/credit-store'; import { createAlbumArtDragImage, @@ -2084,6 +2086,30 @@ export class CoverGrid ); } + /** + * The artist an album card's menu can navigate to — the card's own + * credit line, which stops being a link below the phone breakpoint + * (#67). + * + * A *track* target gets nothing: the dropdown's rows carry no + * links of their own, and the album they sit under is the card + * that opened them. + */ + private get goToTarget(): GoToTarget | undefined { + if (this.contextMenuTarget.kind !== 'album') return undefined; + + const album = this.albums.find( + (a) => a.ID === this.contextMenuAlbumId, + ); + + if (!album) return undefined; + + return { + artistName: album.ArtistName, + artistMBID: album.ArtistMBID, + }; + } + private renderContextMenu() { const { ctxMenu } = this; @@ -2199,6 +2225,11 @@ export class CoverGrid ` : nothing} + ${goToMenuItems(this.goToTarget, { + onSelect: () => ctxMenu.close(), + onHover: () => + ctxMenu.closePlaylistSubmenu(), + })} ` : nothing} diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index 15bd242..f534faf 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -3191,10 +3191,14 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { return html` ${artist ? html`
+ ${creditLink( creditStore.credits(this.releaseGroupMBID), artist, artistMbid, + { keepOnPhone: true }, )}
` : nothing} diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index b750f67..f0c7c83 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -30,6 +30,7 @@ import { libraryStore } from '../../store/library-store'; import { downloadStore } from '../../store/download-store'; import '@awesome.me/webawesome/dist/components/button/button.js'; import { trackLink, exploreLinkStyles } from '../../utils/explore-link'; +import { goToMenuItems } from '../../utils/go-to-menu'; import { describeError } from '../../utils/describe-error'; import { GetAlbumsByArtist, @@ -2705,6 +2706,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost View on MusicBrainz + + ${goToMenuItems( + { albumName: track.releaseName, albumMBID: track.releaseGroupMbid ?? '' }, + { + onSelect: () => this.ctxMenu.close(), + onHover: () => this.ctxMenu.closePlaylistSubmenu(), + }, + )} `; } diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index dace829..4c73434 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -23,6 +23,8 @@ import { queueStore } from '../../store/queue-store'; import { notificationStore } from '../../store/notification-store'; import '../notifications/inline-notice'; import { creditLink, trackLink, exploreLinkStyles } from '../../utils/explore-link'; +import { goToMenuItems } from '../../utils/go-to-menu'; +import type { GoToTarget } from '../../utils/go-to-menu'; import { creditStore } from '@store/credit-store'; import { describeError } from '../../utils/describe-error'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; @@ -54,10 +56,28 @@ export const ExploreRegion = 'explore'; * is present only when owned — that's what gates the playback items, * while `mbid` (always present) is what "View on MusicBrainz" uses, so * a catalog-only card still gets a menu with somewhere useful to go. + * + * `goTo` is the names the card draws -- an artist credit, and for a + * recording row the release its title links to. Below the phone + * breakpoint those are plain text, so the menu is where they went + * (#67); an album card carries no album of its own, because tapping + * the card is already that. */ type ExploreMenuTarget = - | { kind: 'album'; mbid: string; localId?: number; title: string } - | { kind: 'recording'; mbid: string; localId?: number; title: string }; + | { + kind: 'album'; + mbid: string; + localId?: number; + title: string; + goTo?: GoToTarget; + } + | { + kind: 'recording'; + mbid: string; + localId?: number; + title: string; + goTo?: GoToTarget; + }; type ThumbnailRequest = explore.ThumbnailRequest; type MBSearchResult = explore.MBSearchResult; type LyricsResult = explore.LyricsResult; @@ -1386,6 +1406,9 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte View on MusicBrainz + ${goToMenuItems(target.goTo, { + onSelect: () => this.ctxMenu.close(), + })} ` : nothing} @@ -2188,6 +2211,10 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte mbid: rg.mbid, localId: rg.localId, title: rg.title, + goTo: { + artistName: rg.artistCredit, + artistMBID: rg.artistMbid ?? '', + }, })} role="button" tabindex="0" @@ -2200,6 +2227,10 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte mbid: rg.mbid, localId: rg.localId, title: rg.title, + goTo: { + artistName: rg.artistCredit, + artistMBID: rg.artistMbid ?? '', + }, }, )} > @@ -2278,6 +2309,12 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte mbid: r.mbid, localId: r.localId, title: r.title, + goTo: { + artistName: r.artistCredit, + artistMBID: r.artistMbid ?? '', + albumName: r.releaseName ?? '', + albumMBID: r.releaseGroupMbid ?? '', + }, })} @keydown=${(e: KeyboardEvent) => this.onCardKeydown( @@ -2288,6 +2325,12 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte mbid: r.mbid, localId: r.localId, title: r.title, + goTo: { + artistName: r.artistCredit, + artistMBID: r.artistMbid ?? '', + albumName: r.releaseName ?? '', + albumMBID: r.releaseGroupMbid ?? '', + }, }, )} > 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 831b3c8..f40a61a 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -420,11 +420,20 @@ export class NowPlayingView extends LitElement {

${track.title || track.fileName}

+

${creditLink( creditStore.credits(track.recordingMbid), track.artist, track.artistMbid, + { keepOnPhone: true }, )}

${track.album @@ -434,6 +443,7 @@ export class NowPlayingView extends LitElement { track.releaseGroupMbid, undefined, track.artist, + { keepOnPhone: true }, )}

` : nothing} diff --git a/frontend/src/components/playlist-details/playlist-details.ts b/frontend/src/components/playlist-details/playlist-details.ts index e45e499..7f32a9e 100644 --- a/frontend/src/components/playlist-details/playlist-details.ts +++ b/frontend/src/components/playlist-details/playlist-details.ts @@ -74,6 +74,8 @@ import { trackLink, exploreLinkStyles, } from '@utils/explore-link'; +import { goToMenuItems } from '@utils/go-to-menu'; +import type { GoToTarget } from '@utils/go-to-menu'; import { designTokens } from '../../styles/tokens.css'; import { srOnly } from '../../styles/sr-only.css'; import { backButton } from '../../styles/back-button.css'; @@ -589,6 +591,28 @@ export class PlaylistDetails .map((i) => this.tracks[i]!.FilePath); } + /** + * The row "Go to Artist" / "Go to Album" navigate from — one row + * or none, and only below the phone breakpoint, where the row's + * own names stopped being links (#67). + */ + private get goToTarget(): GoToTarget | undefined { + const indices = this.selection.getSelectedIndices(); + + if (indices.length !== 1) return undefined; + + const track = this.tracks[indices[0]!]; + + if (!track) return undefined; + + return { + artistName: track.Artist, + artistMBID: track.ArtistMBID, + albumName: track.Album, + albumMBID: track.ReleaseGroupMBID, + }; + } + // ================================================================= // Context menu actions // ================================================================= @@ -1900,6 +1924,14 @@ export class PlaylistDetails Track Details + ${goToMenuItems(this.goToTarget, { + onSelect: () => { + this.selection.clear(); + this.ctxMenu.close(); + }, + onHover: () => + this.ctxMenu.closePlaylistSubmenu(), + })} ` : nothing} diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index ee65f16..016151f 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -62,6 +62,8 @@ import { trackLink, exploreLinkStyles, } from '@utils/explore-link'; +import { goToMenuItems } from '@utils/go-to-menu'; +import type { GoToTarget } from '@utils/go-to-menu'; import { ICON_NEW, ICON_PLAY, @@ -1624,6 +1626,29 @@ export class QueuePanel .map((i) => tracks[i]!.filePath); } + /** + * The row "Go to Artist" / "Go to Album" navigate from, which is + * one row or none — the rule the Play item already follows. Both + * items are drawn only below the phone breakpoint, where the row's + * own names stopped being links (#67). + */ + private get goToTarget(): GoToTarget | undefined { + const indices = this.selection.getSelectedIndices(); + + if (indices.length !== 1) return undefined; + + const track = this.queue.tracks[indices[0]!]; + + if (!track) return undefined; + + return { + artistName: track.artist, + artistMBID: track.artistMbid, + albumName: track.album, + albumMBID: track.releaseGroupMbid, + }; + } + // ================================================================= // Drop target (tracks dropped into queue) // ================================================================= @@ -2341,6 +2366,14 @@ export class QueuePanel Track Details + ${goToMenuItems(this.goToTarget, { + onSelect: () => { + this.selection.clear(); + this.ctxMenu.close(); + }, + onHover: () => + this.ctxMenu.closePlaylistSubmenu(), + })} ` : nothing} diff --git a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts index edde4c7..e918db1 100644 --- a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts +++ b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts @@ -64,6 +64,8 @@ import { trackLink, exploreLinkStyles, } from '@utils/explore-link'; +import { goToMenuItems } from '@utils/go-to-menu'; +import type { GoToTarget } from '@utils/go-to-menu'; import '@components/smart-playlist-editor/smart-playlist-editor.js'; import { designTokens } from '../../styles/tokens.css'; import { backButton } from '../../styles/back-button.css'; @@ -928,6 +930,28 @@ export class SmartPlaylistDetails .map((i) => this.tracks[i]!.FilePath); } + /** + * The row "Go to Artist" / "Go to Album" navigate from — one row + * or none, and only below the phone breakpoint, where the row's + * own names stopped being links (#67). + */ + private get goToTarget(): GoToTarget | undefined { + const indices = this.selection.getSelectedIndices(); + + if (indices.length !== 1) return undefined; + + const track = this.tracks[indices[0]!]; + + if (!track) return undefined; + + return { + artistName: track.Artist, + artistMBID: track.ArtistMBID, + albumName: track.Album, + albumMBID: track.ReleaseGroupMBID, + }; + } + // ================================================================= // Context menu actions // ================================================================= @@ -1683,6 +1707,14 @@ export class SmartPlaylistDetails > Track Details + ${goToMenuItems(this.goToTarget, { + onSelect: () => { + this.selection.clear(); + this.ctxMenu.close(); + }, + onHover: () => + this.ctxMenu.closePlaylistSubmenu(), + })} ` : nothing} diff --git a/frontend/src/components/top-results-row/top-results-row.ts b/frontend/src/components/top-results-row/top-results-row.ts index f25e6ad..8f4cdd8 100644 --- a/frontend/src/components/top-results-row/top-results-row.ts +++ b/frontend/src/components/top-results-row/top-results-row.ts @@ -371,8 +371,11 @@ export class TopResultsRow extends LitElement { ${r.name} ${artistPart || metaPart ? html`${artistPart - ? creditLink(creditStore.credits(r.mbid), artistPart, r.artistMbid ?? '') + >${artistPart + ? creditLink(creditStore.credits(r.mbid), artistPart, r.artistMbid ?? '', { keepOnPhone: true }) : nothing}${artistPart && metaPart ? ' · ' : ''}${metaPart} Track Details + ${goToMenuItems(this.goToTarget, { + onSelect: () => { + this.selection.clear(); + this.ctxMenu.close(); + }, + onHover: () => this.ctxMenu.closePlaylistSubmenu(), + })} this.onContextMenuAction( diff --git a/frontend/src/utils/explore-link.ts b/frontend/src/utils/explore-link.ts index 7450d79..a10871a 100644 --- a/frontend/src/utils/explore-link.ts +++ b/frontend/src/utils/explore-link.ts @@ -13,11 +13,35 @@ * bug, not as a statement about metadata. The only case that still * renders as text is one we genuinely cannot route (no name at all, or * nothing in the library by that name). + * + * ## Below the phone breakpoint a name is not a link (#67) + * + * A few characters of text inside a row is not a touch target, and the + * click handling below is explicitly a *desktop* compromise: the + * navigation is held for one double-click interval so double-clicking + * the row can still play it, which means nothing at all on touch. On + * a phone the row's own gesture wins anyway — a claimed `yj-tap` has + * its click swallowed by `utils/touch-gestures.ts`, so the link was + * unreachable as well as fiddly. + * + * So the rule lives here rather than at twenty call sites, which is + * what the Findings on #67 ask for: a name renders as plain text below + * `PHONE_QUERY`, and the row's context menu carries "Go to Artist" / + * "Go to Album" in its place (`goToMenuItems`). + * + * The exception is `keepOnPhone`, and it is not a preference. Three + * surfaces render a name with **no menu to carry the destination** — + * `now-playing-view`, `explore-album-details`' header credit and + * `top-results-row` — so suppressing the link there takes the action + * away entirely rather than moving it, which is what plan 018's "no + * action is unreachable at any supported size" refuses. Each of those + * call sites says so. */ import { html, css } from 'lit'; import type { TemplateResult } from 'lit'; import { libraryStore } from '../store/library-store'; +import { PHONE_QUERY } from './breakpoints'; /** Shared CSS for explore link styling. Import into component styles. */ export const exploreLinkStyles = css` @@ -32,6 +56,57 @@ export const exploreLinkStyles = css` } `; +/** + * Options every link function takes, for the one case that is not the + * default. + */ +export interface LinkOptions { + /** + * Keep the name navigable at phone width. + * + * For a surface with no context menu to carry the destination — + * see the header of this file. A row must not pass it: the row's + * tap already means "play", and the menu is where the destination + * went. + */ + keepOnPhone?: boolean; +} + +/** + * The live phone breakpoint, made once and read per link. + * + * A `MediaQueryList` is live, so one object answers for the life of + * the page and a resize needs nothing from here. The identity check + * is the test seam: this tier's viewport is fixed by the runner, so a + * spec answers the query by replacing `window.matchMedia` (the same + * stub `now-playing-phone.test.ts` installs), and swapping the + * function is what tells us to ask again. + */ +let phoneQuery: MediaQueryList | undefined; +let phoneQuerySource: typeof window.matchMedia | undefined; + +/** + * Whether an inline name still navigates. + * + * Exported because the menus that carry the destination in its place + * are drawn under exactly the same condition -- one answer, not two. + */ +export function inlineLinksSuppressed(): boolean { + if (!window.matchMedia) return false; + + if (phoneQuerySource !== window.matchMedia) { + phoneQuerySource = window.matchMedia; + phoneQuery = window.matchMedia(PHONE_QUERY); + } + + return phoneQuery?.matches ?? false; +} + +/** Whether this call site should render plain text rather than a link. */ +function plainText(options?: LinkOptions): boolean { + return !options?.keepOnPhone && inlineLinksSuppressed(); +} + /** Fire a navigate event from the clicked element. */ function navigate(target: EventTarget, detail: Record): void { target.dispatchEvent( @@ -153,39 +228,19 @@ function singleClick( * @param mbid - The MusicBrainz artist ID. Empty string = local only. * @param content - Optional custom content to render inside the link * (e.g. highlighted search result). Defaults to artistName. + * @param options - See `LinkOptions`. */ export function artistLink( artistName: string, mbid: string, content?: TemplateResult | string, + options?: LinkOptions, ): TemplateResult | string { if (!artistName) return artistName; + if (plainText(options)) return content ?? artistName; const onClick = singleClick((target) => { - void (async () => { - if (mbid) { - navigate(target, { - view: 'explore-artist-details', - artistMBID: mbid, - artistName, - }); - - return; - } - - const local = await findLocalArtist(artistName); - if (!local) return; - - // The caller's row had no MBID, but the library row for the - // same artist may — the grid routes by exactly this field, - // so reading it here is what keeps the two paths agreeing. - navigate(target, { - view: 'explore-artist-details', - artistMBID: local.MBID || '', - artistName, - localArtistId: local.ID, - }); - })(); + void openArtistPage(target, artistName, mbid); }); return html` { - void openAlbum(target, albumName, mbid, artistName); + void openAlbumPage(target, albumName, mbid, artistName); })} title=${mbid ? 'View album on Explore' : 'View album in your library'} >${content ?? albumName}`; @@ -232,6 +290,7 @@ export function albumLink( * @param recordingMBID - The track's MusicBrainz recording ID. * @param content - Optional custom content (e.g. highlighted text). * @param artistName - Disambiguates same-named albums in the library. + * @param options - See `LinkOptions`. */ export function trackLink( trackName: string, @@ -240,14 +299,16 @@ export function trackLink( recordingMBID: string, content?: TemplateResult | string, artistName?: string, + options?: LinkOptions, ): TemplateResult | string { if (!trackName) return trackName; if (!albumName) return content ?? trackName; + if (plainText(options)) return content ?? trackName; return html` { - void openAlbum( + void openAlbumPage( target, albumName, releaseGroupMBID, @@ -262,11 +323,48 @@ export function trackLink( >${content ?? trackName}`; } +/** + * Route to an artist page, preferring the catalog and falling back to + * the library copy. + * + * Exported because a menu item goes to the same place a name does, and + * two routings of "go to this artist" is how the two come to disagree + * about an untagged one. + */ +export async function openArtistPage( + target: EventTarget, + artistName: string, + mbid: string, +): Promise { + if (mbid) { + navigate(target, { + view: 'explore-artist-details', + artistMBID: mbid, + artistName, + }); + + return; + } + + const local = await findLocalArtist(artistName); + if (!local) return; + + // The caller's row had no MBID, but the library row for the + // same artist may — the grid routes by exactly this field, + // so reading it here is what keeps the two paths agreeing. + navigate(target, { + view: 'explore-artist-details', + artistMBID: local.MBID || '', + artistName, + localArtistId: local.ID, + }); +} + /** * Route to an album page, preferring the catalog and falling back to * the library copy. `highlight*` marks one track on arrival. */ -async function openAlbum( +export async function openAlbumPage( target: EventTarget, albumName: string, releaseGroupMBID: string, @@ -337,23 +435,31 @@ export interface CreditPart { * @param parts - The credit's parts in position order, if known. * @param fallbackName - The credit as a single string. * @param fallbackMbid - The primary artist's MBID. + * @param options - See `LinkOptions`. */ export function creditLink( parts: readonly CreditPart[] | undefined, fallbackName: string, fallbackMbid: string, + options?: LinkOptions, ): TemplateResult | string { // One part is one link, so it is the fallback rather than a special // case — and a zero-part credit reaching here would otherwise // render as nothing at all, which is worse than the single-artist // answer it replaced. if (!parts || parts.length < 2) { - return artistLink(fallbackName, fallbackMbid); + return artistLink(fallbackName, fallbackMbid, undefined, options); } + // A decomposed credit is rendered from the same parts either way, + // so the join phrases survive the suppression and the text reads + // as it did — which is `creditText`'s job, and it is the string + // the `title=` beside these already uses. + if (plainText(options)) return creditText(parts, fallbackName); + return html`${parts.map( (part) => - html`${artistLink(part.creditedName, part.artistMbid)}${part.joinPhrase}`, + html`${artistLink(part.creditedName, part.artistMbid, undefined, options)}${part.joinPhrase}`, )}`; } diff --git a/frontend/src/utils/go-to-menu.ts b/frontend/src/utils/go-to-menu.ts new file mode 100644 index 0000000..579ef6d --- /dev/null +++ b/frontend/src/utils/go-to-menu.ts @@ -0,0 +1,109 @@ +/** + * "Go to Artist" / "Go to Album", for the menus that carry a name the + * phone stopped drawing as a link (#67). + * + * `utils/explore-link.ts` renders a plain string below the phone + * breakpoint, because a few characters inside a row is not a touch + * target and the row's own tap already means "play". That takes a + * destination away, so the row's context menu gives it back — which is + * the whole of this issue: the navigation moves, it does not go. + * + * Three things about it are load-bearing. + * + * **It is drawn under exactly the condition the link is not.** + * `inlineLinksSuppressed()` answers both, so a desktop menu is + * untouched (the name beside it is still a link, and a menu that + * repeats what the row already offers is furniture) and a phone menu + * cannot be missing what the row lost. + * + * **It goes where the name went.** `openArtistPage` / `openAlbumPage` + * are `explore-link`'s own routing, exported rather than reimplemented, + * so an untagged artist reaches the library page here for the same + * reason and by the same lookup it does from a link. + * + * **The host says when it is over**, through `onSelect` — every menu in + * this app closes itself and most clear their selection, and both are + * the host's bookkeeping rather than something a shared item may do on + * its behalf. `onHover` is for the four hosts with a playlist submenu, + * which closes on any other item being pointed at. + */ + +import { html, nothing } from 'lit'; +import type { TemplateResult } from 'lit'; + +import { inlineLinksSuppressed, openArtistPage, openAlbumPage } from './explore-link'; + +/** + * The entities one row or card can send you to. + * + * Everything is optional because the hosts differ: a track row knows + * both, an album card knows only its artist, and an artist page's own + * tracklist knows only the album. + */ +export interface GoToTarget { + artistName?: string; + artistMBID?: string; + albumName?: string; + albumMBID?: string; +} + +export interface GoToHandlers { + /** Called before navigating: close the menu, clear the selection. */ + onSelect?: () => void; + /** Called on hover: close a playlist submenu, where the host has one. */ + onHover?: () => void; +} + +/** + * The menu items for a target, or nothing at all where the name beside + * them is still a link. + */ +export function goToMenuItems( + target: GoToTarget | undefined, + handlers: GoToHandlers = {}, +): TemplateResult | typeof nothing { + if (!target || !inlineLinksSuppressed()) return nothing; + + const artist = target.artistName?.trim(); + const album = target.albumName?.trim(); + + if (!artist && !album) return nothing; + + return html` + ${artist + ? html` { + handlers.onSelect?.(); + void openArtistPage( + e.currentTarget as EventTarget, + artist, + target.artistMBID ?? '', + ); + }} + @mouseenter=${() => handlers.onHover?.()} + > + + Go to Artist + ` + : nothing} + ${album + ? html` { + handlers.onSelect?.(); + void openAlbumPage( + e.currentTarget as EventTarget, + album, + target.albumMBID ?? '', + artist, + ); + }} + @mouseenter=${() => handlers.onHover?.()} + > + + Go to Album + ` + : nothing} + `; +} diff --git a/frontend/test/components/phone-entity-links.test.ts b/frontend/test/components/phone-entity-links.test.ts new file mode 100644 index 0000000..12c6686 --- /dev/null +++ b/frontend/test/components/phone-entity-links.test.ts @@ -0,0 +1,275 @@ +/** + * A name is not a link on a phone, and the menu is where it went (#67). + * + * `utils/explore-link.ts` makes every track, album and artist name + * navigable, with click handling that is explicitly a desktop + * compromise — the navigation is held for one double-click interval so + * double-clicking the row can still play it. On touch that is a delay + * on an ambiguous target, and since #63 the row's own tap claims the + * click anyway, so the link was unreachable as well as fiddly. + * + * So below the phone breakpoint a name renders as plain text and the + * row's context menu carries "Go to Artist" / "Go to Album" instead. + * The two halves are asserted together on purpose: a suppressed link + * with no menu item behind it is not a smaller affordance, it is a + * destination that cannot be reached, which is what plan 018 promises + * against. + * + * The breakpoint is stubbed rather than emulated for the reason + * `now-playing-phone.test.ts` states: this tier's viewport is fixed at + * 1280x800 by the runner, and `matchMedia` is the seam. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; +import { html, render } from 'lit'; +import type { LitElement } from 'lit'; + +import '@components/playlist-details/playlist-details'; +import { + albumLink, + artistLink, + creditLink, + trackLink, +} from '@utils/explore-link'; +import { goToMenuItems } from '@utils/go-to-menu'; +import { stub, flush, resetHarness } from '@test/support/harness'; +import { fixture, shadowAll } from '@test/support/render'; + +/** Answer the phone breakpoint, and hand back the undo. */ +function atPhone(phone: boolean): () => void { + const real = window.matchMedia.bind(window); + + window.matchMedia = ((q: string) => + q.includes('max-width: 599px') + ? { + matches: phone, + media: q, + addEventListener() {}, + removeEventListener() {}, + } + : real(q)) as typeof window.matchMedia; + + return () => { + window.matchMedia = real as typeof window.matchMedia; + }; +} + +/** Render a template into a detached container and hand it back. */ +function draw(template: unknown): HTMLElement { + const host = document.createElement('div'); + + document.body.append(host); + render(html`${template}`, host); + + return host; +} + +describe('an inline name below the phone breakpoint', () => { + let restore: () => void = () => {}; + + afterEach(() => { + restore(); + document.querySelectorAll('body > div').forEach((el) => el.remove()); + }); + + it('is a link on a desktop', () => { + restore = atPhone(false); + + const host = draw(artistLink('Cocteau Twins', 'artist-mbid')); + + expect(host.querySelector('a.explore-link')).not.toBeNull(); + expect(host.textContent?.trim()).toBe('Cocteau Twins'); + }); + + it('is plain text on a phone, for all four shapes', () => { + restore = atPhone(true); + + const host = draw(html` + ${artistLink('Cocteau Twins', 'artist-mbid')} + ${albumLink('Heaven or Las Vegas', 'rg-mbid')} + ${trackLink('Iceblink Luck', 'Heaven or Las Vegas', 'rg-mbid', 'rec-mbid')} + ${creditLink( + [ + { + creditedName: 'Skrillex', + artistMbid: 'a1', + joinPhrase: ' feat. ', + }, + { creditedName: 'Swae Lee', artistMbid: 'a2', joinPhrase: '' }, + ], + 'Skrillex & Swae Lee', + 'a1', + )} + `); + + expect(host.querySelectorAll('a.explore-link')).toHaveLength(0); + + // The words survive, join phrases included — a decomposed credit is + // still assembled from its parts, so the text does not change with + // the affordance. + expect(host.textContent).toContain('Cocteau Twins'); + expect(host.textContent).toContain('Heaven or Las Vegas'); + expect(host.textContent).toContain('Iceblink Luck'); + expect(host.textContent).toContain('Skrillex feat. Swae Lee'); + }); + + it('stays a link where the caller has no menu to carry it', () => { + restore = atPhone(true); + + const host = draw( + albumLink('Heaven or Las Vegas', 'rg-mbid', undefined, 'Cocteau Twins', { + keepOnPhone: true, + }), + ); + + expect(host.querySelector('a.explore-link')).not.toBeNull(); + }); +}); + +describe('the "Go to" menu items', () => { + let restore: () => void = () => {}; + + afterEach(() => { + restore(); + document.querySelectorAll('body > div').forEach((el) => el.remove()); + }); + + it('are absent on a desktop, where the name beside them is a link', () => { + restore = atPhone(false); + + const host = draw( + goToMenuItems({ artistName: 'Cocteau Twins', albumName: 'Treasure' }), + ); + + expect(host.querySelectorAll('wa-dropdown-item')).toHaveLength(0); + }); + + it('offer only what the target knows', () => { + restore = atPhone(true); + + const both = draw( + goToMenuItems({ artistName: 'Cocteau Twins', albumName: 'Treasure' }), + ); + const artistOnly = draw(goToMenuItems({ artistName: 'Cocteau Twins' })); + const neither = draw(goToMenuItems({})); + + expect(both.querySelectorAll('wa-dropdown-item')).toHaveLength(2); + expect(artistOnly.querySelectorAll('wa-dropdown-item')).toHaveLength(1); + expect(neither.querySelectorAll('wa-dropdown-item')).toHaveLength(0); + }); +}); + +// ===================================================================== +// The menu that carries the destination +// ===================================================================== + +function playlistTracks(n: number) { + return Array.from({ length: n }, (_, i) => ({ + ID: i + 1, + FilePath: `/music/track-${i}.mp3`, + Title: `Track ${i}`, + Artist: 'Cocteau Twins', + ArtistMBID: 'artist-mbid', + Album: 'Heaven or Las Vegas', + ReleaseGroupMBID: 'rg-mbid', + Duration: 180000, + Phantom: false, + })); +} + +describe('a playlist row’s context menu on a phone', () => { + let el: LitElement; + let restore: () => void = () => {}; + + beforeEach(async () => { + resetHarness(); + restore = atPhone(true); + stub('playlist.Service.GetPlaylistTracks', playlistTracks(8)); + stub('playlist.Service.GetAllPlaylists', []); + + el = await fixture('playlist-details', { + playlistId: 1, + playlistName: 'A playlist', + }); + el.style.display = 'block'; + el.style.height = '600px'; + await flush(); + await el.updateComplete; + await new Promise((r) => setTimeout(r, 60)); + }); + + afterEach(() => { + restore(); + }); + + /** Right-click a row and hand back the menu's items. */ + async function openMenu(index: number): Promise { + const row = shadowAll(el, '.track-item').find( + (r) => r.getAttribute('data-index') === String(index), + ); + + row!.dispatchEvent( + new MouseEvent('contextmenu', { bubbles: true, composed: true }), + ); + await el.updateComplete; + + return shadowAll(el, 'wa-dropdown-item'); + } + + it('carries the artist and the album the row stopped linking to', async () => { + const labels = (await openMenu(3)).map((i) => i.textContent?.trim()); + + expect(labels).toContain('Go to Artist'); + expect(labels).toContain('Go to Album'); + }); + + it('navigates where the name would have', async () => { + const seen: CustomEvent[] = []; + const listen = (e: Event) => seen.push(e as CustomEvent); + + document.addEventListener('navigate', listen); + + try { + const items = await openMenu(3); + + items + .find((i) => i.textContent?.trim() === 'Go to Artist')! + .click(); + await flush(); + } finally { + document.removeEventListener('navigate', listen); + } + + expect(seen.map((e) => e.detail)).toEqual([ + { + view: 'explore-artist-details', + artistMBID: 'artist-mbid', + artistName: 'Cocteau Twins', + }, + ]); + }); + + it('is absent while several rows are selected', async () => { + // "Go to the album" of five different albums means nothing, which + // is the rule the Play item already follows: one row is a + // position, several are an explicit choice of those tracks. + const rows = shadowAll(el, '.track-item'); + const click = (i: number, modifiers: MouseEventInit) => + rows + .find((r) => r.getAttribute('data-index') === String(i))! + .dispatchEvent( + new MouseEvent('click', { + bubbles: true, + composed: true, + ...modifiers, + }), + ); + + click(1, {}); + click(4, { ctrlKey: true }); + await el.updateComplete; + + const labels = (await openMenu(4)).map((i) => i.textContent?.trim()); + + expect(labels).not.toContain('Go to Artist'); + }); +});