Weigh a deferred click's position, and cancel it on a marker drag (task 229) #165

Merged

rob merged 7 commits from fix/task-229-deferred-click-position-and-drag into main 2026-08-14 18:52:13 +00:00

Conversation 2 Commits 7 Files changed 6 +485 -31

rob commented 2026-08-14 16:34:25 +00:00

Owner

Copy link

Closes task 229 — the two gaps review of task 228 (PR #161, ADR-0144) left out.

Gap 2: a picker marker's dragstart/dragend now cancel a pending click. Previously, tapping the picker's map to place the pin then immediately dragging it to adjust could see the tap's own deferred callback (ADR-0144) fire mid-drag or after, overwriting the dragged position with the stale tapped coordinate. Cancelled on dragstart (closes the mid-drag window) and dragend (defence in depth).

Gap 1: deferSingleClick's second-click cancellation now checks position, not only timing. A second click within DOUBLE_TAP_WINDOW_MS only cancels the pending click it lands within CLICK_CANCEL_SLOP_PIXELS (5, GTK's own gtk-double-click-distance default) of — a second click further away is deferred in its own right, rather than both being silently discarded.

ADR-0145 records both, and partially supersedes ADR-0144's own claim that a marker drag "has no ambiguity... to be confused with."

Both gaps now proven with real Chromium input, watched failing first.

