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

Fix double-tap zoom (#6217) #8086

Merged
merged 9 commits into from
Apr 9, 2019
Merged

Fix double-tap zoom (#6217) #8086

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cccb9f9
Fix/add tests for existing dblclick-zoom functionality
Mar 22, 2019
edb7d2b
Add tests for existing dbltap-zoom functionality
Mar 22, 2019
daff833
Prevent dbltap-zoom if touch points differ, add test
Mar 23, 2019
c2a1ca8
Trigger dbltap-zoom on second touchend, add tests
Mar 23, 2019
81f7c0c
Increase double-tap maxDelta to 30
Mar 27, 2019
b5824b3
Prevent duplicate zoom on dblclick if double-tap zoom is triggered
Mar 27, 2019
1dabf9b
Address review comments from @mourner & @asheemmamoowala
Apr 1, 2019
61362b0
Clarify comments re passing touchstart vs. -end event
Apr 1, 2019
ae80d83
Fix type-only import
Apr 2, 2019
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
41 changes: 37 additions & 4 deletions src/ui/handler/dblclick_zoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class DoubleClickZoomHandler {
_enabled: boolean;
_active: boolean;
_tapped: ?TimeoutID;
_tappedPoint: ?{x: number, y: number};

/**
* @private
Expand Down Expand Up @@ -71,15 +72,47 @@ class DoubleClickZoomHandler {
if (!this.isEnabled()) return;
if (e.points.length > 1) return;

const maxDelta = 30;
vakila marked this conversation as resolved.
Show resolved Hide resolved

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 && Math.abs(firstTap.x - newTap.x) <= maxDelta && Math.abs(firstTap.y - newTap.y) <= maxDelta) {
vakila marked this conversation as resolved.
Show resolved Hide resolved
e.originalEvent.preventDefault(); // prevent duplicate zoom on dblclick

const onTouchEnd = () => {
if (this._tapped) { // make sure we are still within the timeout window
this._zoom(e); // pass this touchstart event, as touchend events have no points
asheemmamoowala marked this conversation as resolved.
Show resolved Hide resolved
}
this._map.off('touchend', onTouchEnd);
this._resetTapped();
};

const onTouchCancel = () => {
this._map.off('touchend', onTouchEnd);
this._map.off('touchcancel', onTouchCancel);
this._resetTapped();
};

this._map.on('touchend', onTouchEnd);
this._map.on('touchcancel', onTouchCancel);
Copy link
Member

Choose a reason for hiding this comment

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

To simplify this a bit, you can do this._map.once(...) instead of on — this way listener doesn't have to manually remove itself. It also ensures touchcancel listeners are not accumulated if they never happen.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, disregard the last sentence — it seems like we still need to make sure touchcancel listeners don't accumulate with every tap when touches are not cancelled.

Copy link
Author

Choose a reason for hiding this comment

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

👍 thanks for catching the missing touchcancel removal! I've now used .once() for both listeners and still manually remove the other listener in each handler, so I think all bases should be covered.


} 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();
Expand Down
158 changes: 158 additions & 0 deletions test/unit/ui/handler/dblclick_zoom.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,21 @@ function createMap(t) {
return new Map({ container: DOM.create('div', '', window.document.body) });
}

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

Expand All @@ -18,9 +33,152 @@ 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);

const simulateDoubleTap = () => {
return new Promise(resolve => {
simulate.touchstart(map.getCanvas());
simulate.touchend(map.getCanvas());
setTimeout(() => {
simulate.touchstart(map.getCanvas());
simulate.touchend(map.getCanvas());
map._renderTaskQueue.run();
resolve();
}, 100);
});
};

simulateDoubleTap(map, 100).then(() => {
vakila marked this conversation as resolved.
Show resolved Hide resolved
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);

const simulateDelayedDoubleTap = () => {
vakila marked this conversation as resolved.
Show resolved Hide resolved
return new Promise(resolve => {
simulate.touchstart(map.getCanvas());
simulate.touchend(map.getCanvas());
setTimeout(() => {
simulate.touchstart(map.getCanvas());
simulate.touchend(map.getCanvas());
map._renderTaskQueue.run();
resolve();
}, 300);
});
};

simulateDelayedDoubleTap().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 simulateTwoDifferentTaps = () => {
return new Promise(resolve => {
simulate.touchstart(map.getCanvas(), {touches: [{clientX: 0, clientY: 0}]});
simulate.touchend(map.getCanvas());
setTimeout(() => {
simulate.touchstart(map.getCanvas(), {touches: [{clientX: 30.5, clientY: 30.5}]});
simulate.touchend(map.getCanvas());
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);

simulate.touchstart(map.getCanvas(), {touches: [{clientX: 0.5, clientY: 0.5}]});
simulate.touchend(map.getCanvas());
simulate.touchstart(map.getCanvas(), {touches: [{clientX: 0.5, clientY: 0.5}]});
map._renderTaskQueue.run();
t.notOk(zoom.called, 'should not trigger zoom before second touchend');

simulate.touchcancel(map.getCanvas());
simulate.touchend(map.getCanvas());
map._renderTaskQueue.run();
t.notOk(zoom.called, 'should not trigger zoom if second touch is canceled');

simulate.touchstart(map.getCanvas(), {touches: [{clientX: 0.5, clientY: 0.5}]});
simulate.touchend(map.getCanvas());
simulate.touchstart(map.getCanvas(), {touches: [{clientX: 0.5, clientY: 0.5}]});
vakila marked this conversation as resolved.
Show resolved Hide resolved
map._renderTaskQueue.run();
t.notOk(zoom.called);

simulate.touchend(map.getCanvas());
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 simulateSlowSecondTap = () => {
return new Promise(resolve => {
simulate.touchstart(map.getCanvas());
simulate.touchend(map.getCanvas());
simulate.touchstart(map.getCanvas());
setTimeout(() => {
simulate.touchend(map.getCanvas());
map._renderTaskQueue.run();
resolve();
}, 300);
});
};

simulateSlowSecondTap().then(() => {
t.notOk(zoom.called);

t.end();
});
});