fix(explore): repaint the tracklist when a track is requested

The album page's downloadStore subscription assigned canDownload and
re-synced isRequested, both of which are about the *release group* --
so requesting a **track** changed no reactive property, Lit had no
reason to re-render, and every row's badge kept the plus it was drawn
with. The request was filed and visible in Downloads; the control that
filed it appeared to do nothing, which is what making it a button was
meant to stop.

libraryStatusFor() reads the store at render time, so the fix is to
say so: one explicit requestUpdate on the subscription.

The badge is also no longer revealed on hover. That rule was inherited
from the green ticks it replaced, and it does not survive the reason
those went: a tick marked the common case, this marks the rows that are
*not* here. A mark on the exception is the information, and one that
exists only under the pointer cannot be seen, counted, or reached by a
finger or a keyboard.

Refs #33

e2e/specs/queue-toggle-state.spec.ts

@@ -1,66 +0,0 @@
-import { test, expect } from '../support/fixtures.js';
-
-/**
- * The queue button says whether the queue is open.
- *
- * It used to look identical in both states, so the only way to tell
- * what pressing it would do was to look at the other side of the window
- * and infer it — and for anyone not looking at all there was nothing to
- * infer from: no `aria-expanded`, no `aria-controls`, no pressed state.
- *
- * The state is reflected *from the panel*, not kept beside the click,
- * because the button is not the only thing that opens the queue —
- * `now-playing-view` sets the same attribute, since it hides the bar
- * this button lives in. A flag maintained by the click handler would be
- * right until something else opened the panel and then quietly wrong,
- * which is the second test here.
- */
-test.describe('the queue toggle', () => {
-  test('reports open and closed, and names what it controls', async ({
-    app,
-  }) => {
-    const toggle = app.locator('#queue-button');
-
-    await expect(toggle).toHaveAttribute('aria-controls', 'queue-panel');
-    await expect(toggle).toHaveAttribute('aria-expanded', 'false');
-
-    await toggle.click();
-    await expect(toggle).toHaveAttribute('aria-expanded', 'true');
-
-    // The state is not only in the accessibility tree: a control that
-    // announces a state it does not draw is half a fix.
-    //
-    // Background rather than colour, because the pointer is still on
-    // the button after the click and `:hover` paints it the same accent
-    // the open state does -- so a colour comparison here passes on the
-    // broken build and proves nothing.
-    const [open, closed] = await toggle.evaluate((el) => {
-      const now = getComputedStyle(el).backgroundColor;
-
-      el.setAttribute('aria-expanded', 'false');
-      const shut = getComputedStyle(el).backgroundColor;
-
-      el.setAttribute('aria-expanded', 'true');
-
-      return [now, shut];
-    });
-
-    expect(open).not.toBe(closed);
-
-    await toggle.click();
-    await expect(toggle).toHaveAttribute('aria-expanded', 'false');
-  });
-
-  test('follows the panel when something else opens it', async ({ app }) => {
-    const toggle = app.locator('#queue-button');
-
-    await expect(toggle).toHaveAttribute('aria-expanded', 'false');
-
-    // Exactly what `now-playing-view`'s queue button does.
-    await app.evaluate(() =>
-      document.getElementById('queue-panel')?.setAttribute('open', ''),
-    );
-
-    await expect(toggle).toHaveAttribute('aria-expanded', 'true');
-  });
-});

frontend/index.css

