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 all 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
19 changes: 18 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=0] 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). The default is to inherit map's clickTolerance.
* @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;
_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 || 0;
this._isDragging = false;
this._state = 'inactive';
this._rotation = options && options.rotation || 0;
this._rotationAlignment = options && options.rotationAlignment || 'auto';
Expand Down Expand Up @@ -484,6 +491,12 @@ export default class Marker extends Evented {
}

_onMove(e: MapMouseEvent | MapTouchEvent) {
if (!this._isDragging) {
const clickTolerance = this._clickTolerance || this._map._clickTolerance;
this._isDragging = e.point.dist(this._pointerdownPos) >= clickTolerance;
}
if (!this._isDragging) return;

this._pos = e.point.sub(this._positionDelta);
this._lngLat = this._map.unproject(this._pos);
this.setLngLat(this._lngLat);
Expand Down Expand Up @@ -524,6 +537,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 +571,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
164 changes: 148 additions & 16 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 @@ -370,7 +370,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 mouse-triggered drag with map-inherited clickTolerance', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
Expand All @@ -385,20 +385,86 @@ 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);
simulate.mousemove(el, {clientX: 2.9, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0, "drag not called yet, movement below marker's map-inherited click tolerance");
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

// above map's click tolerance
simulate.mousemove(el, {clientX: 3.1, clientY: 0});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1, 'drag fired once click tolerance exceeded');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

simulate.mousemove(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 2, 'drag fired when moving back within clickTolerance of mousedown');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

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

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

test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to mouse-triggered drag with marker-specific clickTolerance', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true, clickTolerance: 4})
.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, '');

simulate.mousemove(el, {clientX: 3.9, clientY: 0});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0, "drag not called yet, movement below marker's map-inherited click tolerance");
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

// above map's click tolerance
simulate.mousemove(el, {clientX: 4.1, clientY: 0});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1, 'drag fired once click tolerance exceeded');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

simulate.mousemove(el, {clientX: 0, clientY: 0});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(drag.callCount, 2, 'drag fired when moving back within clickTolerance of mousedown');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

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

map.remove();
t.end();
Expand All @@ -419,12 +485,12 @@ 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);
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 +504,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 with map-inherited clickTolerance', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true})
.setLngLat([0, 0])
Expand All @@ -453,20 +519,86 @@ 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, {touches: [{clientX: 2.9, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0, "drag not called yet, movement below marker's map-inherited click tolerance");
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

// above map's click tolerance
simulate.touchmove(el, {touches: [{clientX: 3.1, clientY: 0}]});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1, 'drag fired once click tolerance exceeded');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

simulate.touchmove(el, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 2, 'drag fired when moving back within clickTolerance of touchstart');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

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

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

test('Marker with draggable:true fires dragstart, drag, and dragend events at appropriate times in response to a touch-triggered drag with marker-specific clickTolerance', (t) => {
const map = createMap(t);
const marker = new Marker({draggable: true, clickTolerance: 4})
.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, '');

simulate.touchmove(el, {touches: [{clientX: 3.9, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0, "drag not called yet, movement below marker's map-inherited click tolerance");
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, '');

// above map's click tolerance
simulate.touchmove(el, {touches: [{clientX: 4.1, clientY: 0}]});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1, 'drag fired once click tolerance exceeded');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

simulate.touchmove(el);
simulate.touchmove(el, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(drag.callCount, 2, 'drag fired when moving back within clickTolerance of touchstart');
t.equal(dragend.callCount, 0);
t.equal(el.style.pointerEvents, 'none', 'cancels pointer events while dragging');

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

map.remove();
t.end();
Expand All @@ -487,12 +619,12 @@ 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, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.touchmove(el);
simulate.touchmove(el, {touches: [{clientX: 0, clientY: 0}]});
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);
Expand Down