From 136b165954dc5e2d419f9c6da8192a196dc8864d Mon Sep 17 00:00:00 2001 From: Florian Date: Fri, 30 Oct 2026 15:14:25 +0100 Subject: [PATCH] fix(map): let a DOM marker own the clicks that land on it MapLibre mounts marker elements inside the map's event container, so a click on a marker also fires a map click at that point. The parked-car marker, the location dot, the arrival parking markers, waypoints, the selected route pill and the dropped pin left that click unclaimed: clicking the parked car opened its panel and also dropped a grey pin, and layer handlers could open whatever lay beneath a marker. bindMapMarkerActivation now owns marker clicks for every marker. It claims the click through map click ownership instead of stopping it, so map handlers leave it alone while click-away dismissal still sees it, and a click an active picker owns stays the picker's. A static test requires markers to bind through it and every map click handler to consult ownership. --- .../src/components/map/MapClickHandler.tsx | 4 + .../components/map/MapClickOwnership.test.tsx | 45 +++++- .../map/ParkedVehicleMarkers.test.tsx | 15 ++ .../components/map/ParkedVehicleMarkers.tsx | 13 +- .../map/UserLocationMarker.test.tsx | 17 +++ .../src/components/map/UserLocationMarker.tsx | 13 +- .../src/components/map/WaypointMarkers.tsx | 4 + .../components/map/layers/RouteLayer.test.tsx | 17 +++ .../src/components/map/layers/RouteLayer.tsx | 17 +-- .../navigation/ArrivalParkingMarkers.test.tsx | 31 ++++ .../navigation/ArrivalParkingMarkers.tsx | 18 +-- apps/web/src/hooks/usePinMarker.ts | 18 +-- .../integration-api/map/mapClickOwnership.ts | 23 ++- .../map/mapMarkerActivation.test.ts | 124 ++++++++++++++++ .../map/mapMarkerActivation.ts | 46 ++++++ apps/web/src/lib/mapMarkerClickGate.test.ts | 133 ++++++++++++++++++ docs/docs/developer/integration-system.md | 17 +++ 17 files changed, 493 insertions(+), 62 deletions(-) create mode 100644 apps/web/src/integration-api/map/mapMarkerActivation.test.ts create mode 100644 apps/web/src/integration-api/map/mapMarkerActivation.ts create mode 100644 apps/web/src/lib/mapMarkerClickGate.test.ts diff --git a/apps/web/src/components/map/MapClickHandler.tsx b/apps/web/src/components/map/MapClickHandler.tsx index 2a97f1d86..e9b1ac1a5 100644 --- a/apps/web/src/components/map/MapClickHandler.tsx +++ b/apps/web/src/components/map/MapClickHandler.tsx @@ -17,6 +17,7 @@ import { useEffect, useRef } from "react"; import { INTERACTIVE_LAYER_IDS } from "@/integration-api/map/interactiveLayers"; import { useMap } from "@/integration-api/map/MapContext"; import { claimMapClick, getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; import { initializeMapClickModes } from "@/lib/mapClickModes"; // Configure before any map listener mounts. Read live mode state at the first @@ -119,6 +120,9 @@ export function MapClickHandler() { `; + // Tapping the dropped pin would otherwise re-drop it at the tapped spot, + // a pin's height above where it stood. + bindMapMarkerActivation(el); markerRef.current = new ml.Marker({ element: el, anchor: "bottom" }) .setLngLat(clickedLngLat) .addTo(mapRef.current); diff --git a/apps/web/src/components/map/MapClickOwnership.test.tsx b/apps/web/src/components/map/MapClickOwnership.test.tsx index 528197422..a4ae0666c 100644 --- a/apps/web/src/components/map/MapClickOwnership.test.tsx +++ b/apps/web/src/components/map/MapClickOwnership.test.tsx @@ -5,9 +5,10 @@ import { MeasurementLayer } from "@integrations/overlay-tool-measurement/map-lay import { useMeasurementStore } from "@integrations/overlay-tool-measurement/store"; import { TravelTimeLayer } from "@integrations/overlay-tool-travel-time/map-layer"; import { useTravelTimeStore } from "@integrations/overlay-tool-travel-time/store"; -import { useParkingStore } from "@openmapx/core"; +import { useMapClickStore, useParkingStore } from "@openmapx/core"; import { act, cleanup, render } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { claimMapClickForMarker } from "@/integration-api/map/mapClickOwnership"; import { createFakeMap } from "@/test/fakeMap"; import { MapClickHandler } from "./MapClickHandler"; @@ -94,6 +95,48 @@ describe("physical map click ownership", () => { }, ); + it("leaves a click a marker claimed to the marker", () => { + const fake = toolMap(); + useMapClickStore.getState().setClickedLngLat(null); + render(); + const onMarker = new MouseEvent("click"); + expect(claimMapClickForMarker(onMarker)).toBe(true); + act(() => + fake.emit("click", { + point: { x: 1, y: 2 }, + lngLat: { lng: 8, lat: 50 }, + originalEvent: onMarker, + }), + ); + expect(useMapClickStore.getState().clickedLngLat).toBeNull(); + + act(() => + fake.emit("click", { + point: { x: 1, y: 2 }, + lngLat: { lng: 8, lat: 50 }, + originalEvent: new MouseEvent("click"), + }), + ); + expect(useMapClickStore.getState().clickedLngLat).toEqual([8, 50]); + useMapClickStore.getState().setClickedLngLat(null); + }); + + it("does not let a marker claim a click an active tool owns", () => { + const fake = toolMap(); + act(() => useMeasurementStore.getState().activate()); + render(); + const onMarker = new MouseEvent("click"); + expect(claimMapClickForMarker(onMarker)).toBe(false); + act(() => + fake.emit("click", { + point: { x: 1, y: 2 }, + lngLat: { lng: 8, lat: 50 }, + originalEvent: onMarker, + }), + ); + expect(useMeasurementStore.getState().points).toEqual([[8, 50]]); + }); + it("gives measurement priority over travel-time when both tools are active", () => { const fake = toolMap(); act(() => { diff --git a/apps/web/src/components/map/ParkedVehicleMarkers.test.tsx b/apps/web/src/components/map/ParkedVehicleMarkers.test.tsx index eba728f3f..1e0dd9569 100644 --- a/apps/web/src/components/map/ParkedVehicleMarkers.test.tsx +++ b/apps/web/src/components/map/ParkedVehicleMarkers.test.tsx @@ -36,6 +36,7 @@ vi.mock("next-intl", async () => (await import("@/test/intl")).mockNextIntl()); import { PANEL, useParkingStore, useSidebarStore } from "@openmapx/core"; import * as mapContext from "@/integration-api/map/MapContext"; +import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; import { ParkedVehicleMarkers } from "./ParkedVehicleMarkers"; const mapContextTest = ( @@ -89,6 +90,20 @@ describe("ParkedVehicleMarkers", () => { fireEvent.keyDown(container.firstElementChild as HTMLElement, { key: "Enter" }); expect(useParkingStore.getState().selectedParkedId).toBe("p1"); }); + it("claims its click from the map, but leaves a picker's click to it", async () => { + const mapClickOwners: unknown[] = []; + container.addEventListener("click", (event) => mapClickOwners.push(getMapClickOwner(event))); + render(); + await waitFor(() => expect(container.children).toHaveLength(1)); + + (container.firstElementChild as HTMLElement).click(); + expect(useParkingStore.getState().selectedParkedId).toBe("p1"); + + useParkingStore.getState().setPicking(true); + (container.firstElementChild as HTMLElement).click(); + expect(mapClickOwners).toEqual(["marker", "parking"]); + }); + it("renders one marker per parked record", async () => { render(); await waitFor(() => expect(container.children).toHaveLength(1)); diff --git a/apps/web/src/components/map/ParkedVehicleMarkers.tsx b/apps/web/src/components/map/ParkedVehicleMarkers.tsx index 219bd5167..d34499a8e 100644 --- a/apps/web/src/components/map/ParkedVehicleMarkers.tsx +++ b/apps/web/src/components/map/ParkedVehicleMarkers.tsx @@ -11,7 +11,7 @@ import type * as maplibregl from "maplibre-gl"; import { useTranslations } from "next-intl"; import { useEffect, useRef } from "react"; import { useMap } from "@/integration-api/map/MapContext"; -import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; const BADGE_FILL = "#1A73E8"; @@ -68,16 +68,7 @@ function buildMarkerElement(label: string, onSelect: () => void): HTMLElement { ].join(";"); wrapper.append(badge, caption); - wrapper.addEventListener("click", (event) => { - if (getMapClickOwner(event)) return; - onSelect(); - }); - wrapper.addEventListener("keydown", (event) => { - if (event.key !== "Enter" && event.key !== " ") return; - event.preventDefault(); - event.stopPropagation(); - onSelect(); - }); + bindMapMarkerActivation(wrapper, { onActivate: onSelect }); return wrapper; } diff --git a/apps/web/src/components/map/UserLocationMarker.test.tsx b/apps/web/src/components/map/UserLocationMarker.test.tsx index dfd393aff..230543186 100644 --- a/apps/web/src/components/map/UserLocationMarker.test.tsx +++ b/apps/web/src/components/map/UserLocationMarker.test.tsx @@ -2,6 +2,7 @@ import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; vi.mock("next-intl", async () => (await import("@/test/intl")).mockNextIntl()); vi.mock("@/components/panels/MyLocationCard", () => ({ @@ -80,6 +81,22 @@ describe("UserLocationMarker", () => { fireEvent.keyDown(mapContainer.firstElementChild as HTMLElement, { key: " " }); expect(screen.getByText("my-location-card")).toBeInTheDocument(); }); + it("claims its click from the map", async () => { + const mapContainer = document.createElement("div"); + const mapClickOwners: unknown[] = []; + mapContainer.addEventListener("click", (event) => mapClickOwners.push(getMapClickOwner(event))); + mapContextTest.mapRef.current = { container: mapContainer }; + mapContextTest.mapReady = true; + useMapStore.setState({ userLocation: [13.4, 52.5] }); + render(); + await waitFor(() => expect(mapContainer.children).toHaveLength(1)); + + fireEvent.click(mapContainer.firstElementChild as HTMLElement); + + expect(screen.getByText("my-location-card")).toBeInTheDocument(); + expect(mapClickOwners).toEqual(["marker"]); + }); + it("adds a location published before map readiness when the map becomes ready", async () => { const mapContainer = document.createElement("div"); useMapStore.setState({ userLocation: [13.4, 52.5] }); diff --git a/apps/web/src/components/map/UserLocationMarker.tsx b/apps/web/src/components/map/UserLocationMarker.tsx index 42e742b83..ad93efb55 100644 --- a/apps/web/src/components/map/UserLocationMarker.tsx +++ b/apps/web/src/components/map/UserLocationMarker.tsx @@ -6,7 +6,7 @@ import { useTranslations } from "next-intl"; import { useEffect, useRef, useState } from "react"; import { MyLocationCard } from "@/components/panels/MyLocationCard"; import { useMap } from "@/integration-api/map/MapContext"; -import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; export function UserLocationMarker() { const t = useTranslations("parking"); @@ -51,16 +51,7 @@ export function UserLocationMarker() { wrapper.setAttribute("aria-label", locationLabel); wrapper.style.cssText = "position:relative;width:16px;height:16px;padding:0;border:0;background:none;cursor:pointer;"; - wrapper.addEventListener("click", (event) => { - if (getMapClickOwner(event)) return; - setCardOpen(true); - }); - wrapper.addEventListener("keydown", (event) => { - if (event.key !== "Enter" && event.key !== " ") return; - event.preventDefault(); - event.stopPropagation(); - setCardOpen(true); - }); + bindMapMarkerActivation(wrapper, { onActivate: () => setCardOpen(true) }); // Outer pulsing ring const pulse = document.createElement("div"); diff --git a/apps/web/src/components/map/WaypointMarkers.tsx b/apps/web/src/components/map/WaypointMarkers.tsx index 928c8b61b..58778bab8 100644 --- a/apps/web/src/components/map/WaypointMarkers.tsx +++ b/apps/web/src/components/map/WaypointMarkers.tsx @@ -5,6 +5,7 @@ import { useDirectionsStore } from "@openmapx/core"; import type * as maplibregl from "maplibre-gl"; import { useEffect, useRef } from "react"; import { useMap } from "@/integration-api/map/MapContext"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; import { BRAND } from "@/integration-api/runtime/theme"; const PIN_SVG_NS = "http://www.w3.org/2000/svg"; @@ -113,6 +114,9 @@ export function WaypointMarkers() { const map = mapRef.current; if (!map) return; + // A waypoint is moved by dragging; a plain tap on it must not fill the + // next empty waypoint or drop a pin at the spot it covers. + bindMapMarkerActivation(el); const marker = new maplibregl.Marker({ element: el, anchor, draggable: true }) .setLngLat(wp.coords) .addTo(map); diff --git a/apps/web/src/components/map/layers/RouteLayer.test.tsx b/apps/web/src/components/map/layers/RouteLayer.test.tsx index 4e03fcfb9..f6d792d75 100644 --- a/apps/web/src/components/map/layers/RouteLayer.test.tsx +++ b/apps/web/src/components/map/layers/RouteLayer.test.tsx @@ -2,6 +2,7 @@ import { type EvChargeStop, useParkingStore } from "@openmapx/core"; import { act, fireEvent, render } from "@testing-library/react"; import type { MapGeoJSONFeature } from "maplibre-gl"; import { describe, expect, it, vi } from "vitest"; +import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; import { useMapAttributionStore } from "@/integration-api/overlay/mapAttributionStore"; import { createFakeMap, expectStyleSwapIsLossless } from "@/test"; @@ -121,6 +122,22 @@ describe("RouteLayer across a style change", () => { view.unmount(); useParkingStore.getState().reset(); }); + it("claims clicks on either route badge from the map", async () => { + activateRoute.mockClear(); + const view = render(); + await act(async () => {}); + const canvasContainer = fake.map.getCanvasContainer(); + const mapClickOwners: unknown[] = []; + const mapClick = (event: Event) => mapClickOwners.push(getMapClickOwner(event)); + canvasContainer.addEventListener("click", mapClick); + const pills = [...canvasContainer.querySelectorAll(".omx-route-map-pill")]; + expect(new Set(pills.map((pill) => pill.tagName))).toEqual(new Set(["SPAN", "BUTTON"])); + for (const pill of pills) fireEvent.click(pill); + expect(activateRoute).toHaveBeenCalledTimes(1); + expect(mapClickOwners).toEqual(["marker", "marker"]); + canvasContainer.removeEventListener("click", mapClick); + view.unmount(); + }); it("keeps the drawn route", () => { render(); const before = fake.state.sources.get("route-source")?.data as { features: unknown[] }; diff --git a/apps/web/src/components/map/layers/RouteLayer.tsx b/apps/web/src/components/map/layers/RouteLayer.tsx index 5c54ce0a3..31866758f 100644 --- a/apps/web/src/components/map/layers/RouteLayer.tsx +++ b/apps/web/src/components/map/layers/RouteLayer.tsx @@ -25,6 +25,7 @@ import { type MapLayerGroup, type SlottedLayer, } from "@/integration-api/map/mapLayerGroup"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; import { useDrawnDirectionsRoutes } from "@/integration-api/map/useDrawnDirectionsRoutes"; import { useMapLayerGroup } from "@/integration-api/map/useMapLayerGroup"; import { markerMapContributors } from "@/integration-api/overlay/markerMapContribution"; @@ -278,17 +279,11 @@ export function RouteLayer() { : theme.palette.divider; if (element instanceof HTMLButtonElement) { element.type = "button"; - element.addEventListener("click", (event) => { - if (getMapClickOwner(event)) return; - event.stopPropagation(); - setActiveRouteIndex(routeIndex); - }); - element.addEventListener("keydown", (event) => { - if (event.key !== "Enter" && event.key !== " ") return; - event.preventDefault(); - event.stopPropagation(); - setActiveRouteIndex(routeIndex); - }); + bindMapMarkerActivation(element, { onActivate: () => setActiveRouteIndex(routeIndex) }); + } else { + // The selected route's pill has nothing to do, but a tap on it is + // still not a tap on the map below. + bindMapMarkerActivation(element); } elements.set(routeIndex, element); pillReceipts.current.set( diff --git a/apps/web/src/components/navigation/ArrivalParkingMarkers.test.tsx b/apps/web/src/components/navigation/ArrivalParkingMarkers.test.tsx index 1b0f90030..042cdee08 100644 --- a/apps/web/src/components/navigation/ArrivalParkingMarkers.test.tsx +++ b/apps/web/src/components/navigation/ArrivalParkingMarkers.test.tsx @@ -3,6 +3,7 @@ import type { CategoryPlace } from "@openmapx/core"; import { useParkingStore } from "@openmapx/core"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; import { fireEvent, render } from "@/test"; const { createdMarkers, FakeMarker } = vi.hoisted(() => { @@ -154,6 +155,36 @@ describe("ArrivalParkingMarkers", () => { expect(onSelectPlace).toHaveBeenCalledWith(samplePlaces[0]); }); + it("claims clicks from the map, even while disabled", () => { + mockMapContext.current = { mapRef: { current: { container: mapContainer } } }; + const mapClickOwners: unknown[] = []; + const mapClick = (event: Event) => mapClickOwners.push(getMapClickOwner(event)); + mapContainer.addEventListener("click", mapClick); + const onSelectPlace = vi.fn(); + const { rerender } = render( + , + ); + fireEvent.click(mapContainer.querySelector(".omx-arrival-parking-marker") as HTMLElement); + expect(onSelectPlace).toHaveBeenCalledWith(samplePlaces[0]); + + rerender( + , + ); + fireEvent.click(mapContainer.querySelector(".omx-arrival-parking-marker") as HTMLElement); + expect(onSelectPlace).toHaveBeenCalledTimes(1); + expect(mapClickOwners).toEqual(["marker", "marker"]); + mapContainer.removeEventListener("click", mapClick); + }); + it("renders without crashing when map context is null", () => { const { container } = render( , diff --git a/apps/web/src/components/navigation/ArrivalParkingMarkers.tsx b/apps/web/src/components/navigation/ArrivalParkingMarkers.tsx index 6e3844fae..a79f2063f 100644 --- a/apps/web/src/components/navigation/ArrivalParkingMarkers.tsx +++ b/apps/web/src/components/navigation/ArrivalParkingMarkers.tsx @@ -7,7 +7,7 @@ import { Marker } from "maplibre-gl"; import { useTranslations } from "next-intl"; import { useEffect, useId, useRef } from "react"; import { useMapOptional } from "@/integration-api/map/MapContext"; -import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; import { useMapAttributionStore } from "@/integration-api/overlay/mapAttributionStore"; import { markerMapContributors } from "@/integration-api/overlay/markerMapContribution"; import { renderedMapAttributionTracker } from "@/integration-api/overlay/renderedMapAttributionTracker"; @@ -80,16 +80,12 @@ export function ArrivalParkingMarkers({ el.style.boxShadow = "0 2px 4px rgba(0,0,0,0.2)"; el.textContent = "P"; - el.addEventListener("click", (event) => { - if (getMapClickOwner(event)) return; - if (!disabled) onSelectPlace(isSelected ? null : place); - }); - - el.addEventListener("keydown", (e) => { - if (!disabled && (e.key === "Enter" || e.key === " " || e.key === "Spacebar")) { - e.preventDefault(); - onSelectPlace(isSelected ? null : place); - } + // A disabled marker still owns its clicks: it is drawn, so a tap on it + // is not a tap on the map below. + bindMapMarkerActivation(el, { + onActivate: () => { + if (!disabled) onSelectPlace(isSelected ? null : place); + }, }); const marker = new Marker({ element: el }).setLngLat(coords).addTo(map); diff --git a/apps/web/src/hooks/usePinMarker.ts b/apps/web/src/hooks/usePinMarker.ts index c53eb9aae..d512f95b0 100644 --- a/apps/web/src/hooks/usePinMarker.ts +++ b/apps/web/src/hooks/usePinMarker.ts @@ -9,7 +9,7 @@ import { import type * as maplibregl from "maplibre-gl"; import { useEffect, useRef, useState } from "react"; import { useMap } from "@/integration-api/map/MapContext"; -import { getMapClickOwner } from "@/integration-api/map/mapClickOwnership"; +import { bindMapMarkerActivation } from "@/integration-api/map/mapMarkerActivation"; import { useMarkerMapAttributions } from "@/integration-api/overlay/useMarkerMapAttributions"; import { getMapObstructionInsets, subscribeMapObstructions } from "@/lib/mapObstructions"; @@ -266,17 +266,11 @@ export function usePinMarker( el.tabIndex = 0; el.setAttribute("aria-label", label); } - el.addEventListener("click", (event) => { - if (getMapClickOwner(event)) return; - if (!onActivateRef.current) return; - event.stopPropagation(); - onActivateRef.current(); - }); - el.addEventListener("keydown", (event) => { - if (!onActivateRef.current || (event.key !== "Enter" && event.key !== " ")) return; - event.preventDefault(); - event.stopPropagation(); - onActivateRef.current(); + // Without an action the pin is see-through: a hover pin drawn over a + // feature still lets that feature take the click. + bindMapMarkerActivation(el, { + onActivate: () => onActivateRef.current?.(), + isEnabled: () => Boolean(onActivateRef.current), }); const svgDiv = document.createElement("div"); diff --git a/apps/web/src/integration-api/map/mapClickOwnership.ts b/apps/web/src/integration-api/map/mapClickOwnership.ts index 4f24f521c..c3b5b1061 100644 --- a/apps/web/src/integration-api/map/mapClickOwnership.ts +++ b/apps/web/src/integration-api/map/mapClickOwnership.ts @@ -1,18 +1,18 @@ import { useParkingStore } from "@openmapx/core"; -type MapClickOwner = "parking" | "crowd" | "measurement" | "travel-time"; +/** `marker`: a DOM marker took the click; see `bindMapMarkerActivation`. */ +type MapClickOwner = "parking" | "crowd" | "measurement" | "travel-time" | "marker"; +type ModeOwner = Exclude; interface Ownership { owner: MapClickOwner | null; claimed: boolean; } const ownership = new WeakMap(); -let readToolOwner: () => Exclude | null = () => null; +let readToolOwner: () => ModeOwner | null = () => null; /** The map shell supplies integration state; this API stays independent of it. */ -export function configureMapClickOwnership( - resolver: () => Exclude | null, -): void { +export function configureMapClickOwnership(resolver: () => ModeOwner | null): void { readToolOwner = resolver; } @@ -44,3 +44,16 @@ export function claimMapClick(event: object, owner: MapClickOwner): boolean { captured.claimed = true; return true; } + +/** + * A DOM marker takes a click no picker or tool owns. Every map click handler + * then sees an owner and leaves the click alone, while listeners outside the + * map (click-away dismissal) still receive it. + */ +export function claimMapClickForMarker(event: object): boolean { + const captured = getOwnership(event); + if (captured.owner) return false; + captured.owner = "marker"; + captured.claimed = true; + return true; +} diff --git a/apps/web/src/integration-api/map/mapMarkerActivation.test.ts b/apps/web/src/integration-api/map/mapMarkerActivation.test.ts new file mode 100644 index 000000000..db5d815a5 --- /dev/null +++ b/apps/web/src/integration-api/map/mapMarkerActivation.test.ts @@ -0,0 +1,124 @@ +// @vitest-environment jsdom + +import { useParkingStore } from "@openmapx/core"; +import { fireEvent } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getMapClickOwner } from "./mapClickOwnership"; +import { bindMapMarkerActivation } from "./mapMarkerActivation"; + +// Stands in for MapLibre's canvas container: marker elements are mounted inside +// it, and the map turns a click that reaches it into a map `click`. +let mapContainer: HTMLElement; +let marker: HTMLElement; +let mapClicks: Array; +let documentClicks: number; + +beforeEach(() => { + useParkingStore.getState().reset(); + mapContainer = document.createElement("div"); + marker = document.createElement("div"); + marker.append(document.createElement("span")); + mapContainer.append(marker); + document.body.append(mapContainer); + mapClicks = []; + mapContainer.addEventListener("click", (event) => mapClicks.push(getMapClickOwner(event))); + mapContainer.addEventListener("keydown", () => mapClicks.push("key")); + documentClicks = 0; + document.addEventListener("click", countDocumentClick); +}); + +function countDocumentClick() { + documentClicks += 1; +} + +afterEach(() => { + document.removeEventListener("click", countDocumentClick); + mapContainer.remove(); + useParkingStore.getState().reset(); +}); + +describe("bindMapMarkerActivation", () => { + it("activates the marker and claims the click from the map", () => { + const onActivate = vi.fn(); + bindMapMarkerActivation(marker, { onActivate }); + + (marker.firstElementChild as HTMLElement).click(); + + expect(onActivate).toHaveBeenCalledTimes(1); + expect(mapClicks).toEqual(["marker"]); + }); + + it("still lets the click reach listeners outside the map", () => { + bindMapMarkerActivation(marker, { onActivate: () => undefined }); + + marker.click(); + + expect(documentClicks).toBe(1); + }); + + it("claims the click when there is nothing to activate", () => { + bindMapMarkerActivation(marker); + + marker.click(); + + expect(mapClicks).toEqual(["marker"]); + }); + + it("leaves a picker-owned click to the map without activating", () => { + const onActivate = vi.fn(); + bindMapMarkerActivation(marker, { onActivate }); + useParkingStore.getState().setPicking(true); + + marker.click(); + + expect(onActivate).not.toHaveBeenCalled(); + expect(mapClicks).toEqual(["parking"]); + }); + + it("is see-through while disabled", () => { + const onActivate = vi.fn(); + let enabled = false; + bindMapMarkerActivation(marker, { onActivate, isEnabled: () => enabled }); + + marker.click(); + fireEvent.keyDown(marker, { key: "Enter" }); + expect(onActivate).not.toHaveBeenCalled(); + expect(mapClicks).toEqual([null, "key"]); + + enabled = true; + marker.click(); + expect(onActivate).toHaveBeenCalledTimes(1); + expect(mapClicks).toEqual([null, "key", "marker"]); + }); + + it.each(["Enter", " "])("activates on %j without passing the key on", (key) => { + const onActivate = vi.fn(); + bindMapMarkerActivation(marker, { onActivate }); + + const handled = !fireEvent.keyDown(marker, { key }); + + expect(handled).toBe(true); + expect(onActivate).toHaveBeenCalledTimes(1); + expect(mapClicks).toEqual([]); + }); + + it("activates from the keyboard even while a picker is active", () => { + const onActivate = vi.fn(); + bindMapMarkerActivation(marker, { onActivate }); + useParkingStore.getState().setPicking(true); + + fireEvent.keyDown(marker, { key: "Enter" }); + + expect(onActivate).toHaveBeenCalledTimes(1); + }); + + it("ignores other keys", () => { + const onActivate = vi.fn(); + bindMapMarkerActivation(marker, { onActivate }); + + fireEvent.keyDown(marker, { key: "a" }); + + expect(onActivate).not.toHaveBeenCalled(); + expect(mapClicks).toEqual(["key"]); + }); +}); diff --git a/apps/web/src/integration-api/map/mapMarkerActivation.ts b/apps/web/src/integration-api/map/mapMarkerActivation.ts new file mode 100644 index 000000000..2ac9f373d --- /dev/null +++ b/apps/web/src/integration-api/map/mapMarkerActivation.ts @@ -0,0 +1,46 @@ +import { claimMapClickForMarker } from "./mapClickOwnership"; + +export interface MapMarkerActivationOptions { + /** Runs on a pointer click or Enter/Space. Omit for a marker that only keeps its clicks off the map. */ + onActivate?: () => void; + /** + * Read on every click. While it returns false the marker is see-through: the + * click reaches the map as if the marker were not there. + */ + isEnabled?: () => boolean; +} + +/** + * Makes a DOM marker element own the clicks that land on it. + * + * MapLibre mounts marker elements inside the map's canvas container, so a click + * on one bubbles to the map and fires the map's `click` event at that point. + * Unclaimed, every map click handler treats it as a click on the map + * underneath: the plain-map handler drops a grey pin and closes the place + * panel, and layer handlers open whatever feature lies below the marker. + * + * The click is claimed through map click ownership rather than stopped, so it + * still reaches listeners outside the map, such as the context menu's + * click-away dismissal. A click an active picker or tool owns (repositioning a + * parked car, placing a report, measuring) stays the picker's: the marker + * neither activates nor claims it. + */ +export function bindMapMarkerActivation( + element: HTMLElement, + { onActivate, isEnabled }: MapMarkerActivationOptions = {}, +): void { + element.addEventListener("click", (event) => { + if (isEnabled && !isEnabled()) return; + if (!claimMapClickForMarker(event)) return; + onActivate?.(); + }); + if (!onActivate) return; + element.addEventListener("keydown", (event) => { + if (event.key !== "Enter" && event.key !== " ") return; + if (isEnabled && !isEnabled()) return; + event.preventDefault(); + // The key was handled here; nothing behind the marker should act on it too. + event.stopPropagation(); + onActivate(); + }); +} diff --git a/apps/web/src/lib/mapMarkerClickGate.test.ts b/apps/web/src/lib/mapMarkerClickGate.test.ts new file mode 100644 index 000000000..0d2922234 --- /dev/null +++ b/apps/web/src/lib/mapMarkerClickGate.test.ts @@ -0,0 +1,133 @@ +// @vitest-environment node +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +const REPO_ROOT = fileURLToPath(new URL("../../../..", import.meta.url)); +const ROOTS = ["apps/web/src", "integrations"]; + +// MapLibre mounts DOM markers inside the map's event container, so a click on +// one is also a map click at that point. Markers go through +// `bindMapMarkerActivation`, which claims the click for the marker unless an +// active picker owns it. These files create markers that deliberately leave +// clicks to the map. +const PASS_THROUGH = new Set([ + // Hover elevation dot; `pointer-events: none`, never under the pointer. + "apps/web/src/components/map/ElevationHoverMarker.tsx", + // Navigation puck: decoration under the follow camera, nothing to activate. + "apps/web/src/lib/navigation/useNavCamera.ts", + // Standalone preview map with no click handling of its own. + "apps/web/src/integration-api/components/LocationMinimap.tsx", +]); + +// The binding itself is the one place a marker click listener belongs. +const BINDING = "apps/web/src/integration-api/map/mapMarkerActivation.ts"; + +const MAP_CLICK_HANDLER = /\.on\(\s*["'`]click["'`]/; +const CONSULTS_OWNERSHIP = /\b(?:getMapClickOwner|claimMapClick)\s*\(/; +const NEW_MARKER = /\bnew\s+(?:[A-Za-z_$][\w$]*\.)?Marker\s*\(/; +const NEW_MARKERS = new RegExp(NEW_MARKER.source, "g"); +const BINDS_MARKER = /\bbindMapMarkerActivation\s*\(/g; +const RAW_CLICK_LISTENER = /\.addEventListener\(\s*["'`]click["'`]/; + +function collect(dir: string): string[] { + const out: string[] = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.name === "node_modules" || entry.name === "__tests__") continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...collect(full)); + else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) out.push(full); + } + return out; +} + +function markerClickOffenders(files: Array<{ file: string; source: string }>): string[] { + const offenders: string[] = []; + for (const { file, source } of files) { + if (file === BINDING || !NEW_MARKER.test(source)) continue; + const created = source.match(NEW_MARKERS)?.length ?? 0; + const bound = source.match(BINDS_MARKER)?.length ?? 0; + if (!PASS_THROUGH.has(file) && bound < created) { + offenders.push(`${file}: creates a Marker without bindMapMarkerActivation`); + } + source.split("\n").forEach((line, index) => { + if (RAW_CLICK_LISTENER.test(line)) offenders.push(`${file}:${index + 1}: raw click listener`); + }); + } + return offenders; +} + +/** + * A marker claims its click through map click ownership instead of stopping it, + * so the click still reaches listeners outside the map. That only works while + * every map click handler asks who owns the click before acting on it. + */ +function unownedMapClickHandlers(files: Array<{ file: string; source: string }>): string[] { + return files + .filter(({ source }) => MAP_CLICK_HANDLER.test(source) && !CONSULTS_OWNERSHIP.test(source)) + .map(({ file }) => file); +} + +/** + * A text match, not a call graph: it counts binding calls against marker + * constructions per file, so a marker element built in one file and mounted in + * another escapes it. Every marker so far is built where it is + * mounted, and the gate makes each new one choose between binding and the + * documented pass-through list. + */ +describe("DOM marker clicks stay off the map", () => { + const files = ROOTS.flatMap((root) => collect(path.join(REPO_ROOT, root))).map((full) => ({ + file: path.relative(REPO_ROOT, full), + source: fs.readFileSync(full, "utf8"), + })); + + it("finds the markers it guards", () => { + expect(files.filter(({ source }) => NEW_MARKER.test(source)).length).toBeGreaterThan(8); + }); + + it("binds every interactive marker through bindMapMarkerActivation", () => { + expect(markerClickOffenders(files)).toEqual([]); + }); + + it("has every map click handler consult click ownership", () => { + expect(unownedMapClickHandlers(files)).toEqual([]); + }); + + it("matcher flags map click handlers that ignore ownership", () => { + const handler = 'map.on("click", LAYER, (e) => openFeature(e));'; + expect(unownedMapClickHandlers([{ file: "x.ts", source: handler }])).toEqual(["x.ts"]); + const owned = `${handler}\nif (getMapClickOwner(e)) return;`; + expect(unownedMapClickHandlers([{ file: "x.ts", source: owned }])).toEqual([]); + }); + + it("matcher flags unbound markers and raw click listeners", () => { + const unbound = "const m = new maplibregl.Marker({ element }).addTo(map);"; + expect(markerClickOffenders([{ file: "x.ts", source: unbound }])).toEqual([ + "x.ts: creates a Marker without bindMapMarkerActivation", + ]); + const raw = [ + "bindMapMarkerActivation(el);", + 'el.addEventListener("click", onClick);', + "new Marker({ element: el });", + ].join("\n"); + expect(markerClickOffenders([{ file: "x.ts", source: raw }])).toEqual([ + "x.ts:2: raw click listener", + ]); + const bound = "bindMapMarkerActivation(el, { onActivate });\nnew ml.Marker({ element: el });"; + expect( + markerClickOffenders([ + { file: "x.ts", source: `${bound}\nnew ml.Marker({ element: other });` }, + ]), + ).toEqual(["x.ts: creates a Marker without bindMapMarkerActivation"]); + expect(markerClickOffenders([{ file: "x.ts", source: bound }])).toEqual([]); + expect(markerClickOffenders([{ file: "x.ts", source: "new MarkerCluster()" }])).toEqual([]); + }); + + it("exempts only files that still exist", () => { + const missing = [...PASS_THROUGH, BINDING].filter( + (file) => !fs.existsSync(path.join(REPO_ROOT, file)), + ); + expect(missing).toEqual([]); + }); +}); diff --git a/docs/docs/developer/integration-system.md b/docs/docs/developer/integration-system.md index 93cd93a46..26ffbefb0 100644 --- a/docs/docs/developer/integration-system.md +++ b/docs/docs/developer/integration-system.md @@ -1153,6 +1153,23 @@ missing. Fence asynchronous completions by the current effect, map and source identity. Layer-scoped event listeners tolerate a layer that does not exist yet, so bind interactions to the overlay's visibility rather than to installation. +A physical map click has one owner. Every map click handler, layer-scoped or +not, starts with `getMapClickOwner(event)` from +`@/integration-api/map/mapClickOwnership` and leaves an owned click alone; an +active picker or tool takes its point with `claimMapClick(event, owner)`. + +MapLibre mounts DOM markers inside the map's event container, so a click on a +marker is also a map click at that point. Unclaimed, the plain-map handler drops +a pin there and layer handlers open whatever lies beneath. Bind each marker +element with `bindMapMarkerActivation(element, { onActivate })` from +`@/integration-api/map/mapMarkerActivation`. It claims the click for the marker +and runs the action, leaves a click an active picker owns to the picker, and +handles Enter and Space. The click is claimed, not stopped, so listeners outside +the map such as click-away dismissal still see it. Omit `onActivate` for a +marker that has no action but still covers the map. A static test rejects map +click handlers that skip the ownership check and marker code that bypasses the +binding. + The modules under `apps/web/src/integration-api/` are an app-local API for trusted integrations compiled into the OpenMapX web build. They group map, overlay, runtime, and reusable component capabilities behind focused import