Compare commits

..
Author SHA1 Message Date
logan 92665cc872 feat(android): the phone's "More" is a bottom sheet
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m30s
CI / e2e (pull_request) Successful in 9m44s
The tab bar's fifth item opened `<app-sidebar>` in a `wa-drawer`
sliding in from the side, which is a desktop shape put on a phone: a
200px column of a 424px screen, opening away from the thumb that asked
for it, with the rest of its 400px band empty. It also had three nested
scrollers in it -- the dialog, its body, and the sidebar's own
`overflow-y: auto` host -- so which box a drag moved depended on where
the finger landed, which is the "only part of the screen scrolls under
my finger" in the report.

It is the same element with `placement="bottom"` and `without-header`,
so the surface is the sheet #60 already built rather than a second
pattern: a `wa-drawer` is a native `<dialog>` opened with `showModal()`,
which is exactly the top layer that finding rests on, so the focus
trap, Escape, tap-outside and `wa-after-hide` come along unchanged and
nothing new has to be proved about paint containment.

The sidebar is still mounted rather than re-listed as data, because the
shell's own copy is `display: none` below 600px rather than removed --
a second list drawing `nav-*` handles is the duplicate-testid failure
this component already renders conditionally to avoid. What `expanded`
means had to grow to say the host owns the *box*: `app-sidebar` writes
an inline width and caps itself at 400px, which beats any rule the host
could write, so the width, the scrolling and the mouse-only resize
handle now follow that attribute. The rows are 48px below 600px, stated
in the sidebar's own stylesheet since that is the only place it renders
there.

Measured in the running app at 424x439: the sheet is 424 wide, 373 tall
(85vh, so there is an outside to tap), rows 48px, one scroller with
`overscroll-behavior: contain`, and Settings' row reachable at the end
of it. Desktop and Compact are untouched.

