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

Allow very small twodrag and pinch gestures #1916

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow very small twodrag and pinch gestures #1916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 15 additions & 12 deletions core/input/gesturehandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,14 @@ export default class GestureHandler {
}

if (!this._hasDetectedGesture()) {
// Ignore moves smaller than the minimum threshold
if (Math.hypot(deltaX, deltaY) < GH_MOVE_THRESHOLD) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

This breaks two finger gestures if the first finger moves slightly before the second finger makes its initial touch. Which I'd suspect is very likely, given how sensitive touch devices are.

// Can't be a tap or long press as we've seen movement
this._state &= ~(GH_ONETAP | GH_TWOTAP | GH_THREETAP | GH_LONGPRESS);
this._stopLongpressTimeout();
let deltaMove = Math.hypot(deltaX, deltaY);

// Can't be a tap or long press if we've seen movement
if (deltaMove >= GH_MOVE_THRESHOLD) {
this._state &= ~(GH_ONETAP | GH_TWOTAP | GH_THREETAP | GH_LONGPRESS);
this._stopLongpressTimeout();
}

if (this._tracked.length !== 1) {
this._state &= ~(GH_DRAG);
Copy link
Member

Choose a reason for hiding this comment

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

This should have already been excluded in the touchstart handler. Perhaps best to remove this redundancy to make things easier to read and understand.

Expand All @@ -213,10 +213,10 @@ export default class GestureHandler {
let prevDeltaMove = Math.hypot(prevTouch.firstX - prevTouch.lastX,
prevTouch.firstY - prevTouch.lastY);

// We know that the current touch moved far enough,
// but unless both touches moved further than their
// threshold we don't want to disqualify any gestures
if (prevDeltaMove > GH_MOVE_THRESHOLD) {
// Unless both touches moved further than their threshold we
// don't want to disqualify any gestures right now
if (deltaMove > GH_MOVE_THRESHOLD &&
prevDeltaMove > GH_MOVE_THRESHOLD) {

// The angle difference between the direction of the touch points
let deltaAngle = Math.abs(touch.angle - prevTouch.angle);
Expand All @@ -234,7 +234,8 @@ export default class GestureHandler {
}
} else if (!this._isTwoTouchTimeoutRunning()) {
// We can't determine the gesture right now, let's
// wait and see if more events are on their way
// wait and see if more events are on their way.
// If not, we'll have to decide which gesture it is.
this._startTwoTouchTimeout();
Copy link
Member

Choose a reason for hiding this comment

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

This breaks other two finger gestures as the two touch timeout blindly assumes that only a pinch or drag is left to choose from. It might need to be fixed so it also just excludes things, leaving it still uncertain for any other gestures that have yet to be ruled out.

I'm not sure if this timeout was ever really used, or at least very little, given the existing large movement threshold. We might need to reexamine it in detail.

Copy link
Member

Choose a reason for hiding this comment

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

The lack of distance threshold also means that the timeout handler has much more noisy data to deal with when trying to identify a pinch from a drag. So it might be much less robust there as well.

}
}
Expand All @@ -260,6 +261,7 @@ export default class GestureHandler {
(this._tracked.length === 0)) {
this._state = GH_INITSTATE;
this._waitingRelease = false;
this._stopTwoTouchTimeout();
}
return;
}
Expand Down Expand Up @@ -346,6 +348,7 @@ export default class GestureHandler {
if ((this._ignored.length === 0)) {
this._state = GH_INITSTATE;
this._waitingRelease = false;
this._stopTwoTouchTimeout();
}
}

Expand Down
86 changes: 74 additions & 12 deletions tests/test.gesturehandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ describe('Gesture handler', function () {
touchStart(1, 50.0, 40.0);
touchStart(2, 60.0, 40.0);
touchMove(1, 80.0, 40.0);
touchMove(2, 110.0, 40.0);
touchMove(2, 90.0, 40.0);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely clear on why these unit tests needed to be changed?


expect(gestures).to.not.have.been.called;

Expand All @@ -601,15 +601,15 @@ describe('Gesture handler', function () {
detail: { type: 'twodrag',
clientX: 55.0,
clientY: 40.0,
magnitudeX: 40.0,
magnitudeX: 30.0,
magnitudeY: 0.0 } }));
});

it('should handle slow vertical two finger drag', function () {
touchStart(1, 40.0, 40.0);
touchStart(2, 40.0, 60.0);
touchMove(2, 40.0, 80.0);
touchMove(1, 40.0, 100.0);
touchMove(1, 40.0, 80.0);

expect(gestures).to.not.have.been.called;

Expand All @@ -631,14 +631,14 @@ describe('Gesture handler', function () {
clientX: 40.0,
clientY: 50.0,
magnitudeX: 0.0,
magnitudeY: 40.0 } }));
magnitudeY: 30.0 } }));
});

