diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index 4e141d47..64717012 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -2094,6 +2094,153 @@ test("puts the songs playback action beside the sort and names its scope", async await expect(page.locator(".player-track strong")).toHaveText("Army of Me"); }); +/** + * Every credited artist in a song list is a link to its own page. + * + * The column used to be the display string as plain text, so a playlist or a + * mix offered no way to reach an artist. A track credited to two artists names + * two pages; a track whose credits the server did not list still links through + * its primary credit; an artist the catalogue does not know stays text. + */ +test("links each credited artist in a song list to its page", async ({ + page, +}, testInfo) => { + await page.route("**/api/v2/songs?*", (route) => + route.fulfill({ + json: [ + { + ...track, + id: "song-duo", + title: "Soleil Bleu", + artist: "Bleu Soleil, LUIZA", + artist_id: "artist-1", + artists: [ + { id: "artist-1", name: "Bleu Soleil" }, + { id: "artist-2", name: "LUIZA" }, + ], + }, + { + ...track, + id: "song-primary", + title: "Second track", + artist: "Björk", + artist_id: "artist-1", + artists: [], + }, + { + ...track, + id: "song-unknown", + title: "Third track", + artist: "Nobody We Know", + artist_id: null, + artists: [], + }, + ], + }), + ); + await page.goto("/songs"); + const rows = page.locator(".songs tbody tr"); + await expect(rows).toHaveCount(3); + + // Every layout: the track's menu names each credited artist, which is how a + // phone, whose song table hides the artist column, reaches one. + await rows.nth(0).locator("button.song-title").focus(); + await page.keyboard.press("Shift+F10"); + const menu = page.getByRole("menu", { name: "Actions for Soleil Bleu" }); + await expect(menu.getByRole("menuitem", { name: /^Go to / })).toHaveText([ + "Go to album", + "Go to Bleu Soleil", + "Go to LUIZA", + ]); + await menu.getByRole("menuitem", { name: "Go to LUIZA" }).click(); + await expect(page).toHaveURL(/\/artists\/artist-2$/); + await page.goBack(); + await expect(rows).toHaveCount(3); + + // The column itself is drawn on a wide screen only. + if (testInfo.project.name !== "desktop") return; + const duo = rows.nth(0).locator(".song-artists"); + await expect(duo.getByRole("link")).toHaveCount(2); + await expect(duo).toHaveText("Bleu Soleil, LUIZA"); + await expect(duo.getByRole("link", { name: "LUIZA" })).toHaveAttribute( + "href", + "/artists/artist-2", + ); + + await expect( + rows.nth(1).locator(".song-artists").getByRole("link", { name: "Björk" }), + ).toHaveAttribute("href", "/artists/artist-1"); + await expect(rows.nth(2).locator(".song-artists").getByRole("link")).toHaveCount( + 0, + ); + await expect(rows.nth(2).locator(".song-artists")).toHaveText("Nobody We Know"); + + await expectNoWcagViolations(page); + + await duo.getByRole("link", { name: "Bleu Soleil" }).click(); + await expect(page).toHaveURL(/\/artists\/artist-1$/); +}); + +/** + * A track menu taller than the window stays on screen and scrolls itself. + * + * One "Go to" entry per credited artist means a compilation track can carry a + * menu longer than a short window. It is capped at the window and scrolls on + * its own; the wheel over it scrolls it rather than closing it, and the wheel + * over the page still closes it. + */ +test("keeps a long track menu on screen and lets it scroll", async ({ + page, +}) => { + await page.setViewportSize({ width: 900, height: 420 }); + const many = Array.from({ length: 30 }, (_, index) => ({ + id: `artist-${index + 1}`, + name: `Guest ${index + 1}`, + })); + await page.route("**/api/v2/songs?*", (route) => + route.fulfill({ + json: [ + { + ...track, + id: "song-crowd", + title: "Crowded", + artist: many.map((artist) => artist.name).join(", "), + artist_id: "artist-1", + artists: many, + }, + ], + }), + ); + await page.goto("/songs"); + await page.locator(".songs tbody tr button.song-title").first().focus(); + await page.keyboard.press("Shift+F10"); + const menu = page.getByRole("menu", { name: "Actions for Crowded" }); + await expect(menu).toBeVisible(); + + const box = await menu.boundingBox(); + expect(box).not.toBeNull(); + expect((box?.y ?? 0) + (box?.height ?? 0)).toBeLessThanOrEqual(420); + expect(await menu.evaluate((node) => node.scrollHeight > node.clientHeight)).toBe( + true, + ); + + // The wheel over the menu scrolls it and leaves it open. + await menu.hover(); + await page.mouse.wheel(0, 400); + await expect + .poll(() => menu.evaluate((node) => node.scrollTop)) + .toBeGreaterThan(0); + await expect(menu).toBeVisible(); + const last = menu.getByRole("menuitem", { name: "Go to Guest 30" }); + await last.scrollIntoViewIfNeeded(); + await expect(last).toBeInViewport(); + + // The wheel anywhere else still closes it. + await page.mouse.move(880, 400); + await page.mouse.wheel(0, 100); + await expect(menu).toHaveCount(0); +}); + /** * The right click on a track, and the properties it opens (RFC-012 decision 4). * @@ -2141,6 +2288,9 @@ test("opens a track's properties from its context menu", async ({ page }) => { "Download", // RFC-022: this browser supports Cache Storage, so it offers the copy. "Keep for offline", + // Where the track belongs, reachable from any list and on a phone. + "Go to album", + "Go to Skunk Anansie", "Properties", "Edit tags", "Move to folder…", @@ -2151,11 +2301,11 @@ test("opens a track's properties from its context menu", async ({ page }) => { "outline-style", "solid", ); - // Past "Keep for offline" (RFC-022), the fourth item. - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); + // Past "Keep for offline" (RFC-022) and the album and artist entries, the + // seventh item. + for (let press = 0; press < 6; press += 1) { + await page.keyboard.press("ArrowDown"); + } await expect(menu.getByRole("menuitem", { name: "Properties" })).toBeFocused(); await page.keyboard.press("Enter"); @@ -2182,10 +2332,9 @@ test("opens a track's properties from its context menu", async ({ page }) => { // The backdrop, outside the panel's box, does close it. await page.keyboard.press("Shift+F10"); - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); - await page.keyboard.press("ArrowDown"); + for (let press = 0; press < 6; press += 1) { + await page.keyboard.press("ArrowDown"); + } await page.keyboard.press("Enter"); await expect(panel).toBeVisible(); await page.mouse.click(2, 2); diff --git a/webapp/src/api/catalog.ts b/webapp/src/api/catalog.ts index e50e75fd..51498825 100644 --- a/webapp/src/api/catalog.ts +++ b/webapp/src/api/catalog.ts @@ -26,6 +26,13 @@ export type Song = { album: string | null; artist: string | null; artist_id: string | null; + /** + * Every credited artist, in tag order: the structured form of `artist`, + * which stays the display string. On the wire since `SongItem` gained it; + * declared here so each name can link to its own page. Optional, so a mock + * that leaves it out is a track credited by its display string alone. + */ + artists?: Array<{ id: string; name: string }>; artwork_hash: string | null; duration_ms: number; track: number | null; diff --git a/webapp/src/components/song-table.tsx b/webapp/src/components/song-table.tsx index 2e729aa4..d89ac745 100644 --- a/webapp/src/components/song-table.tsx +++ b/webapp/src/components/song-table.tsx @@ -3,6 +3,7 @@ import { Link } from "@tanstack/react-router"; import { type ReactNode, useState } from "react"; import { formatDuration, type Song, setFavorite, setRating } from "../api"; +import { creditedArtists } from "../lib/artist-credits"; import { usePlayer } from "../player/player"; import { useI18n } from "../state/i18n"; import { mayCorrectTracks, useLibraryScope } from "../state/library-scope"; @@ -12,6 +13,31 @@ import { useTrackMenu } from "./track-menu"; const RATING_STARS = [1, 2, 3, 4, 5]; +/** + * A track's artists, each a link to its own page. + * + * A track credited to two artists ("Bleu Soleil, LUIZA") names two pages, so + * the structured list is what is linked, in tag order, rather than the display + * string. Where the list is empty, the primary credit still links if it has an + * id, and an artist the catalogue does not know stays plain text. + */ +function ArtistLinks({ song }: { song: Song }) { + const credited = creditedArtists(song); + if (credited.length === 0) return <>{song.artist}; + return ( + <> + {credited.map((artist, index) => ( + + {index > 0 ? ", " : null} + + {artist.name} + + + ))} + + ); +} + /** * What a dragged row carries: the track ids, as JSON. A type of its own, so a * folder takes a drop from a song row and nothing else. @@ -224,7 +250,9 @@ export function SongTable({ {variant === "default" ? ( - {song.artist} + + + ) : null} {formatDuration(song.duration_ms)} diff --git a/webapp/src/components/track-menu.tsx b/webapp/src/components/track-menu.tsx index 3d269d6e..c3b57844 100644 --- a/webapp/src/components/track-menu.tsx +++ b/webapp/src/components/track-menu.tsx @@ -12,6 +12,7 @@ import { import { createPortal } from "react-dom"; import { placeTracks, type Song, streamUrl } from "../api"; +import { creditedArtists } from "../lib/artist-credits"; import { downloadUrl, saveFrom } from "../lib/download"; import { forgetOffline, @@ -341,10 +342,17 @@ function TrackMenu({ const away = (event: Event) => { if (!list.current?.contains(event.target as Node)) closeRef.current(); }; - // The wheel closes it wherever it turns, over the menu too: the menu does - // not scroll, so a wheel over it scrolls the page beneath and leaves the - // menu where its row used to be. - const wheel = () => closeRef.current(); + // The wheel closes it, over the menu too, unless the menu has rows to + // scroll: a track credited to many artists makes it taller than a short + // window, and it is then capped and scrolls on its own. A menu with nothing + // to scroll would hand the wheel to the page beneath and leave the menu + // where its row used to be, so that one still closes. + const wheel = (event: Event) => { + const menu = list.current; + const inside = menu?.contains(event.target as Node) ?? false; + if (inside && menu && menu.scrollHeight > menu.clientHeight) return; + closeRef.current(); + }; const leave = () => closeRef.current(); // A long press opens the menu with the finger still down, and that finger // drifts as it lifts: its moves are the gesture that opened the menu, not @@ -429,6 +437,33 @@ function TrackMenu({ }, ] : []), + // Where the track belongs, from every list it appears in. The artist + // column of a song table is hidden on a narrow screen, so this is how a + // phone reaches an artist from a playlist or a mix. + ...(song.album_id + ? [ + { + key: "album", + icon: "albums" as const, + label: t("trackMenu.goToAlbum"), + run: () => + void navigate({ + to: "/albums/$albumId", + params: { albumId: song.album_id as string }, + }), + }, + ] + : []), + ...creditedArtists(song).map((artist) => ({ + key: `artist-${artist.id}`, + icon: "artists" as const, + label: t("trackMenu.goToArtist", { name: artist.name }), + run: () => + void navigate({ + to: "/artists/$artistId", + params: { artistId: artist.id }, + }), + })), { key: "properties", icon: "info", diff --git a/webapp/src/lib/artist-credits.test.ts b/webapp/src/lib/artist-credits.test.ts new file mode 100644 index 00000000..82adf1aa --- /dev/null +++ b/webapp/src/lib/artist-credits.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "vitest"; + +import { creditedArtists } from "./artist-credits"; + +describe("creditedArtists", () => { + it("uses the structured list, in tag order, when the server sends one", () => { + expect( + creditedArtists({ + artist: "Bleu Soleil, LUIZA", + artist_id: "artist-1", + artists: [ + { id: "artist-1", name: "Bleu Soleil" }, + { id: "artist-2", name: "LUIZA" }, + ], + }), + ).toEqual([ + { id: "artist-1", name: "Bleu Soleil" }, + { id: "artist-2", name: "LUIZA" }, + ]); + }); + + it("falls back to the primary credit when the list is missing or empty", () => { + const primary = [{ id: "artist-1", name: "Björk" }]; + expect(creditedArtists({ artist: "Björk", artist_id: "artist-1" })).toEqual( + primary, + ); + expect( + creditedArtists({ artist: "Björk", artist_id: "artist-1", artists: [] }), + ).toEqual(primary); + }); + + it("links nothing for an artist the catalogue does not know", () => { + expect( + creditedArtists({ + artist: "Nobody We Know", + artist_id: null, + artists: [], + }), + ).toEqual([]); + expect(creditedArtists({ artist: null, artist_id: null })).toEqual([]); + }); +}); diff --git a/webapp/src/lib/artist-credits.ts b/webapp/src/lib/artist-credits.ts new file mode 100644 index 00000000..6c8c8902 --- /dev/null +++ b/webapp/src/lib/artist-credits.ts @@ -0,0 +1,21 @@ +import type { Song } from "../api"; + +/** One artist a track can link to. */ +export type CreditedArtist = { id: string; name: string }; + +/** + * The artists a track can link to, in tag order. + * + * The structured list the server sends, where it has one. Otherwise the + * primary credit, if the catalogue knows it by an id, and nothing at all for + * an artist it does not know: a name with no page behind it is not a link. + */ +export function creditedArtists( + song: Pick, +): CreditedArtist[] { + if (song.artists && song.artists.length > 0) return song.artists; + if (song.artist_id && song.artist) { + return [{ id: song.artist_id, name: song.artist }]; + } + return []; +} diff --git a/webapp/src/state/i18n.tsx b/webapp/src/state/i18n.tsx index 4e43c976..04b1db8c 100644 --- a/webapp/src/state/i18n.tsx +++ b/webapp/src/state/i18n.tsx @@ -132,6 +132,8 @@ const en = { "trackMenu.forgetOffline": "Remove the offline copy", "trackMenu.downloadFailed": "The download could not start. Try again.", "trackMenu.dismiss": "Dismiss", + "trackMenu.goToAlbum": "Go to album", + "trackMenu.goToArtist": "Go to {name}", "trackMenu.properties": "Properties", "trackMenu.edit": "Edit tags", "properties.kicker": "Properties", @@ -1264,6 +1266,8 @@ const fr: Record = { "trackMenu.downloadFailed": "Le téléchargement n’a pas pu démarrer. Réessayez.", "trackMenu.dismiss": "Fermer", + "trackMenu.goToAlbum": "Aller à l’album", + "trackMenu.goToArtist": "Aller à {name}", "trackMenu.properties": "Propriétés", "trackMenu.edit": "Corriger les tags", "properties.kicker": "Propriétés", diff --git a/webapp/src/styles/styles.css b/webapp/src/styles/styles.css index 1941b341..393bb08d 100644 --- a/webapp/src/styles/styles.css +++ b/webapp/src/styles/styles.css @@ -1192,6 +1192,14 @@ main { vertical-align: middle; } +/* Each credited artist is a link; at rest it reads as the muted text it was, + and says it can be followed only when pointed at or focused. */ +.song-artists a:hover, +.song-artists a:focus-visible { + color: var(--text); + text-decoration: underline; +} + .songs .index { width: 2.5rem; color: var(--muted); @@ -2840,6 +2848,11 @@ button.danger { z-index: 40; display: grid; min-width: 13rem; + /* A track credited to many artists lists one entry each: capped at the + window, the menu scrolls on its own rather than running off the screen. */ + max-height: calc(100dvh - 2rem); + overflow-y: auto; + overscroll-behavior: contain; padding: var(--space-1); border: 1px solid var(--line-strong); border-radius: 0.75rem;