Click Tolerance for Markers #9640

ChristopherChudzicki · 2020-04-28T14:19:58Z

Implements a clickTolerance option for Markers, as discussed in #9636. Click tolerance is optionally set on a per-marker basis. If not set, the map's click tolerance is used.

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
manually test the debug page
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

Changelog Entry:

Added `clickTolerance` as an option for `Marker`s. Pointer movement below `clickTolerance` does not fire `Marker` drag events.

This is so that Marker instances can access map._clickTolerance if the Markers do not have their own click tolerances.

If the pointer movement ever exceeds (or equals) click tolerance, then the movement fires drag events, otherwise it fires click events. Includes getClickTolerance and setClickTolerance methods. Marker clickTolerance is inherited from the map if not provided as an option.

ChristopherChudzicki · 2020-04-28T14:34:25Z

@ryanhamley I believe that you described this issue as a bug in #9624. It could also be considered a feature and a breaking change. (Would be non-breaking if the default tolerance was 0px instead of inherited from map). Anyway, I guess all bug fixes are breaking changes. I'll let you all decide what to do with it.

As also discussed in #9624, I think there should be a separate issue / PR to suppress map click events when dragging markers. Currently, if pointer movement is ABOVE the marker click tolerance but BELOW the map click tolerance, then marker fires drag events AND map fires click events. That seems undesirable. (This really is a separate issue from this PR, and is currently present on master, where marker click tolerance is effectively 0px.)

Thanks for considering this PR, and for mapbox!

PS: The commit history in this PR looks a little out of order and I do not know why. It is in the correct order on my fork: https://github.com/ChristopherChudzicki/mapbox-gl-js/commits/marker-clicktol

andrewharvey · 2020-04-29T03:11:33Z

src/ui/marker.js

