Compare commits

..
Author SHA1 Message Date
logan 70ab3ddf94 docs: record two measurements from the queue selection work
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m41s
CI / e2e (pull_request) Successful in 8m1s
The first is a second instance of a rule CLAUDE.md already states, with
numbers: a virtualized list can be repainting for a reason you are about
to delete, and here there are two such reasons — so removing either
alone changes nothing observable, and removing both leaves the highlight
seconds late rather than absent. That is the shape a poll cannot see,
which is the general lesson worth keeping.

The second is the hit-scan, because it stopped a wrong fix: the queue
panel is 12% link and the track list 21%, which is the opposite of the
assumption the fix was being built on.
2026-08-19 23:00:03 -04:00
logan 4f7529c315 test(queue): pin the panel's mouse model, and bound the highlight
Single click selects, ctrl and shift extend, double click plays from
that row — all four already worked, and nothing in either tier pinned
any of them, which is why the report could be made and could not be
settled. `queue-reorder.spec.ts` covers the keyboard and
`queue-overlay.spec.ts` the panel's mode; the pointer path had no
coverage at all, so "selection is broken here" and "selection is fine
here" were equally consistent with a green suite.

Measured with real mouse events rather than dispatched ones, because a
synthetic click aimed at the row bypasses the only thing that could be
swallowing it: click row 1 selects 1, ctrl+click 4 gives 1 and 4,
shift+click 7 extends to 1,4,5,6,7, a plain click collapses to one, and
a double click on row 3 leaves the backend playing row 3.

The three candidates the issue lists are all answered. The repaint was
already correct, and already correct on the day the issue was filed.
`resolveTrackIndexFromEvent` reads data-index, and DOM order matches
data order. A row control does swallow the click — `explore-link` stops
propagation on purpose, so a click on a name navigates and selects
nothing — but a hit-scan across a row makes the queue 12% link against
the track list's 21%, so the panel called broken is *less* covered by
links than the list called correct. That measurement killed the fix
this started out as.

Two traps are written into the spec because both faked a defect while
measuring. Fixture tracks are 2 seconds, so "double click row 3" read a
moment later reports whatever auto-advance moved on to — recorded twice
as an off-by-one that is not one, which is what `LONG_TRACK` exists
for. And the selection assertions are bounded at 500ms rather than
polled with the default 5s: `queue-panel` repaints two ways, the
explicit `requestUpdate()` and a per-render `keyFunction` arrow, and
with *both* removed the highlight still arrives — at 134ms, 3.9s and
5.8s against 5-17ms healthy. Four seconds is indistinguishable from
broken to a user and invisible to a generous poll.

Mutation-tested rather than trusted: `playAtIndex(index + 1)` fails both
double-click tests, treating every click as ctrl+click fails both
selection tests, and removing both repaint mechanisms fails all three
selection tests — the last only because of the bound.

