Skip to content

fix(guidance): Stop button drops taps because render() rebuilds it on every GPS fix (#179)#180

Merged
jasoneplumb merged 1 commit intomainlinefrom
fix/guidance-event-delegation
Apr 26, 2026
Merged

fix(guidance): Stop button drops taps because render() rebuilds it on every GPS fix (#179)#180
jasoneplumb merged 1 commit intomainlinefrom
fix/guidance-event-delegation

Conversation

@jasoneplumb
Copy link
Copy Markdown
Owner

Summary

  • updateGuidance() is called from onLocationFound on every accepted GPS fix (~1 Hz on a moving phone). Its last action is render(), which does panelEl.innerHTML = '' and re-creates the Stop button (via appendButton).
  • iOS Safari's touch-to-click synthesis tracks the specific DOM element that received touchstart. If that element is destroyed before touchend arrives, no click is fired. With GPS fixes faster than typical tap-and-release timing, almost every tap on the Stop button lands in the rebuild window and is silently dropped.

Fix

Switch to event delegation: a single persistent click listener attached once to panelEl at control creation. The listener checks event.target.closest('.guidance-btn') and calls onStop when matched. Because panelEl itself never gets destroyed (only its inner HTML), the listener survives every render and the button can come and go freely.

Also adds type="button" to the buttons defensively.

Closes #179.

Why event delegation is the right shape

  • The panel re-renders aren't going away — distance, ETA, current step legitimately change every fix.
  • A surgical "update text content but keep buttons" refactor would work but is more code than the bug warrants.
  • Delegation is one line, lives in addGuidanceControl once, and can never desynchronise.

Test plan

  • npm run type-check clean
  • npm run lint clean
  • npm test clean (633 tests)
  • Manual: long-press → Navigate → guiding state → Stop responds reliably (no need to tap multiple times)
  • Manual: search → "Navigate here" → guiding state → Stop responds reliably
  • Regression: routing-state Cancel still works (delegation covers both)

🤖 Generated with Claude Code

… mid-tap

updateGuidance() is called on every GPS fix (~1 Hz on a moving
phone) and ends with render(), which does panelEl.innerHTML = ''
and recreates the Stop button. iOS Safari tracks the specific DOM
element that received touchstart — if it's removed before touchend,
no click is synthesized and the tap is silently dropped. With GPS
fixes coming faster than typical tap timing, almost every tap lands
in the rebuild window.

Switch to event delegation: a single persistent click listener on
panelEl that checks .closest('.guidance-btn') and calls onStop.
The listener survives every render; buttons can come and go without
breaking the touch path.

Also adds type="button" to the buttons defensively.

Closes #179

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jasoneplumb jasoneplumb added bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review labels Apr 26, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Code Review

Overview

This PR fixes a genuine iOS Safari bug: render() is called at ~1 Hz (every GPS fix) and rebuilds the Stop/Cancel button each time. iOS touch synthesis tracks the specific DOM node that received touchstart; if that node is replaced before touchend, the click never fires. The fix is event delegation on the persistent panelEl, so button recreation is harmless. The root cause diagnosis and fix are both correct.


What's Good

  • Minimal and surgical. 7 additions, 1 deletion — exactly as large as the fix warrants, no incidental cleanup.
  • Correct Leaflet API usage. L.DomEvent.on(c, 'click', ...) is consistent with how the rest of the file attaches events and ensures Leaflet's event normalization applies.
  • type="button" is a solid defensive addition. Prevents accidental form submission if this ever ends up inside a <form> element.
  • Delegation covers Cancel too. The original appendButton called btn.addEventListener('click', onStop) for both Stop and Cancel buttons — the delegation preserves that behavior correctly.
  • Comment in appendButton explains the absence, which is better than leaving future readers wondering why no listener is attached.

Suggestions / Minor Notes

1. Optional: guard e.target against null

L.DomEvent.on(c, 'click', (e: Event) => {
  if ((e.target as HTMLElement | null)?.closest('.guidance-btn')) onStop();
});

In practice, a click event always has an HTMLElement target inside panelEl, so the cast is safe as written. This is a style/defensiveness nit only — not a correctness concern.

2. Only onStop is reachable via delegation

Both the Stop button (guiding state) and the Cancel button (routing state) have class guidance-btn, so both correctly route to onStop. This matches the pre-fix behavior. Worth noting: if a future button in appendButton ever needs a different action, the delegation pattern will need to be extended (e.g. a data-action attribute). Not a problem now, just something to keep in mind if the guidance panel grows.

3. No new automated tests

The project's test philosophy (pure functions only, no DOM/Leaflet integration) means this is expected and acceptable. The manual test plan in the PR description covers it appropriately.


Verdict

LGTM. The fix is correct, the approach is idiomatic, and it's the right size for the problem. The iOS tap-drop bug is a real class of issue in heavily re-rendering UIs and event delegation is the canonical solution. Ready to merge pending the manual test plan items.

@jasoneplumb jasoneplumb merged commit 905c5b4 into mainline Apr 26, 2026
3 checks passed
@jasoneplumb jasoneplumb deleted the fix/guidance-event-delegation branch April 26, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(guidance): Stop button unresponsive — render() destroys button mid-tap on every GPS fix

1 participant