rob/PlaceMark

Watch

Fork 0

Fix drag-zoom gesture direction and recentring (task 230) #162

Merged

rob merged 2 commits from fix/task-230-drag-zoom-direction-and-anchor into main

2026-08-14 14:30:57 +00:00

rob commented

2026-08-14 14:07:00 +00:00

Owner

Task 230. Rob, testing on a real phone, found the double-tap-drag zoom went the wrong way and jumped so the tapped point became the viewport centre.

Direction is inverted: dragging down now zooms in. ADR-0143 had documented the opposite as checked against Google Maps via two web sources; device testing contradicts them, and ADR-0143 is annotated to record that the citation lost to the device.

The anchor was a coding error, not a design choice. The handler's own doc comment claimed it anchored at the tap point "not the map's centre", while delta was measured from the gesture's own start point — so at drag start the centre became the tapped point. delta is now measured from the container's centre point (map.getSize().divideBy(2), captured once in _onTouchStart), mirroring Leaflet's own Map.TouchZoom._onTouchMove. ADR-0146 carries the algebra: the corrected formula reduces to the map's pre-gesture centre exactly at gesture start, and holds _startLatLng under the finger for any point and zoom during the drag. The previous formula coincided only when the finger had not moved.

Proven rather than reasoned, following task 228's pattern:

Test-only commit 31c2d78 first — CI run #586's e2e job failed on the new geometric assertions, with the recentring measured at ~169px against an asserted <40px, and the zoom moving the wrong way. Confirmed from the job log itself, not merely the job status.
Fix commit 1a2f9b1 — CI run #587 green: 19/19 E2E facts, 1955/1955 xUnit (Domain 12, Contracts 139, Architecture 123, WebUI 755, Infrastructure 321, Api 605).

The tests assert geometric properties through Leaflet's own API, so a zoom-level-only check cannot pass them and a partial fix — direction corrected but anchoring still wrong, or the reverse — is caught. That required a small element.placeMarkMap = map hook in map.js.

ADR-0146 added; ADR-0143 annotated per ADR-0107.

Task 230. Rob, testing on a real phone, found the double-tap-drag zoom went the wrong way and jumped so the tapped point became the viewport centre. **Direction** is inverted: dragging down now zooms in. ADR-0143 had documented the opposite as checked against Google Maps via two web sources; device testing contradicts them, and ADR-0143 is annotated to record that the citation lost to the device. **The anchor was a coding error, not a design choice.** The handler's own doc comment claimed it anchored at the tap point "not the map's centre", while `delta` was measured from the gesture's own start point — so at drag start the centre became the tapped point. `delta` is now measured from the container's centre point (`map.getSize().divideBy(2)`, captured once in `_onTouchStart`), mirroring Leaflet's own `Map.TouchZoom._onTouchMove`. ADR-0146 carries the algebra: the corrected formula reduces to the map's pre-gesture centre exactly at gesture start, and holds `_startLatLng` under the finger for any point and zoom during the drag. The previous formula coincided only when the finger had not moved. Proven rather than reasoned, following task 228's pattern: - **Test-only commit `31c2d78` first** — CI run #586's `e2e` job failed on the new geometric assertions, with the recentring measured at ~169px against an asserted <40px, and the zoom moving the wrong way. Confirmed from the job log itself, not merely the job status. - **Fix commit `1a2f9b1`** — CI run #587 green: 19/19 E2E facts, 1955/1955 xUnit (Domain 12, Contracts 139, Architecture 123, WebUI 755, Infrastructure 321, Api 605). The tests assert geometric properties through Leaflet's own API, so a zoom-level-only check cannot pass them and a partial fix — direction corrected but anchoring still wrong, or the reverse — is caught. That required a small `element.placeMarkMap = map` hook in `map.js`. ADR-0146 added; ADR-0143 annotated per ADR-0107.

rob added 1 commit

2026-08-14 14:07:00 +00:00

Add failing E2E tests for drag-zoom direction and anchor (task 230)

CI / build (pull_request) Successful in 3m3s

Details

CI / container-images (pull_request) Has been skipped

Details

CI / e2e (pull_request) Failing after 2m30s

Details

31c2d78e49

Rob found the drag-zoom gesture backwards, and recentring on the tap
point instead of anchoring under the finger, on a real phone. These
tests read Leaflet's own API through a new element.placeMarkMap hook
to prove both geometrically; the fix follows in the next commit.

rob added 1 commit

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

Correct the drag-zoom gesture's direction and anchor (task 230)

