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.

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 DragRotate when mouseup occurs outside window or iframe #9512

Merged
merged 2 commits into from
Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions src/ui/handler/mouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ import type Point from '@mapbox/point-geometry';
const LEFT_BUTTON = 0;
const RIGHT_BUTTON = 2;

// the values for each button in MouseEvent.buttons
const BUTTONS_FLAGS = {
[LEFT_BUTTON]: 1,
[RIGHT_BUTTON]: 2
};

function buttonStillPressed(e: MouseEvent, button: number) {
const flag = BUTTONS_FLAGS[button];
return e.buttons === undefined || (e.buttons & flag) !== flag;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to press the left button, move the mouse outside the window, release the button, then click the left button again and bring it back inside the window so that this isn't triggered? Or does releasing the button always make e.buttons undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to press the left button, move the mouse outside the window, release the button, then click the left button again and bring it back inside the window so that this isn't triggered?
I think it might be possible but I wasn't able to trigger it. With both ctrl-click and right click that starts outside the browser it only starts firing mousemoves back in the window after mouseup. But I think if it did happen it should be ok. The gesture would continue where the user left off.

Or does releasing the button always make e.buttons undefined?

e.buttons got added in safari in 11. To handle those browsers more gracefully it's saying "yes the button is pressed" in those cases even if we can't tell. If this bug were present in those browsers it would remain but everything else would still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And as you noted it's not present in android chrome at all. This would handle that gracefully if you had a mouse connected to Android.

}

