Theme Leaflet's zoom and attribution controls in dark mode #183
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-246-leaflet-theming"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 246. Leaflet's own controls (
lib/leaflet/leaflet.css) hardcode a light palette with no awareness ofprefers-color-scheme, so the zoom buttons and the attribution box stayed white regardless of scheme. Addswwwroot/css/leaflet-theme.css, an override stylesheet bound totheme.css's existing tokens, loaded after both vendored stylesheets — the vendored files themselves are untouched.Scope check, per control:
.leaflet-bar a) — themed: background/border/glyph now follow--colour-surface/--colour-control-border/--colour-text, with hover/focus and.leaflet-disabledcovered too, plus the touch-only border variant (.leaflet-touch .leaflet-bar).a { color: var(--colour-accent) }inapp.cssat all, in either scheme — leaflet.css's own.leaflet-container a { color: #0078A8 }outranks that plain-tag rule on specificity regardless of stylesheet load order.MarkerCluster.Default.css(the sheet with the default green/yellow/orange classes) was already deliberately not linked;map.jsdraws every cluster as an inline-SVGdivIconwith its own explicit fill and a solid white stroke, so it's unaffected by either scheme already.map.jsnever callsbindPopup/bindTooltip, so Leaflet never renders either.Contrast, computed independently per scheme (all against
--colour-surface):--colour-text): 17.09:1 light, 13.70:1 dark.--colour-control-border): 3.30:1 light, 3.33:1 dark — reuses ADR-0087's own measured pairing rather than computing a second one.--colour-text-muted): 5.82:1 light, 6.97:1 dark — visibly dimmer than enabled without fading to illegible.--colour-text-muted): 5.82:1 light, 6.97:1 dark.--colour-accent): 5.18:1 light, 6.83:1 dark.All exceed the 3:1 floor for non-text UI, and the attribution pairs exceed WCAG AA's 4.5:1 for text.
Verification: bUnit can't evaluate
prefers-color-scheme, so I used Playwright's colour-scheme emulation.LeafletControlThemingTests(new, in the existing opt-in E2E suite) drives a real Chromium instance per scheme and asserts each control's computed style against a hidden probe element carrying the same custom property — this keeps discriminating the override if a token's value changes later, and ifleaflet-theme.css's selectors ever lose the cascade toleaflet.css's own. The disabled-zoom-out state is forced deterministically viaelement.placeMarkMap.setZoom(getMinZoom())and waited for throughExpect(...).ToBeVisibleAsync()'s own polling, not a fixed delay, to avoid task 233's known intermittent.I also independently verified the fix outside the app (no server, no ports) by loading the real vendored + override stylesheets in a static harness under Chromium with colour-scheme emulation, reading
getComputedStyleback, and confirming it reverts to the vendor's hardcoded white/black whenleaflet-theme.cssis removed — the same defect Rob reported.No ADR: this applies ADR-0122's and the theme layer's existing decisions rather than making a new one.
Verdict: mergeable
Checked the specific risks called out for this one rather than taking the PR body at face value:
leaflet-theme.cssis tokens-only — no hardcoded colour, no secondprefers-color-schemeblock. Satisfies theme.css's own rule.#fff/blackon.leaflet-bar a,#f4f4f4/#bbbdisabled,rgba(255,255,255,0.8)/#333attribution) differ from the token values in every case except the light-scheme zoom-control background (#fffhappens to equal--colour-surfacethere) — so unlinking the override would still redden every one of these tests via thecolorassertion. See inline note on the one case where that's the sole discriminator..leaflet-container ais (0,1,1); the new.leaflet-container .leaflet-control-attribution ais (0,2,1) — correctly scoped to the attribution box only, doesn't touch the popup close button or any other link under.leaflet-container.setZoom()fix (fd8529f) is genuinely test-harness-only. None ofmap.js's own exported interop functions (setView,panIntoView, etc.) hand a chainable Leaflet return value back across the Blazor JS-interop boundary the way the expression-bodied PlaywrightEvaluateAsynccall did — production code can't hit the same serialisation failure.map.js: nobindPopup/bindTooltipcalls anywhere,MarkerCluster.Default.cssnot linked inindex.html,clusterIcondraws an inline SVG with explicitfill/stroke.One loose end, not a blocker:
PlaceMarkAppFixture's class remarks still say the E2E suite "runs on a schedule rather than per PR (ADR-0091)".ci.yml'se2ejob actually triggers onpull_request/pushtomain, same asbuild— there's noschedule:trigger anywhere in the workflow. This PR touches that exact file (addingNewPageAsync(ColorScheme)), and the Vikunja comment notes the suite "turned out to run per-PR already", but the stale sentence itself wasn't corrected while the file was open. Worth a one-line follow-up.@ -0,0 +21,4 @@{[E2EFact]public async Task ZoomControl_LightScheme_UsesSurfaceAndTextTokens() =>await AssertControlMatchesTokensAsync(ColorScheme.Light, ".leaflet-control-zoom-in", "--colour-surface", "--colour-text");Note for future maintenance: in light scheme,
.leaflet-control-zoom-in's background alone doesn't discriminate the override here — leaflet.css's hardcoded#fffalready equals--colour-surface's#ffffff, so this test currently only catches an unlinked/removed override through thecolorassertion below it. Not a bug, but if--colour-textever converges with vendor'sblacktoo, this specific test (not the others) would stop discriminating without any change to the test itself.Both addressed, pushed as
f733a91:PlaceMarkAppFixture's remarks corrected: the schedule-only claim was true of ADR-0091's original design but ADR-0092 partially superseded exactly that paragraph on Rob's own instruction, so the suite now gates every pull request viaci.yml'se2ejob. Left the ADR-0091 citation on the Chromium-install sentence alone — that part genuinely still stands, per ADR-0092's own "Everything else here stands" note — and didn't touch either ADR's frozen body: both already carry the correctPartially superseded by/supersession metadata, so ADR-0107'sAnswered bymechanism doesn't apply here (that's for an explicit deferred question, not this kind of partial reversal).--colour-control-border) to the two zoom-control "enabled" tests, which gives the light-scheme case a second, independent discriminator (the vendor's#cccdivider never coincides with the token in either scheme) and adds coverage of the border override that nothing here checked before. Verified against the real vendored+override CSS outside the test run: lightrgb(140,142,140)=#8c8e8c, darkrgb(102,113,126)=#66717e, both matching--colour-control-borderexactly.CI on
f733a91is green.Verdict: mergeable
Re-reviewed at
f733a91. Everything cleared atfd8529f5still stands; narrowed to the two changes plus the process point.ADR-0092 citation checks out. Read both records directly: ADR-0092's own "Supersedes" line names ADR-0091's "Consequence for where the suite runs" paragraph specifically — the exact sentence the old stale comment quoted (
...therefore runs on a schedule ... rather than through ci.yml) — and ADR-0091's header carries the matching "Partially superseded by: ADR-0092" note. Both already show correct supersession metadata (docs/adr/README.md's generated table agrees), and this PR touches neither frozen body. Not a substituted-but-wrong citation.The border assertion genuinely closes the gap. Vendor's
.leaflet-bar asetsborder-bottom: 1px solid #ccc; neither--colour-control-bordervalue (#8c8e8clight,#66717edark) coincides with#cccin either scheme, so it's a real second discriminator, not the same coincidence twice. Correctly targets.leaflet-control-zoom-in(first-child), not-zoom-out, avoiding the vendor's:last-child { border-bottom: none }rule that would have muddied that check.The log-archive workaround doesn't matter here. Nothing I verified this round (ADR text, CSS, specificity, contrast) depended on a suite total from a log; matching by job status on the full head SHA is the right call given the documented quirk, and it's the same failure mode ADR-0092 itself already worked around.
Flagging a genuine conflict, not a tooling glitch. The PR shows
mergeable: falseagainst currentmain. Cause: #185 rewrotePlaceMarkAppFixture's class-level<remarks>paragraph (newStubOidcProvider/OIDC sign-in content) on the same lines this PR's ADR-0092 correction touches — a real textual conflict. Needs a rebase/merge ofmainbefore the merge action will succeed. #185's other changes to that file (constants,_oidcProviderfield,InitializeAsync) don't overlap anything this PR edits, so resolving it should be a manual splice of the two remarks edits, not a re-review of substance.f733a913e064cd25744bVerdict: mergeable
Confirmed at
64cd257, rebase-only check:ci.yml's owne2ejob) — but the port-sharing constraint itself was never about the trigger, so it still stands unchanged." Past tense for ADR-0091's original position, present tense for what governs now — no reader ambiguity about which applies today.leaflet-theme.cssandindex.htmlare byte-identical to what I reviewed atf733a91/fd8529f5.LeafletControlThemingTests.cscarries the same 141 lines (border-colour assertion included).NewPageAsync(ColorScheme)sits untouched alongsidehasTouch, clear of the OIDC wiring.