CI / build (pull_request) Successful in 2m50s

Details

CI / container-images (pull_request) Has been skipped

Details

CI / e2e (pull_request) Successful in 2m12s

Details

1a2f9b1a42

Invert the drag sign per Rob's own device test, and measure the anchor
delta from the container's centre point rather than the gesture's own
start point, so the map no longer recentres on the tapped point.

rob reviewed

2026-08-14 14:30:10 +00:00

rob left a comment

Verdict: mergeable

Head SHA reviewed: 1a2f9b1a42eb380d9fe1fe67a81827bc71b69fbb.

Red run checked and it failed for the right reason. Run #586 (job 733→ no — internal run id 678, index_in_repo 586, head_sha 31c2d78e confirmed matching the test-only commit): build succeeded, e2e failed 5/19. All five failures are exactly the two bugs this PR fixes, nothing else:

AtGestureStart_LeavesTheMapsCentreWhereItWas / AsTheDragProceeds_KeepsTheTappedPointUnderTheFinger: distance ~169.5px / ~169.7px vs the asserted <40px/<5px — the recentring bug, reproduced.
DraggedDown_IncreasesZoom / DraggedUp_DecreasesZoom: zoom moved the wrong way (1 vs expected >2, 3 vs expected <2) — the inverted direction, reproduced.
DraggedUpPastTheMinimumZoom_ZoomsOutToTheBoundary (the renamed boundary test): dragging up never disabled the zoom-out control, because on the unfixed handler dragging up still zoomed in — same direction bug, different symptom.

No compile error, no timeout, no unrelated flake, and element.placeMarkMap was demonstrably present and working in this run (the tests read real getZoom()/getCenter() numbers to fail the way they did — a missing hook would throw, not mismatch by a clean 169px). Run #587 (fix commit, 1a2f9b1a...) is green, e2e 19/19, build's per-project counts match the PR body exactly (Domain 12, Contracts 139, Architecture 123, WebUI 755, Infrastructure 321, Api 605 = 1955).

One inaccuracy in the PR body/Vikunja comment: "20 E2E facts" — actual is 19, confirmed straight from run #587's own e2e job log (Passed: 19, Total: 19), and cross-checked against main's baseline (15 facts at merge-base 1a94135) plus this PR's net +4 new [E2EFact] methods (one renamed, four added) = 19. Worth a one-line fix to the PR description before merging; not a code defect.