class MouseHandler {

_enabled: boolean;
Expand Down Expand Up @@ -50,6 +61,17 @@ class MouseHandler {
if (!lastPoint) return;
e.preventDefault();

if (buttonStillPressed(e, this._eventButton)) {
// Some browsers don't fire a `mouseup` when the mouseup occurs outside
// the window or iframe:
// https://github.com/mapbox/mapbox-gl-js/issues/4622
//
// If the button is no longer pressed during this `mousemove` it may have
// been released outside of the window or iframe.
this.reset();
return;
}

if (!this._moved && point.dist(lastPoint) < this._clickTolerance) return;
this._moved = true;
this._lastPoint = point;
Expand All @@ -59,6 +81,7 @@ class MouseHandler {
}

mouseupWindow(e: MouseEvent) {
if (!this._lastPoint) return;
const eventButton = DOM.mouseButton(e);
if (eventButton !== this._eventButton) return;
if (this._moved) DOM.suppressClick();
Expand Down
35 changes: 19 additions & 16 deletions test/unit/ui/handler/drag_pan.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ function createMap(t, clickTolerance, dragPan) {
});
}

// MouseEvent.buttons = 1 // left button
const buttons = 1;

test('DragPanHandler fires dragstart, drag, and dragend events at appropriate times in response to a mouse-triggered drag', (t) => {
const map = createMap(t);

Expand All @@ -30,7 +33,7 @@ test('DragPanHandler fires dragstart, drag, and dragend events at appropriate ti
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
Expand Down Expand Up @@ -63,7 +66,7 @@ test('DragPanHandler captures mousemove events during a mouse-triggered drag (re
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(window.document.body, {clientX: 10, clientY: 10});
simulate.mousemove(window.document.body, {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
Expand Down Expand Up @@ -121,7 +124,7 @@ test('DragPanHandler prevents mousemove events from firing during a drag (#1555)
simulate.mousedown(map.getCanvasContainer());
map._renderTaskQueue.run();

simulate.mousemove(map.getCanvasContainer(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvasContainer(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();

simulate.mouseup(map.getCanvasContainer());
Expand All @@ -142,7 +145,7 @@ test('DragPanHandler ends a mouse-triggered drag if the window blurs', (t) => {
simulate.mousedown(map.getCanvas());
map._renderTaskQueue.run();

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();

simulate.blur(window);
Expand Down Expand Up @@ -176,14 +179,14 @@ test('DragPanHandler requests a new render frame after each mousemove event', (t
const requestFrame = t.spy(map, '_requestRenderFrame');

simulate.mousedown(map.getCanvas());
simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
t.ok(requestFrame.callCount > 0);

map._renderTaskQueue.run();

// https://github.com/mapbox/mapbox-gl-js/issues/6063
requestFrame.resetHistory();
simulate.mousemove(map.getCanvas(), {clientX: 20, clientY: 20});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 20, clientY: 20});
t.equal(requestFrame.callCount, 1);

map.remove();
Expand All @@ -208,7 +211,7 @@ test('DragPanHandler can interleave with another handler', (t) => {
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
Expand All @@ -221,7 +224,7 @@ test('DragPanHandler can interleave with another handler', (t) => {
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 20, clientY: 20});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 20, clientY: 20});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 2);
Expand Down Expand Up @@ -250,13 +253,13 @@ test('DragPanHandler can interleave with another handler', (t) => {
map.on('drag', drag);
map.on('dragend', dragend);

simulate.mousedown(map.getCanvas(), {[`${modifier}Key`]: true});
simulate.mousedown(map.getCanvas(), {buttons, [`${modifier}Key`]: true});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {[`${modifier}Key`]: true, clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, [`${modifier}Key`]: true, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
Expand Down Expand Up @@ -296,7 +299,7 @@ test('DragPanHandler can interleave with another handler', (t) => {
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 0);
t.equal(drag.callCount, 0);
Expand Down Expand Up @@ -359,25 +362,25 @@ test('DragPanHandler does not end a drag on right button mouseup', (t) => {
t.equal(drag.callCount, 0);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);

simulate.mousedown(map.getCanvas(), {buttons: 2, button: 2});
simulate.mousedown(map.getCanvas(), {buttons: buttons + 2, button: 2});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);

simulate.mouseup(map.getCanvas(), {buttons: 0, button: 2});
simulate.mouseup(map.getCanvas(), {buttons, button: 2});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 1);
t.equal(dragend.callCount, 0);

simulate.mousemove(map.getCanvas(), {clientX: 20, clientY: 20});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 20, clientY: 20});
map._renderTaskQueue.run();
t.equal(dragstart.callCount, 1);
t.equal(drag.callCount, 2);
Expand Down Expand Up @@ -409,7 +412,7 @@ test('DragPanHandler does not begin a drag if preventDefault is called on the mo
simulate.mousedown(map.getCanvas());
map._renderTaskQueue.run();

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();

simulate.mouseup(map.getCanvas());
Expand Down
60 changes: 60 additions & 0 deletions test/unit/ui/handler/mouse_rotate.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import {test} from '../../../util/test';
import {extend} from '../../../../src/util/util';
import window from '../../../../src/util/window';
import Map from '../../../../src/ui/map';
import DOM from '../../../../src/util/dom';
import simulate from '../../../util/simulate_interaction';
import browser from '../../../../src/util/browser';

function createMap(t, options) {
t.stub(Map.prototype, '_detectMissingCSS');
return new Map(extend({container: DOM.create('div', '', window.document.body)}, options));
}

test('MouseRotateHandler#isActive', (t) => {
const map = createMap(t);
const mouseRotate = map.handlers._handlersById.mouseRotate;

// Prevent inertial rotation.
t.stub(browser, 'now').returns(0);
t.equal(mouseRotate.isActive(), false);

simulate.mousedown(map.getCanvas(), {buttons: 2, button: 2});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), false);

simulate.mousemove(map.getCanvas(), {buttons: 2, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), true);

simulate.mouseup(map.getCanvas(), {buttons: 0, button: 2});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), false);

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

test('MouseRotateHandler#isActive #4622 regression test', (t) => {
const map = createMap(t);
const mouseRotate = map.handlers._handlersById.mouseRotate;

// Prevent inertial rotation.
simulate.mousedown(map.getCanvas(), {buttons: 2, button: 2});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), false);

simulate.mousemove(map.getCanvas(), {buttons: 2, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), true);

// Some browsers don't fire mouseup when it happens outside the window.
// Make the handler in active when it encounters a mousemove without the button pressed.

simulate.mousemove(map.getCanvas(), {buttons: 0, clientX: 10, clientY: 10});
map._renderTaskQueue.run();
t.equal(mouseRotate.isActive(), false);

map.remove();
t.end();
});
7 changes: 5 additions & 2 deletions test/unit/ui/map/isMoving.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ function createMap(t) {
return new Map({container: DOM.create('div', '', window.document.body)});
}

// MouseEvent.buttons
const buttons = 1;

test('Map#isMoving returns false by default', (t) => {
const map = createMap(t);
t.equal(map.isMoving(), false);
Expand Down Expand Up @@ -49,7 +52,7 @@ test('Map#isMoving returns true when drag panning', (t) => {
simulate.mousedown(map.getCanvas());
map._renderTaskQueue.run();

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();

simulate.mouseup(map.getCanvas());
Expand Down Expand Up @@ -139,7 +142,7 @@ test('Map#isMoving returns true when drag panning and scroll zooming interleave'
simulate.mousedown(map.getCanvas());
map._renderTaskQueue.run();

simulate.mousemove(map.getCanvas(), {clientX: 10, clientY: 10});
simulate.mousemove(map.getCanvas(), {buttons, clientX: 10, clientY: 10});
map._renderTaskQueue.run();

const browserNow = t.stub(browser, 'now');
Expand Down