Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
2365806d18 | ||
|
|
7d348f243a | ||
|
|
ddd04623f7 | ||
|
|
9ad1477b1e | ||
|
|
70ab3ddf94 | ||
|
|
4f7529c315 | ||
|
|
bb21072386 |
@@ -3824,3 +3824,140 @@ Staging it is `/__test/emit` with a `JobsChanged` snapshot; a job with
|
||||
`state: "running"` never completes, so it stays up until an empty
|
||||
snapshot is emitted, which is what makes an idle re-measurement look
|
||||
like the fix not working.
|
||||
|
||||
## Two repaint mechanisms, and neither is pinned alone (measured 2026-08-20)
|
||||
|
||||
`CLAUDE.md` already states the rule — *a virtualized list repaints only
|
||||
when you tell it to, and the accidental way you were telling it may be
|
||||
the thing you are about to delete* — found in `artists-view` and
|
||||
`genres-view`. `queue-panel` is a second instance with numbers, and the
|
||||
numbers are the part worth keeping.
|
||||
|
||||
It repaints its rows **two** ways:
|
||||
|
||||
- `onSelectionChanged()` calls `virtualizer.requestUpdate()`, which is
|
||||
the intended one and the one `track-list` has always had;
|
||||
- `.keyFunction=${(track) => track.id}` is a **per-render arrow**, so it
|
||||
is a changed property on every host update and repaints the rows by
|
||||
itself.
|
||||
|
||||
Removing *either* alone changes nothing observable. That is why #43
|
||||
could not be settled by reading the code: the hypothesis in its Findings
|
||||
(the repaint is missing) was checkable, false, and would have looked
|
||||
identical either way.
|
||||
|
||||
Removing **both** does not break selection either — it delays it. Time
|
||||
from click to `aria-selected`, three clicks each:
|
||||
|
||||
| build | ms to highlight |
|
||||
|---|---|
|
||||
| healthy | 5, 16, 17 |
|
||||
| both mechanisms removed | 134, 3,866, 5,816 |
|
||||
|
||||
The highlight arrives on whatever unrelated render happens next (the
|
||||
player's 1 Hz position report is the usual candidate). **Four seconds is
|
||||
indistinguishable from broken to a user, and invisible to a spec** —
|
||||
`expect.poll`'s default 5 s timeout passes the degraded build on every
|
||||
assertion. `queue-selection.spec.ts` bounds its selection assertions at
|
||||
500 ms for that reason, which is ~30x the healthy case and an order of
|
||||
magnitude under the degraded one.
|
||||
|
||||
The general form, for the next spec about anything push-driven: **a poll
|
||||
generous enough to be stable is generous enough to miss a latency
|
||||
regression entirely.** If "late" is a failure mode worth having, the
|
||||
timeout has to say so.
|
||||
|
||||
## A hit-scan says how much of a row is not selectable (measured 2026-08-20)
|
||||
|
||||
`explore-link` stops the click's propagation on purpose — "the row must
|
||||
not also treat it as a selection" — so a click on a track, album or
|
||||
artist *name* navigates and selects nothing. That is app-wide and
|
||||
deliberate, and the useful question about any given list is how much of
|
||||
its row it costs.
|
||||
|
||||
Asking `elementFromPoint` what is under each x across a row, at three
|
||||
heights:
|
||||
|
||||
| list | link coverage |
|
||||
|---|---|
|
||||
| queue panel | 12% |
|
||||
| track list | 21% |
|
||||
|
||||
This killed a fix in progress. #43 reads as "selection is broken in the
|
||||
queue panel, and fine in the track list", the obvious mechanism is that
|
||||
the queue's narrow rows are mostly name, and it is **wrong**: the panel
|
||||
is *less* link-covered than the list it is being compared against. The
|
||||
scan takes a minute and is worth running before demoting anybody's links
|
||||
— `explore-album-details`'s tracklist (number / title / artist /
|
||||
duration) is the one that plausibly *is* mostly link, and is the one
|
||||
#5 is about to add selection to.
|
||||
|
||||
## A layout is still moving when a guard says it has arrived (measured 2026-08-20)
|
||||
|
||||
`album-dropdown.spec.ts` failed with `Expected 80, Received 10` twice
|
||||
over two sessions, and #133 already strengthened its guard from
|
||||
"scrollable at all" to "has at least the range the assertion needs".
|
||||
That was necessary and could not be sufficient, and the reason is
|
||||
structural rather than a matter of thresholds: **a guard and the write
|
||||
it guards are separate CDP round trips**, so the page is free to
|
||||
re-lay-out between them. Polling harder cannot close a window between
|
||||
two moments; only removing the window can.
|
||||
|
||||
Measured directly, sampling `scrollHeight - clientHeight` on
|
||||
`.grid-scroll-container` every frame across a 1440x900 → 900x600 resize,
|
||||
three runs:
|
||||
|
||||
| t (ms) | range |
|
||||
|---|---|
|
||||
| 0 | 0 |
|
||||
| 1 | **88** |
|
||||
| 8–14 | 330 (settled) |
|
||||
|
||||
88 satisfies a guard asking for 80 and is not the settled value, so the
|
||||
guard can pass while the grid is one layout pass from done. Under
|
||||
full-suite load the transient is worse — the observed failure had 10 —
|
||||
which is why it shows up on the second run of a suite and not in ten
|
||||
consecutive runs of the file alone (0/10 both before and after the fix).
|
||||
|
||||
The shape to write instead: **one page-side call that performs the
|
||||
action and returns what it observes**, with `expect.poll` retrying
|
||||
*that*. `scrollTo()` sets `scrollTop` and returns `scrollTop`, so the
|
||||
assertion is about what the grid did rather than about what it was
|
||||
ready to do. `layout-overflow.spec.ts`'s sidebar probe already had the
|
||||
fused half and was missing the retry; it has both now.
|
||||
|
||||
Worth generalising: a spec that resizes and then measures is asserting
|
||||
about a moving target for the next dozen frames. Fuse, then poll.
|
||||
|
||||
## "The first N tracks" is not a way to ask for an ordinary one (2026-08-20)
|
||||
|
||||
`queue-selection.spec.ts` staged its queue from the first few rows of
|
||||
`library.Library.GetTracks(0)` and clicked a track *name*, which
|
||||
`explore-link` routes to that track's **album** page. Four tracks in the
|
||||
fixture library have no album at all — `01 Tone A`, `02 Tone B`,
|
||||
`Title Only`, `no-tags-at-all` — and a name with nothing to route to
|
||||
renders as **plain text**, not as a link.
|
||||
|
||||
Two things follow, and the second is the sharper one.
|
||||
|
||||
**The order is the scan's.** `GetTracks` returns `audio_files.id` order,
|
||||
i.e. the order the scan inserted rows, which depends on concurrency and
|
||||
directory traversal. Locally the first eight are all from two proper
|
||||
albums, so the spec passed twice over; CI rebuilds its seed with a real
|
||||
scan, got a different eight, and failed on both engines. This is the
|
||||
same family as "a seed freezes every default it has already persisted" —
|
||||
the fixture library is not a list, it is a *set* with an incidental
|
||||
order, and no spec should depend on that order.
|
||||
|
||||
**A loose locator hid it.** The row was located with
|
||||
`.locator('.explore-link').first()`, and a row has two — the title and
|
||||
the artist. When the title is plain text, `first()` silently resolves to
|
||||
the **artist** link, so the click went somewhere real and the assertion
|
||||
was about a destination the test had not exercised. `.track-title
|
||||
.explore-link` is the locator that says which one it means; the loose
|
||||
one turned a fixture problem into a mystery.
|
||||
|
||||
The general rule for this repo's fixture library: it is deliberately
|
||||
full of edge cases (untagged, unicode, duplicates, extremes), so a spec
|
||||
that wants an *ordinary* track has to **say so** — filter on the
|
||||
property it depends on rather than slicing.
|
||||
|
||||
@@ -110,27 +110,30 @@ test.describe('the album dropdown', () => {
|
||||
await app.setViewportSize({ width: 900, height: 600 });
|
||||
|
||||
try {
|
||||
// Wait for the range the assertion below actually needs, not for
|
||||
// "scrollable at all" (#133). The guard used to be
|
||||
// `scrollHeight > clientHeight + 40` while the next line asks to
|
||||
// reach 80, so any range in 41-79 satisfied it and could not
|
||||
// satisfy the assertion — and the grid passes through exactly
|
||||
// that while it settles, because it recomputes its columns after
|
||||
// the resize rather than during it. The settled range here is
|
||||
// 330, so this waits rather than weakening anything.
|
||||
// The container has to be a scroller at all, which is the thing
|
||||
// the defect behind this spec broke and is a property rather
|
||||
// than a moment.
|
||||
await expect
|
||||
.poll(() => scrollRange(app))
|
||||
.toMatchObject({ room: true, overflowY: 'auto' });
|
||||
.toMatchObject({ overflowY: 'auto' });
|
||||
|
||||
await app.evaluate((target) => {
|
||||
const sc = document
|
||||
.querySelector('cover-grid')
|
||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||
|
||||
if (sc) sc.scrollTop = target;
|
||||
}, SCROLL_TARGET);
|
||||
|
||||
expect(await scrollTop(app)).toBe(SCROLL_TARGET);
|
||||
// **Scrolling it and reading it back are one round trip** (#151).
|
||||
//
|
||||
// #133 made the guard ask for the range this needs rather than
|
||||
// for "scrollable at all", which was necessary and is not
|
||||
// sufficient: a guard and the write it guards are separate
|
||||
// `evaluate` calls, so the grid can satisfy the range and settle
|
||||
// out of it before the write lands. It still does — observed as
|
||||
// `Expected 80, Received 10` in the second of three consecutive
|
||||
// full-suite runs, with the spec green alone on the same app
|
||||
// straight afterwards.
|
||||
//
|
||||
// Polling harder cannot close a window between two moments; only
|
||||
// removing the window can. So the probe sets `scrollTop` and
|
||||
// returns what it reads back, in one page-side call, and the
|
||||
// poll retries *that* — which also means the assertion is about
|
||||
// what the grid did rather than about what it was ready to do.
|
||||
await expect.poll(() => scrollTo(app, SCROLL_TARGET)).toBe(SCROLL_TARGET);
|
||||
|
||||
// And the dropdown it opens is on screen, wherever the manager
|
||||
// decides that leaves the scroll. It is *not* "the position is
|
||||
@@ -261,7 +264,7 @@ async function closeDropdown(app: Page): Promise<void> {
|
||||
});
|
||||
}
|
||||
|
||||
/** Whether the grid can scroll at all, which decides if a probe can move. */
|
||||
/** Whether the grid is a scroller at all, which is what the bug broke. */
|
||||
async function scrollRange(app: Page) {
|
||||
return app.evaluate((target) => {
|
||||
const sc = document
|
||||
@@ -269,22 +272,37 @@ async function scrollRange(app: Page) {
|
||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||
|
||||
return {
|
||||
// `room` is the precondition of the assertion that follows it:
|
||||
// enough range to actually reach the target. A threshold below
|
||||
// what the caller depends on is not a guard.
|
||||
// Reported for the failure message rather than waited on: `room`
|
||||
// was the guard #133 strengthened, and #151 is that a guard in
|
||||
// its own round trip cannot speak for the write in the next one.
|
||||
// `scrollTo` below is the assertion now; this says *why* it did
|
||||
// not reach the target when it does not.
|
||||
room: !!sc && sc.scrollHeight - sc.clientHeight >= target,
|
||||
overflowY: sc ? getComputedStyle(sc).overflowY : '',
|
||||
};
|
||||
}, SCROLL_TARGET);
|
||||
}
|
||||
|
||||
async function scrollTop(app: Page): Promise<number> {
|
||||
return app.evaluate(
|
||||
() =>
|
||||
document
|
||||
.querySelector('cover-grid')
|
||||
?.shadowRoot?.querySelector('.grid-scroll-container')?.scrollTop ?? -1,
|
||||
);
|
||||
/**
|
||||
* Scroll the grid and report where it actually landed, in one call.
|
||||
*
|
||||
* The whole point is that the set and the read share a moment: a
|
||||
* `scrollTop` write is clamped to the range *at the instant it lands*,
|
||||
* so reading it back in a second round trip asks a container that may
|
||||
* have re-laid out in between.
|
||||
*/
|
||||
async function scrollTo(app: Page, target: number): Promise<number> {
|
||||
return app.evaluate((to) => {
|
||||
const sc = document
|
||||
.querySelector('cover-grid')
|
||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||
|
||||
if (!sc) return -1;
|
||||
|
||||
sc.scrollTop = to;
|
||||
|
||||
return sc.scrollTop;
|
||||
}, target);
|
||||
}
|
||||
|
||||
/** Whether the open dropdown is inside the scroll container's viewport. */
|
||||
|
||||
@@ -132,22 +132,34 @@ test.describe('the app fits in its own window', () => {
|
||||
// be dragged here, but a scaled display or a large system font can
|
||||
// still land the layout in it, and clipping the nav with no scroll
|
||||
// is the failure that made Settings unreachable.
|
||||
const reachable = await app.locator('app-sidebar').evaluate((el) => {
|
||||
const settings = el.shadowRoot?.querySelector<HTMLElement>(
|
||||
'[data-testid="nav-settings"]',
|
||||
);
|
||||
//
|
||||
// The scroll and the measurement share one `evaluate` — #151's
|
||||
// rule, which this already had — and the whole probe is polled,
|
||||
// which it did not: a viewport change settles asynchronously, so a
|
||||
// single attempt reads whatever the sidebar happened to be doing.
|
||||
// The probe is safe to repeat because scrolling to the bottom twice
|
||||
// is scrolling to the bottom.
|
||||
await expect
|
||||
.poll(() =>
|
||||
app.locator('app-sidebar').evaluate((el) => {
|
||||
const settings = el.shadowRoot?.querySelector<HTMLElement>(
|
||||
'[data-testid="nav-settings"]',
|
||||
);
|
||||
|
||||
if (!settings) return null;
|
||||
if (!settings) return null;
|
||||
|
||||
el.scrollTop = el.scrollHeight;
|
||||
el.scrollTop = el.scrollHeight;
|
||||
|
||||
const item = settings.getBoundingClientRect();
|
||||
const pane = el.getBoundingClientRect();
|
||||
const item = settings.getBoundingClientRect();
|
||||
const pane = el.getBoundingClientRect();
|
||||
|
||||
return item.bottom <= Math.ceil(pane.bottom) && item.top >= Math.floor(pane.top);
|
||||
});
|
||||
|
||||
expect(reachable).toBe(true);
|
||||
return (
|
||||
item.bottom <= Math.ceil(pane.bottom) &&
|
||||
item.top >= Math.floor(pane.top)
|
||||
);
|
||||
}),
|
||||
)
|
||||
.toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -0,0 +1,299 @@
|
||||
import {
|
||||
test,
|
||||
expect,
|
||||
callBinding,
|
||||
navigateTo,
|
||||
LONG_TRACK,
|
||||
NO_QUEUE_SOURCE,
|
||||
} from '../support/fixtures.js';
|
||||
import type { Page } from '@playwright/test';
|
||||
|
||||
/**
|
||||
* The queue panel's mouse model (#43): single click selects, ctrl and
|
||||
* shift extend, double click plays from that row.
|
||||
*
|
||||
* **All four already worked, and nothing pinned any of them** — which is
|
||||
* the whole reason the report could be made and could not be settled.
|
||||
* `queue-reorder.spec.ts` covers the keyboard, `queue-overlay.spec.ts`
|
||||
* covers the panel's mode, and the component tier has the reorder
|
||||
* arithmetic; the pointer path had no coverage in either tier, so
|
||||
* "selection is broken here" and "selection is fine here" were equally
|
||||
* consistent with a green suite.
|
||||
*
|
||||
* Two things this spec is deliberately shaped around.
|
||||
*
|
||||
* **The clicks are real.** A `dispatchEvent(new MouseEvent('click'))`
|
||||
* on a row exercises the delegated handler and *not* the question being
|
||||
* asked, which is what the pointer lands on: the rows carry
|
||||
* `explore-link` names that take their own clicks, and a synthetic
|
||||
* event aimed at the row reports a selection the mouse would never have
|
||||
* produced. Every click here goes through Playwright.
|
||||
*
|
||||
* **The playing assertions use the 90-second fixture.** Every other
|
||||
* track is 2–6 seconds, so "double click plays row 3" read against a
|
||||
* 2-second track reports whatever auto-advance moved on to — measured
|
||||
* during this work as row 3 double-clicked and row 4 playing, which
|
||||
* reads exactly like an off-by-one in `PlayIndex` and is not one.
|
||||
*/
|
||||
|
||||
/**
|
||||
* How long a click may take to show up as a highlight.
|
||||
*
|
||||
* **A poll with the default 5s timeout cannot see this defect**, and
|
||||
* that is the point of naming it. `queue-panel` repaints its rows two
|
||||
* ways — `onSelectionChanged()` calls `virtualizer.requestUpdate()`,
|
||||
* and `.keyFunction` is a per-render arrow, which is itself a changed
|
||||
* property the virtualizer reacts to. With **both** removed the
|
||||
* highlight still arrives, on whatever unrelated render happens next:
|
||||
* measured at 134ms, 3,866ms and 5,816ms for three clicks, against
|
||||
* 5ms, 16ms and 17ms on a healthy build.
|
||||
*
|
||||
* A user cannot tell "four seconds late" from "broken", which is very
|
||||
* close to what this issue reports. So the assertion is that the
|
||||
* highlight is *prompt*, with a bound ~30x the measured healthy case
|
||||
* and an order of magnitude under the degraded one.
|
||||
*/
|
||||
const HIGHLIGHT_MS = 500;
|
||||
|
||||
/** The queue's own answer, never the DOM's. */
|
||||
async function playing(app: Page): Promise<{ index: number; title: string }> {
|
||||
const state = await callBinding<{
|
||||
currentIndex: number;
|
||||
tracks: { title: string }[];
|
||||
}>(app, 'queue.Queue.GetState');
|
||||
|
||||
return {
|
||||
index: state.currentIndex,
|
||||
title: state.tracks[state.currentIndex]?.title ?? '',
|
||||
};
|
||||
}
|
||||
|
||||
/** Which rows are selected, as the accessibility tree sees it. */
|
||||
const selected = (app: Page) =>
|
||||
app.evaluate(() =>
|
||||
[
|
||||
...document
|
||||
.querySelector('queue-panel')!
|
||||
.shadowRoot!.querySelectorAll('[data-index]'),
|
||||
]
|
||||
.filter((row) => row.getAttribute('aria-selected') === 'true')
|
||||
.map((row) => Number((row as HTMLElement).dataset['index'])),
|
||||
);
|
||||
|
||||
/**
|
||||
* Six tracks with the long one in the middle, so a "play from here"
|
||||
* assertion has something to land on that will still be playing when it
|
||||
* is read back.
|
||||
*/
|
||||
async function queueSixAndOpen(app: Page): Promise<void> {
|
||||
const paths = await app.evaluate(async (longTitle) => {
|
||||
// `TrackName`, not `Title`: the library model names it after the
|
||||
// tag, and the *queue* is what calls it `title`.
|
||||
const tracks = (await window.__yjEvents.call(
|
||||
'library.Library.GetTracks',
|
||||
[0],
|
||||
10_000,
|
||||
)) as { FilePath: string; TrackName: string; Album: string }[];
|
||||
|
||||
const long = tracks.find((t) => t.TrackName === longTitle);
|
||||
|
||||
/**
|
||||
* **Tracks that have an album**, which is a requirement of one of
|
||||
* the tests and was previously left to luck (#156).
|
||||
*
|
||||
* `explore-link` routes a track name to its *album's* page, so a
|
||||
* track with no album renders a name that navigates nowhere — and
|
||||
* the fixture library deliberately contains two (`01 Tone A`,
|
||||
* `02 Tone B`). Which tracks arrive first is `audio_files.id`
|
||||
* order, i.e. the order the **scan** inserted them, which depends
|
||||
* on concurrency and directory traversal: locally the first eight
|
||||
* all had albums and the spec passed twice over, and CI rebuilds
|
||||
* its seed with a real scan and got a different eight.
|
||||
*
|
||||
* Asking for what the test needs is the fix. It is not a
|
||||
* narrowing: every assertion here wants an ordinary track, and
|
||||
* "the first five rows" was never a way to ask for one in a
|
||||
* library whose whole purpose is edge cases.
|
||||
*/
|
||||
const rest = tracks
|
||||
.filter((t) => t.TrackName !== longTitle && t.Album !== '')
|
||||
.slice(0, 5);
|
||||
|
||||
// Index 3 is the long one: far enough down that a shift-extend has
|
||||
// room either side of it.
|
||||
return [
|
||||
...rest.slice(0, 3).map((t) => t.FilePath),
|
||||
long!.FilePath,
|
||||
...rest.slice(3).map((t) => t.FilePath),
|
||||
];
|
||||
}, LONG_TRACK);
|
||||
|
||||
await callBinding(app, 'queue.Queue.SetQueue', [
|
||||
paths,
|
||||
0,
|
||||
false,
|
||||
NO_QUEUE_SOURCE,
|
||||
]);
|
||||
|
||||
// A closed panel renders no list at all.
|
||||
await app.locator('#queue-button').click();
|
||||
await expect(app.locator('queue-panel .track-item').first()).toBeVisible();
|
||||
await expect(app.locator('queue-panel .track-item')).toHaveCount(6);
|
||||
}
|
||||
|
||||
/** The row at a data-index, not the nth child: see the note in the file. */
|
||||
const row = (app: Page, index: number) =>
|
||||
app.locator(`queue-panel .track-item[data-index="${index}"]`);
|
||||
|
||||
test.describe('selecting in the queue with a mouse', () => {
|
||||
// The suite shares one backend in file order, and a queue and an open
|
||||
// panel both outlive the page. `queue-reorder.spec.ts` sets the
|
||||
// precedent and the reason: a spec that spends state fails the next
|
||||
// one, in a list that reads like a regression in whatever you hold.
|
||||
test.afterEach(async ({ app }) => {
|
||||
await callBinding(app, 'queue.Queue.Clear').catch(() => {
|
||||
/* an empty queue is the state we were asking for */
|
||||
});
|
||||
|
||||
const open = await app.locator('queue-panel[open]').count();
|
||||
|
||||
if (open > 0) await app.locator('#queue-button').click();
|
||||
});
|
||||
|
||||
test('a single click selects that row and only that row', async ({ app }) => {
|
||||
await queueSixAndOpen(app);
|
||||
|
||||
await row(app, 1).click();
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([1]);
|
||||
|
||||
// And it *replaces* rather than accumulating, which is the half a
|
||||
// test of one click cannot see.
|
||||
await row(app, 4).click();
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([4]);
|
||||
});
|
||||
|
||||
test('ctrl adds a row and shift extends a range', async ({ app }) => {
|
||||
await queueSixAndOpen(app);
|
||||
|
||||
await row(app, 1).click();
|
||||
await row(app, 3).click({ modifiers: ['Control'] });
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([1, 3]);
|
||||
|
||||
// From the last row touched, so 3→5, keeping the ctrl-picked 1.
|
||||
await row(app, 5).click({ modifiers: ['Shift'] });
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([1, 3, 4, 5]);
|
||||
|
||||
// A plain click collapses the whole thing back to one.
|
||||
await row(app, 2).click();
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([2]);
|
||||
});
|
||||
|
||||
test('a double click plays from that row', async ({ app }) => {
|
||||
await queueSixAndOpen(app);
|
||||
|
||||
// Row 3 is the 90-second track. Asked of the backend, because the
|
||||
// panel's own highlight is a different claim.
|
||||
await row(app, 3).dblclick();
|
||||
|
||||
await expect.poll(() => playing(app)).toEqual({
|
||||
index: 3,
|
||||
title: LONG_TRACK,
|
||||
});
|
||||
|
||||
// Playing is not selecting: the double click clears the selection
|
||||
// it made on the way through, or every play leaves a row looking
|
||||
// picked out for an action the user did not ask for.
|
||||
await expect.poll(() => selected(app)).toEqual([]);
|
||||
});
|
||||
|
||||
/**
|
||||
* The one collision the report is actually about.
|
||||
*
|
||||
* Every track, album and artist name in the app navigates
|
||||
* (`utils/explore-link.ts`), and it does that by **stopping the
|
||||
* click's propagation** — in its own words, "the row must not also
|
||||
* treat it as a selection". So a click that lands on the name text
|
||||
* navigates and selects nothing, in the queue panel and in the track
|
||||
* list alike.
|
||||
*
|
||||
* That is deliberate and it is pinned here rather than argued with,
|
||||
* because the measurement says the queue is not the surface where it
|
||||
* hurts: a horizontal hit-scan of a row at three heights makes the
|
||||
* queue row **12%** link and the track list's row **21%** — the panel
|
||||
* the report calls broken is *less* covered by links than the list it
|
||||
* calls correct. What is left is one deliberate exception, and a
|
||||
* change to it should have to fail a test.
|
||||
*/
|
||||
test('a click on a name navigates instead, and that is the exception', async ({
|
||||
app,
|
||||
}) => {
|
||||
await queueSixAndOpen(app);
|
||||
|
||||
await row(app, 1).click();
|
||||
await expect
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.toEqual([1]);
|
||||
|
||||
// `.track-title .explore-link`, not `.explore-link` first(): a row
|
||||
// has two, and which one `first()` finds depends on whether the
|
||||
// *title* is a link at all. It is not, for a track with no album —
|
||||
// `explore-link` renders plain text where it cannot route — so the
|
||||
// loose locator silently clicked the **artist** instead and the
|
||||
// assertion below was about a different destination than the one
|
||||
// being exercised (#156).
|
||||
await row(app, 2).locator('.track-title .explore-link').click();
|
||||
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'explore-album-details',
|
||||
);
|
||||
|
||||
// Row 2 did not join the selection — the link took the click.
|
||||
await expect.poll(() => selected(app)).toEqual([1]);
|
||||
|
||||
await navigateTo(app, 'tracks');
|
||||
});
|
||||
|
||||
/**
|
||||
* And the other half of that bargain: the link holds its navigation
|
||||
* for one double-click interval and drops it if a second click
|
||||
* arrives, so double-clicking a *name* still plays the row rather
|
||||
* than navigating away from it. That is what makes the exception
|
||||
* above survivable, and it is the part most likely to break silently
|
||||
* if the grace interval is ever removed.
|
||||
*/
|
||||
test('a double click on a name plays rather than navigating', async ({
|
||||
app,
|
||||
}) => {
|
||||
await queueSixAndOpen(app);
|
||||
|
||||
// Read rather than assumed: which view the app lands on is the
|
||||
// user's `DefaultPage`, so naming one here would be asserting on a
|
||||
// config value in a test about a double click.
|
||||
const before = await app
|
||||
.getByTestId('main-content')
|
||||
.getAttribute('data-active-view');
|
||||
|
||||
await row(app, 3).locator('.explore-link').first().dblclick();
|
||||
|
||||
await expect.poll(() => playing(app)).toEqual({
|
||||
index: 3,
|
||||
title: LONG_TRACK,
|
||||
});
|
||||
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
before!,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -277,6 +277,26 @@ export class QueuePanel
|
||||
return this.queue.tracks.length;
|
||||
}
|
||||
|
||||
/**
|
||||
* Repaint the rows when the selection changes.
|
||||
*
|
||||
* `<lit-virtualizer>` renders through the `virtualize` directive,
|
||||
* which reacts to its *own* properties and not to the host having
|
||||
* re-rendered, so host state like a selection reaches the rows only
|
||||
* if it is pushed. `track-list` has always done this and both
|
||||
* playlist views had to be taught it.
|
||||
*
|
||||
* **There is a second, accidental mechanism here and it must not be
|
||||
* mistaken for this one**: `.keyFunction` below is a per-render
|
||||
* arrow, so it is a changed property on every host update and
|
||||
* repaints the rows by itself. Removing *either* alone changes
|
||||
* nothing observable, which is why #43 could not be settled by
|
||||
* reading the code. With both gone the highlight still arrives —
|
||||
* on whatever unrelated render happens next, measured at 134ms,
|
||||
* 3,866ms and 5,816ms against 5–17ms healthy, which a user cannot
|
||||
* tell from broken. `queue-selection.spec.ts` asserts the
|
||||
* *promptness* rather than the eventual state for that reason.
|
||||
*/
|
||||
onSelectionChanged(): void {
|
||||
this.virtualizer?.requestUpdate();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user