Core: map-anchored popup service driven over the event bus - #303
Closed
CarsonDavis wants to merge 53 commits into
Closed
CarsonDavis wants to merge 53 commits into
CarsonDavis wants to merge 53 commits into
Conversation
Closed
7 tasks
This was referenced Aug 11, 2026
Closed
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.
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
Accept any longitude — a map panned across the antimeridian projects one past ±180 to the pixel the caller meant — and keep only the latitude gate Mercator actually needs. Follow `moveend` as well as `move`, so a card tracks deck.gl's comparison panes. Send `<area href>` out to its own tab like `<a href>`. Clamp the card to the map rect intersected with the viewport, so it cannot sit under a panel region with its buttons out of reach. Rewrite the stacking and bare-fragment comments to what the platform does, and drop the bus mock MapPopup_ never touches.
Drop the unreachable `getAttributeNS` branch and its namespace constant, unexport a config with no importer, and point the focus-stop doc at the predicate that does the filtering. Say what the anchor and the request fields really require, that `request` rejects rather than throws, and that `addOverlay` is superseded only for card-shaped content — an anchored bare node such as MapControl's measure label stays on it. Refuse an array in the `options` slot the way any other non-object is refused.
Dispatch a window `resize` and assert where the card lands, rather than only that the listener is removed. Assert the deck jump's frames against the camera the caller named, not against the state the adapter emitted. Stop pinning `moveend` on every frame of an interactive gesture — `move` leading is what an anchored consumer needs — and stand a prefixed recorder where the caller-stamp spec asserted against nothing. Drop the modern.js scraper and keep the compact layout's half as a stylesheet tripwire that throws when a selector it reads is gone.
…-service # Conflicts: # src/essence/Basics/MapEngines/IMapEngine.ts
Cap the card's height to the visible map before measuring it, so a card taller than the map scrolls its body instead of hanging past the bottom edge with its actions row under a panel. Pin both clamps to the near edge when the map has less room than the card needs, rather than inverting and pushing the card off the opposite edge.
Point both adapters' addOverlay at IMapEngine.addOverlay, which says an anchored bare node stays there, instead of sending every caller to the popup provider. Drop forPlugin's claim that a provider may refuse an anonymous caller; none does.
A cap below the card's own chrome clipped the actions row away, so a sliver of visible map now leaves the card uncapped, and the maxHeight write is skipped when it would set the value the card already has. Corrects the narrow-map test's comment to the real tie-break reason and drops an assertion that restated the sizing stub's arithmetic.
…o feature/298-map-popup-service
Collaborator
Author
|
Superseded. The popup service was rebuilt as a stack of smaller PRs off #352: #417 (FetchStats subscribes once), #418 (adapter camera events and feature on clicks), #419 (bus handles, caller stamp, release), #422 (tool identity), #423 (the popup service), #424 (AOI onto the popup), plus #416 (cleanup off development). This branch stays for reference. |
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.
A plugin can open a popup pinned to a spot on the map by sending one plain-data request, and core renders it, keeps it glued to that spot, sanitizes what is inside it, closes it, and reports back how it closed.
Closes #298
A plugin opens a map popup by sending data, not a component
Previously, a plugin author who wanted a little card floating over a point on the map had to build the card. They asked core for an overlay and handed it a mount function — a live JavaScript function that receives an empty DOM node and returns a cleanup function — then filled that node themselves. So every plugin that wanted a popup shipped its own markup, its own CSS, and its own bookkeeping for showing and hiding it, and no two looked quite the same.
The reason that hurts is that a function is not data. It cannot be written down and sent somewhere; it only works because the plugin is running in the same page as core, sharing the same DOM. The moment a plugin lives behind a sandbox boundary, a mount function cannot cross it, and neither can the popup.
The fix: a plugin now asks the bus for a popup with a lat/lng, an optional title, optional HTML, and up to two button labels — all of it plain JSON — and holds the promise it gets back. Core owns the card from there. The promise stays pending while the card is up and then tells the plugin exactly how it ended: which button was pressed, that the user dismissed it, or that it was closed out from under them. Nothing about a popup travels over the bus as an event, so one plugin's popup can never trip another plugin's handlers.
Demo mission: no. Nothing in the app calls the service yet — moving the AOI tool onto it is #309. You can drive it by hand from the browser console through
window.mmgisAPI.The card is a core-styled dialog that stays glued to its anchor
Previously, the app's one popup was the AOI tool's, and it looked like it: styled by AOI, no close button, no way to shut it with the keyboard, no way to reach its buttons by Tab. It was also centred on its anchor point rather than placed above it, it froze in place while you dragged the deck.gl map and jumped to catch up when you let go, and it carried its own timer to hide the flicker that caused.
The fix: the card is core's now, drawn from the same theme tokens as the rest of the app. It has an X, a filled primary button and an outlined secondary one, and a single button gets the primary look whichever slot it arrived in. It announces itself as a dialog, takes focus when it opens, keeps Tab inside itself, closes on Escape, and hands focus back where it found it. Position-wise it sits above its anchor, flips below when it would clip off the top of the map, and clamps to the visible map rather than the browser window, so it never slides under a side panel. If it is taller than the map it caps its height and scrolls its contents so the buttons stay reachable. It hides for the length of a Leaflet zoom animation, and if the anchor itself pans off the map the card simply stops clamping and rides out with it, parking itself only once its own box has fully left the visible map and coming back when you pan back.
Demo mission: no, not until a plugin opens one.
What a plugin puts inside a card is sanitized and boxed in
Previously, nothing in MMGIS cleaned HTML that came from a plugin. That was survivable while every plugin was code we wrote, in this repo. It stops being survivable the moment a card's contents can come from a plugin somebody else published: HTML dropped straight into core's DOM is a script-injection hole, and a stylesheet dropped there leaks into the whole app.
The fix: the body of a card goes through DOMPurify before it is rendered, and then mounts inside its own shadow root — a small walled-off piece of DOM, so an author's stylesheet reaches their card and stops at its edge. On top of that, form controls render inert and a submit is refused, and every link or image-map area that points anywhere opens in a new tab, so a card can never navigate the app out from under the user.
Demo mission: no.
Every way a card can close answers the plugin, and only its opener can retract it
Previously, there was nothing to answer, because there was no service — the AOI tool hand-rolled its own show and hide, nothing cleaned up an overlay when the mission switched or the layout tore down, and a plugin could remove any overlay on the map, including one it did not open.
The fix: there is one popup slot, and everything that ends a card resolves its request — a button, the X, Escape, a click on empty map, a replacement card from any plugin, an explicit retraction, a mission re-init, a full layout teardown. Retraction is scoped to whoever opened the card, which means the bus now has to know who is asking. A plugin's scoped handle gained a
requestmethod that stamps the plugin's id onto the call, and providers are handed that caller alongside the data. So a plugin can call retract blind in its own cleanup and it will only ever close its own card; anyone who is not the open card's owner getsfalseback instead. Callers that arrive with no id all share one anonymous identity, so one anonymous caller can still retract another's card. Two new signals, one per plugin destroyed and one after a whole teardown, let core drop a stranded card without the tool controller reaching into the popup directly.Demo mission: the teardown signals fire on any layout re-render, but nothing is visible unless a plugin is holding a card.
On deck.gl, anything anchored to the map follows a drag instead of snapping at the end
Previously, on the deck.gl engine, you drew an AOI and dragged the map, and its Analyze/Cancel card sat still until you let go, then jumped to the shape. The adapter only told the bus the camera had moved once the move was over, so
map:movenever fired at all on that engine, and a programmatic jump with no animation reported nothing whatsoever.The fix: the adapter now reports the camera continuously, from the basemap handler and from every view-state change, and a zero-duration programmatic jump reports a move followed by a move-end like any other. Anchored content gets a position per frame and tracks the drag.
Demo mission: yes, and it is the one thing here you can see today. Draw an AOI on the demo, then drag or zoom the map: the existing card stays glued to the shape.
In the compact layout, panels paint above the map
Previously, in the compact grid layout, the centre map cell claimed a stacking level and the four panel regions around it claimed none, so anything drawn in the centre region could paint on top of a panel. Nothing exercised that until now, because nothing floated near the map's edge.
The fix: the card is mounted next to the map container rather than at the top of the page, which puts it inside the centre region and squarely into that bug. The four panel regions are now positioned and given a stacking level above the map, so a card near the map's edge slides under a panel instead of over it.
Demo mission: partly. The demo uses the compact layout, so it is worth confirming the panels still overlap the map correctly, but the difference only shows with a card up.
Decisions to review
forPlugin. Only that id can retract it. The other design is a claim ticket: the open call returns an unguessable handle, and closing means presenting it. That needs no notion of identity on the bus and can't be spoofed. As built, only AOI, FetchStats and the classic controller actually pass an id. The shared React adapters pass nothing, so those plugins all look like one anonymous caller, and one closing "its" card can close another's. The id is also just a string the plugin claims, so it is forgeable until plugins run in a sandbox.dismissmeans the user did it: the X, Escape, or a click on the map.closedmeans code did it: a new request replaced the card, the plugin retracted it, or the map was torn down. A map click schedules a dismissal one task later. If the plugin opens its replacement card synchronously in the click handler, the old card reportsclosed. If the plugin awaits a network call first, the dismissal fires before the new card exists, and the old one reportsdismiss. Same picture on screen, different answer to the plugin, decided by the plugin's own latency. A deterministic fix is to carry whether the click landed on a feature and not treat feature clicks as dismissals.<style>tags it would otherwise strip. The issue said plugins should supply zero styling. A plugin's stylesheet can only reach the plugin's own markup, not the card's title, buttons or frame. What does cross into the shadow root is the theme's CSS variables and inherited typography, so those token names become the contract plugins depend on. That should be documented as such.aria-modaland puts up no click barrier. A keyboard user can only leave with Escape or a button, while a mouse user can click anywhere and keep working. The consistent options are a card that doesn't trap Tab at all, or a real modal with a barrier over the app.moveendalready fired on every frame of a drag before this PR. This PR adds a per-framemovebeside it, and a syntheticmoveplusmoveendforsetView,setZoomandfitBounds. The per-frame dispatch is cheap; the cost is the listeners. Two dynamic-extent vector layers refetch on everymoveend, and they now also refetch on every programmatic fit, including AOI's own fit after a selection. The popup anchor is the only thing that needs per-frame updates and it listens to the engine directly, not the bus. The fix is to emitmoveendonly when the camera comes to rest, matching Leaflet.