configurable drag pan threshold #6809
Conversation
|
Hi @msbarry thanks for submitting this PR! I added a few comments but overall I think it looks pretty good. |
| @@ -663,3 +663,81 @@ test('DragPanHandler does not begin a drag on spurious touchmove events', (t) => | |||
| map.remove(); | |||
| t.end(); | |||
| }); | |||
|
|
|||
| test('DragPanHandler does not beging a mouse drag if moved less than threshold', (t) => { | |||
There was a problem hiding this comment.
Minor nitpick but there's a typo in begin
| t.end(); | ||
| }); | ||
|
|
||
| test('DragPanHandler does not beging a touch drag if moved less than threshold', (t) => { |
debug/drag_threshold.html
Outdated
| @@ -0,0 +1,32 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
I don't think it's necessary to create a separate debug page for this since it's really just a simple option.
src/ui/map.js
Outdated
| @@ -170,6 +172,8 @@ const defaultOptions = { | |||
| * bearing will snap to north. For example, with a `bearingSnap` of 7, if the user rotates | |||
| * the map within 7 degrees of north, the map will automatically snap to exact north. | |||
| * @param {boolean} [options.pitchWithRotate=true] If `false`, the map's pitch (tilt) control with "drag to rotate" interaction will be disabled. | |||
| * @param {number} [options.dragThreshold=0] The threshold, measured in pixels, that determines how far a drag must be before it is considered a drag | |||
There was a problem hiding this comment.
I have two related suggestions here:
-
I think
clickTolerancemight be a more accurate name for this option. For one thing, that's what this option is called in Leaflet (even though it's part of theDraggableclass) which might help people who are migrating to GL-JS or searching for a solution to this problem. Additionally, the original issue for this PR was how to get a click event instead of a drag event when the shift was only a couple pixels, so really this is about setting a tolerance limit around the click event to prevent it turning into a drag. -
I think the description could be clearer. Consider how Leaflet describes
clickTolerance: "The max number of pixels a user can shift the mouse pointer during a click for it to be considered a valid click (as opposed to a mouse drag)." You may want to make it clear in your description that the value of the option is the max number inclusive since you're checking for<=while Leaflet just checks<.
test/unit/ui/map_events.test.js
Outdated
| @@ -553,3 +553,35 @@ test(`Map#on mousedown doesn't fire subsequent click event if mousepos changes`, | |||
| map.remove(); | |||
| t.end(); | |||
| }); | |||
|
|
|||
| test(`Map#on mousedown fires subsequent click event if mousepos changes less than drag threshold`, (t) => { | |||
There was a problem hiding this comment.
Minor again, but I'd spell out mouse position instead of mousePos in these two test descriptions.
|
Thanks @ryanhamley! I renamed the option to What do you think for the default value? Leaflet uses 3, do you think we should switch to that or retain existing behavior for users who don't set the option? Also, should we apply the threshold to other built-in handlers that look for drags like box zoom and drag rotate? It seems less critical since if you control or shift click it doesn't fire a click event so the drag handlers don't need to differentiate from a click. |
|
Awesome PR! No one complained about Leaflet's default in years, so I'd vote for |
|
Switching the default from 0 to 3 would be a breaking change in the API but I'm not sure if there's really a use case where you'd depend on this value being 0 so it seems like a fairly safe breaking change to me. It doesn't seem like Leaflet uses any type of tolerance detection in |
|
Updated default click tolerance to 3px. Let me know what you decide about box zoom, I think it's fairly straightforward to add if needed. |
|
I think using the same threshold for box zoom is a good idea. It was requested in #6132. |
|
Sounds good. I applied click tolerance to box zoom handler as well. |
This is an attempt to fix #1832. I added a
dragThresholdoption which lets you set the threshold above which a drag is considered a drag and below which a drag is considered a click. It defaults to 0, which retains the existing behavior. This is my first pull request here, so let me know if there's a better way to do this or anything that I'm missing, thanks!@mapbox/studioand/or@mapbox/maps-designif this PR includes style spec changes