Core: map-anchored popup service driven over the event bus - #303
Open
CarsonDavis wants to merge 39 commits into
Open
Core: map-anchored popup service driven over the event bus#303CarsonDavis wants to merge 39 commits into
CarsonDavis wants to merge 39 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.
A plugin's bus handle now forwards `request`, stamping the plugin's id on the way through. It travels beside the payload rather than inside it, so a payload of any shape — a string, an array, nothing at all — reaches the provider as it was written, and a request that carries no payload still carries its caller. The id comes from the handle rather than from anything author code fills in, which is what makes it worth reading: a provider can tell who is asking without asking the caller to say so honestly.
There is one popup slot, so `map:hidePopup` was a request to empty it whoever had filled it — a plugin tearing itself down could close another plugin's popup, and the only defence was for every plugin to track whether what was on screen was still its own. Core knows better than the caller does, so let it answer: record who opened a popup and retract it only for them, reporting whether anything was. Ownership is compared exactly, with "no caller" a value of its own, so a caller with no handle can neither reach a plugin's popup nor have its own reached. Showing is unaffected — a request still takes the slot whoever held it, and the evicted popup is still answered 'closed'. Core's own teardown, on a mission switch or a replacement, bypasses ownership as before.
CarsonDavis
marked this pull request as ready for review
August 21, 2026 15:07
Fixes the defects found reviewing the map popup service, and takes two changes asked for while testing it. Card placement and lifecycle: - Clamp the card's bottom edge and cap its height so a tall card's action buttons stay reachable; the body scrolls under a pinned heading and a pinned actions row. - Hide the card only once it no longer overlaps the map, and let it ride off with the map instead of parking against the viewport edge. - Take the dismissal signal from the engine rather than the bus, so only a real map click dismisses a popup. - Reject anchors outside the geographic range, and labels that are blank once trimmed. Title: - An optional `title`, rendered as text on the close control's row and pinned above the body. It names the card for assistive technology when present. Every field but the anchor is now optional, and a request carrying neither a title nor html is rejected. Layering: - The card claims no stacking level and mounts beside the map container, so the panels paint over it. The compact layout's panel regions claim a level above the centre region, which the overlay layout already had. Drawing: - The Leaflet adapter decides once, where clicks are emitted, whether a drawing session owns a click, matching the deck.gl adapter. Tests cover each of these, including branches that previously survived deletion untested: the resize and ResizeObserver reposition paths, the popup teardown on map re-init, the deck.gl click wiring in both modes, the interactive `move` emit, and the draw guard's zoom-restore states.
Plugin content mounts in a shadow root of its own, which is what lets a
card carry a `<style>` and be an author's to style: its rules reach the
card's content and stop there, while the theme's custom properties and
the card's typography still cross inwards. The sanitizer works from an
explicit allow-list rather than DOMPurify's defaults, so markup it does
not name is dropped, and dompurify is pinned exactly — the list is
derived by subtracting from the library's own attribute sets.
Links: every href that goes anywhere, and every SVG xlink:href, opens in
a tab of its own, so following a link in a card never navigates the app
away. A capture-phase guard backs that up.
The card is a dialog: focus moves onto it as it opens, Tab and
Shift+Tab cycle within it — through the plugin's own controls, counting
only the stops a keyboard can actually reach — and focus returns to
whatever held it when the popup closes. Escape closes it. There is no
`aria-modal`, because nothing outside the card is inert.
A card hides by parking off-screen rather than by `visibility`, which
its own content could override, and claims paint containment so nothing
inside it can paint over the app before its first placement lands.
Requests carry per-call settings in an options object — `request(name,
data, { caller })` — so the caller a plugin's handle stamps and the
timeout the sandbox bridge will want stop competing for one positional
slot. The old positional form is refused rather than misread.
Tearing down a tool announces `plugins:destroyed`, and the map answers
by closing whatever popup is open, so a card cannot outlive the plugin
that opened it. A layout re-render destroys every tool without
re-initialising the map, which is the path that left cards stranded.
The draw-click guard covers the click a drawing owes from the moment
that pointer left the map, rather than from whenever the session ended,
so a shape finished with a key still covers the vertex click deck.gl is
holding, while a session ended with the pointer long idle no longer
swallows the user's next click.
A popup outlives the tools around it only when every tool goes at once — a layout re-render destroys them all without re-initialising the map, so the map's own cleanup never runs. `destroyAllTools` says so with `plugins:allDestroyed`, and the map releases the card it holds when it hears it. A single tool unloading no longer takes a card that belongs to somebody else, which is what closing on every teardown did. A plugin that opens a card and does not retract it in `destroy()` leaves it standing after its own unload. The card is the user's to dismiss, and recognising whose card it is would mean reconciling the id a tool is registered under with the one it asks for popups under; closing every card instead is what took a bystander's away. Sanitizing follows the dompurify range again rather than one release of it: the sets a card's allow-list is checked against move with the library, and the spec that re-derives them says so when they do. The release that answers the range today hardens its own forbidden-contents default, which pinning held the app back from. The comments this touches say what the code does. The card paints under the app's panel layer, a card's own buttons are the one control its content selector matches, and deck's `click` is the recognizer that waits — each of those was stated the other way around.
Cut the tests whose regressions another test already catches: scenario permutations merged into single representative specs, guard-timing semantics pinned once instead of per engine, and the two bus-integration specs folded into one file sharing a single harness. Every decision in the PR body keeps a pin, the DOMPurify drift test included.
7 tasks
The card carried five hand-copied lists — tags, HTML, SVG, MathML and namespaced attributes — and a drift test that re-derived one of them from the library's source to keep the copy honest. Upstream curates that answer already, against markup a card's author would never think of, so the lists go and the defaults stand in their place. Two divergences remain. `ADD_TAGS: ['style']` gives back the stylesheet DOMPurify strips whole, which is the point of mounting content in a shadow root, and `FORBID_ATTR` keeps out `popover`/`popovertarget`, the one default-passed capability that promotes content into the browser's top layer, above the app's panels and outside the card's clipping. `FORCE_BODY` stays: it keeps a leading `<style>` where the author wrote it. What this admits is form controls and `<canvas>`. They were held out for UX honesty rather than security — the contract carries no script, so nothing reads a field back — and they now render as inert content, with the capture-phase navigation guard already refusing the submit that would otherwise take the app away. The focus trap counts them, so a Tab cannot leave the dialog through a field a card renders.
This was referenced Aug 27, 2026
CarsonDavis
changed the base branch from
development
to
feature/351-draw-end-clicks
August 27, 2026 16:54
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 over the event bus. A plugin sends one serializable
map:showPopuprequest — a lat/lng, an optional title, optional HTML, up to two labelled buttons — and the core renders, positions, styles and closes the card. The request's own promise resolves with how it closed; nothing is broadcast on the plugin's behalf.Closes #298
Stacked on #351 (
feature/351-draw-end-clicks), which carries the drawing-click guard the popup's click-away dismissal depends on.What this changes
<style>,:hover, animations — without any of it escaping into the rest of the app.Decisions for review
1. Plugin content gets its own shadow root, and may carry
<style>.Authors get
:hover,@keyframesand real formatting freedom inside their card, and the browser enforces the boundary rather than a list we maintain: a stylesheet in a card cannot reach the app, and a plugin'sids cannot shadow core's. Theme tokens and typography still inherit inwards, so a card that styles nothing looks like the app it opened over. The alternative was cutting<style>and giving authors inlinestyle=only, which costs all CSS interactivity.The shadow root replaces the
<style>exclusion, not the sanitizer. Measured:onclickandonerrorboth fire inside a shadow root, and a<form>with a password field mounts and works — shadow DOM is encapsulation, not a sandbox. Sanitizing still matters becausehtmlcarries uploaded geodata, so a place name containingonmouseover=would otherwise execute.2. The sanitizer runs on DOMPurify's defaults, plus two deliberate divergences.
htmlis sanitized with the library's own defaults rather than tag and attribute lists of ours. Two changes sit on top.<style>is added back, because DOMPurify strips it whole and an author's stylesheet is the point of decision 1's shadow root.popoverandpopovertargetare forbidden — the one default-passed capability a card cannot contain, a popover promoting itself into the browser's top layer, above the app's panels and outside the card's clipping.FORCE_BODYkeeps a leading<style>where the author wrote it, the parser otherwise hoisting a document-opening stylesheet into a head DOMPurify does not return.What this admits, against the hand-curated lists it replaces, is form controls and
<canvas>. Those were excluded for UX honesty rather than security: the contract carries no script, so a field's value can never be read back and a canvas has nothing to paint it. They now render as inert content, which is the ordinary answer to markup nobody can wire up — a card built around a<form>comes back whole rather than blank — andsubmitnavigation is already refused by the capture-phase guard that keeps links from taking the app away. The focus trap counts the controls, so a Tab cannot leave the dialog through one. Frames and the SMIL animation elements needed no exclusion of ours at all: the defaults strip them.The trade is that security curation belongs upstream, maintained against markup we would never think of, while what stays here is short enough to read in one screen and is only ever about the shape of a card.
3. The card paints under the app's panels.
No z-index value could express this.
#main-containercarries a filter, which makes it a stacking context, so anything mounted ondocument.bodyis either entirely above it or entirely below it. The card therefore mounts beside the map container and claims no stacking level of its own, and the compact layout raises its panel regions above the map region. Consequence: a card anchored near the map's edge can be partially occluded by a panel, buttons included.4.
request(name, data, { caller })— the third argument is now an options object.calleris stamped by the plugin's own handle and must never be author-settable;timeout, which the sandbox bridge will want, is the author's to choose per call. Positionally those two cannot share a slot without forcing an author to passcallerto reachtimeout. The old positional form now throws rather than being misread as a caller id.5. A full teardown closes the popup; a single tool unloading does not.
Cards were outliving the app's tools with a request promise that never settled — the path that matters is a layout re-render, which destroys every tool without re-initialising the map. Two signals now:
plugins:destroyedstays per-tool, anddestroyAllToolsemitsplugins:allDestroyedonce; the map answers only the second. A full teardown has no bystanders by construction, since every tool is going.The accepted cost: a plugin that opens a card and fails to retract it in
destroy()leaves it standing after its own unload. The card is the user's to dismiss, and it is pinned as the contract with a test.Closing only the owner's card is the obvious alternative, and it cannot be done from here: the teardown event carries the controller's tool id (
AOITool) while the popup's owner is theforPluginid (aoi), and nothing maps the two. That is fixable, and worth doing — the modern controller should mint each tool's handle at load the way the legacy one already does atToolController_.js:489, which would make core own the mapping by construction. Only two tools mint their own handle today (AOI and FetchStats), and both already declare the id as a constant; legacy's lowercase derivation is not enough, sinceFetchStatsToolyieldsfetchstatsagainst a declaredfetch-stats.It is not in this PR because it changes how every modern tool receives its bus handle, and it settles two other things while it is there: the permanently-unreleased
plugin:{id}:getVarsprovider gains a release point, andthis.apibecomes true on the modern path, which the docs already tell authors it is. That is a lifecycle change with its own blast radius, and it should be reviewed as one.6.
dompurifystays on a caret range.An earlier exact pin was reverted: it held deployments off sanitizer hardening that ships in patch releases — 3.4.14 adds
selectedcontentto its forbidden-contents default as explicit mXSS/DoS mitigation. Floating the range is what lets that hardening arrive, and after decision 2 there is nothing of the library's copied into our source for it to drift away from: what a card may hold is whatever the installed DOMPurify allows, deliberately. What the config itself decides is pinned intests/unit/MapPopup_.spec.tsagainst real markup — a<style>survives and stays inside the shadow root,popover/popovertargetdo not survive, a leading stylesheet stays where the author wrote it, and a<form>'s submit is refused at the click.7. This branch amends the plugin-communication ADR.
The sandbox bridge's
requestsignature, and a carve-out exempting long-lived requests from the 5s default timeout —map:showPopupanswers only when the popup closes, which can be minutes. Every future bridge implementer inherits both.8. The popup is a dialog with a focus trap, no click barrier, and no
aria-modal.Focus moves into the card, Tab and Shift+Tab cycle within it, Escape closes it, and focus returns to the opener. Nothing is laid over the rest of the app, so a pointer can still reach everything — which is why there is no
aria-modalclaiming otherwise. Adding a barrier would make it fully modal.9. One global popup slot: anyone may replace, only the owner may retract.
A fresh request from any plugin replaces the current card.
map:hidePopupis owner-gated, so a plugin can call it blind on teardown. The outcome enum conflates the causes of'closed'— replaced, retracted, mission switch, decision 5's teardown — which is what makes the bystander case invisible to the plugin receiving it.10. Leaflet hides the card for its ~250ms zoom animation.
Leaflet suppresses
moveduring the animation and only reports the landing, so tracking per-frame would mean sampling its internal pane transforms and coupling core to Leaflet internals.flyTo, pinch and drag track per-frame on both engines; deck.gl tracks through everything.Known gaps
htmlyet. AOI, the only caller, sends a title and two buttons. So the content contract above — the shadow root, the sanitizer, the link handling — is covered by unit tests and browser probes but is not exercised by anything a user can click. It is a contract for the plugins that come next.ariaLabelon the request. A card takes its accessible name fromtitle; one sending onlyhtmlannounces a generic name.