Core: map-anchored popup service driven over the event bus - #303
Draft
CarsonDavis wants to merge 27 commits into
Draft
Core: map-anchored popup service driven over the event bus#303CarsonDavis wants to merge 27 commits into
CarsonDavis wants to merge 27 commits into
Conversation
8 tasks
This was referenced Aug 11, 2026
CarsonDavis
force-pushed
the
feature/298-map-popup-service
branch
from
August 11, 2026 19:53
fc1820a to
f70ba50
Compare
Overlay-mode basemap movement synced the view state silently and left out bearing and pitch, so anchored consumers subscribed to 'move' never heard from deck.gl and projection drifted on a rotated or tilted basemap.
A plugin sends one serializable request — lat/lng, sanitized HTML, up to two naked action events and a dismiss event — and core owns the DOM, the theme and the lifecycle. The popup hosts on document.body so it paints above the panel layer and its clicks cannot reach either engine's click pipeline, and it tracks the anchor on every engine move. A single popup exists at a time; a mission switch tears it down through the provider cleanups.
Both basemap handlers copied the camera into `_viewState`, but only the `move` path carried `bearing` and `pitch`, so a rotated or tilted basemap projected anchors from a stale camera whenever `moveend` was the last event to land. One `_syncViewState(eventName)` now serves both, and the moveend path is covered by its own spec.
The dismissal was arbitrated by global bus ordering and a singleton timer slot, which broke in the two orderings that matter most: deck.gl calls its feature-click handler before it emits the click, so a popup opened from `map:featureClick` dismissed itself a tick later, and a click handler left over from a replaced popup closed its replacement and fired the replacement's dismiss event. Each popup now ignores map clicks until the task that opened it has run to completion, and every deferral checks that its own popup is still the open one before closing anything. Both orderings are pinned by specs. The card also carries its own placement now that the zero-size host is gone: the host's zero width made the card's shrink-to-fit resolve against nothing, so every popup rendered at `min-width` and `max-width` never applied. Dropping it leaves one source for the anchor gap and one element of state. An anchor projected outside the map container hides the card instead of pinning it to the viewport edge over the panel layer. Also folds the card builder into the service, drops the Escape listener the contract does not define, drops the redundant `moveend` subscription (both engines emit `move` on every camera change that reaches `moveend`), and ignores an action whose label or event is not a usable string.
A plugin being unloaded had no way to take its popup down: closing was core-only, so an orphaned popup could outlive the tool that opened it. The provider takes no payload and fires no dismiss event, matching the silent close a replacing request already performs.
Covers `map:hidePopup`, the ordering a plugin sees when a click dismisses one popup while its replacement is still pending, and the action validation. The overlay providers come back out of the public table: `map:addOverlay` takes a live `mount` function, which is the boundary this popup service exists to replace, so documenting it invites new consumers of the contract on its way out. Escape is gone, and the button row now reads in the order the code runs.
The popup no longer broadcasts caller-named events. `show` returns a promise that stays pending while the popup is open and resolves with how it closed, so an outcome reaches only the plugin that asked for it and the core never emits an event on a caller's behalf. Actions carry a label only, `dismissEvent` is gone, and a replaced or retracted popup now tells its requester (`closed`) instead of vanishing silently. Invalid requests reject rather than returning false.
Every lifecycle path asserts the value its request resolves with, recorded as a list so a promise that never settles or settles twice fails the test. Adds coverage for an invalid request rejecting, a replaced popup resolving `closed`, and a dismissal followed by teardown settling only once.
Rewrites the `map:showPopup` section for the new contract: a request without event names, the `MapPopupResult` it resolves with, when each action value occurs, and the rejection cases. Drops the note on namespacing event names handed to core, which no longer has a referent.
The teardown steps in hide() ran before the settle, so a throw part-way through would strand the request promise for good: _open is already null, so no later hide() can reach the record. Settling in a finally makes exactly-once unconditional instead of contingent on teardown not throwing. The reject-before-unwind ordering is unchanged — the first settlement is still the answer. Also drop the leftover whole-action parameter from buildActionButton, which reads only the label, and name the button-slot union once. The track helper's docstring claimed the specs catch a double settlement. A promise absorbs every settlement after the first, so they cannot; what they do catch is a missing or wrong first outcome. Say that instead.
The ADR's sandbox bridge sketches a 5s default request timeout. A popup can stay open for minutes, so a bridge built to the ADR as written would reject map:showPopup while its popup is still on screen. Record the carve-out where whoever builds the bridge will read it. The canonical plugin example also no longer runs as written: the request now stays pending until the popup closes, so the trailing hidePopup could never be reached. Store the request, handle its outcome in a then, and move the retraction into the plugin's own teardown, the way a plugin would structure it.
The action row is a grid of equal `fr` columns rather than a flex row, so it measures as the widest label instead of the sum of both: the card grows to fit two full-width buttons, and a label like "Analyze area" no longer wraps. Past the card's max width an outsized label ellipsizes rather than wrapping. The primary action now leads the row, and a lone action takes the primary styling whichever field it arrived in while still reporting that field.
CarsonDavis
force-pushed
the
feature/298-map-popup-service
branch
from
August 14, 2026 17:14
9c72e45 to
cb13772
Compare
Drawing a rectangle in the AOI tool drew it, opened the analyze/cancel popup, and then dropped both on its own. The click that finished the rectangle came back around as a map click after the popup was already open, and the popup reads a map click as a dismissal — which is AOI's Cancel, so the selection went with it. terra-draw commits a shape on pointerup and the engine hears about that same gesture's click only afterwards: Leaflet on the native click that follows, and deck.gl up to 300ms later, because its click recognizer waits for a double-click to fail before firing. By then the session has ended, so deck's "am I drawing?" check — cleared synchronously in the finish handler — no longer covers it, and Leaflet never had such a check at all. The popup's own guard is one task wide, which a 300ms-late click clears easily. So the engines now remember, when a session ends, that its closing click may still be in flight, and drop it. Only that gesture's click can be covered: the next gesture opens with a pointerdown, which disarms the guard — and in deck's case cancels the pending click outright. A cancelled session arms it too, since the last vertex click can still be on its way. The two identical deck click/hover handlers move out of the standalone and overlay init paths into one place each, so the check lives at a single site.
The guard covered one trailing click and stood down on the next pointerdown, but the gesture that finishes a drawing is often a double-click: terra-draw commits on the first tap, and the second reaches the engine as a further Leaflet click, or as the onClick deck maps its dblclick recognizer onto a tap interval after that tap's pointerup. Either way it landed as a map click and dismissed the popup the drawing had just opened. Absorb by time instead of by count. A pointerdown inside hammer's tap interval may still be that second tap, so it leaves the window open; a later one is the user's own gesture and closes it. Each pointerup inside the window re-opens it for as long as an engine may take to turn that pointer into a click. The window now also closes on its own, so a session that ended without a click at all cannot leave the guard absorbing, and terra-draw's double-click zoom is held back for as long as the window is open so the same gesture does not zoom the map as well.
The request type said the primary button is rendered last and the secondary first. The card leads with the primary, which is what the docs and the spec already say.
The Leaflet branch subscribed clearOnMapClick straight onto the L.Map, so it
never saw the adapter's guard against the click a drawing ended on: finishing
a drawing deselected whatever feature the user had active. The deck.gl branch
already reached the engine through the addEventListener shim.
The engine reports a click's layer point as plain {x, y}, which L.Bounds would
take for a bounds and throw on, so rebuild it as an L.Point.
There is one popup slot, so map:hidePopup can retract another plugin's popup. Show the guard that keeps a plugin to its own: ask only while its own request is still unresolved.
Leaflet fires a map click for every native click and terra-draw's adapter never stops click propagation, so on the 2D engine each vertex the user placed reached the bus as a map click — dismissing whatever a plugin had open and clearing its selection halfway through a drawing. DeckGLAdapter has always checked the session on its own click path; the Leaflet paths checked only the end-of-draw guard. Check the session on both, and only for clicks: the drawing's own events are emitted through the same wrapper while the session is live.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a core map-anchored popup service driven entirely over the event bus: a plugin sends one serializable
map:showPopuprequest (lat/lng, HTML, up to two labeled buttons) and the core renders, positions, styles, and closes the popup on both map engines. The request's promise stays pending while the popup is open and resolves with the outcome — the core broadcasts nothing on the plugin's behalf. Also fixes the deck.gl adapter's missingmoveemission (drag frames, programmatic jumps, bearing/pitch sync), which the popup's tracking depends on.Closes #298
How it works
map:showPopup— single global popup slot; a fresh request from any plugin replaces the current one. Payload and response are pure JSON (both survive a future sandboxed-pluginpostMessageboundary unchanged). Content HTML is sanitized with DOMPurify (already a dependency) at the bus boundary; button labels render as text, never HTML.{action: 'primary' | 'secondary' | 'dismiss' | 'closed'}— which button was pressed, that the user dismissed via X/click-away, or that the popup was closed without the user acting (replaced,map:hidePopup, mission switch). Invalid requests and mount failures reject. Because the response is point-to-point, the confused-deputy and cross-plugin event-spoofing vectors don't exist — no caller-identity machinery needed, ever. A plugin that wants to announce an outcome emits its own namespaced event afterward with its own authority. (Security analysis and option comparison are in the issue's history; the one sandbox-era note is that the future bridge must exemptmap:showPopupfrom any default request timeout.)map:hidePopup— no payload; retracts the current popup, whose request resolves'closed'.<body>and the service does its own lat/lng→viewport projection eachmoveframe. That structurally solves two things at once: popups can float above the panel layer (#mapisz-index: 0under#modern-content's10), and clicks inside the popup can never reach either engine's click pipeline — no propagation guards needed.--theme-color-secondary(destructive red in USWDS).Decisions to review
map:clicksubscription attaches one tick aftershow(), so no event from the popup's own opening gesture can dismiss it — on either engine's emission order (deck.gl firesfeatureClickbeforeclick; Leaflet the reverse). A genuine later click dismisses at the end of its own task; a popup replaced in that window resolves'closed', so a requester is never left waiting (a gap the original event design had).map:hidePopupexists although the issue originally said closing needs no plugin-facing hide: AOI: move the analyze/cancel popup onto the core popup service #299's "no orphaned popup when the tool closes right after selecting" is unreachable without a retraction path, and any plugin can already blow the popup away viashowPopup, so a naked hide grants no new power.zoomend, because Leaflet suppressesmoveduring its CSS zoom animation and sampling internal pane transforms would couple the core to Leaflet internals.flyTo/pinch/drag track per-frame on both engines; deck.gl tracks through everything. This trades a sliver of the "tracks during zoom" wording for zero engine coupling — flagging for explicit sign-off.group—dialogover-promised with no focus management. Payload options deliberately not taken:placement,closable,disabled, anariaLabelfield, DOMPurifyFORBID_TAGStightening. All trivially addable when a consumer needs them.Follow-ups for #299 (recorded here so they don't get lost)
map:hidePopupbefore itsmoveendwait and track/cancel its 1500 ms fallback timer.map:addOverlay/removeOverlayare now@deprecatedand undocumented; AOI: move the analyze/cancel popup onto the core popup service #299 removes their last plugin consumer (AOI). Full retirement is deferred to a follow-up issue — MapControl's measure-distance label (merged after these branches were cut) still uses them.Verification
tsc --noEmitclean. The 25-case service spec asserts the resolved outcome on every exit path, that each promise settles exactly once, that the service broadcasts nothing, and pins both engines' click-ordering races plus replace/stale-timer regressions (mutation-tested guards). deck.gl camera-event specs cover themovefix.