• MarkerDragCancelsPendingClickJourneyTests — an initial version raced click-then-drag against the 300ms window and passed even with the fix reverted, because dragend's own report always lands last regardless of cancellation; rewritten to hold the drag deliberately past the deferral window before releasing, checking the state a reader would see mid-drag. Reddened against the real defect (await latitudeField.InputValueAsync() should be "38.272688535980976" but was "38.273510817496465" — the seeded coordinate silently overwritten mid-hold).
• DistantMapClicksBothSurviveJourneyTests (added on review: gap 1 had no real-input E2E case, and unlike this file's other "reasoned, not reproduced" gaps, nothing here needed CDP touch — two ordinary ILocator.ClickAsync clicks prove it directly). Watched failing against a temporarily reverted distance check: Locator expected to be visible... waiting for Locator("#place-form-latitude") — the add-place panel never opened at all, both clicks having cancelled each other with nothing scheduled for either.

docs/adr/README.md's index is regenerated; diff outside the 0144/0145 rows is empty. Rebased onto current main (task 226/223 merges); no conflicts.

Test plan

CI run #697, pinned SDK 10.0.100, head 13b70c1:

• build: PlaceMark.Domain.Tests 12, PlaceMark.Contracts.Tests 139, PlaceMark.Architecture.Tests 123, PlaceMark.WebUI.Tests 751 passed/4 skipped/755 total, PlaceMark.Infrastructure.Tests 321, PlaceMark.Api.Tests 605 — all passed.
• e2e: 22 passed, 0 failed, 0 skipped.
• dotnet format --verify-no-changes clean.

Closes task 229 — the two gaps review of task 228 (PR #161, ADR-0144) left out. **Gap 2: a picker marker's `dragstart`/`dragend` now cancel a pending click.** Previously, tapping the picker's map to place the pin then immediately dragging it to adjust could see the tap's own deferred callback (ADR-0144) fire mid-drag or after, overwriting the dragged position with the stale tapped coordinate. Cancelled on `dragstart` (closes the mid-drag window) and `dragend` (defence in depth). **Gap 1: `deferSingleClick`'s second-click cancellation now checks position, not only timing.** A second click within `DOUBLE_TAP_WINDOW_MS` only cancels the pending click it lands within `CLICK_CANCEL_SLOP_PIXELS` (5, GTK's own `gtk-double-click-distance` default) of — a second click further away is deferred in its own right, rather than both being silently discarded. ADR-0145 records both, and partially supersedes ADR-0144's own claim that a marker drag "has no ambiguity... to be confused with." **Both gaps now proven with real Chromium input, watched failing first.** - `MarkerDragCancelsPendingClickJourneyTests` — an initial version raced click-then-drag against the 300ms window and passed even with the fix reverted, because `dragend`'s own report always lands last regardless of cancellation; rewritten to hold the drag deliberately past the deferral window before releasing, checking the state a reader would see mid-drag. Reddened against the real defect (`await latitudeField.InputValueAsync() should be "38.272688535980976" but was "38.273510817496465"` — the seeded coordinate silently overwritten mid-hold). - `DistantMapClicksBothSurviveJourneyTests` (added on review: gap 1 had no real-input E2E case, and unlike this file's other "reasoned, not reproduced" gaps, nothing here needed CDP touch — two ordinary `ILocator.ClickAsync` clicks prove it directly). Watched failing against a temporarily reverted distance check: `Locator expected to be visible... waiting for Locator("#place-form-latitude")` — the add-place panel never opened at all, both clicks having cancelled each other with nothing scheduled for either. `docs/adr/README.md`'s index is regenerated; diff outside the 0144/0145 rows is empty. Rebased onto current `main` (task 226/223 merges); no conflicts. ## Test plan CI run [#697](https://git.robware.uk/rob/PlaceMark/actions/runs/605), pinned SDK 10.0.100, head `13b70c1`: - `build`: `PlaceMark.Domain.Tests` 12, `PlaceMark.Contracts.Tests` 139, `PlaceMark.Architecture.Tests` 123, `PlaceMark.WebUI.Tests` 751 passed/4 skipped/755 total, `PlaceMark.Infrastructure.Tests` 321, `PlaceMark.Api.Tests` 605 — all passed. - `e2e`: 22 passed, 0 failed, 0 skipped. - `dotnet format --verify-no-changes` clean.

rob added 1 commit 2026-08-14 16:34:25 +00:00

Add a failing E2E test for a picker marker drag racing a pending click ... 97562c98c5

Task 229: proves gap 2 (stale deferred click overwriting a dragged marker) before the map.js fix lands.

rob added 1 commit 2026-08-14 16:56:02 +00:00

Assert the marker-drag test against the held mid-drag state, not the end ... b0f60e35b6

The first version raced click-then-drag against a 300ms window and passed even with the fix reverted, because dragend's own unconditional final write always lands last. Holding the drag deliberately past the deferral window before releasing checks the state the bug actually corrupts.

rob added 1 commit 2026-08-14 17:09:35 +00:00

Use Hover, not a manually-captured box, to position the drag start ... 3115e4f28b

The previous run's own failure showed the drag never registered at all (the field never changed even after mouseup) — a stale, possibly pre-settle bounding-box read was the likely cause; HoverAsync computes the marker's current point itself.

rob added 1 commit 2026-08-14 17:17:35 +00:00

Weigh a deferred click's position, and cancel it on a marker drag ... 2be052302d

Task 229, ADR-0145: a picker's own marker drag now cancels a pending click on dragstart and dragend, and a second click only cancels the pending one it lands near, so two distant deliberate clicks no longer both vanish.

rob changed title from Fix two gaps in the deferred map click (task 229) to Weigh a deferred click's position, and cancel it on a marker drag (task 229) 2026-08-14 17:24:18 +00:00

rob reviewed 2026-08-14 18:18:00 +00:00

rob left a comment

Copy link

Verdict: mergeable

Reviewed at head 2be0523 (matches PR's own reported head).

Red-run claim, verified against job logs, not the diff. Forgejo's run-id-vs-html_url numbering is inconsistent here (querying /actions/runs/595 directly returns an unrelated 2026-08-12 run whose own html_url is runs/503, even though the tool that reported #687's URL said runs/595) — matched on head_sha instead, per the brief. Run #687 (b0f60e3, e2e job 1036) failed on the final assertion (Not.ToHaveValueAsync — the drag itself hadn't landed, an unrelated locator bug), not the guarded defect. Run #689 (3115e4f, job 1042) is the one that actually reddens on the claimed line: await latitudeField.InputValueAsync() should be "38.272688535980976" but was "38.273510817496465", at MarkerDragCancelsPendingClickJourneyTests.cs:108 — confirmed map.js at that commit still has the pre-fix, undeferred dragend handler (leafletMarker.on("dragend", () => reportPick(...)), no cancellation), so this is a genuine failure against unfixed code, not a stale/misleading status. Also confirmed the first test version (97562c9) asserts only the settled value after a back-to-back click-then-drag-then-wait — exactly the shape that lets a marker's unconditional dragend write win regardless of cancellation, and that version's run (#685) reports success against the same unfixed code, corroborating the "passed even reverted" claim. The lesson is real, not merely reasoned.

Suite totals in the PR body match job 1044/1045's logs exactly (12/139/123/751+4/321/605, e2e 21) — genuinely re-derived, not carried over.

map.js logic review (deferSingleClick/cancelPendingClick/toLeafletMarkers): the pending-clicks-as-list redesign is correct — cancelPendingClick iterates and clears every entry (not just the nearest), so dispose() still fully drains both channels with no leak; the timeout callback removes only its own entry by identity (indexOf(entry)), so two independently-pending clicks at different points each fire on their own schedule without clobbering each other's bookkeeping. Marker dragstart/dragend both cancel both channels unconditionally, which is harmless (the inactive channel's cancelPendingClick is a no-op).

Gap 1 (positional check) has no E2E coverage, and I don't think that's the same problem as this file's three prior "reasoned, not reproduced" burns. Those were all cases of a genuine environmental limitation (CDP touch input not reaching a DOM listener) forcing an untested claim. Two real mouse clicks at different screen positions have no such limitation — Playwright's ClickAsync at two positions is entirely reliable here, so this gap is closer to "didn't write the test" than "couldn't." That said, the ticket explicitly permitted recording gap 1 as a judged, non-blocking decision rather than closing it, and ADR-0145 does exactly that, with reasoning specific to why it wasn't added (no existing journey shape touches it, would need new plumbing). I'd take this as a good candidate for a quick follow-up rather than a blocker for this PR — the multi-pending-click list is a real behavioural change on every click and would benefit from a positive-case E2E proof, even a narrow one.

ADR annotations, README index diff, and British English all check out.

Verdict: mergeable Reviewed at head `2be0523` (matches PR's own reported head). **Red-run claim, verified against job logs, not the diff.** Forgejo's run-`id`-vs-`html_url` numbering is inconsistent here (querying `/actions/runs/595` directly returns an unrelated 2026-08-12 run whose own `html_url` is `runs/503`, even though the tool that reported #687's URL said `runs/595`) — matched on `head_sha` instead, per the brief. Run #687 (`b0f60e3`, `e2e` job 1036) failed on the *final* assertion (`Not.ToHaveValueAsync` — the drag itself hadn't landed, an unrelated locator bug), not the guarded defect. Run #689 (`3115e4f`, job 1042) is the one that actually reddens on the claimed line: `await latitudeField.InputValueAsync() should be "38.272688535980976" but was "38.273510817496465"`, at `MarkerDragCancelsPendingClickJourneyTests.cs:108` — confirmed `map.js` at that commit still has the pre-fix, undeferred `dragend` handler (`leafletMarker.on("dragend", () => reportPick(...))`, no cancellation), so this is a genuine failure against unfixed code, not a stale/misleading status. Also confirmed the first test version (`97562c9`) asserts only the settled value after a back-to-back click-then-drag-then-wait — exactly the shape that lets a marker's unconditional `dragend` write win regardless of cancellation, and that version's run (#685) reports `success` against the same unfixed code, corroborating the "passed even reverted" claim. The lesson is real, not merely reasoned. Suite totals in the PR body match job 1044/1045's logs exactly (12/139/123/751+4/321/605, e2e 21) — genuinely re-derived, not carried over. **`map.js` logic review** (`deferSingleClick`/`cancelPendingClick`/`toLeafletMarkers`): the pending-clicks-as-list redesign is correct — `cancelPendingClick` iterates and clears every entry (not just the nearest), so `dispose()` still fully drains both channels with no leak; the timeout callback removes only its own entry by identity (`indexOf(entry)`), so two independently-pending clicks at different points each fire on their own schedule without clobbering each other's bookkeeping. Marker `dragstart`/`dragend` both cancel both channels unconditionally, which is harmless (the inactive channel's `cancelPendingClick` is a no-op). **Gap 1 (positional check) has no E2E coverage, and I don't think that's the same problem as this file's three prior "reasoned, not reproduced" burns.** Those were all cases of a genuine environmental limitation (CDP touch input not reaching a DOM listener) forcing an untested claim. Two real mouse clicks at different screen positions have no such limitation — Playwright's `ClickAsync` at two positions is entirely reliable here, so this gap is closer to "didn't write the test" than "couldn't." That said, the ticket explicitly permitted recording gap 1 as a judged, non-blocking decision rather than closing it, and ADR-0145 does exactly that, with reasoning specific to why it wasn't added (no existing journey shape touches it, would need new plumbing). I'd take this as a good candidate for a quick follow-up rather than a blocker for this PR — the multi-pending-click list is a real behavioural change on every click and would benefit from a positive-case E2E proof, even a narrow one. ADR annotations, README index diff, and British English all check out.

rob force-pushed fix/task-229-deferred-click-position-and-drag from 2be052302d to 3c9c630109 2026-08-14 18:20:51 +00:00 Compare

rob added 1 commit 2026-08-14 18:23:50 +00:00

Add a failing E2E test proving gap 1's positional check with real clicks ... 96a01f97ca

Temporarily neuters the distance check in deferSingleClick, matching the pre-ADR-0145 shape, so the new test reddens against a real regression rather than an inference. Restored in the next commit.

rob added 1 commit 2026-08-14 18:34:23 +00:00

Restore the positional check, now proven by a real two-click test ... a5edf91c05

The previous commit's temporary neutering reddened against DistantMapClicksBothSurviveJourneyTests exactly as expected: the add-place panel never opened at all, because both clicks cancelled each other with nothing scheduled for either.

rob added 1 commit 2026-08-14 18:42:00 +00:00

Correct ADR-0145's own claim that gap 1 has no E2E proof ... 13b70c1577

It now does — DistantMapClicksBothSurviveJourneyTests, watched failing against a temporarily reverted check first, per review.

rob reviewed 2026-08-14 18:51:52 +00:00

rob left a comment

Copy link

Verdict: mergeable

Confirmation pass at new head 13b70c1 (matches PR's own reported head).

Job 1057's log genuinely shows the discarded-both defect, not a locator bug. Matched run 694 to head_sha 96a01f9 (not to any id-derived URL, per the earlier lesson in this same PR). Failure: Locator expected to be visible... waiting for Locator("#place-form-latitude") at line 75 of the new test — the same #place-form-latitude locator dozens of other passing tests (including this PR's own MarkerDragCancelsPendingClickJourneyTests) use successfully, so the selector itself isn't in question. Confirmed map.js at 96a01f9 has the distance check itself stubbed to pending.length > 0 ? 0 : -1 (any pending entry matches, regardless of position) rather than an unrelated typo elsewhere — a targeted reversion of exactly the thing under test, producing exactly the "both clicks cancel each other, neither schedules" failure mode the PR describes.

Discriminates the shipped code, not just the stub. main at 118e1bd still has ADR-0144's original single-slot deferSingleClick — any second click within the window unconditionally clears the pending slot and returns, scheduling nothing — behaviourally identical to the 96a01f9 stub for this scenario. So the new test would redden against current main too, not only against a straw-man revert.

ADR-0145 correction is accurate. The "Gap 1 has no equivalent real-input E2E coverage" paragraph is replaced with one correctly describing DistantMapClicksBothSurviveJourneyTests, its watched-red run, and the exact failure message — matches the job log. No other paragraph in the record still claims an absent proof.

Rebase disturbed nothing. src/PlaceMark.WebUI/wwwroot/js/map.js, docs/adr/0144-...md, and MarkerDragCancelsPendingClickJourneyTests.cs are byte-identical to the previously-reviewed head (2be0523) — my earlier findings on the list-based pending-click redesign stand unchanged. docs/adr/README.md's diff is still confined to the 0144/0145 rows.

Verdict: mergeable Confirmation pass at new head `13b70c1` (matches PR's own reported head). **Job 1057's log genuinely shows the discarded-both defect, not a locator bug.** Matched run 694 to `head_sha` 96a01f9 (not to any id-derived URL, per the earlier lesson in this same PR). Failure: `Locator expected to be visible... waiting for Locator("#place-form-latitude")` at line 75 of the new test — the same `#place-form-latitude` locator dozens of other passing tests (including this PR's own `MarkerDragCancelsPendingClickJourneyTests`) use successfully, so the selector itself isn't in question. Confirmed `map.js` at `96a01f9` has the distance check itself stubbed to `pending.length > 0 ? 0 : -1` (any pending entry matches, regardless of position) rather than an unrelated typo elsewhere — a targeted reversion of exactly the thing under test, producing exactly the "both clicks cancel each other, neither schedules" failure mode the PR describes. **Discriminates the shipped code, not just the stub.** `main` at `118e1bd` still has ADR-0144's original single-slot `deferSingleClick` — any second click within the window unconditionally clears the pending slot and returns, scheduling nothing — behaviourally identical to the 96a01f9 stub for this scenario. So the new test would redden against current `main` too, not only against a straw-man revert. **ADR-0145 correction is accurate.** The "Gap 1 has no equivalent real-input E2E coverage" paragraph is replaced with one correctly describing `DistantMapClicksBothSurviveJourneyTests`, its watched-red run, and the exact failure message — matches the job log. No other paragraph in the record still claims an absent proof. **Rebase disturbed nothing.** `src/PlaceMark.WebUI/wwwroot/js/map.js`, `docs/adr/0144-...md`, and `MarkerDragCancelsPendingClickJourneyTests.cs` are byte-identical to the previously-reviewed head (`2be0523`) — my earlier findings on the list-based pending-click redesign stand unchanged. `docs/adr/README.md`'s diff is still confined to the 0144/0145 rows.

rob merged commit be2a075198 into main 2026-08-14 18:52:13 +00:00

rob deleted branch fix/task-229-deferred-click-position-and-drag 2026-08-14 18:52:13 +00:00

rob referenced this pull request from a commit 2026-08-14 18:52:15 +00:00

Weigh a deferred click's position, and cancel it on a marker drag (#165)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!165

No description provided.