Closes #43
2026-08-19 22:59:55 -04:00
logan bb21072386 Merge pull request 'The top bar decides what it can afford to show' (#149) from fix/143-top-bar-fits-its-window into main
CI / check (push) Successful in 2m34s
CI / e2e (push) Successful in 7m59s
Fits the top bar to its window by measuring it, at every supported
width and with work in flight.

Closes #143
2026-08-20 02:11:54 +00:00
logan ead1354e4d docs: record how the top bar decides what to drop
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m35s
CI / e2e (pull_request) Successful in 8m38s
The shell section already states the three size bands and the promise
that no action is unreachable at any of them; how the header chooses
what to give up belongs beside them, because the promise is what
decides it.

Two measured facts go to NOTES.md rather than here. `scrollWidth`
counts a box's left padding and not its right, so the obvious fit
predicate under-reports by a gutter and passed on a bar with a control
jammed against the window edge. And the overflow is 11px idle and 262px
while working, which is why the issue was filed twice with different
numbers — a seeded app that has finished scanning is idle by the time
you resize it.

Closes #143
2026-08-19 21:35:12 -04:00
logan ae85df0dad fix(shell): give the top bar a measured fit at every width
The bar was 611px inside a 600px viewport at the bottom of the Compact
band, and 862px while a scan with a real library's title ran, because
`job-indicator` is `hidden` when idle and 235px wide when it is not.
`body` is `overflow-x: auto`, so a user got a horizontal scrollbar on a
shell #24 promised would not need one — and the band is 600 to 899 with
work in flight, not the 600 to 610 the idle measurement suggested.

`services/top-bar-fit.ts` is `page-header`'s treatment one bar up: a
ResizeObserver, every pass starting from all-visible, hiding the
lowest-priority child until it fits. Measured rather than breakpointed
because three of the five children are as wide as their content — the
library filter by the longest library name, the indicator by the
running job's title, the search box by its view-scoped placeholder — so
any width picked is right for one library, one job and one view.

What yields is decided by #24's own sentence, which rules out the two
cheapest candidates in the Direction. Hiding the library filter takes
away an action, since it is the only control in the app that selects a
library (filed as #148, which is the phone already doing it), and
collapsing search to an icon is #57's, which is blocked behind #62. So
the wordmark yields first — a brand the window title bar repeats, and
visually-hidden rather than `display: none` because that h1 is the
document's heading — and then the indicator's label, leaving the ring,
which the component already does below 600px and whose live region
announces the state either way.

"Fits" is the children against the content box, not `scrollWidth`
against `clientWidth`: `scrollWidth` counts the left padding and not
the right, so the first version read 700/700 with the indicator sitting
in the whole right gutter. And the bar does not resize when a job
starts, which is the case this is for, so every child is observed too.

Pinned before it was fixed, as the issue asks. On the unfixed build the
new spec fails at 600 idle and at 600, 800 and 900 with a job, and
passes at 390, 899 and 1440; `layout-overflow.spec.ts` gains 600x600
and failed there. That spec asserts on the *shell*, so it was green
throughout this defect — the per-child measurement is #69's lesson, and
it is what caught the gutter case above.

Closes #143
2026-08-19 21:35:03 -04:00
logan 6e7e349e63 Merge pull request 'Fold the Jobs tab into the places the work is started' (#147) from feat/27-jobs-into-settings into main
CI / check (push) Successful in 2m34s
CI / e2e (push) Successful in 7m25s
2026-08-20 01:02:41 +00:00
10 changed files with 972 additions and 10 deletions
+130
View File
@@ -3761,3 +3761,133 @@ the same run. Reproducing it locally is running the suite twice against
one `make dev-headless` — which is worth doing for any change that
leaves state behind, since it is the only place a cross-engine order
dependency shows up.
## `scrollWidth` counts the left padding and not the right (measured 2026-08-20)
The obvious predicate for "does this flex row fit" is
`el.scrollWidth <= el.clientWidth`, and on a box with symmetric gutters
it **under-reports by one gutter**. `scrollWidth` is the extent of the
scrollable content area, which includes `padding-left` and excludes
`padding-right`; `clientWidth` includes both. So a child may end up to
`padding-right` past where content is allowed to go while the box
reports a perfect fit.
Measured on the top bar (`padding: 0 2em`) at 700x600 with a long-titled
scan staged: `clientWidth 700`, `scrollWidth 700` — and
`job-indicator`'s right edge at 700 against a content edge of 668, i.e.
sitting in the whole right gutter. `#143`'s first fix passed its own
measurement and left the indicator visibly jammed against the window
edge.
The predicate `services/top-bar-fit.ts` uses instead is the one its
spec asserts: no in-flow child's rect outside the parent's *content*
box, both edges, with half a pixel of slack for fractional flex widths.
This is the same family as #69's title trap — the measurement easiest to
reach for is the one that cannot see the failure — and it is worth
knowing before writing the next one of these: **the fit test and the
assertion that proves it should be the same test.** It was found only
because `top-bar-fit.spec.ts` measures per child rather than asserting
on the container, which is exactly why #69 needed
`header-action-overflow.spec.ts`.
## The top bar's overflow is 11px idle and 262px while working (measured 2026-08-20)
#143 was filed as "11px at 600x600" and re-measured as 171. Both are the
same defect seen with different jobs running: `job-indicator` is
`hidden` when idle, ~144px wide showing "Scanning Music", and **235px**
showing a real library's scan title ("Scanning Music from the external
drive"), because the label is capped at 12rem and gets there.
Swept against the running app with that job staged, `header.top-bar`
client vs scroll:
| width | idle | with the long-titled scan |
|---|---|---|
| 320, 390, 599 | fits | fits (the phone rules drop the filter and the label) |
| 600 | 611 | **862** |
| 700 | fits | 862 |
| 800 | fits | 862 |
| 899 | fits | 899 (fits) |
| 900 | fits | 946 |
| 1100, 1440 | fits | fits |
Two things worth keeping. The band is **600610 idle and 600900 while
working**, so "a narrow corner" and "the header is crowded from 900
down" are both true and the difference is entirely what is in flight —
which is the case a seeded, settled app can never show you. And 899
fits while 900 does not, because `nav-history` appears at 900: the worst
width for the header is not the narrowest one, the same way 900 rather
than 800 is the worst width for the content area.
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.
+52
View File
@@ -1520,6 +1520,58 @@ three, *no action is ever unreachable at any supported size*. The bands
themselves already existed; what was new is that they are a promise and
that the queue panel is inside it.
**The top bar decides what it can afford, and what it gives up is never
an action.** Its five children do not fit at the bottom of the Compact
band: the bar was 611px inside a 600px viewport idle and **862px while
a scan ran**, because `job-indicator` is `hidden` when idle and 235px
wide showing a real library's scan title (#143). So `services/
top-bar-fit.ts` is `page-header`'s treatment one bar up — a
ResizeObserver, every pass starting from all-visible, hiding the
lowest-priority child until it fits.
Five things about it are load-bearing.
**It is measured rather than breakpointed for a reason specific to this
bar**: three of its five children are as wide as their *content* — the
library filter is a `<select>` sized by the longest library name, the
indicator by the running job's title, the search box by its view-scoped
placeholder — so any width picked is right for one library, one job and
one view. Swept with a long-titled scan staged, the bar overflowed at
**every** width from 600 to 899 *and* at 900 where `nav-history`
appears, while 899 fits; a breakpoint fixing "600 to 610" would have
fixed whichever case happened to be idle when it was measured.
**What yields is decided by the promise above, which rules out the two
cheapest answers.** Hiding the library filter takes away an action —
`library-filter` is the only control in the app that calls
`setSelectedLibrary` — so it trades this promise for the same promise
(#148 is the phone already doing that). Collapsing the search box to an
icon is what #57 wants and #57 is blocked behind #62, so building it
here is building it without the thing that blocks it. The two that
yield are the two that are **not** actions: the wordmark, which the
window's own title bar repeats and which #48 wants down to "YJ" at
every width anyway, and then the job indicator's *label*, leaving the
ring — which is not a new judgement, since the component already drops
it below 600px and its `sr-only` live region is what announces the
state either way.
**The wordmark yields its width, not its existence.** The collapsed
rule is visually-hidden rather than `display: none`, because that `h1`
is the document's top-level heading as well as the brand.
**"Fits" is the children against the content box, and `scrollWidth`
cannot express it.** `scrollWidth` counts a box's left padding and not
its right, so with 2em gutters it under-reports by 32px: the first fix
read `700/700` — a perfect fit — with the indicator sitting in the
whole right gutter. Same family as #69's title trap, and found only
because `top-bar-fit.spec.ts` measures **per child**, which is what
`layout-overflow.spec.ts` cannot do and why that spec was green
throughout the defect.
And **the bar does not resize when a job starts**, which is the case the
whole thing is for — a ResizeObserver on the header alone never fires,
so every element child is observed too.
**900 is the worst desktop width, not the 800×600 minimum.** The
sidebar collapses to icons *below* 900, so the main panel is 843px at
899 and 700px at 900 — the narrowest content area any desktop width
+33 -9
View File
@@ -33,6 +33,14 @@ const VIEWPORTS = [
// was missing its own worst case.
{ name: '900×600 (the widest sidebar, so the narrowest content)', width: 900, height: 600 },
{ name: `the minimum (${MIN_VIEWPORT.width}×${MIN_VIEWPORT.height})`, ...MIN_VIEWPORT },
// Below the enforced minimum on purpose, and for the reason 700×480
// is below it further down: a scaled display or a large system font
// lands the layout here without the window ever being dragged there,
// and 600 is the last width before the phone layout takes over. The
// *narrowest header* is a different question from the narrowest
// content area and has a different answer — this one (#143), where
// the bar was 611px inside 600 sitting still.
{ name: '600×600 (the bottom of the Compact band)', width: 600, height: 600 },
];
/**
@@ -145,6 +153,12 @@ test.describe('the app fits in its own window', () => {
test.describe('the title block fits its bar', () => {
test('the hgroup stays inside the 4em top bar', async ({ app }) => {
// Stated rather than inherited from whatever ran last. Since #143
// the wordmark is visually hidden at widths where the bar cannot
// afford it, so a test about its *vertical* fit has to say which
// width it is asking about.
await app.setViewportSize({ width: 1440, height: 900 });
// The state a11y.29 landed in. The pair is flex-centred and a UA
// gives an `h1` a 0.67em top margin, so the block measured 67px
// inside 64 — pre-existing, and invisible until dropping the h3's
@@ -246,16 +260,26 @@ test.describe('the shell reflows rather than hiding what does not fit', () => {
test(`no scrollbar appears at ${vp.name}`, async ({ app }) => {
await app.setViewportSize({ width: vp.width, height: vp.height });
const excess = await app.evaluate(() => {
const de = document.documentElement;
// Polled, for the reason the track-row test above is: since #143
// the top bar's fit is *measured* — a ResizeObserver decides what
// it can afford at this width — so a single read taken straight
// after the resize races the observer and reports the frame
// before it. Read once, this passed alone and failed in the full
// suite, which is the shape of a timing assumption rather than of
// a defect.
//
// The other half of the assertion: at every size this app
// promises, the fix costs nothing. A scrollbar that is always
// there is a worse answer than the clipping it replaced.
await expect
.poll(() =>
app.evaluate(() => {
const de = document.documentElement;
return de.scrollWidth - de.clientWidth;
});
// The other half: at every size this app promises, the fix costs
// nothing. A scrollbar that is always there is a worse answer
// than the clipping it replaced.
expect(excess).toBe(0);
return de.scrollWidth - de.clientWidth;
}),
)
.toBe(0);
});
}
});
+271
View File
@@ -0,0 +1,271 @@
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 26 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 }[];
const long = tracks.find((t) => t.TrackName === longTitle);
const rest = tracks.filter((t) => t.TrackName !== longTitle).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]);
await row(app, 2).locator('.explore-link').first().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!,
);
});
});
+207
View File
@@ -0,0 +1,207 @@
import { test, expect } from '../support/fixtures.js';
/**
* The top bar fits the window it is in (#143).
*
* **This is measured per child, not on the shell**, which is #69's
* lesson repeated one component over: `layout-overflow.spec.ts` asserts
* the *document* needs no sideways scrolling, and clipping inside a
* component is invisible to it — which is exactly why that spec was
* green throughout this defect. What a user sees is a control rendered
* past the edge of the bar it belongs to, so that is what is asserted.
*
* **And it is measured with a job running**, which is the half the
* original report missed. `job-indicator` is `hidden` while idle and up
* to 235px wide when it is not, so the bar was 611px inside 600 sitting
* still and 862px during a scan — 171 to 262px of overflow, arriving
* exactly when a user has reason to look at that bar. Nothing else in
* this suite has ever measured a layout with work in flight;
* `/__test/emit` stages it without staging the scan.
*/
type Page = import('@playwright/test').Page;
/**
* The widths this asks about.
*
* 600 is the bottom of the Compact band (#24) and where the defect
* lands; 899 and 900 straddle `nav-history` appearing (68px more to
* find, at the width that just gained the sidebar's labels); 800 is the
* enforced minimum; 390 is a phone, where the answer must be that
* nothing collapses because the media queries already did the work.
*/
const WIDTHS = [390, 600, 800, 899, 900, 1440];
/**
* A scan whose title is as long as a real one gets. The label is capped
* at 12rem by the component, so this is the widest the indicator can
* be — measuring with "Scanning" instead reports a bar that fits and a
* defect that is 100px smaller than it is.
*/
const LONG_JOB = {
id: 'top-bar-fit',
kind: 'library-scan',
state: 'running',
title: 'Scanning Music from the external drive',
current: 40,
total: 100,
};
/**
* Every child's right edge against the bar's own content box.
*
* The content box, not `clientWidth`: the bar has a 2em right gutter,
* and a control sitting in the padding is already the failure — it is
* simply one that `scrollWidth` under-reports, because `scrollWidth`
* counts the left padding and not the right.
*/
const overflowingChildren = (page: Page) =>
page.evaluate(() => {
const bar = document.querySelector<HTMLElement>('header.top-bar')!;
const style = getComputedStyle(bar);
const box = bar.getBoundingClientRect();
const left = box.left + parseFloat(style.paddingLeft);
const right = box.right - parseFloat(style.paddingRight);
return [...bar.children]
.filter((child) => {
const cs = getComputedStyle(child);
// Out of flow is out of the question: a collapsed wordmark is
// `position: absolute` and 1px wide precisely so it costs the
// row nothing.
if (cs.display === 'none' || cs.position === 'absolute') return false;
const r = child.getBoundingClientRect();
return r.width > 0 && (r.right > right + 0.5 || r.left < left - 0.5);
})
.map((child) => {
const r = child.getBoundingClientRect();
return `${child.tagName.toLowerCase()}: ${Math.round(r.left)}..${Math.round(r.right)} outside ${Math.round(left)}..${Math.round(right)}`;
});
});
/** What the fit pass gave up, read back off the DOM it changed. */
const collapsed = (page: Page) =>
page.evaluate(() => ({
wordmark: !!document.querySelector('header.top-bar hgroup.yj-collapsed'),
jobLabel: !!document.querySelector('job-indicator[compact]'),
}));
test.describe('the top bar fits the window', () => {
for (const width of WIDTHS) {
test(`no control sits outside the bar at ${width}px, idle`, async ({
app,
}) => {
await app.setViewportSize({ width, height: 600 });
await expect.poll(() => overflowingChildren(app)).toEqual([]);
});
test(`no control sits outside the bar at ${width}px, with a job running`, async ({
app,
testctl,
}) => {
await app.setViewportSize({ width, height: 600 });
await testctl.emit('JobsChanged', [LONG_JOB]);
// The indicator has to actually be up, or this test passes by
// measuring the idle case under another name.
await expect(app.locator('job-indicator')).toBeVisible();
await expect.poll(() => overflowingChildren(app)).toEqual([]);
});
}
/**
* The other half of "measured, never breakpointed": a rule that
* collapses defensively at every narrow width fits just as well and
* is a worse app. 1440 is roomy at any job title; 899 was measured to
* fit with the longest one, because `nav-history` is not there yet.
*/
test('nothing is given up where there is room for it', async ({
app,
testctl,
}) => {
await app.setViewportSize({ width: 1440, height: 900 });
await testctl.emit('JobsChanged', [LONG_JOB]);
await expect(app.locator('job-indicator')).toBeVisible();
await expect.poll(() => collapsed(app)).toEqual({
wordmark: false,
jobLabel: false,
});
});
/**
* And it gives them back. The pass starts from all-visible every
* time, so this is the property that a rule which only ever *added*
* to the collapsed set would fail — the wordmark would be gone for
* the rest of the session after one narrow moment.
*/
test('the wordmark comes back when the window does', async ({
app,
testctl,
}) => {
await testctl.emit('JobsChanged', [LONG_JOB]);
await app.setViewportSize({ width: 600, height: 600 });
await expect.poll(() => collapsed(app)).toEqual({
wordmark: true,
jobLabel: true,
});
await app.setViewportSize({ width: 1440, height: 900 });
await expect.poll(() => collapsed(app)).toEqual({
wordmark: false,
jobLabel: false,
});
});
/**
* The wordmark yields its width and not its existence: `display:
* none` would take the document from one top-level heading to none.
*/
test('the collapsed wordmark is still the document heading', async ({
app,
testctl,
}) => {
await testctl.emit('JobsChanged', [LONG_JOB]);
await app.setViewportSize({ width: 600, height: 600 });
await expect.poll(() => collapsed(app)).toMatchObject({ wordmark: true });
await expect(
app.getByRole('heading', { name: 'YellowJacket', level: 1 }),
).toHaveCount(1);
});
/**
* And the indicator keeps saying what it is doing after its visible
* label goes — the `sr-only` live region is what announces the state,
* which is the same argument the phone's own rule was written on.
*/
test('the job indicator still announces its state without its label', async ({
app,
testctl,
}) => {
await testctl.emit('JobsChanged', [LONG_JOB]);
await app.setViewportSize({ width: 600, height: 600 });
await expect.poll(() => collapsed(app)).toMatchObject({ jobLabel: true });
const spoken = await app
.locator('job-indicator')
.evaluate(
(el) =>
el.shadowRoot?.querySelector('[aria-live]')?.textContent?.trim() ?? '',
);
expect(spoken).toContain('Scanning Music from the external drive');
// Leave the app as the next spec expects to find it.
await app.setViewportSize({ width: 1440, height: 900 });
});
});
+36
View File
@@ -104,6 +104,42 @@ p {
flex: 0 1 320px;
}
/* What the bar gives up when it does not fit is decided by measuring
it (`services/top-bar-fit.ts`, #143). Two rules here are what make
that measurement mean anything.
**Nothing but the search box may shrink.** `scrollWidth` reports a
perfect fit while a child quietly truncates -- #69's trap, one
component over -- and the indicator's label is `text-overflow:
ellipsis`, so it would have absorbed the deficit and hidden it. The
search box is exempt because it shrinks between its 320px basis and
the 200px floor its own stylesheet sets, and a narrower input hides
nothing it was showing. */
.top-bar hgroup,
.top-bar library-filter,
.top-bar job-indicator {
flex-shrink: 0;
}
/* **The wordmark yields its width, not its existence.** It is the
app's top-level heading as well as its brand, and `display: none`
would take a document from one `h1` to none at exactly the widths
where the view's own header is the only thing left saying where you
are. This is `styles/sr-only.css.ts`'s recipe, written out because
that one is a `CSSResult` for shadow roots and this is the light
DOM. */
.top-bar hgroup.yj-collapsed {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip-path: inset(50%);
white-space: nowrap;
border: 0;
}
/* The bar is `justify-content: space-between`, which with four children
spreads them evenly and left back/forward floating in the middle of
nothing. Collecting the free space *after* this one puts the pair
+9
View File
@@ -54,6 +54,7 @@ import '@store/theme-store';
import './src/services/keyboard-shortcut-service';
import { activateView, deactivateView } from '@utils/view-lifecycle';
import { installLongPressContextMenu } from '@utils/long-press';
import { installTopBarFit } from './src/services/top-bar-fit';
import {
hasTrackPayload,
getDragPayload,
@@ -73,6 +74,14 @@ registerBundledIcons();
// on `pointerType === 'touch'` only.
installLongPressContextMenu();
// The top bar decides what it can afford to show (#143). Here rather
// than in a component because the bar is light DOM in index.html and
// its children are five separate elements; the shell is the only thing
// that can see all five at once.
const topBar = document.querySelector<HTMLElement>('header.top-bar');
if (topBar) installTopBarFit(topBar);
// ---------------------------------------------------------------------------
// View caching navigation system
// ---------------------------------------------------------------------------
+15 -1
View File
@@ -155,13 +155,27 @@ export class JobIndicator extends LitElement {
goes -- the live region in render() is what announces
this, and it is unaffected, so the ring keeps its
accessible name and screen readers keep hearing the
state change. */
state change.
[compact] is the same removal asked for by measurement
rather than by width, and it is set from outside: the
shell's fit pass (services/top-bar-fit.ts, #143) owns
it, because between 600 and 900 whether this label fits
depends on what else is in the bar and on how long the
running job's title is -- 235px for "Scanning Music from
the external drive" -- rather than on the viewport. Two
triggers, one effect, and the phone's is unconditional
because it was argued and pinned before this existed. */
@media (max-width: 599px) {
.label {
display: none;
}
}
:host([compact]) .label {
display: none;
}
.alert-dot {
width: 6px;
height: 6px;
@@ -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 517ms 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();
}
+199
View File
@@ -0,0 +1,199 @@
/**
* What the top bar drops when it runs out of room (#143).
*
* The bar holds five children — the wordmark, back/forward, the library
* filter, the search box and the job indicator — and at the bottom of
* the Compact band they do not all fit. Measured on `main` at 600×600:
* the bar is 611px inside a 600px viewport sitting still, and **862px
* while a scan with a long title is running**, because `job-indicator`
* is `hidden` when idle and up to 235px wide when it is not. `body` is
* `overflow-x: auto`, so what a user sees is a horizontal scrollbar on
* a shell that #24 promised would not need one.
*
* **The fit is measured, never breakpointed**, which is `page-header`'s
* rule (#69) and applies here for a reason specific to this bar: three
* of its five children are as wide as their *content*. The library
* filter is a `<select>` sized by the longest library name, the job
* indicator by the running job's title, and the search box by its
* view-scoped placeholder — so any width you pick is right for exactly
* one library, one job and one view. The same sweep that produced the
* numbers above found the bar overflowing at every width from 600 to
* 899 *and* at 900, where `nav-history` reappears; a breakpoint fixing
* "600 to 610" would have fixed the case that happened to be idle.
*
* **What yields is chosen by #24's own sentence** — *no action is ever
* unreachable at any supported size* — which rules out the two cheapest
* candidates the issue lists. Hiding the library filter takes away an
* action: `library-filter` is the **only** control in the app that sets
* the selected library (nothing else calls `setSelectedLibrary`), so
* hiding it is trading this promise for the same promise. Collapsing
* the search box to an icon is what #57 wants on a phone, but #57 is
* blocked behind #62 and building its modal here would be building it
* without the thing that blocks it.
*
* So the two things that yield are the two that are **not** actions and
* whose content survives elsewhere:
*
* 1. **The wordmark**, which is a brand — the window's own title bar
* says the same thing, and #48 wants it down to "YJ" at every width
* anyway. It yields its *width*, not its existence: the rule in
* `index.css` is visually-hidden rather than `display: none`, so the
* document keeps its top-level heading.
* 2. **The job indicator's label**, leaving the ring. This is not a new
* judgement — the component already drops it below 600px for exactly
* this reason, and its `sr-only` live region is what announces the
* state either way, so nothing is lost to anyone. What a measurement
* adds is the band between 600 and 900, where whether the label fits
* depends on what else is in the bar rather than on the width alone.
*
* Measured against the running app with a long-titled scan staged, that
* order fits at every width from 320 to 1440 — and collapses nothing at
* 320, 390, 599, 899 and 1100, which is the other half of the claim.
*
* Three things about the mechanism are load-bearing.
*
* **Every pass starts from all-visible**, so the collapsed set is a
* pure function of the current width rather than of how the window got
* there. `page-header` states the same rule and the same reasons: a
* pass that only ever added would never give the wordmark back, and one
* that adjusted by a step would need a hysteresis band to stop it
* oscillating on the pixel where it exactly fits.
*
* **"Fits" is the children against the content box, not `scrollWidth`
* against `clientWidth`** — and that distinction is not pedantry, it
* is a measured false pass. `scrollWidth` counts a box's *left*
* padding and not its right, so with this bar's 2em gutters it
* under-reports by 32px: at 700px with a scan running it read
* `700/700`, a perfect fit, while `job-indicator` ended 32px past
* where the content may go and sat in the gutter. Same family as #69's
* title trap, one property over — the measurement that is easiest to
* reach for is the one that cannot see the failure. So the predicate
* here is the same one `top-bar-fit.spec.ts` asserts: no in-flow child
* outside the content box.
*
* That is only truthful in turn because **nothing here absorbs pressure
* by truncating**. The collapsible children are `flex-shrink: 0` in
* `index.css`, so a deficit shows up as a child out of bounds instead
* of quietly eating the indicator's label, which is `text-overflow:
* ellipsis` and would have. The search box is the one child that may
* shrink, between its 320px basis and the 200px floor its own
* stylesheet sets, and a narrower input hides nothing it was showing.
*
* **The bar does not resize when a job starts**, which is the case the
* whole thing is for. A ResizeObserver on the header alone never fires:
* the indicator goes 0 → 235 inside a bar whose width has not changed.
* Every element child is observed too.
*/
/** One thing the bar can give up, cheapest first. */
interface FitStep {
/** For tests and for reading the DOM back. */
readonly id: string;
/** Applied to the bar; `on` collapses. */
readonly collapse: (bar: HTMLElement, on: boolean) => void;
}
/**
* The order things are given up in. Lowest priority first — see the
* argument above for why these two and not the library filter.
*/
export const FIT_STEPS: readonly FitStep[] = [
{
id: 'wordmark',
collapse: (bar, on) =>
bar.querySelector('hgroup')?.classList.toggle('yj-collapsed', on),
},
{
id: 'job-label',
collapse: (bar, on) =>
bar.querySelector('job-indicator')?.toggleAttribute('compact', on),
},
];
/**
* Decide what the bar shows at its current width.
*
* Exported for the component tier, which can hand it a bar of known
* widths; the app installs the observer below and never calls this.
*
* @returns the ids collapsed, in the order they were given up.
*/
export function measureTopBarFit(bar: HTMLElement): string[] {
const fits = () => {
const style = getComputedStyle(bar);
const box = bar.getBoundingClientRect();
const left = box.left + parseFloat(style.paddingLeft);
const right = box.right - parseFloat(style.paddingRight);
for (const child of bar.children) {
const cs = getComputedStyle(child);
// Out of flow is out of the question: a collapsed wordmark
// is absolutely positioned and 1px wide precisely so that
// it costs the row nothing.
if (cs.display === 'none' || cs.position === 'absolute') continue;
const r = child.getBoundingClientRect();
// Sub-pixel slack: a flex row's widths are fractional and a
// rounding difference is not an overflow anyone can see.
if (r.width > 0 && (r.right > right + 0.5 || r.left < left - 0.5)) {
return false;
}
}
return true;
};
for (const step of FIT_STEPS) step.collapse(bar, false);
const collapsed: string[] = [];
if (!fits()) {
for (const step of FIT_STEPS) {
step.collapse(bar, true);
collapsed.push(step.id);
if (fits()) break;
}
}
return collapsed;
}
/**
* Watch the bar and its children, and keep it fitting.
*
* Returns the uninstaller, which the tests use; the app installs once
* for the life of the session.
*/
export function installTopBarFit(bar: HTMLElement): () => void {
let measuring = false;
const measure = () => {
// A pass resizes the children it collapses, which the observer
// would report back to us. It settles either way — the pass is
// idempotent at a given width — but re-entering it is work for
// no news, and it is what "ResizeObserver loop completed with
// undelivered notifications" is.
if (measuring) return;
measuring = true;
try {
measureTopBarFit(bar);
} finally {
measuring = false;
}
};
const observer = new ResizeObserver(measure);
observer.observe(bar);
for (const child of bar.children) observer.observe(child);
measure();
return () => observer.disconnect();
}