Anchor algebra verified against the actual vendored bundle, not just ADR-0146's own proof. Pulled Map.TouchZoom._onTouchStart/_onTouchMove from leaflet.js directly: it captures _centerPoint = getSize()/2 once, and computes delta from that same _centerPoint, exactly as this PR's _onTouchMove now does. Reduced the algebra independently (not just read the ADR's): with point == startPoint and zoom == startZoom, the corrected formula reduces to the map's pre-gesture getCenter() exactly; for any point/zoom during the drag, containerPointToLatLng(point) reduces to _startLatLng exactly — the invariant holds generally, not only at t=0. The previous formula (delta from _startPoint) reduces to _startLatLng only when point == startPoint, i.e. only at gesture start, and drifts as an off-centre tap moves — matches why the new tests deliberately tap 120px off-centre.

Direction: sign flip confirmed correct (point.y - startPoint.y, drag down = positive = zoom in). ADR-0143 annotated correctly and minimally — only its Status and Partially superseded by metadata fields changed, nothing in Context/Decision/Consequences touched, matching ADR-0107's immutable-body rule.

Test discrimination: confirmed by the actual red run, not just by inspection — a zoom-level-only regression couldn't pass the anchor tests (they assert pixel distance independently of zoom), and the anchor tests alone couldn't pass with direction still wrong, since the boundary test's direction is asserted separately. A partial fix (either alone) would still redden at least one of the five.

element.placeMarkMap: additive, inert, scoped to the host element, consistent with the existing ad hoc map.placeMark* property convention already in this file. Acceptable.

No regression found in click-suppression takeover, the one-step double-tap zoom (still exercised at the end of the boundary test), or task 228's click deferral — diffed the full map.js and none of that code moved.

docs/adr/README.md diff is exactly the two index-table edits (0143's status text, new 0146 row); ADR-0146 numbering is clear of 0145 (reserved, unmerged, task 229) and the rest of 0001–0146. British English consistent; code identifiers mirroring Leaflet's own American-spelled internals (_centerPoint, getCenter) are the documented exception, not an oversight. No AI attribution.

Verdict: mergeable Head SHA reviewed: `1a2f9b1a42eb380d9fe1fe67a81827bc71b69fbb`. **Red run checked and it failed for the right reason.** Run #586 (job 733→ no — internal run id 678, index_in_repo 586, `head_sha` 31c2d78e confirmed matching the test-only commit): `build` succeeded, `e2e` failed 5/19. All five failures are exactly the two bugs this PR fixes, nothing else: - `AtGestureStart_LeavesTheMapsCentreWhereItWas` / `AsTheDragProceeds_KeepsTheTappedPointUnderTheFinger`: distance ~169.5px / ~169.7px vs the asserted <40px/<5px — the recentring bug, reproduced. - `DraggedDown_IncreasesZoom` / `DraggedUp_DecreasesZoom`: zoom moved the wrong way (1 vs expected >2, 3 vs expected <2) — the inverted direction, reproduced. - `DraggedUpPastTheMinimumZoom_ZoomsOutToTheBoundary` (the renamed boundary test): dragging up never disabled the zoom-out control, because on the unfixed handler dragging up still zoomed *in* — same direction bug, different symptom. No compile error, no timeout, no unrelated flake, and `element.placeMarkMap` was demonstrably present and working in this run (the tests read real `getZoom()`/`getCenter()` numbers to fail the way they did — a missing hook would throw, not mismatch by a clean 169px). Run #587 (fix commit, `1a2f9b1a...`) is green, `e2e` 19/19, `build`'s per-project counts match the PR body exactly (Domain 12, Contracts 139, Architecture 123, WebUI 755, Infrastructure 321, Api 605 = 1955). **One inaccuracy in the PR body/Vikunja comment: "20 E2E facts" — actual is 19**, confirmed straight from run #587's own `e2e` job log (`Passed: 19, Total: 19`), and cross-checked against main's baseline (15 facts at merge-base `1a94135`) plus this PR's net +4 new `[E2EFact]` methods (one renamed, four added) = 19. Worth a one-line fix to the PR description before merging; not a code defect. **Anchor algebra verified against the actual vendored bundle**, not just ADR-0146's own proof. Pulled `Map.TouchZoom._onTouchStart`/`_onTouchMove` from `leaflet.js` directly: it captures `_centerPoint = getSize()/2` once, and computes `delta` from that same `_centerPoint`, exactly as this PR's `_onTouchMove` now does. Reduced the algebra independently (not just read the ADR's): with `point == startPoint` and `zoom == startZoom`, the corrected formula reduces to the map's pre-gesture `getCenter()` exactly; for any `point`/`zoom` during the drag, `containerPointToLatLng(point)` reduces to `_startLatLng` exactly — the invariant holds generally, not only at t=0. The previous formula (delta from `_startPoint`) reduces to `_startLatLng` only when `point == startPoint`, i.e. only at gesture start, and drifts as an off-centre tap moves — matches why the new tests deliberately tap 120px off-centre. **Direction**: sign flip confirmed correct (`point.y - startPoint.y`, drag down = positive = zoom in). ADR-0143 annotated correctly and minimally — only its `Status` and `Partially superseded by` metadata fields changed, nothing in Context/Decision/Consequences touched, matching ADR-0107's immutable-body rule. **Test discrimination**: confirmed by the actual red run, not just by inspection — a zoom-level-only regression couldn't pass the anchor tests (they assert pixel distance independently of zoom), and the anchor tests alone couldn't pass with direction still wrong, since the boundary test's direction is asserted separately. A partial fix (either alone) would still redden at least one of the five. **`element.placeMarkMap`**: additive, inert, scoped to the host element, consistent with the existing ad hoc `map.placeMark*` property convention already in this file. Acceptable. **No regression found** in click-suppression takeover, the one-step double-tap zoom (still exercised at the end of the boundary test), or task 228's click deferral — diffed the full `map.js` and none of that code moved. `docs/adr/README.md` diff is exactly the two index-table edits (0143's status text, new 0146 row); ADR-0146 numbering is clear of 0145 (reserved, unmerged, task 229) and the rest of 0001–0146. British English consistent; code identifiers mirroring Leaflet's own American-spelled internals (`_centerPoint`, `getCenter`) are the documented exception, not an oversight. No AI attribution.

rob merged commit ef9220885f into main

2026-08-14 14:30:57 +00:00

rob deleted branch fix/task-230-drag-zoom-direction-and-anchor

2026-08-14 14:30:57 +00:00

rob referenced this pull request from a commit

2026-08-14 14:30:58 +00:00

Fix drag-zoom gesture direction and recentring (#162)