-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve single vs. multi-touch zoom & pan interaction (#7196) #8100
Conversation
to reflect how we usually expect these events to look
- Stop an active single-touch drag if the user adds additional fingers and touchZoomRotate is enabled, so the latter can take over interaction - Start a single-touch drag if the user removes all but one finger - Add tests for expected single vs. multi-touch interactions
To test the behavior changes implemented here (cc @asheemmamoowala )
|
Ah just discovered the multi-to-single-touch interaction problem was ticketed in #6900, which this also fixes; updated description above. |
Slightly off-topic: does GL JS currently address the case of a two-finger pinch-to-zoom gesture where the averaged point between the fingers moves (a “wandering zoom”)? On a touchscreen, it’s quite difficult to keep that centerpoint stationary as you pinch your fingers or spread them apart. To maintain a responsive feel, it’s necessary to pan at the same time as zooming (though perhaps some use cases may call for a more precise gesture handler that locks the center while zooming). |
@1ec5 currently not, and it's been bothering me too — perhaps we should capture that in a ticket too. Leaflet does this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests 💯
LGTM except one nit.
src/ui/handler/drag_pan.js
Outdated
_onMultiTouchEnd(e: TouchEvent) { | ||
// A multi-finger touch is ending; decide whether to activate dragging | ||
|
||
if (e.touches && e.touches.length > 1) return; // there are still multiple fingers touching; wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this logic to something like isMultiTouch(e)
?
Thanks for the review @arindam1993! Did you by any chance try this out on a touch device, and if so did you have any issues with the new behavior as describe above (#8100 (comment))? Wondering if you ran into the same issues @asheemmamoowala had a while back. |
I did some testing on Android Chrome,
2 works as expected and is a big improvement imo. With respect to 1 though it feels like we've traded some light consistent jitteriness with a large 1 frame hitch. Maybe we're skipping one |
Thanks @arindam1993 !
Hm maybe these were the issues @asheemmamoowala alluded to. Wonder if it's possible we're dropping one handler a little early, such that there's a gap before the other handler takes over. Will try to investigate further, lmk if you want to pair on that @arindam1993 ! |
Additional tests/cases to capture the fact that we only expect touchZoomRotateHandler to take over once we've actually begun zoom/rotate gesture; two-finger straight pan should still be handled by dragPanHandler. See comment on #8100 for more details. Tests currently failing.
Capturing progress @arindam1993 & I made today: Arindam found a bug in the transition from one to two fingers, during which we hand off control from the The solution we came up with is to stop disabling |
Co-authored-by: Anjana Vakil <anjana.vakil@mapbox.com>
Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
To account for the fact that move event handlers might be inspecting the DragPanHandler's (in)active state, update the state in the onMove handler rather than in onDragFrame. However, don't fire the dragstart/movestart events immediately on the move event as we had been doing before; schedule these to be fired on the first drag frame. Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 2 small things, looks good otherwise!
Thanks for the ✔️ @arindam1993! Made one update per your comment. @asheemmamoowala did you want to 👀 this again or are we good to 🚢 ? |
Improves interaction when changing between single- and multi-finger touches, by checking for multi-touch events in
dragPan
handler and deactivating/reactivating that handler as appropriate iftouchZoomRotate
handler is active.Changes
touchend
/touchcancel
events to have an emptytouches
list by defaultLaunch Checklist
document any changes to public APIsN/Apost benchmark scoresN/A for these changes?cc @asheemmamoowala