fix(map): the popup never took the dark theme #136

Merged
tudor merged 1 commits from fix/dark-mode-map-popup-contrast into main 2026-08-27 21:57:53 +00:00
Owner

Reported: the school name and the headline figure in the map pin popup are unreadable in dark mode.

Cause

leaflet.css hardcodes the popup surface:

.leaflet-popup-content-wrapper,
.leaflet-popup-tip { background: white; color: #333; }

LeafletMapInner binds themed content onto it — the name and the figure are var(--text-primary), which is #E9EEF0 in dark. Near-white on white.

It isn't only the two elements reported. Every foreground in that popup fails, from the one cause:

element token dark, on Leaflet white on --bg-card
school name, headline figure --text-primary 1.17:1 13.52:1
phase · LA · distance --text-muted 2.90:1 5.45:1
vs-national delta --status-above 1.94:1 8.14:1
Ofsted badge --phase-secondary-text 1.74:1 9.11:1

So the fix is the surface, not the text: one rule, every foreground above 4.5:1. 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 missed.

The View Details button needed a separate fix. It pairs background:var(--status-above) with a literal color:white, which theming the card doesn't reach: --status-above is #36743F in light but #7FCB8A in dark, taking the label from 5.63:1 to 1.94:1. Now --text-inverse (9.07:1 dark, 5.63:1 light unchanged) — the token for ink on a saturated fill, already used by this popup's own Ofsted badge.

Why the existing guard missed it

darkThemeSafety.test.ts guards exactly this defect class — its docstring even describes the same failure on the map controls. But it scans .module.css, and neither half of this one is there: the surface belongs to a third-party sheet, the text is inline in a TSX template string. It scanned clean the whole time.

Two rules added for that layer:

  • themed popup text requires a themed popup surface (with a vacuity guard, so it can't pass if the popups are ever rewritten as React components)
  • no literal white label on a themed fill — the mirror image of the existing module rule

Writing them required fixing a blind spot in the shared rules() helper: it took only a selector's last line, so in a grouped rule every selector but the final one was discarded. A safety guard that can't see half its input fails open. Comments are now stripped before matching instead of after, which is what the last-line trick was working around. The two existing rules are unaffected — they filter on rule bodies.

Verification

Rendered from the real token blocks in globals.css and Leaflet's real popup CSS. Before reproduces the report; after is legible; light mode is unchanged.

  • npx jest — 36 suites, 331 tests, all passing (6 in darkThemeSafety, up from 3)
  • npx tsc --noEmit — clean
  • Both new rules watched failing first: the surface rule listed both unthemed surfaces, the label rule named LeafletMapInner.tsx

No e2e journey: contrast in a themed popup isn't a thing Playwright asserts well, and the static guard covers the regression directly.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM

Reported: the school name and the headline figure in the map pin popup are unreadable in dark mode. ## Cause `leaflet.css` hardcodes the popup surface: ```css .leaflet-popup-content-wrapper, .leaflet-popup-tip { background: white; color: #333; } ``` `LeafletMapInner` binds themed content onto it — the name and the figure are `var(--text-primary)`, which is `#E9EEF0` in dark. Near-white on white. It isn't only the two elements reported. Every foreground in that popup fails, from the one cause: | element | token | dark, on Leaflet white | on `--bg-card` | |---|---|---|---| | school name, headline figure | `--text-primary` | **1.17:1** | 13.52:1 | | phase · LA · distance | `--text-muted` | **2.90:1** | 5.45:1 | | vs-national delta | `--status-above` | **1.94:1** | 8.14:1 | | Ofsted badge | `--phase-secondary-text` | **1.74:1** | 9.11:1 | So the fix is the surface, not the text: one rule, every foreground above 4.5:1. 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 missed. **The View Details button needed a separate fix.** It pairs `background:var(--status-above)` with a literal `color:white`, which theming the card doesn't reach: `--status-above` is `#36743F` in light but `#7FCB8A` in dark, taking the label from 5.63:1 to **1.94:1**. Now `--text-inverse` (9.07:1 dark, 5.63:1 light unchanged) — the token for ink on a saturated fill, already used by this popup's own Ofsted badge. ## Why the existing guard missed it `darkThemeSafety.test.ts` guards exactly this defect class — its docstring even describes the same failure on the map controls. But it scans `.module.css`, and neither half of this one is there: the surface belongs to a third-party sheet, the text is inline in a TSX template string. It scanned clean the whole time. Two rules added for that layer: - themed popup text requires a themed popup surface (with a vacuity guard, so it can't pass if the popups are ever rewritten as React components) - no literal white label on a themed fill — the mirror image of the existing module rule Writing them required fixing a blind spot in the shared `rules()` helper: it took only a selector's **last line**, so in a grouped rule every selector but the final one was discarded. A safety guard that can't see half its input fails open. Comments are now stripped before matching instead of after, which is what the last-line trick was working around. The two existing rules are unaffected — they filter on rule bodies. ## Verification Rendered from the real token blocks in `globals.css` and Leaflet's real popup CSS. Before reproduces the report; after is legible; light mode is unchanged. - `npx jest` — 36 suites, 331 tests, all passing (6 in `darkThemeSafety`, up from 3) - `npx tsc --noEmit` — clean - Both new rules watched failing first: the surface rule listed both unthemed surfaces, the label rule named `LeafletMapInner.tsx` No e2e journey: contrast in a themed popup isn't a thing Playwright asserts well, and the static guard covers the regression directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM
tudor added 1 commit 2026-08-27 21:34:41 +00:00
fix(map): the popup never took the dark theme
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 10s
PR Checks / Build Frontend (no push) (pull_request) Successful in 46s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 15s
a7829d591a
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM

🤖 AI Code Review (Claude Code)

This PR fixes a dark-theme contrast bug where Leaflet map popups rendered themed text (e.g. school name, headline figures) on a hardcoded white background and a themed 'View Details' link colored white-on-white in dark mode. It adds CSS overrides pulling the popup surface onto theme tokens, swaps a literal white for --text-inverse, and adds new regression tests (with comment-stripping fixed in the test's CSS rule parser) to guard against regressions.

✅ No issues found.

## 🤖 AI Code Review (Claude Code) This PR fixes a dark-theme contrast bug where Leaflet map popups rendered themed text (e.g. school name, headline figures) on a hardcoded white background and a themed 'View Details' link colored white-on-white in dark mode. It adds CSS overrides pulling the popup surface onto theme tokens, swaps a literal white for --text-inverse, and adds new regression tests (with comment-stripping fixed in the test's CSS rule parser) to guard against regressions. ✅ No issues found.
tudor merged commit 7c08138fe4 into main 2026-08-27 21:57:53 +00:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: tudor/school_compare#136