From f705fd2a4ddd8a33a13c6af048212d0f697d5c59 Mon Sep 17 00:00:00 2001 From: Anjana Sofia Vakil Date: Tue, 9 Apr 2019 16:45:54 +0200 Subject: [PATCH] Fix double-tap zoom (#6217) (#8086) * Fix/add tests for existing dblclick-zoom functionality * Add tests for existing dbltap-zoom functionality * Prevent dbltap-zoom if touch points differ, add test * Trigger dbltap-zoom on second touchend, add tests * Increase double-tap maxDelta to 30 * Prevent duplicate zoom on dblclick if double-tap zoom is triggered * Address review comments from @mourner & @asheemmamoowala - Use proper Point obj w/ .dist() method - Remove touchcancel handler on touchend - Attach touchend/cancel handlers with .once() - Move distance threshold to top-level constant, add doc comment - Refactor tests to avoid repetition * Clarify comments re passing touchstart vs. -end event * Fix type-only import --- src/ui/handler/dblclick_zoom.js | 42 +++++- test/unit/ui/handler/dblclick_zoom.test.js | 153 +++++++++++++++++++++ 2 files changed, 191 insertions(+), 4 deletions(-) diff --git a/src/ui/handler/dblclick_zoom.js b/src/ui/handler/dblclick_zoom.js index 1be4de8b931..e4b5f23d21f 100644 --- a/src/ui/handler/dblclick_zoom.js +++ b/src/ui/handler/dblclick_zoom.js @@ -4,6 +4,10 @@ import { bindAll } from '../../util/util'; import type Map from '../map'; import type {MapMouseEvent, MapTouchEvent} from '../events'; +import type Point from '@mapbox/point-geometry'; + +// maximum distance between two tap Points for them to qualify as a double-tap +const maxDist = 30; /** * The `DoubleClickZoomHandler` allows the user to zoom the map at a point by @@ -14,6 +18,7 @@ class DoubleClickZoomHandler { _enabled: boolean; _active: boolean; _tapped: ?TimeoutID; + _tappedPoint: ?Point; /** * @private @@ -72,14 +77,43 @@ class DoubleClickZoomHandler { if (e.points.length > 1) return; if (!this._tapped) { - this._tapped = setTimeout(() => { this._tapped = null; }, 300); + this._tappedPoint = e.points[0]; + this._tapped = setTimeout(() => { this._tapped = null; this._tappedPoint = null; }, 300); } else { - clearTimeout(this._tapped); - this._tapped = null; - this._zoom(e); + const newTap = e.points[0]; + const firstTap = this._tappedPoint; + + if (firstTap && firstTap.dist(newTap) <= maxDist) { + e.originalEvent.preventDefault(); // prevent duplicate zoom on dblclick + + const onTouchEnd = () => { // ignore the touchend event, as it has no point we can zoom to + if (this._tapped) { // make sure we are still within the timeout window + this._zoom(e); // pass the original touchstart event, with the tapped point + } + this._map.off('touchcancel', onTouchCancel); + this._resetTapped(); + }; + + const onTouchCancel = () => { + this._map.off('touchend', onTouchEnd); + this._resetTapped(); + }; + + this._map.once('touchend', onTouchEnd); + this._map.once('touchcancel', onTouchCancel); + + } else { // touches are too far apart, don't zoom + this._resetTapped(); + } } } + _resetTapped() { + clearTimeout(this._tapped); + this._tapped = null; + this._tappedPoint = null; + } + onDblClick(e: MapMouseEvent) { if (!this.isEnabled()) return; e.originalEvent.preventDefault(); diff --git a/test/unit/ui/handler/dblclick_zoom.test.js b/test/unit/ui/handler/dblclick_zoom.test.js index 1d00c55fe78..d0a3671abe2 100644 --- a/test/unit/ui/handler/dblclick_zoom.test.js +++ b/test/unit/ui/handler/dblclick_zoom.test.js @@ -9,6 +9,35 @@ function createMap(t) { return new Map({ container: DOM.create('div', '', window.document.body) }); } +function simulateDoubleTap(map, delay = 100) { + const canvas = map.getCanvas(); + return new Promise(resolve => { + simulate.touchstart(canvas); + simulate.touchend(canvas); + setTimeout(() => { + simulate.touchstart(canvas); + simulate.touchend(canvas); + map._renderTaskQueue.run(); + resolve(); + }, delay); + }); +} + +test('DoubleClickZoomHandler zooms on dblclick event', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + simulate.dblclick(map.getCanvas()); + map._renderTaskQueue.run(); + + t.ok(zoom.called); + + map.remove(); + t.end(); +}); + test('DoubleClickZoomHandler does not zoom if preventDefault is called on the dblclick event', (t) => { const map = createMap(t); @@ -18,9 +47,133 @@ test('DoubleClickZoomHandler does not zoom if preventDefault is called on the db map.on('zoom', zoom); simulate.dblclick(map.getCanvas()); + map._renderTaskQueue.run(); t.equal(zoom.callCount, 0); map.remove(); t.end(); }); + +test('DoubleClickZoomHandler zooms on double tap if touchstart events are < 300ms apart', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + simulateDoubleTap(map, 100).then(() => { + t.ok(zoom.called); + + map.remove(); + t.end(); + }); + +}); + +test('DoubleClickZoomHandler does not zoom on double tap if touchstart events are > 300ms apart', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + simulateDoubleTap(map, 300).then(() => { + t.equal(zoom.callCount, 0); + + map.remove(); + t.end(); + }); + +}); + +test('DoubleClickZoomHandler does not zoom on double tap if touchstart events are in different locations', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + const canvas = map.getCanvas(); + + const simulateTwoDifferentTaps = () => { + return new Promise(resolve => { + simulate.touchstart(canvas, {touches: [{clientX: 0, clientY: 0}]}); + simulate.touchend(canvas); + setTimeout(() => { + simulate.touchstart(canvas, {touches: [{clientX: 30.5, clientY: 30.5}]}); + simulate.touchend(canvas); + map._renderTaskQueue.run(); + resolve(); + }, 100); + }); + }; + + simulateTwoDifferentTaps().then(() => { + t.equal(zoom.callCount, 0); + + map.remove(); + t.end(); + }); + +}); + +test('DoubleClickZoomHandler zooms on the second touchend event of a double tap', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + const canvas = map.getCanvas(); + const touchOptions = {touches: [{clientX: 0.5, clientY: 0.5}]}; + + simulate.touchstart(canvas, touchOptions); + simulate.touchend(canvas); + simulate.touchstart(canvas, touchOptions); + map._renderTaskQueue.run(); + t.notOk(zoom.called, 'should not trigger zoom before second touchend'); + + simulate.touchcancel(canvas); + simulate.touchend(canvas); + map._renderTaskQueue.run(); + t.notOk(zoom.called, 'should not trigger zoom if second touch is canceled'); + + simulate.touchstart(canvas, touchOptions); + simulate.touchend(canvas); + simulate.touchstart(canvas, touchOptions); + map._renderTaskQueue.run(); + t.notOk(zoom.called); + + simulate.touchend(canvas); + map._renderTaskQueue.run(); + + t.ok(zoom.called, 'should trigger zoom after second touchend'); + t.deepEquals(zoom.getCall(0).args[0].point, { x: 0.5, y: 0.5 }, 'should zoom to correct point'); + + t.end(); +}); + +test('DoubleClickZoomHandler does not zoom on double tap if second touchend is >300ms after first touchstart', (t) => { + const map = createMap(t); + + const zoom = t.spy(); + map.on('zoom', zoom); + + const canvas = map.getCanvas(); + + const simulateSlowSecondTap = () => { + return new Promise(resolve => { + simulate.touchstart(canvas); + simulate.touchend(canvas); + simulate.touchstart(canvas); + setTimeout(() => { + simulate.touchend(canvas); + map._renderTaskQueue.run(); + resolve(); + }, 300); + }); + }; + + simulateSlowSecondTap().then(() => { + t.notOk(zoom.called); + + t.end(); + }); +});