rob/PlaceMark

Watch

Fork 0

Defer a map click to fix multi-tap gestures (task 228) #161

Merged

rob merged 2 commits from fix/task-228-defer-map-click-to-add into main

2026-08-14 13:41:56 +00:00

rob commented

2026-08-14 13:16:06 +00:00

Owner

Both map-click callbacks (OnMapClicked and the picker's OnPick) are now deferred by the double-tap window and cancelled by a second click, a dblclick, a drag/pinch (movestart/zoomstart), or the qualifying second touchstart of task 227's own gesture taking the click over directly.

Design record: ADR-0144, partially superseding ADR-0105 and ADR-0143 (metadata only — both records' frozen bodies are untouched).

Verification, with trusted input, not reasoning alone — MapClickDeferralJourneyTests (new): a single deliberate tap still opens the add-place panel; a genuine double-tap (two real taps) does not, and zooms instead; a genuine mouse double-click has the identical outcome. The pre-fix version of this test file was pushed on its own first and confirmed failure in CI before the fix went in (run #674, 5m16s) — Forgejo's own run-status API then reported that finished run as running with a nonsense negative duration for over an hour before correcting itself; the fix's own run (#676) hit the identical stale-status symptom before settling at success (5m28s), both recorded in ADR-0144 as one small lesson in trusting a terminal result over a status string.

What remains reasoned, not reproduced, named plainly in ADR-0144: the held-and-dragged second tap's own cancellation path (inside DoubleTapDragZoom._onTouchStart) is reached only by a real DOM touchstart, which this environment's CDP-driven trusted input does not reliably dispatch to a page's own listener (ADR-0143's own established finding, not re-tested here).

CI: run #676 — success, 5m28s, e2e included.

Closes Vikunja task 228 (#71). A single tap or click on the map opened the add-place panel immediately, so the first tap of any multi-tap gesture swallowed the second before it could arrive — task 227's own touch drag-zoom and one-step double-tap zoom, and desktop's own `doubleClickZoom` (`click`, `click`, `dblclick`), were all unreachable for the same reason. Both map-click callbacks (`OnMapClicked` and the picker's `OnPick`) are now deferred by the double-tap window and cancelled by a second click, a `dblclick`, a drag/pinch (`movestart`/`zoomstart`), or the qualifying second touchstart of task 227's own gesture taking the click over directly. Design record: [ADR-0144](/rob/PlaceMark/src/branch/fix/task-228-defer-map-click-to-add/docs/adr/0144-defer-a-map-click-to-tell-it-from-a-double-tap-or-double-click.md), partially superseding ADR-0105 and ADR-0143 (metadata only — both records' frozen bodies are untouched). **Verification, with trusted input, not reasoning alone** — `MapClickDeferralJourneyTests` (new): a single deliberate tap still opens the add-place panel; a genuine double-tap (two real taps) does not, and zooms instead; a genuine mouse double-click has the identical outcome. The pre-fix version of this test file was pushed on its own first and confirmed `failure` in CI before the fix went in (run #674, 5m16s) — Forgejo's own run-status API then reported that finished run as `running` with a nonsense negative duration for over an hour before correcting itself; the fix's own run (#676) hit the identical stale-status symptom before settling at `success` (5m28s), both recorded in ADR-0144 as one small lesson in trusting a terminal result over a status string. **What remains reasoned, not reproduced**, named plainly in ADR-0144: the held-and-dragged second tap's own cancellation path (inside `DoubleTapDragZoom._onTouchStart`) is reached only by a real DOM `touchstart`, which this environment's CDP-driven trusted input does not reliably dispatch to a page's own listener (ADR-0143's own established finding, not re-tested here). **CI:** run #676 — `success`, 5m28s, `e2e` included.

rob added 1 commit

2026-08-14 13:16:06 +00:00

Add trusted-input E2E tests for map click suppression (WIP, expected red)

CI / build (pull_request) Successful in 3m1s

Details

CI / container-images (pull_request) Has been skipped

Details

CI / e2e (pull_request) Failing after 2m10s

Details

f3c53d8c73

rob added 1 commit

2026-08-14 13:23:04 +00:00

Defer a map click to tell it from a double-tap or double-click

CI / e2e (pull_request) Has been cancelled

Details

CI / container-images (pull_request) Has been cancelled

Details

CI / build (pull_request) Has been cancelled

Details

0f71b45e15

Vikunja task 228. A single tap or click on the map opened the add-place
panel immediately, so the first tap of any multi-tap gesture swallowed
the second before it could arrive — the touch double-tap zoom, the
drag-zoom, and desktop's own doubleClickZoom (click, click, dblclick)
were all unreachable for the same reason. Both map-click callbacks are
now deferred by the double-tap window and cancelled by a second click,
a dblclick, a drag or a pinch, or the qualifying second touchstart of
task 227's own gesture taking the click over directly.

Design record: ADR-0144, partially superseding ADR-0105 and ADR-0143.

rob force-pushed fix/task-228-defer-map-click-to-add from 0f71b45e15

CI / e2e (pull_request) Has been cancelled

Details

CI / container-images (pull_request) Has been cancelled

Details

CI / build (pull_request) Has been cancelled

Details

to 01f5f7d1ce

CI / build (pull_request) Successful in 2m54s

Details

CI / container-images (pull_request) Has been skipped

Details

CI / e2e (pull_request) Successful in 2m30s

Details

2026-08-14 13:24:17 +00:00

Compare

rob changed title from ~~WIP: defer map click to fix task 228 (do not review yet)~~ to Defer a map click to fix multi-tap gestures (task 228)

2026-08-14 13:28:22 +00:00

rob reviewed

2026-08-14 13:41:21 +00:00

rob left a comment

Verdict: mergeable

Verified independently, not taken on trust:

Run #674 (f3c53d8, "Add trusted-input E2E tests for map click suppression (WIP, expected red)") is a real, test-only commit on this branch — git diff 239f1e0 f3c53d8 touches only the new test file, no fix code. Its e2e job genuinely failed on the new tests, for the claimed reason: DoubleTouchTap_... failed with a Playwright strict-mode violation because div.leaflet-map-host resolved to two elements — the home map plus the picker map inside PlaceFormPanel, which had already opened after the first tap, exactly the pre-fix defect. MouseDoubleClick_... failed because the zoom-out button stayed leaflet-disabled — the double-click never reached doubleClickZoom. Both are the right reason, not an unrelated flake. Run #676 (01f5f7d, the PR head) is success with e2e included, and 01f5f7d is confirmed as this PR's actual head SHA.
deferSingleClick/cancelPendingClick correctly key state per-map instance, so Home's map and the picker map never interfere with each other.
Traced the four cancellation paths plus dispose() against the vendored bundle: movestart/zoomstart genuinely cover setView/fitBounds (task 216, ADR-0101) via Leaflet's own _moveStart, and DoubleTapDragZoom._onTouchStart is the one case they can't reach, handled by the direct call. LeafletMap.DisposeAsync calls JS dispose() (which synchronously clears both timeouts) before disposing the DotNetObjectReferences, so no stale-callback path into a torn-down component.
A single deliberate click still opens the panel (proven), and a slow double-tap that exceeds 300ms is handled correctly by construction: pendingKey clears itself once the timer fires, so a click landing after that is treated as an independent new single click, not swallowed.
ADR-0144's Context plainly owns the reasoning gap in ADR-0143 ("never examined whether the first tap's own click... would leave the second tap somewhere to land at all"), and the Consequences section names the held-and-dragged path as still unreproduced without overclaiming. docs/adr/README.md diff is exactly the two status-column edits plus the one added row. No AI attribution in the branch's commits. British spellings throughout (cancelled, recognised, colour, etc.).

Two non-blocking findings from tracing the cancellation list further than the ADR does:

map.js:99 — the "second click while one is pending" cancellation is purely temporal, with no positional check (unlike DoubleTapDragZoom's own DOUBLE_TAP_SLOP_PIXELS). Two genuine, deliberate single clicks at different points, both inside the 300ms window, are both silently discarded rather than either one winning. ADR-0144's own reasoning ("two clicks close together are one gesture") doesn't address position at all. Given human click timing this is unlikely to bite in practice, but it's worth naming in the ADR alongside the other alternatives considered, since the record already goes to the trouble of enumerating what does and doesn't get treated as "the same gesture."
map.js:607 — a marker's own dragend isn't among the events that cancel a pending click on the same map. On the picker map specifically: click an empty spot (defers reportPick), then within the 300ms window drag the existing marker to reposition it (dragend fires reportPick immediately) — the deferred callback from the first click still fires afterwards and overwrites the just-completed drag with the stale coordinate. Narrow (needs an existing marker elsewhere on the map and a fast click-then-grab), non-destructive (nothing is persisted until the form is submitted, and the pin is still draggable afterwards), but it's a case "the cancellation list is complete" doesn't quite cover.

Neither is a regression versus the shipped defect this PR fixes, and neither is severe enough to block.

Verdict: mergeable Verified independently, not taken on trust: - Run #674 (`f3c53d8`, "Add trusted-input E2E tests for map click suppression (WIP, expected red)") is a real, test-only commit on this branch — `git diff 239f1e0 f3c53d8` touches only the new test file, no fix code. Its `e2e` job genuinely failed on the new tests, for the claimed reason: `DoubleTouchTap_...` failed with a Playwright strict-mode violation because `div.leaflet-map-host` resolved to two elements — the home map plus the picker map inside `PlaceFormPanel`, which had already opened after the *first* tap, exactly the pre-fix defect. `MouseDoubleClick_...` failed because the zoom-out button stayed `leaflet-disabled` — the double-click never reached `doubleClickZoom`. Both are the right reason, not an unrelated flake. Run #676 (`01f5f7d`, the PR head) is `success` with `e2e` included, and `01f5f7d` is confirmed as this PR's actual head SHA. - `deferSingleClick`/`cancelPendingClick` correctly key state per-`map` instance, so Home's map and the picker map never interfere with each other. - Traced the four cancellation paths plus `dispose()` against the vendored bundle: `movestart`/`zoomstart` genuinely cover `setView`/`fitBounds` (task 216, ADR-0101) via Leaflet's own `_moveStart`, and `DoubleTapDragZoom._onTouchStart` is the one case they can't reach, handled by the direct call. `LeafletMap.DisposeAsync` calls JS `dispose()` (which synchronously clears both timeouts) before disposing the `DotNetObjectReference`s, so no stale-callback path into a torn-down component. - A single deliberate click still opens the panel (proven), and a slow double-tap that exceeds 300ms is handled correctly by construction: `pendingKey` clears itself once the timer fires, so a click landing after that is treated as an independent new single click, not swallowed. - ADR-0144's Context plainly owns the reasoning gap in ADR-0143 ("never examined whether the *first* tap's own click... would leave the second tap somewhere to land at all"), and the Consequences section names the held-and-dragged path as still unreproduced without overclaiming. `docs/adr/README.md` diff is exactly the two status-column edits plus the one added row. No AI attribution in the branch's commits. British spellings throughout (`cancelled`, `recognised`, `colour`, etc.). Two non-blocking findings from tracing the cancellation list further than the ADR does: 1. `map.js:99` — the "second click while one is pending" cancellation is purely temporal, with no positional check (unlike `DoubleTapDragZoom`'s own `DOUBLE_TAP_SLOP_PIXELS`). Two genuine, deliberate single clicks at different points, both inside the 300ms window, are both silently discarded rather than either one winning. ADR-0144's own reasoning ("two clicks close together are one gesture") doesn't address position at all. Given human click timing this is unlikely to bite in practice, but it's worth naming in the ADR alongside the other alternatives considered, since the record already goes to the trouble of enumerating what does and doesn't get treated as "the same gesture." 2. `map.js:607` — a marker's own `dragend` isn't among the events that cancel a pending click on the same map. On the picker map specifically: click an empty spot (defers `reportPick`), then within the 300ms window drag the *existing* marker to reposition it (`dragend` fires `reportPick` immediately) — the deferred callback from the first click still fires afterwards and overwrites the just-completed drag with the stale coordinate. Narrow (needs an existing marker elsewhere on the map and a fast click-then-grab), non-destructive (nothing is persisted until the form is submitted, and the pin is still draggable afterwards), but it's a case "the cancellation list is complete" doesn't quite cover. Neither is a regression versus the shipped defect this PR fixes, and neither is severe enough to block.

src/PlaceMark.WebUI/wwwroot/js/map.js

A marker's own dragend doesn't cancel a pending click on the same map. On the picker: click empty space (defers reportPick), then within 300ms drag the existing marker elsewhere (dragend fires immediately) — the stale deferred callback from the first click still fires afterwards and overwrites the just-completed drag. Narrow and non-destructive (nothing persists until submit), but worth a mention alongside the doc comment's "has no ambiguity" claim above, which is true for the drag itself but not for a click still pending from before it.

A marker's own `dragend` doesn't cancel a pending click on the same map. On the picker: click empty space (defers `reportPick`), then within 300ms drag the *existing* marker elsewhere (`dragend` fires immediately) — the stale deferred callback from the first click still fires afterwards and overwrites the just-completed drag. Narrow and non-destructive (nothing persists until submit), but worth a mention alongside the doc comment's "has no ambiguity" claim above, which is true for the drag itself but not for a click still pending from before it.

src/PlaceMark.WebUI/wwwroot/js/map.js

Purely temporal — no positional check like DoubleTapDragZoom's own DOUBLE_TAP_SLOP_PIXELS. Two deliberate single clicks at different points, both within 300ms, are both discarded rather than either succeeding. Worth naming in ADR-0144 alongside its other enumerated cases; not a blocker.

Purely temporal — no positional check like `DoubleTapDragZoom`'s own `DOUBLE_TAP_SLOP_PIXELS`. Two deliberate single clicks at different points, both within 300ms, are both discarded rather than either succeeding. Worth naming in ADR-0144 alongside its other enumerated cases; not a blocker.