Fix double-tap zoom (#6217) #8086
Conversation
src/ui/handler/dblclick_zoom.js
Outdated
| }; | ||
|
|
||
| this._map.on('touchend', onTouchEnd); | ||
| this._map.on('touchcancel', onTouchCancel); |
There was a problem hiding this comment.
To simplify this a bit, you can do this._map.once(...) instead of on — this way listener doesn't have to manually remove itself. It also ensures touchcancel listeners are not accumulated if they never happen.
There was a problem hiding this comment.
Ah, disregard the last sentence — it seems like we still need to make sure touchcancel listeners don't accumulate with every tap when touches are not cancelled.
There was a problem hiding this comment.
👍 thanks for catching the missing touchcancel removal! I've now used .once() for both listeners and still manually remove the other listener in each handler, so I think all bases should be covered.
|
Thanks for the reviews @mourner @asheemmamoowala ! I'll take all your comments into account & push a new set of changes that incorporates both. |
- Use proper Point obj w/ .dist() method - Remove touchcancel handler on touchend - Attach touchend/cancel handlers with .once() - Move distance threshold to top-level constant, add doc comment - Refactor tests to avoid repetition
Hi folks! Per chat with @asheemmamoowala I'm jumping in on a
good first issue, trying to fix the double-tap zoom issues described in #6217. Thanks in advance for the feedback 🙏Changes:
touchendevent (nottouchstart) of double-tap within the time threshold, & add testsKnown issues/questions:
maxDeltabetween tap locations (in both x & y) to 30, rather arbitrarily based on my manual tests - happy to use a different value if there is some standard for this.Launch Checklist
document any changes to public APIsN/A afaict