From e820e7fecdace3fd9573b89be789f03978a97635 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 09:22:37 +0100 Subject: [PATCH 1/3] fix(analytics): the funnel source read a referrer that never changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The staging E2E gate has been red since #132 merged (run 1064, and 1066 after it): "a school reached from a location page is attributed to it, not to direct" expects `place`, receives `direct`. #132 fixed a real bug — `/schools/` had no case and fell through to `direct` — but the mechanism underneath it never worked. getNavigationSource read document.referrer, which the browser writes only when a *document* loads. Every internal navigation here is an App Router soft navigation: history.pushState, no new document, so document.referrer goes on naming whatever opened the tab for the whole session. Verified on staging: load /schools/brentwood, click a school, the URL becomes /school/… and document.referrer is still "". So `from` reported `direct` for essentially every in-app journey, not just the ones through the location layer — search, rankings, compare and detail were all being counted as "typed the URL". The unit suite passed throughout because every case set document.referrer directly, which only happens on a full page load. The fix is a module-level trail written by RouteTrail, a render-nothing client component in the root layout. Its lifetime is exactly right: it survives soft navigation, and it dies on a real document load — which is precisely when document.referrer becomes meaningful again, so the two cover each other with no overlap. Reading it skips entries equal to the current path rather than taking the second-to-last. That makes the answer independent of whether the layout effect or the page effect ran first — React orders those by tree position, which is not a contract worth resting a measurement on — and it gives the right answer both when the user returns to a page they came from and on a hard load of a school page. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM --- .../__tests__/components/RouteTrail.test.tsx | 38 ++++++++ nextjs-app/__tests__/lib/analytics.test.ts | 94 +++++++++++++++++++ nextjs-app/app/layout.tsx | 5 + nextjs-app/components/RouteTrail.tsx | 27 ++++++ nextjs-app/lib/analytics.ts | 77 ++++++++++++--- 5 files changed, 230 insertions(+), 11 deletions(-) create mode 100644 nextjs-app/__tests__/components/RouteTrail.test.tsx create mode 100644 nextjs-app/components/RouteTrail.tsx diff --git a/nextjs-app/__tests__/components/RouteTrail.test.tsx b/nextjs-app/__tests__/components/RouteTrail.test.tsx new file mode 100644 index 0000000..cc95c9e --- /dev/null +++ b/nextjs-app/__tests__/components/RouteTrail.test.tsx @@ -0,0 +1,38 @@ +/** + * The trail has to be written by something, and it has to be written on every + * route — not only the ones that happen to track an event. + */ +import { render } from '@testing-library/react'; + +const recordVisitedPath = jest.fn(); +let pathname = '/schools/brentwood'; + +jest.mock('next/navigation', () => ({ usePathname: () => pathname })); +jest.mock('@/lib/analytics', () => ({ + recordVisitedPath: (p: string) => recordVisitedPath(p), +})); + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { RouteTrail } = require('@/components/RouteTrail'); + +describe('RouteTrail', () => { + beforeEach(() => recordVisitedPath.mockClear()); + + it('records the page it is mounted on', () => { + render(); + expect(recordVisitedPath).toHaveBeenCalledWith('/schools/brentwood'); + }); + + it('records each new route as the user moves through the app', () => { + const { rerender } = render(); + pathname = '/school/115429-brentwood-school'; + rerender(); + expect(recordVisitedPath).toHaveBeenLastCalledWith( + '/school/115429-brentwood-school'); + }); + + it('renders nothing, so it can sit anywhere in the layout', () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); +}); diff --git a/nextjs-app/__tests__/lib/analytics.test.ts b/nextjs-app/__tests__/lib/analytics.test.ts index 22af2d7..96dd0fb 100644 --- a/nextjs-app/__tests__/lib/analytics.test.ts +++ b/nextjs-app/__tests__/lib/analytics.test.ts @@ -62,3 +62,97 @@ describe('getNavigationSource', () => { expect(getNavigationSource()).toBe('direct'); }); }); + +/* + * The defect the existing suite could not see. + * + * Every test above sets document.referrer, which the browser writes only when + * a *document* loads. Every internal navigation in this app is an App Router + * soft navigation — history.pushState, no new document — so document.referrer + * keeps naming whatever opened the tab for the whole session. Verified on + * staging: /schools/brentwood → click a school → URL changes to /school/… + * and document.referrer is still "". + * + * So `from` reported 'direct' for essentially every in-app journey, and the + * suite passed because it only ever exercised the full-page-load path. + */ +function freshAnalytics() { + let mod!: typeof import('@/lib/analytics'); + jest.isolateModules(() => { + mod = require('@/lib/analytics'); + }); + return mod; +} + +function at(path: string) { + window.history.pushState({}, '', path); +} + +describe('getNavigationSource across a soft navigation', () => { + afterEach(() => { + referrer(''); + at('/'); + }); + + it('attributes a school view to the place page the user actually came from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('place'); + }); + + it('does not depend on whether the new path was recorded first', () => { + // The trail is written by a layout-level effect and read by a page-level + // one. React orders those by tree position, which is not a contract worth + // resting a measurement on, so the answer must be the same either way. + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + recordVisitedPath('/rankings'); + at('/school/115429-brentwood-school'); + + expect(source()).toBe('rankings'); + }); + + it('names the previous page, not the current one, when both are schools', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/school/100010-brecknock-primary-school'); + recordVisitedPath('/school/100010-brecknock-primary-school'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('detail'); + }); + + it('looks past a return visit to the page the user came back from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + for (const p of ['/schools/brentwood', '/school/115429-brentwood-school', + '/schools/brentwood']) { + at(p); + recordVisitedPath(p); + } + expect(source()).toBe('detail'); + }); + + it('falls back to the referrer on a real document load, where it is true', () => { + // A fresh module is a fresh document: nothing has been recorded, and + // document.referrer is meaningful again. + const { getNavigationSource: source } = freshAnalytics(); + at('/school/115429-brentwood-school'); + referrer(`${ORIGIN}/schools/barnet`); + + expect(source()).toBe('place'); + }); + + it('still reads an arrival from outside as direct', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + referrer('https://www.google.com/search?q=schools+in+brentwood'); + + expect(source()).toBe('direct'); + }); +}); diff --git a/nextjs-app/app/layout.tsx b/nextjs-app/app/layout.tsx index 2f888b9..96a2be4 100644 --- a/nextjs-app/app/layout.tsx +++ b/nextjs-app/app/layout.tsx @@ -4,6 +4,7 @@ import Script from 'next/script'; import { Navigation } from '@/components/Navigation'; import { Footer } from '@/components/Footer'; import { ComparisonToast } from '@/components/ComparisonToast'; +import { RouteTrail } from '@/components/RouteTrail'; import { ComparisonProvider } from '@/context/ComparisonProvider'; import { SITE_URL } from '@/lib/site'; import './globals.css'; @@ -114,6 +115,10 @@ export default function RootLayout({ /> + {/* Records every route so funnel attribution has a previous page to + name. document.referrer cannot: a soft navigation creates no + document, so the browser never updates it. */} + Skip to main content diff --git a/nextjs-app/components/RouteTrail.tsx b/nextjs-app/components/RouteTrail.tsx new file mode 100644 index 0000000..4531693 --- /dev/null +++ b/nextjs-app/components/RouteTrail.tsx @@ -0,0 +1,27 @@ +/** + * Writes the in-app navigation trail that funnel attribution reads. + * + * Renders nothing. It exists because document.referrer cannot answer "which + * page did they come from" in an App Router app: a soft navigation creates no + * document, so the browser never updates it. See the trail comment in + * lib/analytics.ts. + * + * Mounted once in the root layout, so every route is recorded — including the + * ones that fire no event of their own, which are still somebody else's + * previous page. + */ +'use client'; + +import { useEffect } from 'react'; +import { usePathname } from 'next/navigation'; +import { recordVisitedPath } from '@/lib/analytics'; + +export function RouteTrail() { + const pathname = usePathname(); + + useEffect(() => { + recordVisitedPath(pathname); + }, [pathname]); + + return null; +} diff --git a/nextjs-app/lib/analytics.ts b/nextjs-app/lib/analytics.ts index bd05ba1..591ed7d 100644 --- a/nextjs-app/lib/analytics.ts +++ b/nextjs-app/lib/analytics.ts @@ -60,22 +60,77 @@ export function track(name: EventName, data?: Payload): void { export type NavigationSource = 'search' | 'rankings' | 'compare' | 'detail' | 'place' | 'direct'; +/* + * The in-app trail. + * + * document.referrer is written by the browser only when a *document* loads. + * Every internal navigation here is an App Router soft navigation — + * history.pushState, no new document — so document.referrer goes on naming + * whatever opened the tab (usually nothing, or a search engine) for the whole + * session. Reading it to answer "which page did they come from" therefore + * returned 'direct' for essentially every in-app journey, including the one + * the location layer exists to produce. + * + * Verified on staging: /schools/brentwood, click a school, the URL becomes + * /school/… and document.referrer is still "". + * + * A module-level trail is the counterpart with exactly the right lifetime. It + * survives soft navigation, and it dies on a real document load — which is + * precisely when document.referrer becomes meaningful again, so the two cover + * each other with no overlap. + */ +const TRAIL_LIMIT = 4; +const trail: string[] = []; + +/** Record a path the user is now on. Called by RouteTrail on every route. */ +export function recordVisitedPath(path: string): void { + if (trail[trail.length - 1] === path) return; + trail.push(path); + if (trail.length > TRAIL_LIMIT) trail.shift(); +} + +/** + * The most recent path that is not the one being viewed. + * + * Skipping the current path rather than taking trail[length - 2] is what + * makes the answer independent of ordering: the trail is written by a + * layout-level effect and read by a page-level one, and React orders those by + * tree position — not a contract worth resting a measurement on. It also + * gives the right answer when the user goes back to a page they came from. + */ +function previousInAppPath(): string | null { + if (typeof window === 'undefined') return null; + const current = window.location.pathname; + for (let i = trail.length - 1; i >= 0; i -= 1) { + if (trail[i] !== current) return trail[i]; + } + return null; +} + +function classifyPath(p: string): NavigationSource { + if (p === '/' || p === '') return 'search'; + if (p.startsWith('/rankings')) return 'rankings'; + if (p.startsWith('/compare')) return 'compare'; + // `/schools/` before `/school/`: they differ by one letter and mean + // different things — the location layer versus a single school. Checked + // first so the narrower-looking prefix cannot shadow it if either string + // is ever edited. + if (p.startsWith('/schools/')) return 'place'; + if (p.startsWith('/school/')) return 'detail'; + return 'direct'; +} + export function getNavigationSource(): NavigationSource { + const internal = previousInAppPath(); + if (internal) return classifyPath(internal); + + // No trail means this is the first page of the document, so the referrer is + // the only witness — and an honest one. if (typeof window === 'undefined' || !document.referrer) return 'direct'; try { const ref = new URL(document.referrer); if (ref.origin !== window.location.origin) return 'direct'; - const p = ref.pathname; - if (p === '/' || p === '') return 'search'; - if (p.startsWith('/rankings')) return 'rankings'; - if (p.startsWith('/compare')) return 'compare'; - // `/schools/` before `/school/`: they differ by one letter and mean - // different things — the location layer versus a single school. Checked - // first so the narrower-looking prefix cannot shadow it if either string - // is ever edited. - if (p.startsWith('/schools/')) return 'place'; - if (p.startsWith('/school/')) return 'detail'; - return 'direct'; + return classifyPath(ref.pathname); } catch { return 'direct'; } From 7a16b1b52fad2e52d4b4c35c0d99fb80d0464f9e Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 21:17:16 +0100 Subject: [PATCH 2/3] fix(admissions): flag-off pages must not speak for the council MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The secondary admissions section words the absence of a cut-off distance: " has not published a cut-off distance for this school." That sentence is true when the authority publishes nothing. It is false when the authority does publish and the admission_distance flag is simply off — and off is the current state, so every secondary page with an EES admissions row has been making a claim about a council on our behalf. The backend already draws the distinction the copy needs. /api/schools/{urn} omits the admission_distance key entirely while the flag is dark rather than sending null, precisely so that "we are not publishing cut-offs" stays distinguishable from "this school has no cut-off"; lib/types.ts says so in as many words. The page then collapsed the two with `?? null` before the section ever saw them. So stop collapsing it: thread the raw field to SecondarySchoolSections and word the absence only when the feature is on. Null still gets the sentence naming the authority — that case is unchanged and still tested. Primary pages are unaffected: AdmissionsSection carries no absence copy and renders nothing when there is no figure. DistanceSection already treated absent and null alike; only its type widens. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM --- e2e/tests/journeys.spec.ts | 46 +++++++++++++++++++ .../components/lastDistanceOffered.test.tsx | 11 +++++ nextjs-app/app/school/[slug]/page.tsx | 2 +- .../components/school/DistanceSection.tsx | 2 +- .../school/SecondaryAdmissionsSection.tsx | 12 +++-- .../school/SecondarySchoolSections.tsx | 5 +- 6 files changed, 72 insertions(+), 6 deletions(-) diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 966f2ab..e006c5d 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1304,6 +1304,52 @@ test('with the distance feature off, the section is absent rather than empty', a .toHaveCount(0); }); +/** + * A secondary school carrying an EES admissions row, which is what makes its + * Admissions section render while the distance feature is dark. + */ +async function secondarySchoolWithAdmissions(page: Page) { + const list = await page.request.get('/api/schools?phase=secondary&page_size=40'); + if (!list.ok()) return null; + const body = await list.json(); + for (const s of (body?.schools ?? []).slice(0, 25)) { + const res = await page.request.get(`/api/schools/${s.urn}`); + if (!res.ok()) continue; + const detail = await res.json(); + if (detail?.admissions == null) continue; + return { urn: s.urn as number }; + } + return null; +} + +test('with the distance feature off, a secondary page makes no claim about publication', async ({ page }) => { + /* + * Shipping dark must not put words in the council's mouth. The secondary + * template is the only one that words the absence, and "X has not published + * a cut-off distance for this school" is false wherever X does publish and + * we are simply withholding it. + * + * This is why the API omits the key rather than sending null: absent means + * "cut-offs are not published at all", null means "this school has none". + * Only the second is a fact about the school, and only the second is sayable. + */ + test.skip(await distanceFeatureIsOn(page), + 'the admission_distance flag is on in this environment'); + + const found = await secondarySchoolWithAdmissions(page); + test.skip(found === null, 'no secondary school in the sample has an admissions row'); + + await page.goto(`/school/${found!.urn}`); + await expect(page.locator('h1').first()).toBeVisible({ timeout: 15_000 }); + + // The Admissions section is still there — this is not a test that the whole + // section vanished, which would pass for the wrong reason. + await expect(page.locator('#admissions')).toHaveCount(1); + + await expect(page.getByText(/has not published a cut-off distance/)).toHaveCount(0); + await expect(page.getByText(/Contact the admissions authority/)).toHaveCount(0); +}); + test('/api/flags is not reachable from the public internet', async ({ page }) => { // It names every unreleased feature and whether it is on. Next reads it // server-side over the Docker network; the public proxy must deny it. diff --git a/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx b/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx index 9f16df5..3f9efa1 100644 --- a/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx +++ b/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx @@ -98,6 +98,17 @@ describe('secondary detail page', () => { expect(screen.getByText(/has not published a cut-off distance/)).toBeInTheDocument(); }); + + it('makes no claim about publication when the feature is switched off', () => { + // Absent, not null. The API omits the key entirely while the + // admission_distance flag is off, and "Islington has not published a + // cut-off distance" is then a statement about us, not about Islington — + // false wherever the authority does publish one. + renderSecondarySchoolDetail({ ...secondaryFixture, admissionDistance: undefined }); + + expect(screen.queryByText(/has not published a cut-off distance/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Contact the admissions authority/)).not.toBeInTheDocument(); + }); }); // ── The Distance section ─────────────────────────────────────────────── diff --git a/nextjs-app/app/school/[slug]/page.tsx b/nextjs-app/app/school/[slug]/page.tsx index 4e9f6aa..bef695a 100644 --- a/nextjs-app/app/school/[slug]/page.tsx +++ b/nextjs-app/app/school/[slug]/page.tsx @@ -232,7 +232,7 @@ export default async function SchoolPage({ params }: SchoolPageProps) { census={census ?? null} admissions={admissions ?? null} admissionsHistory={admissions_history ?? []} - admissionDistance={admission_distance ?? null} + admissionDistance={admission_distance} deprivation={deprivation ?? null} finance={finance ?? null} nationalAvg={nationalAvg} diff --git a/nextjs-app/components/school/DistanceSection.tsx b/nextjs-app/components/school/DistanceSection.tsx index 8990960..45651b5 100644 --- a/nextjs-app/components/school/DistanceSection.tsx +++ b/nextjs-app/components/school/DistanceSection.tsx @@ -24,7 +24,7 @@ export function DistanceSection({ admissionDistance, schoolInfo, }: { - admissionDistance: SchoolAdmissionDistance | null; + admissionDistance: SchoolAdmissionDistance | null | undefined; schoolInfo: School; }) { // Without a figure there is nothing to compare against, and without diff --git a/nextjs-app/components/school/SecondaryAdmissionsSection.tsx b/nextjs-app/components/school/SecondaryAdmissionsSection.tsx index 49c8970..63471dc 100644 --- a/nextjs-app/components/school/SecondaryAdmissionsSection.tsx +++ b/nextjs-app/components/school/SecondaryAdmissionsSection.tsx @@ -21,11 +21,17 @@ export function SecondaryAdmissionsSection({ published cut-off and no EES admissions row. */ admissions: SchoolAdmissions | null; admissionsHistory: SchoolAdmissions[]; - admissionDistance: SchoolAdmissionDistance | null; + admissionDistance: SchoolAdmissionDistance | null | undefined; schoolInfo: School; hasSixthForm: boolean; }) { const cutoff = describeCutoff(admissionDistance); + /* Absent means cut-offs are not being published at all; null means this + school has no published cut-off. Only the second is a fact about the + school, and only the second can be stated. Saying "X has not published a + cut-off" while the feature is dark describes us, and is false wherever the + authority does publish one. */ + const featureOn = admissionDistance !== undefined; // Moved with this section from SecondarySchoolDetailView, its only consumer. const admissionsTag = (() => { const policy = schoolInfo.admissions_policy?.toLowerCase() ?? ''; @@ -102,7 +108,7 @@ export function SecondaryAdmissionsSection({ {CUTOFF_NOTE} {CUTOFF_MEASUREMENT_NOTE} {cutoff.routeNote && <> {cutoff.routeNote}}

