From a7829d591a386ddef554ece8749a0b10854f9d69 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 22:34:18 +0100 Subject: [PATCH] fix(map): the popup never took the dark theme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit leaflet.css paints `background: white; color: #333` on the popup card and its tip. LeafletMapInner binds themed content into it — the school name and the headline figure are var(--text-primary) — so in dark mode #E9EEF0 landed on #FFFFFF at 1.17:1. The two things the popup exists to say were the two least readable things on the page. Every other foreground in that popup failed too, from the same cause: the muted phase line at 2.90:1, the vs-national delta at 1.94:1, the Ofsted badge at 1.74:1. Moving the surface onto --bg-card fixes all of them at once — 13.52, 5.45, 8.14 and 9.11:1 respectively. In light mode --bg-card is #FFFFFF, so the popup renders exactly as it did. globals.css already pulls the rest of Leaflet's chrome onto the tokens, and says why: "this matters most in dark mode, where Leaflet's white attribution bar would otherwise sit on a near-black page." The popup was simply missed. The View Details button needed its own fix. It pairs background:var(--status- above) with a literal white label, which theming the card does not reach: --status-above is #36743F in light but #7FCB8A in dark, taking the label from 5.63:1 to 1.94:1. --text-inverse is the token for ink on a saturated fill, and the popup's own Ofsted badge already uses it. darkThemeSafety already guards this defect class, but only inside .module.css. Neither half of this one lives there — the surface is a third party's, the text is inline in a TSX template — so it scanned clean throughout. Two rules added for the layer it could not see. Fixing the grouped-selector blind spot in its rules() helper was needed to write them: taking only a selector's last line discarded every selector in a grouped rule but the final one, which makes a safety guard fail open. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM --- .../components/darkThemeSafety.test.ts | 92 ++++++++++++++++++- nextjs-app/app/globals.css | 29 ++++++ nextjs-app/components/LeafletMapInner.tsx | 2 +- 3 files changed, 119 insertions(+), 4 deletions(-) diff --git a/nextjs-app/__tests__/components/darkThemeSafety.test.ts b/nextjs-app/__tests__/components/darkThemeSafety.test.ts index 60445ee..8904e8c 100644 --- a/nextjs-app/__tests__/components/darkThemeSafety.test.ts +++ b/nextjs-app/__tests__/components/darkThemeSafety.test.ts @@ -32,10 +32,17 @@ function stylesheets(dir: string): string[] { } /** Innermost `selector { body }` pairs. Nested at-rules never match as rules, - * because their body contains braces. */ + * because their body contains braces. + * + * Comments are stripped before matching rather than after, so that the whole + * selector survives. Taking only its last line — which is what stripping a + * leading comment used to require — silently discarded every selector in a + * grouped rule but the final one, and a safety guard that cannot see half its + * input fails open. */ function rules(css: string): Array<{ selector: string; body: string }> { - return Array.from(css.matchAll(/([^{}]+)\{([^{}]*)\}/g), (m) => ({ - selector: m[1].trim().split('\n').pop()!.trim(), + const bare = css.replace(/\/\*[\s\S]*?\*\//g, ''); + return Array.from(bare.matchAll(/([^{}]+)\{([^{}]*)\}/g), (m) => ({ + selector: m[1].trim().replace(/\s*\n\s*/g, ' '), body: m[2], })); } @@ -45,6 +52,15 @@ const THEMED_COLOR = /(?:^|[^-])color:\s*var\(--/; const files = stylesheets(COMPONENTS); +/** Component sources, for the third-party-surface rule below. */ +function sources(dir: string): string[] { + return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return sources(full); + return entry.name.endsWith('.tsx') ? [full] : []; + }); +} + describe('dark-theme safety', () => { it('finds stylesheets to check', () => { expect(files.length).toBeGreaterThan(0); @@ -76,3 +92,73 @@ describe('dark-theme safety', () => { expect(offenders).toEqual([]); }); }); + +/** + * The same defect one stylesheet further out. + * + * The rules above scan our own CSS modules. They cannot see a surface painted + * by a third-party sheet: leaflet.css hardcodes `background: white` on + * `.leaflet-popup-content-wrapper` and `.leaflet-popup-tip`, and + * LeafletMapInner builds its popup as an HTML string with inline + * `color: var(--text-primary)`. Neither half lives in a .module.css, so the + * module scan passed while dark mode rendered #E9EEF0 on #FFFFFF — 1.17:1, + * with the school name and the headline figure effectively invisible. + * + * globals.css already pulls the rest of Leaflet's chrome onto the tokens (the + * attribution bar, the zoom controls) for exactly this reason. The popup was + * simply missed. + */ +describe('third-party surfaces under themed text', () => { + const GLOBALS = path.join(__dirname, '..', '..', 'app', 'globals.css'); + + /** Leaflet surfaces our own code writes token-coloured text onto. */ + const LEAFLET_POPUP_SURFACES = [ + '.leaflet-popup-content-wrapper', + '.leaflet-popup-tip', + ]; + + it('still finds a component painting themed text into a Leaflet popup', () => { + // Guards the rule below against passing vacuously if the popups are ever + // rewritten as React components rather than HTML strings. + const themed = sources(COMPONENTS).filter((file) => { + const src = fs.readFileSync(file, 'utf8'); + return /bindPopup\(/.test(src) && /color:var\(--|color: var\(--/.test(src); + }); + + expect(themed.length).toBeGreaterThan(0); + }); + + it('themes the Leaflet popup surface, because the text on it is themed', () => { + const globals = rules(fs.readFileSync(GLOBALS, 'utf8')); + + const unthemed = LEAFLET_POPUP_SURFACES.filter((surface) => { + const rule = globals.find((r) => r.selector.includes(surface)); + return !rule || !/background[^;]*var\(--/.test(rule.body); + }); + + // Leaflet's white is not a colour this site owns. Either the surface + // follows the theme or the text on it must be literal — and the text is + // already themed. + expect(unthemed).toEqual([]); + }); + + it('never puts a literal white label on a themed fill', () => { + /* + * The mirror image of the module-CSS rule above, and the half of the popup + * that theming the card does not reach. "View Details" is + * `background:var(--status-above);color:white`; --status-above is #36743F + * in light but #7FCB8A in dark, so the label went from 5.63:1 to 1.94:1. + * + * --text-inverse is the token for ink on a saturated fill — #FFFFFF in + * light, #111A20 in dark — and the popup's Ofsted badge already uses it. + */ + const offenders = sources(COMPONENTS).flatMap((file) => { + const src = fs.readFileSync(file, 'utf8'); + return Array.from( + src.matchAll(/background:\s*var\(--[^;"']*;[^"']*?color:\s*(white|#fff\b|#ffffff\b)/gi), + () => path.relative(COMPONENTS, file)); + }); + + expect(offenders).toEqual([]); + }); +}); diff --git a/nextjs-app/app/globals.css b/nextjs-app/app/globals.css index 096e4f6..7800688 100644 --- a/nextjs-app/app/globals.css +++ b/nextjs-app/app/globals.css @@ -588,6 +588,35 @@ html .leaflet-bar a:hover { color: var(--text-primary); } +/* + * The popup, which leaflet.css paints `background: white; color: #333` on both + * the card and its tip. The content LeafletMapInner binds into it is themed — + * the school name and the headline figure are `var(--text-primary)` — so in + * dark mode that was #E9EEF0 on #FFFFFF, a contrast ratio of 1.17:1. The name + * and the number were the two least readable things on the page. + * + * Moving the surface onto --bg-card fixes every foreground at once rather than + * one at a time: the muted phase line goes 2.90:1 -> 5.45:1, the vs-national + * delta 1.94:1 -> 8.14:1, the Ofsted badge 1.74:1 -> 9.11:1. In light mode + * --bg-card is #FFFFFF, so the popup looks as it always did. + */ +html .leaflet-popup-content-wrapper, +html .leaflet-popup-tip { + background: var(--bg-card); + color: var(--text-primary); +} + +/* Leaflet's own selector is `.leaflet-container a.leaflet-popup-close-button` + at 0,2,1 — an `html` prefix alone would lose to it. */ +html .leaflet-container a.leaflet-popup-close-button { + color: var(--text-muted); +} + +html .leaflet-container a.leaflet-popup-close-button:hover, +html .leaflet-container a.leaflet-popup-close-button:focus { + color: var(--text-primary); +} + /* Main content column */ .main { max-width: 1400px; diff --git a/nextjs-app/components/LeafletMapInner.tsx b/nextjs-app/components/LeafletMapInner.tsx index 5bfe7db..9132b43 100644 --- a/nextjs-app/components/LeafletMapInner.tsx +++ b/nextjs-app/components/LeafletMapInner.tsx @@ -184,7 +184,7 @@ export default function LeafletMapInner({ schools, center, zoom, referencePoint, ${phaseLabel}${school.local_authority ? ` · ${escapeHtml(school.local_authority)}` : ''}${distanceStr} ${metricHtml} - View Details → + View Details → `; marker.bindPopup(popupContent);