Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Click Tolerance for Markers #9640

Merged
merged 9 commits into from
Sep 2, 2020
Merged

Click Tolerance for Markers #9640

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f42dc49
add private _clickTolerance property to map
ChristopherChudzicki Apr 28, 2020
7158bef
fix flow typing for Marker._positionDelta
ChristopherChudzicki Apr 28, 2020
21fc6f4
add clickTolerance option to markers
ChristopherChudzicki Apr 28, 2020
eebd191
fix marker tests after clicktolerance update
ChristopherChudzicki Apr 28, 2020
c0c29e3
assert drag handlers do not fire below clicktol
ChristopherChudzicki Apr 28, 2020
1f46995
add tests for getting/setting marker clicktol
ChristopherChudzicki Apr 28, 2020
621bc69
add pointerEvents assertions to drag tests
ChristopherChudzicki Apr 28, 2020
c69263f
reword clickTolerance jsdoc to mirror map option
ChristopherChudzicki Apr 30, 2020
3688f94
remove null clickTolerance, remove get/set methods
ChristopherChudzicki Jul 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ class Map extends Camera {
_requestManager: RequestManager;
_locale: Object;
_removed: boolean;
_clickTolerance: number;

/**
* The map's {@link ScrollZoomHandler}, which implements zooming in and out with a scroll wheel or trackpad.
Expand Down Expand Up @@ -398,6 +399,7 @@ class Map extends Camera {
this._controls = [];
this._mapId = uniqueId();
this._locale = extend({}, defaultLocale, options.locale);
this._clickTolerance = options.clickTolerance;

this._requestManager = new RequestManager(options.transformRequest, options.accessToken);

Expand Down
55 changes: 54 additions & 1 deletion src/ui/marker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type Options = {
color?: string,
scale?: number,
draggable?: boolean,
clickTolerance?: number,
rotation?: number,
rotationAlignment?: string,
pitchAlignment?: string
Expand All @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param {number} [options.rotation=0] The rotation angle of the marker in degrees, relative to its respective `rotationAlignment` setting. A positive value will rotate the marker clockwise.
* @param {string} [options.pitchAlignment='auto'] `map` aligns the `Marker` to the plane of the map. `viewport` aligns the `Marker` to the plane of the viewport. `auto` automatically matches the value of `rotationAlignment`.
* @param {string} [options.rotationAlignment='auto'] `map` aligns the `Marker`'s rotation relative to the map, maintaining a bearing as the map rotates. `viewport` aligns the `Marker`'s rotation relative to the viewport, agnostic to map rotations. `auto` is equivalent to `viewport`.
Expand All @@ -58,8 +60,11 @@ export default class Marker extends Evented {
_scale: number;
_defaultMarker: boolean;
_draggable: boolean;
_clickTolerance: ?number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I belive this is resolved.

_isDragging: boolean;
_state: 'inactive' | 'pending' | 'active'; // used for handling drag events
_positionDelta: ?number;
_positionDelta: ?Point;
_pointerdownPos: ?Point;
_rotation: number;
_pitchAlignment: string;
_rotationAlignment: string;
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null -> 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is resolved.

this._isDragging = false;
this._state = 'inactive';
this._rotation = options && options.rotation || 0;
this._rotationAlignment = options && options.rotationAlignment || 'auto';
Expand Down Expand Up @@ -483,7 +490,49 @@ export default class Marker extends Evented {
return this;
}

/**
* 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this method.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: value of the click tolerance in pixels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this method.

* @returns {void}
*/
setClickTolerance(value: ?number) {
if (value === null || value === undefined) {
this._clickTolerance = null; // set to inherit from map.
}
const numeric = +value;
if (!Number.isNaN(numeric)) {
this._clickTolerance = numeric;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this method.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to square these numbers?

Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this method.


_onMove(e: MapMouseEvent | MapTouchEvent) {
if (!this._isDragging) {
this._isDragging = this._hasMovedBeyondClickTolerance(e.point);
}
if (!this._isDragging) return;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this._pos = e.point.sub(this._positionDelta);
this._lngLat = this._map.unproject(this._pos);
this.setLngLat(this._lngLat);
Expand Down Expand Up @@ -524,6 +573,8 @@ export default class Marker extends Evented {
// revert to normal pointer event handling
this._element.style.pointerEvents = 'auto';
this._positionDelta = null;
this._pointerdownPos = null;
this._isDragging = false;
this._map.off('mousemove', this._onMove);
this._map.off('touchmove', this._onMove);

Expand Down Expand Up @@ -556,6 +607,8 @@ export default class Marker extends Evented {
// creating a jarring UX effect.
this._positionDelta = e.point.sub(this._pos).add(this._offset);

this._pointerdownPos = e.point;

this._state = 'pending';
this._map.on('mousemove', this._onMove);
this._map.on('touchmove', this._onMove);
Expand Down
139 changes: 127 additions & 12 deletions test/unit/ui/marker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import LngLat from '../../../src/geo/lng_lat';
import Point from '@mapbox/point-geometry';
import simulate from '../../util/simulate_interaction';

function createMap(t) {
function createMap(t, options = {}) {
const container = window.document.createElement('div');
Object.defineProperty(container, 'clientWidth', {value: 512});
Object.defineProperty(container, 'clientHeight', {value: 512});
return globalCreateMap(t, {container});
return globalCreateMap(t, {container, ...options});
}

test('Marker uses a default marker element with an appropriate offset', (t) => {
Expand Down Expand Up @@ -329,6 +329,41 @@ test('Popup anchors around default Marker', (t) => {
t.end();
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const marker = new Marker({clickTolerance: 5});
t.equal(marker.getClickTolerance(), 5);
t.end();
});

test('Marker#getClickTolerance gets map clickTolerance if clickTolerance has not been set', (t) => {
const map = createMap(t, {clickTolerance: 4});
const marker = new Marker()
.setLngLat([0, 0])
.addTo(map);
t.equal(marker.getClickTolerance(), 4);

map.remove();
t.end();
});

test('Marker#getClickTolerance uses Marker clickTolerance in favor of Map Click Tolerance if both specified', (t) => {
const map = createMap(t, {clickTolerance: 4});
const marker = new Marker({clickTolerance: 5})
.setLngLat([0, 0])
.addTo(map);
t.equal(marker.getClickTolerance(), 5);

map.remove();
t.end();
});

test('Marker#setClickTolerance sets the click tolerance', (t) => {
const marker = new Marker({clickTolerance: 5});
marker.setClickTolerance(10);
t.equal(marker.getClickTolerance(), 10);
t.end();
});

test('Marker drag functionality can be added with drag option', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
Expand Down Expand Up @@ -370,7 +405,7 @@ test('Marker#setDraggable turns off drag functionality', (t) => {
t.end();
});

test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to a mouse-triggered drag', (t) => {
test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to a mouse-triggered drag and cancels element pointer events.', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
Expand All @@ -385,20 +420,59 @@ test('Marker with draggable:true fires dragstart, drag, and dragend events at ap
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.mousedown(el);
simulate.mousedown(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

simulate.mousemove(el);
t.equal(marker.getClickTolerance(), 3, 'subsequent movement exceeds than clickTolerance');
simulate.mousemove(el, {clientX: 3, clientY: 1});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none');

simulate.mouseup(el);
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 1);
t.equal(el.style.pointerEvents, 'auto');

map.remove();
t.end();
});

test('Marker with draggable:true does NOT fire drag events if mouse movement is below clickTolerance and allows element pointer events', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
.addTo(map);
const el = marker.getElement();

const dragstart = t.spy();
const drag = t.spy();
const dragend = t.spy();

marker.on('dragstart', dragstart);
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.mousedown(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

t.equal(marker.getClickTolerance(), 3, 'subsequent movement is below clickTolerance');
simulate.mousemove(el, {clientX: 1, clientY: 1});
t.equal(el.style.pointerEvents, '');

simulate.mouseup(el);
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'auto');

map.remove();
t.end();
Expand All @@ -419,12 +493,13 @@ test('Marker with draggable:false does not fire dragstart, drag, and dragend eve
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.mousedown(el);
simulate.mousedown(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(el);
t.equal(marker.getClickTolerance(), 3, 'subsequent movement exceeds than clickTolerance');
simulate.mousemove(el, {clientX: 3, clientY: 1});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
Expand All @@ -438,7 +513,7 @@ test('Marker with draggable:false does not fire dragstart, drag, and dragend eve
t.end();
});

test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to a touch-triggered drag', (t) => {
test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to a touch-triggered drag and cancels element pointer events.', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
Expand All @@ -453,20 +528,59 @@ test('Marker with draggable:true fires dragstart, drag, and dragend events at ap
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.touchstart(el);
simulate.touchstart(el, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

simulate.touchmove(el);
t.equal(marker.getClickTolerance(), 3, 'subsequent movement exceeds than clickTolerance');
simulate.touchmove(el, {touches: [{clientX: 3, clientY: 1}]});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none');

simulate.touchend(el);
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 1);
t.equal(el.style.pointerEvents, 'auto');

map.remove();
t.end();
});

test('Marker with draggable:true does NOT fire drag events if touch movement is below clickTolerance and allows element pointer events', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
.addTo(map);
const el = marker.getElement();

const dragstart = t.spy();
const drag = t.spy();
const dragend = t.spy();

marker.on('dragstart', dragstart);
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.touchstart(el, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

t.equal(marker.getClickTolerance(), 3, 'subsequent movement is below clickTolerance');
simulate.touchmove(el, {touches: [{clientX: 1, clientY: 1}]});
t.equal(el.style.pointerEvents, '');

simulate.touchend(el);
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'auto');

map.remove();
t.end();
Expand All @@ -487,12 +601,13 @@ test('Marker with draggable:false does not fire dragstart, drag, and dragend eve
marker.on('drag', drag);
marker.on('dragend', dragend);

simulate.touchstart(el);
simulate.touchstart(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.touchmove(el);
t.equal(marker.getClickTolerance(), 3, 'subsequent movement exceeds than clickTolerance');
simulate.touchmove(el, {clientX: 3, clientY: 1});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
Expand Down