amadan
fix: dark mode, and the row and column names it made invisible
Three bugs, one screenshot. **data-theme carried the wrong axis.** rastrillo's theme.css declares every colour as light-dark(light, dark) and its entire toggle is :root[data-theme="light"] and :root[data-theme="dark"], which re-resolve those calls by setting color-scheme. We were writing the THEME NAME — day, plain, signal — which are palette files an app picks once at scaffold, not a per-person setting. Neither rule matched, so an explicit choice never beat the OS and the Day/Plain/Signal buttons did nothing at all. The cookie now carries light, dark or auto, and auto writes no attribute, which is how "follow the OS" is spelled. page_test asserted `data-theme="day"` and passed. A test can only pin a mistake it shares; the scheme is now pinned against the toggle selectors read out of theme.css itself, so the day rastrillo moves them this fails instead of the page. **The grid CSS targeted .jexcel.** Jspreadsheet v5 renamed everything to .jss_*, so the whole override block matched nothing and the grid stayed hard-coded white inside dark chrome. **Five invented token names.** --rst-border, --rst-text-mut, --rst-warning, --rst-font-mono and --rst-shell-bar-h do not exist; the real ones are --rst-line, --rst-text-muted, --rst-tone-warning-fg and --rst-font, and the last had no equivalent at all. CSS resolves an unknown custom property to its fallback in silence, so the page looked plausible and simply was not themed. csstokens_test.go now checks every var(--rst-*) we write against the files doctor copies, and I verified it fails on the exact name that shipped. The invisible A/B/C and 1/2/3 fall out of the second and third: the library sets background-color on the headers and no color, so the text came from the page — light grey on light grey. Every rule here that sets a background now sets a foreground beside it. Measured in a real browser across all four cases (OS dark, OS light, picked dark on a light OS, picked light on a dark OS): headers 8.05:1 and 7.05:1, cells 15.3:1 and 16.9:1. Live sync across tabs still works.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
8 files changed,
+416
−154
internal/guardstest/csstokens_test.go+99 −0internal/sheets/app.go+4 −4internal/sheets/page_test.go+8 −4internal/sheets/render.go+22 −15internal/sheets/static/sheets.css+102 −43internal/sheets/templates/layout.html+15 −8internal/sheets/theme.go+66 −45internal/sheets/theme_test.go+100 −35
CI passed — run details
diff --git a/internal/guardstest/csstokens_test.go b/internal/guardstest/csstokens_test.go| new file mode 100644 |
| index 0000000..043f231 |
| --- /dev/null |
| +++ b/internal/guardstest/csstokens_test.go |
| @@ -0,0 +1,99 @@ |
| +package guards |
| + |
| +import ( |
| + "os" |
| + "path/filepath" |
| + "regexp" |
| + "sort" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// ourStylesheets are the CSS files this app writes. The vendored ones |
| +// are not checked: they define their own custom properties and know |
| +// nothing about rastrillo's. |
| +var ourStylesheets = []string{ |
| + "internal/sheets/static/sheets.css", |
| +} |
| + |
| +// tokenSources are the files that DEFINE rastrillo's tokens: the |
| +// structural ones and the theme's palette, both copied by |
| +// `rastrillo doctor` and verified by it. |
| +var tokenSources = []string{ |
| + "internal/sheets/static/tokens.css", |
| + "internal/sheets/static/theme.css", |
| +} |
| + |
| +var ( |
| + tokenUse = regexp.MustCompile(`var\(\s*(--rst-[a-zA-Z0-9-]+)`) |
| + tokenDefine = regexp.MustCompile(`(--rst-[a-zA-Z0-9-]+)\s*:`) |
| +) |
| + |
| +// TestEveryTokenWeUseExists is the check that would have caught five |
| +// invented token names shipping to a canary. |
| +// |
| +// CSS resolves an undefined custom property to its fallback without a |
| +// warning anywhere — no console message, no parse error, nothing in a |
| +// Go test. So `var(--rst-border, 1px solid #ccc)` renders a plausible |
| +// page that is simply not themed, and the only way to notice is to look |
| +// at it in both schemes and know what you expected. That is not a gate. |
| +// |
| +// The five were --rst-border, --rst-text-mut, --rst-warning, |
| +// --rst-font-mono and --rst-shell-bar-h. The real names are |
| +// --rst-line, --rst-text-muted, --rst-tone-warning-fg and --rst-font; |
| +// the last had no equivalent at all and the layout was measuring |
| +// something the framework never published. |
| +func TestEveryTokenWeUseExists(t *testing.T) { |
| + root := repoRoot(t) |
| + |
| + defined := map[string]bool{} |
| + for _, rel := range tokenSources { |
| + body, err := os.ReadFile(filepath.Join(root, rel)) |
| + if err != nil { |
| + t.Fatalf("guards: read %s: %v — the token sources are what `rastrillo doctor` copies; "+ |
| + "if one moved, this test needs updating, not deleting", rel, err) |
| + } |
| + for _, m := range tokenDefine.FindAllStringSubmatch(string(body), -1) { |
| + defined[m[1]] = true |
| + } |
| + } |
| + if len(defined) == 0 { |
| + t.Fatal("guards: found no --rst-* definitions at all; this test is not checking anything") |
| + } |
| + |
| + var used int |
| + for _, rel := range ourStylesheets { |
| + body, err := os.ReadFile(filepath.Join(root, rel)) |
| + if err != nil { |
| + t.Fatalf("guards: read %s: %v", rel, err) |
| + } |
| + var missing []string |
| + for _, m := range tokenUse.FindAllStringSubmatch(string(body), -1) { |
| + used++ |
| + if !defined[m[1]] { |
| + missing = append(missing, m[1]) |
| + } |
| + } |
| + sort.Strings(missing) |
| + for _, name := range dedupe(missing) { |
| + t.Errorf("%s: var(%s) is not defined in %s — CSS resolves an unknown "+ |
| + "custom property to its fallback in silence, so this renders a page "+ |
| + "that is simply not themed", |
| + rel, name, strings.Join(tokenSources, " or ")) |
| + } |
| + } |
| + if used == 0 { |
| + t.Fatal("guards: our stylesheets reference no tokens at all; either they stopped " + |
| + "using the design system or this test is looking in the wrong place") |
| + } |
| +} |
| + |
| +func dedupe(in []string) []string { |
| + var out []string |
| + for i, s := range in { |
| + if i == 0 || s != in[i-1] { |
| + out = append(out, s) |
| + } |
| + } |
| + return out |
| +} |
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index af3e4b4..429e3bc 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -109,10 +109,10 @@ func (a *App) Router() http.Handler { |
| // http.FileServerFS semantics, where the URL path is "/" plus the |
| // FS path. |
| r.Handle("/static/*", staticHandler()) |
| - // The theme is a per-person preference set by a form, so it works |
| - // with scripting off: a person who needs a high-contrast theme is |
| - // badly served by being told to enable JavaScript first. |
| - r.Post("/theme", a.handleTheme) |
| + // Light/dark is a per-person preference set by a form, so it works |
| + // with scripting off: somebody who needs one of them is badly |
| + // served by being told to enable JavaScript first. |
| + r.Post("/mode", a.handleMode) |
| if a.auth != nil { |
| // The four handlers rastrillo/auth expects an app to mount, at |
diff --git a/internal/sheets/page_test.go b/internal/sheets/page_test.go| index a077b8e..17ac33a 100644 |
| --- a/internal/sheets/page_test.go |
| +++ b/internal/sheets/page_test.go |
| @@ -25,13 +25,17 @@ func TestIndexServesHTML(t *testing.T) { |
| } |
| // Affordances, not copy. This test used to assert on four |
| // sentences of prose, and every one of them had to be edited the |
| - // day the page moved onto a template — which taught nothing, since |
| - // the thing worth pinning is that an anonymous visitor can get in |
| - // and that the page carries the viewer's theme. |
| + // day the page moved onto a template — which taught nothing. |
| + // |
| + // It then asserted `data-theme="day"`, which is worse than |
| + // teaching nothing: "day" is a rastrillo THEME NAME and the |
| + // attribute takes a light/dark SCHEME, so the assertion passed on |
| + // markup whose switcher did nothing at all. A test can only pin a |
| + // mistake it shares. The scheme is pinned in theme_test.go now, |
| + // against the toggle selectors in theme.css itself. |
| for _, want := range []string{ |
| "<h1>Oficina Sheets</h1>", |
| `<a href="/signin">`, // an anonymous visitor must be able to get in |
| - `data-theme="day"`, // resolved server-side, before first paint |
| } { |
| if !strings.Contains(rec.Body.String(), want) { |
| t.Errorf("body missing %q", want) |
diff --git a/internal/sheets/render.go b/internal/sheets/render.go| index 4a7ddc0..eaf9f10 100644 |
| --- a/internal/sheets/render.go |
| +++ b/internal/sheets/render.go |
| @@ -81,17 +81,22 @@ func parsePages() map[string]*template.Template { |
| // PageData is what every template receives. |
| // |
| -// Theme is resolved here, server-side, from a cookie — never fetched. |
| -// AGENTS.md: light/dark must resolve before first paint, because cell |
| -// fills resolve against the theme and a late preference repaints every |
| -// coloured cell. A page that carries its theme in its first byte cannot |
| +// The light/dark choice is resolved here, server-side, from a cookie — |
| +// never fetched. AGENTS.md: it must resolve before first paint, because |
| +// cell fills resolve against the theme and a late preference repaints |
| +// every coloured cell. A page carrying it in its first byte cannot |
| // flash; one that asks for it can. |
| type PageData struct { |
| - Title string |
| - Theme string |
| - Address string |
| - SignedIn bool |
| - Path string |
| + Title string |
| + // Mode is the viewer's light/dark choice — light, dark or auto. |
| + // ThemeAttr is what <html data-theme> gets, and is EMPTY for auto: |
| + // rastrillo's toggle is two rules keyed on that attribute, and no |
| + // attribute is how "follow the OS" is spelled. |
| + Mode string |
| + ThemeAttr string |
| + Address string |
| + SignedIn bool |
| + Path string |
| // Data is the page's own payload. Named rather than embedded so a |
| // template always says .Data.Sheets and never has to guess whether |
| // a field belongs to the page or the shell. |
| @@ -101,13 +106,15 @@ type PageData struct { |
| // pageData builds the shell fields from the request. |
| func (a *App) pageData(r *http.Request, title string, data any) PageData { |
| address, signedIn := a.identity(r) |
| + mode := modeFrom(r) |
| return PageData{ |
| - Title: title, |
| - Theme: themeFrom(r), |
| - Address: address, |
| - SignedIn: signedIn, |
| - Path: r.URL.Path, |
| - Data: data, |
| + Title: title, |
| + Mode: mode, |
| + ThemeAttr: themeAttr(mode), |
| + Address: address, |
| + SignedIn: signedIn, |
| + Path: r.URL.Path, |
| + Data: data, |
| } |
| } |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index e44b522..720a93e 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -1,30 +1,43 @@ |
| /* Sheets' own chrome, on top of rastrillo's tokens. |
| * |
| - * Every colour here is a token, never a literal. Two reasons, and the |
| - * second is the one that bites: a literal does not follow the person's |
| - * theme, and cell fills resolve against the theme, so one hard-coded |
| - * grey in the grid chrome is a grey that stays wrong in two themes out |
| - * of three. */ |
| + * Every colour here is a token, never a literal — a literal does not |
| + * follow the person's light/dark choice, and cell fills resolve against |
| + * the theme, so one hard-coded grey stays wrong in one of the two |
| + * schemes forever. |
| + * |
| + * Token names are checked mechanically by |
| + * internal/guardstest/csstokens_test.go. Five invented ones shipped |
| + * here first (--rst-border, --rst-text-mut, --rst-warning, |
| + * --rst-font-mono, --rst-shell-bar-h): CSS resolves an undefined custom |
| + * property to its fallback in silence, so the page looked plausible and |
| + * was simply not themed. The real names are --rst-line, |
| + * --rst-text-muted, --rst-tone-warning-fg and --rst-font. |
| + */ |
| /* The grid surface fills what is left of the viewport under the topbar. |
| * It is why this app has its own layout shell rather than rastrillo's: |
| * a spreadsheet is the one screen that should not sit in a padded, |
| - * max-width page. */ |
| + * max-width page. |
| + * |
| + * The topbar's height is measured by the layout rather than assumed: |
| + * there is no token for it, and a guessed constant is a gap or a |
| + * scrollbar at every width it was not guessed for. */ |
| [data-sheet-surface] { |
| display: flex; |
| flex-direction: column; |
| - height: calc(100dvh - var(--rst-shell-bar-h, 3.5rem)); |
| + height: 100dvh; |
| min-height: 0; |
| } |
| [data-sheet-bar] { |
| display: flex; |
| align-items: center; |
| - gap: var(--rst-sp-2, 0.5rem); |
| + gap: var(--rst-sp-2); |
| flex-wrap: wrap; |
| - padding: var(--rst-sp-2, 0.5rem) var(--rst-sp-3, 0.75rem); |
| - border-bottom: 1px solid var(--rst-border); |
| + padding: var(--rst-sp-2) var(--rst-sp-3); |
| + border-bottom: 1px solid var(--rst-line); |
| background: var(--rst-surface); |
| + color: var(--rst-text); |
| } |
| [data-sheet-title] { |
| @@ -37,7 +50,7 @@ |
| [data-formula-input] { |
| flex: 1 1 12rem; |
| max-inline-size: 40rem; |
| - font-family: var(--rst-font-mono, ui-monospace, monospace); |
| + font-family: ui-monospace, SFMono-Regular, Menlo, Consolas, monospace; |
| } |
| /* The live indicator. It says connected or reconnecting and nothing |
| @@ -45,62 +58,108 @@ |
| * knows is worse than no dot. */ |
| [data-sheet-live] { |
| margin-inline-start: auto; |
| - font-size: var(--rst-fs-sm, 0.875rem); |
| - color: var(--rst-text-mut); |
| + font-size: var(--rst-fs-sm); |
| + color: var(--rst-text-muted); |
| } |
| [data-sheet-live][data-state="off"] { |
| - color: var(--rst-warning, var(--rst-text-mut)); |
| + color: var(--rst-tone-warning-fg); |
| } |
| [data-sheet-grid] { |
| flex: 1; |
| min-height: 0; |
| overflow: auto; |
| + background: var(--rst-bg); |
| +} |
| + |
| +/* ── The vendored grid ─────────────────────────────────────────────── |
| + * |
| + * Jspreadsheet paints its own colours, hard-coded: #fff for the table, |
| + * #f3f3f3 for the headers, #ccc for every border. None of them follow a |
| + * theme, so all of them are restated here in tokens. |
| + * |
| + * The selectors are `.jss_*`. Jspreadsheet v5 renamed everything from |
| + * v4's `.jexcel`, and this block was first written against `.jexcel` — |
| + * so it matched nothing at all, and the grid stayed white inside dark |
| + * chrome. Check the vendored stylesheet, not a memory of the last major |
| + * version, when these stop working. |
| + * |
| + * Every rule that sets a background also sets a color. The library sets |
| + * background-color on the headers and no color, so the text is |
| + * inherited from the page: in dark mode that put light grey text on a |
| + * light grey header and the row and column names became invisible. A |
| + * background without a foreground beside it is the bug, not the |
| + * colour. */ |
| +[data-sheet-grid] .jss_worksheet { |
| + background-color: var(--rst-bg); |
| + color: var(--rst-text); |
| + border-right-color: var(--rst-line); |
| + border-bottom-color: var(--rst-line); |
| } |
| -/* Jspreadsheet paints its own colours. These override the ones that |
| - * would otherwise ignore the theme entirely — the grid is the surface |
| - * where a wrong background is most obvious, because every cell shows |
| - * it. */ |
| -[data-sheet-grid] .jexcel, |
| -[data-sheet-grid] .jexcel > tbody > tr > td, |
| -[data-sheet-grid] .jexcel > thead > tr > td { |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td { |
| background-color: var(--rst-bg); |
| color: var(--rst-text); |
| - border-color: var(--rst-border); |
| + border-top-color: var(--rst-line); |
| + border-left-color: var(--rst-line); |
| +} |
| + |
| +/* The row and column names. Both headers, one look. */ |
| +[data-sheet-grid] .jss_worksheet > thead > tr > td, |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td:first-child { |
| + background-color: var(--rst-surface-2); |
| + color: var(--rst-text-muted); |
| + border-top-color: var(--rst-line); |
| + border-left-color: var(--rst-line); |
| + font-weight: 600; |
| +} |
| + |
| +[data-sheet-grid] .jss_worksheet > thead > tr > td.selected, |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td:first-child.selected { |
| + background-color: var(--rst-accent-soft); |
| + color: var(--rst-text); |
| } |
| -[data-sheet-grid] .jexcel > thead > tr > td, |
| -[data-sheet-grid] .jexcel > tbody > tr > td:first-child { |
| - background-color: var(--rst-surface); |
| - color: var(--rst-text-mut); |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight { |
| + background-color: var(--rst-accent-soft); |
| + color: var(--rst-text); |
| +} |
| + |
| +/* The cell editor is an input the library drops into the td. Left |
| + * alone it inherits the browser's own white. */ |
| +[data-sheet-grid] .jss_worksheet td input, |
| +[data-sheet-grid] .jss_worksheet td textarea { |
| + background-color: var(--rst-bg); |
| + color: var(--rst-text); |
| } |
| -[data-sheet-grid] .jexcel > tbody > tr > td.highlight { |
| - background-color: var(--rst-accent-soft, var(--rst-surface)); |
| +[data-sheet-grid] .jss_container, |
| +[data-sheet-grid] .jss_content { |
| + background-color: var(--rst-bg); |
| } |
| /* A cell whose imported fill fails WCAG AA against its ink. Spec §11.1 |
| * ruled that we retain such a fill rather than silently correcting it, |
| * so the fill stays and this marks it instead: a corner flag that costs |
| * nothing to ignore and is impossible to miss once you look. */ |
| +[data-sheet-grid] td[data-contrast="low"] { |
| + position: relative; |
| +} |
| + |
| [data-sheet-grid] td[data-contrast="low"]::after { |
| content: ""; |
| position: absolute; |
| inset-block-start: 0; |
| inset-inline-end: 0; |
| - border-block-start: 0.5rem solid var(--rst-warning, currentColor); |
| + border-block-start: 0.5rem solid var(--rst-tone-warning-fg); |
| border-inline-start: 0.5rem solid transparent; |
| } |
| -[data-sheet-grid] td[data-contrast="low"] { |
| - position: relative; |
| -} |
| - |
| -/* The scriptless table. Not a fallback in the apologetic sense: it is |
| - * the only view that works everywhere, and every affordance the grid |
| - * has must exist here too. */ |
| +/* ── The scriptless table ──────────────────────────────────────────── |
| + * |
| + * Not a fallback in the apologetic sense: it is the only view that |
| + * works everywhere, and every affordance the grid has exists here too. */ |
| [data-cell-table] { |
| border-collapse: collapse; |
| width: 100%; |
| @@ -108,21 +167,21 @@ |
| [data-cell-table] th, |
| [data-cell-table] td { |
| - border: 1px solid var(--rst-border); |
| - padding: var(--rst-sp-1, 0.25rem) var(--rst-sp-2, 0.5rem); |
| + border: 1px solid var(--rst-line); |
| + padding: var(--rst-sp-1) var(--rst-sp-2); |
| text-align: start; |
| vertical-align: top; |
| } |
| [data-cell-table] th { |
| - background: var(--rst-surface); |
| - color: var(--rst-text-mut); |
| + background: var(--rst-surface-2); |
| + color: var(--rst-text-muted); |
| font-weight: 600; |
| } |
| [data-cell-table] td form { |
| display: flex; |
| - gap: var(--rst-sp-1, 0.25rem); |
| + gap: var(--rst-sp-1); |
| } |
| [data-cell-table] td input[type="text"] { |
| @@ -132,6 +191,6 @@ |
| [data-cell-value] { |
| display: block; |
| - color: var(--rst-text-mut); |
| - font-size: var(--rst-fs-sm, 0.875rem); |
| + color: var(--rst-text-muted); |
| + font-size: var(--rst-fs-sm); |
| } |
diff --git a/internal/sheets/templates/layout.html b/internal/sheets/templates/layout.html| index 5137f7b..cf850d2 100644 |
| --- a/internal/sheets/templates/layout.html |
| +++ b/internal/sheets/templates/layout.html |
| @@ -1,10 +1,17 @@ |
| {{define "layout"}}<!doctype html> |
| {{/* data-theme is written into the first bytes of the response, from a |
| cookie read server-side. AGENTS.md: light/dark must resolve before |
| - first paint, because cell fills resolve against the theme and a late |
| - preference repaints every coloured cell. Never move this to a fetch |
| - or to a script that runs after parse. */}} |
| -<html lang="en" dir="ltr" data-theme="{{.Theme}}"> |
| + first paint, because cell fills resolve against the theme and a |
| + late preference repaints every coloured cell. Never move this to a |
| + fetch or to a script that runs after parse. |
| + |
| + It is ABSENT for "auto", and that is the mechanism, not an |
| + omission: theme.css declares every colour as light-dark(a, b) and |
| + its entire toggle is :root[data-theme="light"] and |
| + :root[data-theme="dark"]. With no attribute the browser resolves |
| + against the OS. Writing anything else here — a theme NAME, say — |
| + matches neither rule, and the switcher silently does nothing. */}} |
| +<html lang="en" dir="ltr"{{if .ThemeAttr}} data-theme="{{.ThemeAttr}}"{{end}}> |
| <head> |
| <meta charset="utf-8"> |
| <meta name="viewport" content="width=device-width, initial-scale=1"> |
| @@ -40,11 +47,11 @@ |
| of chrome that must work with JavaScript off, because a person |
| who needs a high-contrast theme is not well served by being told |
| to enable scripting first. */}} |
| -<form method="post" action="/theme"> |
| +<form method="post" action="/mode"> |
| <input type="hidden" name="return" value="{{.Path}}"> |
| -<button rst-btn="ghost" type="submit" name="theme" value="day">Day</button> |
| -<button rst-btn="ghost" type="submit" name="theme" value="plain">Plain</button> |
| -<button rst-btn="ghost" type="submit" name="theme" value="signal">Signal</button> |
| +<button rst-btn="ghost" type="submit" name="mode" value="light"{{if eq .Mode "light"}} aria-current="true"{{end}}>Light</button> |
| +<button rst-btn="ghost" type="submit" name="mode" value="dark"{{if eq .Mode "dark"}} aria-current="true"{{end}}>Dark</button> |
| +<button rst-btn="ghost" type="submit" name="mode" value="auto"{{if eq .Mode "auto"}} aria-current="true"{{end}}>System</button> |
| </form> |
| <form method="post" action="/signout"> |
| <button rst-btn="ghost" type="submit">Sign out</button> |
diff --git a/internal/sheets/theme.go b/internal/sheets/theme.go| index 974b740..ac11d0c 100644 |
| --- a/internal/sheets/theme.go |
| +++ b/internal/sheets/theme.go |
| @@ -5,73 +5,94 @@ import ( |
| "net/url" |
| "strings" |
| "time" |
| - |
| - "github.com/carlosframework/rastrillo/ui" |
| ) |
| -// themeCookie carries the per-person theme. |
| +// modeCookie carries the viewer's light/dark preference. |
| // |
| // Per person, not per document (spec §9.3, AGENTS.md), so it is a |
| // cookie on this origin rather than a column on Sheet. It is read |
| -// server-side on every request and interpolated into the page, which is |
| -// what lets the theme resolve before first paint. |
| -const themeCookie = "sheets_theme" |
| +// server-side on every request and written into the page, which is what |
| +// lets the choice resolve before first paint. |
| +const modeCookie = "sheets_mode" |
| -// defaultTheme is the floor. rastrillo ships day, plain and signal; |
| -// anything else — a stale cookie, a hand-edited one, an attempt at path |
| -// traversal through a stylesheet href — resolves to this and never |
| -// reaches a template. |
| -const defaultTheme = "day" |
| +// The three states, and they are three rather than two on purpose: |
| +// "follow the machine" is a real preference, and an app that only |
| +// offers light and dark forces everyone to restate a choice their OS |
| +// already knows. |
| +const ( |
| + modeAuto = "auto" |
| + modeLight = "light" |
| + modeDark = "dark" |
| +) |
| -// themeCookieMaxAge is a year. A preference somebody set once should |
| -// not quietly expire back to the default a month later. |
| -const themeCookieMaxAge = 365 * 24 * 60 * 60 |
| +// modeCookieMaxAge is a year. A preference somebody set once should not |
| +// quietly expire back to the default a month later. |
| +const modeCookieMaxAge = 365 * 24 * 60 * 60 |
| -// themeFrom reads the viewer's theme. |
| +// modeFrom reads the viewer's light/dark choice, defaulting to auto. |
| // |
| -// The value is checked against the shipped theme names rather than |
| -// merely sanitised: it ends up in a data attribute and, in a design that |
| -// later switches stylesheets by name, in a URL. An allowlist of three |
| -// strings cannot be wrong in the way an escaping rule can. |
| -func themeFrom(r *http.Request) string { |
| - c, err := r.Cookie(themeCookie) |
| +// The value is checked against the three names rather than merely |
| +// sanitised: it is written into a data attribute that selects a CSS |
| +// rule, and an allowlist of three strings cannot be wrong in the way an |
| +// escaping rule can. |
| +func modeFrom(r *http.Request) string { |
| + c, err := r.Cookie(modeCookie) |
| if err != nil { |
| - return defaultTheme |
| + return modeAuto |
| } |
| - for _, name := range ui.ThemeNames() { |
| - if c.Value == name { |
| - return c.Value |
| - } |
| + switch c.Value { |
| + case modeLight, modeDark, modeAuto: |
| + return c.Value |
| } |
| - return defaultTheme |
| + return modeAuto |
| } |
| -// handleTheme sets the theme cookie and returns the person to where |
| -// they were. |
| -func (a *App) handleTheme(w http.ResponseWriter, r *http.Request) { |
| - theme := r.FormValue("theme") |
| - valid := false |
| - for _, name := range ui.ThemeNames() { |
| - if theme == name { |
| - valid = true |
| - break |
| - } |
| +// themeAttr is what goes in <html data-theme="…">, and it is empty for |
| +// auto. |
| +// |
| +// Empty matters. rastrillo's theme.css declares every colour once as |
| +// light-dark(light, dark) under `color-scheme: light dark`, and its |
| +// whole toggle is two rules — `:root[data-theme="light"]` and |
| +// `:root[data-theme="dark"]` — which re-resolve every one of those |
| +// calls by setting color-scheme. With no attribute the browser resolves |
| +// them against the OS, which IS what auto means. |
| +// |
| +// This is also the bug this function exists to prevent recurring. The |
| +// attribute used to be handed rastrillo's THEME NAME — day, plain, |
| +// signal — which are the palette files an app picks once at scaffold |
| +// (`rastrillo doctor` copies one to static/theme.css), not a per-person |
| +// setting. Neither toggle rule matched, so an explicit choice never |
| +// beat the OS and the switcher did nothing at all. Theme and scheme are |
| +// orthogonal: this app ships `day` (ruling 7) and every person picks |
| +// their own light or dark within it. |
| +func themeAttr(mode string) string { |
| + if mode == modeLight || mode == modeDark { |
| + return mode |
| } |
| - if !valid { |
| - http.Error(w, "Unknown theme.", http.StatusBadRequest) |
| + return "" |
| +} |
| + |
| +// handleMode sets the preference and returns the person to where they |
| +// were. |
| +func (a *App) handleMode(w http.ResponseWriter, r *http.Request) { |
| + mode := r.FormValue("mode") |
| + switch mode { |
| + case modeLight, modeDark, modeAuto: |
| + default: |
| + http.Error(w, "Unknown display mode.", http.StatusBadRequest) |
| return |
| } |
| http.SetCookie(w, &http.Cookie{ |
| - Name: themeCookie, |
| - Value: theme, |
| + Name: modeCookie, |
| + Value: mode, |
| Path: "/", |
| // Not HttpOnly: this is a display preference, not a |
| - // credential, and a client-side theme toggle should be able to |
| - // set it without a round trip when one is added later. |
| + // credential, and a client-side toggle should be able to set it |
| + // without a round trip when one is added. |
| SameSite: http.SameSiteLaxMode, |
| Secure: r.TLS != nil || strings.EqualFold(r.Header.Get("X-Forwarded-Proto"), "https"), |
| - MaxAge: themeCookieMaxAge, |
| - Expires: time.Now().Add(themeCookieMaxAge * time.Second), |
| + MaxAge: modeCookieMaxAge, |
| + Expires: time.Now().Add(modeCookieMaxAge * time.Second), |
| }) |
| http.Redirect(w, r, safeReturn(r.FormValue("return")), http.StatusSeeOther) |
| } |
diff --git a/internal/sheets/theme_test.go b/internal/sheets/theme_test.go| index 5a2a62f..9a39ab7 100644 |
| --- a/internal/sheets/theme_test.go |
| +++ b/internal/sheets/theme_test.go |
| @@ -6,32 +6,81 @@ import ( |
| "net/url" |
| "strings" |
| "testing" |
| + |
| + "github.com/carlosframework/rastrillo/ui" |
| ) |
| -func TestThemeDefaultsToDayWithNoCookie(t *testing.T) { |
| +func TestModeDefaultsToAutoWithNoCookie(t *testing.T) { |
| r := httptest.NewRequest(http.MethodGet, "/", nil) |
| - if got := themeFrom(r); got != defaultTheme { |
| - t.Fatalf("themeFrom with no cookie = %q, want %q", got, defaultTheme) |
| + if got := modeFrom(r); got != modeAuto { |
| + t.Fatalf("modeFrom with no cookie = %q, want %q", got, modeAuto) |
| + } |
| + // Auto means NO attribute. rastrillo's toggle is two rules keyed on |
| + // data-theme, and the browser falls back to the OS only when |
| + // neither matches — so an attribute here, of any value, would take |
| + // the OS out of the decision. |
| + if got := themeAttr(modeAuto); got != "" { |
| + t.Fatalf("themeAttr(auto) = %q, want empty so the OS decides", got) |
| } |
| } |
| -func TestThemeReadsTheCookie(t *testing.T) { |
| - r := httptest.NewRequest(http.MethodGet, "/", nil) |
| - r.AddCookie(&http.Cookie{Name: themeCookie, Value: "signal"}) |
| - if got := themeFrom(r); got != "signal" { |
| - t.Fatalf("themeFrom = %q, want %q", got, "signal") |
| +func TestModeReadsTheCookie(t *testing.T) { |
| + for _, want := range []string{modeLight, modeDark, modeAuto} { |
| + r := httptest.NewRequest(http.MethodGet, "/", nil) |
| + r.AddCookie(&http.Cookie{Name: modeCookie, Value: want}) |
| + if got := modeFrom(r); got != want { |
| + t.Errorf("modeFrom = %q, want %q", got, want) |
| + } |
| + } |
| +} |
| + |
| +// The bug this whole file exists to stop coming back: data-theme takes |
| +// the light/dark SCHEME, not rastrillo's theme NAME. day, plain and |
| +// signal are palette files an app picks once at scaffold; feeding one |
| +// to the attribute matches neither toggle rule, so an explicit choice |
| +// never beats the OS and the switcher silently does nothing. |
| +func TestThemeAttrIsASchemeNeverAThemeName(t *testing.T) { |
| + if got := themeAttr(modeLight); got != "light" { |
| + t.Errorf("themeAttr(light) = %q, want \"light\"", got) |
| + } |
| + if got := themeAttr(modeDark); got != "dark" { |
| + t.Errorf("themeAttr(dark) = %q, want \"dark\"", got) |
| + } |
| + for _, name := range ui.ThemeNames() { |
| + r := httptest.NewRequest(http.MethodGet, "/", nil) |
| + r.AddCookie(&http.Cookie{Name: modeCookie, Value: name}) |
| + if got := themeAttr(modeFrom(r)); got != "" { |
| + t.Errorf("a %q cookie produced data-theme=%q; theme names are not schemes", name, got) |
| + } |
| + } |
| +} |
| + |
| +// theme.css's toggle is exactly these two selectors. If rastrillo ever |
| +// changes them, everything above is wrong and this says so. |
| +func TestRastrillosToggleStillKeysOnLightAndDark(t *testing.T) { |
| + css, err := staticFiles().(interface { |
| + ReadFile(string) ([]byte, error) |
| + }).ReadFile("static/theme.css") |
| + if err != nil { |
| + t.Fatalf("read theme.css: %v", err) |
| + } |
| + for _, want := range []string{`[data-theme="light"]`, `[data-theme="dark"]`} { |
| + if !strings.Contains(string(css), want) { |
| + t.Fatalf("theme.css no longer carries %s — the light/dark toggle moved, "+ |
| + "and themeAttr is now writing an attribute nothing reads", want) |
| + } |
| } |
| } |
| // The value is interpolated into a data attribute and, in a design that |
| // later switches stylesheets by name, into a URL. An allowlist cannot |
| // be wrong in the way an escaping rule can. |
| -func TestThemeRejectsAnUnknownValue(t *testing.T) { |
| - for _, junk := range []string{"../../etc/passwd", "", "DAY", "day evil"} { |
| +func TestModeRejectsAnUnknownValue(t *testing.T) { |
| + for _, junk := range []string{"../../etc/passwd", "", "DARK", "dark evil", "day"} { |
| r := httptest.NewRequest(http.MethodGet, "/", nil) |
| - r.AddCookie(&http.Cookie{Name: themeCookie, Value: junk}) |
| - if got := themeFrom(r); got != defaultTheme { |
| - t.Errorf("themeFrom with cookie %q = %q, want the %q floor", junk, got, defaultTheme) |
| + r.AddCookie(&http.Cookie{Name: modeCookie, Value: junk}) |
| + if got := modeFrom(r); got != modeAuto { |
| + t.Errorf("modeFrom with cookie %q = %q, want the %q floor", junk, got, modeAuto) |
| } |
| } |
| } |
| @@ -41,55 +90,67 @@ func TestThemeRejectsAnUnknownValue(t *testing.T) { |
| // coloured cell. A page that carries its theme in its first byte cannot |
| // flash; one that fetches it can. This test fails the moment somebody |
| // moves the decision into JavaScript. |
| -func TestThemeIsInTheFirstResponseNotFetched(t *testing.T) { |
| +func TestModeIsInTheFirstResponseNotFetched(t *testing.T) { |
| rec := httptest.NewRecorder() |
| req := httptest.NewRequest(http.MethodGet, "/", nil) |
| - req.AddCookie(&http.Cookie{Name: themeCookie, Value: "signal"}) |
| + req.AddCookie(&http.Cookie{Name: modeCookie, Value: modeDark}) |
| New(Config{}).Router().ServeHTTP(rec, req) |
| - if !strings.Contains(rec.Body.String(), `data-theme="signal"`) { |
| - t.Fatal("the rendered page does not carry the theme; it would flash on load") |
| + if !strings.Contains(rec.Body.String(), `data-theme="dark"`) { |
| + t.Fatal("the rendered page does not carry the choice; it would flash on load") |
| + } |
| +} |
| + |
| +// Auto must emit no attribute at all, not an empty one: an empty |
| +// data-theme matches neither toggle rule but still looks deliberate, |
| +// and the next person to read the HTML would think it was. |
| +func TestAutoEmitsNoThemeAttributeAtAll(t *testing.T) { |
| + rec := httptest.NewRecorder() |
| + req := httptest.NewRequest(http.MethodGet, "/", nil) |
| + New(Config{}).Router().ServeHTTP(rec, req) |
| + if strings.Contains(rec.Body.String(), "data-theme") { |
| + t.Fatal("a viewer with no preference got a data-theme attribute; the OS should decide") |
| } |
| } |
| -func TestPostThemeSetsTheCookieAndReturns(t *testing.T) { |
| - form := url.Values{"theme": {"signal"}, "return": {"/sheets"}} |
| - req := httptest.NewRequest(http.MethodPost, "/theme", strings.NewReader(form.Encode())) |
| +func TestPostModeSetsTheCookieAndReturns(t *testing.T) { |
| + form := url.Values{"mode": {modeDark}, "return": {"/sheets"}} |
| + req := httptest.NewRequest(http.MethodPost, "/mode", strings.NewReader(form.Encode())) |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| rec := httptest.NewRecorder() |
| New(Config{}).Router().ServeHTTP(rec, req) |
| if rec.Code != http.StatusSeeOther { |
| - t.Fatalf("POST /theme = %d, want 303", rec.Code) |
| + t.Fatalf("POST /mode = %d, want 303", rec.Code) |
| } |
| if loc := rec.Header().Get("Location"); loc != "/sheets" { |
| t.Fatalf("Location = %q, want /sheets", loc) |
| } |
| var found bool |
| for _, c := range rec.Result().Cookies() { |
| - if c.Name == themeCookie && c.Value == "signal" { |
| + if c.Name == modeCookie && c.Value == modeDark { |
| found = true |
| } |
| } |
| if !found { |
| - t.Fatalf("no %s cookie was set: %v", themeCookie, rec.Result().Cookies()) |
| + t.Fatalf("no %s cookie was set: %v", modeCookie, rec.Result().Cookies()) |
| } |
| } |
| -func TestPostThemeRefusesAnUnknownTheme(t *testing.T) { |
| - form := url.Values{"theme": {"neon"}} |
| - req := httptest.NewRequest(http.MethodPost, "/theme", strings.NewReader(form.Encode())) |
| +func TestPostModeRefusesAnUnknownMode(t *testing.T) { |
| + form := url.Values{"mode": {"neon"}} |
| + req := httptest.NewRequest(http.MethodPost, "/mode", strings.NewReader(form.Encode())) |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| rec := httptest.NewRecorder() |
| New(Config{}).Router().ServeHTTP(rec, req) |
| if rec.Code != http.StatusBadRequest { |
| - t.Fatalf("POST /theme with an unknown theme = %d, want 400", rec.Code) |
| + t.Fatalf("POST /mode with an unknown mode = %d, want 400", rec.Code) |
| } |
| } |
| // A form field that becomes a Location header is an open redirect |
| // unless it is constrained to a path on this origin. |
| -func TestThemeReturnIsNotAnOpenRedirect(t *testing.T) { |
| +func TestModeReturnIsNotAnOpenRedirect(t *testing.T) { |
| for _, hostile := range []string{ |
| "https://evil.example.com/", |
| "//evil.example.com/", |
| @@ -107,13 +168,17 @@ func TestThemeReturnIsNotAnOpenRedirect(t *testing.T) { |
| } |
| } |
| -// The theme switcher is chrome, and chrome that needs JavaScript is |
| -// chrome a person who needs a high-contrast theme cannot reach. |
| -func TestThemeSwitcherIsAForm(t *testing.T) { |
| +// The switcher is chrome, and chrome that needs JavaScript is chrome |
| +// somebody who needs dark mode cannot reach. |
| +func TestModeSwitcherIsAForm(t *testing.T) { |
| app, cookies := signIn(t) |
| - rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/", nil), cookies)) |
| - body := rec.Body.String() |
| - if !strings.Contains(body, `action="/theme"`) { |
| - t.Fatal("the signed-in page offers no theme form") |
| + body := do(t, app, as(httptest.NewRequest(http.MethodGet, "/", nil), cookies)).Body.String() |
| + if !strings.Contains(body, `action="/mode"`) { |
| + t.Fatal("the signed-in page offers no display-mode form") |
| + } |
| + for _, want := range []string{`value="light"`, `value="dark"`, `value="auto"`} { |
| + if !strings.Contains(body, want) { |
| + t.Errorf("the switcher is missing %s", want) |
| + } |
| } |
| } |