feat: improve legacy map accessibility - #143
Conversation
kawacukennedy
left a comment
There was a problem hiding this comment.
Thanks for taking this on, @anthony-maio — the map accessibility work in #108 is overdue and this is a thoughtful, well-scoped PR. The helper functions are clean, the tests are focused, and separating accessibility logic into mapAccessibility.ts makes it easy to reason about. I went through it carefully and have a few findings, two of which I'd consider blockers because they introduce a visual regression on the map (ironic for an accessibility PR) and a mismatch between the announced label and what's shown.
🐛 Blocking
1. d-sr-only is undefined → visible text regression on the map
webapp/src/components/LeafletMap.vue uses class="d-sr-only" in two places — the cluster icon (<span class="d-sr-only">${accessibleLabel}</span>) and the aria-live status line (<p class="d-sr-only" aria-live="polite">). But d-sr-only (or any sr-only class) is not defined anywhere — I grepped webapp/src, webapp/src/assets/main.css, index.html, and the installed Vuetify CSS. That means:
- Every cluster will visibly render its text label next to/behind the number (e.g. "7" + "7 cameras" both shown). This is compounded by the existing rule in
main.css:.marker-cluster span { color: #000000 !important; }, so the extra text is black-on-cluster. - The aria-live status
<p>will render as a visible line of text pinned inside the map container, and will keep growing as announcements fire.
The exact thing this PR sets out to fix (screen-reader-only content) is what breaks. You'll want to add a proper visually-hidden utility (ideally in main.css, not scoped per-component) and use it in both spots, e.g.:
.sr-only {
position: absolute !important;
width: 1px; height: 1px;
padding: 0; margin: -1px;
overflow: hidden;
clip: rect(0 0 0 0);
white-space: nowrap;
border: 0;
}2. cameraAriaLabel() reads only tags.manufacturer/tags.operator, but the popup reads 4/2 keys
mapAccessibility.ts:4-9 uses camera.tags.manufacturer and camera.tags.operator exclusively. However DFMapPopup.vue:68-73 resolves the manufacturer from manufacturer || surveillance:manufacturer || brand || surveillance:brand, and the operator from operator || surveillance:operator (line 89-93). Since the project's whole tagging convention is under the surveillance:* prefix (see constants.ts), a large share of real-world cameras will be labelled "unknown vendor / unknown operator" by the screen reader while the very same popup shows the correct values. The accessible name should mirror what's displayed, otherwise the fix delivers exactly the kind of generic, context-free label #108 is complaining about.
Suggest extracting a shared getManufacturer(tags) / getOperator(tags) (or a single cameraDescription) used by both DFMapPopup and mapAccessibility.cameraAriaLabel, so the announced name and the visible content can't drift.
🔎 Worth checking (non-blocking)
3. Tests aren't wired into CI and there's no test script
The new webapp/src/components/__tests__/mapAccessibility.test.ts passes locally (bun test), but webapp/package.json has no test script and no workflow runs these tests — so they won't gate anything on PRs. That's a pre-existing gap (the whole webapp has no test setup), so not introduced by this PR, but it's worth folding into this effort: otherwise the "6 passing" won't prevent silent regressions. Happy to help add a test script + a CI step.
4. Cluster keyboard activation relies on Leaflet's implicit keyboard handling, unlike markers
Individual markers get explicit tabindex="0" / role="button" via enableMarkerKeyboardBehavior, but cluster activation depends on cluster.options.keyboard = true and Leaflet adding tabindex automatically. Worth confirming in a real keyboard-only pass that clusters actually receive focus and that handleClusterKeydown fires — it may be fine, but it's the one interaction path not covered by the focused unit tests.
✅ Looks good
filterCamerasInBounds/paginateCamerasand their tests are correct and well-scoped (boundary inclusion, 1-based pagination, empty-page handling).- Focus restoration with the generation counter is a solid approach to the "restore focus when popup closes" requirement.
- The camera-list panel is a nice, genuinely useful addition and follows the existing
bottomrightslot pattern.
I'm happy to help test the fixes or wire up the test script + CI if useful — this is great work and close to mergeable once the d-sr-only and label-mismatch items are addressed.
|
Thanks for the careful review — both blockers were valid. I’ve added a global .sr-only utility and replaced both undefined d-sr-only usages. I also extracted shared manufacturer/operator resolvers and now use them in both DFMapPopup and cameraAriaLabel(), preserving the existing 4-key manufacturer and 2-key operator precedence. I added regression coverage for the prefixed tags and fallback precedence. All 8 focused tests pass, the production build succeeds, and git diff --check is clean. I kept the CI wiring and cluster keyboard verification out of this patch since they’re non-blocking and broader than these two fixes. |
kawacukennedy
left a comment
There was a problem hiding this comment.
Thanks @anthony-maio — I re-verified both fixes on 5e37a69 and they resolve the blockers cleanly.
.sr-onlyutility — added tomain.csswith the standard visually-hidden pattern and both priord-sr-onlyusages converted. Confirmed nod-sr-onlyremains anywhere in the PR.- Shared resolvers —
getCameraManufacturer(4-key precedence) andgetCameraOperator(2-key precedence) live inmapAccessibility.tsand are now imported by bothDFMapPopupandcameraAriaLabel. This is even better than I asked for: a single source of truth means the announced name and the visible popup content can't drift again. - Regression coverage — the new tests pin the prefixed-tag fallback and the precedence ordering.
I ran the focused suite against 5e37a69 myself: bun test src/components/__tests__/mapAccessibility.test.ts → 8 pass, 0 fail, and bun run build-only succeeds. The chunk-size warning on the map bundle is pre-existing and unrelated.
Agreed on leaving the CI wiring and cluster-keyboard verification for a follow-up — both are non-blocking and better done as their own focused change. Happy to review that when it lands.
Summary
Verification
bun test src/components/__tests__/mapAccessibility.test.ts— 6 passed, 0 failedbun run build-only— passedbun run type-check— still reports only the pre-existing@vitejs/plugin-vuedeclaration errors andsrc/App.vuenavigation-item typing errors; no errors in the changed filesScope
Closes #108