amadan
fix: the review's findings on the selection outline
Opus review of 4a259e0. Five findings taken, two of them the ones that would have blocked a merge, and one overcall recorded with its measurement rather than argued. **The gate did not gate.** TestVendorSelectionChromeIsRestated matched the bare string `.highlight-right` against the whole of sheets.css — and the paragraph explaining the bug contains the line `.highlight-right/-bottom border: 1px solid #000`. Delete the actual rule and every guard stayed green: the prose describing the bug matched the test looking for its fix. Comments are now stripped before matching, and each entry anchors on the full scoped selector rather than the class, so a restatement written at the vendor's own specificity fails too. Proved by mutation, not by reading. Four reintroductions, each caught: the .highlight-right rule deleted; the outline written at `.jss_worksheet .highlight-top`; `border-right-color: #7ba7ff` in place of the token; the row-header override put back on the td where it matches nothing. **A wrong number, stated three times.** The selected row header was --rst-text-muted (#a8b0ba) on #dcdcdc, which is 1.60:1, not the 1.9:1 the last commit put in the permanent record. 1.91:1 is --rst-text-faint, a token that rule never used. The bug was worse than it was described. **.selection* is the fill-handle drag preview, not formula chrome.** The library adds those classes from updateCopySelection, off selectedCorner, and grid.js sets `parseFormulas: false` — this app has no formula-range highlighting to theme. The rules were right and the comment naming them would have sent the next person looking for a rule that does not exist. The anchor rule now also comes last on purpose: it ties .highlight and .selection at (0,3,3), and the anchor sits inside both mid-drag, so on a tie it has to be later or the cursor vanishes exactly while you are aiming with it. **Load order, which is the real reason the vendored #dcdcdc won.** sheet.html links the vendored stylesheets into a head block layout.html renders after sheets.css, so the library is later in source order and an EQUAL-specificity restatement loses on order alone. The header rule and the vendored one are both (0,3,3); the last commit told only the dead-selector half of that story. Ours must be strictly higher, never merely equal — now written into AGENTS.md, and pinned by a test that fails if sheets.css ever moves after that head block. **The right-click menu.** Not Jspreadsheet's — jsuites', bound to `contextmenu` whether or not the host configures one, so it is reachable on every cell and was #fff with #555 text: a white card dropped on a dark grid, and the last unthemed surface on this screen. Restated in tokens. Verified open in Chromium: #1a1f26 with #e8eaed in dark, white with #1a1d21 in light. Two more from the same pass: `td.highlight > a` was the browser's own `blue`, 2.4:1 on the fill in dark; and .highlight-selected set a background with no color, which is the exact shape this file's own comment calls the bug. Both fixed. The comment is now qualified for the things with no text in them, since the fill handle is a 1px knob. **One overcall, recorded.** The review flagged `.jss_content::-webkit-scrollbar-*` (#eee, #666) as a live gap. Measured in Chromium: .jss_content computes `overflow: visible` with scrollWidth == clientWidth, so it never has a scrollbar of its own — the grid scrolls on our [data-sheet-grid], whose bars the browser paints from color-scheme. Named in the test's exclusion list with that measurement rather than dropped. TestOurStylesheetHasNoColourLiterals is new and closes the last hole: csstokens_test.go proves every var(--rst-…) we use is defined, and says nothing about a declaration that skips var() altogether. make test green including vet, fmt, migrations and race.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiPVt2xfkKStDgkv4qLCm1
AGENTS.md+19 −9internal/guardstest/vendorpaint_test.go+147 −85internal/sheets/static/sheets.css+91 −22
CI passed — run details
Intent
<task-notification> <task-id>ac737cf6b95c2c010</task-id> <tool-use-id>toolu_01Ms5ShPjqrTmN7xe2nRzkx8</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-sheets-core/e2cc9163-61af-4161-828a-b77ea1500a39/tasks/ac737cf6b95c2c010.output</output-file> <status>completed</status> <summary>Agent "Review dark selection fix" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Reviewed. Findings ranked, most severe first. I changed nothing. Root: `/home/paulca/amadan.net/carlos/sheets/.claude/worktrees/dark-selection` ## Verdict on the two direct questions **Q1 — cascade.** Every new rule wins. I hand-counted each against both stylesheets and there is no dead rule among the additions: | new rule | spec | strongest competitor | spec | |---|---|---|---| | `…> tr > td.highlight-top` (sheets.css:187) | (0,3,3) | vendor `.jss_worksheet .highlight-top.highlight-left` :310 | (0,3,0) | | same, vs our own grid line (sheets.css:114) | (0,3,3) | (0,2,3) | | | `…> tr.selected > td:first-child` (:139) | (0,4,3) | vendor :198 **and** our own header rule :123, both (0,3,3) | | | `[data-sheet-grid] .jss_corner` (:224) | (0,2,0) | vendor `.jss_corner` :340 | (0,1,0) | The corner is reachable: `jspreadsheet.js` does `e.content.appendChild(e.corner)` and `grid.js:126` mounts into `[data-sheet-grid]`. `box-shadow: none` also beats the `-webkit-box-shadow` alias in that vendor rule. And `jspreadsheet.css:75` is `border-collapse: separate`, so the 1px dashed/dotted edges are not eaten by a neighbour's solid border under collapsed-border conflict resolution — I checked that specifically because it would have been silent. **Q4 — contrast.** `--rst-accent` on `--rst-accent-soft` = **6.59:1 dark** (`#7ba7ff` on `#17233a`), **4.75:1 light** (`#2464e0` on `#eef3fe`). Both correct, both match `theme.css:59` and `theme.css:87`. One other number is wrong — see finding 3. --- ## Findings **1. `.selection*` is mislabelled. It is the drag-fill preview, not "the range a formula is pointing at."** `sheets.css:169-172` and `sheets.css:213-214`, repeated in `vendorpaint_test.go` and the commit message. In `jspreadsheet.js` the `selection`/`selection-top`/`-bottom`/`-left`/`-right` classes are added by `updateCopySelection`, which runs off the fill-handle drag. And `grid.js:127` sets `parseFormulas: false`, so formula-range highlighting does not exist in this app at all. Failure scenario: someone drags the fill handle down five rows. The source range is `--rst-accent-soft` + solid accent; the preview is now also `--rst-accent-soft` + dotted accent. The two merge into one accent slab whose only boundary is 1px of dotted-vs-solid in an identical hue. That is parity with the vendored look (both were `rgba(0,0,0,0.05)`), so it is not a regression — but the comment tells the next person this rule is formula chrome, and they will look for the fill-preview rule and not find one. **2. The guard test goes green with the bug reintroduced.** `vendorpaint_test.go:99` is `strings.Contains(ours, ".highlight-right")`. `sheets.css:150` contains, inside the explanatory comment, the literal text `.highlight-right/-bottom border: 1px solid #000`. Proof — I deleted `sheets.css:201` (the `.highlight-right` selector line) on a copy and re-ran both guards' substring checks: all pass, and the only surviving match for `.highlight-right` is line 150, the comment. The selection's right edge then falls back to the vendor's `border-right: 1px solid #000` — invisible on `#111418`, which is the reported bug. `.highlight-top` has the same hole via lines 151 and 157, but is rescued by the second test. Fix: strip `/* … */` before matching, or match the full scoped selector (`> tbody > tr > td.highlight-right`) the way the second test does. **3. The row-header contrast figure names the wrong token and understates the problem.** Commit message, `sheets.css:136-137` and `vendorpaint_test.go:31-32` all say "`--rst-text-muted` at 1.9:1". Computed from `theme.css:157`: dark `--rst-text-muted` `#a8b0ba` on `#dcdcdc` = **1.60:1**. 1.91:1 is `--rst-text-faint` (`#98a1ac`). The rule that actually set the colour is `sheets.css:125` (`color: var(--rst-text-muted)`), so 1.60 is the real number — the bug was worse than the commit claims. **4. The new AGENTS.md bullet claims an enforcement it does not have, and the repo violates it on landing.** `AGENTS.md:145-153`: "every one the app can render must be restated in `sheets.css`. Enforced by `TestVendorSelectionChromeIsRestated`." That test covers 15 selection selectors. Vendored literals the app renders today and does not restate: - `internal/sheets/static/vendor/jspreadsheet.css:66-72` — `.jss_content::-webkit-scrollbar-track { background:#eee }`, `-thumb { background:#666 }`. `.jss_content` is the grid's own scroll container inside `[data-sheet-grid]`. In Chromium dark mode, the grid's scrollbar is light grey. - `internal/sheets/static/vendor/jsuites.css:978-1120` — the entire `.jcontextmenu` block. `grid.js` sets no `contextMenu`, and `jspreadsheet.js` binds `defaultContextMenu` to the `contextmenu` event, so right-click on any cell opens a jsuites menu painted in literals. Also, the bullet says "the vendored stylesheet" singular; `sheet.html:2-3` links two. Either narrow the bullet to selection chrome (matching the test's name) or add these to the exclusion list. `AGENTS.md:139` — "Rules here are enforced by tests, not by memory" — is the standard this misses. **5. `selectionChrome`'s docstring overclaims relative to the list.** `vendorpaint_test.go:15-17` says "every vendored rule that paints the SELECTION in a hard-coded colour and that this app can actually render". Missing, all selection chrome with literals: - `jspreadsheet.css:288` `.jss_worksheet .highlight { rgba(0,0,0,0.05) }` — restated at `sheets.css:174`, unguarded. - `jspreadsheet.css:320` `.jss_worksheet .selection { rgba(0,0,0,0.05) }` — restated at `sheets.css:215` **by this very commit**, and no guard entry was added for it. - `jspreadsheet.css:111` `.jss_worksheet > thead > tr > td.selected { #dcdcdc }` — restated at `sheets.css:138`, unguarded; only the tbody half is listed. Delete the thead half of that selector and every test stays green. - `jspreadsheet.css:270` `.jss_worksheet > tbody > tr > td.highlight > a { color: blue }` — neither restated nor named in the exclusion list. Whichever it is, it needs a line, by the commit's own standard. **6. `TestSelectionOutlineOutranksTheGridLine` does not pin specificity — it pins a string.** `vendorpaint_test.go:107-146`. Its own comment says "pins the specificity, which is the half of the bug that is invisible in a diff." If `sheets.css:114` ever gains one step — `td:not(.jss_freezed)`, an extra class, a `:where()` used wrong — it becomes (0,4,3), the outline rules stay at (0,3,3), and the original bug returns with both tests green. Name that limit in the comment, or compute specificity properly. **7. Vendor CSS loads *after* sheets.css, so ties go to the library — and the new AGENTS.md rationale omits this.** `layout.html:21` links `sheets.css`; `layout.html:24` renders `{{block "head"}}`, and `sheet.html:2-3` links `jsuites.css` and `jspreadsheet.css` there. So the vendored sheets are later in source order. The bullet at `AGENTS.md:150-153` justifies the scoped selector only by the grid-line rule's extra step, but even an *equal*-specificity restatement loses: `[data-sheet-grid] .jss_worksheet .highlight-top` is (0,3,0), tying `jspreadsheet.css:310` `.jss_worksheet .highlight-top.highlight-left` (0,3,0), and the vendor's `box-shadow` would win on order. This is also the real reason the vendored `#dcdcdc` beat our generic header rule at `sheets.css:123` — both are (0,3,3), and the vendor came last. The commit tells only the dead-selector half of that story. Cheapest durable fix: move the vendor `<link>`s above `sheets.css`, or write "strictly higher, because vendor CSS loads last" into the bullet. **8. `sheets.css` breaks the invariant it states three paragraphs earlier.** `sheets.css:101-106`: "Every rule that sets a background also sets a color… A background without a foreground beside it is the bug, not the colour." `sheets.css:183-185` sets `background-color: var(--rst-bg)` with no `color`. It happens to render fine (it inherits `--rst-text` from `sheets.css:176`, 15.32:1), but the file's own stated rule is now false in the file. **9. Source-order collision between the anchor and the fill preview.** `sheets.css:183` (`.highlight-selected` → `--rst-bg`) and `sheets.css:215` (`.selection` → `--rst-accent-soft`) are both (0,3,3), and 215 is later. With `.selection` correctly understood as the drag-fill preview (finding 1), a cell that is both the anchor and inside the preview loses its cursor. Put the anchor rule last, or give it one more step. **10. `td[data-contrast="low"]` is the same dead-rule shape this commit is written against.** `sheets.css:246-257`. `grep -rn data-contrast` across `.go`, `.js` and `.html` returns *only* `sheets.css`. Fills are imported and stored (`store.Format.FillHex`), contrast-checked (`internal/sheets/contrast.go:88-91`) and surfaced as the banner at `layout.html:70-85`, but no fill is ever painted into a cell. Pre-existing, not introduced here — but it is why the fill-interaction question is moot today. Forward risk worth writing down: when fills do land, if they are applied by class or attribute rather than an inline `style`, `sheets.css:183` at (0,3,3) will paint `--rst-bg` over a filled anchor cell's fill, and `sheets.css:174` will do the same across the whole range. **11. Nothing stops a restatement being a literal.** `csstokens_test.go` only checks that every `var(--rst-…)` used is defined. `.highlight-right { border-right-color: #7ba7ff }` passes all three guards and is wrong in light mode. **12. Process.** `AGENTS.md` Shipping cadence 2 says branches are cut from current `main`; this is cut from `core` and there is no local `main` in the worktree — confirm the stack is deliberate. §Review also requires the reviewing model recorded in the branch discussion, not just the PR body. --- ## Verified clean - All fifteen new/changed selectors take effect; no new dead rule. - `copying-*` is added *on top of* `highlight-*` (`jspreadsheet.js`), both sit in the same declaration blocks, and the vendor's `dashed` style survives the colour-only override — the marquee works both while it overlaps the selection and after the selection moves away. - `.readonly` / `.jss_freezed` / `.jss_pagination` exclusions are accurate: `grid.js` contains no `readOnly`, `freeze`, `pagination` or `editable`, and `sheet.html` does no role gating, so no read-only grid exists today. - `[data-cell-table]` is untouched and unreachable from every new selector (all scoped under `[data-sheet-grid]`). - `vendorpaint_test.go` is 146 lines, under the cap; `go vet` and `gofmt` clean; `go test ./internal/sheets/... ./internal/guardstest/` green. The two that would block me merging are **2** (the gate does not gate) and **3** (a wrong number stated three times in the permanent record). **1** and **4** are documentation that will mislead the next person.</result> <usage><subagent_tokens>118150</subagent_tokens><tool_uses>47</tool_uses><duration_ms>591214</duration_ms></usage> </task-notification>
Attributed to this commit: 171 g CO₂e (17074051 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-dark-selection, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/AGENTS.md b/AGENTS.md| index 306ee56..8d9691d 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -142,15 +142,25 @@ relying on remembering one, make it a test instead. |
| **Frontend** |
| - No build step. Jspreadsheet CE is vendored as a static asset, not fetched |
| from a CDN and not bundled. |
| -- **The vendored stylesheet's colours are literals, and every one the app |
| - can render must be restated in `sheets.css`.** Enforced by |
| - `TestVendorSelectionChromeIsRestated`, which also checks the other |
| - direction — that each vendored selector still exists, since v5 renamed |
| - every `.jexcel` to `.jss_*` once already. Restate at |
| - `> tbody > tr > td.foo`, not at the library's own |
| - `.jss_worksheet .foo`: our grid-line rule is one step more specific, so |
| - a shorter selector is a rule that parses, uses a token, and paints |
| - nothing. |
| +- **Both vendored stylesheets paint in literals, and every rule the app |
| + can render must be restated in `sheets.css`.** `TestVendorPaintIsRestated` |
| + holds the list and checks it in both directions — that each vendored |
| + selector still exists (v5 renamed every `.jexcel` to `.jss_*` once |
| + already) and that we restate it. Rules the app cannot render are named |
| + in that test with a checkable reason, never silently dropped. |
| + Two more guards sit beside it: no colour literal may appear in |
| + `sheets.css` at all, and `sheets.css` must stay linked before the head |
| + block that carries the vendored links. |
| +- **Restate at `> tbody > tr > td.foo`, never at the library's own |
| + `.jss_worksheet .foo`.** Two independent reasons, and the second is the |
| + one that bites: our grid-line rule is one specificity step higher than |
| + the library's, *and* `sheet.html` links the vendored stylesheets into a |
| + head block that `layout.html` renders after `sheets.css`, so the |
| + library is later in source order and an equal-specificity restatement |
| + loses on order alone. Ours must be strictly higher, never merely equal. |
| + Written short, a restatement parses, uses a token, and paints nothing — |
| + which is how the grid's selection outline went missing in dark for the |
| + whole of the `core` branch. |
| - **Write the attribute grammar: `rst-btn="primary compact"`, not |
| `class="rst-btn rst-btn--primary"`.** This reverses the earlier rule in |
| this file, which was written against v0.20.0 where the attribute |
diff --git a/internal/guardstest/vendorpaint_test.go b/internal/guardstest/vendorpaint_test.go| index 8423e9d..47703d9 100644 |
| --- a/internal/guardstest/vendorpaint_test.go |
| +++ b/internal/guardstest/vendorpaint_test.go |
| @@ -3,18 +3,20 @@ package guards |
| import ( |
| "os" |
| "path/filepath" |
| + "regexp" |
| "strings" |
| "testing" |
| ) |
| -// The vendored Jspreadsheet stylesheet, and ours. |
| +// The vendored stylesheets, and ours. |
| const ( |
| - vendorSheet = "internal/sheets/static/vendor/jspreadsheet.css" |
| - ourSheet = "internal/sheets/static/sheets.css" |
| + vendorGrid = "internal/sheets/static/vendor/jspreadsheet.css" |
| + vendorMenu = "internal/sheets/static/vendor/jsuites.css" |
| + ourSheet = "internal/sheets/static/sheets.css" |
| ) |
| -// selectionChrome is every vendored rule that paints the SELECTION in a |
| -// hard-coded colour and that this app can actually render. |
| +// vendorPaint is every vendored rule that paints in a hard-coded colour |
| +// and that this app can actually render. |
| // |
| // sheets.css restated the vendored greys for the grid, the headers and |
| // the range fill, and stopped there. It did not restate the four |
| @@ -22,125 +24,185 @@ const ( |
| // borders, .jss_corner, or the selected row's header — so in dark mode |
| // the range fill was drawn and its outline was not: |
| // |
| -// - highlight-right/-bottom border 1px solid #000 → invisible on #111418. |
| +// - highlight-right/-bottom border 1px solid #000 → invisible on #111418. |
| // - highlight-top/-left their border is overwritten by our own |
| // `td { border-top-color: var(--rst-line) }`, one specificity step |
| // higher, so the outline became an ordinary grid line — while their |
| // decorative `box-shadow: -1px 0 #ccc` survived and became the one |
| // bright edge on screen. A selection lit down its left side and |
| // nowhere else. |
| -// - tr.selected > td:first-child #dcdcdc → a light slab in a dark |
| -// grid, carrying --rst-text-muted at 1.9:1. |
| +// - tr.selected > td:first-child #dcdcdc → a light slab in a dark |
| +// grid, carrying --rst-text-muted (#a8b0ba) at 1.60:1. |
| // - .jss_corner a black knob ringed in white → reads inverted. |
| +// - jsuites' .jcontextmenu #fff with #555 text → a white card on a |
| +// dark grid, on every cell's right-click. |
| // |
| // The row header is the instructive one. sheets.css DID write an |
| // override for it — `tbody > tr > td:first-child.selected` — but |
| -// Jspreadsheet puts `.selected` on the <tr>, not on the <td>, so the |
| -// rule matched nothing and looked like a fix. A CSS rule that matches |
| -// nothing fails exactly like a rule nobody wrote. |
| +// Jspreadsheet puts `.selected` on the <tr>, not on its first cell, so |
| +// the rule matched nothing and looked like a fix. A CSS rule that |
| +// matches nothing fails exactly like a rule nobody wrote. |
| // |
| // Each entry is checked in BOTH directions: the vendored selector must |
| -// still exist (v5 renamed every `.jexcel` to `.jss_*`, and a stale list |
| -// like this one is how that rename goes unnoticed a second time), and |
| -// ours must restate it. |
| +// still exist (v5 renamed every `.jexcel` to `.jss_*` once already, and a |
| +// stale list like this one is how that rename goes unnoticed a second |
| +// time), and ours must restate it. |
| // |
| -// Vendored rules with hard-coded colours that are deliberately NOT here, |
| -// because this app never renders them — if one starts rendering it |
| -// belongs in the list: |
| +// Vendored rules with hard-coded colours that are deliberately NOT here. |
| +// Each reason is checkable against the code, and if one stops holding, |
| +// the rule belongs in the list: |
| // |
| -// .readonly no column sets readOnly and nothing calls |
| -// setReadOnly; grid.js contains neither word. |
| -// .jss_freezed grid.js passes no freezeColumns or freezeRows. |
| -// .jss_pagination a sheet is one page; pagination is off. |
| -// .jupload, editors we do not enable — the cell editor is the |
| -// .jss_richtext plain text one. |
| -var selectionChrome = []struct { |
| - vendor string // must still exist in the vendored stylesheet |
| - restated string // sheets.css must carry this selector fragment |
| +// .readonly no column sets readOnly and nothing calls |
| +// setReadOnly; grid.js contains neither word. |
| +// .jss_freezed grid.js passes no freezeColumns or freezeRows. |
| +// .jss_pagination a sheet is one page; pagination is off. |
| +// .jupload, editors we do not enable — the cell editor is the |
| +// .jss_richtext plain text one. |
| +// .jss_content ::-webkit-scrollbar-track/-thumb, #eee and #666. |
| +// scrollbar Measured in Chromium: .jss_content computes |
| +// `overflow: visible` with scrollWidth == |
| +// clientWidth, so it never has a scrollbar of its |
| +// own. The grid scrolls on OUR [data-sheet-grid] |
| +// (overflow: auto), whose bars the browser paints |
| +// from color-scheme. |
| +var vendorPaint = []struct { |
| + file string // the vendored stylesheet it lives in |
| + vendor string // must still exist in that stylesheet |
| + restated string // sheets.css must carry this selector, in a RULE not a comment |
| }{ |
| - {".jss_worksheet > tbody > tr.selected > td:first-child", "tr.selected > td:first-child"}, |
| + {vendorGrid, ".jss_worksheet > thead > tr > td.selected", "> thead > tr > td.selected"}, |
| + {vendorGrid, ".jss_worksheet > tbody > tr.selected > td:first-child", "> tbody > tr.selected > td:first-child"}, |
| - {".jss_worksheet .highlight-top", ".highlight-top"}, |
| - {".jss_worksheet .highlight-left", ".highlight-left"}, |
| - {".jss_worksheet .highlight-right", ".highlight-right"}, |
| - {".jss_worksheet .highlight-bottom", ".highlight-bottom"}, |
| - {".jss_worksheet .highlight-selected", ".highlight-selected"}, |
| + {vendorGrid, ".jss_worksheet .highlight ", "> tbody > tr > td.highlight "}, |
| + {vendorGrid, ".jss_worksheet .highlight-top", "> tbody > tr > td.highlight-top"}, |
| + {vendorGrid, ".jss_worksheet .highlight-left", "> tbody > tr > td.highlight-left"}, |
| + {vendorGrid, ".jss_worksheet .highlight-right", "> tbody > tr > td.highlight-right"}, |
| + {vendorGrid, ".jss_worksheet .highlight-bottom", "> tbody > tr > td.highlight-bottom"}, |
| + {vendorGrid, ".jss_worksheet .highlight-selected", "> tbody > tr > td.highlight-selected"}, |
| + {vendorGrid, ".jss_worksheet > tbody > tr > td.highlight > a", "> tbody > tr > td.highlight > a"}, |
| - {".jss_worksheet .selection-top", ".selection-top"}, |
| - {".jss_worksheet .selection-bottom", ".selection-bottom"}, |
| - {".jss_worksheet .selection-left", ".selection-left"}, |
| - {".jss_worksheet .selection-right", ".selection-right"}, |
| + {vendorGrid, ".jss_worksheet .selection\n", "> tbody > tr > td.selection "}, |
| + {vendorGrid, ".jss_worksheet .selection-top", "> tbody > tr > td.selection-top"}, |
| + {vendorGrid, ".jss_worksheet .selection-bottom", "> tbody > tr > td.selection-bottom"}, |
| + {vendorGrid, ".jss_worksheet .selection-left", "> tbody > tr > td.selection-left"}, |
| + {vendorGrid, ".jss_worksheet .selection-right", "> tbody > tr > td.selection-right"}, |
| - {".jss_worksheet .copying-top", ".copying-top"}, |
| - {".jss_worksheet .copying-bottom", ".copying-bottom"}, |
| - {".jss_worksheet .copying-left", ".copying-left"}, |
| - {".jss_worksheet .copying-right", ".copying-right"}, |
| + {vendorGrid, ".jss_worksheet .copying-top", "> tbody > tr > td.copying-top"}, |
| + {vendorGrid, ".jss_worksheet .copying-bottom", "> tbody > tr > td.copying-bottom"}, |
| + {vendorGrid, ".jss_worksheet .copying-left", "> tbody > tr > td.copying-left"}, |
| + {vendorGrid, ".jss_worksheet .copying-right", "> tbody > tr > td.copying-right"}, |
| - {".jss_corner", ".jss_corner"}, |
| + {vendorGrid, ".jss_corner", "[data-sheet-grid] .jss_corner"}, |
| + |
| + {vendorMenu, ".jcontextmenu {", "[data-sheet-grid] .jcontextmenu {"}, |
| + {vendorMenu, ".jcontextmenu > div a", "[data-sheet-grid] .jcontextmenu > div a"}, |
| + {vendorMenu, ".jcontextmenu > div:hover", "[data-sheet-grid] .jcontextmenu > div:hover"}, |
| + {vendorMenu, ".jcontextmenu .jcontextmenu-disabled a", ".jcontextmenu .jcontextmenu-disabled a"}, |
| + {vendorMenu, ".jcontextmenu hr", "[data-sheet-grid] .jcontextmenu hr"}, |
| } |
| -func TestVendorSelectionChromeIsRestated(t *testing.T) { |
| - root := repoRoot(t) |
| +// comments strips /* … */ so a selector NAMED in an explanatory comment |
| +// cannot stand in for the rule itself. |
| +// |
| +// This is not hypothetical tidying. The first draft of these tests |
| +// matched bare `.highlight-right` against the whole file, and the |
| +// paragraph above the rules contains the line |
| +// `.highlight-right/-bottom border: 1px solid #000` — so deleting the |
| +// actual rule left every guard green and the reported bug back. The |
| +// prose describing a bug matched the test looking for its fix. |
| +var comments = regexp.MustCompile(`(?s)/\*.*?\*/`) |
| - vendor, err := os.ReadFile(filepath.Join(root, vendorSheet)) |
| +func rulesOnly(t *testing.T, rel string) string { |
| + t.Helper() |
| + body, err := os.ReadFile(filepath.Join(repoRoot(t), rel)) |
| if err != nil { |
| - t.Fatalf("guards: read %s: %v", vendorSheet, err) |
| + t.Fatalf("guards: read %s: %v", rel, err) |
| } |
| - ours, err := os.ReadFile(filepath.Join(root, ourSheet)) |
| - if err != nil { |
| - t.Fatalf("guards: read %s: %v", ourSheet, err) |
| + // The vendored files are CRLF and ours is LF; normalise so an entry |
| + // above can anchor on a line break without caring which. |
| + return comments.ReplaceAllString(strings.ReplaceAll(string(body), "\r\n", "\n"), "") |
| +} |
| + |
| +func TestVendorPaintIsRestated(t *testing.T) { |
| + ours := rulesOnly(t, ourSheet) |
| + vendors := map[string]string{ |
| + vendorGrid: rulesOnly(t, vendorGrid), |
| + vendorMenu: rulesOnly(t, vendorMenu), |
| } |
| - for _, c := range selectionChrome { |
| - if !strings.Contains(string(vendor), c.vendor) { |
| + for _, c := range vendorPaint { |
| + if !strings.Contains(vendors[c.file], c.vendor) { |
| t.Errorf("%s: no rule for %q — the vendored library renamed or dropped it "+ |
| "(v5 renamed every .jexcel to .jss_*). Re-derive this list from the "+ |
| "vendored stylesheet; deleting the entry hides the rule, it does not fix it", |
| - vendorSheet, c.vendor) |
| + c.file, c.vendor) |
| continue |
| } |
| - if !strings.Contains(string(ours), c.restated) { |
| - t.Errorf("%s: %s is painted in a hard-coded colour by %s and is not restated here — "+ |
| - "a literal does not follow the person's light/dark choice, so the selection "+ |
| - "loses its outline in one of the two schemes", |
| - ourSheet, c.restated, vendorSheet) |
| + if !strings.Contains(ours, c.restated) { |
| + t.Errorf("%s: %s is painted in a hard-coded colour by %s and no RULE here restates it — "+ |
| + "a literal does not follow the person's light/dark choice, so this surface "+ |
| + "is wrong in one of the two schemes. (Comments are stripped before this "+ |
| + "check: naming the selector in prose does not count.)", |
| + ourSheet, c.restated, c.file) |
| } |
| } |
| } |
| -// outlineCollidesWithGridLine are the selection-outline classes that set |
| -// the same two properties as our own grid line, and so must be written |
| -// at the same specificity or higher. |
| +// The vendored stylesheets are linked from sheet.html's head block, |
| +// which layout.html renders AFTER sheets.css — so the library is later |
| +// in source order and an EQUAL-specificity restatement loses. Ours has |
| +// to be strictly higher, which for the grid means carrying the same |
| +// `> tbody > tr > td` the grid-line rule carries. That is why every |
| +// `restated` string above is the full scoped selector rather than the |
| +// bare class: written short, the rule parses, uses a token, and paints |
| +// nothing. |
| // |
| -// This is the half of the bug that is invisible in a diff. sheets.css |
| -// sets border-top-color and border-left-color on every body cell at |
| -// (0,2,3); the vendored `.jss_worksheet .highlight-top` is (0,2,0). A |
| -// restatement written at the vendor's specificity loses to our own grid |
| -// line and paints nothing: the rule is there, the colour is a token, and |
| -// the outline still does not appear. Scoping it through |
| -// `> tbody > tr > td` the way the grid-line rule is scoped puts it back |
| -// on top. |
| -var outlineCollidesWithGridLine = []string{ |
| - "> tbody > tr > td.highlight-top", |
| - "> tbody > tr > td.highlight-left", |
| - "> tbody > tr > td.selection-top", |
| - "> tbody > tr > td.selection-left", |
| - "> tbody > tr > td.copying-top", |
| - "> tbody > tr > td.copying-left", |
| -} |
| - |
| -func TestSelectionOutlineOutranksTheGridLine(t *testing.T) { |
| +// The limit of this check, stated so nobody trusts it further than it |
| +// goes: it compares strings, not specificity. If the grid-line rule at |
| +// the top of sheets.css ever gains a step of its own — an added class, |
| +// a `:not()`, a misused `:where()` — these selectors stay green and stop |
| +// winning. Recount by hand when you touch that rule. |
| +func TestVendorLinksLoadAfterOurs(t *testing.T) { |
| root := repoRoot(t) |
| - body, err := os.ReadFile(filepath.Join(root, ourSheet)) |
| + layout, err := os.ReadFile(filepath.Join(root, "internal/sheets/templates/layout.html")) |
| if err != nil { |
| - t.Fatalf("guards: read %s: %v", ourSheet, err) |
| + t.Skipf("guards: read layout.html: %v", err) |
| } |
| - for _, sel := range outlineCollidesWithGridLine { |
| - if !strings.Contains(string(body), sel) { |
| - t.Errorf("%s: expected a rule scoped `%s`. Written any shorter it is less "+ |
| - "specific than our own `td { border-top-color: var(--rst-line) }`, and the "+ |
| - "selection's top and left edges resolve to the grid line instead of to the "+ |
| - "outline colour", ourSheet, sel) |
| + sheet, err := os.ReadFile(filepath.Join(root, "internal/sheets/templates/sheet.html")) |
| + if err != nil { |
| + t.Skipf("guards: read sheet.html: %v", err) |
| + } |
| + ours := strings.Index(string(layout), "sheets.css") |
| + head := strings.Index(string(layout), `"head"`) |
| + if ours < 0 || head < 0 { |
| + t.Skip("guards: layout.html no longer links sheets.css before a head block") |
| + } |
| + if !strings.Contains(string(sheet), "jspreadsheet.css") { |
| + t.Skip("guards: sheet.html no longer links the vendored stylesheet") |
| + } |
| + if ours > head { |
| + t.Errorf("layout.html: sheets.css is linked after the head block that carries the "+ |
| + "vendored stylesheets (%d > %d). That reverses the load order these overrides "+ |
| + "were counted against — recheck every restatement in %s, since ties now go "+ |
| + "to us and some may be over-specified", ours, head, ourSheet) |
| + } |
| +} |
| + |
| +// TestOurStylesheetHasNoColourLiterals is the other half of the token |
| +// rule, and the one the token-name check cannot make. |
| +// |
| +// csstokens_test.go proves every `var(--rst-…)` we use is defined. It |
| +// says nothing about a declaration that skips var() altogether: |
| +// `border-right-color: #7ba7ff` passes that check, passes the |
| +// restatement check above, and is wrong in light mode forever. |
| +func TestOurStylesheetHasNoColourLiterals(t *testing.T) { |
| + css := rulesOnly(t, ourSheet) |
| + literal := regexp.MustCompile(`(?i)(#[0-9a-f]{3,8}\b|\brgba?\(|\bhsla?\(|:\s*(red|blue|green|black|white|gray|grey|orange|yellow|purple)\b)`) |
| + for i, line := range strings.Split(css, "\n") { |
| + if m := literal.FindString(line); m != "" { |
| + t.Errorf("%s:%d: colour literal %q — every colour here is a token, never a "+ |
| + "literal, because a literal cannot follow the person's light/dark choice. "+ |
| + "Line: %s", ourSheet, i+1, m, strings.TrimSpace(line)) |
| } |
| } |
| } |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index a451d27..16cff30 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -98,12 +98,13 @@ |
| * 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. */ |
| + * Every rule that sets a background on something carrying text 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. (The exceptions are the things with no text in |
| + * them — the fill handle is a 1px knob.) */ |
| [data-sheet-grid] .jss_worksheet { |
| background-color: var(--rst-bg); |
| color: var(--rst-text); |
| @@ -133,8 +134,8 @@ |
| * two halves need different selectors. Written as `td:first-child.selected` |
| * the second half matched nothing at all, and a rule that matches nothing |
| * fails exactly like a rule nobody wrote: the vendored #dcdcdc stood, and |
| - * a dark grid grew a light-grey slab down its left edge carrying muted |
| - * grey text at 1.9:1. */ |
| + * a dark grid grew a light-grey slab down its left edge carrying |
| + * --rst-text-muted (#a8b0ba) at 1.60:1. */ |
| [data-sheet-grid] .jss_worksheet > thead > tr > td.selected, |
| [data-sheet-grid] .jss_worksheet > tbody > tr.selected > td:first-child { |
| background-color: var(--rst-accent-soft); |
| @@ -160,15 +161,28 @@ |
| * one bright thing on screen. What you get is a navy slab lit down its |
| * top and left and nowhere else, with no way to see where it ends. |
| * |
| - * So: one outline colour, --rst-accent, on all four sides, at a |
| - * specificity that clears the grid line (6.59:1 on the fill in dark, |
| - * 4.75:1 in light — both past 1.4.11's 3:1 for a component boundary). |
| - * The shadow goes: with the outline in a colour that reads, masking the |
| - * line underneath it buys nothing and costs the selection its symmetry. |
| + * ── Why every rule below is scoped `> tbody > tr > td` ──────────────── |
| + * Two reasons, and the second is the one that bites. |
| + * |
| + * 1. The grid-line rule above is (0,2,3). A restatement written at the |
| + * library's own `.jss_worksheet .highlight-top` is (0,2,0) and loses |
| + * to it — a rule that parses, uses a token, and paints nothing. |
| + * 2. sheet.html links the vendored stylesheets in the head block, which |
| + * layout.html renders AFTER sheets.css. The library is later in |
| + * source order, so equal specificity loses too. Ours must be |
| + * strictly higher, never merely equal. That is also the real reason |
| + * the vendored #dcdcdc beat the header rule above: both are (0,3,3), |
| + * and the vendor came last. |
| + * |
| + * So: one outline colour, --rst-accent, on all four sides, above both. |
| + * Measured on the fill it sits on, 6.59:1 in dark and 4.75:1 in light — |
| + * both past 1.4.11's 3:1 for a component boundary. The shadow goes: |
| + * with the outline in a colour that reads, masking the line underneath |
| + * it buys nothing and costs the selection its symmetry. |
| * |
| * The library gives each of its three range states a different border |
| * STYLE — solid for the selection, dashed for the copy marquee, dotted |
| - * for the range a formula is pointing at — and the style survives our |
| + * for the fill-handle drag preview — and the style survives our |
| * colour-only override. That is the distinction doing the work, so all |
| * three take the same accent and stay told apart by shape. */ |
| [data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight { |
| @@ -176,12 +190,10 @@ |
| color: var(--rst-text); |
| } |
| -/* The anchor — the one cell the keyboard is on. The library paints it |
| - * transparent on purpose so it reads out of the fill; our .highlight |
| - * rule was opaque and took that back, leaving a range with no visible |
| - * cursor in either scheme. */ |
| -[data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight-selected { |
| - background-color: var(--rst-bg); |
| +/* A link inside a selected cell. Vendored as the browser's own `blue`, |
| + * which on the fill in dark is 2.4:1. */ |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight > a { |
| + color: var(--rst-accent); |
| } |
| [data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight-top, |
| @@ -210,13 +222,32 @@ |
| border-bottom-color: var(--rst-accent); |
| } |
| -/* The range a formula is pointing at while you edit it. Vendored as |
| - * rgba(0,0,0,0.05) — a darkening, which on a dark grid is nothing. */ |
| +/* The rows the fill handle is being dragged over — the preview of what |
| + * a release would fill, NOT the range a formula is pointing at. There is |
| + * no formula chrome in this app: grid.js sets `parseFormulas: false`, |
| + * because our engine computes and the client only paints. The library |
| + * adds these classes from updateCopySelection, off `selectedCorner`. |
| + * Vendored as rgba(0,0,0,0.05) — a darkening, which on a dark grid is |
| + * nothing at all. */ |
| [data-sheet-grid] .jss_worksheet > tbody > tr > td.selection { |
| background-color: var(--rst-accent-soft); |
| color: var(--rst-text); |
| } |
| +/* The anchor — the one cell the keyboard is on. The library paints it |
| + * transparent on purpose so it reads out of the fill; our .highlight |
| + * rule was opaque and took that back, leaving a range with no visible |
| + * cursor in either scheme. |
| + * |
| + * It comes LAST on purpose. It ties .highlight and .selection at |
| + * (0,3,3), and the anchor is inside both while you drag the fill handle |
| + * — so on a tie it has to be the later rule or the cursor disappears at |
| + * exactly the moment you are aiming with it. */ |
| +[data-sheet-grid] .jss_worksheet > tbody > tr > td.highlight-selected { |
| + background-color: var(--rst-bg); |
| + color: var(--rst-text); |
| +} |
| + |
| /* The fill handle at the corner of the range: a black knob ringed in |
| * white, which in dark reads as a white dot with a dark centre — the |
| * inverse of the thing it is meant to be. It is 1px plus its ring, so |
| @@ -226,6 +257,44 @@ |
| border-color: var(--rst-bg); |
| } |
| +/* ── The right-click menu ──────────────────────────────────────────── |
| + * |
| + * Not Jspreadsheet's: jsuites', which it binds to `contextmenu` whether |
| + * or not the host app configures one. So it exists here, it is reachable |
| + * on every cell, and vendored it is #fff with #555 text — a white card |
| + * dropped on a dark grid, and the last unthemed surface on this screen. |
| + * |
| + * It is `position: fixed` and mounted inside the grid, so it is scoped |
| + * the same way everything else here is. */ |
| +[data-sheet-grid] .jcontextmenu { |
| + background: var(--rst-surface); |
| + color: var(--rst-text); |
| + border-color: var(--rst-line-strong); |
| + box-shadow: var(--rst-shadow-pop); |
| +} |
| + |
| +[data-sheet-grid] .jcontextmenu > div a { |
| + color: var(--rst-text); |
| +} |
| + |
| +[data-sheet-grid] .jcontextmenu > div span { |
| + color: var(--rst-text-muted); |
| +} |
| + |
| +[data-sheet-grid] .jcontextmenu > div:hover { |
| + background: var(--rst-accent-soft); |
| + color: var(--rst-text); |
| +} |
| + |
| +[data-sheet-grid] .jcontextmenu .jcontextmenu-disabled a, |
| +[data-sheet-grid] .jcontextmenu .jcontextmenu-disabled::before { |
| + color: var(--rst-text-faint); |
| +} |
| + |
| +[data-sheet-grid] .jcontextmenu hr { |
| + border-color: var(--rst-line); |
| +} |
| + |
| /* 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, |