- ) : ( + ) : featureOn ? (

{describeCutoffAbsence({ localAuthority: schoolInfo.local_authority, @@ -110,7 +116,7 @@ export function SecondaryAdmissionsSection({ admissionsHistory, })}

- )} + ) : null} {hasSixthForm && (
diff --git a/nextjs-app/components/school/SecondarySchoolSections.tsx b/nextjs-app/components/school/SecondarySchoolSections.tsx index 1ba3d7a..10bf3c8 100644 --- a/nextjs-app/components/school/SecondarySchoolSections.tsx +++ b/nextjs-app/components/school/SecondarySchoolSections.tsx @@ -36,7 +36,10 @@ export interface SecondarySchoolSectionsProps { /** Needed to tell a year with no published cut-off apart from a year the * school simply was not oversubscribed. */ admissionsHistory: SchoolAdmissions[]; - admissionDistance: SchoolAdmissionDistance | null; + /** Absent — not null — while the admission_distance flag is off. The two + * mean different things to the reader and must stay distinguishable: + * see SecondaryAdmissionsSection, which words the absence. */ + admissionDistance: SchoolAdmissionDistance | null | undefined; deprivation: SchoolDeprivation | null; finance: SchoolFinance | null; nationalAvg: NationalAverages | null; From a7829d591a386ddef554ece8749a0b10854f9d69 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 22:34:18 +0100 Subject: [PATCH 3/3] 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);