it('should handle slow diagonal two finger drag', function () {
touchStart(1, 50.0, 40.0);
touchStart(2, 40.0, 60.0);
touchMove(1, 70.0, 60.0);
touchMove(2, 90.0, 110.0);
touchMove(2, 60.0, 80.0);

expect(gestures).to.not.have.been.called;

Expand All @@ -659,8 +659,8 @@ describe('Gesture handler', function () {
detail: { type: 'twodrag',
clientX: 45.0,
clientY: 50.0,
magnitudeX: 35.0,
magnitudeY: 35.0 } }));
magnitudeX: 20.0,
magnitudeY: 20.0 } }));
});

it('should ignore too slow two finger drag', function () {
Expand Down Expand Up @@ -783,7 +783,36 @@ describe('Gesture handler', function () {
it('should handle pinching inwards slowly', function () {
touchStart(1, 0.0, 0.0);
touchStart(2, 130.0, 130.0);
touchMove(1, 50.0, 40.0);
touchMove(1, 30.0, 20.0);
touchMove(2, 100.0, 130.0);

expect(gestures).to.not.have.been.called;

clock.tick(60);

expect(gestures).to.have.been.calledTwice;

expect(gestures.firstCall).to.have.been.calledWith(
sinon.match({ type: 'gesturestart',
detail: { type: 'pinch',
clientX: 65.0,
clientY: 65.0,
magnitudeX: 130.0,
magnitudeY: 130.0 } }));

expect(gestures.secondCall).to.have.been.calledWith(
sinon.match({ type: 'gesturemove',
detail: { type: 'pinch',
clientX: 65.0,
clientY: 65.0,
magnitudeX: 70.0,
magnitudeY: 110.0 } }));
});

it('should handle second pinch afterwards', function () {
touchStart(1, 0.0, 0.0);
touchStart(2, 130.0, 130.0);
touchMove(1, 30.0, 20.0);
touchMove(2, 100.0, 130.0);

expect(gestures).to.not.have.been.called;
Expand All @@ -805,14 +834,47 @@ describe('Gesture handler', function () {
detail: { type: 'pinch',
clientX: 65.0,
clientY: 65.0,
magnitudeX: 50.0,
magnitudeY: 90.0 } }));
magnitudeX: 70.0,
magnitudeY: 110.0 } }));

touchEnd(1);
touchEnd(2);

gestures.resetHistory();

touchStart(3, 0.0, 0.0);
touchStart(4, 130.0, 130.0);
touchMove(3, 30.0, 20.0);
touchMove(4, 100.0, 130.0);

expect(gestures).to.not.have.been.called;

clock.tick(60);

expect(gestures).to.have.been.calledTwice;

expect(gestures.firstCall).to.have.been.calledWith(
sinon.match({ type: 'gesturestart',
detail: { type: 'pinch',
clientX: 65.0,
clientY: 65.0,
magnitudeX: 130.0,
magnitudeY: 130.0 } }));

expect(gestures.secondCall).to.have.been.calledWith(
sinon.match({ type: 'gesturemove',
detail: { type: 'pinch',
clientX: 65.0,
clientY: 65.0,
magnitudeX: 70.0,
magnitudeY: 110.0 } }));

});

it('should handle pinching outwards slowly', function () {
touchStart(1, 100.0, 130.0);
touchStart(2, 110.0, 130.0);
touchMove(2, 200.0, 130.0);
touchMove(2, 130.0, 130.0);

expect(gestures).to.not.have.been.called;

Expand All @@ -833,7 +895,7 @@ describe('Gesture handler', function () {
detail: { type: 'pinch',
clientX: 105.0,
clientY: 130.0,
magnitudeX: 100.0,
magnitudeX: 30.0,
magnitudeY: 0.0 } }));
});

Expand Down
Loading