@@ -207,17 +207,6 @@ body div.sidebar {
         color: var(--yj-accent, #ffd43b);
     }
 
-    /* An open queue is a state this button can be in, and it used to
-       look exactly like the closed one -- so the only way to tell what
-       pressing it would do was to look at the other side of the window
-       and infer it. `aria-expanded` is the same fact for anyone not
-       looking at all, and it points at the panel it controls. */
-    #queue-button[aria-expanded='true'] {
-        color: var(--yj-accent, #ffd43b);
-        background: var(--yj-bg-overlay, #404040);
-        border-radius: 4px;
-    }
-
     #queue-button.drag-over {
         color: var(--yj-accent, #ffd43b);
         outline: 2px dashed var(--yj-accent, #ffd43b);

frontend/index.html

@@ -37,8 +37,7 @@
     <footer class="bottom-bar">
         <now-playing></now-playing>
         <audio-player></audio-player>
-        <button aria-label="Toggle queue" aria-controls="queue-panel" aria-expanded="false"
-            id="queue-button">
+        <button aria-label="Toggle queue" id="queue-button">
             <wa-icon name="list"></wa-icon>
         </button>
     </footer>

frontend/index.ts

@@ -521,28 +521,6 @@ if (queueButton && queuePanel) {
         }
     });
 
-    // The button says whether the panel is open, and it learns that
-    // from the panel rather than from its own click handler.
-    //
-    // It is not the only thing that opens the queue -- `now-playing-view`
-    // sets the same attribute, because it hides the bar this button
-    // lives in -- so a state kept beside the click would be right until
-    // something else opened the panel and then quietly wrong. The panel's
-    // `open` attribute is the one fact; this reflects it.
-    const reflectQueueState = () => {
-        queueButton.setAttribute(
-            'aria-expanded',
-            String(queuePanel.hasAttribute('open')),
-        );
-    };
-
-    new MutationObserver(reflectQueueState).observe(queuePanel, {
-        attributes: true,
-        attributeFilter: ['open'],
-    });
-
-    reflectQueueState();
-
     // ---------------------------------------------------------------
     // Queue button as drop target (when queue panel is closed)
     // ---------------------------------------------------------------

frontend/src/components/explore-album-details/explore-album-details.ts

@@ -662,34 +662,20 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
                 font-weight: 400;
             }
 
-            /* The request control is only offered where there is
-             * something to request, and only when the row is being
-             * attended to — a column of plus signs down a mostly-owned
-             * album is the clutter the green ticks were.
+            /* The request control is offered on every row that has
+             * something to request, and is not revealed on hover.
              *
-             * Hidden with opacity, never display:none or visibility,
-             * so it keeps its place in the layout (rows do not reflow
-             * as the pointer moves) and stays in the tab order and the
-             * accessibility tree.  focus-within is what makes it
-             * reachable without a mouse: tabbing to the button reveals
-             * it, and the row's own focus reveals it before you get
-             * there. */
+             * It used to be transparent until the row was hovered or
+             * focused, on the reasoning that a column of plus signs is
+             * clutter.  That reasoning was inherited from the green
+             * ticks it replaced and does not survive the rule those
+             * were removed for: a tick marked the *common* case, while
+             * this marks the rows that are **not** here.  A mark on
+             * the exception is the information, and one that appears
+             * only under the pointer cannot be seen, counted, or found
+             * by anyone driving this with a finger or a keyboard. */
             .track-row .track-request {
                 flex-shrink: 0;
-                opacity: 0;
-                transition: opacity 0.12s ease;
-            }
-
-            .track-row:hover .track-request,
-            .track-row:focus-within .track-request,
-            .track-row .track-request:focus-visible {
-                opacity: 1;
-            }
-
-            @media (prefers-reduced-motion: reduce) {
-                .track-row .track-request {
-                    transition: none;
-                }
             }
         `,
     ];
@@ -720,9 +706,18 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
 
         // The download button only appears once a client is connected,
         // so this tracks the provider list rather than assuming.
+        //
+        // The explicit `requestUpdate` is what makes a *track*'s badge
+        // move.  `canDownload` and `syncRequested` are both about the
+        // release group, so neither changes when a row is requested —
+        // and every row's badge reads `libraryStatusFor(...)` out of
+        // the store at render time, so with no reactive property
+        // changed Lit had no reason to re-render and the badge sat on
+        // a plus for a request that had already been filed.
         this.downloadUnsub = downloadStore.subscribe(() => {
             this.canDownload = downloadStore.available;
             this.syncRequested();
+            this.requestUpdate();
         });
 
         void downloadStore.init().then(() => {

frontend/test/components/album-track-request.test.ts

@@ -0,0 +1,153 @@
+/**
+ * Asking for a track from an album's tracklist.
+ *
+ * The badge on an unowned row is a `<button>` that files a durable
+ * request, and its state is computed at render time from the download
+ * store — `libraryStatusFor(owned, mbid)` reads `requestFor(mbid)` out
+ * of the cached list.
+ *
+ * That is what made this fail. The page's `downloadStore` subscription
+ * assigned `canDownload` and re-synced `isRequested` for the *release
+ * group*, and neither of those changes when a **track** is requested,
+ * so Lit saw no reactive property change and never re-rendered. The
+ * request was filed, the Downloads tab showed it, and the badge stayed
+ * on a plus reading "not in your library" — a control that appears to
+ * do nothing, which is exactly what the badge was made a button to stop
+ * being.
+ *
+ * The second assertion is the row's own: the badge is not revealed on
+ * hover. A mark on the rows you do *not* have is the information on
+ * this page, and one that only exists under the pointer cannot be seen,
+ * counted, or reached by a finger.
+ */
+import { describe, expect, it, beforeEach } from 'vitest';
+import type { LitElement } from 'lit';
+
+import '@components/explore-album-details/explore-album-details';
+import { stub, flush, resetHarness } from '@test/support/harness';
+import { fixture, shadow, shadowAll } from '@test/support/render';
+
+type StoredRequest = {
+  id: number;
+  mbid: string;
+  entity: string;
+  libraryId: number;
+  artist: string;
+  title: string;
+  state: string;
+  attempts: number;
+};
+
+/** The request list the store reads, mutated by the stubbed AddRequest
+ *  exactly as the backend's own list would be. */
+let requests: StoredRequest[] = [];
+
+function track(n: number, owned: boolean) {
+  return {
+    position: n,
+    discNumber: 1,
+    title: `Track ${n}`,
+    length: 200000,
+    mbid: `mbid-${n}`,
+    inLibrary: owned,
+  };
+}
+
+/** An album with one owned track and one that is not here. */
+async function albumWithAnUnownedTrack(): Promise<LitElement> {
+  const el = await fixture<LitElement>('explore-album-details', {
+    albumName: 'Glass Harbour',
+    releaseGroupMBID: 'rg-1',
+  });
+
+  const tracks = [track(1, true), track(2, false)];
+
+  stub('library.Library.GetFilePathsByRecordingMBIDs', {
+    'mbid-1': ['/music/mbid-1.mp3'],
+  });
+
+  Object.assign(el, {
+    versionEntries: [
+      {
+        key: 'v1',
+        label: '2019',
+        sublabel: '2 tracks',
+        tracks,
+      },
+    ],
+    selectedVersionKey: 'v1',
+    loadingReleases: false,
+    loadingInfo: false,
+  });
+  el.requestUpdate();
+  await flush();
+  await el.updateComplete;
+
+  return el;
+}
+
+const badges = (el: LitElement) =>
+  shadowAll(el, 'library-status-indicator.track-request');
+
+describe('requesting a track from the album tracklist', () => {
+  beforeEach(async () => {
+    resetHarness();
+    requests = [];
+
+    stub('library.Library.GetFilePathsByRecordingMBIDs', {});
+    stub('library.Library.GetFilePathsByAlbums', {});
+    stub('library.Library.GetAlbumTracks', []);
+    stub('library.Library.GetAllLibrariesWithTrackCounts', [
+      { id: 1, name: 'Music', path: '/music', trackCount: 1 },
+    ]);
+
+    stub('download.Service.ProviderKinds', []);
+    stub('download.Service.ListProviders', []);
+    stub('download.Service.ListDownloads', []);
+    stub('download.Service.ListRequests', () => requests);
+    stub('download.Service.AddRequest', (input: Record<string, unknown>) => {
+      const id = requests.length + 1;
+
+      requests.push({
+        id,
+        mbid: String(input.mbid),
+        entity: String(input.entity),
+        libraryId: Number(input.libraryId),
+        artist: String(input.artist ?? ''),
+        title: String(input.title ?? ''),
+        state: 'wanted',
+        attempts: 0,
+      });
+
+      return id;
+    });
+  });
+
+  it('marks the row as queued without a reload', async () => {
+    const el = await albumWithAnUnownedTrack();
+
+    // Only the unowned row offers one: there is nothing left to ask for
+    // on a track there is a file for.
+    const [badge] = badges(el);
+
+    expect(badge).toBeTruthy();
+    expect(badge?.getAttribute('status')).toBe('not-in-library');
+
+    shadow<HTMLElement>(badge!, 'button')?.click();
+    await flush();
+    await el.updateComplete;
+
+    expect(requests.map((r) => r.mbid)).toEqual(['mbid-2']);
+    expect(badges(el)[0]?.getAttribute('status')).toBe('queued');
+  });
+
+  it('shows the badge without being hovered', async () => {
+    const el = await albumWithAnUnownedTrack();
+    const [badge] = badges(el);
+
+    // The old rule hid it at `opacity: 0` until `:hover`. Computed
+    // opacity is the assertion rather than the absence of a rule,
+    // because the rule could come back under another selector.
+    expect(getComputedStyle(badge!).opacity).toBe('1');
+  });
+});