oficina / sheets Public

Per AGENTS.md, this discussion is the branch's review record.

Two commits on fix/dark-selection-outline, cut from core rather than main — the sheets code only exists on core, so that is the trunk this stacks on. Flagging it rather than assuming it: if the intent is that fixes land on main first, this needs rebasing.

The bug

Reported from a canary screenshot: a selected range in dark mode is a navy slab with one bright white line down its left side and no edge anywhere else.

Root cause, measured in Chromium against the real binary, not read off the source. Every colour in the vendored Jspreadsheet stylesheet is a literal, and sheets.css restated the ones for the grid, the headers and the range fill. It stopped there, and the outline is where the rest of them live:

• .highlight-right/-bottom are 1px solid #000 — invisible on a #111418 grid.
• .highlight-top/-left would be too, except our own td { border-top-color: var(--rst-line) } at (0,2,3) outranks the library's (0,2,0) and overwrote them with an ordinary grid line — while their decorative box-shadow: -1px 0 #ccc, which exists only to mask the grid line the border sits on, survived and became the only visible edge. That is the white L.

Three more of the same shape turned up while measuring the first:

• The selected row's header was #dcdcdc carrying --rst-text-muted (#a8b0ba) at 1.60:1. There was an override for it — tbody > tr > td:first-child.selected — but the library puts .selected on the <tr>, not on its first cell, so it matched nothing and looked like a fix. A CSS rule that matches nothing fails exactly like a rule nobody wrote.
• The fill handle is a black knob ringed in white: inverted in dark.
• The copy marquee (dashed #000) and the fill-drag preview (dotted #000) are both invisible.

And one found during review: jsuites' .jcontextmenu is bound to contextmenu whether or not the host app configures one, so a right-click on any cell opened a #fff card with #555 text on the dark grid — the last unthemed surface on this screen.

The fix

One outline colour, --rst-accent, on all four sides of all three range states, scoped > tbody > tr > td.foo, and the #ccc shadow dropped. The library gives each state a different border style — solid, dashed, dotted — and the style survives a colour-only override, so shape keeps them apart. --rst-accent on --rst-accent-soft measures 6.59:1 dark and 4.75:1 light, both past 1.4.11's 3:1 for a component boundary.

One change is not dark-specific and is called out as such: .highlight-selected — the anchor cell the keyboard is on — is transparent in the library so it reads out of the fill, and our opaque .highlight rule had taken that back. A range had no visible cursor in either scheme.

Review pass, with the model recorded

Reviewer: Claude Opus 5 (1M context) — same capability as the author, adversarial pass over the full diff, both vendored stylesheets, grid.js and the templates. Not a self-endorsement: it landed five findings, two of which would have blocked a merge.

1. The gate did not gate. The guard 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.

Proved by mutation rather than by reading — four reintroductions, each caught:

| reintroduction | caught by | | --- | --- | | .highlight-right rule deleted | TestVendorPaintIsRestated | | outline written at .jss_worksheet .highlight-top | TestVendorPaintIsRestated | | border-right-color: #7ba7ff instead of the token | TestOurStylesheetHasNoColourLiterals | | row-header override back on the td | TestVendorPaintIsRestated |

2. A wrong number, stated three times. The first commit put "1.9:1" in the permanent record. Computed from theme.css, #a8b0ba on #dcdcdc is 1.60:1; 1.91:1 is --rst-text-faint, a token that rule never used. The bug was worse than it was described. Corrected in the CSS comment, the test docstring and the second commit message.

*3. `.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 — there is no formula-range highlighting in this app to theme. The rules were right; the comment naming them would have sent the next person hunting 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 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.

4. 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. The library is later in source order, so an equal-specificity restatement loses on order alone — the header rule and the vendored one are both (0,3,3), and the first commit told only the dead-selector half of that story. Now in AGENTS.md, and pinned by a test that fails if sheets.css ever moves after that head block.

5. Two smaller ones. td.highlight > a was the browser's own blue, 2.4:1 on the fill in dark. .highlight-selected set a background with no color — the exact shape this file's own comment calls the bug.

One overcall, recorded rather than argued. The review flagged .jss_content::-webkit-scrollbar-* (#eee, #666) as a live gap. Measured: .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. It is named in the test's exclusion list with that measurement.

What is now enforced

• TestVendorPaintIsRestated — 24 vendored rules across both stylesheets, checked in both directions: that each vendored selector still exists (v5 renamed every .jexcel to .jss_* once already, and a stale list is how that goes unnoticed a second time), and that we restate it in a rule, not in prose. Rules the app cannot render are named with a checkable reason.
• TestOurStylesheetHasNoColourLiterals — closes the hole csstokens_test.go cannot see: it proves every var(--rst-…) we use is defined and says nothing about a declaration that skips var() altogether.
• TestVendorLinksLoadAfterOurs — pins the load order the specificity counting depends on.

Stated limit: TestVendorPaintIsRestated 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. That limit is written into the test, not left for someone to discover.

Verification

Chromium, both schemes, on the real binary via tools/drive-style automation — not a unit test's opinion of the CSS:

• Outline #7ba7ff dark / #2464e0 light on all four sides, box-shadow: none.
• Row header on --rst-accent-soft with --rst-text; fill handle on the accent ringed in --rst-bg.
• Copy marquee dashed and visible; fill-drag preview dotted and visible.
• Context menu #1a1f26 / #e8eaed in dark, white / #1a1d21 in light.

make test green — vet, fmt, build, migrations, go, race.

Not fixed here, worth someone's ruling

1. The right-click menu's items are the library's, not ours. It offers Delete selected rows while grid.js sets allowInsertRow: false, plus Save as… and About. Themed now, but the menu is still saying things this app did not choose to say. A product call, not a CSS one.
2. td[data-contrast="low"] in sheets.css is dead today — grep -rn data-contrast across .go, .js and .html hits only the stylesheet. Fills are imported, stored and contrast-checked, but no fill is ever painted into a cell. Pre-existing, not introduced here. Forward risk worth writing down: when fills do land, if they are applied by class or attribute rather than an inline style, the anchor rule at (0,3,3) will paint --rst-bg over a filled anchor cell.
3. Not yet reviewed on a deployed canary. AGENTS.md requires that, and this has only been driven on localhost.