Closes #71
2026-08-23 07:50:03 -04:00
22 changed files with 511 additions and 793 deletions
+1 -8
View File
@@ -158,7 +158,7 @@ only climb when it cannot.
| You changed | Run | Cost |
|---|---|---|
| A Lit component, a store, the shortcut service | `make ui-test` | ~2 s, no app |
| …and it renders differently | `make ui-visual` | + 10 baselines, opt-in, never gates |
| …and it renders differently | `make ui-visual` | + 6 baselines, opt-in |
| Any Go code | `make test` | 3 passes, ~2 min |
| A service that emits events | `make test` — assert on the payload, see `backend/queue/emit_test.go` | in-process, no app |
| A bound method or a bound struct field | `make bindings` then `make ui-test` | ~1.5 s + 2 s |
@@ -180,13 +180,6 @@ less than it looks.)
Two rules about climbing:
- **If you moved a component's geometry, run `make ui-visual` and
refresh that component's baseline in the same commit.** Nothing else
will: it is the one tier in this repo no hook and no CI job runs, and
it cannot be one — its references are machine-specific, measured in
[references/ui-tier.md](references/ui-tier.md). Four of them drifted
across three merges before anyone noticed (#196). Read the image;
never bless a reference you did not cause.
- **A component test passing is not the app rendering.** If you touched
anything in `frontend/src`, verify it in the real app too — start it
headless, `screenshot --filename=/tmp/shot.png`, and *read the PNG*.
@@ -78,58 +78,9 @@ synchronously.
Microtasks and not a timer, deliberately: a timer hangs forever under
the suites that install fake ones.
## The visual tier does not gate, and that is measured (#196)
`make ui-visual` is the same suite with nine `toMatchScreenshot`
baselines switched on. **Nothing runs it but a person**, deliberately,
and the reason is a number rather than a preference: the committed
baselines were recorded on Arch, and replayed in a bare `ubuntu:24.04`
container — CI's `check` image — three of them fail for reasons that
have nothing to do with any component.
| baseline | Arch | ubuntu:24.04 |
|---|---|---|
| `page-header` filtered-by-search | passes | ratio 0.03 differ, against a 0.02 allowance |
| `track-info` | passes | ratio 0.03 differ |
| `seek-bar` | 1152×18 | 1152×17 |
The two references that were genuinely stale did not even agree about
their *new* size — `now-playing` renders 1152×65 on Arch and 1152×64 in
the container. So moving CI's `check` job from `make ui-test` to
`make ui-visual` is not a one-line change: it needs a second,
container-recorded baseline set, which every local run would then fail
against. That is the same trap the other way round, and a pre-push hook
is the same fault again — one machine's baselines against everybody
else's renderer.
So the tier stays local and opt-in, and the rule that replaces the gate
is:
- **A change that moves a component's geometry refreshes that
component's reference in the same commit, having read the image.**
Look at the PNG; the dimensions in the failure message are the cheap
half of the answer.
- **Never refresh a reference you did not cause.** #196 exists because
four of them drifted across three unrelated merges, and every red run
made the next person likelier to stop running the tier than to read
it.
- **State the world the shot is taken in.** The stores are singletons,
so a visual case that sets nothing photographs whatever the previous
case left behind — which is how the sidebar's baseline came to have
Tracks lit and `now-playing`'s to be playing from a dynamic mix.
- **Record one file with `make ui-visual-update UI_ARGS=<path>`**, and
check `git status` before committing either way. That filter is only
honoured since #204: the recipe was a bare `--update`, and vitest
takes the following positional as the flag's value, so the path was
swallowed and *every* baseline was re-recorded — blessing any stale
one in silence.
What the tier is worth, for the record: it is a *layout* check, blind to
colour (the component tier has no `:root`, so it renders the fallbacks —
`make ui-visual` passed unchanged through a whole palette rewrite,
twice), and it has caught one thing nothing else could — swapping
`library-status-indicator`'s `<button>` for a `<span>` lost the UA
stylesheet's `box-sizing` and grew the badge 36→38px.
Visual baselines are font-hinting and compositing sensitive, which is
why they are opt-in: they only mean anything on the machine that
recorded them.
## Bindings
+36 -75
View File
@@ -262,26 +262,6 @@ real store code. A binding carries an **ID**, not a name
`yellowjacket/backend/home.Service.GetShelves`), so the fake derives
that map from the generated tree rather than writing it down.
**A test file does not get its own origin, so `setup.ts` clears
`localStorage` between tests.** `@vitest/browser-playwright` opens one
BrowserContext per session and runs several files in it one after
another, so everything a component persists — the track list's sort and
column widths, the cover size, `now-playing`'s scroll mode — is still
there when the next file mounts the same component. Which files share a
tab, and in what order, changes run to run, so the symptom is a spec
that fails about one test in three and passes every time it is run on
its own: #138 cost three scheduled runs, one of them a PR whose diff
held no frontend code at all. Measured on the build before the fix, a
single full run started **24** tests with storage already set. Two
things follow. The clear is safe precisely because the leak is
sequential — files in a session do not overlap, so it cannot wipe
storage a concurrently-running file is in the middle of using — and it
belongs in `setup.ts` rather than in the specs that write, because the
spec that *reads* is never the one that knows. And a spec whose
assertion depends on an order still **states that order** rather than
inheriting a default, or the next change to a default is the same
mystery again.
**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so
the pre-commit codegen check does not cover it. `make bindings-check`
(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook)
@@ -1113,9 +1093,9 @@ not the fix and cannot be: that function is the `document` listener for
`navigate`, so it is an infinite loop.
**It is a store rather than an event, because a component that mounts
after a navigation still has to know.** `bottom-nav`'s "More" drawer
after a navigation still has to know.** `bottom-nav`'s "More" sheet
creates its `<app-sidebar>` on open, and that copy had heard no
`navigate` at all — standing on Albums, the drawer opened highlighting
`navigate` at all — standing on Albums, it opened highlighting
Home. An event has no answer for a listener that was not there.
**A detail view is not a view here**, so the destination it was opened
@@ -1216,7 +1196,7 @@ and then vanishing.
than a general rule about phones.** `PHONE_COLUMN_IDS` is the precedent
for "what a phone shows is a different question", and it would apply —
except that `bottom-nav`'s "More" opens the *same* `<app-sidebar>`,
which filters, so an unfiltered bar would contradict its own drawer one
which filters, so an unfiltered bar would contradict its own sheet one
tap away. Which four tabs is still plan 016's committed subset; this
only removes from it, and "More" is never filtered because it is how
everything else stays reachable.
@@ -1810,7 +1790,7 @@ listing the destinations again — but rendering it unconditionally put a
second copy of every `data-testid="nav-*"` in the DOM, and 30 existing
specs failed with "strict mode violation: resolved to 2 elements" on a
desktop viewport where the element is not even visible. It renders only
while the drawer is open, and `bottom-nav.test.ts` asserts its absence
while the sheet is open, and `bottom-nav.test.ts` asserts its absence
before that.
**The tab bar is four destinations and a way to the rest.** Three to
@@ -1819,6 +1799,33 @@ is 32px each. Which four is plan 016's committed subset, and everything
else — Settings included, because a phone still needs it — is behind
"More".
**And "More" rises from the bottom, on #60's sheet rather than a
second one** (#71). It was a `wa-drawer placement="start"`: a 200px
column of a 424px screen, opening away from the thumb that asked for
it, with the rest of its 400px band empty. It is the *same element*
with `placement="bottom"` and `without-header`, which is what keeps
the change to where it comes from — `wa-drawer` renders a native
`<dialog>` and opens it with `showModal()`, so #60's containment
finding carries over with nothing new to prove, and the focus trap,
Escape, tap-outside and `wa-after-hide` all come along. Measured at
424x439: 424 wide, 373 tall (85vh, so there is an outside to tap),
48px rows.
Three things about it are load-bearing. **The sidebar is mounted
rather than re-listed as data**, which the issue offers as the
alternative: the shell's own `<app-sidebar>` is `display: none` below
600px rather than removed, so a second list drawing `nav-*` handles is
the duplication above, and it would be a second place to add the next
view to. **There is one scroller, and it is the sheet's body** — the
reported "only part of the screen scrolls under my finger" is three
nested ones (the dialog, its body, and the sidebar's own
`overflow-y: auto` host), so which box a drag moves depends on where
the finger landed; `overscroll-behavior: contain` is the other half.
And **`expanded` means the host owns the box, not just the labels**:
`app-sidebar` writes an *inline* width and caps itself at 400px, which
beats any rule the host could write, so the width, the scrolling and
the mouse-only resize handle all follow that attribute.
**There are three supported size bands, and the queue is part of the
promise.** Plan 018 (#24) wrote them down: **Phone** below 600 (bottom
nav, reflows, fits 320px exactly), **Compact** 600899 (icon sidebar),
@@ -2569,38 +2576,11 @@ Five things about it are load-bearing, and four of them fail silently:
correctly. Confidently wrong is worse than absent here, which is the
same rule `Known` exists for.
One gap this did not close and #104 did: **`dhowden/tag` has no RIFF
reader**, so nothing the tag writer put in a WAV's `id3 ` chunk was
visible to `metadata.ExtractTags` — not the totals and not the title
either, on files the app itself had just tagged. `wav_test.go` read
that chunk itself, which is why no test noticed: a round trip asserted
through the writer's own parser is a test of the writer.
`backend/riff` is where the container is now read, and it is its own
package because the alternative is an import cycle — `tagwriter`
imports `metadata`, so `metadata` cannot reach back for `parseRIFF`.
`backend/tagtotals` is the precedent.
Three things about it are load-bearing. **The two readers are
deliberately different**: `Parse` holds every chunk in memory, which is
what rewriting a file needs, and a WAV's audio *is* a chunk — so the
scan path uses `ID3Chunk`, which seeks over what it is not looking for.
**The container decides, before `tag.ReadFrom` rather than after it
fails**, because that library's last resort is an ID3v1 trailer and a
WAV carrying both would otherwise be read by the wrong one. And **an
untagged WAV is a file with no tags, not a file with a problem**: no
chunk, an RF64 container or a tag holding no frames all read as empty
metadata with no `TagReadWarning`, since the scanner's filename
fallback is the right answer and a warning would put a fault on a file
that has none.
The gap was pinned by a test that said so, which failed the moment the
reader learned and carried the instructions for what to update in its
own comment. So it is deleted, `TestFixturesMatchManifest` no longer
skips `wav`, and `totals_test.go`'s WAV case goes through
`metadata.ExtractTags` like the other three formats. The fixture
library's two WAV tracks scan with their tags and their cover now,
which is a change to what every seeded tier sees.
One gap this did not close, and it is older: **`dhowden/tag` has no
RIFF reader**, so nothing the tag writer puts in a WAV's `id3 ` chunk
is visible to `metadata.ExtractTags` — not the totals and not the title
either. `wav_test.go` reads that chunk itself, which is why no test
ever noticed.
**The absence is what gets marked, not the presence.** The tracklist
put a green tick against every owned track and a legend underneath
@@ -3471,25 +3451,6 @@ Pre-commit hooks verify generated code is fresh — always run `make generate` a
Tests use `database.NewTestDB(t)` for in-memory SQLite, built by the same
`applySchema` production uses so the two cannot diverge. Test audio fixtures live in `test_data/music_library_test/`. Table-driven tests are the norm.
**`make ui-visual` is the one tier nothing but a person runs, and it
cannot become one.** Its ten `toMatchScreenshot` baselines were recorded
on a developer's Arch box; replayed in a bare `ubuntu:24.04` container
— CI's `check` image — three of them fail on font metrics and
compositing alone (`track-info` and one `page-header` shot at a 0.03
mismatch ratio against a 0.02 allowance, `seek-bar` one pixel shorter),
and two components disagree about their own height between the two
machines. So CI keeps running `make ui-test`, which is the same suite
with the comparisons off, and a pre-push hook would be the same fault
with the machines swapped. What replaces the gate is a rule, in
`.pi/skills/yellowjacket-dev/references/ui-tier.md`: **a change that
moves a component's geometry refreshes that component's baseline in the
same commit, having read the image, and never one it did not cause**.
That is #196, which was four stale references accumulated across three
unrelated merges — a red tier nobody could read, which is how it stayed
red. A visual case must also **state the world it photographs**, since
the stores are singletons and a case that sets nothing records whatever
the previous one left in them.
## Git Workflow
Feature branches and PRs are the only way in: **`main` is a protected
+2 -5
View File
@@ -160,11 +160,8 @@ ui-watch: ## Same suite, in watch mode
ui-visual: ## Run the suite including screenshot comparisons
@cd frontend && YJ_VISUAL=1 npx vitest run $(UI_ARGS)
# `--update=true`, never a bare `--update`: vitest takes the following
# positional as the flag's value, so `--update <path>` swallows the path
# and re-records every baseline in the repo instead of the one named.
ui-visual-update: ## Re-record the screenshot baselines (UI_ARGS=<path> to filter)
@cd frontend && YJ_VISUAL=1 npx vitest run --update=true $(UI_ARGS)
ui-visual-update: ## Re-record the screenshot baselines
@cd frontend && YJ_VISUAL=1 npx vitest run --update $(UI_ARGS)
ui-setup: ## Install the Vitest browser provider's own Chromium (once)
@cd frontend && pnpm install && npx playwright install chromium
-8
View File
@@ -74,14 +74,6 @@ func ExtractTags(path string) (*TrackMetadata, error) {
// ExtractTagsFromReader reads metadata from an io.ReadSeeker.
func ExtractTagsFromReader(r io.ReadSeeker) (*TrackMetadata, error) {
// The container decides, so this is asked before tag.ReadFrom and
// not after its failure: a WAV's tags live in a RIFF chunk that
// dhowden/tag cannot see, and its fallback -- an ID3v1 trailer --
// would otherwise outrank them.
if meta, ok := wavTags(r); ok {
return meta, nil
}
m, err := tag.ReadFrom(r)
if err != nil {
// No tags found is not necessarily an error - return empty metadata
-68
View File
@@ -1,68 +0,0 @@
package metadata
import (
"bytes"
"errors"
"fmt"
"io"
"strings"
"yellowjacket/backend/riff"
)
// wavTags reads the ID3v2 tag a WAV carries in its RIFF "id3 " chunk,
// which is where backend/tagwriter puts it and where dhowden/tag --
// having no RIFF reader at all -- cannot look. Without this a WAV
// scans as an untagged file however carefully it was tagged.
//
// ok is false when r is not a RIFF/WAVE container, and the read
// position is restored either way so the caller can carry on.
func wavTags(r io.ReadSeeker) (*TrackMetadata, bool) {
start, err := r.Seek(0, io.SeekCurrent)
if err != nil {
return nil, false
}
id3Data, chunkErr := riff.ID3Chunk(r)
if _, err := r.Seek(start, io.SeekStart); err != nil {
return nil, false
}
switch {
case chunkErr == nil:
return wavTagsFrom(id3Data), true
// Not ours to read: let the ordinary dispatch have the file.
case errors.Is(chunkErr, riff.ErrNotRIFF), errors.Is(chunkErr, riff.ErrNotWAVE):
return nil, false
// A RIFF container we cannot get a tag out of -- no chunk, an RF64
// file, a truncated header. That is a file with no readable tags,
// which is what the scanner's filename fallback is for.
default:
return &TrackMetadata{}, true
}
}
// wavTagsFrom parses the bytes of a WAV's ID3v2 chunk.
func wavTagsFrom(id3Data []byte) *TrackMetadata {
meta, err := extractID3v2Lenient(bytes.NewReader(id3Data))
if err != nil {
// A tag holding no frames is not a damaged tag: writing every
// field back out empty leaves one, and warning about it would
// put a fault on a file that has none.
if errors.Is(err, ErrTagsUnreadable) {
return &TrackMetadata{}
}
return &TrackMetadata{
TagReadWarning: fmt.Errorf("%w: %w", ErrTagsUnreadable, err),
}
}
// extractID3v2Lenient names MP3, being the recovery path for one.
meta.FileFormat = strings.ToUpper(strings.TrimPrefix(string(WAV), "."))
return meta
}
-183
View File
@@ -1,183 +0,0 @@
// Package riff reads the chunk layout of a RIFF/WAVE container.
//
// It exists because both halves of WAV tagging need it and neither can
// import the other: backend/tagwriter writes a WAV's tags into a RIFF
// "id3 " chunk and already imports backend/metadata, which is what has
// to read them back out. backend/tagtotals is the precedent.
//
// The two readers here are deliberately different. Parse holds every
// chunk's data in memory, which is what rewriting a file needs; a WAV's
// audio *is* the "data" chunk, so doing that on the scan path would
// read every library file in full. ID3Chunk seeks over what it is not
// looking for instead. Both walk the same headers.
package riff
import (
"bytes"
"encoding/binary"
"errors"
"fmt"
"io"
"strings"
)
// Sentinel errors describing a container this package will not read.
var (
ErrRF64NotSupported = errors.New("RF64 files are not yet supported")
ErrNotRIFF = errors.New("not a RIFF file")
ErrNotWAVE = errors.New("not a WAVE file")
ErrNoID3Chunk = errors.New("no ID3 chunk in RIFF file")
)
// Chunk holds a single RIFF sub-chunk (ID + raw data).
type Chunk struct {
ID [4]byte
Data []byte
}
// IsID3 reports whether id is that of an ID3v2 RIFF chunk. Both
// lowercase "id3 " and uppercase "ID3 " are accepted.
func IsID3(id [4]byte) bool {
return strings.ToLower(string(id[:3])) == "id3"
}
// Parse reads every RIFF sub-chunk from r, in order, starting at the
// reader's current position. It rejects RF64 files and non-WAVE
// containers with descriptive errors. The parser is lenient: it
// tolerates a missing final padding byte and ignores the declared
// RIFF size.
func Parse(r io.Reader) ([]Chunk, error) {
if err := readContainer(r); err != nil {
return nil, err
}
var chunks []Chunk
for {
id, size, err := nextHeader(r)
if errors.Is(err, io.EOF) {
break
}
if err != nil {
return nil, err
}
data := make([]byte, size)
if _, err := io.ReadFull(r, data); err != nil {
return nil, fmt.Errorf("read chunk data for %q: %w", id, err)
}
chunks = append(chunks, Chunk{ID: id, Data: data})
// Odd-length chunks have a padding byte. Lenient: if the
// read fails (e.g. EOF), just break rather than error.
if size%2 != 0 {
var pad [1]byte
if _, err := r.Read(pad[:]); err != nil {
break
}
}
}
return chunks, nil
}
// ID3Chunk returns the payload of the ID3v2 chunk of the RIFF/WAVE
// container at the reader's current position, seeking over every other
// chunk rather than reading it. It returns ErrNoID3Chunk when the
// container carries no such chunk, and leaves the read position
// unspecified either way.
func ID3Chunk(r io.ReadSeeker) ([]byte, error) {
if err := readContainer(r); err != nil {
return nil, err
}
for {
id, size, err := nextHeader(r)
if errors.Is(err, io.EOF) {
return nil, ErrNoID3Chunk
}
if err != nil {
return nil, err
}
if !IsID3(id) {
// Odd-length chunks carry a padding byte. Seeking past
// the end of the file is not an error; the next header
// read is what reports the end.
if _, err := r.Seek(int64(size)+int64(size%2), io.SeekCurrent); err != nil {
return nil, fmt.Errorf("skip chunk %q: %w", id, err)
}
continue
}
// Copied rather than allocated up front: a truncated file is
// free to declare a chunk larger than the whole of itself.
var data bytes.Buffer
if _, err := io.CopyN(&data, r, int64(size)); err != nil {
return nil, fmt.Errorf("read chunk data for %q: %w", id, err)
}
return data.Bytes(), nil
}
}
// readContainer consumes the 12-byte RIFF/WAVE header at the reader's
// current position.
func readContainer(r io.Reader) error {
var magic [4]byte
if _, err := io.ReadFull(r, magic[:]); err != nil {
return fmt.Errorf("read RIFF magic: %w", err)
}
if string(magic[:]) == "RF64" {
return ErrRF64NotSupported
}
if string(magic[:]) != "RIFF" {
return fmt.Errorf("%w: got %q", ErrNotRIFF, magic)
}
// Read (and discard) RIFF size — lenient, do not enforce.
var riffSize uint32
if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil {
return fmt.Errorf("read RIFF size: %w", err)
}
var form [4]byte
if _, err := io.ReadFull(r, form[:]); err != nil {
return fmt.Errorf("read WAVE form type: %w", err)
}
if string(form[:]) != "WAVE" {
return fmt.Errorf("%w: got %q", ErrNotWAVE, form)
}
return nil
}
// nextHeader reads one sub-chunk header. It returns io.EOF once the
// chunks are exhausted, including for a header cut short.
func nextHeader(r io.Reader) ([4]byte, uint32, error) {
var id [4]byte
_, err := io.ReadFull(r, id[:])
if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
return id, 0, io.EOF
}
if err != nil {
return id, 0, fmt.Errorf("read chunk ID: %w", err)
}
var size uint32
if err := binary.Read(r, binary.LittleEndian, &size); err != nil {
return id, 0, fmt.Errorf("read chunk size for %q: %w", id, err)
}
return id, size, nil
}
-211
View File
@@ -1,211 +0,0 @@
package riff_test
import (
"bytes"
"encoding/binary"
"errors"
"testing"
"yellowjacket/backend/riff"
)
// chunk is one sub-chunk to put in a test container.
type chunk struct {
id string
data []byte
}
// buildRIFF assembles a container from magic, form type and chunks,
// padding odd-length chunks the way a writer must.
func buildRIFF(magic, form string, chunks []chunk) []byte {
var body bytes.Buffer
body.WriteString(form)
for _, c := range chunks {
body.WriteString(c.id)
_ = binary.Write(&body, binary.LittleEndian, uint32(len(c.data)))
body.Write(c.data)
if len(c.data)%2 != 0 {
body.WriteByte(0)
}
}
var out bytes.Buffer
out.WriteString(magic)
_ = binary.Write(&out, binary.LittleEndian, uint32(body.Len()))
out.Write(body.Bytes())
return out.Bytes()
}
func TestID3Chunk_FindsTheTagPastTheAudio(t *testing.T) {
t.Parallel()
tests := []struct {
name string
chunks []chunk
want string
}{
{
name: "after an odd-length chunk",
chunks: []chunk{
{id: "fmt ", data: make([]byte, 16)},
{id: "LIST", data: []byte("INFOodd")},
{id: "data", data: make([]byte, 200)},
{id: "id3 ", data: []byte("ID3vTAG")},
},
want: "ID3vTAG",
},
{
// The chunk ID is written both ways in the wild, and the
// writer accepts either, so the reader must too.
name: "uppercase ID3",
chunks: []chunk{
{id: "data", data: make([]byte, 8)},
{id: "ID3 ", data: []byte("upper")},
},
want: "upper",
},
{
name: "first chunk",
chunks: []chunk{
{id: "id3 ", data: []byte("first")},
{id: "data", data: make([]byte, 8)},
},
want: "first",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
r := bytes.NewReader(buildRIFF("RIFF", "WAVE", tc.chunks))
got, err := riff.ID3Chunk(r)
if err != nil {
t.Fatalf("ID3Chunk: %v", err)
}
if string(got) != tc.want {
t.Errorf("chunk data: got %q, want %q", got, tc.want)
}
})
}
}
func TestID3Chunk_RejectsWhatItCannotRead(t *testing.T) {
t.Parallel()
tests := []struct {
name string
bytes []byte
want error
}{
{
name: "no ID3 chunk",
bytes: buildRIFF("RIFF", "WAVE", []chunk{{id: "data", data: []byte{1, 2}}}),
want: riff.ErrNoID3Chunk,
},
{
name: "no chunks at all",
bytes: buildRIFF("RIFF", "WAVE", nil),
want: riff.ErrNoID3Chunk,
},
{
name: "not RIFF",
bytes: []byte("ID3\x03\x00\x00\x00\x00\x00\x00\x00\x00"),
want: riff.ErrNotRIFF,
},
{
name: "not WAVE",
bytes: buildRIFF("RIFF", "AVI ", []chunk{{id: "id3 ", data: []byte("x")}}),
want: riff.ErrNotWAVE,
},
{
name: "RF64",
bytes: buildRIFF("RF64", "WAVE", []chunk{{id: "id3 ", data: []byte("x")}}),
want: riff.ErrRF64NotSupported,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
_, err := riff.ID3Chunk(bytes.NewReader(tc.bytes))
if !errors.Is(err, tc.want) {
t.Errorf("ID3Chunk error: got %v, want %v", err, tc.want)
}
})
}
}
// A file cut short mid-chunk is a file with no tag, not a reason to
// allocate the size it claims: the declared size is four bytes any
// truncation can leave saying 4 GB.
func TestID3Chunk_ToleratesATruncatedFile(t *testing.T) {
t.Parallel()
full := buildRIFF("RIFF", "WAVE", []chunk{
{id: "data", data: make([]byte, 64)},
{id: "id3 ", data: []byte("tag")},
})
t.Run("cut inside the audio", func(t *testing.T) {
t.Parallel()
_, err := riff.ID3Chunk(bytes.NewReader(full[:32]))
if !errors.Is(err, riff.ErrNoID3Chunk) {
t.Errorf("ID3Chunk error: got %v, want %v", err, riff.ErrNoID3Chunk)
}
})
t.Run("cut inside the tag", func(t *testing.T) {
t.Parallel()
if _, err := riff.ID3Chunk(bytes.NewReader(full[:len(full)-2])); err == nil {
t.Error("ID3Chunk: got nil error for a truncated tag chunk")
}
})
}
// Parse is the writer's half and reads every chunk into memory, which
// is what preserving them needs.
func TestParse_ReadsEveryChunkInOrder(t *testing.T) {
t.Parallel()
raw := buildRIFF("RIFF", "WAVE", []chunk{
{id: "fmt ", data: make([]byte, 16)},
{id: "LIST", data: []byte("INFOodd")},
{id: "id3 ", data: []byte("tag")},
})
chunks, err := riff.Parse(bytes.NewReader(raw))
if err != nil {
t.Fatalf("Parse: %v", err)
}
want := []string{"fmt ", "LIST", "id3 "}
if len(chunks) != len(want) {
t.Fatalf("chunk count: got %d, want %d", len(chunks), len(want))
}
for i, id := range want {
if got := string(chunks[i].ID[:]); got != id {
t.Errorf("chunk %d: got %q, want %q", i, got, id)
}
}
if !riff.IsID3(chunks[2].ID) || string(chunks[2].Data) != "tag" {
t.Errorf("id3 chunk: got %q", chunks[2].Data)
}
// The padding byte after an odd chunk is not part of its data.
if string(chunks[1].Data) != "INFOodd" {
t.Errorf("odd chunk data: got %q, want %q", chunks[1].Data, "INFOodd")
}
}
+4 -5
View File
@@ -13,10 +13,9 @@ import (
// indistinguishable from never having written one. So these assert the
// round trip through the *reader the scan uses*, not the bytes.
//
// WAV was the exception until #104 -- dhowden/tag has no RIFF reader,
// so metadata.ExtractTags could not see a WAV's ID3 chunk and this
// case read the chunk itself, which is a test of the writer wearing
// the shape of a round trip. All four go through the scanner now.
// WAV is the exception and it is not this change's: dhowden/tag has no
// RIFF reader at all, so metadata.ExtractTags cannot see a WAV's ID3
// chunk -- which is why every other test here reads that chunk itself.
func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) {
t.Parallel()
@@ -92,7 +91,7 @@ func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) {
},
{
name: "wav",
read: viaScanner,
read: readWavID3Tags,
write: func(t *testing.T, dir string) string {
t.Helper()
+105 -11
View File
@@ -8,22 +8,116 @@ import (
"io"
"log/slog"
"os"
"strings"
id3v2 "github.com/bogem/id3v2/v2"
"yellowjacket/backend/fileutil"
"yellowjacket/backend/riff"
)
// errFileTooLargeForWAV is the one RIFF error that belongs to the
// writer; reading rejects a container in backend/riff.
var errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)")
// Sentinel errors for WAV RIFF operations.
var (
errRF64NotSupported = errors.New("RF64 files are not yet supported")
errNotRIFF = errors.New("not a RIFF file")
errNotWAVE = errors.New("not a WAVE file")
errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)")
)
// riffChunk holds a single RIFF sub-chunk (ID + raw data).
type riffChunk struct {
id [4]byte
data []byte
}
// parseRIFF reads all RIFF sub-chunks from r. It rejects RF64 files
// and non-WAVE containers with descriptive errors. The parser is
// lenient on read: it tolerates missing padding bytes and ignores
// the declared RIFF size.
func parseRIFF(r io.ReadSeeker) ([]riffChunk, error) {
// Read 4-byte container magic.
var magic [4]byte
if _, err := io.ReadFull(r, magic[:]); err != nil {
return nil, fmt.Errorf("read RIFF magic: %w", err)
}
if string(magic[:]) == "RF64" {
return nil, errRF64NotSupported
}
if string(magic[:]) != "RIFF" {
return nil, fmt.Errorf("%w: got %q", errNotRIFF, magic)
}
// Read (and discard) RIFF size — lenient, do not enforce.
var riffSize uint32
if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil {
return nil, fmt.Errorf("read RIFF size: %w", err)
}
// Read 4-byte form type.
var form [4]byte
if _, err := io.ReadFull(r, form[:]); err != nil {
return nil, fmt.Errorf("read WAVE form type: %w", err)
}
if string(form[:]) != "WAVE" {
return nil, fmt.Errorf("%w: got %q", errNotWAVE, form)
}
// Read sub-chunks until EOF.
var chunks []riffChunk
for {
var chunkID [4]byte
_, err := io.ReadFull(r, chunkID[:])
if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
break
}
if err != nil {
return nil, fmt.Errorf("read chunk ID: %w", err)
}
var chunkSize uint32
if err := binary.Read(r, binary.LittleEndian, &chunkSize); err != nil {
return nil, fmt.Errorf("read chunk size for %q: %w", chunkID, err)
}
data := make([]byte, chunkSize)
if _, err := io.ReadFull(r, data); err != nil {
return nil, fmt.Errorf("read chunk data for %q: %w", chunkID, err)
}
chunks = append(chunks, riffChunk{id: chunkID, data: data})
// Odd-length chunks have a padding byte. Lenient: if the
// read fails (e.g. EOF), just break rather than error.
if chunkSize%2 != 0 {
var pad [1]byte
if _, err := r.Read(pad[:]); err != nil {
break
}
}
}
return chunks, nil
}
// isID3ChunkID returns true if id represents an ID3v2 RIFF chunk.
// Both lowercase "id3 " and uppercase "ID3 " are accepted.
func isID3ChunkID(id [4]byte) bool {
s := strings.ToLower(string(id[:3]))
return s == "id3"
}
// writeRIFF writes a complete RIFF/WAVE container to w, preserving
// the given chunks in order and appending the id3Data as the final
// "id3 " chunk. Returns errFileTooLargeForWAV if the result would
// exceed the 4 GB RIFF limit.
func writeRIFF(w io.Writer, chunks []riff.Chunk, id3Data []byte) error {
func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error {
// Calculate total RIFF payload size:
// 4 bytes (WAVE form type)
// + for each preserved chunk: 8 (header) + len(data) + padding
@@ -31,7 +125,7 @@ func writeRIFF(w io.Writer, chunks []riff.Chunk, id3Data []byte) error {
riffPayload := uint64(4)
for _, c := range chunks {
sz := uint64(len(c.Data))
sz := uint64(len(c.data))
riffPayload += 8 + sz
if sz%2 != 0 {
@@ -68,7 +162,7 @@ func writeRIFF(w io.Writer, chunks []riff.Chunk, id3Data []byte) error {
// Write each preserved chunk.
for _, c := range chunks {
if err := writeChunk(w, c.ID, c.Data); err != nil {
if err := writeChunk(w, c.id, c.data); err != nil {
return err
}
}
@@ -131,7 +225,7 @@ func writeWavTags(
return fmt.Errorf("open wav for reading: %w", err)
}
allChunks, err := riff.Parse(f)
allChunks, err := parseRIFF(f)
// Close immediately — we need the handle released before
// AtomicWrite creates the replacement file.
@@ -143,13 +237,13 @@ func writeWavTags(
// Separate preserved chunks from existing ID3 data.
var (
preserved []riff.Chunk
preserved []riffChunk
existingID3 []byte
)
for _, c := range allChunks {
if riff.IsID3(c.ID) {
existingID3 = c.Data
if isID3ChunkID(c.id) {
existingID3 = c.data
} else {
preserved = append(preserved, c)
}
+17 -103
View File
@@ -12,7 +12,6 @@ import (
id3v2 "github.com/bogem/id3v2/v2"
"yellowjacket/backend/metadata"
"yellowjacket/backend/riff"
)
// createTestWAV builds a minimal valid WAV file with an optional
@@ -271,88 +270,6 @@ func TestWriteWavTags_PartialUpdate(t *testing.T) {
assertStrField(t, "Composer", meta.Composer, "Original Composer")
}
// The writer has always been correct and the reader could not see it:
// a WAV tagged by this app scanned as an untagged file, so editing
// tags, autotagging a folder or importing a WAV download all appeared
// to work and changed nothing the library could show (#104). So this
// asserts the write through metadata.ExtractTags -- the reader the
// scan uses -- rather than through the id3 chunk.
func TestWriteWavTags_ReadBackByTheScanner(t *testing.T) {
t.Parallel()
dir := t.TempDir()
path := createTestWAV(t, dir, "scanner.wav", nil)
art := tinyJPEG(t)
changes := TagChanges{
FieldTitle: "Some Song",
FieldArtist: "Some Artist",
FieldAlbum: "Some Album",
FieldAlbumArtist: "Some Album Artist",
FieldGenre: "Rock",
FieldYear: 2024,
FieldTrackNumber: 3,
FieldComposer: "Some Composer",
FieldCoverArt: art,
}
if err := writeWavTags(testLogger(), path, changes); err != nil {
t.Fatalf("writeWavTags: %v", err)
}
meta, err := metadata.ExtractTags(path)
if err != nil {
t.Fatalf("ExtractTags: %v", err)
}
if meta.TagReadWarning != nil {
t.Errorf("TagReadWarning: %v", meta.TagReadWarning)
}
assertStrField(t, "Title", meta.Title, "Some Song")
assertStrField(t, "Artist", meta.Artist, "Some Artist")
assertStrField(t, "Album", meta.Album, "Some Album")
assertStrField(t, "AlbumArtist", meta.AlbumArtist, "Some Album Artist")
assertStrField(t, "Genre", meta.Genre, "Rock")
assertStrField(t, "Composer", meta.Composer, "Some Composer")
assertStrField(t, "FileFormat", meta.FileFormat, "WAV")
assertIntField(t, "Year", meta.Year, 2024)
assertIntField(t, "TrackNumber", meta.TrackNumber, 3)
if !strings.HasPrefix(meta.TagFormat, "ID3v2") {
t.Errorf("TagFormat: got %q, want an ID3v2 version", meta.TagFormat)
}
if meta.Picture == nil {
t.Fatal("expected cover art, got nil")
}
if !bytes.Equal(meta.Picture.Data, art) {
t.Errorf("picture data mismatch: got %d bytes, want %d",
len(meta.Picture.Data), len(art))
}
}
// An untagged WAV is a file with no tags, not a file with a problem:
// the scanner falls back to the filename and must not be handed a
// warning to surface about it.
func TestUntaggedWav_ReadsAsEmptyWithoutAWarning(t *testing.T) {
t.Parallel()
path := createTestWAV(t, t.TempDir(), "bare.wav", nil)
meta, err := metadata.ExtractTags(path)
if err != nil {
t.Fatalf("ExtractTags: %v", err)
}
if meta.TagReadWarning != nil {
t.Errorf("TagReadWarning: %v", meta.TagReadWarning)
}
assertStrField(t, "Title", meta.Title, "")
}
func TestWriteWavTags_ChunkPreservation(t *testing.T) {
t.Parallel()
@@ -365,7 +282,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
t.Fatalf("open original: %v", err)
}
origChunks, err := riff.Parse(origFile)
origChunks, err := parseRIFF(origFile)
_ = origFile.Close()
if err != nil {
@@ -375,7 +292,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
// Record original chunk data by ID string.
origData := map[string][]byte{}
for _, c := range origChunks {
origData[string(c.ID[:])] = c.Data
origData[string(c.id[:])] = c.data
}
// Write a tag to trigger RIFF rewrite.
@@ -392,7 +309,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
t.Fatalf("open after write: %v", err)
}
newChunks, err := riff.Parse(newFile)
newChunks, err := parseRIFF(newFile)
_ = newFile.Close()
if err != nil {
@@ -403,7 +320,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
origNonID3 := 0
for _, c := range origChunks {
if !riff.IsID3(c.ID) {
if !isID3ChunkID(c.id) {
origNonID3++
}
}
@@ -411,7 +328,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
newNonID3 := 0
for _, c := range newChunks {
if !riff.IsID3(c.ID) {
if !isID3ChunkID(c.id) {
newNonID3++
}
}
@@ -442,17 +359,17 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) {
// in chunks and its data matches want byte-for-byte.
func checkChunkPreserved(
t *testing.T,
chunks []riff.Chunk,
chunks []riffChunk,
idStr string,
want []byte,
) {
t.Helper()
for _, c := range chunks {
if string(c.ID[:]) == idStr {
if !bytes.Equal(c.Data, want) {
if string(c.id[:]) == idStr {
if !bytes.Equal(c.data, want) {
t.Errorf("chunk %q data changed: got %d bytes, want %d",
idStr, len(c.Data), len(want))
idStr, len(c.data), len(want))
}
return
@@ -513,7 +430,7 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) {
buf.WriteString("WAVE")
// Minimal ds64 chunk (required for RF64 but we just need
// enough bytes for riff.Parse to hit the RF64 rejection).
// enough bytes for parseRIFF to hit the RF64 rejection).
buf.WriteString("ds64")
_ = binary.Write(&buf, binary.LittleEndian, uint32(28)) //nolint:mnd
buf.Write(make([]byte, 28)) //nolint:mnd
@@ -537,12 +454,9 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) {
// readWavID3Tags extracts ID3v2 metadata from a WAV file by parsing
// the RIFF structure and reading the id3 chunk with bogem/id3v2.
//
// metadata.ExtractTags reads a WAV since #104 and is what the round
// trips assert through. This stays for the two cases that are about
// the bytes rather than about the scan: a tag with every frame
// cleared, which no reader reports as anything, and the chunk
// preservation test, which is already parsing the container itself.
// dhowden/tag's ReadFrom does not support WAV files, and its
// ReadID3v2Tags fails on empty tags (after clearing all frames).
// Using bogem/id3v2.ParseReader handles all cases correctly.
func readWavID3Tags(
t *testing.T,
path string,
@@ -556,17 +470,17 @@ func readWavID3Tags(
defer func() { _ = f.Close() }()
chunks, err := riff.Parse(f)
chunks, err := parseRIFF(f)
if err != nil {
t.Fatalf("riff.Parse: %v", err)
t.Fatalf("parseRIFF: %v", err)
}
// Find the id3 chunk.
var id3Data []byte
for _, c := range chunks {
if riff.IsID3(c.ID) {
id3Data = c.Data
if isID3ChunkID(c.id) {
id3Data = c.data
break
}
+52
View File
@@ -98,6 +98,58 @@ test.describe('the shell on a phone', () => {
).toBeVisible();
});
test('draws "More" as a sheet on the bottom edge (#71)', async ({ app }) => {
await app.getByTestId('tab-more').click();
await expect(app.getByTestId('nav-drawer').locator('app-sidebar'))
.toBeVisible();
// What the report is about is geometry, and geometry is what no
// other assertion here can see: the side drawer was a 200px column
// opening away from the thumb that asked for it, with the rest of
// its 400px band empty. Measured rather than screenshotted, since
// the failure is a number.
//
// Polled, because a sheet *arrives*: the drawer's show animation
// translates it a full height below the fold, so a measurement
// taken the moment its content is visible reports a box hanging
// 412px off the bottom of the screen. Asking for the settled
// number is the assertion; asking once is a race.
const measure = () => app.evaluate(() => {
const nav = document.querySelector('bottom-nav');
const drawer = nav?.shadowRoot?.querySelector('wa-drawer');
const dialog = drawer?.shadowRoot?.querySelector('[part~="dialog"]');
const sidebar = nav?.shadowRoot?.querySelector('app-sidebar');
const row = sidebar?.shadowRoot?.querySelector('li button');
const box = dialog?.getBoundingClientRect();
return {
left: Math.round(box?.left ?? -1),
right: Math.round(box?.right ?? -1),
bottom: Math.round(box?.bottom ?? -1),
height: Math.round(box?.height ?? -1),
row: Math.round(row?.getBoundingClientRect().height ?? -1),
viewport: [window.innerWidth, window.innerHeight],
};
});
await expect
.poll(async () => (await measure()).bottom)
.toBe(PHONE.height);
const sheet = await measure();
expect(sheet.left).toBe(0);
expect(sheet.right).toBe(sheet.viewport[0]);
// A surface covering the whole screen is a page, not a sheet --
// which is also what leaves an outside to tap on, the only pointer
// route out of it (#171 is the same question one surface over).
expect(sheet.height).toBeLessThan(sheet.viewport[1]);
// 48px rows, from #186's touch floor and #60's context sheet.
expect(sheet.row).toBeGreaterThanOrEqual(48);
});
for (const vp of [PHONE, SMALL_PHONE]) {
test(`does not scroll sideways at ${vp.width}×${vp.height}`, async ({ app }) => {
await app.setViewportSize(vp);
@@ -27,15 +27,43 @@ interface Tab {
* three to five items before the targets stop being thumb-sized —
* 360 px over eleven sidebar entries is 32 px each — so the four here
* are the ones plan 016's subset says a phone is *for*, and "More"
* opens the existing `<app-sidebar>` in a drawer. That is deliberately
* opens the existing `<app-sidebar>` in a sheet. That is deliberately
* a reuse rather than a second nav: two lists of destinations is two
* places to add the next view to, and the sidebar already carries the
* drag-to-navigate behaviour, the active state and the labels.
*
* **"More" rises from the bottom, and it is the same sheet a context
* menu is** (#71). It was a `wa-drawer` sliding in from the side: a
* 200px column of a 424px screen, opening away from the thumb that
* asked for it, with three nested scrollers in it — the dialog, its
* body, and the sidebar's own `overflow-y: auto` host — which is the
* "only part of the screen scrolls under my finger" in the report.
*
* Three things about the replacement are load-bearing.
*
* **It is the same element with another `placement`, not a new
* surface.** `wa-drawer` renders a native `<dialog>` and opens it with
* `showModal()`, which is exactly what `menu-surface`'s sheet relies
* on — Chrome 37, the real top layer — so #60's containment finding
* carries over with nothing new to prove, and the focus trap, Escape,
* tap-outside and `wa-after-hide` all come along unchanged.
*
* **The body is the only scroller**, with `overscroll-behavior:
* contain`, and the sidebar is told to stop being one. Nesting them is
* what makes a drag scroll the wrong box.
*
* **The sidebar is still mounted rather than re-listed as data**,
* which the issue offers as an alternative. Its `data-testid` per
* destination is the reason: the shell's own sidebar is `display:
* none` below 600px rather than removed, so a second list drawing
* `nav-*` handles is the duplication this component already renders
* conditionally to avoid — and it would be a second place to add the
* next view to, with its own copy of #25's visibility filter.
*
* It emits the same bubbling, composed `navigate` event the sidebar
* does, so `index.ts` needs no knowledge of it, and it listens for that
* event globally for the same reason the sidebar does: a navigation it
* did not send (a card click, a detail view, the drawer) still has to
* did not send (a card click, a detail view, the sheet) still has to
* move the highlight.
*/
@customElement('bottom-nav')
@@ -104,15 +132,50 @@ export class BottomNav extends LitElement {
white-space: nowrap;
}
wa-drawer::part(body) {
padding: 0;
/* The sheet. --size is the drawer's own API for the axis its
placement uses, so auto is what makes it hug its content
instead of being a fixed 25rem band; the rest is the shape
the menu-surface context sheet already has, so a phone meets
one sheet rather than two. 85vh for its reason too: a surface
covering the whole screen is a page, not a sheet. */
wa-drawer {
--size: auto;
}
app-sidebar {
/* The sidebar sizes itself inline and collapses to icons
below 900px, which is every phone. In the drawer there
is room for the labels, so it is told not to. */
height: 100%;
wa-drawer::part(dialog) {
max-height: 85vh;
border-radius: 12px 12px 0 0;
/* The sidebar paints its own surface, so the sheet takes
that colour rather than the menus' elevated one: two
greys in one sheet is a seam across the middle of it. */
background-color: var(--yj-bg-surface, #212529);
/* One scroller, and it is the body below. The dialog's own
overflow: auto is what let the sheet scroll as well as
its content, and it is also what would square off the
corners this rule just rounded. */
overflow: hidden;
}
wa-drawer::part(body) {
padding: 0;
/* A scroll that reaches the end of this list must not
become a scroll of the page underneath it. */
overscroll-behavior: contain;
/* The sheet sits on the bottom edge, so the last
destination would otherwise be under the home indicator
on a gesture-navigation phone -- the same allowance the
bar itself makes above. */
padding-bottom: env(safe-area-inset-bottom, 0);
}
/* A sheet is dragged at with a thumb, so it says where its top
edge is. Decorative: the destinations are below it. */
.grip {
width: 36px;
height: 4px;
margin: 8px auto 4px;
border-radius: 2px;
background: var(--yj-text-tertiary, #888);
}
`];
@@ -147,7 +210,7 @@ export class BottomNav extends LitElement {
private visibilityCtrl = new ViewVisibilityController(this);
/**
* Whether the drawer has been asked for.
* Whether the sheet has been asked for.
*
* The sidebar inside it is rendered only while this is true, and
* that is not an optimisation. `app-sidebar` carries a
@@ -189,15 +252,17 @@ export class BottomNav extends LitElement {
override updated() {
// Web Awesome renders its heading into its own shadow root and
// never points aria-labelledby at it, so the drawer would
// never points aria-labelledby at it, so the sheet would
// otherwise be announced unnamed -- the same fix, and the same
// reason, as every wa-dialog in the app. A drawer's shadow root
// has the same shape, so the helper needs no change.
// has the same shape, so the helper needs no change; under
// `without-header` there is no heading to point at, which is
// that helper's documented `aria-label` path.
nameDialog(this.drawer);
}
private onGlobalNavigate = () => {
// A navigation from inside the drawer is the drawer's job done.
// A navigation from inside the sheet is the sheet's job done.
// The highlight is not this listener's business any more.
this.drawerOpen = false;
};
@@ -263,12 +328,14 @@ export class BottomNav extends LitElement {
</nav>
<wa-drawer
placement="start"
placement="bottom"
without-header
label="All views"
data-testid="nav-drawer"
?open=${this.drawerOpen}
@wa-after-hide=${this.onDrawerHide}
>
<div class="grip"></div>
${this.drawerOpen
? html`<app-sidebar expanded></app-sidebar>`
: nothing}
+56 -5
View File
@@ -42,6 +42,26 @@ export class AppSidebar extends LitElement {
scrollbar-width: thin;
}
/* A host that has made room owns the box, not just the labels
(#71). The bottom-nav sheet is the width of the screen and
provides the one scroll container it needs; left to itself
the sidebar is a 200px column with a second scroller inside
it, which is what a nested scroll region feels like under a
thumb -- part of the surface moves and part of it does not. */
:host([expanded]) {
max-width: none;
height: auto;
overflow: visible;
}
/* And the width is not draggable there. It is a mouse
affordance (mousedown, col-resize) sitting on the right edge
of a touch surface, where the compatibility mouse events a
tap synthesises can start a resize nobody asked for. */
:host([expanded]) .resize-handle {
display: none;
}
.resize-handle {
position: absolute;
top: 0;
@@ -145,6 +165,25 @@ export class AppSidebar extends LitElement {
:host(.collapsed) li button wa-icon {
font-size: var(--yj-icon-md);
}
/* Below 600px the only place this renders is the bottom-nav
sheet -- the shell's own copy is display: none there -- so
the rows are sized for the thumb that opened it: 48px, which
is #186's floor and the height every row in #60's context
sheet already has. A media query inside a shadow root is
answered by the viewport, so the component states this
itself rather than the sheet reaching in. */
@media (max-width: 599px) {
ul {
padding: 4px 8px 8px;
}
li button {
min-height: 48px;
padding: 12px 10px;
gap: 14px;
}
}
`];
/** Delay in ms before a drag-hover triggers navigation. */
@@ -173,11 +212,14 @@ export class AppSidebar extends LitElement {
/**
* Keep the labels regardless of the viewport, for a host that has
* made room for them -- `bottom-nav`'s drawer, which is the whole
* made room for them -- `bottom-nav`'s sheet, which is the whole
* screen wide on the phone where this would otherwise auto-collapse
* to icons. The auto-collapse is a *width* response to a narrow
* shell, and inside a drawer the shell is not what the sidebar is
* shell, and inside a sheet the shell is not what the sidebar is
* sharing space with.
*
* It says the host owns the *box*, not only the labels: the width,
* the scrolling and the resize handle all follow it (#71).
*/
@property({ type: Boolean, reflect: true })
expanded = false;
@@ -350,9 +392,18 @@ export class AppSidebar extends LitElement {
* be a media query in the stylesheet.
*/
private applyViewportWidth() {
const narrow =
!this.expanded &&
(this.narrowViewport?.matches ?? false);
// A host that made room decides how much: `bottom-nav`'s sheet
// is the whole screen wide, and the inline width below -- which
// beats any rule the host could write -- would draw the old
// 200px side drawer inside it.
if (this.expanded) {
this.style.width = '100%';
this.collapsed = false;
return;
}
const narrow = this.narrowViewport?.matches ?? false;
const width = narrow
? MIN_WIDTH
: this.userWidth;
Binary file not shown.

Before

Width:  |  Height:  |  Size: 14 KiB

After

Width:  |  Height:  |  Size: 15 KiB

Binary file not shown.

Before

Width:  |  Height:  |  Size: 6.7 KiB

After

Width:  |  Height:  |  Size: 4.4 KiB

+115
View File
@@ -193,4 +193,119 @@ describe('bottom-nav', () => {
expect(shadow<HTMLElement>(el, 'app-sidebar')?.hasAttribute('expanded'))
.toBe(true);
});
it('draws "More" as a sheet rising from the bottom', async () => {
const el = await fixture<Nav>('bottom-nav');
const drawer = shadow<HTMLElement>(el, 'wa-drawer');
// #71. A side drawer is a desktop shape: it opened away from the
// thumb that asked for it and drew a 200px column of a 424px
// screen. `placement` is the whole of the change to *where* it
// comes from, and `without-header` is what makes it the same sheet
// `menu-surface` draws rather than a second pattern with a title
// bar and a close button.
expect(drawer?.getAttribute('placement')).toBe('bottom');
expect(drawer?.hasAttribute('without-header')).toBe(true);
// Named all the same: `nameDialog`'s documented aria-label path,
// since without-header renders no heading to point at.
expect(drawer?.getAttribute('label')).toBe('All views');
});
it('leaves exactly one scroll container, and it is the sheet body', async () => {
const el = await fixture<Nav>('bottom-nav');
const drawer = shadow<HTMLElement & { open: boolean }>(el, 'wa-drawer');
if (!drawer) throw new Error('no drawer');
const shown = once(drawer, 'wa-after-show');
shadow<HTMLButtonElement>(el, '[data-testid="tab-more"]')?.click();
await shown;
await update(el, {});
// The report is "only part of the screen scrolls under my finger",
// and the cause is three boxes that each scroll: the dialog, its
// body, and the sidebar's own overflow-y host. Which one a drag
// moves depends on where the finger landed.
const dialog = drawer.shadowRoot?.querySelector('[part~="dialog"]');
const body = drawer.shadowRoot?.querySelector('[part~="body"]');
const sidebar = shadow<HTMLElement>(el, 'app-sidebar');
if (!dialog || !body || !sidebar) throw new Error('no sheet');
expect(getComputedStyle(dialog).overflowY).toBe('hidden');
expect(getComputedStyle(body).overflowY).toBe('auto');
expect(getComputedStyle(sidebar).overflowY).toBe('visible');
// And the one that does scroll keeps it to itself, or reaching the
// end of the destinations scrolls the page behind the sheet.
expect(getComputedStyle(body).overscrollBehaviorY).toBe('contain');
});
it('gives the sheet the whole width, which the sidebar does not take', async () => {
const el = await fixture<Nav>('bottom-nav');
shadow<HTMLButtonElement>(el, '[data-testid="tab-more"]')?.click();
await update(el, {});
const sidebar = shadow<HTMLElement>(el, 'app-sidebar');
if (!sidebar) throw new Error('no sidebar');
// `app-sidebar` writes an *inline* width and caps itself at 400px,
// which beats any rule this host could write — so "the host owns
// the box" has to be part of what `expanded` means, or the sheet
// draws the old 200px column inside a full-width surface.
expect(sidebar.style.width).toBe('100%');
expect(getComputedStyle(sidebar).maxWidth).toBe('none');
});
it('sizes the sheet rows for a thumb, below the phone breakpoint', async () => {
const el = await fixture<Nav>('bottom-nav');
shadow<HTMLButtonElement>(el, '[data-testid="tab-more"]')?.click();
await update(el, {});
const sidebar = shadow<HTMLElement>(el, 'app-sidebar');
const sheets = sidebar?.shadowRoot?.adoptedStyleSheets ?? [];
const phoneRules: string[] = [];
for (const sheet of sheets) {
for (const rule of Array.from(sheet.cssRules)) {
if (!(rule instanceof CSSMediaRule)) continue;
if (!/max-width:\s*599px/.test(rule.conditionText)) continue;
for (const inner of Array.from(rule.cssRules)) {
phoneRules.push(inner.cssText);
}
}
}
// Asserted against the parsed stylesheet, like
// `hover-affordance.test.ts` and for the same reason: this tier's
// iframe is not 599px wide, so the rule cannot be *rendered* here —
// but the regression worth catching is someone moving it out of the
// query, which nothing on a desktop draws differently.
expect(phoneRules.length).toBeGreaterThan(0);
expect(phoneRules.some((r) => /min-height:\s*48px/.test(r))).toBe(true);
});
it('takes the resize handle out of the sheet', async () => {
const el = await fixture<Nav>('bottom-nav');
shadow<HTMLButtonElement>(el, '[data-testid="tab-more"]')?.click();
await update(el, {});
const handle = shadow<HTMLElement>(el, 'app-sidebar')
?.shadowRoot?.querySelector('.resize-handle');
if (!handle) throw new Error('no resize handle');
// A col-resize strip on the right edge of a touch surface: the
// compatibility mouse events a tap synthesises reach its
// `mousedown`, so it can start a resize nobody asked for.
expect(getComputedStyle(handle).display).toBe('none');
});
});
-5
View File
@@ -137,11 +137,6 @@ describe('<app-sidebar>', () => {
});
it('looks the way it did last time', async () => {
// Stated rather than inherited: `activeViewStore` is a singleton, so
// without this the shot records whichever view the *previous* case
// left in it and the reference moves when the file is reordered.
activeViewStore.setView('home', true);
const el = await fixture('app-sidebar');
await visual(el, 'app-sidebar');
@@ -366,16 +366,6 @@ describe('<now-playing>', () => {
const el = await fixture('now-playing');
emit(Events.TrackChanged, { ...TRACK, trackChangeId: 6 });
// Stated rather than inherited: the queue store is a singleton, so
// without this the shot records whichever source the *previous*
// case left in it and the reference moves when the file is
// reordered. Three lines is what the bar renders while playing
// from somewhere, which is the arrangement worth recording.
setQueue([queueTrack(1, 'Ashes to Ashes')], 0, {
type: 'album',
id: 7,
label: 'Scary Monsters',
});
await flush();
await el.updateComplete;
@@ -161,15 +161,6 @@ describe('double-clicking a row in the track list', () => {
localStorage.removeItem('track-list-column-widths');
el = await fixture<LitElement>('track-list', { externalTracks: LIST });
// Say which order is being asserted rather than inheriting one.
// `restoreSortPreferences()` runs in `connectedCallback`, so the
// list opens in whatever sort was last persisted -- and
// `track-11` sorts before `track-3` by title, which is the shape
// of #138. The row below is the fixture's third track only while
// nothing is sorting the list.
(el as unknown as { sortField: string | null }).sortField = null;
el.style.display = 'block';
el.style.height = '600px';
await flush();
@@ -181,10 +172,6 @@ describe('double-clicking a row in the track list', () => {
const rows = shadowAll(el, '.track-row');
const row = rows.find((r) => r.getAttribute('data-index') === '3');
// Stated first, so a list that is not in the order this asserts
// fails by saying so rather than as an off-by-eight file path.
expect(row?.getAttribute('data-file-path')).toBe('/music/track-3.mp3');
dblclick(row!);
await flush();
-17
View File
@@ -72,23 +72,6 @@ document.body.style.margin = '0';
beforeEach(() => {
resetHarness();
// A test file does not get its own origin. `@vitest/browser-playwright`
// opens one BrowserContext per session and runs several files in it,
// one after another, so everything a component persists — the track
// list's sort and column widths, the cover size, `now-playing`'s
// scroll mode — is still there when the next file mounts the same
// component. That is invisible until it is intermittent, because
// which files share a tab and in what order changes run to run: it
// cost #138 three scheduled runs, one of them a PR with no frontend
// code in it at all.
//
// Clearing here rather than in the specs that write is deliberate —
// the spec that *reads* is never the one that knows. It is safe for
// the same reason the leak exists: files in a session are
// sequential, so this cannot wipe storage a concurrent file is in
// the middle of using.
localStorage.clear();
});
afterEach(() => {
+39
View File
@@ -30,6 +30,12 @@ func TestFixturesMatchManifest(t *testing.T) {
m := testfixtures.Load(t)
for _, want := range m.Tracks {
// WAV tags are write-only today; see
// TestWAVTagsAreNotReadableYet.
if want.Format == "wav" {
continue
}
t.Run(want.Path, func(t *testing.T) {
t.Parallel()
@@ -155,6 +161,39 @@ func TestDuplicateFixturesAreIndistinguishable(t *testing.T) {
}
}
// TestWAVTagsAreNotReadableYet pins a known gap rather than hiding it.
//
// backend/tagwriter writes WAV tags into a RIFF "id3 " chunk, but
// backend/metadata reads through dhowden/tag, which recognises MP3,
// FLAC, OGG, MP4 and DSF and has no RIFF parser at all. So every tag
// the app writes to a WAV is invisible to the app that wrote it, and
// WAV tracks always scan in as untitled.
//
// The fixtures are tagged correctly on disk, so when the reader learns
// to unwrap the RIFF chunk this test starts failing — which is the
// point. Delete it then and drop the "wav" skip in
// TestFixturesMatchManifest.
func TestWAVTagsAreNotReadableYet(t *testing.T) {
t.Parallel()
m := testfixtures.Load(t)
for _, path := range m.Case(t, testfixtures.CaseWAVTracks) {
got, err := metadata.ExtractTags(path)
if err != nil {
t.Fatalf("extract tags from %s: %v", path, err)
}
if got.Title != "" {
t.Errorf(
"%s: WAV tags are now readable (%q) — good news; "+
"see this test's comment for what to update",
filepath.Base(path), got.Title,
)
}
}
}
func assertTag(t *testing.T, field string, want testfixtures.Track, got string) {
t.Helper()