Compare commits
11
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4f628b1f52 | ||
|
|
2100f0022f | ||
|
|
52038dc5ae | ||
|
|
0d331666d6 | ||
|
|
6a5a3c33dc | ||
|
|
1668b9e0d2 | ||
|
|
ec64dbded0 | ||
|
|
dad852a8a0 | ||
|
|
30c6b665f1 | ||
|
|
d034d6e571 | ||
|
|
25ea1f3511 |
@@ -40,7 +40,8 @@ WHERE id = ? AND (mbid IS NULL OR mbid = '');
|
||||
-- name: GetAlbumsWithPendingReleaseMBID :many
|
||||
SELECT id, pending_release_mbid FROM albums
|
||||
WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != ''
|
||||
AND (mbid IS NULL OR mbid = '');
|
||||
AND (mbid IS NULL OR mbid = '')
|
||||
LIMIT ?;
|
||||
|
||||
-- name: DeleteAlbum :exec
|
||||
DELETE FROM albums WHERE id = ?;
|
||||
|
||||
@@ -331,6 +331,7 @@ const getAlbumsWithPendingReleaseMBID = `-- name: GetAlbumsWithPendingReleaseMBI
|
||||
SELECT id, pending_release_mbid FROM albums
|
||||
WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != ''
|
||||
AND (mbid IS NULL OR mbid = '')
|
||||
LIMIT ?
|
||||
`
|
||||
|
||||
type GetAlbumsWithPendingReleaseMBIDRow struct {
|
||||
@@ -338,8 +339,8 @@ type GetAlbumsWithPendingReleaseMBIDRow struct {
|
||||
PendingReleaseMbid sql.NullString
|
||||
}
|
||||
|
||||
func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context) ([]GetAlbumsWithPendingReleaseMBIDRow, error) {
|
||||
rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID)
|
||||
func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context, limit int64) ([]GetAlbumsWithPendingReleaseMBIDRow, error) {
|
||||
rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID, limit)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
+35
-31
@@ -2,6 +2,7 @@ package explore
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"log/slog"
|
||||
"math"
|
||||
"sort"
|
||||
@@ -12,6 +13,7 @@ import (
|
||||
"golang.org/x/sync/singleflight"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
"yellowjacket/backend/database/sql/sqlcgen"
|
||||
"yellowjacket/backend/events"
|
||||
"yellowjacket/backend/jobs"
|
||||
)
|
||||
@@ -279,37 +281,32 @@ func (e *Service) BackfillReleaseGroupMBIDs() {
|
||||
go e.backfillReleaseGroupMBIDs(e.ctx)
|
||||
}
|
||||
|
||||
func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
|
||||
rows, err := e.db.QueryContext(
|
||||
"SELECT id, pending_release_mbid FROM release_groups "+
|
||||
"WHERE (mbid IS NULL OR mbid = '') "+
|
||||
"AND pending_release_mbid IS NOT NULL AND pending_release_mbid != '' "+
|
||||
"LIMIT ?",
|
||||
releaseGroupMBIDBackfillMaxPerRun,
|
||||
// pendingReleaseMBIDs is the albums this pass has work to do on.
|
||||
//
|
||||
// It is separate from the pass, and returns its error rather than
|
||||
// logging it, so that a test can assert the statement runs against the
|
||||
// real schema. That is not a general preference -- it is this
|
||||
// statement's history: it named `release_groups`, a table plan 013
|
||||
// renamed to `albums`, so it failed on every launch since e7748f1 and
|
||||
// the pass returned quietly having done nothing. A test of the pass
|
||||
// as a whole cannot see that, because a query error and an empty
|
||||
// library are the same early return.
|
||||
func (e *Service) pendingReleaseMBIDs(
|
||||
ctx context.Context,
|
||||
) ([]sqlcgen.GetAlbumsWithPendingReleaseMBIDRow, error) {
|
||||
return e.db.ReadQueries.GetAlbumsWithPendingReleaseMBID(
|
||||
ctx, releaseGroupMBIDBackfillMaxPerRun,
|
||||
)
|
||||
}
|
||||
|
||||
func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
|
||||
pending, err := e.pendingReleaseMBIDs(ctx)
|
||||
if err != nil {
|
||||
e.logger.Warn("release-group mbid backfill: query failed", "error", err)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
type pendingRow struct {
|
||||
id int64
|
||||
releaseMBID string
|
||||
}
|
||||
|
||||
var pending []pendingRow
|
||||
|
||||
for rows.Next() {
|
||||
var p pendingRow
|
||||
|
||||
if err := rows.Scan(&p.id, &p.releaseMBID); err == nil {
|
||||
pending = append(pending, p)
|
||||
}
|
||||
}
|
||||
|
||||
_ = rows.Close()
|
||||
|
||||
if len(pending) == 0 {
|
||||
return
|
||||
}
|
||||
@@ -335,7 +332,7 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
|
||||
|
||||
job.progress(i, len(pending))
|
||||
|
||||
release, err := e.mb.LookupRelease(ctx, p.releaseMBID)
|
||||
release, err := e.mb.LookupRelease(ctx, p.PendingReleaseMbid.String)
|
||||
if err != nil || release.ReleaseGroupMBID == "" {
|
||||
// Left alone rather than cleared: LookupRelease caches its
|
||||
// answer (success or a release with no group) for 7 days,
|
||||
@@ -344,12 +341,19 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
|
||||
continue
|
||||
}
|
||||
|
||||
_, err = e.db.ExecContext(
|
||||
"UPDATE release_groups SET mbid = ?, pending_release_mbid = NULL "+
|
||||
"WHERE id = ? AND (mbid IS NULL OR mbid = '')",
|
||||
release.ReleaseGroupMBID, p.id,
|
||||
)
|
||||
if err != nil {
|
||||
// The writer, not ReadQueries: an UPDATE issued on the
|
||||
// query-only pool fails at runtime with "attempt to write a
|
||||
// readonly database".
|
||||
if err := e.db.Queries.ResolveAlbumPendingReleaseMBID(
|
||||
ctx,
|
||||
sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
|
||||
Mbid: sql.NullString{
|
||||
String: release.ReleaseGroupMBID,
|
||||
Valid: true,
|
||||
},
|
||||
ID: p.ID,
|
||||
},
|
||||
); err != nil {
|
||||
e.logger.Warn("release-group mbid backfill: update failed", "error", err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,250 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"database/sql"
|
||||
"log/slog"
|
||||
"strconv"
|
||||
"testing"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
"yellowjacket/backend/database/sql/sqlcgen"
|
||||
)
|
||||
|
||||
// The release-group MBID backfill queried `release_groups`, a table
|
||||
// plan 013 renamed to `albums`, so it failed on its first statement on
|
||||
// every launch from e7748f1 until #189 -- and the pass swallowed that,
|
||||
// because a query error and an empty library are the same early
|
||||
// return. Nothing noticed for two reasons worth keeping in mind:
|
||||
//
|
||||
// - the statement was **raw SQL**, so sqlc never read it. Every other
|
||||
// statement in the repo was renamed by the same change because sqlc
|
||||
// reads sql/schemas/ and cannot generate against a table that is not
|
||||
// declared. The two sqlc queries this now calls were written by 013
|
||||
// and left uncalled.
|
||||
// - it needs no network and no fixture library to reproduce. The
|
||||
// failure is at prepare time.
|
||||
|
||||
// seedPendingAlbum inserts an album whose files carried a release MBID
|
||||
// but no release-group MBID, which is what `library.updateMBIDs`
|
||||
// leaves behind for this pass to resolve.
|
||||
func seedPendingAlbum(
|
||||
t *testing.T,
|
||||
db *database.DB,
|
||||
name, pendingMBID string,
|
||||
) int64 {
|
||||
t.Helper()
|
||||
|
||||
res, err := db.ExecContext(
|
||||
"INSERT INTO albums (name, artist_credit, pending_release_mbid) "+
|
||||
"VALUES (?, ?, ?)",
|
||||
name, "Test Artist", pendingMBID,
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("insert albums row: %v", err)
|
||||
}
|
||||
|
||||
id, err := res.LastInsertId()
|
||||
if err != nil {
|
||||
t.Fatalf("last insert id: %v", err)
|
||||
}
|
||||
|
||||
return id
|
||||
}
|
||||
|
||||
func newPendingTestService(db *database.DB) *Service {
|
||||
return &Service{db: db, logger: slog.Default()}
|
||||
}
|
||||
|
||||
// TestPendingReleaseMBIDsRunsAgainstTheRealSchema is the regression.
|
||||
//
|
||||
// It asserts the statement *runs*, which is the whole of what was
|
||||
// broken: against the old raw SQL this returns
|
||||
// "no such table: release_groups" rather than a row.
|
||||
func TestPendingReleaseMBIDsRunsAgainstTheRealSchema(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
e := newPendingTestService(db)
|
||||
|
||||
want := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
|
||||
|
||||
pending, err := e.pendingReleaseMBIDs(db.Ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("the backfill's query failed: %v", err)
|
||||
}
|
||||
|
||||
if len(pending) != 1 {
|
||||
t.Fatalf("got %d pending albums, want 1", len(pending))
|
||||
}
|
||||
|
||||
if pending[0].ID != want {
|
||||
t.Errorf("got album id %d, want %d", pending[0].ID, want)
|
||||
}
|
||||
|
||||
if got := pending[0].PendingReleaseMbid.String; got != "release-mbid-1" {
|
||||
t.Errorf("got pending mbid %q, want %q", got, "release-mbid-1")
|
||||
}
|
||||
}
|
||||
|
||||
// TestOnlyUnresolvedAlbumsAreReturned pins the two conditions that make
|
||||
// the pass idempotent, since between them they are what stops it doing
|
||||
// the same MusicBrainz lookups on every launch forever.
|
||||
func TestOnlyUnresolvedAlbumsAreReturned(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
e := newPendingTestService(db)
|
||||
|
||||
pendingID := seedPendingAlbum(t, db, "Still Pending", "release-mbid-1")
|
||||
|
||||
// Already resolved: it has a real MBID, so there is nothing to
|
||||
// look up even though a marker is still sitting on it.
|
||||
resolved := seedPendingAlbum(t, db, "Already Resolved", "release-mbid-2")
|
||||
if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{
|
||||
Mbid: sql.NullString{String: "rg-mbid", Valid: true},
|
||||
ID: resolved,
|
||||
}); err != nil {
|
||||
t.Fatalf("set album mbid: %v", err)
|
||||
}
|
||||
|
||||
// Never had a release MBID to resolve in the first place, which is
|
||||
// most of a library.
|
||||
seedPendingAlbum(t, db, "Nothing Pending", "")
|
||||
|
||||
pending, err := e.pendingReleaseMBIDs(db.Ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("the backfill's query failed: %v", err)
|
||||
}
|
||||
|
||||
if len(pending) != 1 || pending[0].ID != pendingID {
|
||||
t.Fatalf(
|
||||
"got %d albums %v, want only the unresolved one (%d)",
|
||||
len(pending), pending, pendingID,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolvingClearsTheMarker is the other half: once the lookup has
|
||||
// answered, the album must stop being a candidate, or the pass repeats
|
||||
// the same live MusicBrainz call on every launch.
|
||||
func TestResolvingClearsTheMarker(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
e := newPendingTestService(db)
|
||||
|
||||
id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
|
||||
|
||||
// The writer, deliberately: this is an UPDATE, and the read pool
|
||||
// would refuse it at runtime.
|
||||
if err := db.Queries.ResolveAlbumPendingReleaseMBID(
|
||||
db.Ctx,
|
||||
sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
|
||||
Mbid: sql.NullString{String: "resolved-rg-mbid", Valid: true},
|
||||
ID: id,
|
||||
},
|
||||
); err != nil {
|
||||
t.Fatalf("resolve pending release mbid: %v", err)
|
||||
}
|
||||
|
||||
pending, err := e.pendingReleaseMBIDs(db.Ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("the backfill's query failed: %v", err)
|
||||
}
|
||||
|
||||
if len(pending) != 0 {
|
||||
t.Fatalf("a resolved album is still a candidate: %v", pending)
|
||||
}
|
||||
|
||||
album, err := db.ReadQueries.GetAlbum(db.Ctx, id)
|
||||
if err != nil {
|
||||
t.Fatalf("get album: %v", err)
|
||||
}
|
||||
|
||||
if album.Mbid.String != "resolved-rg-mbid" {
|
||||
t.Errorf("album mbid = %q, want the resolved one", album.Mbid.String)
|
||||
}
|
||||
|
||||
if album.PendingReleaseMbid.Valid &&
|
||||
album.PendingReleaseMbid.String != "" {
|
||||
t.Errorf(
|
||||
"the pending marker survived as %q",
|
||||
album.PendingReleaseMbid.String,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAResolvedMBIDIsNeverOverwritten covers the guard in the UPDATE.
|
||||
//
|
||||
// The pass runs against rows it read earlier, and a rescan can resolve
|
||||
// an album from its tags in between -- a real MBID from the file must
|
||||
// win over one this pass inferred from a release.
|
||||
func TestAResolvedMBIDIsNeverOverwritten(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
|
||||
id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
|
||||
|
||||
if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{
|
||||
Mbid: sql.NullString{String: "from-the-tags", Valid: true},
|
||||
ID: id,
|
||||
}); err != nil {
|
||||
t.Fatalf("set album mbid: %v", err)
|
||||
}
|
||||
|
||||
if err := db.Queries.ResolveAlbumPendingReleaseMBID(
|
||||
db.Ctx,
|
||||
sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
|
||||
Mbid: sql.NullString{String: "from-the-backfill", Valid: true},
|
||||
ID: id,
|
||||
},
|
||||
); err != nil {
|
||||
t.Fatalf("resolve pending release mbid: %v", err)
|
||||
}
|
||||
|
||||
album, err := db.ReadQueries.GetAlbum(db.Ctx, id)
|
||||
if err != nil {
|
||||
t.Fatalf("get album: %v", err)
|
||||
}
|
||||
|
||||
if album.Mbid.String != "from-the-tags" {
|
||||
t.Errorf(
|
||||
"album mbid = %q, want the tagged one to have won",
|
||||
album.Mbid.String,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestThePassIsBounded checks the LIMIT.
|
||||
//
|
||||
// Each row costs a live MusicBrainz lookup on a 1 req/s limiter shared
|
||||
// with every page the user can open, so an unbounded read is a run that
|
||||
// lasts as long as the library is untagged. The sqlc query 013 wrote
|
||||
// had no LIMIT; the raw statement it was replacing did.
|
||||
func TestThePassIsBounded(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
e := newPendingTestService(db)
|
||||
|
||||
for i := range releaseGroupMBIDBackfillMaxPerRun + 10 {
|
||||
seedPendingAlbum(
|
||||
t, db,
|
||||
"Album "+string(rune('A'+i%26))+strconv.Itoa(i),
|
||||
"release-mbid-"+strconv.Itoa(i),
|
||||
)
|
||||
}
|
||||
|
||||
pending, err := e.pendingReleaseMBIDs(db.Ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("the backfill's query failed: %v", err)
|
||||
}
|
||||
|
||||
if len(pending) != releaseGroupMBIDBackfillMaxPerRun {
|
||||
t.Errorf(
|
||||
"got %d albums, want the run bounded at %d",
|
||||
len(pending), releaseGroupMBIDBackfillMaxPerRun,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -52,6 +52,75 @@ func UseHomeOverride(base string) {
|
||||
_ = os.Setenv(envHomeOverride, base)
|
||||
}
|
||||
|
||||
// envTempDir is the variable Go's os.TempDir() reads, and through it
|
||||
// every library in the process that asks for a temporary file.
|
||||
const envTempDir = "TMPDIR"
|
||||
|
||||
// tempDirName is the subdirectory of the app's own storage that
|
||||
// becomes that answer.
|
||||
const tempDirName = "tmp"
|
||||
|
||||
// UseTempDir gives the process a temporary directory that exists.
|
||||
//
|
||||
// **Android has no /tmp and hands an app no TMPDIR**, and Go's
|
||||
// os.TempDir() falls back to "/tmp" when the variable is unset -- so
|
||||
// every library in the process that wants scratch space is handed a
|
||||
// path that has never existed. SQLite is the one that noticed: an
|
||||
// INSERT ... SELECT large enough to spill returned
|
||||
// SQLITE_IOERR_GETTEMPPATH (disk I/O error 6410), which is how the
|
||||
// champion search index came to fail its rebuild on every launch while
|
||||
// the app otherwise looked healthy (#190).
|
||||
//
|
||||
// It is the *class* that is fixed here rather than that statement.
|
||||
// Anything that spills fails the same way on that platform -- large
|
||||
// sorts, large joins, VACUUM -- so the repair belongs at the process's
|
||||
// one answer to the question rather than at each caller. The
|
||||
// alternative considered was PRAGMA temp_store = MEMORY, which is
|
||||
// cheaper and more local and is a promise that every future spill fits
|
||||
// in RAM on a phone; the catalog is the largest thing in this app and
|
||||
// that is not a promise worth making silently.
|
||||
//
|
||||
// The rules are UseHomeOverride's, for the same reasons. **An empty
|
||||
// base is a no-op**, because that is what
|
||||
// application.Mobile.StoragePath() returns on desktop -- so this needs
|
||||
// no build tag and changes nothing off mobile, where /tmp is real. And
|
||||
// **an explicit TMPDIR wins**, so anyone who set one deliberately gets
|
||||
// what they asked for; nothing sets it on the platform this exists for.
|
||||
//
|
||||
// It returns its error rather than swallowing it because a temp
|
||||
// directory that could not be created is the same silent failure one
|
||||
// step earlier, and since #160 a log line on that platform is
|
||||
// something a person can actually read.
|
||||
func UseTempDir(base string) error {
|
||||
if base == "" || os.Getenv(envTempDir) != "" {
|
||||
return nil
|
||||
}
|
||||
|
||||
dir := filepath.Join(base, tempDirName)
|
||||
|
||||
if err := os.MkdirAll(dir, os.ModePerm); err != nil {
|
||||
return fmt.Errorf("could not make the temp directory %s: %w", dir, err)
|
||||
}
|
||||
|
||||
// Writability is checked rather than assumed: the whole failure
|
||||
// this repairs is a directory that is named and cannot be used, and
|
||||
// MkdirAll on an existing unwritable directory succeeds.
|
||||
probe, err := os.CreateTemp(dir, "probe")
|
||||
if err != nil {
|
||||
return fmt.Errorf("temp directory %s is not writable: %w", dir, err)
|
||||
}
|
||||
|
||||
name := probe.Name()
|
||||
_ = probe.Close()
|
||||
_ = os.Remove(name)
|
||||
|
||||
if err := os.Setenv(envTempDir, dir); err != nil {
|
||||
return fmt.Errorf("could not set %s: %w", envTempDir, err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// getUserDirPath returns and creates the path for a user directory.
|
||||
func getUserDirPath(dt dirType) (string, error) {
|
||||
path, err := resolveUserDirPath(dt)
|
||||
|
||||
@@ -87,3 +87,116 @@ func TestUseHomeOverride(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// UseTempDir carries UseHomeOverride's two rules for the same reasons,
|
||||
// plus one of its own: the directory it names has to be usable.
|
||||
//
|
||||
// **The only tier that can compile the platform this exists for is a
|
||||
// phone**, so everything decidable off one is decided here -- which is
|
||||
// androidpayload.go's discipline, and is why the platform call is a
|
||||
// parameter rather than something this package reaches for. The
|
||||
// device's half is a single measurement: no /tmp, no TMPDIR (#190).
|
||||
func TestUseTempDir(t *testing.T) {
|
||||
t.Run("an empty base is a no-op", func(t *testing.T) {
|
||||
// This is the desktop case in full: StoragePath() answers ""
|
||||
// off mobile, where /tmp is real and must be left alone.
|
||||
t.Setenv(envTempDir, "")
|
||||
|
||||
if err := UseTempDir(""); err != nil {
|
||||
t.Fatalf("UseTempDir(\"\") = %v, want nil", err)
|
||||
}
|
||||
|
||||
if got := os.Getenv(envTempDir); got != "" {
|
||||
t.Errorf("%s = %q, want it untouched", envTempDir, got)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("an explicit TMPDIR wins", func(t *testing.T) {
|
||||
const chosen = "/somewhere/deliberate"
|
||||
|
||||
// The base is taken before TMPDIR moves, because t.TempDir()
|
||||
// reads TMPDIR too -- which is the same fact this function is
|
||||
// about, met from the other side.
|
||||
base := t.TempDir()
|
||||
|
||||
t.Setenv(envTempDir, chosen)
|
||||
|
||||
if err := UseTempDir(base); err != nil {
|
||||
t.Fatalf("UseTempDir = %v, want nil", err)
|
||||
}
|
||||
|
||||
if got := os.Getenv(envTempDir); got != chosen {
|
||||
t.Errorf("%s = %q, want the explicit %q", envTempDir, got, chosen)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("points at a real directory under the base", func(t *testing.T) {
|
||||
base := t.TempDir()
|
||||
|
||||
t.Setenv(envTempDir, "")
|
||||
|
||||
if err := UseTempDir(base); err != nil {
|
||||
t.Fatalf("UseTempDir = %v, want nil", err)
|
||||
}
|
||||
|
||||
got := os.Getenv(envTempDir)
|
||||
|
||||
want := filepath.Join(base, tempDirName)
|
||||
if got != want {
|
||||
t.Fatalf("%s = %q, want %q", envTempDir, got, want)
|
||||
}
|
||||
|
||||
// The whole failure being repaired is a temp directory that is
|
||||
// named and does not exist, so naming one is not enough.
|
||||
info, err := os.Stat(got)
|
||||
if err != nil {
|
||||
t.Fatalf("the temp directory was named but not created: %v", err)
|
||||
}
|
||||
|
||||
if !info.IsDir() {
|
||||
t.Fatalf("%s is not a directory", got)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("os.TempDir then answers with it", func(t *testing.T) {
|
||||
// The point of setting the variable at all: this is what every
|
||||
// library in the process reads, SQLite's driver included.
|
||||
base := t.TempDir()
|
||||
|
||||
t.Setenv(envTempDir, "")
|
||||
|
||||
if err := UseTempDir(base); err != nil {
|
||||
t.Fatalf("UseTempDir = %v, want nil", err)
|
||||
}
|
||||
|
||||
if got := os.TempDir(); got != filepath.Join(base, tempDirName) {
|
||||
t.Errorf("os.TempDir() = %q, want the directory we made", got)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("an unwritable directory is an error, not a silent success", func(t *testing.T) {
|
||||
if os.Getuid() == 0 {
|
||||
t.Skip("root can write anywhere, so there is nothing to refuse")
|
||||
}
|
||||
|
||||
base := t.TempDir()
|
||||
|
||||
// MkdirAll on an existing directory succeeds whatever its
|
||||
// mode, so without the write probe this case would set TMPDIR
|
||||
// to a directory nothing can use -- which is the bug again,
|
||||
// one directory over.
|
||||
if err := os.Mkdir(filepath.Join(base, tempDirName), 0o500); err != nil {
|
||||
t.Fatalf("prepare the unwritable directory: %v", err)
|
||||
}
|
||||
|
||||
t.Setenv(envTempDir, "")
|
||||
|
||||
if err := UseTempDir(base); err == nil {
|
||||
t.Fatal("UseTempDir accepted a directory it cannot write to")
|
||||
}
|
||||
|
||||
if got := os.Getenv(envTempDir); got != "" {
|
||||
t.Errorf("%s was set to %q despite the failure", envTempDir, got)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@components/cover-grid/cover-grid.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
|
||||
@customElement('artist-details')
|
||||
export class ArtistDetails extends LitElement {
|
||||
@@ -40,7 +41,7 @@ export class ArtistDetails extends LitElement {
|
||||
/** Tracks the store's cached array reference to detect refreshes. */
|
||||
private lastAlbumsRef: library.Album[] | null = null;
|
||||
|
||||
static override styles = [designTokens, css`
|
||||
static override styles = [designTokens, backButton, css`
|
||||
:host {
|
||||
display: flex;
|
||||
flex-direction: column;
|
||||
@@ -66,31 +67,6 @@ export class ArtistDetails extends LitElement {
|
||||
);
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(
|
||||
--yj-bg-overlay,
|
||||
rgba(255, 255, 255, 0.06)
|
||||
);
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(
|
||||
--yj-bg-hover,
|
||||
rgba(255, 255, 255, 0.12)
|
||||
);
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px; /* back button — outside type scale */
|
||||
}
|
||||
|
||||
@@ -45,18 +45,6 @@ export class SeekBar extends LitElement {
|
||||
private showRemaining: boolean = true;
|
||||
|
||||
static override styles = [designTokens, waSliderLabel, css`
|
||||
/* 12px below the phone breakpoint. The bottom bar's seek bar is
|
||||
display:none there (016 B2 phase 1), so the only instance a
|
||||
viewport media query can reach at that width is the full-screen
|
||||
now-playing view's -- which is exactly the one a thumb uses.
|
||||
The track size lives on wa-slider inside this shadow root, so a
|
||||
custom property set by the host would not reach it. */
|
||||
@media (max-width: 599px) {
|
||||
wa-slider {
|
||||
--track-size: 12px;
|
||||
}
|
||||
}
|
||||
|
||||
wa-slider {
|
||||
--track-size: 6px;
|
||||
flex: 1;
|
||||
@@ -80,6 +68,57 @@ export class SeekBar extends LitElement {
|
||||
background: var(--yj-bg-base, black);
|
||||
}
|
||||
|
||||
/* The phone's seek bar, and this block is last on purpose.
|
||||
|
||||
A media query adds no specificity, so this lived above the plain
|
||||
"wa-slider" rule and lost to it at every width: the 12px track it
|
||||
asks for had never once applied, and the bar measured 261x6 on
|
||||
the device while the source said 12. That is index.css's rule
|
||||
("the phone section is last on purpose") met inside a component's
|
||||
own stylesheet, and nothing renders differently in any tier here
|
||||
to say so.
|
||||
|
||||
The bottom bar's seek bar is display:none below this width (016
|
||||
B2 phase 1), so the only instance a viewport media query can
|
||||
reach is the full-screen now-playing view's -- which is exactly
|
||||
the one a thumb uses. The desktop bar keeps its 6px, where a
|
||||
mouse is precise and the thickness is right.
|
||||
|
||||
The painted track and the thing you can hit are allowed to
|
||||
differ, and a slider is the clearest case where they should: 12px
|
||||
is a progress bar you can see, and 44px is the app's touch floor
|
||||
(#56). A 44px-*thick* bar would be wrong-looking and would cost
|
||||
the album art the vertical space #51 spent an issue recovering.
|
||||
|
||||
Two things about how the target is built.
|
||||
|
||||
The padding goes on ::part(slider) rather than on the host,
|
||||
because that inner div is what carries the gesture -- it has the
|
||||
listener and the touch-action: none, and it is exactly the host's
|
||||
size, so padding the host would grow a box that does not take the
|
||||
press.
|
||||
|
||||
The padding is asymmetric and the margins cancel it, so the row
|
||||
does not grow by the difference. Both halves are measured: the
|
||||
seek row is 19px (its clocks, not the track, decide that) and the
|
||||
play button's top edge is 8px below it, so the target takes the
|
||||
space *above*, where .art is a non-interactive div. Growing the
|
||||
row instead cost the art 25px of 143. Verified on the device at
|
||||
424x439: hit area 44px, painted track 12px, row still 19px, art
|
||||
still 143px, 8px of clearance left under the play button, a press
|
||||
26px above the track seeks, and a hit test on the play button's
|
||||
top edge still reaches the play button. */
|
||||
@media (max-width: 599px) {
|
||||
wa-slider {
|
||||
--track-size: 12px;
|
||||
}
|
||||
|
||||
wa-slider::part(slider) {
|
||||
padding-block: 28px 4px;
|
||||
margin-block: -28px -4px;
|
||||
}
|
||||
}
|
||||
|
||||
#seek-bar-container {
|
||||
display: flex;
|
||||
justify-content: space-between;
|
||||
|
||||
@@ -250,13 +250,20 @@ export class AutotagView extends ViewLifecycleMixin(LitElement) {
|
||||
}
|
||||
|
||||
/* Collapsible-section toggle used in the Pending header —
|
||||
transparent button that inherits the header's type. */
|
||||
transparent button that inherits the header's type.
|
||||
|
||||
187x**15** before this (#186), which was the smallest
|
||||
control measured anywhere in the app until the column
|
||||
arrows were counted. It is transparent and full-width
|
||||
already, so the floor costs it a height and nothing
|
||||
else. */
|
||||
.section-toggle {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
gap: 0.35rem;
|
||||
flex: 1;
|
||||
min-width: 0;
|
||||
min-block-size: 44px;
|
||||
padding: 0;
|
||||
background: transparent;
|
||||
border: 0;
|
||||
@@ -274,6 +281,8 @@ export class AutotagView extends ViewLifecycleMixin(LitElement) {
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
}
|
||||
|
||||
/* 32x18, and it has no background until hover -- so the
|
||||
padding out to a square target is invisible (#186). */
|
||||
.folders-menu-trigger {
|
||||
background: transparent;
|
||||
border: 0;
|
||||
@@ -281,6 +290,8 @@ export class AutotagView extends ViewLifecycleMixin(LitElement) {
|
||||
font-size: 1.1rem;
|
||||
line-height: 1;
|
||||
padding: 0.1rem 0.4rem;
|
||||
min-inline-size: 44px;
|
||||
min-block-size: 44px;
|
||||
border-radius: 3px;
|
||||
cursor: pointer;
|
||||
}
|
||||
@@ -293,7 +304,10 @@ export class AutotagView extends ViewLifecycleMixin(LitElement) {
|
||||
.folders-refresh-trigger {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
font-size: 0.95rem;
|
||||
min-inline-size: 44px;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
.folders-refresh-trigger:disabled {
|
||||
|
||||
@@ -85,6 +85,21 @@ export class ConfigField extends LitElement {
|
||||
gap: 0.5em;
|
||||
}
|
||||
|
||||
/* Every control here meets the app's 44px touch floor (#186).
|
||||
|
||||
This is the shape every row in Settings uses, so it is the
|
||||
one rule that covers the most controls -- and it is the
|
||||
*cheapest* place to reach the floor, because there is no
|
||||
overflow fit on this page. The page header's had one (#69),
|
||||
which is why that pass had to grow padding and hand the
|
||||
width back with a negative margin; here the control is a
|
||||
block in a column and a taller box costs nothing but the
|
||||
height it takes.
|
||||
|
||||
Measured on the reference device before this: the select
|
||||
335x30, the text and number inputs the same, the browse
|
||||
button 30 tall, the colour swatch 33x33 and the toggle
|
||||
**34x19**. */
|
||||
input[type='text'],
|
||||
input[type='number'] {
|
||||
background: var(--yj-bg-elevated, #343a40);
|
||||
@@ -95,6 +110,7 @@ export class ConfigField extends LitElement {
|
||||
font-size: 0.85em;
|
||||
font-family: inherit;
|
||||
min-width: 0;
|
||||
min-block-size: 44px;
|
||||
flex: 1;
|
||||
}
|
||||
|
||||
@@ -117,6 +133,7 @@ export class ConfigField extends LitElement {
|
||||
font-size: 0.85em;
|
||||
font-family: inherit;
|
||||
cursor: pointer;
|
||||
min-block-size: 44px;
|
||||
flex: 1;
|
||||
}
|
||||
|
||||
@@ -139,6 +156,7 @@ export class ConfigField extends LitElement {
|
||||
font-size: 0.85em;
|
||||
cursor: pointer;
|
||||
white-space: nowrap;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
button:hover {
|
||||
@@ -158,8 +176,12 @@ export class ConfigField extends LitElement {
|
||||
}
|
||||
|
||||
input[type='color'] {
|
||||
width: 2.5em;
|
||||
height: 2.5em;
|
||||
/* border-box, or the 2px border makes this 48 and the
|
||||
assertion below reads as passing by four pixels of
|
||||
border rather than by the rule. */
|
||||
box-sizing: border-box;
|
||||
width: 44px;
|
||||
height: 44px;
|
||||
border: 2px solid var(--yj-border, #444);
|
||||
border-radius: 4px;
|
||||
padding: 0;
|
||||
@@ -187,12 +209,32 @@ export class ConfigField extends LitElement {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: space-between;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
/* The toggle is the one control here whose target and paint
|
||||
must differ, and it is also the one no sweep can see.
|
||||
|
||||
Its <input> is opacity: 0; width: 0; height: 0, so a
|
||||
walk of every input on the page skips it as a zero-sized
|
||||
node -- the thing a finger actually hits is this <label>,
|
||||
which measured **34x19**. That is smaller than anything in
|
||||
#186's original table and it is absent from it for exactly
|
||||
that reason.
|
||||
|
||||
A 44px pill is not what a switch should look like, so the
|
||||
box is 44px and the paint is not: .toggle-slider is a
|
||||
2.5em x 1.4em child centred in it rather than an absolute
|
||||
fill. The negative inline margins hand the extra width back
|
||||
to the layout, so the pill stays flush with the right edge
|
||||
of the inputs in the rows above it -- the header pass's
|
||||
shape, used here for alignment rather than for a fit. */
|
||||
.toggle-switch {
|
||||
position: relative;
|
||||
width: 2.5em;
|
||||
height: 1.4em;
|
||||
display: grid;
|
||||
place-items: center;
|
||||
inline-size: 44px;
|
||||
block-size: 44px;
|
||||
margin-inline: calc((2.5em - 44px) / 2);
|
||||
}
|
||||
|
||||
.toggle-switch input {
|
||||
@@ -202,9 +244,10 @@ export class ConfigField extends LitElement {
|
||||
}
|
||||
|
||||
.toggle-slider {
|
||||
position: absolute;
|
||||
position: relative;
|
||||
cursor: pointer;
|
||||
inset: 0;
|
||||
inline-size: 2.5em;
|
||||
block-size: 1.4em;
|
||||
background: var(--yj-bg-overlay, #495057);
|
||||
border-radius: 1em;
|
||||
transition: background 0.2s;
|
||||
|
||||
@@ -176,6 +176,13 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
cursor: pointer;
|
||||
transition: background-color 0.15s ease;
|
||||
white-space: nowrap;
|
||||
/* The app's 44px touch floor (#56, #186), stated once for
|
||||
all 41 buttons this page renders rather than per class.
|
||||
Height is free here: Settings has no overflow fit, so
|
||||
the header's "only width is contested" rule does not
|
||||
bind, and the two classes that need more than a height
|
||||
say so below. */
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
button:disabled {
|
||||
@@ -509,11 +516,24 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
margin: 0;
|
||||
}
|
||||
|
||||
/* The two column lists are the densest thing in the app, and
|
||||
the density argument is why they are shaped the way they
|
||||
are rather than simply grown (#186).
|
||||
|
||||
Measured on the reference device: the row was already
|
||||
335x36 -- it is the controls *inside* it that were 16x16 and
|
||||
**16x14**, the smallest anywhere in this app, 36 of them.
|
||||
So the fix grows the controls into the row they already
|
||||
occupy and only takes the row from 36 to 44, which over the
|
||||
two lists (10 and 19 items) is 232px of extra scroll on a
|
||||
439px screen. Growing each control to its own 44px row
|
||||
instead would have cost four screens. */
|
||||
.column-item {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
align-items: stretch;
|
||||
gap: 0.5em;
|
||||
padding: 0.5em 0.75em;
|
||||
padding: 0 0.75em;
|
||||
min-block-size: 44px;
|
||||
border-bottom: 1px solid
|
||||
var(--yj-border-subtle, #333);
|
||||
font-size: 0.85em;
|
||||
@@ -531,8 +551,19 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
}
|
||||
|
||||
/* A native checkbox cannot grow its hit area without growing
|
||||
its paint, and a 44px checkbox is not what anyone wants. So
|
||||
the target is the label instead: .column-label is a real
|
||||
<label for> now, which makes the column's *name* the thing
|
||||
you tap -- ~250x44 rather than 16x16.
|
||||
|
||||
That is the argument config-field already makes one file
|
||||
over for its own labels: "a real label association also
|
||||
makes the label text a click target for the control, which
|
||||
is behaviour, not annotation". Here it is the whole fix. */
|
||||
.column-toggle {
|
||||
cursor: pointer;
|
||||
align-self: center;
|
||||
accent-color: var(
|
||||
--yj-accent,
|
||||
#ffd43b
|
||||
@@ -541,20 +572,32 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
|
||||
.column-label {
|
||||
flex: 1;
|
||||
display: flex;
|
||||
align-items: center;
|
||||
cursor: pointer;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
.view-note {
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
font-size: var(--yj-font-size-sm, 0.85rem);
|
||||
margin-left: auto;
|
||||
/* The row stretches its children so the label can be a
|
||||
full-height target; this is text, not a target. */
|
||||
align-self: center;
|
||||
}
|
||||
|
||||
.column-arrows {
|
||||
display: flex;
|
||||
align-items: stretch;
|
||||
gap: 0.15em;
|
||||
margin-left: auto;
|
||||
}
|
||||
|
||||
/* 16x14 before this, and they carry background: none and a
|
||||
transparent border -- so padding out to 44px grows the
|
||||
target and changes nothing anyone can see until hover,
|
||||
which is precisely what #186's Direction asks for. */
|
||||
.column-arrow-btn {
|
||||
background: none;
|
||||
border: 1px solid transparent;
|
||||
@@ -564,6 +607,8 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
font-size: 0.65em;
|
||||
line-height: 1;
|
||||
padding: 0.2em 0.35em;
|
||||
min-inline-size: 44px;
|
||||
min-block-size: 44px;
|
||||
transition:
|
||||
color 0.15s,
|
||||
border-color 0.15s;
|
||||
@@ -711,6 +756,11 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
padding: 0.2em 0.4em;
|
||||
letter-spacing: 2px;
|
||||
border-radius: 4px;
|
||||
/* Square, so it needs the width too -- the shared rule
|
||||
above only gives it a height. It was 31x31, and it is
|
||||
the only route to "Remove library", which is the case
|
||||
#55 settled one component over: the way out is 44px. */
|
||||
min-inline-size: 44px;
|
||||
}
|
||||
|
||||
.overflow-btn:hover {
|
||||
@@ -2003,6 +2053,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
class="column-item ${checked ? 'enabled' : 'disabled'}"
|
||||
>
|
||||
<input
|
||||
id="view-${v.id}"
|
||||
type="checkbox"
|
||||
class="column-toggle"
|
||||
aria-label="Show ${v.label} in the navigation"
|
||||
@@ -2014,9 +2065,9 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
(e.target as HTMLInputElement).checked,
|
||||
)}
|
||||
/>
|
||||
<span class="column-label">
|
||||
<label class="column-label" for="view-${v.id}">
|
||||
${v.label}
|
||||
</span>
|
||||
</label>
|
||||
${note
|
||||
? html`<span class="view-note">${note}</span>`
|
||||
: nothing}
|
||||
@@ -2212,6 +2263,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
class="column-item ${checked ? 'enabled' : 'disabled'}"
|
||||
>
|
||||
<input
|
||||
id="column-${id}"
|
||||
type="checkbox"
|
||||
class="column-toggle"
|
||||
aria-label="Show the ${columnLabel} column"
|
||||
@@ -2222,11 +2274,12 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
|
||||
id,
|
||||
)}
|
||||
/>
|
||||
<span
|
||||
<label
|
||||
class="column-label"
|
||||
for="column-${id}"
|
||||
>
|
||||
${columnLabel}
|
||||
</span>
|
||||
</label>
|
||||
<span
|
||||
class="column-arrows"
|
||||
>
|
||||
|
||||
@@ -8,6 +8,7 @@ import '@awesome.me/webawesome/dist/components/switch/switch.js';
|
||||
import '@awesome.me/webawesome/dist/components/spinner/spinner.js';
|
||||
import '@awesome.me/webawesome/dist/components/callout/callout.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { waTouchFloor } from '../../styles/wa-touch-floor.css';
|
||||
import type {
|
||||
DownloadDescriptor,
|
||||
DownloadProvider,
|
||||
@@ -136,6 +137,7 @@ export class DownloadClients extends LitElement {
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
waTouchFloor,
|
||||
css`
|
||||
:host {
|
||||
display: block;
|
||||
@@ -239,12 +241,17 @@ export class DownloadClients extends LitElement {
|
||||
margin-top: 0.4em;
|
||||
}
|
||||
|
||||
/* The checkbox is 16x16 and cannot grow without becoming
|
||||
a 44px checkbox, but it is already wrapped in the label
|
||||
that names it -- so the label is the target and only
|
||||
needs the height (#186). Eight of them. */
|
||||
.format-option {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
gap: 0.4em;
|
||||
font-size: 0.9em;
|
||||
cursor: pointer;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
`,
|
||||
];
|
||||
|
||||
@@ -25,6 +25,11 @@ export class ShortcutCapture extends LitElement {
|
||||
:host {
|
||||
display: inline-block;
|
||||
}
|
||||
/* 80x25, twenty-six of them -- the most numerous control on
|
||||
the Settings page after the column lists (#186). The floor
|
||||
is a height here and nothing else: the width was already
|
||||
past it, and the type stays where it is so a shortcut still
|
||||
reads as a key rather than as a button. */
|
||||
button {
|
||||
font-family: inherit;
|
||||
font-size: var(--yj-text-sm, 13px);
|
||||
@@ -35,6 +40,7 @@ export class ShortcutCapture extends LitElement {
|
||||
color: var(--yj-text-primary, #eee);
|
||||
cursor: pointer;
|
||||
min-width: 80px;
|
||||
min-height: 44px;
|
||||
text-align: center;
|
||||
transition:
|
||||
border-color 0.15s,
|
||||
@@ -61,6 +67,11 @@ export class ShortcutCapture extends LitElement {
|
||||
opacity: 0.7;
|
||||
}
|
||||
}
|
||||
/* Reset renders only for a rebound shortcut, so a sweep of a
|
||||
freshly-installed app never sees it -- it is not in #186's
|
||||
tables for that reason, and it is a touch target the moment
|
||||
anybody uses the feature. It also has no background, so the
|
||||
padding out to 44px is invisible. */
|
||||
.reset-btn {
|
||||
font-size: var(--yj-text-xs, 11px);
|
||||
padding: 2px 6px;
|
||||
@@ -69,7 +80,8 @@ export class ShortcutCapture extends LitElement {
|
||||
background: transparent;
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
cursor: pointer;
|
||||
min-width: auto;
|
||||
min-width: 44px;
|
||||
min-height: 44px;
|
||||
opacity: 0;
|
||||
transition: opacity 0.15s;
|
||||
}
|
||||
|
||||
@@ -93,8 +93,26 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) {
|
||||
border-bottom: 1px solid var(--yj-bg-overlay, rgba(255, 255, 255, 0.08));
|
||||
}
|
||||
|
||||
/* 85x34 and 96x34 before this (#186). A tab is the only
|
||||
route to the panel it names, so it is the last control
|
||||
that should be hard to hit -- and the underline that
|
||||
marks the active one is drawn on the bottom border,
|
||||
which a taller box moves further from the label. So the
|
||||
height goes on *padding*, keeping the border against
|
||||
the label rather than 10px below a centred one.
|
||||
|
||||
The min-size is the floor and is not redundant: padding
|
||||
alone made this 44px here and **43px in CI**, because
|
||||
the total is 13 + 13 + 2 + whatever line box the font
|
||||
gives 13px text, and ubuntu:24.04's is a pixel shorter
|
||||
than this machine's. A height computed from a font's
|
||||
line box is not a height you control -- the same
|
||||
mistake #195 made about a layout property measured on
|
||||
one engine, one layer down, and caught here by the test
|
||||
rather than by a person. */
|
||||
.tab {
|
||||
padding: 8px 14px;
|
||||
min-block-size: 44px;
|
||||
padding: 13px 14px;
|
||||
font-size: 13px;
|
||||
font-weight: 600;
|
||||
color: var(--yj-text-secondary, #b3b3b3);
|
||||
|
||||
@@ -2,6 +2,7 @@ import { LitElement, html, css, nothing } from 'lit';
|
||||
import { customElement, property, state, query } from 'lit/decorators.js';
|
||||
import { classMap } from 'lit/directives/class-map.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
import { srOnly } from '../../styles/sr-only.css';
|
||||
import { unownedLabel, unownedStyles } from '@utils/ownership';
|
||||
import {
|
||||
@@ -355,6 +356,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
backButton,
|
||||
exploreLinkStyles,
|
||||
contextMenuStyles,
|
||||
srOnly,
|
||||
@@ -379,25 +381,6 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
var(--yj-border-subtle, rgba(255, 255, 255, 0.06));
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(--yj-bg-hover, rgba(255, 255, 255, 0.12));
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px;
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ import { LitElement, html, css, nothing } from 'lit';
|
||||
import { customElement, property, state, query } from 'lit/decorators.js';
|
||||
import { classMap } from 'lit/directives/class-map.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
import {
|
||||
LookupArtist,
|
||||
BrowseReleaseGroups,
|
||||
@@ -266,6 +267,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
backButton,
|
||||
exploreLinkStyles,
|
||||
contextMenuStyles,
|
||||
unownedStyles,
|
||||
@@ -289,25 +291,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
|
||||
var(--yj-border-subtle, rgba(255, 255, 255, 0.06));
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(--yj-bg-hover, rgba(255, 255, 255, 0.12));
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px;
|
||||
}
|
||||
|
||||
@@ -256,15 +256,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
margin-bottom: 10px;
|
||||
}
|
||||
|
||||
/* 89x26 and 79x26 before this (#186). */
|
||||
.search-mode-tab {
|
||||
display: inline-flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
gap: 6px;
|
||||
background: none;
|
||||
border: 1px solid transparent;
|
||||
border-radius: 6px;
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
cursor: pointer;
|
||||
min-block-size: 44px;
|
||||
padding: 5px 12px;
|
||||
font-size: var(--yj-text-sm);
|
||||
font-family: inherit;
|
||||
@@ -289,7 +292,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
border-radius: 6px;
|
||||
padding: 0 12px;
|
||||
gap: 8px;
|
||||
height: 36px;
|
||||
min-height: 44px;
|
||||
max-width: 520px;
|
||||
transition: border-color 0.15s ease;
|
||||
}
|
||||
@@ -364,8 +367,15 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
flex-shrink: 0;
|
||||
}
|
||||
|
||||
/* The input measured 325x**18** and the box around it 36,
|
||||
which is two faults rather than one (#186): the row was
|
||||
under the floor, and the input did not fill it, so eight
|
||||
of those pixels were not a target at all. The container
|
||||
is 44 and the input stretches to it -- a tap anywhere in
|
||||
the box now lands on the input rather than beside it. */
|
||||
input {
|
||||
flex: 1;
|
||||
align-self: stretch;
|
||||
background: none;
|
||||
border: none;
|
||||
outline: none;
|
||||
@@ -379,15 +389,21 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
}
|
||||
|
||||
/* No background until hover, so the target grows and the
|
||||
glyph does not. It is inside a 44px box already, hence
|
||||
the width alone. */
|
||||
.clear-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
align-self: stretch;
|
||||
background: none;
|
||||
border: none;
|
||||
color: var(--yj-text-tertiary, #888);
|
||||
cursor: pointer;
|
||||
padding: 0;
|
||||
min-inline-size: 44px;
|
||||
margin-inline-end: -12px;
|
||||
font-size: var(--yj-text-sm);
|
||||
flex-shrink: 0;
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ import { describeError } from '@utils/describe-error';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@components/track-list/track-list.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
import { list } from '@utils/binding';
|
||||
|
||||
@customElement('genre-details')
|
||||
@@ -37,7 +38,7 @@ export class GenreDetails extends LitElement {
|
||||
private scanCompleteCleanup: (() => void) | null =
|
||||
null;
|
||||
|
||||
static override styles = [designTokens, css`
|
||||
static override styles = [designTokens, backButton, css`
|
||||
:host {
|
||||
display: flex;
|
||||
flex-direction: column;
|
||||
@@ -77,31 +78,6 @@ export class GenreDetails extends LitElement {
|
||||
);
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(
|
||||
--yj-bg-overlay,
|
||||
rgba(255, 255, 255, 0.06)
|
||||
);
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(
|
||||
--yj-bg-hover,
|
||||
rgba(255, 255, 255, 0.12)
|
||||
);
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px; /* back button — outside type scale */
|
||||
}
|
||||
|
||||
@@ -23,8 +23,14 @@ export class LibraryFilter extends LitElement {
|
||||
align-items: center;
|
||||
}
|
||||
|
||||
/* 120x32 on the reference device (#186). This control has two
|
||||
placements since #57 -- the desktop top bar and Settings ->
|
||||
Libraries -- and it is the only route to setSelectedLibrary
|
||||
in either, so it is one of the controls #148 argued must not
|
||||
simply be taken away. It is one component, so it reaches the
|
||||
floor in one place. */
|
||||
select {
|
||||
height: 32px;
|
||||
min-height: 44px;
|
||||
padding: 0 8px;
|
||||
border-radius: 6px;
|
||||
border: 1px solid
|
||||
|
||||
@@ -298,6 +298,47 @@ export class PageHeader extends LitElement {
|
||||
flex-shrink: 0;
|
||||
}
|
||||
|
||||
/* Every control in this header meets the app's 44px touch
|
||||
floor -- the number #56 set for the transport and the
|
||||
queue header already keeps (#186).
|
||||
|
||||
It is min-size rather than padding with a negative
|
||||
margin, which is what the seek bar needed (#187), and
|
||||
the difference is worth stating because it decides
|
||||
whether targets can collide. There the painted track had
|
||||
to stay thin, so the target was grown past its own box
|
||||
and had to be checked against its neighbours. Here the
|
||||
control *is* the target: the boxes are flex items, so
|
||||
the gap keeps them apart and no two can overlap by
|
||||
construction.
|
||||
|
||||
There is no phone branch. With the target being the box,
|
||||
a 44px control on a desktop is merely large, and a
|
||||
second declaration of what a phone shows is a second
|
||||
thing to keep in step -- which is the reason this
|
||||
component has never had one. It also avoids a media
|
||||
query that no tier here renders, which is exactly how
|
||||
the seek bar's phone rule came to be dead for months.
|
||||
|
||||
**The height is the box and the width is not**, and that
|
||||
asymmetry is the whole of what the overflow fit below
|
||||
cares about. That pass measures inline size, so a taller
|
||||
control costs it nothing and a wider one costs it
|
||||
directly. Growing the two square controls to 44px wide
|
||||
added 22px, which fits at every width Chromium was
|
||||
checked at and clipped the overflow trigger at 320px in
|
||||
**WebKit** -- the engine closest to what actually ships,
|
||||
and the one no machine here can run. So the horizontal
|
||||
half is padding with the margin cancelling it, which is
|
||||
what the issue asked for in the first place: the target
|
||||
grows and the layout does not.
|
||||
|
||||
The cost is that a horizontal target can now overlap a
|
||||
neighbour, which the box version could not. The arrow's
|
||||
is deliberately lopsided for the seek bar's reason
|
||||
(#187): the select is 6px to its left and there is open
|
||||
space to its right, so it takes the side with nothing to
|
||||
steal from. */
|
||||
.sort select {
|
||||
font: inherit;
|
||||
color: inherit;
|
||||
@@ -306,6 +347,7 @@ export class PageHeader extends LitElement {
|
||||
border-radius: 4px;
|
||||
padding: 3px 6px;
|
||||
cursor: pointer;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
.sort-dir {
|
||||
@@ -318,6 +360,18 @@ export class PageHeader extends LitElement {
|
||||
color: inherit;
|
||||
cursor: pointer;
|
||||
padding: 3px 5px;
|
||||
/* 28x21 before this, the smallest control in the
|
||||
header and the only one that failed the floor in
|
||||
both directions.
|
||||
|
||||
Vertically the box grows, because the header has the
|
||||
room and nothing measures it. Horizontally the box
|
||||
must not: 28 + 2 + 14 is a 44px target over a 28px
|
||||
layout box, weighted right because the select is 6px
|
||||
to the left. */
|
||||
min-block-size: 44px;
|
||||
padding-inline: 5px 21px;
|
||||
margin-inline: 0 -16px;
|
||||
}
|
||||
|
||||
.sort-dir:hover {
|
||||
@@ -377,10 +431,20 @@ export class PageHeader extends LitElement {
|
||||
gap: 6px;
|
||||
white-space: nowrap;
|
||||
flex-shrink: 0;
|
||||
justify-content: center;
|
||||
min-block-size: 44px;
|
||||
}
|
||||
|
||||
.more-button {
|
||||
padding: 6px 10px;
|
||||
/* 38x27, and it is the route to every collapsed
|
||||
action, so it is the last control that should be
|
||||
hard to hit -- and the one WebKit clipped at 320px
|
||||
when this was 6px wider as a box. 38 + 3 + 3 is a
|
||||
44px target over a 38px layout box; the actions row
|
||||
has an 8px gap, so this one can be symmetric. */
|
||||
padding-inline: 13px;
|
||||
margin-inline: -3px;
|
||||
}
|
||||
|
||||
/* The display: flex above outranks the UA stylesheet's
|
||||
|
||||
@@ -71,6 +71,7 @@ import {
|
||||
exploreLinkStyles,
|
||||
} from '@utils/explore-link';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
import { list } from '@utils/binding';
|
||||
import {
|
||||
ICON_PLAYLIST,
|
||||
@@ -954,6 +955,7 @@ export class PlaylistDetails
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
backButton,
|
||||
contextMenuStyles,
|
||||
exploreLinkStyles,
|
||||
css`
|
||||
@@ -981,31 +983,6 @@ export class PlaylistDetails
|
||||
);
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(
|
||||
--yj-bg-overlay,
|
||||
rgba(255, 255, 255, 0.06)
|
||||
);
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(
|
||||
--yj-bg-hover,
|
||||
rgba(255, 255, 255, 0.12)
|
||||
);
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px;
|
||||
}
|
||||
|
||||
@@ -61,11 +61,32 @@ export class SearchTrigger extends LitElement {
|
||||
display: inline-flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
/* The smallest a touch target should be. The header's
|
||||
own action buttons are smaller because they carry a
|
||||
label; this one is a glyph. */
|
||||
min-width: 40px;
|
||||
min-height: 40px;
|
||||
/* The app's touch floor, from #56 -- and this is the
|
||||
control that should least have to argue for it: #57
|
||||
created it as the phone's replacement for the header
|
||||
search box, so it exists *only* where there is a
|
||||
thumb.
|
||||
|
||||
It shipped at 40px under a comment calling that "the
|
||||
smallest a touch target should be", which was the
|
||||
floor being restated four pixels short rather than a
|
||||
second opinion about it (#186). The rest of that
|
||||
comment said the header's own action buttons are
|
||||
smaller because they carry a label; they are 44px
|
||||
now too, so that no longer distinguishes anything.
|
||||
|
||||
The extra width is a target rather than a box, for
|
||||
page-header's reason: this button sits in that
|
||||
header, whose overflow fit (#69) measures inline
|
||||
size, and four pixels there is four pixels the
|
||||
trigger for every collapsed action does not get at
|
||||
320px. Height is free -- nothing measures it. */
|
||||
min-width: 44px;
|
||||
min-height: 44px;
|
||||
/* Border-box, so the 44 above is the whole target and
|
||||
the margin is what hands the four extra pixels back
|
||||
to the row. */
|
||||
margin-inline: -2px;
|
||||
padding: 0;
|
||||
background: none;
|
||||
border: 1px solid var(--yj-border-subtle, #555);
|
||||
|
||||
@@ -62,6 +62,7 @@ import {
|
||||
} from '@utils/explore-link';
|
||||
import '@components/smart-playlist-editor/smart-playlist-editor.js';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
import { list } from '@utils/binding';
|
||||
import {
|
||||
ICON_PLAYLIST,
|
||||
@@ -242,6 +243,7 @@ export class SmartPlaylistDetails
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
backButton,
|
||||
contextMenuStyles,
|
||||
exploreLinkStyles,
|
||||
css`
|
||||
@@ -269,31 +271,6 @@ export class SmartPlaylistDetails
|
||||
);
|
||||
}
|
||||
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 32px;
|
||||
height: 32px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(
|
||||
--yj-bg-overlay,
|
||||
rgba(255, 255, 255, 0.06)
|
||||
);
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(
|
||||
--yj-bg-hover,
|
||||
rgba(255, 255, 255, 0.12)
|
||||
);
|
||||
}
|
||||
|
||||
.back-button wa-icon {
|
||||
font-size: 16px;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
import { css } from 'lit';
|
||||
|
||||
/**
|
||||
* The way out of a detail view, at the app's 44px touch floor.
|
||||
*
|
||||
* #186's second table names `artist-details`' back button at
|
||||
* **32x32**. It is the same declaration in **six** components —
|
||||
* `artist-details`, `genre-details`, `playlist-details`,
|
||||
* `smart-playlist-details`, `explore-artist-details` and
|
||||
* `explore-album-details` — byte-identical, 32px in all six, and the
|
||||
* sweep that filed the issue visited one of them.
|
||||
*
|
||||
* That is the argument for this file rather than six edits. A device
|
||||
* sweep walks the views somebody thought to open, so six copies of a
|
||||
* control is six chances for the next pass to miss five; the arrows
|
||||
* and the toggles were each one declaration covering 36 and 29
|
||||
* controls, and this is the same shape stated the other way round.
|
||||
*
|
||||
* **It is a real 44px box, not padding with the width handed back.**
|
||||
* The header pass had to grow a hit area past its own layout box
|
||||
* because `page-header` measures itself for #69's overflow fit; a
|
||||
* detail view's header does not, so the control can simply be the
|
||||
* target. It also *should* be — this button has a visible background,
|
||||
* so a hit area larger than the circle would be a control that is
|
||||
* bigger than it looks, which is the thing #187 accepts only where a
|
||||
* thin painted track is the point.
|
||||
*
|
||||
* The size is #55's, arrived at for the same reason one component
|
||||
* over: "the way out is 44px on a phone", when the queue panel's close
|
||||
* button was 25x21 and, at phone width, the only pointer route off a
|
||||
* full-screen surface. A detail view has the platform's back gesture
|
||||
* as well, so this is less severe than the queue was — it is the same
|
||||
* control wearing the same mistake.
|
||||
*/
|
||||
export const backButton = css`
|
||||
.back-button {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 44px;
|
||||
height: 44px;
|
||||
border: none;
|
||||
border-radius: 50%;
|
||||
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
|
||||
color: var(--yj-text-primary, #fff);
|
||||
cursor: pointer;
|
||||
flex-shrink: 0;
|
||||
transition: background-color 0.15s ease;
|
||||
}
|
||||
|
||||
.back-button:hover {
|
||||
background: var(--yj-bg-hover, rgba(255, 255, 255, 0.12));
|
||||
}
|
||||
`;
|
||||
@@ -0,0 +1,48 @@
|
||||
import { css } from 'lit';
|
||||
|
||||
/**
|
||||
* A Web Awesome form control is at least the app's 44px touch floor.
|
||||
*
|
||||
* #56 named 44px and #186 found nothing but the transport had reached
|
||||
* it. Web Awesome's form controls are the part of Settings this app
|
||||
* does not draw: measured on the reference device (TLP301, 424x439),
|
||||
* `wa-input`'s control is **204x20** and `wa-button` **185x21** — the
|
||||
* shortest controls on the page, and the only ones whose height is
|
||||
* decided inside somebody else's shadow root.
|
||||
*
|
||||
* `--wa-form-control-height` is that decision, and it is the library's
|
||||
* own theming variable rather than a part or an internal — the default
|
||||
* theme sets it at `:root` and every control that has a height reads
|
||||
* it (button, input, select, radio). So this is `wa-slider-label`'s
|
||||
* better half: the API first, and no reach into a shadow root at all.
|
||||
*
|
||||
* Three things about it are load-bearing.
|
||||
*
|
||||
* **A custom property inherits through a shadow boundary**, which is
|
||||
* what lets a `:host` declaration reach a `wa-input` the host renders.
|
||||
* That is also why it is a stylesheet a component adopts rather than a
|
||||
* `:root` rule in `index.css`: a `:root` rule would cover every wa
|
||||
* control in the app in one line and be invisible to the component
|
||||
* tier, which renders a component and no page stylesheet. Here the
|
||||
* floor is measurable where it is applied.
|
||||
*
|
||||
* **It is a flat 44px rather than a floor over the library's own
|
||||
* expression.** The default is `round(calc(2 * padding-block + 1em *
|
||||
* line-height), 1px)` — em-based, so `size="small"` is what produced
|
||||
* the 20px above — and a `max(44px, …)` would have to restate that
|
||||
* formula here, which is a copy of somebody else's arithmetic that
|
||||
* goes stale silently. A flat value is safe because this app uses
|
||||
* exactly two sizes, `small` and the default, and both are under the
|
||||
* floor; a `size="large"` added later would be pinned down to 44 and
|
||||
* should take that as the prompt to revisit this.
|
||||
*
|
||||
* **Only the height is pinned.** The font size still comes from
|
||||
* `size="small"`, so a control grows its hit area without growing its
|
||||
* visual weight — which is what #186's Direction asks for and what the
|
||||
* page header's second pass had to be corrected to do.
|
||||
*/
|
||||
export const waTouchFloor = css`
|
||||
:host {
|
||||
--wa-form-control-height: 44px;
|
||||
}
|
||||
`;
|
||||
BIN
Binary file not shown.
|
Before Width: | Height: | Size: 6.4 KiB After Width: | Height: | Size: 6.5 KiB |
BIN
Binary file not shown.
|
Before Width: | Height: | Size: 4.2 KiB After Width: | Height: | Size: 4.3 KiB |
@@ -0,0 +1,230 @@
|
||||
/**
|
||||
* The controls #186's second table found, outside Settings.
|
||||
*
|
||||
* Six one-off controls across four surfaces, and the reason they are a
|
||||
* test rather than six stylesheet edits is `back-button`. The issue
|
||||
* names it in `artist-details` at **32x32**; it is the same
|
||||
* declaration, byte-identical, in *six* components — because a device
|
||||
* sweep walks the views somebody thought to open, and five of them
|
||||
* were not opened.
|
||||
*
|
||||
* So the assertion is over the whole set rather than over the one that
|
||||
* was measured. That is `icon-language.test.ts`'s shape and it is here
|
||||
* for the same reason: checking one call site checks one call site.
|
||||
*
|
||||
* | control | before | where |
|
||||
* |---|---|---|
|
||||
* | `.folders-menu-trigger` | **32x18** | autotag |
|
||||
* | `.section-toggle` | 187x**15** | autotag |
|
||||
* | `.back-button` | 32x32 | six detail views |
|
||||
* | Requests / Downloads tabs | 85x**34**, 96x**34** | downloads |
|
||||
* | `.search-mode-tab` | 89x**26**, 79x**26** | explore |
|
||||
* | explore search input | 325x**18** in a 36px box | explore |
|
||||
*
|
||||
* `page-action-check-now` (113x29) is in that table and is **not**
|
||||
* here: it is a `PageAction`, so #195 raised it with the rest of the
|
||||
* page header's actions, and `touch-targets.test.ts` already covers
|
||||
* it. Re-asserting it here would be a second statement of one rule.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import '@components/artist-details/artist-details';
|
||||
import '@components/autotag-view/autotag-view';
|
||||
import '@components/downloads-view/downloads-view';
|
||||
import '@components/explore-album-details/explore-album-details';
|
||||
import '@components/explore-artist-details/explore-artist-details';
|
||||
import '@components/explore-view/explore-view';
|
||||
import '@components/genre-details/genre-details';
|
||||
import '@components/playlist-details/playlist-details';
|
||||
import '@components/smart-playlist-details/smart-playlist-details';
|
||||
|
||||
import { flush, stub } from '@test/support/harness';
|
||||
import { fixture, shadow, shadowAll } from '@test/support/render';
|
||||
|
||||
/** The app's touch floor, from #56. */
|
||||
const FLOOR = 44;
|
||||
|
||||
/**
|
||||
* Every component that draws a back button.
|
||||
*
|
||||
* The list is here rather than derived because deriving it means
|
||||
* reading the source, and this tier renders instead — but it is
|
||||
* checked against the source by `the back button is one declaration`
|
||||
* below, so a seventh view cannot join quietly.
|
||||
*/
|
||||
const BACK_BUTTON_VIEWS = [
|
||||
'artist-details',
|
||||
'genre-details',
|
||||
'playlist-details',
|
||||
'smart-playlist-details',
|
||||
'explore-artist-details',
|
||||
'explore-album-details',
|
||||
] as const;
|
||||
|
||||
function boxOf(el: Element | null | undefined): { w: number; h: number } {
|
||||
if (!el) return { w: 0, h: 0 };
|
||||
|
||||
const box = el.getBoundingClientRect();
|
||||
|
||||
return { w: Math.round(box.width), h: Math.round(box.height) };
|
||||
}
|
||||
|
||||
describe('the way out of a detail view', () => {
|
||||
beforeEach(() => {
|
||||
for (const path of [
|
||||
'library.Library.GetTracks',
|
||||
'library.Library.GetAlbums',
|
||||
'library.Library.GetArtists',
|
||||
'library.Library.GetGenres',
|
||||
'playlist.Service.GetAllPlaylists',
|
||||
'playlist.Service.GetAllPlaylistsWithTracks',
|
||||
]) {
|
||||
stub(path, []);
|
||||
}
|
||||
});
|
||||
|
||||
it.each(BACK_BUTTON_VIEWS)('is 44px in <%s>', async (tag) => {
|
||||
// #55 settled this one component over, when the queue panel's
|
||||
// close button was 25x21 and, at phone width, the only pointer
|
||||
// route off a full-screen surface: "the way out is 44px". A detail
|
||||
// view has the platform's back gesture as well, so it is less
|
||||
// severe -- and it is the same control wearing the same mistake.
|
||||
const el = await fixture(tag);
|
||||
|
||||
await flush();
|
||||
|
||||
const back = shadow(el, '.back-button');
|
||||
|
||||
expect(back, `${tag} draws a back button`).toBeTruthy();
|
||||
expect(boxOf(back)).toEqual({ w: FLOOR, h: FLOOR });
|
||||
});
|
||||
|
||||
it('is one declaration, so a seventh view cannot miss it', async () => {
|
||||
// The regression this exists for is not a size changing -- it is
|
||||
// somebody adding a detail view and writing `.back-button` out
|
||||
// again at 32px, which is exactly how there came to be six copies.
|
||||
// A sweep of the running app would not catch it either, because a
|
||||
// sweep visits the views you think to open.
|
||||
const sources = import.meta.glob('../../src/components/**/*.ts', {
|
||||
query: '?raw',
|
||||
import: 'default',
|
||||
eager: true,
|
||||
}) as Record<string, string>;
|
||||
|
||||
expect(Object.keys(sources).length, 'the glob read something').toBeGreaterThan(0);
|
||||
|
||||
const redeclared = Object.entries(sources)
|
||||
.filter(([, src]) => /^\s*\.back-button\s*(?::[a-z-]+\s*)?\{/m.test(src))
|
||||
.map(([path]) => path);
|
||||
|
||||
expect(redeclared).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('autotag', () => {
|
||||
it('raises the two smallest controls the sweep found', async () => {
|
||||
// 187x15 and 32x18. The section toggle was the smallest control
|
||||
// measured anywhere in the app until the column arrows were
|
||||
// counted, and autotag is off by default (#25), which is
|
||||
// presumably why nobody had met either.
|
||||
const el = await fixture('autotag-view');
|
||||
|
||||
await flush();
|
||||
|
||||
for (const selector of ['.section-toggle', '.folders-menu-trigger']) {
|
||||
const control = shadowAll<HTMLElement>(el, selector).find(
|
||||
(c) => c.getBoundingClientRect().height > 0,
|
||||
);
|
||||
|
||||
if (!control) continue;
|
||||
|
||||
expect(boxOf(control).h, `${selector} height`).toBeGreaterThanOrEqual(FLOOR);
|
||||
}
|
||||
|
||||
// The stylesheet is the assertion for whichever of the two this
|
||||
// fixture does not render -- both are behind state a bare mount
|
||||
// does not reach, and a test that silently checked nothing is the
|
||||
// trap icon-language.test.ts's first assertion exists for.
|
||||
const sheet = (el.constructor as typeof HTMLElement & { styles?: unknown })
|
||||
.styles;
|
||||
|
||||
expect(String(sheet)).toContain('min-block-size: 44px');
|
||||
});
|
||||
});
|
||||
|
||||
describe('the Downloads tabs', () => {
|
||||
beforeEach(() => {
|
||||
stub('download.Service.ListDownloads', []);
|
||||
stub('download.Service.ListRequests', []);
|
||||
stub('download.Service.ListProviders', []);
|
||||
});
|
||||
|
||||
it('are the only route to their panels, and are 44px', async () => {
|
||||
const el = await fixture('downloads-view');
|
||||
|
||||
await flush();
|
||||
|
||||
const tabs = shadowAll<HTMLElement>(el, '[role="tab"]');
|
||||
|
||||
expect(tabs).toHaveLength(2);
|
||||
|
||||
for (const tab of tabs) {
|
||||
expect(boxOf(tab).h, tab.textContent?.trim()).toBeGreaterThanOrEqual(FLOOR);
|
||||
}
|
||||
});
|
||||
|
||||
it('keeps the active underline against the label', async () => {
|
||||
// The height is padding rather than a min-size, because the mark
|
||||
// for the selected tab is the bottom border -- a min-size would
|
||||
// centre the label and leave the underline 10px below it.
|
||||
const el = await fixture('downloads-view');
|
||||
|
||||
await flush();
|
||||
|
||||
const tab = shadowAll<HTMLElement>(el, '[role="tab"]')[0]!;
|
||||
const style = getComputedStyle(tab);
|
||||
|
||||
expect(parseFloat(style.paddingBlockStart)).toBeGreaterThan(8);
|
||||
expect(style.paddingBlockStart).toBe(style.paddingBlockEnd);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Explore's own search row", () => {
|
||||
beforeEach(() => {
|
||||
stub('explore.Service.GetShelves', { State: 'ready', Shelves: [] });
|
||||
stub('explore.Service.GetIndexStatus', {});
|
||||
});
|
||||
|
||||
it('raises the mode tabs', async () => {
|
||||
const el = await fixture('explore-view');
|
||||
|
||||
await flush();
|
||||
|
||||
const tabs = shadowAll<HTMLElement>(el, '.search-mode-tab');
|
||||
|
||||
expect(tabs.length).toBeGreaterThan(0);
|
||||
|
||||
for (const tab of tabs) {
|
||||
expect(boxOf(tab).h, tab.textContent?.trim()).toBeGreaterThanOrEqual(FLOOR);
|
||||
}
|
||||
});
|
||||
|
||||
it('makes the whole search box the input, not the middle 18px of it', async () => {
|
||||
// Two faults, not one: the row was 36px and the input inside it
|
||||
// was **18**, so half the box was not a target at all -- a tap
|
||||
// near the top or bottom edge landed on the container and did
|
||||
// nothing. The container is 44 and the input stretches to fill it.
|
||||
const el = await fixture('explore-view');
|
||||
|
||||
await flush();
|
||||
|
||||
const box = shadow(el, '.search-container');
|
||||
const input = shadow(el, '.search-container input');
|
||||
|
||||
expect(box, 'the search row renders').toBeTruthy();
|
||||
expect(input, 'it holds an input').toBeTruthy();
|
||||
|
||||
expect(boxOf(box).h).toBeGreaterThanOrEqual(FLOOR);
|
||||
expect(boxOf(input).h).toBeGreaterThanOrEqual(FLOOR);
|
||||
});
|
||||
});
|
||||
@@ -99,6 +99,25 @@ describe('<search-trigger>', () => {
|
||||
}
|
||||
});
|
||||
|
||||
it('meets the touch floor it was shipped four pixels under', async () => {
|
||||
stubPhone(true);
|
||||
|
||||
// #57 created this as the phone's replacement for the header search
|
||||
// box, so it exists *only* where there is a thumb -- and it shipped
|
||||
// at 40x40 under a comment calling that "the smallest a touch
|
||||
// target should be", which was the app's own 44px floor (#56)
|
||||
// restated short rather than a second opinion about it. #186.
|
||||
const el = await fixture('search-trigger');
|
||||
const button = shadow<HTMLButtonElement>(el, '[data-testid="search-trigger"]');
|
||||
|
||||
expect(button).not.toBeNull();
|
||||
|
||||
const box = button!.getBoundingClientRect();
|
||||
|
||||
expect(Math.round(box.width)).toBeGreaterThanOrEqual(44);
|
||||
expect(Math.round(box.height)).toBeGreaterThanOrEqual(44);
|
||||
});
|
||||
|
||||
it('names what the button will search', async () => {
|
||||
stubPhone(true);
|
||||
|
||||
|
||||
@@ -0,0 +1,211 @@
|
||||
/**
|
||||
* The seek bar's painted track and the thing you can hit are allowed to
|
||||
* differ, and a slider is the clearest case where they should.
|
||||
*
|
||||
* On `now-playing-view` — the screen that exists so a phone has
|
||||
* somewhere to seek from — the slider measured 261x6 on the reference
|
||||
* device (#187). Six pixels is the whole of the drag target on the
|
||||
* app's primary seeking affordance, against a 44px floor the app set
|
||||
* for itself in #56 and holds to in the queue panel.
|
||||
*
|
||||
* Two separate faults, and the first is why the second was not obvious.
|
||||
*
|
||||
* **The phone rule had never applied.** `seek-bar`'s stylesheet asked
|
||||
* for a 12px track below 599px and then set 6px in a plain `wa-slider`
|
||||
* rule *written after it*. A media query adds no specificity, so the
|
||||
* plain rule won at every width — which is `index.css`'s documented
|
||||
* rule ("the phone section is last on purpose") reproduced inside a
|
||||
* component's own stylesheet. The source said 12 and the device said 6.
|
||||
*
|
||||
* **And 12px would still be under the floor**, so the target is built
|
||||
* around the track rather than by thickening it: padding on the part
|
||||
* that carries the gesture, with margins cancelling it so the row does
|
||||
* not grow.
|
||||
*
|
||||
* This is asserted against the *parsed stylesheet*, on
|
||||
* `hover-affordance.test.ts`'s precedent and with the same limitation
|
||||
* stated rather than hidden: no tier here renders at a phone width with
|
||||
* a real `wa-slider` laid out, so what can be checked is the shape the
|
||||
* browser built from the css`` literal. The pixel measurements that
|
||||
* chose these numbers were taken on the device and are recorded on
|
||||
* #187 and in the stylesheet's own comment — a number measured on a
|
||||
* phone is not a number CI can assert.
|
||||
*
|
||||
* Which is the regression worth catching anyway. Both failures are
|
||||
* invisible on a desktop: hoisting the block back above the plain rule
|
||||
* renders identically at every width CI runs at, and it is exactly what
|
||||
* a tidy-up does.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import '@components/audio-player/seekbar/seek-bar';
|
||||
import { fixture } from '@test/support/render';
|
||||
|
||||
/** The app's touch floor, from #56. */
|
||||
const TOUCH_FLOOR = 44;
|
||||
|
||||
/** The width below which the phone's rules apply. */
|
||||
const PHONE_QUERY = /max-width:\s*599px/;
|
||||
|
||||
type Rule = { text: string; condition: string | null };
|
||||
|
||||
/**
|
||||
* Every rule in the element's own adopted stylesheets, flattened **in
|
||||
* order**, which is the whole point here: the fault being guarded is a
|
||||
* rule sitting in the wrong place, not a rule being absent.
|
||||
*/
|
||||
function rulesOf(host: Element): Rule[] {
|
||||
const sheets = host.shadowRoot?.adoptedStyleSheets ?? [];
|
||||
const out: Rule[] = [];
|
||||
|
||||
for (const sheet of sheets) {
|
||||
for (const rule of Array.from(sheet.cssRules)) {
|
||||
if (rule instanceof CSSMediaRule) {
|
||||
for (const inner of Array.from(rule.cssRules)) {
|
||||
out.push({ text: inner.cssText, condition: rule.conditionText });
|
||||
}
|
||||
|
||||
continue;
|
||||
}
|
||||
|
||||
out.push({ text: rule.cssText, condition: null });
|
||||
}
|
||||
}
|
||||
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* The two px numbers of a `*-block` declaration, as [start, end].
|
||||
*
|
||||
* A symmetric pair is **serialised back as one value** — `padding-block:
|
||||
* 16px 16px` reads as `padding-block: 16px` — so a naive pair-reader
|
||||
* fails on the shorthand rather than on the thing it is checking, and
|
||||
* says the wrong thing about why. That is not hypothetical: it is what
|
||||
* the symmetric-padding reversion did while this test was being
|
||||
* proved.
|
||||
*/
|
||||
function blockPair(text: string, property: string): [number, number] | null {
|
||||
const declaration = new RegExp(`${property}:\\s*([^;]+)`).exec(text)?.[1];
|
||||
|
||||
if (declaration === undefined) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const values = [...declaration.matchAll(/(-?[\d.]+)px/g)].map((m) =>
|
||||
Number(m[1]),
|
||||
);
|
||||
|
||||
const [start, end] = values;
|
||||
|
||||
if (start === undefined) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return [start, end ?? start];
|
||||
}
|
||||
|
||||
describe("the seek bar's phone rules", () => {
|
||||
it('are last, so they are not silently overridden', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
const rules = rulesOf(el);
|
||||
|
||||
// A sweep that read nothing passes vacuously — the same first
|
||||
// assertion icon-language.test.ts makes, for the same reason.
|
||||
expect(rules.length).toBeGreaterThan(0);
|
||||
|
||||
const declaresTrackSize = (r: Rule) => /--track-size:/.test(r.text);
|
||||
|
||||
const lastUnconditional = rules.findLastIndex(
|
||||
(r) => r.condition === null && declaresTrackSize(r),
|
||||
);
|
||||
const phoneOverride = rules.findLastIndex(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& declaresTrackSize(r),
|
||||
);
|
||||
|
||||
expect(lastUnconditional).toBeGreaterThanOrEqual(0);
|
||||
expect(phoneOverride).toBeGreaterThanOrEqual(0);
|
||||
|
||||
// A media query adds no specificity. Written first, it loses.
|
||||
expect(phoneOverride).toBeGreaterThan(lastUnconditional);
|
||||
});
|
||||
|
||||
it('give the slider a pointer target of at least the touch floor', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
const rules = rulesOf(el);
|
||||
|
||||
const track = rules.find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& /--track-size:/.test(r.text),
|
||||
);
|
||||
const target = rules.find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(track).toBeDefined();
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const trackSize = Number(
|
||||
/--track-size:\s*(-?[\d.]+)px/.exec(track!.text)?.[1],
|
||||
);
|
||||
const padding = blockPair(target!.text, 'padding-block');
|
||||
|
||||
expect(padding).not.toBeNull();
|
||||
|
||||
// The padding is on ::part(slider) rather than on the host because
|
||||
// that inner div is what carries the gesture: it has the listener
|
||||
// and the touch-action, and it is exactly the host's size, so
|
||||
// padding the host grows a box that does not take the press.
|
||||
const hitArea = trackSize + padding![0] + padding![1];
|
||||
|
||||
expect(hitArea).toBeGreaterThanOrEqual(TOUCH_FLOOR);
|
||||
});
|
||||
|
||||
it('do not grow the row they sit in', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
|
||||
const target = rulesOf(el).find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const padding = blockPair(target!.text, 'padding-block');
|
||||
const margin = blockPair(target!.text, 'margin-block');
|
||||
|
||||
expect(padding).not.toBeNull();
|
||||
expect(margin).not.toBeNull();
|
||||
|
||||
// now-playing-view's vertical budget is fixed and #51 measured
|
||||
// every pixel of it: letting the row grow by the difference cost
|
||||
// the album art 25px of 143 when it was tried on the device.
|
||||
expect(margin![0]).toBe(-padding![0]);
|
||||
expect(margin![1]).toBe(-padding![1]);
|
||||
});
|
||||
|
||||
it('take the space above, because what is below is the transport', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
|
||||
const target = rulesOf(el).find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const pair = blockPair(target!.text, 'padding-block');
|
||||
|
||||
expect(pair).not.toBeNull();
|
||||
|
||||
const [above, below] = pair!;
|
||||
|
||||
// Measured at 424x439: the seek row is 19px and the play button's
|
||||
// top edge is 8px below it, while `.art` above is a non-interactive
|
||||
// div. A symmetric target would reach into the play button — the
|
||||
// most important control on the screen — so the growth is upward.
|
||||
expect(above).toBeGreaterThan(below);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,425 @@
|
||||
/**
|
||||
* Every control in Settings is at least 44px (#186, second pass).
|
||||
*
|
||||
* The header pass covered the five controls a user meets on every
|
||||
* screen. Settings is the other half and is much the larger one: swept
|
||||
* on the reference device (TLP301, 424x439) with all eleven
|
||||
* `config-section`s expanded, **120 controls** were under the floor,
|
||||
* not the 93 the issue's first table implies, and `config-field` — the
|
||||
* row shape the issue names — is eight of them. The bulk is behind the
|
||||
* disclosures:
|
||||
*
|
||||
* | control | size | count |
|
||||
* |---|---|---|
|
||||
* | `.column-arrow-btn` | **16x14** | 36 |
|
||||
* | `.column-toggle` | 16x16 | 29 |
|
||||
* | `shortcut-capture` button | 80x**25** | 26 |
|
||||
* | download format checkbox | 16x16 | 8 |
|
||||
* | `config-field` select | 335x**30** | 7 |
|
||||
* | `wa-input` / `wa-button` | 204x**20**, 185x**21** | 6 |
|
||||
* | `library-filter` select | 120x**32** | 1 |
|
||||
* | `.overflow-btn` | 31x31 | 1 |
|
||||
*
|
||||
* **This tier can measure it, unlike #187's seek bar**, for the reason
|
||||
* the header pass gives: the controls are real elements and the rules
|
||||
* are min-sizes, so a real Chromium rendering a real component gives
|
||||
* the actual answer at any width. And unlike the header there is no
|
||||
* overflow fit on this page, so nothing here needs the negative-margin
|
||||
* treatment — height is free and the two square controls can simply be
|
||||
* square.
|
||||
*
|
||||
* **What the sweep cannot see is written down here as a test rather
|
||||
* than as a comment**, because it is the trap this whole issue keeps
|
||||
* setting. Two controls are invisible to a walk of `button, select,
|
||||
* input`: `config-field`'s toggle, whose `<input>` is
|
||||
* `opacity: 0; width: 0; height: 0` so the thing a finger hits is the
|
||||
* `<label>` around it (34x19, smaller than anything in either of the
|
||||
* issue's tables), and `shortcut-capture`'s reset button, which renders
|
||||
* only for a shortcut somebody has already rebound. Both are asserted
|
||||
* by name below.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import '@components/config-page/config-field';
|
||||
import '@components/config-page/config-page';
|
||||
import '@components/config-page/download-clients';
|
||||
import '@components/config-page/shortcut-capture';
|
||||
import '@components/library-filter/library-filter';
|
||||
|
||||
import { flush, stub } from '@test/support/harness';
|
||||
import { fixture, shadow, shadowAll } from '@test/support/render';
|
||||
|
||||
/** The app's touch floor, from #56. */
|
||||
const FLOOR = 44;
|
||||
|
||||
/**
|
||||
* Everything a finger can hit, through every shadow root under `root`.
|
||||
*
|
||||
* It descends rather than querying one root because Settings is a tree
|
||||
* of components — `config-page` renders `config-section`s holding
|
||||
* `config-field`s and `shortcut-capture`s — and the defect was
|
||||
* distributed across all of them. This is the device sweep, run here.
|
||||
*/
|
||||
function controlsUnder(root: Document | ShadowRoot | Element): { name: string; el: HTMLElement }[] {
|
||||
const SELECTOR = 'button, select, input, [role="button"], [role="tab"], [role="switch"]';
|
||||
const found: { name: string; el: HTMLElement }[] = [];
|
||||
const seen = new Set<Element>();
|
||||
|
||||
const walk = (node: ParentNode, depth: number): void => {
|
||||
if (depth > 20) return;
|
||||
|
||||
for (const el of Array.from(node.querySelectorAll('*'))) {
|
||||
if (el.matches(SELECTOR) && !seen.has(el)) {
|
||||
seen.add(el);
|
||||
|
||||
const box = el.getBoundingClientRect();
|
||||
const style = getComputedStyle(el);
|
||||
const rendered =
|
||||
(box.width > 0 || box.height > 0) &&
|
||||
style.visibility !== 'hidden' &&
|
||||
style.display !== 'none';
|
||||
|
||||
if (rendered) {
|
||||
const host = (el.getRootNode() as ShadowRoot).host;
|
||||
|
||||
found.push({
|
||||
name: `${host ? host.tagName.toLowerCase() : 'root'} ${
|
||||
(typeof el.className === 'string' && el.className) || el.tagName.toLowerCase()
|
||||
}`,
|
||||
el: el as HTMLElement,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
if (el.shadowRoot) walk(el.shadowRoot, depth + 1);
|
||||
}
|
||||
};
|
||||
|
||||
walk(root as ParentNode, 0);
|
||||
|
||||
return found;
|
||||
}
|
||||
|
||||
/**
|
||||
* What a finger actually hits for `el`.
|
||||
*
|
||||
* For everything in this app that is one element, that is the element.
|
||||
* A native checkbox is the exception and is why this function exists:
|
||||
* it cannot grow its hit area without growing its paint, and a 44px
|
||||
* checkbox is not what anyone wants — so a checkbox that has a label
|
||||
* is targeted *through* the label, which is the fix the column lists
|
||||
* and the download formats both use.
|
||||
*
|
||||
* The fallback is the checkbox itself, deliberately: a checkbox with
|
||||
* no label is a 16px target and this must still say so.
|
||||
*/
|
||||
function hitTarget(el: HTMLElement): HTMLElement {
|
||||
const input = el as HTMLInputElement;
|
||||
|
||||
if (input.type !== 'checkbox' && input.type !== 'radio') return el;
|
||||
|
||||
const wrapping = el.closest('label');
|
||||
const root = el.getRootNode() as ShadowRoot | Document;
|
||||
const associated = input.id
|
||||
? root.querySelector<HTMLLabelElement>(`label[for="${CSS.escape(input.id)}"]`)
|
||||
: null;
|
||||
|
||||
return wrapping ?? associated ?? el;
|
||||
}
|
||||
|
||||
/** The ones that miss the floor, reported with the numbers. */
|
||||
function tooSmall(controls: { name: string; el: HTMLElement }[]): string[] {
|
||||
return controls
|
||||
.map(({ name, el }) => {
|
||||
const box = hitTarget(el).getBoundingClientRect();
|
||||
|
||||
return { name, w: Math.round(box.width), h: Math.round(box.height) };
|
||||
})
|
||||
.filter((c) => c.w < FLOOR || c.h < FLOOR)
|
||||
.map((c) => `${c.name} ${c.w}x${c.h}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Open every disclosure under `host`, so the sweep can see the page.
|
||||
*
|
||||
* A collapsed `config-section` renders its body with `hidden`, so its
|
||||
* controls measure 0x0 — which is exactly why the issue's first table
|
||||
* lists seven Settings controls and the real count is 120.
|
||||
*/
|
||||
async function expandEverySection(host: HTMLElement & { updateComplete: Promise<unknown> }) {
|
||||
const sections = shadowAll<HTMLElement>(host, 'config-section');
|
||||
|
||||
expect(sections.length, 'the page renders disclosures to open').toBeGreaterThan(0);
|
||||
|
||||
for (const section of sections) {
|
||||
section.shadowRoot
|
||||
?.querySelector<HTMLButtonElement>('button[aria-expanded="false"]')
|
||||
?.click();
|
||||
}
|
||||
|
||||
await flush();
|
||||
await host.updateComplete;
|
||||
|
||||
for (const section of sections) {
|
||||
await (section as HTMLElement & { updateComplete?: Promise<unknown> }).updateComplete;
|
||||
}
|
||||
|
||||
return sections;
|
||||
}
|
||||
|
||||
/** A control's own box, named so a failure says which and how small. */
|
||||
function boxOf(el: Element | null | undefined): string {
|
||||
if (!el) return 'missing';
|
||||
|
||||
const box = el.getBoundingClientRect();
|
||||
|
||||
return `${Math.round(box.width)}x${Math.round(box.height)}`;
|
||||
}
|
||||
|
||||
function meetsFloor(el: Element | null | undefined): boolean {
|
||||
if (!el) return false;
|
||||
|
||||
const box = el.getBoundingClientRect();
|
||||
|
||||
return Math.round(box.width) >= FLOOR && Math.round(box.height) >= FLOOR;
|
||||
}
|
||||
|
||||
describe('a config field is the shape every Settings row uses', () => {
|
||||
it.each(['text', 'number', 'select', 'directory', 'color'] as const)(
|
||||
'a %s field meets the floor',
|
||||
async (type) => {
|
||||
const el = await fixture('config-field', {
|
||||
schema: {
|
||||
key: 'k',
|
||||
label: 'Music folder',
|
||||
type,
|
||||
options: [{ value: 'dark', label: 'Dark' }],
|
||||
},
|
||||
value: type === 'color' ? '#ffd43b' : '',
|
||||
});
|
||||
|
||||
const controls = controlsUnder(el.shadowRoot!);
|
||||
|
||||
// A sweep that found nothing passes vacuously.
|
||||
expect(controls.length).toBeGreaterThan(0);
|
||||
expect(tooSmall(controls)).toEqual([]);
|
||||
},
|
||||
);
|
||||
|
||||
it('grows the toggle, which no sweep of inputs can see', async () => {
|
||||
// The `<input>` is opacity: 0; width: 0; height: 0, so the walk
|
||||
// above skips it as a zero-sized node -- and the thing a finger
|
||||
// hits is the styling <label> around it, which measured 34x19 on
|
||||
// the device. It is absent from #186's tables for exactly that
|
||||
// reason, and it is smaller than everything in them.
|
||||
const el = await fixture('config-field', {
|
||||
schema: { key: 'x', label: 'Scan on startup', type: 'toggle' },
|
||||
value: true,
|
||||
});
|
||||
|
||||
const target = shadow(el, '.toggle-switch');
|
||||
|
||||
expect(boxOf(target)).toBe('44x44');
|
||||
});
|
||||
|
||||
it('keeps the toggle painted at its old size, in its old place', async () => {
|
||||
// A 44px pill is not what a switch should look like. The box is
|
||||
// 44px and the paint is not: the slider is a child centred in it,
|
||||
// and negative inline margins hand the extra width back so the
|
||||
// pill stays flush with the inputs in the rows above.
|
||||
const el = await fixture('config-field', {
|
||||
schema: { key: 'x', label: 'Scan on startup', type: 'toggle' },
|
||||
value: true,
|
||||
});
|
||||
|
||||
const target = shadow(el, '.toggle-switch') as HTMLElement;
|
||||
const slider = shadow(el, '.toggle-slider');
|
||||
|
||||
expect(slider!.getBoundingClientRect().height).toBeLessThan(FLOOR);
|
||||
|
||||
const style = getComputedStyle(target);
|
||||
const handedBack =
|
||||
parseFloat(style.marginInlineStart) + parseFloat(style.marginInlineEnd);
|
||||
|
||||
expect(handedBack).toBeLessThan(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the shortcut editor', () => {
|
||||
it('meets the floor', async () => {
|
||||
const el = await fixture('shortcut-capture', {
|
||||
action: 'play.toggle',
|
||||
label: 'Play/pause',
|
||||
currentKey: 'Space',
|
||||
defaultKey: 'Space',
|
||||
});
|
||||
|
||||
expect(meetsFloor(shadow(el, 'button'))).toBe(true);
|
||||
});
|
||||
|
||||
it('grows the reset button, which only a rebound shortcut renders', async () => {
|
||||
// Not in either of #186's tables, and it cannot be: a sweep of a
|
||||
// freshly-installed app never sees it. It appears the moment
|
||||
// anybody uses the feature.
|
||||
const el = await fixture('shortcut-capture', {
|
||||
action: 'play.toggle',
|
||||
label: 'Play/pause',
|
||||
currentKey: 'K',
|
||||
defaultKey: 'Space',
|
||||
});
|
||||
|
||||
const reset = shadow(el, '.reset-btn');
|
||||
|
||||
expect(reset, 'a rebound shortcut renders a reset button').toBeTruthy();
|
||||
expect(meetsFloor(reset)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the library filter', () => {
|
||||
it('meets the floor in both of its placements', async () => {
|
||||
// One component, two mount points since #57 -- the desktop top bar
|
||||
// and Settings -> Libraries -- so it reaches the floor once.
|
||||
const el = await fixture('library-filter');
|
||||
|
||||
const select = shadow(el, 'select');
|
||||
|
||||
expect(select, 'the filter renders a select').toBeTruthy();
|
||||
expect(Math.round(select!.getBoundingClientRect().height)).toBeGreaterThanOrEqual(FLOOR);
|
||||
});
|
||||
});
|
||||
|
||||
describe('download clients', () => {
|
||||
beforeEach(() => {
|
||||
stub('download.Service.ListProviders', []);
|
||||
stub('download.Service.ProviderKinds', []);
|
||||
stub('config.Config.GetDownloadPreferences', {});
|
||||
});
|
||||
|
||||
it('gives Web Awesome form controls the floor through the library API', async () => {
|
||||
// wa-input's control is inside somebody else's shadow root, so the
|
||||
// height comes from --wa-form-control-height rather than from a
|
||||
// rule of ours reaching in. A custom property inherits through a
|
||||
// shadow boundary, which is what makes a :host declaration reach
|
||||
// it -- and what makes it measurable here.
|
||||
const el = await fixture('download-clients');
|
||||
|
||||
await flush();
|
||||
await expandEverySection(el);
|
||||
|
||||
expect(getComputedStyle(el).getPropertyValue('--wa-form-control-height').trim()).toBe(
|
||||
'44px',
|
||||
);
|
||||
|
||||
// And the outcome, not only the mechanism. A sweep of `input`
|
||||
// reports a wa-input at 204x**42** even when this is right,
|
||||
// because the inner input sits *inside* the control's own 1px
|
||||
// border -- Web Awesome sizes it
|
||||
// `calc(--wa-form-control-height - border-width * 2)`. What a
|
||||
// finger hits is `part=base`, measured at 238x44 on the device.
|
||||
//
|
||||
// This reaches into another library's shadow root, which
|
||||
// `name-dialog.ts` only permits where the failure is bounded. It
|
||||
// is bounded the other way here: this is a test, so a renamed
|
||||
// part fails loudly rather than silently passing, which is the
|
||||
// direction that costs nobody a device session.
|
||||
const input = shadowAll<HTMLElement>(el, 'wa-input').find(
|
||||
(w) => w.getBoundingClientRect().height > 0,
|
||||
);
|
||||
|
||||
expect(input, 'the add form renders a wa-input').toBeTruthy();
|
||||
|
||||
const base = input!.shadowRoot?.querySelector('[part~="base"]');
|
||||
|
||||
expect(base, 'wa-input still calls its control box "base"').toBeTruthy();
|
||||
expect(Math.round(base!.getBoundingClientRect().height)).toBeGreaterThanOrEqual(FLOOR);
|
||||
});
|
||||
|
||||
it('makes each allowed-format checkbox label a target', async () => {
|
||||
const el = await fixture('download-clients');
|
||||
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
|
||||
// Every section starts collapsed, and a collapsed body is `hidden`
|
||||
// — so its controls measure 0x0 and a sweep of an unexpanded page
|
||||
// reports them all as fine. That is how the issue's first table
|
||||
// came to list seven Settings controls when there are 120.
|
||||
await expandEverySection(el);
|
||||
|
||||
const options = shadowAll<HTMLElement>(el, '.format-option');
|
||||
|
||||
expect(options.length).toBeGreaterThan(0);
|
||||
|
||||
const short = options
|
||||
.map((o) => ({ label: o.textContent?.trim(), h: Math.round(o.getBoundingClientRect().height) }))
|
||||
.filter((o) => o.h < FLOOR);
|
||||
|
||||
expect(short).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the whole Settings page', () => {
|
||||
beforeEach(() => {
|
||||
for (const path of [
|
||||
'library.Library.GetAllLibrariesWithTrackCounts',
|
||||
'jobs.Service.GetJobs',
|
||||
'download.Service.ListProviders',
|
||||
'download.Service.ProviderKinds',
|
||||
]) {
|
||||
stub(path, []);
|
||||
}
|
||||
|
||||
stub('config.Config.GetShortcuts', {});
|
||||
stub('config.Config.GetDownloadPreferences', {});
|
||||
stub('config.Config.GetThemeAccentColor', '#ffd43b');
|
||||
stub('config.Config.GetThemeBackgroundShade', 'dark');
|
||||
});
|
||||
|
||||
it('has no control under the floor with every section expanded', async () => {
|
||||
// The device sweep, run here: eleven collapsed sections is what
|
||||
// made the first table look like seven controls. The density is
|
||||
// behind the disclosures.
|
||||
const el = await fixture('config-page');
|
||||
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
await expandEverySection(el);
|
||||
|
||||
const controls = controlsUnder(el.shadowRoot!);
|
||||
|
||||
expect(controls.length).toBeGreaterThan(0);
|
||||
expect(tooSmall(controls)).toEqual([]);
|
||||
});
|
||||
|
||||
it('makes a column row a target by naming it, not by growing the checkbox', async () => {
|
||||
// A native checkbox cannot grow its hit area without growing its
|
||||
// paint. The label is the target instead -- which is also the
|
||||
// argument config-field already makes for its own labels, and it
|
||||
// is behaviour rather than annotation: the column's name is now a
|
||||
// click target for its checkbox.
|
||||
const el = await fixture('config-page');
|
||||
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
await expandEverySection(el);
|
||||
|
||||
const labels = shadowAll<HTMLLabelElement>(el, 'label.column-label');
|
||||
|
||||
expect(labels.length).toBeGreaterThan(0);
|
||||
|
||||
for (const label of labels) {
|
||||
const target = label.htmlFor
|
||||
? el.shadowRoot!.getElementById(label.htmlFor)
|
||||
: null;
|
||||
|
||||
expect(
|
||||
(target as HTMLInputElement | null)?.type,
|
||||
`${label.textContent?.trim()} names its checkbox`,
|
||||
).toBe('checkbox');
|
||||
expect(
|
||||
Math.round(label.getBoundingClientRect().height),
|
||||
`${label.textContent?.trim()} is a target`,
|
||||
).toBeGreaterThanOrEqual(FLOOR);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,162 @@
|
||||
/**
|
||||
* Every control a finger meets is at least 44px (#186).
|
||||
*
|
||||
* #56 sized the playback transport for a thumb and named 44px; the
|
||||
* queue header keeps it; nothing else was resized. So the controls a
|
||||
* user meets on *every* screen — the sort control, its direction
|
||||
* button, the page actions, the overflow trigger and the phone's search
|
||||
* button — sat between a third and two thirds of the app's own floor.
|
||||
* Measured on the reference device (TLP301, 424x439): `page-sort` 99x23,
|
||||
* `page-sort-direction` **28x21**, `page-actions-more` 38x27,
|
||||
* `search-trigger` 40x40.
|
||||
*
|
||||
* Unlike the seek bar's target (#187), this one can be measured here
|
||||
* rather than inferred from the stylesheet. There the painted track had
|
||||
* to stay thin, so the hit area was grown past its own box and only a
|
||||
* phone-width layout of a third-party slider could show it. Here the
|
||||
* control *is* the target, so a real Chromium rendering a real
|
||||
* `page-header` gives the actual answer — and because it is a `min-size`
|
||||
* rather than a media query, the answer is the same at every width,
|
||||
* which is what makes it checkable in this tier at all.
|
||||
*
|
||||
* That is also why there is no phone branch to test: a 44px control on
|
||||
* a desktop is merely large, and a second declaration of what a phone
|
||||
* shows is a second thing to keep in step.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import type { PageAction, PageHeader } from '@components/page-header/page-header';
|
||||
|
||||
import '@components/page-header/page-header';
|
||||
import { fixture, shadowAll } from '@test/support/render';
|
||||
|
||||
/** The app's touch floor, from #56. */
|
||||
const FLOOR = 44;
|
||||
|
||||
const SORTS = [
|
||||
{ id: 'name', label: 'Name' },
|
||||
{ id: 'tracks', label: 'Tracks' },
|
||||
];
|
||||
|
||||
function actions(): PageAction[] {
|
||||
return [
|
||||
{ id: 'import', label: 'Import', icon: 'file-import', priority: 0, onSelect: () => {} },
|
||||
{ id: 'new', label: 'New Playlist', icon: 'plus', priority: 2, onSelect: () => {} },
|
||||
];
|
||||
}
|
||||
|
||||
/** Every visible control in the header's own shadow root. */
|
||||
function controlsOf(el: PageHeader): { name: string; el: HTMLElement }[] {
|
||||
return shadowAll<HTMLElement>(el, 'button, select')
|
||||
.filter((c) => !(c as HTMLButtonElement).hidden)
|
||||
.map((c) => ({
|
||||
name: c.dataset.testid ?? (c.className || c.tagName.toLowerCase()),
|
||||
el: c,
|
||||
}));
|
||||
}
|
||||
|
||||
function tooSmall(controls: { name: string; el: HTMLElement }[]): string[] {
|
||||
return controls
|
||||
.map(({ name, el }) => {
|
||||
const b = el.getBoundingClientRect();
|
||||
|
||||
return { name, w: Math.round(b.width), h: Math.round(b.height) };
|
||||
})
|
||||
.filter((c) => c.w < FLOOR || c.h < FLOOR)
|
||||
.map((c) => `${c.name} ${c.w}x${c.h}`);
|
||||
}
|
||||
|
||||
describe("the page header's controls", () => {
|
||||
it('all meet the touch floor', async () => {
|
||||
const el = await fixture<PageHeader>('page-header', {
|
||||
heading: 'Playlists',
|
||||
count: 50,
|
||||
countNoun: 'playlist',
|
||||
sortOptions: SORTS,
|
||||
sortField: 'name',
|
||||
sortDirection: 'asc',
|
||||
actions: actions(),
|
||||
});
|
||||
|
||||
const controls = controlsOf(el);
|
||||
|
||||
// A sweep that found no controls passes vacuously — the same first
|
||||
// assertion icon-language.test.ts makes, for the same reason.
|
||||
expect(controls.length).toBeGreaterThan(0);
|
||||
|
||||
// The two that were smallest, named so a regression says which.
|
||||
expect(controls.map((c) => c.name)).toContain('page-sort-direction');
|
||||
expect(controls.map((c) => c.name)).toContain('page-sort');
|
||||
|
||||
expect(tooSmall(controls)).toEqual([]);
|
||||
});
|
||||
|
||||
it('grows the target without growing the box, so the overflow fit is untouched', async () => {
|
||||
// The regression this exists for, and it was a real one: growing
|
||||
// the two square controls to 44px *wide* added 22px to the header,
|
||||
// which fit at every width Chromium was checked at and clipped the
|
||||
// overflow trigger at 320x600 in WebKit -- the engine closest to
|
||||
// what ships, and the one no machine here can run. #69's fit pass
|
||||
// measures inline size, so a taller control is free and a wider one
|
||||
// is not.
|
||||
//
|
||||
// Negative inline margins are what keep the box out of it: the
|
||||
// padding makes the target, and the margin gives the space back.
|
||||
const el = await fixture<PageHeader>('page-header', {
|
||||
heading: 'Playlists',
|
||||
sortOptions: SORTS,
|
||||
sortField: 'name',
|
||||
actions: actions(),
|
||||
});
|
||||
|
||||
el.style.width = '320px';
|
||||
|
||||
for (let frame = 0; frame < 3; frame += 1) {
|
||||
await new Promise((r) => requestAnimationFrame(r));
|
||||
await el.updateComplete;
|
||||
}
|
||||
|
||||
for (const selector of ['.sort-dir', '.more-button']) {
|
||||
const control = shadowAll<HTMLElement>(el, selector).filter(
|
||||
(c) => !(c as HTMLButtonElement).hidden,
|
||||
)[0];
|
||||
|
||||
expect(control, selector).toBeTruthy();
|
||||
|
||||
const style = getComputedStyle(control!);
|
||||
const added =
|
||||
parseFloat(style.marginInlineStart) + parseFloat(style.marginInlineEnd);
|
||||
|
||||
expect(added, `${selector} gives its extra width back`).toBeLessThan(0);
|
||||
}
|
||||
});
|
||||
|
||||
it('includes the overflow trigger, which is the route to the rest', async () => {
|
||||
// At 320px the fit pass collapses actions into the menu, so the
|
||||
// trigger is rendered — and it is then the only way to reach them,
|
||||
// which makes it the last control that should be hard to hit.
|
||||
const el = await fixture<PageHeader>('page-header', {
|
||||
heading: 'Playlists',
|
||||
sortOptions: SORTS,
|
||||
sortField: 'name',
|
||||
actions: actions(),
|
||||
});
|
||||
|
||||
el.style.width = '320px';
|
||||
|
||||
for (let frame = 0; frame < 3; frame += 1) {
|
||||
await new Promise((r) => requestAnimationFrame(r));
|
||||
await el.updateComplete;
|
||||
}
|
||||
|
||||
const more = shadowAll<HTMLButtonElement>(el, '.more-button').filter(
|
||||
(b) => !b.hidden,
|
||||
);
|
||||
|
||||
expect(more.length).toBe(1);
|
||||
|
||||
const box = more[0]!.getBoundingClientRect();
|
||||
|
||||
expect(Math.round(box.width)).toBeGreaterThanOrEqual(FLOOR);
|
||||
expect(Math.round(box.height)).toBeGreaterThanOrEqual(FLOOR);
|
||||
});
|
||||
});
|
||||
@@ -113,6 +113,19 @@ func main() {
|
||||
slog.SetDefault(sLogger)
|
||||
sLogger.Info("starting yellowjacket", "version", version, "commit", commit)
|
||||
|
||||
// Android has no /tmp and gives an app no TMPDIR, so anything in
|
||||
// this process that spills to a temporary file is handed a path that
|
||||
// does not exist -- see system.UseTempDir. It runs here rather than
|
||||
// beside UseHomeOverride above because it has something to say when
|
||||
// it fails and the logger does not exist up there; what matters is
|
||||
// that it is before NewYellowJacketApp, which opens the database.
|
||||
//
|
||||
// A failure is not fatal: it leaves the platform's own answer in
|
||||
// place, which is what every release before this one ran with.
|
||||
if err := system.UseTempDir(application.Mobile.StoragePath()); err != nil {
|
||||
sLogger.Error("could not set up a temp directory", "err", err.Error())
|
||||
}
|
||||
|
||||
// Start profiling server (pprof + trace). In production builds this
|
||||
// is a no-op — the compiler eliminates all profiling code.
|
||||
stopProfiler := profiling.Start(sLogger)
|
||||
|
||||
Reference in New Issue
Block a user