mirror of
https://github.com/renorris/openfsd
synced 2026-08-13 04:55:42 +08:00
airport-editor: select-mode drag status tip and design note
Surface selection shows a short tip to drag handles. Promote the vertex-drag design doc and note rail-only vertex delete vs Del deleting the surface.
This commit is contained in:
Reese Norris
991
docs/design/airport-editor-vertex-drag.md
Normal file
991
docs/design/airport-editor-vertex-drag.md
Normal file
@@ -0,0 +1,991 @@
|
||||
# Airport Editor: Reliable Vertex Drag (Bugfix + UX Polish)
|
||||
|
||||
| Field | Value |
|
||||
|-------|--------|
|
||||
| **Document** | Reliable vertex drag for `/airport-editor` polylines and points |
|
||||
| **Author** | _(design author / implementer)_ |
|
||||
| **Date** | 2026-07-27 |
|
||||
| **Status** | **Implemented** (PR1: live geometry / handles / click suppress; PR2: select-mode status tip + this design note) |
|
||||
| **Parent design** | [`docs/design/apt-air-editor.md`](apt-air-editor.md) (Implemented) |
|
||||
| **Scope** | Bugfix + minimal UX polish — **not** a GIS rewrite |
|
||||
|
||||
---
|
||||
|
||||
## Overview
|
||||
|
||||
> **Closeout (2026-07-27):** Code for the fix landed in `map-layers.js` / `main.js` / `airport-editor.css` with pure tests under `webjs/airport-editor/`. Select-mode surface selection flashes a one-line tip to drag white handles. Design history below is retained as the engineering record (root causes, state machine, checklist).
|
||||
|
||||
The airport editor’s **vertex drag** path was partially wired but failed as a product experience. Users could select a runway/taxiway/hold/parking surface and see vertex chrome, yet **dragging nodes did not feel like it worked**: the handle may move while the underlying geometry freezes, hit targets are small and visually double-rendered, and drag-end can race with map-click deselection and a full overlay rebuild.
|
||||
|
||||
This design is a **focused fix** in the existing Leaflet PE stack:
|
||||
|
||||
1. Make **select → drag vertex** rock solid for multi-point polylines and single-point parking/hold.
|
||||
2. **Live-update** the surface layer while the handle moves (parent design already required this).
|
||||
3. Slightly **simplify interaction feel** (larger targets, no dual handles, robust drag-end) without new modes or libraries.
|
||||
4. Keep draw tools, rail inspector, insert/delete vertex, aircraft drag, Blob download, dirty tracking, and boring-web constraints intact.
|
||||
|
||||
Primary touch surfaces: `map-layers.js` (`OverlayController`), `main.js` (drag callbacks / refresh lifecycle), `airport-editor.css`, and pure Node tests under `webjs/airport-editor/`.
|
||||
|
||||
---
|
||||
|
||||
## Background & Motivation
|
||||
|
||||
### Current architecture (relevant slice)
|
||||
|
||||
```mermaid
|
||||
flowchart LR
|
||||
User["User pointer"] --> Map["Leaflet map"]
|
||||
Map --> OC["OverlayController<br/>map-layers.js"]
|
||||
OC -->|"onSelect / onVertexDrag*"| Main["main.js"]
|
||||
Main -->|"setVertex / refresh"| Model["model.js document"]
|
||||
Main -->|"overlays.render"| OC
|
||||
Main -->|"rail.render"| Rail["ui-rail.js"]
|
||||
```
|
||||
|
||||
| Piece | Path | Role today |
|
||||
|-------|------|------------|
|
||||
| Overlay render + handles | `internal/web/static/js/openfsd/airport-editor/map-layers.js` | `OverlayController.render`, `_addVertexHandles`, drag listeners |
|
||||
| Edit loop | `…/main.js` | `onVertexDrag` / `onVertexDragEnd` → `setVertex` → `afterAptMutation` → `refresh` |
|
||||
| Document mutations | `…/model.js` | `setVertex`, `insertVertex`, `deleteVertex` (pure, tested) |
|
||||
| Handle CSS | `internal/web/static/css/openfsd/airport-editor.css` | `.apted-vertex-handle` 12×12 + margins |
|
||||
| Pure tests | `webjs/airport-editor/map-layers.test.js`, `model.test.js` | No real Leaflet drag simulation |
|
||||
|
||||
Parent design (`apt-air-editor.md`) already specified:
|
||||
|
||||
- **Move vertex** = select surface → drag handle (Leaflet marker).
|
||||
- **Performance**: *“avoid full map rebuild on single vertex drag (update that latlng only).”*
|
||||
- **Delete vertex** (parent table): “Select handle + Delete key” — **not implemented**; current `main.js` Del/Backspace deletes the **whole surface**. This fix does not close that product gap; vertex delete stays rail-only (see Open Questions / Known gaps).
|
||||
|
||||
Implementation shipped the select/handle scaffolding but **did not complete live geometry update**, and several Leaflet interaction details undermine reliability.
|
||||
|
||||
### Verified root causes (from source — not the suspect list alone)
|
||||
|
||||
#### RC1 — Live polyline/point update is missing (confirmed)
|
||||
|
||||
```115:122:internal/web/static/js/openfsd/airport-editor/main.js
|
||||
onVertexDrag(si, vi, lat, lon) {
|
||||
setVertex(doc, si, vi, { lat, lon });
|
||||
// Live-update surface polyline without full refresh of handles mid-drag.
|
||||
// Full refresh on dragend.
|
||||
},
|
||||
onVertexDragEnd(si, vi, lat, lon) {
|
||||
setVertex(doc, si, vi, { lat, lon });
|
||||
afterAptMutation();
|
||||
```
|
||||
|
||||
- Comment promises live update; **code only mutates the model**.
|
||||
- Leaflet moves the **handle marker** on drag, so something on screen moves — but `_addSurface`’s `L.polyline` / parking `L.circleMarker` is **not** updated until `onVertexDragEnd` → `afterAptMutation` → `refresh` → full `overlays.render`.
|
||||
- Aircraft drag works better because the aircraft **is** the draggable marker; surface geometry is a **separate** path layer under the handle.
|
||||
- Severity: **High** — primary “drag doesn’t work” feel (rubber-band line frozen; parking disc stuck while ghost handle floats).
|
||||
|
||||
#### RC2 — Dual handle construction (visual + interaction confusion)
|
||||
|
||||
```555:599:internal/web/static/js/openfsd/airport-editor/map-layers.js
|
||||
_addVertexHandles(surface, surfaceIndex) {
|
||||
// ...
|
||||
const marker = L.circleMarker([p.lat, p.lon], { radius: 6, /* ... */ });
|
||||
const handle = L.marker([p.lat, p.lon], {
|
||||
draggable: true,
|
||||
zIndexOffset: 2000,
|
||||
icon: L.divIcon({
|
||||
className: 'apted-vertex-handle',
|
||||
iconSize: [12, 12],
|
||||
iconAnchor: [6, 6],
|
||||
}),
|
||||
// ...
|
||||
});
|
||||
// dragstart / drag / dragend on handle only
|
||||
handle.addTo(this.vertexGroup);
|
||||
// Keep circle under for visibility if divIcon fails in tests.
|
||||
marker.addTo(this.vertexGroup);
|
||||
```
|
||||
|
||||
- Two layers per vertex: non-draggable `circleMarker` (overlay pane) + draggable `divIcon` marker (marker pane).
|
||||
- Marker pane is above overlay pane (`leaflet.css` z-index 600 vs 400), so the circle usually does **not** steal pointer events — but the user sees **two dots** (or a thick composite), and the intentional “keep circle under” comment is misleading (add order does not change pane stacking).
|
||||
- Severity: **Medium** — confuses targeting; doubles layer churn; zero benefit in production.
|
||||
|
||||
#### RC3 — CSS double-offset on the hit target
|
||||
|
||||
```256:267:internal/web/static/css/openfsd/airport-editor.css
|
||||
.apted-vertex-handle {
|
||||
width: 12px !important;
|
||||
height: 12px !important;
|
||||
margin-left: -6px !important;
|
||||
margin-top: -6px !important;
|
||||
border: 2px solid #4a7ab0;
|
||||
/* ... */
|
||||
cursor: move;
|
||||
}
|
||||
```
|
||||
|
||||
- Leaflet already positions via `iconAnchor: [6, 6]` on a 12×12 icon.
|
||||
- Extra `margin-left/top: -6px` shifts the **visual** handle northwest of the true lat/lon while the drag anchor remains on the vertex.
|
||||
- Result: handle appears off-vertex; easy to miss the interactive box; looks “broken” even when drag technically starts.
|
||||
- Severity: **High** for perceived accuracy / grab reliability — **must ship with the live-geometry fix**, not a follow-up PR (see PR Plan).
|
||||
|
||||
#### RC4 — Full `render` on dragend rebuilds everything (including handles)
|
||||
|
||||
```412:432:internal/web/static/js/openfsd/airport-editor/map-layers.js
|
||||
render(airport, aircraft, selection = null) {
|
||||
// ...
|
||||
this.group.clearLayers();
|
||||
this.vertexGroup.clearLayers();
|
||||
// ... re-add all surfaces, aircraft, then handles if surface selected
|
||||
}
|
||||
```
|
||||
|
||||
- Mid-drag: no full refresh (good) — and **must stay that way** via caller contract (`onVertexDrag` never refreshes).
|
||||
- Dragend: `afterAptMutation` → `refresh` clears **all** layers and rebuilds — **required and allowed** once the pointer is up (`_dragging === false`).
|
||||
- Parent performance note already forbids full rebuild *during* drag; live path should use `_layerByKey` + `setLatLngs` / `setLatLng`.
|
||||
- Severity: **Medium** for post-drag jank; **High** if anything calls `refresh` while the pointer is still down (`_dragging === true`).
|
||||
|
||||
#### RC5 — Map click vs drag-end race (intermittent deselect risk)
|
||||
|
||||
```392:397:internal/web/static/js/openfsd/airport-editor/map-layers.js
|
||||
map.on('click', (ev) => {
|
||||
if (this._dragging) return;
|
||||
// ...
|
||||
});
|
||||
```
|
||||
|
||||
```588:594:internal/web/static/js/openfsd/airport-editor/map-layers.js
|
||||
handle.on('dragend', (ev) => {
|
||||
// ...
|
||||
setTimeout(() => {
|
||||
this._dragging = false;
|
||||
}, 50);
|
||||
});
|
||||
```
|
||||
|
||||
```137:142:internal/web/static/js/openfsd/airport-editor/main.js
|
||||
if (doc.mode === MODE_SELECT) {
|
||||
if (doc.selection) {
|
||||
doc.selection = null;
|
||||
refresh();
|
||||
}
|
||||
```
|
||||
|
||||
- Select mode: empty-map click clears selection.
|
||||
- Authors already left a **50ms** `_dragging` hold after `dragend` — evidence they anticipated residual clicks, not a measured production failure rate.
|
||||
- On **desktop**, `click` often does **not** fire when mousedown target (handle) differs from mouseup target; Leaflet’s own map-drag click suppression does **not** automatically cover **marker** drag. Residual deselect is therefore an **intermittent** risk (touch / synthesized clicks / browser quirks), not a proven always-on bug from source alone.
|
||||
- Severity: **Medium** (defensive hardening). Extending to ~250ms via a dedicated `dragEndedAt` timeline (separate from pointer-down `_dragging`) + `bubblingMouseEvents: false` + stopPropagation is the mitigation. Old mobile ~300ms synthetic click is the upper-bound rationale for the 250–400ms band (Open Question 1).
|
||||
|
||||
#### RC6 — Vertices only after selection (by design, but two-step)
|
||||
|
||||
```428:432:internal/web/static/js/openfsd/airport-editor/map-layers.js
|
||||
if (this.editable && this._selection?.type === 'surface') {
|
||||
const s = surfaces[this._selection.index];
|
||||
if (s) this._addVertexHandles(s, this._selection.index);
|
||||
}
|
||||
```
|
||||
|
||||
- Matches parent UX table (“Select surface → drag handle”).
|
||||
- User ask (“point/click/drag existing nodes on any visible polyline”) can be read as wanting **zero-step** grab-anywhere-vertices. That is a larger product change (clutter at 200 surfaces).
|
||||
- This design: **keep select-then-drag**, make it obvious and reliable; optional light polish only (status tip on select). Always-on vertices for every surface is a **non-goal**.
|
||||
|
||||
#### RC7 — Small hit target (12×12)
|
||||
|
||||
- 12 CSS px is below common 24–44px target guidelines; border eats interior.
|
||||
- Severity: **Low–Medium** alone; stacks with RC3. Ship together with CSS alignment in the same fix PR.
|
||||
|
||||
#### RC8 — `editable` flag
|
||||
|
||||
- `this.editable = opts.editable !== false` in `OverlayController` constructor; `main.js` does not pass `editable: false`.
|
||||
- **Not** a current bug. Keep flag for tests/read-only demos.
|
||||
|
||||
### Pain points (user-facing)
|
||||
|
||||
1. Select a taxiway/runway → handles appear → drag → line does not follow.
|
||||
2. Handles look misaligned / doubled; hard to grab.
|
||||
3. Sometimes after release, selection/handles disappear (intermittent).
|
||||
4. Interaction feels “mode-heavy” even though Select is already the right mode — friction is broken feedback, not missing modes.
|
||||
|
||||
---
|
||||
|
||||
## Goals & Non-Goals
|
||||
|
||||
### Goals
|
||||
|
||||
1. **Reliable vertex drag** for selected surfaces:
|
||||
- Multi-point: `RUNWAY`, `TAXIWAY`, multi-point `HOLD` polylines.
|
||||
- Single-point: `PARKING` and single-point `HOLD` (circleMarker geometry follows).
|
||||
2. **Live geometry**: while dragging a handle, the corresponding surface layer tracks the pointer (no frozen rubber band).
|
||||
3. **Stable drag-end**: model committed, dirty/validation/titlebar updated, selection **retained**, handles remain for further edits; full `render` **does** run on dragend.
|
||||
4. **Simpler feel** without new tools: single handle per vertex, larger hit target, correct alignment, status copy that names the select→drag flow.
|
||||
5. **Preserve** draw modes, select mode, rail inspector (insert/delete vertex, props), aircraft drag, Blob download, dirty hash / `beforeunload`, server validate, import-graph and package layout.
|
||||
6. **Tests**: pure Node tests for extractable logic (including live-layer apply helper + suppress matrix + handle-options symmetry); documented **manual smoke** for Leaflet drag. No Playwright/Cypress by default (house policy).
|
||||
7. **Scope small**: fix `map-layers.js` + `main.js` (+ CSS/tests) in a **user-complete** fix PR; no new packages, no leaflet-draw.
|
||||
|
||||
### Non-Goals
|
||||
|
||||
- Always-visible vertices on every unselected surface / “grab any node without selecting.”
|
||||
- Mid-segment insert handles on the map (rail “Add vertex” remains).
|
||||
- Snap-to-grid, snap-to-nearby-vertex, topology merge of taxi intersections.
|
||||
- Vendoring `leaflet-draw` / `leaflet-geoman` / any new map framework.
|
||||
- Touch-first mobile authoring optimization.
|
||||
- Browser automation suite (Playwright etc.) unless product owner overrides house policy after complexity-gate write-up.
|
||||
- Server-side persistence of APT/AIR.
|
||||
- Changing wire format (`pkg/twrfiles`) or Go validation semantics.
|
||||
- Reworking draw-tool click/dblclick finish.
|
||||
- **Vertex-level Delete key** (parent table claim); stays rail-only for this fix.
|
||||
|
||||
---
|
||||
|
||||
## Key Decisions
|
||||
|
||||
| Decision | Choice | Rationale |
|
||||
|----------|--------|-----------|
|
||||
| **K1. Interaction model** | Keep **select surface → drag handles**; do not show all vertices always | Matches parent design; avoids clutter at sweatbox-scale airports (~200 surfaces); broken feedback is the real bug |
|
||||
| **K2. Live update ownership** | Overlay **paints first** from handle latlng (order-independent), then `onVertexDrag` updates model; no full `render` mid-drag | Encapsulates Leaflet; model remains source of truth on commit |
|
||||
| **K3. Single handle widget** | One `L.marker` + `divIcon` per vertex; **remove** dual `circleMarker` | Eliminates double-dot UX; marker pane already correct for drag |
|
||||
| **K4. CSS positioning** | Size via `iconSize` / `iconAnchor` only; **no** compensating negative margins; ship in same PR as live geometry | Fixes high-severity RC3 grab misalignment with the rest of the fix |
|
||||
| **K5. Drag-end state machine** | `_dragging` = pointer-down only; clear **before** `onVertexDragEnd`; post-drag map-click shield is **`dragEndedAt` only** via `shouldSuppressMapClick` | Full `refresh`/`render` after dragend must not be blocked; do not overload `_dragging` as the post-drag suppress flag |
|
||||
| **K6. No new library** | Stay on in-tree Leaflet only | Parent design + complexity gate; custom handles are enough once live update works |
|
||||
| **K7. Testing strategy** | Pure helpers (incl. mock-layer apply + handle options) + manual smoke; no Playwright | House default; drag is inherently browser/Leaflet |
|
||||
| **K8. Parking/hold points** | Same vertex-handle path; surface layers are polyline **or** circleMarker only | One code path; explicit layer-type contract |
|
||||
| **K9. Mid-drag render** | **Hard invariant**: callers must not `render`/`refresh` while `_dragging`; **omit** soft early-out | Simpler than cache-only early-out; `onVertexDrag` already forbids refresh; avoids dropping `_selection` |
|
||||
|
||||
---
|
||||
|
||||
## Proposed Design
|
||||
|
||||
### Target interaction (happy path)
|
||||
|
||||
```mermaid
|
||||
sequenceDiagram
|
||||
participant U as User
|
||||
participant M as Map / OverlayController
|
||||
participant App as main.js
|
||||
participant Doc as model (doc)
|
||||
|
||||
U->>M: Click polyline (select mode)
|
||||
M->>App: onSelect(surface i)
|
||||
App->>Doc: selection = surface i
|
||||
App->>M: render(..., selection)
|
||||
M-->>U: Highlight + vertex handles
|
||||
|
||||
U->>M: Pointer down on handle + drag
|
||||
M->>M: _dragging = true
|
||||
loop each drag event
|
||||
M->>M: paint setLatLngs/setLatLng (overlay-local pts)
|
||||
M->>App: onVertexDrag(i, vi, lat, lon)
|
||||
App->>Doc: setVertex (aptDirty) — no refresh
|
||||
end
|
||||
U->>M: Pointer up (dragend)
|
||||
M->>M: _dragEndedAt = now; _dragging = false
|
||||
M->>App: onVertexDragEnd(...)
|
||||
App->>Doc: setVertex
|
||||
App->>App: afterAptMutation → validate + refresh
|
||||
Note over M: Full render runs because _dragging is false
|
||||
M-->>U: Geometry committed; handles still present
|
||||
Note over M: Map click suppressed via dragEndedAt<br/>for MAP_CLICK_SUPPRESS_MS only
|
||||
```
|
||||
|
||||
### Normative drag / click / render state machine
|
||||
|
||||
**Single contract — implement exactly this; do not mix with alternate suppress schemes.**
|
||||
|
||||
| Field | Meaning |
|
||||
|-------|---------|
|
||||
| `_dragging` | `true` only while a vertex (or aircraft) **pointer drag is active** (after `dragstart`, until `dragend` clears it). |
|
||||
| `_dragEndedAt` | `number` ms timestamp of last dragend, or `0` if never / reset on next dragstart. Used **only** for map-click suppress. |
|
||||
| `MAP_CLICK_SUPPRESS_MS` | Default `250`. Duration after `_dragEndedAt` during which map clicks are ignored. |
|
||||
|
||||
**Transitions:**
|
||||
|
||||
1. **`dragstart`**: `_dragging = true`; `_dragEndedAt = 0`.
|
||||
2. **`drag`**: live-paint surface layer; call `onVertexDrag` (model only — **no** `refresh`).
|
||||
3. **`dragend`** (order is normative):
|
||||
1. Read final latlng from handle.
|
||||
2. `_dragEndedAt = Date.now()`.
|
||||
3. `_dragging = false` ← **before** app callback so full rebuild is allowed.
|
||||
4. `onVertexDragEnd(si, vi, lat, lon)` → `setVertex` + `afterAptMutation` → `refresh` → `overlays.render` **must proceed**.
|
||||
4. **Map `click`**: only gate is `shouldSuppressMapClick({ dragging: this._dragging, dragEndedAt: this._dragEndedAt, suppressMs: MAP_CLICK_SUPPRESS_MS }, Date.now())`. Do **not** keep `_dragging` true after pointer-up to “cover” residual clicks.
|
||||
5. **`render`**: full rebuild always when called. Callers **must not** invoke `render`/`refresh` while `_dragging === true`. No soft early-out (see §6).
|
||||
|
||||
```mermaid
|
||||
stateDiagram-v2
|
||||
[*] --> Idle
|
||||
Idle --> Dragging: dragstart\n_dragging=true
|
||||
Dragging --> Idle: dragend\n_dragEndedAt=now\n_dragging=false\nthen onVertexDragEnd
|
||||
note right of Idle
|
||||
Map click suppressed if
|
||||
now - _dragEndedAt < suppressMs
|
||||
Full render always OK
|
||||
end note
|
||||
note right of Dragging
|
||||
Live setLatLngs only
|
||||
No refresh/render
|
||||
end note
|
||||
```
|
||||
|
||||
### 1. Fix handle rendering (`_addVertexHandles`)
|
||||
|
||||
**Remove** the non-draggable `circleMarker` companion. Export pure options for tests:
|
||||
|
||||
```js
|
||||
/** Keep in sync with .apted-vertex-handle width/height in airport-editor.css */
|
||||
export const VERTEX_HANDLE_PX = 16;
|
||||
|
||||
/**
|
||||
* Pure: options for the vertex L.marker (no Leaflet instance required).
|
||||
* @param {number} vertexIndex
|
||||
* @returns {object}
|
||||
*/
|
||||
export function buildVertexHandleOptions(vertexIndex) {
|
||||
const px = VERTEX_HANDLE_PX;
|
||||
return {
|
||||
draggable: true,
|
||||
autoPan: false,
|
||||
keyboard: false,
|
||||
zIndexOffset: 2000,
|
||||
bubblingMouseEvents: false,
|
||||
title: `Vertex ${vertexIndex + 1}`,
|
||||
// icon constructed at call site with L.divIcon({...buildVertexHandleIconOptions()})
|
||||
};
|
||||
}
|
||||
|
||||
/** Pure icon size/anchor for divIcon — symmetry asserted in unit tests. */
|
||||
export function buildVertexHandleIconOptions() {
|
||||
const px = VERTEX_HANDLE_PX;
|
||||
return {
|
||||
className: 'apted-vertex-handle leaflet-interactive',
|
||||
iconSize: [px, px],
|
||||
iconAnchor: [px / 2, px / 2],
|
||||
};
|
||||
}
|
||||
```
|
||||
|
||||
Normative construction:
|
||||
|
||||
```js
|
||||
const handle = L.marker([p.lat, p.lon], {
|
||||
...buildVertexHandleOptions(vi),
|
||||
icon: L.divIcon(buildVertexHandleIconOptions()),
|
||||
});
|
||||
// Only this handle — no companion circleMarker.
|
||||
handle.addTo(this.vertexGroup);
|
||||
```
|
||||
|
||||
Notes:
|
||||
|
||||
- Explicit `leaflet-interactive` is belt-and-suspenders with Leaflet’s marker interactive default (`pointer-events` rules in `leaflet.css` lines 242–256).
|
||||
- `autoPan: false` prevents surprising map pans when dragging near edges during precise geometry work.
|
||||
- **No dual handle.** Regression tests assert `buildVertexHandleIconOptions` has symmetric `iconSize`/`iconAnchor` and that handle construction no longer adds a second layer factory (see Testing).
|
||||
|
||||
### 2. Live geometry update (core fix)
|
||||
|
||||
#### Layer-type contract (normative)
|
||||
|
||||
Surfaces registered under `_layerByKey` key `surface:${index}` are **exactly one of**:
|
||||
|
||||
| Kind / points | Layer type | Live API |
|
||||
|---------------|------------|----------|
|
||||
| Multi-point runway / taxiway / hold | `L.polyline` | `setLatLngs(latlngs)` |
|
||||
| Parking, or hold with 1 finite point | `L.circleMarker` | `setLatLng(latlng)` |
|
||||
|
||||
They must **not** be wrapped in a `FeatureGroup`, `L.marker`, or other container. A future refactor that wraps surface layers would silently break live update if this contract is ignored — preserve the flat registration in `_addSurface`.
|
||||
|
||||
#### Pure helpers
|
||||
|
||||
```js
|
||||
/**
|
||||
* Pure: points → Leaflet latlng tuples (finite only).
|
||||
* @param {{lat:number,lon:number}[]} points
|
||||
* @returns {Array<[number, number]>}
|
||||
*/
|
||||
export function pointsToLatLngs(points) {
|
||||
const out = [];
|
||||
if (!Array.isArray(points)) return out;
|
||||
for (const p of points) {
|
||||
if (Number.isFinite(p?.lat) && Number.isFinite(p?.lon)) {
|
||||
out.push([p.lat, p.lon]);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pure: apply latlngs to a surface layer duck-typed like Leaflet polyline/circleMarker.
|
||||
* Prefer setLatLngs (polyline) then setLatLng (circleMarker). No-op if neither.
|
||||
* @param {{ setLatLngs?: Function, setLatLng?: Function }|null|undefined} layer
|
||||
* @param {Array<[number, number]>} latlngs
|
||||
* @returns {'polyline'|'point'|'none'}
|
||||
*/
|
||||
export function applySurfaceLatLngs(layer, latlngs) {
|
||||
if (!layer || !latlngs || latlngs.length === 0) return 'none';
|
||||
if (typeof layer.setLatLngs === 'function') {
|
||||
layer.setLatLngs(latlngs);
|
||||
return 'polyline';
|
||||
}
|
||||
if (typeof layer.setLatLng === 'function') {
|
||||
layer.setLatLng(latlngs[0]);
|
||||
return 'point';
|
||||
}
|
||||
return 'none';
|
||||
}
|
||||
|
||||
/**
|
||||
* Pure: patch one vertex in a points array (immutable-style new array).
|
||||
* @param {{lat:number,lon:number}[]} points
|
||||
* @param {number} vertexIndex
|
||||
* @param {number} lat
|
||||
* @param {number} lon
|
||||
*/
|
||||
export function patchVertexPoints(points, vertexIndex, lat, lon) {
|
||||
const src = Array.isArray(points) ? points : [];
|
||||
return src.map((p, i) =>
|
||||
i === vertexIndex ? { lat, lon } : { lat: p.lat, lon: p.lon },
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
Instance method:
|
||||
|
||||
```js
|
||||
_liveSetSurfacePoints(surfaceIndex, points) {
|
||||
const layer = this._layerByKey.get(`surface:${surfaceIndex}`);
|
||||
applySurfaceLatLngs(layer, pointsToLatLngs(points));
|
||||
}
|
||||
```
|
||||
|
||||
#### Normative drag handlers (order-independent paint — **only** this path)
|
||||
|
||||
```js
|
||||
handle.on('dragstart', (ev) => {
|
||||
this._dragging = true;
|
||||
this._dragEndedAt = 0;
|
||||
if (ev?.originalEvent) this.L.DomEvent.stopPropagation(ev.originalEvent);
|
||||
});
|
||||
|
||||
handle.on('drag', (ev) => {
|
||||
const ll = ev.target.getLatLng();
|
||||
// 1) Overlay-local paint FIRST (order-independent of model callback).
|
||||
const base = this._airport?.surfaces?.[surfaceIndex]?.points || [];
|
||||
const pts = patchVertexPoints(base, vi, ll.lat, ll.lng);
|
||||
this._liveSetSurfacePoints(surfaceIndex, pts);
|
||||
// 2) Model commit path (main must not refresh).
|
||||
this.onVertexDrag(surfaceIndex, vi, ll.lat, ll.lng);
|
||||
});
|
||||
|
||||
handle.on('dragend', (ev) => {
|
||||
const ll = ev.target.getLatLng();
|
||||
// Final live paint (covers last frame).
|
||||
const base = this._airport?.surfaces?.[surfaceIndex]?.points || [];
|
||||
const pts = patchVertexPoints(base, vi, ll.lat, ll.lng);
|
||||
this._liveSetSurfacePoints(surfaceIndex, pts);
|
||||
// State machine: clear pointer-down flag BEFORE app refresh path.
|
||||
this._dragEndedAt = Date.now();
|
||||
this._dragging = false;
|
||||
this.onVertexDragEnd(surfaceIndex, vi, ll.lat, ll.lng);
|
||||
});
|
||||
```
|
||||
|
||||
**Rejected ordering (do not implement):** call `onVertexDrag` first then read `this._airport.surfaces[i].points` for paint. That works today only because `setVertex` mutates the shared graph in place (`model.js`), but it is order-dependent and breaks if the callback is ever async or non-mutating. Shared mutation remains true for the document model; paint must not rely on callback side effects.
|
||||
|
||||
**Ordering contract with `main.js`:**
|
||||
|
||||
| Event | OverlayController | main.js |
|
||||
|-------|-------------------|---------|
|
||||
| `drag` | `patchVertexPoints` → `_liveSetSurfacePoints` → `onVertexDrag` | `setVertex` only — **no** `refresh` |
|
||||
| `dragend` | paint → `_dragEndedAt` + `_dragging = false` → `onVertexDragEnd` | `setVertex` + `afterAptMutation({ fit: false })` (full `refresh` **runs**) |
|
||||
|
||||
### 3. Map-click suppress after drag (single wire-up)
|
||||
|
||||
**Normative pure helper:**
|
||||
|
||||
```js
|
||||
/**
|
||||
* @param {{ dragging: boolean, dragEndedAt: number, suppressMs: number }} state
|
||||
* @param {number} [now]
|
||||
* @returns {boolean}
|
||||
*/
|
||||
export function shouldSuppressMapClick(state, now = Date.now()) {
|
||||
if (state.dragging) return true;
|
||||
if (
|
||||
state.dragEndedAt > 0 &&
|
||||
now - state.dragEndedAt < state.suppressMs
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
export const MAP_CLICK_SUPPRESS_MS = 250;
|
||||
```
|
||||
|
||||
**Normative controller fields** (constructor):
|
||||
|
||||
```js
|
||||
this._dragging = false;
|
||||
this._dragEndedAt = 0;
|
||||
// MAP_CLICK_SUPPRESS_MS imported/exported constant
|
||||
```
|
||||
|
||||
**Normative map click body** (replace today’s `if (this._dragging) return`):
|
||||
|
||||
```js
|
||||
if (this.onMapClick) {
|
||||
map.on('click', (ev) => {
|
||||
if (
|
||||
shouldSuppressMapClick(
|
||||
{
|
||||
dragging: this._dragging,
|
||||
dragEndedAt: this._dragEndedAt,
|
||||
suppressMs: MAP_CLICK_SUPPRESS_MS,
|
||||
},
|
||||
Date.now(),
|
||||
)
|
||||
) {
|
||||
return;
|
||||
}
|
||||
const ll = ev.latlng;
|
||||
this.onMapClick(ll.lat, ll.lng, ev.originalEvent);
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Aircraft dragend** uses the same state fields and the same helper (set `_dragEndedAt`, clear `_dragging` **before** `onAircraftDragEnd`).
|
||||
|
||||
**Test matrix (required rows):**
|
||||
|
||||
| `dragging` | `dragEndedAt` relative to `now` | `shouldSuppressMapClick` | `render` allowed? |
|
||||
|------------|----------------------------------|--------------------------|-------------------|
|
||||
| `true` | any | `true` | **No** (caller invariant; pointer still down) |
|
||||
| `false` | within `suppressMs` | `true` | **Yes** (post-drag rebuild must run) |
|
||||
| `false` | after `suppressMs` / `0` | `false` | **Yes** |
|
||||
|
||||
This is the critical distinction fixed after review: **post-drag click suppress must not block `render`.**
|
||||
|
||||
### 4. CSS hit target
|
||||
|
||||
```css
|
||||
/* Vertex drag handles (Leaflet divIcon).
|
||||
* Width/height MUST match VERTEX_HANDLE_PX (map-layers.js) — keep in sync.
|
||||
* Do not use negative margin; Leaflet positions via iconAnchor. */
|
||||
.apted-vertex-handle {
|
||||
position: relative; /* containing block for ::after hit pad */
|
||||
overflow: visible; /* do not clip expanded hit area */
|
||||
width: 16px !important;
|
||||
height: 16px !important;
|
||||
margin: 0 !important; /* positioning is iconAnchor only */
|
||||
border: 2px solid #4a7ab0;
|
||||
border-radius: 50%;
|
||||
background: #fff;
|
||||
box-shadow: 0 0 0 1px rgba(0, 0, 0, 0.25);
|
||||
cursor: grab;
|
||||
box-sizing: border-box;
|
||||
}
|
||||
.apted-vertex-handle:active {
|
||||
cursor: grabbing;
|
||||
background: #e8f0fa;
|
||||
}
|
||||
/* Expanded hit area without growing the visible disc.
|
||||
* Relies on position:relative + overflow:visible on the icon. */
|
||||
.apted-vertex-handle::after {
|
||||
content: '';
|
||||
position: absolute;
|
||||
inset: -6px; /* ~28px effective target */
|
||||
}
|
||||
```
|
||||
|
||||
Notes:
|
||||
|
||||
- Leaflet marker icons are typically `position: absolute` with explicit width/height; `position: relative` on the icon element establishes the containing block for `::after` without fighting map placement (the icon’s own transform is on the parent marker pane node).
|
||||
- CSS `!important` width/height overrides Leaflet’s inline size styles from `iconSize` — **both** must stay at 16. Accept comment-synced constants; do **not** inject user-derived HTML via `divIcon` `html:` (XSS).
|
||||
- Checklist item: verify centered grab in manual smoke step 2.
|
||||
|
||||
### 5. `main.js` callback simplification
|
||||
|
||||
```js
|
||||
onVertexDrag(si, vi, lat, lon) {
|
||||
setVertex(doc, si, vi, { lat, lon });
|
||||
// Intentionally no refresh — OverlayController live-updates the surface layer.
|
||||
// Hard invariant: never refresh/render while OverlayController._dragging.
|
||||
},
|
||||
onVertexDragEnd(si, vi, lat, lon) {
|
||||
setVertex(doc, si, vi, { lat, lon });
|
||||
afterAptMutation({ fit: false }); // full refresh — _dragging already false
|
||||
},
|
||||
```
|
||||
|
||||
Status tip (PR2):
|
||||
|
||||
- After successful surface select (map or rail) in Select mode, `showStatus('Drag white handles to move vertices. Click empty map to deselect.', false)`.
|
||||
- Toolbar tip for Select mode remains: “Select mode — click features; drag vertices.”
|
||||
- Do **not** thrash status on every drag tick; tip only when selection becomes a surface.
|
||||
|
||||
Do **not** call `rail.render` on every drag tick (would thrash DOM). Inspector lat/lon table updates on dragend via existing `refresh` — acceptable; live inspector coords are non-goal.
|
||||
|
||||
### 6. Mid-drag `render` policy (hard invariant — no soft early-out)
|
||||
|
||||
**Chosen approach (K9):** callers must not call `render` / `refresh` / `setSelection` while a vertex or aircraft drag is active (`_dragging === true`).
|
||||
|
||||
- `onVertexDrag` / `onAircraftDrag` already forbid refresh.
|
||||
- Rail `onSelect` → `refresh` during an active map drag is not a supported path; if it ever happens, Leaflet’s active `Draggable` is already at risk — a soft early-out that only caches `_airport` would **drop `_selection` updates** and desync controller vs `doc` without fixing the Draggable issue.
|
||||
- Therefore: **omit** a `if (this._dragging) return` early-out inside `render`. Keep `render` simple and always full-rebuild. Enforcement is the main.js / callback contract + state machine above.
|
||||
- `setSelection` continues to call `render` as today; it is only used when the user is not mid-drag of a vertex handle.
|
||||
|
||||
### 7. Parking / single-point hold
|
||||
|
||||
Existing `_addSurface` already uses `circleMarker` for parking and single-point hold. `applySurfaceLatLngs` uses `setLatLng`. No second code path. Layer-type contract in §2 applies.
|
||||
|
||||
**Not in scope:** making the parking disc itself the only drag affordance (handle is enough once aligned).
|
||||
|
||||
### 8. Interaction simplification summary
|
||||
|
||||
| Before | After |
|
||||
|--------|--------|
|
||||
| Dual circle + marker | Single grab handle |
|
||||
| 12px + offset margins | 16px + expanded `::after` hit pad, no margin offset |
|
||||
| Line frozen mid-drag | Line/point follows handle |
|
||||
| 50ms `_dragging` hold doubles as click suppress | `_dragging` pointer-only; `dragEndedAt` + pure helper for clicks |
|
||||
| Dragend refresh blocked if suppress overloads `_dragging` | Dragend clears `_dragging` first; full rebuild always |
|
||||
| Opaque failure modes | Status tip on select; selection retained after drag |
|
||||
|
||||
Modes remain: Select / Park / Taxi / Runway / Hold / Aircraft. No mode removed or added.
|
||||
|
||||
### Architecture after fix
|
||||
|
||||
```mermaid
|
||||
flowchart TB
|
||||
subgraph mid_drag ["During drag (_dragging=true; no full render)"]
|
||||
H[Vertex L.marker]
|
||||
H -->|drag| Patch[patchVertexPoints]
|
||||
Patch --> Live["applySurfaceLatLngs"]
|
||||
H -->|onVertexDrag| SV["setVertex(doc)"]
|
||||
Live --> PL["polyline or circleMarker\n(surface:N contract)"]
|
||||
SV --> Doc[(EditorDocument)]
|
||||
end
|
||||
|
||||
subgraph end_drag ["On dragend"]
|
||||
H2[dragend] --> Clear["_dragEndedAt=now\n_dragging=false"]
|
||||
Clear --> EndCb[onVertexDragEnd]
|
||||
EndCb --> SV2[setVertex]
|
||||
SV2 --> AAM[afterAptMutation]
|
||||
AAM --> Val[validateDocument]
|
||||
AAM --> Ref["refresh → full render\n(allowed)"]
|
||||
Ref --> Rail[rail + titlebar chips]
|
||||
end
|
||||
|
||||
subgraph click ["Map click"]
|
||||
C[map click] --> Sup{shouldSuppressMapClick}
|
||||
Sup -->|dragging or within suppressMs| Ignore[ignore]
|
||||
Sup -->|else| ClearSel[select mode: clear selection]
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## API / Interface Changes
|
||||
|
||||
No server HTTP, Go package, or import-graph changes.
|
||||
|
||||
### JS exports (additive, pure)
|
||||
|
||||
| Export | Module | Purpose |
|
||||
|--------|--------|---------|
|
||||
| `pointsToLatLngs(points)` | `map-layers.js` | Finite point → `[lat,lon][]` |
|
||||
| `applySurfaceLatLngs(layer, latlngs)` | `map-layers.js` | Duck-typed live geometry apply; returns branch taken |
|
||||
| `patchVertexPoints(points, vi, lat, lon)` | `map-layers.js` | Immutable-style vertex patch for paint |
|
||||
| `shouldSuppressMapClick(state, now?)` | `map-layers.js` | Drag / post-drag map-click filter |
|
||||
| `MAP_CLICK_SUPPRESS_MS` | `map-layers.js` | Default 250 |
|
||||
| `VERTEX_HANDLE_PX` | `map-layers.js` | 16; comment-synced with CSS |
|
||||
| `buildVertexHandleOptions(vi)` | `map-layers.js` | Marker options for tests |
|
||||
| `buildVertexHandleIconOptions()` | `map-layers.js` | `iconSize`/`iconAnchor` symmetry for tests |
|
||||
|
||||
### `OverlayController` internal methods
|
||||
|
||||
| Method | Visibility | Behavior |
|
||||
|--------|------------|----------|
|
||||
| `_liveSetSurfacePoints(si, points)` | private | `applySurfaceLatLngs` via `_layerByKey` |
|
||||
| `_addVertexHandles` | private | Single marker; normative drag state machine |
|
||||
| `render` | public | Full rebuild always; **no** mid-drag early-out |
|
||||
|
||||
### Callback contracts (unchanged signatures)
|
||||
|
||||
```ts
|
||||
onVertexDrag?: (surfaceIndex: number, vertexIndex: number, lat: number, lon: number) => void
|
||||
onVertexDragEnd?: (surfaceIndex: number, vertexIndex: number, lat: number, lon: number) => void
|
||||
```
|
||||
|
||||
Semantics clarified:
|
||||
|
||||
- **`onVertexDrag`**: may fire at high frequency; **must not** trigger full overlay `render` / `refresh`.
|
||||
- **`onVertexDragEnd`**: invoked only after OverlayController has set `_dragging = false`; **must** full-refresh via existing mutation path.
|
||||
|
||||
### CSS
|
||||
|
||||
- `.apted-vertex-handle` as §4 (`position: relative`, `overflow: visible`, no negative margin).
|
||||
- No new global Leaflet overrides.
|
||||
|
||||
---
|
||||
|
||||
## Data Model Changes
|
||||
|
||||
**None.**
|
||||
|
||||
- `setVertex` / `insertVertex` / `deleteVertex` remain the mutation API.
|
||||
- Dirty flags, download hash, validation arrays unchanged.
|
||||
- No schema / migration / server storage.
|
||||
|
||||
---
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
### A1 — Vendor `leaflet-draw` / `leaflet-geoman`
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| Mature vertex edit UX | New dependency weight; complexity-gate; harder pure-unit testing; diverges from parent “custom tools first” |
|
||||
| | Larger review surface than a bugfix |
|
||||
|
||||
**Rejected** for this fix. Revisit only if select→drag still inadequate after the fix PR (parent already deferred this).
|
||||
|
||||
### A2 — Always show vertices for all surfaces in Select mode
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| Matches “drag any node on any polyline” literally | Visual clutter (hundreds of handles); hit-testing ambiguity between adjacent taxiways; expensive layer count |
|
||||
| | Still need selection for inspector/delete |
|
||||
|
||||
**Rejected** as default. Optional future: “show all vertices” toggle — out of scope.
|
||||
|
||||
### A3 — Custom mousedown/mousemove drag on `circleMarker` only (no `L.marker`)
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| One layer type for polylines and handles | Reimplement Leaflet.Draggable; more bug surface; map pan conflicts |
|
||||
|
||||
**Rejected** — fix `divIcon` marker path instead.
|
||||
|
||||
### A4 — Live update only on dragend (model-only mid-drag)
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| Minimal code | **Does not fix user report**; line still frozen |
|
||||
|
||||
**Rejected** — contradicts parent performance/UX note and user ask.
|
||||
|
||||
### A5 — Full rewrite of overlay as React/canvas
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| — | Violates boring-web / house SPA ban; massive scope |
|
||||
|
||||
**Rejected.**
|
||||
|
||||
### A6 — Soft `render` early-out while `_dragging` (cache-only)
|
||||
|
||||
| Pros | Cons |
|
||||
|------|------|
|
||||
| Defensive if a buggy caller refreshes mid-drag | Easy to get wrong (drop `_selection`); masks caller bugs; blocked post-drag rebuild when incorrectly tied to suppress window (rev1 defect) |
|
||||
|
||||
**Rejected** in favor of hard caller invariant (K9).
|
||||
|
||||
### Chosen approach vs alternatives
|
||||
|
||||
**Targeted OverlayController + main + CSS fix in one user-complete PR** wins: smallest diff, aligns with parent design, testable pure helpers, no new deps, preserves draw/select modes, non-contradictory state machine.
|
||||
|
||||
---
|
||||
|
||||
## Security & Privacy Considerations
|
||||
|
||||
| Topic | Assessment |
|
||||
|-------|------------|
|
||||
| XSS | Continue **no `innerHTML`** for file-derived labels; `divIcon` uses `className` only (no `html:` with surface names). Tooltips stay `tooltipTextNode` / `textContent`. |
|
||||
| Authz | Unchanged — Admin-only page shell; no new routes. |
|
||||
| CSRF | No new state-changing server endpoints. |
|
||||
| Data | Geometry still browser-local; drag does not POST coordinates. |
|
||||
| Threat | Malicious `.apt` with huge point counts — already a client DoS concern; live `setLatLngs` is O(points per surface) per move event, fine for budgeted sizes. |
|
||||
|
||||
No new privacy surface.
|
||||
|
||||
---
|
||||
|
||||
## Observability
|
||||
|
||||
Client-only feature; no new server metrics.
|
||||
|
||||
| Mechanism | Use |
|
||||
|-----------|-----|
|
||||
| Existing status flash (`showStatus`) | User feedback on select / errors |
|
||||
| `doc.aptDirty` + titlebar `APT*` | Confirms drag committed |
|
||||
| Validate tab | Catches invalid geometry after edit |
|
||||
| `slog` / Gin | Unchanged (page load only) |
|
||||
|
||||
Do **not** add `console.log` noise (hygiene / house style). No browser RUM required.
|
||||
|
||||
---
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Pure Node tests (`webjs/airport-editor/map-layers.test.js`)
|
||||
|
||||
Add cases for:
|
||||
|
||||
1. **`pointsToLatLngs`**: empty, skips non-finite, preserves order.
|
||||
2. **`applySurfaceLatLngs`** with mock layers:
|
||||
- `{ setLatLngs(calls) }` → branch `'polyline'`, called with full latlngs.
|
||||
- `{ setLatLng(calls) }` only → branch `'point'`, called with first latlng.
|
||||
- `{}` / `null` → `'none'`.
|
||||
3. **`patchVertexPoints`**: replaces only index `vi`; does not mutate input array.
|
||||
4. **`shouldSuppressMapClick`** (full matrix from §3):
|
||||
- `dragging: true` → true (render still “allowed” only in the sense of policy — document that post-drag uses `dragging: false`).
|
||||
- `dragging: false`, within `suppressMs` after `dragEndedAt` → true (**and** render must still be allowed — separate concern; assert suppress only).
|
||||
- `dragging: false`, after window / `dragEndedAt: 0` → false.
|
||||
5. **`buildVertexHandleIconOptions`**: `iconSize[0] === iconSize[1] === VERTEX_HANDLE_PX`; `iconAnchor` is half; className includes `apted-vertex-handle`.
|
||||
6. **`buildVertexHandleOptions`**: `draggable: true`, `autoPan: false`, `bubblingMouseEvents: false`.
|
||||
7. Short comment block above the suite: RC1 (live path = `applySurfaceLatLngs`), RC2 (no companion circle factory in exports), RC3 (no CSS margin in JS positioning), K5 (suppress ≠ block render).
|
||||
|
||||
Existing `setVertex` tests in `model.test.js` remain sufficient for model commits.
|
||||
|
||||
**Not required:** parsing `map-layers.js` source text for “circleMarker” string matches (brittle). Prefer exported pure builders so “no dual handle” is a construction discipline + code review of `_addVertexHandles`.
|
||||
|
||||
### What we will not automate (default)
|
||||
|
||||
- Real Leaflet pointer drag in headless Chrome (Playwright).
|
||||
- Visual pixel diffs.
|
||||
|
||||
### Manual smoke checklist (PR description / engineer runbook)
|
||||
|
||||
Run `go run ./cmd/openfsd -web` (or full binary), Admin login, open `/airport-editor`:
|
||||
|
||||
1. Open a multi-surface `.apt` (e.g. KBTV-scale fixture if available) → Fit.
|
||||
2. **Select mode** → click taxiway → handles appear **centered** on vertices (single white disc each — no double-dot).
|
||||
3. Drag a mid vertex → **polyline follows** handle continuously.
|
||||
4. Release → line stays; selection **remains**; titlebar shows `APT*`; inspector coords updated (full refresh ran).
|
||||
5. Drag runway end vertex → same.
|
||||
6. Select parking → drag its single handle → disc follows; release keeps selection.
|
||||
7. Select hold polyline / point → same.
|
||||
8. Click empty map (after >~300ms) → selection clears (handles gone). Immediately after a drag, a spurious map click should **not** clear selection.
|
||||
9. Drag aircraft marker → still works; no vertex regression.
|
||||
10. Draw new taxi (mode Taxi) → finish → select → drag still works.
|
||||
11. Rail: Add vertex / Delete vertex → map updates; drag new vertex works. (**Note:** keyboard Del still deletes **surface**, not vertex — expected; see Known gaps.)
|
||||
12. Download `.apt` → coords reflect dragged positions (spot-check raw / re-open).
|
||||
13. Repeat a drag near map edge with `autoPan: false` — map should not jump unexpectedly.
|
||||
14. JS disabled: page shell still loads (map PE exception); no server regression.
|
||||
|
||||
### CI
|
||||
|
||||
```bash
|
||||
bash scripts/check-webjs.sh
|
||||
# or: cd webjs && npm test
|
||||
go test -race ./internal/web/... # no Go change expected; sanity
|
||||
bash scripts/check-hygiene.sh # if any Go touched (should not be)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Rollout Plan
|
||||
|
||||
| Stage | Action |
|
||||
|-------|--------|
|
||||
| Implement | **One user-complete fix PR** (see PR Plan); optional follow-up for status copy/docs; **no feature flag** |
|
||||
| Verify | Manual smoke on local binary + `check-webjs.sh` green |
|
||||
| Deploy | Standard openfsd release; static assets go with binary (`go:embed` / static tree as today) |
|
||||
| Rollback | Revert PR(s); no migration. Users may have downloaded interim APT files — no server state. |
|
||||
| Flag | Not needed; risk contained to Admin map editor |
|
||||
|
||||
### Risks
|
||||
|
||||
| Risk | Severity | Mitigation |
|
||||
|------|----------|------------|
|
||||
| Residual click still clears selection on some browsers | Med | `dragEndedAt` + 250ms suppress + `bubblingMouseEvents: false` + stopPropagation; tune `MAP_CLICK_SUPPRESS_MS` if smoke fails (see OQ1) |
|
||||
| Live update no-ops if surface layer wrapped later | Med | Layer-type contract in §2; `applySurfaceLatLngs` returns `'none'`; code review of `_addSurface` |
|
||||
| Caller `refresh` mid-drag destroys Draggable | Low | Hard invariant; no soft early-out; keep drag callbacks refresh-free |
|
||||
| CSS/`VERTEX_HANDLE_PX` drift | Low | Comment keep-in-sync; icon options unit test; smoke step 2 |
|
||||
| Operator habit: expects drag without select | Low | Status tip; optional future “show all vertices” non-goal |
|
||||
| QA expects parent “Del = delete vertex” | Low | Document known gap (rail-only vertex delete) in PR / design note |
|
||||
|
||||
---
|
||||
|
||||
## Open Questions
|
||||
|
||||
1. **Suppress duration**: Is 250ms enough on slow trackpads / touch, or should it be 400ms (near old mobile 300ms synthetic click)? Resolve via manual smoke; constant is `MAP_CLICK_SUPPRESS_MS`.
|
||||
2. **Live inspector coordinates**: Update rail vertex table mid-drag? **Default no** (DOM thrash). Confirm with product if instructors edit numbers while dragging (unlikely).
|
||||
3. **Delete key vs parent table**: Parent UX table says “Delete vertex | Select handle + Delete key”; implementation deletes **surface**. This fix **accepts rail-only vertex delete** and documents the gap. Vertex-level Del is a separate small UX PR if desired.
|
||||
4. **Copy design into repo**: **Done** — this file (`docs/design/airport-editor-vertex-drag.md`).
|
||||
|
||||
### Known gaps (document, do not fix here)
|
||||
|
||||
- **Vertex delete is rail-only** (`ui-rail.js` delete-vertex action). Keyboard Delete/Backspace with a surface selected deletes the **entire surface** (`main.js` `onKeyDown`). Parent design table claim is aspirational / stale relative to shipped code.
|
||||
|
||||
---
|
||||
|
||||
## References
|
||||
|
||||
- Parent design: `docs/design/apt-air-editor.md` (Implemented) — vertex UX table, performance note, PR7 edit loop.
|
||||
- Agents / package rules: `Agents.md` — `internal/web` isolation; boring-web mandatory; coverage + `check-webjs.sh`.
|
||||
- Boring-web: `~/.grok/skills/boring-web/SKILL.md`, checklist, decision-test (map PE exception already documented for this page).
|
||||
- Implementation files:
|
||||
- `internal/web/static/js/openfsd/airport-editor/map-layers.js` — `OverlayController`, `_addVertexHandles`
|
||||
- `internal/web/static/js/openfsd/airport-editor/main.js` — `onVertexDrag` / `onVertexDragEnd` / `refresh`
|
||||
- `internal/web/static/js/openfsd/airport-editor/model.js` — `setVertex`
|
||||
- `internal/web/static/css/openfsd/airport-editor.css` — `.apted-vertex-handle`
|
||||
- `webjs/airport-editor/map-layers.test.js`, `model.test.js`
|
||||
- Leaflet CSS: `internal/web/static/css/leaflet.css` (marker pane z-index, `.leaflet-interactive` pointer-events, `.leaflet-div-icon`)
|
||||
|
||||
---
|
||||
|
||||
## PR Plan
|
||||
|
||||
Prefer a **tight, user-complete fix** over a three-way split. CSS grab alignment (RC3, high severity) must not land after “live geometry only.”
|
||||
|
||||
### PR 1 — Fix vertex drag (user-complete) **[default]**
|
||||
|
||||
| Field | Content |
|
||||
|-------|---------|
|
||||
| **Title** | airport-editor: fix vertex drag (live geometry, handles, click suppress) |
|
||||
| **Files** | `internal/web/static/js/openfsd/airport-editor/map-layers.js`; `internal/web/static/js/openfsd/airport-editor/main.js`; `internal/web/static/css/openfsd/airport-editor.css`; `webjs/airport-editor/map-layers.test.js` |
|
||||
| **Depends on** | None |
|
||||
| **Description** | **Single mergeable fix.** Remove dual circleMarker+marker. Order-independent live paint via `patchVertexPoints` + `applySurfaceLatLngs`. Normative state machine: clear `_dragging` before `onVertexDragEnd`; map click uses `shouldSuppressMapClick` with `_dragEndedAt`. CSS: remove double-offset margins, 16px handle, `position: relative` + `overflow: visible` + `::after` hit pad. Export pure helpers + handle option builders; unit tests for points, apply, suppress matrix, iconSize/iconAnchor symmetry. Same suppress for aircraft dragend. **Do not claim “user-complete” without CSS.** |
|
||||
|
||||
### PR 2 — UX copy + docs
|
||||
|
||||
| Field | Content |
|
||||
|-------|---------|
|
||||
| **Title** | airport-editor: select-mode drag status tip and design note |
|
||||
| **Files** | `main.js` (status on surface select / mode tip alignment); `docs/design/airport-editor-vertex-drag.md` (this design); one-liner under parent “Implementation status” |
|
||||
| **Depends on** | PR 1 |
|
||||
| **Description** | On surface select in Select mode, flash a one-line tip to drag handles. Note parent Del-key / rail-only vertex delete gap. No Playwright. |
|
||||
|
||||
### Suggested merge order
|
||||
|
||||
```text
|
||||
PR1 (user-complete fix) → PR2 (copy / docs) [both landed]
|
||||
```
|
||||
|
||||
**Do not** split live geometry from CSS/suppress into separate merges by default — that leaves high-severity RC3 unfixed after “live geometry only.” If a multi-PR split is forced for review bandwidth, the **only** acceptable partial first land is CSS-only (margin fix) as a PR0 that can merge alone; never ship live geometry without CSS alignment as the advertised “fix.”
|
||||
|
||||
### Out-of-plan follow-ups (not blocking)
|
||||
|
||||
- Mid-edge insert handles on map.
|
||||
- Optional “show all vertices” density toggle.
|
||||
- Vertex-level Delete key (align with parent table).
|
||||
- Quantize float chatter on format (parent already optional).
|
||||
|
||||
---
|
||||
|
||||
## Implementation checklist (engineer)
|
||||
|
||||
- [x] Single `divIcon` handle; no dual circleMarker
|
||||
- [x] Live paint: `patchVertexPoints` → `applySurfaceLatLngs` on drag (**before** model callback)
|
||||
- [x] Surface layer contract: `surface:N` is polyline or circleMarker only
|
||||
- [x] `onVertexDrag` never calls `refresh` / `rail.render`
|
||||
- [x] **dragend:** set `_dragEndedAt`, set `_dragging = false`, **then** `onVertexDragEnd` → `afterAptMutation({ fit: false })`
|
||||
- [x] Map click uses `shouldSuppressMapClick` only (not a long-lived `_dragging` after pointer-up)
|
||||
- [x] No soft `render` early-out that can block post-drag rebuild
|
||||
- [x] CSS: no negative margin; `VERTEX_HANDLE_PX` matches; `position: relative`; `overflow: visible`
|
||||
- [x] Pure tests: apply mock layers, suppress matrix (incl. “dragging false + within suppress → click suppressed, render still OK by contract”), icon symmetry
|
||||
- [x] Selection retained after successful drag (smoke)
|
||||
- [x] Document: vertex delete rail-only; Del deletes surface
|
||||
- [x] Select-mode surface tip: `Drag white handles…` (PR2)
|
||||
- [x] `bash scripts/check-webjs.sh` green
|
||||
- [ ] Manual smoke § Testing complete (engineer runbook)
|
||||
- [x] No new imports from `internal/web` → forbidden packages
|
||||
- [x] No Playwright added
|
||||
|
||||
---
|
||||
|
||||
*End of design document.*
|
||||
@@ -24,6 +24,7 @@ Shipped. Summary of the tree as of closeout:
|
||||
| Web JS | Pure modules under `internal/web/static/js/openfsd/`; Node tests in `webjs/` + `scripts/check-webjs.sh` in CI |
|
||||
| Persistence | **None** durable (no disk/DB); Blob download + transient request bodies only |
|
||||
| Playwright | **Cancelled** — no browser automation suite |
|
||||
| Vertex drag | **Fixed** (live geometry + handles); see `docs/design/airport-editor-vertex-drag.md`. Vertex delete remains rail-only (Del/Backspace deletes the whole surface). |
|
||||
|
||||
Design history below is retained. Stale “Format missing / JS tests none” rows are updated in Background.
|
||||
|
||||
|
||||
@@ -111,6 +111,7 @@ function main() {
|
||||
if (doc.mode !== MODE_SELECT) return;
|
||||
doc.selection = normalizeSelection(sel);
|
||||
refresh();
|
||||
showSurfaceSelectTip();
|
||||
},
|
||||
onVertexDrag(si, vi, lat, lon) {
|
||||
setVertex(doc, si, vi, { lat, lon });
|
||||
@@ -187,6 +188,7 @@ function main() {
|
||||
overlays.clearDrawPreview();
|
||||
}
|
||||
refresh();
|
||||
showSurfaceSelectTip();
|
||||
},
|
||||
onAirportPatch(patch) {
|
||||
updateAirportHeaders(doc, patch);
|
||||
@@ -758,6 +760,19 @@ function main() {
|
||||
el.setAttribute('role', isError ? 'alert' : 'status');
|
||||
}
|
||||
|
||||
/**
|
||||
* One-line tip when a surface is selected in Select mode (map or rail).
|
||||
* Only on selection change — never mid-drag.
|
||||
*/
|
||||
function showSurfaceSelectTip() {
|
||||
if (doc.mode !== MODE_SELECT) return;
|
||||
if (doc.selection?.type !== 'surface') return;
|
||||
showStatus(
|
||||
'Drag white handles to move vertices. Click empty map to deselect.',
|
||||
false,
|
||||
);
|
||||
}
|
||||
|
||||
// Silence unused kind imports for tree-shaking edge cases
|
||||
void SurfaceParking;
|
||||
void SurfaceHold;
|
||||
|
||||
Reference in New Issue
Block a user