@@ -36,6 +37,7 @@ type Options = {
 * @param {string} [options.color='#3FB1CE'] The color to use for the default marker if options.element is not provided. The default is light blue.
 * @param {number} [options.scale=1] The scale to use for the default marker if options.element is not provided. The default scale corresponds to a height of `41px` and a width of `27px`.
 * @param {boolean} [options.draggable=false] A boolean indicating whether or not a marker is able to be dragged to a new position on the map.
+ * @param {?number} [options.clickTolerance=null] A number specifying the click tolerance in pixels. For draggable markers, movement must exceed this tolerance to be treated as a drag. A `null` value inherits click tolerance from `Map`.


What is click tolerance here? Is it pixels away from the marker that are still counted as clicking the marker or is a small drag while holding the mouse down within the tolerance is still counted as a click and not a drag?

They are two different things and I can see users getting confused which one this affects.

I see from the changelog entry this would be the latter. I think we should make this clearer in the description to avoid people getting confused it's the former.

Yes, that is a good clarification. I do still think it should be called clickTolerance because that is the name used for the Map option. (The ambiguity you allude to doesn't really exist for the map, though.)

I'll change the jsdoc description here to mirror the map description more closely:

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).

ChristopherChudzicki · 2020-06-10T16:57:56Z

@ryanhamley any progress on this?

ryanhamley

Hi @ChristopherChudzicki thanks for submitting the PR and sorry for the delay in getting to it. This works but I believe the code could be simplified substantially to achieve the same effect. I've outlined a number of changes in specific code comments.

I'd also note that this behavior causes noticeable jumps when the click tolerance is more than a few pixels (in the gif below, the click tolerance is set to 100px). I'm not sure there's anyway around this if we desire this behavior, but I do want to make note of it because it begins to look like something is broken. @andrewharvey what do you think about this?

ryanhamley · 2020-06-10T19:24:09Z

src/ui/marker.js

@@ -36,6 +37,7 @@ type Options = {
 * @param {string} [options.color='#3FB1CE'] The color to use for the default marker if options.element is not provided. The default is light blue.
 * @param {number} [options.scale=1] The scale to use for the default marker if options.element is not provided. The default scale corresponds to a height of `41px` and a width of `27px`.
 * @param {boolean} [options.draggable=false] A boolean indicating whether or not a marker is able to be dragged to a new position on the map.
+ * @param {?number} [options.clickTolerance=null] The max number of pixels a user can shift the mouse pointer during a click on the marker for it to be considered a valid click (as opposed to a marker drag).


I don't think that we need to muck around with null and undefined values when 0 will work fine as a falsey value, especially since we want it to be a number. I also think it's misleading to say that the default tolerance is null or 0 when the default tolerance is actually whatever the map tolerance is, usually 3. We'll have to make a decision on what the default should be to more precisely word this.

I would love not to muck around with null/undefined types and I will plan to make this change. The reason I did not do it this way initially is that using 0 to mean "inherit from the map" means that the v1.11.0 behavior cannot be reproduced.

That said, the v1.11.0 behavior (where markers have 0 tolerance and map has 3px by default) seems undesirable to me, and people could always set the tolerance to be 0.00001.

ryanhamley · 2020-06-10T19:24:52Z

src/ui/marker.js

@@ -58,8 +60,11 @@ export default class Marker extends Evented {
    _scale: number;
    _defaultMarker: boolean;
    _draggable: boolean;
+    _clickTolerance: ?number;


If we change from null to 0, then we don't need this to be a Maybe type.

I belive this is resolved.

ryanhamley · 2020-06-10T19:25:20Z

src/ui/marker.js

@@ -86,6 +91,8 @@ export default class Marker extends Evented {
        this._color = options && options.color || '#3FB1CE';
        this._scale = options && options.scale || 1;
        this._draggable = options && options.draggable || false;
+        this._clickTolerance = options && options.clickTolerance || null;


null -> 0

I believe this is resolved.

ryanhamley · 2020-06-10T19:46:49Z

src/ui/marker.js

+    /**
+     * Returns the marker's click tolerance. If the marker's `clickTolerance`
+     * has not been set, then returns the associated `Map`'s click tolerance. If
+     * there is no associated `Map`, returns 0.
+     * @returns {number} the 's click tolerance.
+     */
+    getClickTolerance() {
+        if (this._clickTolerance !== null && this._clickTolerance !== undefined) {
+            return this._clickTolerance;
+        }
+        if (this._map) {
+            return this._map._clickTolerance;
+        }
+        return null;
+    }


Changing the value to 0 eliminates the need for this function. We can just check this._clickTolerance || this._map_clickTolerance in the _onMove function. If this._clickTolerance is 0, we'll fallback to the map click tolerance and otherwise we'll use the marker's click tolerance. I don't think we need the ability to change the clickTolerance outside of the constructor options so we should choose the simpler API by not adding new methods without a good reason.

Deleted this method.

ryanhamley · 2020-06-10T19:48:09Z

src/ui/marker.js

+        if (value === null || value === undefined) {
+            this._clickTolerance = null; // set to inherit from map.
+        }
+        const numeric = +value;
+        if (!Number.isNaN(numeric)) {
+            this._clickTolerance = numeric;
+        }


This is overly complex with the null/undefined checks. By using 0 for false, we can just make this this._clickTolerance = value and value no longer has to be a Maybe type.

Actually, going back to the discussion above, we may not need this at all. I'm not sure developers are really going to need/want to dynamically update the click tolerance after creation so we could probably remove this function entirely.

Deleted this method.

ryanhamley · 2020-06-10T19:48:23Z

src/ui/marker.js

+
+    /**
+     * Set the `Marker`'s click tolerance.
+     * @param {number} value the click tolerance in pixels


nit: value of the click tolerance in pixels

Deleted this method.

ryanhamley · 2020-06-10T19:49:41Z

src/ui/marker.js

+    _hasMovedBeyondClickTolerance(point: Point) {
+        const clickTolerance = this.getClickTolerance();
+        if (clickTolerance === null) return false;
+        return point.distSqr(this._pointerdownPos) >= clickTolerance ** 2;
+    }


This function is unnecessary. We can get the clickTolerance value as described in the getClickTolerance comment above and take the final line of this function (minus the return) and move it all directly into _onMove to simplify the API.

Deleted this method.

ryanhamley · 2020-06-10T19:51:40Z

src/ui/marker.js

+        if (!this._isDragging) {
+            this._isDragging = this._hasMovedBeyondClickTolerance(e.point);
+        }
+        if (!this._isDragging) return;
+


Rather than worrying about keeping the _isDragging boolean in check, we can just check for the click tolerance directly. What we're attempting to do is check if the tolerance has been exceeded and if so, return.

const clickTolerance = this._clickTolerance || this._map._clickTolerance; if (e.point.distSqr(this._pointerdownPos) <= clickTolerance ** 2) { return; }

@ryanhamley I think the _isDragging boolean is necessary. What we want to check is

"has pointer movement ever exceeded the threshhold while pointer down" not,

"is current pointer beyond threshhold".

The latter condition is checked by e.point.distSqr(this._pointerdownPos) <= clickTolerance ** 2) and results in jerky drag if the marker is dragged back to near where it started. The boolean lets us check the first condition.

ryanhamley · 2020-06-10T19:56:25Z

src/ui/marker.js

+    _hasMovedBeyondClickTolerance(point: Point) {
+        const clickTolerance = this.getClickTolerance();
+        if (clickTolerance === null) return false;
+        return point.distSqr(this._pointerdownPos) >= clickTolerance ** 2;


Why do we need to square these numbers?

Computes the squared distance from this vector to v. If you are just comparing the distance with another distance, you should compare the distance squared instead as it is slightly more efficient to calculate.

That's from the ThreeJS documetnation for distanceToSquared (for 3d webgl stuff). (Compared squared distances avoids square roots.) Obviously Mapbox isn't threejs, but when I saw that Mapbox Point objects have a distSqr method, I figured Mapbox encouraged similar squaring.

Even though the onMove handler is called frequently, this is very likely a premature optimization. I'm certainly happy to remove it.

ryanhamley · 2020-06-10T19:58:34Z

test/unit/ui/marker.test.js

@@ -329,6 +329,41 @@ test('Popup anchors around default Marker', (t) => {
    t.end();
 });

+test('Marker#getClickTolerance gets clickTolerance set via options', (t)  => {


The tests will need to be reworked to remove calls to the simpler API.

- Make the tolerance numeric instead of ?numeric. - 0 marker clickTolerance now inherits from the map - remove get/set methods

ChristopherChudzicki · 2020-07-25T19:28:25Z

@ryanhamley Sorry it took me so long to get back to this. I believe the most recent commit addressed all the comments, except #9640 (comment) for the reason described there.

Question for you: Your comment about keeping the API small hit home with me, and now I am wondering whether mapbox benefits much from marker-specific click tolerances at all. To be clear: I strongly believe marker dragging should respect SOME click tolerance. But maybe markers should ALWAYS use the map click tolerance, rather than having their own.

Pros:

simpler interface
probably what people actually want?

Cons:

a "more" breaking change, in that the existing behavior of 0-tolerance markers, nonzero-tolerance maps cannot be recovered.

If you think that ALWAYS using the map's click tolerance for markers is a better way to go, I am happy to make a separate, cleaner PR for that and I'll try to do it much sooner than I got around to making these changes.

ChristopherChudzicki · 2020-08-19T14:07:49Z

@ryanhamley any thoughts on #9640 (comment), or if per-marker tolerance still seems worth while, any chance this might be reviewed again soon?

ryanhamley

Hi @ChristopherChudzicki thanks so much for your diligence and patience. I think this looks great. I'm all for keeping the API minimal, but this is just adding one new option to the marker constructor so I think it strikes a nice balance between minimalism and flexibility. The tests look good too.

I think there should be a separate issue / PR to suppress map click events when dragging markers

If you want to submit a new PR to address this as well, I should have more bandwidth in the future than I did the past couple months and I'll gladly take a look.

Thanks again for the contribution and good work!

ChristopherChudzicki · 2020-09-02T22:23:24Z

@ryanhamley Glad this got merged! Np about the reviews, I was slow with changes, too---life gets hectic. I'd be happy to submit another PR to supress the map clicks when dragging markers.

nstele · 2021-05-06T18:46:32Z

Is there any similar property for Android SDK symbolManager? Have the same problem on Android draggable symbol

ChristopherChudzicki added 7 commits April 28, 2020 10:04

add private _clickTolerance property to map

f42dc49

This is so that Marker instances can access map._clickTolerance if the Markers do not have their own click tolerances.

fix flow typing for Marker._positionDelta

7158bef

fix marker tests after clicktolerance update

eebd191

assert drag handlers do not fire below clicktol

c0c29e3

add tests for getting/setting marker clicktol

1f46995

add pointerEvents assertions to drag tests

621bc69

andrewharvey reviewed Apr 29, 2020

View reviewed changes

reword clickTolerance jsdoc to mirror map option

c69263f

asheemmamoowala requested a review from ryanhamley May 13, 2020 16:05

ryanhamley suggested changes Jun 10, 2020

View reviewed changes

arindam1993 changed the base branch from master to main June 18, 2020 18:13

remove null clickTolerance, remove get/set methods

3688f94

- Make the tolerance numeric instead of ?numeric. - 0 marker clickTolerance now inherits from the map - remove get/set methods

ryanhamley approved these changes Sep 2, 2020

View reviewed changes

ryanhamley merged commit 38f0072 into mapbox:main Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Click Tolerance for Markers #9640

Click Tolerance for Markers #9640

ChristopherChudzicki commented Apr 28, 2020 •

edited

Loading

ChristopherChudzicki commented Apr 28, 2020

andrewharvey Apr 29, 2020

andrewharvey Apr 29, 2020 •

edited

Loading

ChristopherChudzicki Apr 29, 2020

ChristopherChudzicki commented Jun 10, 2020

ryanhamley left a comment

ryanhamley Jun 10, 2020

ChristopherChudzicki Jun 11, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jul 25, 2020

ryanhamley Jun 10, 2020

ChristopherChudzicki Jun 11, 2020 •

edited

Loading

ryanhamley Jun 10, 2020

ChristopherChudzicki Jun 11, 2020 •

edited

Loading

ryanhamley Jun 10, 2020

ChristopherChudzicki commented Jul 25, 2020

ChristopherChudzicki commented Aug 19, 2020

ryanhamley left a comment

ChristopherChudzicki commented Sep 2, 2020 •

edited

Loading

nstele commented May 6, 2021 •

edited

Loading

Click Tolerance for Markers #9640

Click Tolerance for Markers #9640

Conversation

ChristopherChudzicki commented Apr 28, 2020 • edited Loading

Launch Checklist

Changelog Entry:

ChristopherChudzicki commented Apr 28, 2020

Choose a reason for hiding this comment

andrewharvey Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki commented Jun 10, 2020

ryanhamley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki commented Jul 25, 2020

ChristopherChudzicki commented Aug 19, 2020

ryanhamley left a comment

Choose a reason for hiding this comment

ChristopherChudzicki commented Sep 2, 2020 • edited Loading

nstele commented May 6, 2021 • edited Loading

ChristopherChudzicki commented Apr 28, 2020 •

edited

Loading

andrewharvey Apr 29, 2020 •

edited

Loading

ChristopherChudzicki Jun 11, 2020 •

edited

Loading

ChristopherChudzicki Jun 11, 2020 •

edited

Loading

ChristopherChudzicki commented Sep 2, 2020 •

edited

Loading

nstele commented May 6, 2021 •

edited

Loading