Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
31144e5dc7 | ||
|
|
8af26fee94 |
@@ -1782,3 +1782,93 @@ Nine more things worth keeping, and the first four are all one theme —
|
|||||||
not inherit `box-sizing: border-box` from the UA stylesheet the way a
|
not inherit `box-sizing: border-box` from the UA stylesheet the way a
|
||||||
`<button>` does, so swapping the tag grew the badge 36→38px. Nothing
|
`<button>` does, so swapping the tag grew the badge 36→38px. Nothing
|
||||||
but the stored screenshot would have noticed.
|
but the stored screenshot would have noticed.
|
||||||
|
|
||||||
|
## The state a fix lands in is a state nobody has looked at
|
||||||
|
|
||||||
|
Plan 008 phase 1: the three `a11y.md` findings that lose function — a
|
||||||
|
marquee that cannot be stopped, a combobox that announces nothing, and a
|
||||||
|
queue whose order needs a mouse.
|
||||||
|
|
||||||
|
The generalisation, and it is the mirror of "a finding creates the
|
||||||
|
conditions for the next one": that note is about the code path a fix
|
||||||
|
*opens*. This one is about the code path a fix **sends people to**. A
|
||||||
|
guard, a fallback, an empty state, a disabled variant — the branch a fix
|
||||||
|
makes people live in has usually never been looked at by anyone,
|
||||||
|
precisely because until now nobody arrived there.
|
||||||
|
|
||||||
|
The reduced-motion guard is two lines. Both bugs it exposed were in the
|
||||||
|
place it sends you:
|
||||||
|
|
||||||
|
- **The non-scrolling fallback hard-clipped**, and always had.
|
||||||
|
`text-overflow: ellipsis` was on the outer span while the box that
|
||||||
|
overflows is the `inline-block` child — so it produced an ellipsis in
|
||||||
|
**no mode**, including `hover`, which is the default every user has.
|
||||||
|
A title read "An Exhaustively Overlong Trac|". Found by reading the
|
||||||
|
screenshot of the fix, which is the fourth regression in two plans
|
||||||
|
that only a PNG has caught.
|
||||||
|
- **And fixing *that* broke overflow detection.** Giving the child its
|
||||||
|
own `overflow: hidden` stops the parent overflowing, so
|
||||||
|
`titleOverflows` went false and nothing would have scrolled again for
|
||||||
|
anybody. Caught by the new test's *positive* case — which existed
|
||||||
|
only because a guard that suppresses everything passes the negative
|
||||||
|
case for free, which is this repo's oldest rule wearing its eighth
|
||||||
|
costume.
|
||||||
|
|
||||||
|
Eight more things worth keeping:
|
||||||
|
|
||||||
|
- **A grep triage is a good answer to "is it still there" and no answer
|
||||||
|
to "why".** Checking all 34 findings against the tree took ten
|
||||||
|
minutes and closed at least five the coverage map still showed open,
|
||||||
|
including three (`17`, `19`, `27`) fixed by phases that were not about
|
||||||
|
them. It said nothing about mechanism, and mechanism is what decided
|
||||||
|
that `15`'s obvious CSS-only fix is wrong.
|
||||||
|
- **A finding's stated scope can be half-closed by an unrelated phase.**
|
||||||
|
`a11y.11` is "drag-and-drop has no keyboard equivalent anywhere" and
|
||||||
|
its stated symptom is "there is no keyboard path to add a track to
|
||||||
|
the queue or a playlist" — which Phase 5's `MenuKeyboard` closed. What
|
||||||
|
was actually left is the queue's *order*, the one thing a menu cannot
|
||||||
|
express. Fixing the sentence rather than the residue would have built
|
||||||
|
three menu commands that already exist.
|
||||||
|
- **A count in an audit is scoped by how it was taken.** `a11y.6` says
|
||||||
|
two buttons are "the only truly unnamed controls" — and says, in the
|
||||||
|
same line, that it scanned every `<button>`. The AX tree has two
|
||||||
|
unnamed `combobox` roles that are native `<select>`s, one of them the
|
||||||
|
page header's sort control on nine views. The claim was never wrong;
|
||||||
|
it was answering a narrower question than it reads as.
|
||||||
|
- **The probe was wrong, not the fix — twice more, both on the *after*
|
||||||
|
side.** Reading `activedescendant` out of `getFullAXTree` as
|
||||||
|
`relatedNodes[0].text` reported `(none)` against a working build,
|
||||||
|
because the property carries `value.type: "idref"`. And
|
||||||
|
`__yjEvents.last('QueueChanged')` returned a stale payload, so a
|
||||||
|
reorder that had happened read as one that had not. Dump the whole
|
||||||
|
property; ask `GetState`.
|
||||||
|
- **An operation's index convention is part of its contract, and the
|
||||||
|
symmetric-looking version fails silently.** `MoveQueueTracks` takes an
|
||||||
|
index into the array *before* the move, so up-by-one asks for `i - 1`
|
||||||
|
and down-by-one has to ask for `i + 2` — `i + 1` is where the row
|
||||||
|
already is once its own removal is accounted for, and the backend's
|
||||||
|
contiguous-block guard correctly returns without doing anything. The
|
||||||
|
first version moved rows up and did nothing at all downward, with no
|
||||||
|
error anywhere.
|
||||||
|
- **A roving tab stop that only moves on arrow keys is not where the
|
||||||
|
focus is.** `focusedIndex` was never synced from a click or a Tab, so
|
||||||
|
`Enter` played the first track in the queue from *any* focused row —
|
||||||
|
pre-existing, invisible for as long as the keys only read state, and
|
||||||
|
obvious the moment a key moved something.
|
||||||
|
- **Watch the new spec fail on the old build.** Done for all three
|
||||||
|
landings, by neutering one line rather than by stashing (which
|
||||||
|
reverts every uncommitted change in the file). Two of the three would
|
||||||
|
have passed against the broken build in at least one case if the
|
||||||
|
positive direction had been left out.
|
||||||
|
- **CI's concurrency cancels the previous run's `e2e` when you push
|
||||||
|
again**, and `cancelled` sits one line from `success` in the run
|
||||||
|
list. The first landing's e2e never ran; the signal came from the
|
||||||
|
second push's run, read step by step through
|
||||||
|
`/api/v1/repos/{owner}/{repo}/actions/runs/{run}/jobs`. Same family as
|
||||||
|
the `skipped` WebKit step, one layer out.
|
||||||
|
|
||||||
|
And the one that is no longer worth calling a lesson: **a backtick
|
||||||
|
inside a comment in a `css` tagged template literal ends the literal.**
|
||||||
|
Third session running. It is written in `CLAUDE.md`, in the skill, and
|
||||||
|
in `NOTES.md`, and it was read twice in the session it then cost a
|
||||||
|
cycle in. Knowledge is not working here; it wants a lint rule.
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
# 008 — The last audit, and the one binding that outlived six phases
|
# 008 — The last audit, and the one binding that outlived six phases
|
||||||
|
|
||||||
**Status:** active
|
**Status:** active — Phase 1 shipped (three landings).
|
||||||
**Branch:** main
|
**Branch:** main
|
||||||
**Created:** 2026-08-12
|
**Created:** 2026-08-12
|
||||||
**Follows:** 007-ui-reconciliation
|
**Follows:** 007-ui-reconciliation
|
||||||
@@ -68,6 +68,7 @@ fixed until it has been reproduced in the running app.
|
|||||||
| `22` | Minor | `queue-panel` gained `aria-current`; `track-list` did not, and neither has a non-colour marker. |
|
| `22` | Minor | `queue-panel` gained `aria-current`; `track-list` did not, and neither has a non-colour marker. |
|
||||||
| `24` | Minor | No `title` on the truncating element in `track-info`, `playlist-view`, `queue-panel` or `track-list`. |
|
| `24` | Minor | No `title` on the truncating element in `track-info`, `playlist-view`, `queue-panel` or `track-list`. |
|
||||||
| `25` | Minor | `<wa-progress-bar value=…>` with no label, verbatim as filed. |
|
| `25` | Minor | `<wa-progress-bar value=…>` with no label, verbatim as filed. |
|
||||||
|
| — | new | **Two unnamed native `<select>`s**, one of them `page-header`'s sort control on nine views. Not in the audit: `a11y.6` scanned `<button>`. Found in the AX tree while reproducing `14`. Belongs with `26`. |
|
||||||
| `28` | dropped | Four `@mousedown` `<div>`s with no `role="separator"`. Never measured. |
|
| `28` | dropped | Four `@mousedown` `<div>`s with no `role="separator"`. Never measured. |
|
||||||
| `29` | Polish | `<h3 class="subtitle">` for type size. |
|
| `29` | Polish | `<h3 class="subtitle">` for type size. |
|
||||||
| `30` | Polish | No skip link anywhere. |
|
| `30` | Polish | No skip link anywhere. |
|
||||||
@@ -169,6 +170,93 @@ An e2e case for `11`, because the queue panel's animated width means a
|
|||||||
click issued while it moves lands on whatever slid under the pointer.
|
click issued while it moves lands on whatever slid under the pointer.
|
||||||
A manual pass per landing, with a screenshot read.
|
A manual pass per landing, with a screenshot read.
|
||||||
|
|
||||||
|
### Phase 1 — what actually shipped
|
||||||
|
|
||||||
|
Three landings, one per finding, each reproduced in the running app
|
||||||
|
before anything was written and each watched failing on the pre-fix
|
||||||
|
build before being believed.
|
||||||
|
|
||||||
|
- **`15`.** `shouldScroll()` returns false under
|
||||||
|
`prefers-reduced-motion: reduce`, live (a `matchMedia` listener, so
|
||||||
|
changing the OS setting is honoured without a reload — verified).
|
||||||
|
It covers `hover` as well as `always`.
|
||||||
|
- **`14`.** Ids on the listbox and every option, `aria-controls`,
|
||||||
|
`aria-activedescendant`, and `aria-selected` meaning *chosen* rather
|
||||||
|
than *highlighted*.
|
||||||
|
- **`11`.** Alt+ArrowUp/Down moves the focused queue row, with a live
|
||||||
|
region saying where it went.
|
||||||
|
|
||||||
|
Pinned by `now-playing.test.ts` (+2), `combobox-aria.test.ts` (5),
|
||||||
|
`queue-reorder.test.ts` (7), `e2e/specs/reduced-motion.spec.ts` (2) and
|
||||||
|
`e2e/specs/queue-reorder.spec.ts` (4). `make ui-test` 558 → **572**;
|
||||||
|
`make e2e` 68 → **74**.
|
||||||
|
|
||||||
|
#### Where the plan was wrong — Phase 1
|
||||||
|
|
||||||
|
Nine things, and the first group is the triage being right for the
|
||||||
|
wrong reason.
|
||||||
|
|
||||||
|
- **The grep triage was accurate about *what* is open and wrong about
|
||||||
|
*why* two of them are.** It is a good first pass and it cannot see
|
||||||
|
mechanism. `15` is filed as "no reduced-motion guard", which is true;
|
||||||
|
what makes a CSS-only guard wrong is that the cycle is a transition
|
||||||
|
out, a `transitionend` and a transition back, so suppressing the
|
||||||
|
animation strands the text off its own box with nothing to bring it
|
||||||
|
back. That is only visible by reading the cycle.
|
||||||
|
- **A fix routes people into a state nobody has looked at.** With the
|
||||||
|
marquee off, the fallback hard-clipped — "Overlong Trac|", no
|
||||||
|
ellipsis — because `text-overflow` was on the outer span while the
|
||||||
|
overflowing box is the inline-block child. It had never produced an
|
||||||
|
ellipsis **in any mode**, including the default, and no test saw it.
|
||||||
|
Found by reading the screenshot of the fix.
|
||||||
|
- **…and fixing that broke the measurement it depends on.** Giving the
|
||||||
|
child its own `overflow: hidden` stops the *parent* overflowing, so
|
||||||
|
`titleOverflows` went false and nothing would ever have scrolled
|
||||||
|
again, for anyone. Caught by the new test's positive case, which is
|
||||||
|
the whole reason it has one.
|
||||||
|
- **`a11y.6` scanned `<button>`, and says so.** "The only truly
|
||||||
|
unnamed controls" is a claim about buttons. The AX tree has two
|
||||||
|
unnamed `combobox` roles that are native `<select>`s — one of them
|
||||||
|
the page header's sort control, on nine views. Not fixed here; it is
|
||||||
|
a sweep of every form control, not a one-liner, and it belongs with
|
||||||
|
`a11y.26` in Phase 3.
|
||||||
|
- **The reproduction of the *fix* was wrong twice, on the probe side
|
||||||
|
both times.** Reading `activedescendant` out of the AX tree as
|
||||||
|
`relatedNodes[0].text` returned `(none)` on a working build — the
|
||||||
|
property is there, with `value.type: "idref"`. And `last('QueueChanged')`
|
||||||
|
returned a stale payload, so a reorder that had happened looked like
|
||||||
|
one that had not. Ask `GetState`, dump the whole property.
|
||||||
|
- **`11`'s stated scope is half done and the other half was already
|
||||||
|
closed.** The finding is "drag-and-drop has no keyboard equivalent
|
||||||
|
anywhere" and lists four sites; its stated *symptom* — "there is no
|
||||||
|
keyboard path to add a track to the queue or a playlist" — was closed
|
||||||
|
by Phase 5's `MenuKeyboard`. What was left is the queue's order, which
|
||||||
|
is the one the menu cannot express. Album→queue drag and
|
||||||
|
drop-on-nav-item remain, and are menu commands, not reorder.
|
||||||
|
- **The plan said a backend panel binding; it should not be one.** The
|
||||||
|
queue panel already handles Enter and the roving arrows in its own
|
||||||
|
*delegated* (not document) keydown, which is the sanctioned pattern.
|
||||||
|
Alt+Arrow joins them: it cannot collide with the global Up/Down
|
||||||
|
volume bindings (measured — 0 `VolumeChanged` events from a focused
|
||||||
|
row), and it keeps a reordering key out of a user-editable table
|
||||||
|
where it could be rebound onto something unmodified.
|
||||||
|
- **The index arithmetic is not symmetric, and the symmetric version
|
||||||
|
fails silently.** `MoveQueueTracks` takes an index into the array
|
||||||
|
*before* the move, so down-by-one must ask for `i + 2`; `i + 1` is
|
||||||
|
where the row already is once its own removal is accounted for, and
|
||||||
|
the backend's contiguous-block guard correctly returns without doing
|
||||||
|
anything. Pinned in both tiers.
|
||||||
|
- **`focusedIndex` was only ever moved by an arrow key.** A row reached
|
||||||
|
by a click or by Tab left it at 0, so `Enter` played the first track
|
||||||
|
in the queue from any focused row. Pre-existing, invisible until a
|
||||||
|
key moved something, fixed by reading the index off the row the event
|
||||||
|
came from.
|
||||||
|
|
||||||
|
And one that is not about the audit: **the backtick-in-a-`css`-comment
|
||||||
|
trap cost a cycle again**, in the same session as reading the warning
|
||||||
|
about it twice. It is worth treating as a lint rule rather than a piece
|
||||||
|
of knowledge.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Phase 2 — The two that were never measured
|
## Phase 2 — The two that were never measured
|
||||||
|
|||||||
@@ -505,6 +505,53 @@ first track arrives) and `job-indicator`, whose label swings between
|
|||||||
"Scanning Music", "3 background jobs" and "Finished". The notification
|
"Scanning Music", "3 background jobs" and "Finished". The notification
|
||||||
surface already had one from Phase 3.
|
surface already had one from Phase 3.
|
||||||
|
|
||||||
|
**A stated motion preference outranks an app setting, and the state a
|
||||||
|
fix lands in is a state nobody has looked at.** `now-playing`'s marquee
|
||||||
|
ran for as long as a track played with no way to pause it (WCAG 2.2.2),
|
||||||
|
and the guard is in `shouldScroll()` rather than in CSS: the cycle is a
|
||||||
|
transition out, a `transitionend` and a transition back, so suppressing
|
||||||
|
the animation strands the text off its own box with nothing to bring it
|
||||||
|
back. It covers `hover` as well as `always` — `reduce` is a request
|
||||||
|
about motion, not about autoplay. The two bugs behind it were both in
|
||||||
|
the *fallback*: `text-overflow` sat on the outer span while the box that
|
||||||
|
overflows is the inline-block child, so the non-scrolling state had
|
||||||
|
never produced an ellipsis **in any mode**, including the default; and
|
||||||
|
moving the ellipsis to the child stops the parent overflowing, which
|
||||||
|
silently disabled overflow *detection* and would have stopped anything
|
||||||
|
scrolling ever again. Both measurements come from the child now. The
|
||||||
|
first was found by reading a screenshot, the second by the new test's
|
||||||
|
positive case.
|
||||||
|
|
||||||
|
**Roles have to be wired to each other.** `combobox`, `listbox` and
|
||||||
|
`option` were all present on `<yj-combobox>` and nothing connected them,
|
||||||
|
so arrowing through nineteen options moved a highlight and announced
|
||||||
|
nothing. Ids on the listbox and every option, `aria-controls`,
|
||||||
|
`aria-activedescendant` — and `aria-selected` meaning *chosen*, which is
|
||||||
|
the distinction the pattern rests on: the highlight is what
|
||||||
|
`activedescendant` points at. Unlike `config-section`'s disclosure this
|
||||||
|
IDREF may dangle while closed, because the popup genuinely does not
|
||||||
|
exist then and `aria-expanded` says so. Checked against
|
||||||
|
`Accessibility.getFullAXTree`, not against a snapshot — and read the
|
||||||
|
whole property, since `activedescendant` reports `value.type: "idref"`
|
||||||
|
and an extraction expecting a string reports `(none)` on a working
|
||||||
|
build.
|
||||||
|
|
||||||
|
**The queue's order is reachable from the keyboard.** Alt+ArrowUp/Down
|
||||||
|
moves the focused row, with a live region saying where it went. It is
|
||||||
|
in `queue-panel`'s own *delegated* keydown beside Enter and the roving
|
||||||
|
arrows, not a backend panel binding: it cannot collide with the global
|
||||||
|
Up/Down volume bindings, and a reordering key does not belong in a
|
||||||
|
user-editable table where it could be rebound onto something
|
||||||
|
unmodified. Two things in it are load-bearing. **The index arithmetic
|
||||||
|
is not symmetric** — `MoveQueueTracks` takes an index into the array
|
||||||
|
*before* the move, so down-by-one asks for `i + 2`, because `i + 1` is
|
||||||
|
where the row already is once its own removal is accounted for and the
|
||||||
|
backend's contiguous-block guard correctly treats it as a no-op. And
|
||||||
|
**the index comes off the row the event came from**: `focusedIndex` was
|
||||||
|
only ever moved by an arrow key, so a row reached by a click or by Tab
|
||||||
|
left it at 0 and `Enter` played the first track in the queue from any
|
||||||
|
focused row.
|
||||||
|
|
||||||
**A selectable grid is a listbox.** The four grids that ctrl/shift-select
|
**A selectable grid is a listbox.** The four grids that ctrl/shift-select
|
||||||
(`artists-view`, `genres-view`, `cover-grid`, and the queue) are
|
(`artists-view`, `genres-view`, `cover-grid`, and the queue) are
|
||||||
`role="listbox" aria-multiselectable` over `role="option"` cards, not
|
`role="listbox" aria-multiselectable` over `role="option"` cards, not
|
||||||
|
|||||||
@@ -0,0 +1,120 @@
|
|||||||
|
import { test, expect, callBinding } from '../support/fixtures.js';
|
||||||
|
import type { Page } from '@playwright/test';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* `a11y.11` — the queue's order can be changed without a mouse.
|
||||||
|
*
|
||||||
|
* The component tier pins the arithmetic against a faked binding. This
|
||||||
|
* one is here because the arithmetic is only half of it: `toIndex` is
|
||||||
|
* interpreted by `Queue.MoveQueueTracks`, whose contiguous-block guard
|
||||||
|
* turns the plausible-looking `i + 1` into a silent no-op. Nothing but
|
||||||
|
* the real backend can say whether the order actually moved.
|
||||||
|
*
|
||||||
|
* Reproduced first: with a row focused, Alt/Ctrl/Shift/Meta + arrows all
|
||||||
|
* left the order untouched.
|
||||||
|
*/
|
||||||
|
|
||||||
|
/** The queue's order, asked of the backend rather than of the DOM. */
|
||||||
|
async function order(app: Page): Promise<string[]> {
|
||||||
|
const state = await callBinding<{ tracks: { title: string }[] }>(
|
||||||
|
app,
|
||||||
|
'queue.Queue.GetState',
|
||||||
|
);
|
||||||
|
|
||||||
|
return state.tracks.map((t) => t.title);
|
||||||
|
}
|
||||||
|
|
||||||
|
async function queueFourAndOpen(app: Page): Promise<string[]> {
|
||||||
|
const paths: string[] = await app.evaluate(async () => {
|
||||||
|
const tracks = await window.__yjEvents.call(
|
||||||
|
'library.Library.GetAllTracks',
|
||||||
|
[],
|
||||||
|
10_000,
|
||||||
|
);
|
||||||
|
|
||||||
|
return (tracks as { FilePath: string }[]).slice(0, 4).map((t) => t.FilePath);
|
||||||
|
});
|
||||||
|
|
||||||
|
await callBinding(app, 'queue.Queue.SetQueue', [paths, 0, false]);
|
||||||
|
|
||||||
|
// A closed panel renders no list at all, so there is no row to focus.
|
||||||
|
await app.locator('#queue-button').click();
|
||||||
|
await expect(app.locator('queue-panel .track-item').first()).toBeVisible();
|
||||||
|
|
||||||
|
return order(app);
|
||||||
|
}
|
||||||
|
|
||||||
|
test.describe('reordering the queue from the keyboard', () => {
|
||||||
|
// The 36 specs share one backend process in file order, and these
|
||||||
|
// leave two things behind that outlive the page: a reordered queue
|
||||||
|
// and an open panel. Both are put back, because a spec that spends
|
||||||
|
// state fails the *next* one, in a list that reads like a regression
|
||||||
|
// in whatever you are holding.
|
||||||
|
test.afterEach(async ({ app }) => {
|
||||||
|
await callBinding(app, 'queue.Queue.Clear').catch(() => {
|
||||||
|
/* nothing queued is the state we wanted anyway */
|
||||||
|
});
|
||||||
|
|
||||||
|
const open = await app.locator('queue-panel[open]').count();
|
||||||
|
|
||||||
|
if (open > 0) await app.locator('#queue-button').click();
|
||||||
|
});
|
||||||
|
|
||||||
|
test('Alt+Arrow moves the focused row, and puts it back', async ({ app }) => {
|
||||||
|
const start = await queueFourAndOpen(app);
|
||||||
|
|
||||||
|
expect(start.length).toBe(4);
|
||||||
|
|
||||||
|
await app.locator('queue-panel .track-item').nth(1).focus();
|
||||||
|
await app.keyboard.press('Alt+ArrowUp');
|
||||||
|
await expect.poll(() => order(app)).toEqual([start[1], start[0], ...start.slice(2)]);
|
||||||
|
|
||||||
|
// Down is the direction the obvious index arithmetic gets wrong: it
|
||||||
|
// has to ask for i + 2, because i + 1 is a no-op once the row's own
|
||||||
|
// removal is accounted for. A spec that only moved up would pass
|
||||||
|
// against a build where down does nothing.
|
||||||
|
await app.keyboard.press('Alt+ArrowDown');
|
||||||
|
await expect.poll(() => order(app)).toEqual(start);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('says where the row went', async ({ app }) => {
|
||||||
|
await queueFourAndOpen(app);
|
||||||
|
|
||||||
|
await app.locator('queue-panel .track-item').nth(1).focus();
|
||||||
|
await app.keyboard.press('Alt+ArrowUp');
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
app.locator('queue-panel [role="status"]'),
|
||||||
|
).toHaveText(/Moved to position 1 of 4/);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('refuses at the ends without reordering anything', async ({ app }) => {
|
||||||
|
const start = await queueFourAndOpen(app);
|
||||||
|
|
||||||
|
await app.locator('queue-panel .track-item').first().focus();
|
||||||
|
await app.keyboard.press('Alt+ArrowUp');
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
app.locator('queue-panel [role="status"]'),
|
||||||
|
).toHaveText(/Already first/);
|
||||||
|
expect(await order(app)).toEqual(start);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The plain arrows belong to the roving tab stop, and must not reach
|
||||||
|
// the global volume binding from a focused row.
|
||||||
|
test('leaves the unmodified arrows roving', async ({ app }) => {
|
||||||
|
const start = await queueFourAndOpen(app);
|
||||||
|
|
||||||
|
await app.locator('queue-panel .track-item').first().focus();
|
||||||
|
await app.keyboard.press('ArrowDown');
|
||||||
|
|
||||||
|
const focused = await app.evaluate(
|
||||||
|
() =>
|
||||||
|
document
|
||||||
|
.querySelector('queue-panel')
|
||||||
|
?.shadowRoot?.activeElement?.getAttribute('data-index') ?? null,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect([focused, await order(app)]).toEqual(['1', start]);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
import { LitElement, html, svg, css, nothing, unsafeCSS } from 'lit';
|
import { LitElement, html, svg, css, nothing, unsafeCSS } from 'lit';
|
||||||
import { designTokens } from '../../styles/tokens.css';
|
import { designTokens } from '../../styles/tokens.css';
|
||||||
|
import { srOnly } from '../../styles/sr-only.css';
|
||||||
import {
|
import {
|
||||||
customElement,
|
customElement,
|
||||||
property,
|
property,
|
||||||
@@ -162,6 +163,16 @@ export class QueuePanel
|
|||||||
/** The row holding the roving tab stop. */
|
/** The row holding the roving tab stop. */
|
||||||
@state() private focusedIndex = 0;
|
@state() private focusedIndex = 0;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the live region says about the last keyboard reorder.
|
||||||
|
*
|
||||||
|
* Empty until there has been one — the region itself renders
|
||||||
|
* unconditionally, because a screen reader announces a *change* to a
|
||||||
|
* region it is already watching and ignores one that appears with
|
||||||
|
* its text already in it.
|
||||||
|
*/
|
||||||
|
@state() private moveAnnouncement = '';
|
||||||
|
|
||||||
private panelWidth = DEFAULT_WIDTH;
|
private panelWidth = DEFAULT_WIDTH;
|
||||||
private scrollbarDragging = false;
|
private scrollbarDragging = false;
|
||||||
|
|
||||||
@@ -221,7 +232,7 @@ export class QueuePanel
|
|||||||
return this.playlistSubmenuPopup;
|
return this.playlistSubmenuPopup;
|
||||||
}
|
}
|
||||||
|
|
||||||
static override styles = [designTokens, contextMenuStyles, exploreLinkStyles, css`
|
static override styles = [designTokens, srOnly, contextMenuStyles, exploreLinkStyles, css`
|
||||||
:host {
|
:host {
|
||||||
flex-shrink: 0;
|
flex-shrink: 0;
|
||||||
width: 0;
|
width: 0;
|
||||||
@@ -893,6 +904,19 @@ export class QueuePanel
|
|||||||
'.track-item',
|
'.track-item',
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// `focusedIndex` is the roving tab stop, and until now only the
|
||||||
|
// arrow keys moved it — so a row focused by a click or by Tab
|
||||||
|
// left it saying 0, and every key below acted on the wrong row.
|
||||||
|
// Enter played the first track in the queue from any focused
|
||||||
|
// row, which is a pre-existing bug that Alt+Arrow made visible
|
||||||
|
// by moving something. The key event knows which row it came
|
||||||
|
// from; use that.
|
||||||
|
const rowIndex = Number(row?.dataset.index ?? NaN);
|
||||||
|
|
||||||
|
if (Number.isInteger(rowIndex) && rowIndex !== this.focusedIndex) {
|
||||||
|
this.focusedIndex = rowIndex;
|
||||||
|
}
|
||||||
|
|
||||||
if (isContextMenuKey(e) && row) {
|
if (isContextMenuKey(e) && row) {
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
e.stopPropagation();
|
e.stopPropagation();
|
||||||
@@ -911,6 +935,21 @@ export class QueuePanel
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// a11y.11: reordering the queue was drag-only, so its order
|
||||||
|
// could not be changed without a mouse at all.
|
||||||
|
//
|
||||||
|
// This has to come before `nextRovingIndex`, which switches on
|
||||||
|
// `e.key` and does not look at the modifiers — so Alt+ArrowUp
|
||||||
|
// already moved the roving focus, and would have gone on doing
|
||||||
|
// that *as well* as moving the row.
|
||||||
|
if (e.altKey && (e.key === 'ArrowUp' || e.key === 'ArrowDown')) {
|
||||||
|
e.preventDefault();
|
||||||
|
e.stopPropagation();
|
||||||
|
this.moveFocusedRow(e.key === 'ArrowUp' ? -1 : 1, count);
|
||||||
|
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const next = nextRovingIndex(e.key, this.focusedIndex, count);
|
const next = nextRovingIndex(e.key, this.focusedIndex, count);
|
||||||
|
|
||||||
if (next === null) return;
|
if (next === null) return;
|
||||||
@@ -927,6 +966,48 @@ export class QueuePanel
|
|||||||
);
|
);
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Move the focused row one position, and say where it went.
|
||||||
|
*
|
||||||
|
* It moves the *focused* row rather than the selection, which the
|
||||||
|
* drag path uses: the keyboard model already keeps those in step
|
||||||
|
* (every roving move re-selects the row it lands on), and "Alt+Down
|
||||||
|
* moved four rows you cannot see" is not a thing to do without an
|
||||||
|
* undo.
|
||||||
|
*
|
||||||
|
* The asymmetry in the target index is `MoveQueueTracks`'s, not
|
||||||
|
* ours. `toIndex` is an index into the array *before* the move, so
|
||||||
|
* moving down by one has to ask for `i + 2`: `i + 1` is where the
|
||||||
|
* row already is once you account for its own removal, and the
|
||||||
|
* backend's contiguous-block guard correctly treats it as a no-op.
|
||||||
|
*/
|
||||||
|
private moveFocusedRow(delta: -1 | 1, count: number): void {
|
||||||
|
const from = this.focusedIndex;
|
||||||
|
const to = from + delta;
|
||||||
|
|
||||||
|
if (to < 0 || to >= count) {
|
||||||
|
this.moveAnnouncement =
|
||||||
|
delta < 0
|
||||||
|
? 'Already first in the queue'
|
||||||
|
: 'Already last in the queue';
|
||||||
|
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
this.queue.moveTracksInQueue([from], delta < 0 ? to : from + 2);
|
||||||
|
|
||||||
|
this.focusedIndex = to;
|
||||||
|
this.selection.handleContextMenu(String(to));
|
||||||
|
this.moveAnnouncement = `Moved to position ${to + 1} of ${count}`;
|
||||||
|
|
||||||
|
void focusRovingRow(
|
||||||
|
this,
|
||||||
|
this.virtualizer,
|
||||||
|
to,
|
||||||
|
(i) => `.track-item[data-index="${i}"]`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
private onContextMenuAction(action: string) {
|
private onContextMenuAction(action: string) {
|
||||||
const indices =
|
const indices =
|
||||||
this.selection.getSelectedIndices();
|
this.selection.getSelectedIndices();
|
||||||
@@ -1569,6 +1650,9 @@ export class QueuePanel
|
|||||||
: ''}"
|
: ''}"
|
||||||
@mousedown=${this.handleMouseDown}
|
@mousedown=${this.handleMouseDown}
|
||||||
></div>
|
></div>
|
||||||
|
<div class="sr-only" role="status" aria-live="polite">
|
||||||
|
${this.moveAnnouncement}
|
||||||
|
</div>
|
||||||
<div class="header">
|
<div class="header">
|
||||||
<h3>Queue</h3>
|
<h3>Queue</h3>
|
||||||
<div class="header-actions">
|
<div class="header-actions">
|
||||||
|
|||||||
@@ -0,0 +1,172 @@
|
|||||||
|
/**
|
||||||
|
* `a11y.11` — the queue's order can be changed without a mouse.
|
||||||
|
*
|
||||||
|
* Reproduced in the running app first: with a queue row focused, five
|
||||||
|
* plausible combinations (Alt/Ctrl/Shift/Meta + arrows) all left the
|
||||||
|
* order untouched, because reordering existed only as a drag whose drop
|
||||||
|
* index is computed from the cursor's Y position.
|
||||||
|
*
|
||||||
|
* The arithmetic is what these pin. `MoveQueueTracks`'s `toIndex` is an
|
||||||
|
* index into the array *before* the move, so up-by-one and down-by-one
|
||||||
|
* are not symmetric: up asks for `i - 1` and down has to ask for
|
||||||
|
* `i + 2`, because `i + 1` is where the row already is once its own
|
||||||
|
* removal is accounted for — and the backend's contiguous-block guard
|
||||||
|
* correctly treats that as a no-op. A fix written to look symmetric
|
||||||
|
* silently does nothing in one direction.
|
||||||
|
*/
|
||||||
|
import { describe, expect, it, beforeEach } from 'vitest';
|
||||||
|
|
||||||
|
import '@components/queue-panel/queue-panel';
|
||||||
|
import type { QueuePanel } from '@components/queue-panel/queue-panel';
|
||||||
|
import { Events } from '../../src/events';
|
||||||
|
import { emit, calls, flush, lastArgs } from '@test/support/harness';
|
||||||
|
import { fixture, shadow, shadowAll } from '@test/support/render';
|
||||||
|
import type { QueueTrack } from '@store/queue-store';
|
||||||
|
|
||||||
|
function queueTrack(n: number, title: string): QueueTrack {
|
||||||
|
return {
|
||||||
|
id: n,
|
||||||
|
audioFileId: n,
|
||||||
|
filePath: `/music/${n}.mp3`,
|
||||||
|
position: n,
|
||||||
|
title,
|
||||||
|
artist: 'Artist',
|
||||||
|
album: 'Album',
|
||||||
|
coverArtPath: '',
|
||||||
|
artistMbid: '',
|
||||||
|
releaseGroupMbid: '',
|
||||||
|
recordingMbid: '',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const TRACKS = ['First', 'Second', 'Third', 'Fourth'].map((t, i) =>
|
||||||
|
queueTrack(i + 1, t),
|
||||||
|
);
|
||||||
|
|
||||||
|
type Panel = QueuePanel;
|
||||||
|
|
||||||
|
async function panelWithQueue(): Promise<Panel> {
|
||||||
|
const el = await fixture<Panel>('queue-panel', { open: true });
|
||||||
|
|
||||||
|
emit(Events.QueueChanged, {
|
||||||
|
tracks: TRACKS,
|
||||||
|
currentIndex: 0,
|
||||||
|
shuffleMode: false,
|
||||||
|
repeatMode: 'off',
|
||||||
|
sourcePlaylistId: 0,
|
||||||
|
});
|
||||||
|
await flush();
|
||||||
|
await el.updateComplete;
|
||||||
|
await new Promise((r) => {
|
||||||
|
requestAnimationFrame(() => r(null));
|
||||||
|
});
|
||||||
|
|
||||||
|
return el;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Press a key *from a row*, the way the delegated handler receives it.
|
||||||
|
*
|
||||||
|
* The index comes off the event's own row rather than from the
|
||||||
|
* component's `focusedIndex`, which is the fix for a pre-existing bug:
|
||||||
|
* only the arrow keys used to move that field, so a row reached by a
|
||||||
|
* click or by Tab left it saying 0 and every key acted on the wrong row.
|
||||||
|
*/
|
||||||
|
function pressFrom(el: Panel, index: number, key: string, alt: boolean) {
|
||||||
|
const row = shadowAll(el, `.track-item[data-index="${index}"]`)[0];
|
||||||
|
|
||||||
|
row?.dispatchEvent(
|
||||||
|
new KeyboardEvent('keydown', { key, altKey: alt, bubbles: true }),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
const live = (el: Panel) =>
|
||||||
|
shadow(el, '[role="status"]')?.textContent?.trim() ?? '';
|
||||||
|
|
||||||
|
describe('<queue-panel> keyboard reorder', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
emit(Events.QueueChanged, {
|
||||||
|
tracks: [],
|
||||||
|
currentIndex: -1,
|
||||||
|
shuffleMode: false,
|
||||||
|
repeatMode: 'off',
|
||||||
|
sourcePlaylistId: 0,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('moves a row up by one', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 2, 'ArrowUp', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[2], 1]);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The asymmetry, pinned. `[[1], 2]` would be the symmetric-looking
|
||||||
|
// version and is precisely the no-op the backend guards against.
|
||||||
|
it('moves a row down by one, past its own removal', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 1, 'ArrowDown', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[1], 3]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('acts on the row the key came from, not the last one arrowed to', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 3, 'ArrowUp', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect(lastArgs('queue.Queue.MoveQueueTracks')).toEqual([[3], 2]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('says where the row went', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 2, 'ArrowUp', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect(live(el)).toBe('Moved to position 2 of 4');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses at the ends, and says so rather than silently doing nothing', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 0, 'ArrowUp', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
const top = live(el);
|
||||||
|
|
||||||
|
pressFrom(el, 3, 'ArrowDown', true);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect([top, live(el), calls().some((c) => c.path.includes('Move'))]).toEqual(
|
||||||
|
['Already first in the queue', 'Already last in the queue', false],
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The live region has to be in the DOM before it has anything to say:
|
||||||
|
// most screen readers announce a change to a region they are already
|
||||||
|
// watching and ignore one that appears with its content already in it.
|
||||||
|
it('has the live region mounted and empty before any move', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
expect([shadow(el, '[role="status"]') !== null, live(el)]).toEqual([
|
||||||
|
true,
|
||||||
|
'',
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Without the modifier the same keys must still rove, and must not
|
||||||
|
// reach the global volume binding.
|
||||||
|
it('leaves the plain arrows as a roving move', async () => {
|
||||||
|
const el = await panelWithQueue();
|
||||||
|
|
||||||
|
pressFrom(el, 0, 'ArrowDown', false);
|
||||||
|
await el.updateComplete;
|
||||||
|
|
||||||
|
expect(calls().some((c) => c.path.includes('Move'))).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user