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

Use MouseEvent.buttons for button state tracking #1921

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

Use MouseEvent.buttons for button state tracking #1921

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
102 changes: 66 additions & 36 deletions core/rfb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,35 @@ export default class RFB extends EventTargetMixin {
this.sendKey(keysym, code, down);
}

static _convertButtonMask(buttons) {
/* The bits in MouseEvent.buttons property correspond
* to the following mouse buttons:
* 0: Left
* 1: Right
* 2: Middle
* 3: Back
* 4: Forward
*
* These bits needs to be converted to what they are defined as
* in the RFB protocol.
*/

const buttonMaskMap = {
0: 1 << 0, // Left
1: 1 << 2, // Right
2: 1 << 1, // Middle
3: 1 << 7, // Back
};

let bmask = 0;
for (let i = 0; i < 4; i++) {
if (buttons & (1 << i)) {
bmask |= buttonMaskMap[i];
}
}
return bmask;
}

_handleMouse(ev) {
/*
* We don't check connection status or viewOnly here as the
Expand Down Expand Up @@ -1060,15 +1089,15 @@ export default class RFB extends EventTargetMixin {
let pos = clientToElement(ev.clientX, ev.clientY,
this._canvas);

let bmask = RFB._convertButtonMask(ev.buttons);

switch (ev.type) {
case 'mousedown':
setCapture(this._canvas);
this._handleMouseButton(pos.x, pos.y,
true, 1 << ev.button);
this._handleMouseButton(pos.x, pos.y, true, bmask);
CendioHalim marked this conversation as resolved.
Show resolved Hide resolved
break;
case 'mouseup':
this._handleMouseButton(pos.x, pos.y,
false, 1 << ev.button);
this._handleMouseButton(pos.x, pos.y, false, bmask);
break;
case 'mousemove':
this._handleMouseMove(pos.x, pos.y);
Expand Down Expand Up @@ -1097,7 +1126,7 @@ export default class RFB extends EventTargetMixin {
// Otherwise we treat this as a mouse click event.
// Send the button down event here, as the button up
// event is sent at the end of this function.
this._sendMouse(x, y, bmask);
this._sendMouse(x, y, this._mouseButtonMask);
Comment on lines 1126 to +1129
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the comment here has been considered. Can you still click in place with dragging active?

I guess that means we are lacking at least one unit test.

Copy link
Member

Choose a reason for hiding this comment

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

This drag handling might be on the wrong layer. Perhaps it should be moved up one level in the stack.

Copy link
Author

Choose a reason for hiding this comment

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

Nope, clicking in dragging mode is broken. Moving up the drag handling one level has the added benefit of removing the down argument from _handleMouseButton(), which would be nice.

}
}

Expand All @@ -1108,12 +1137,7 @@ export default class RFB extends EventTargetMixin {
this._sendMouse(x, y, this._mouseButtonMask);
}

if (down) {
this._mouseButtonMask |= bmask;
} else {
this._mouseButtonMask &= ~bmask;
}

this._mouseButtonMask = bmask;
CendioHalim marked this conversation as resolved.
Show resolved Hide resolved
this._sendMouse(x, y, this._mouseButtonMask);
}

Expand Down Expand Up @@ -1177,6 +1201,7 @@ export default class RFB extends EventTargetMixin {
let pos = clientToElement(ev.clientX, ev.clientY,
this._canvas);

let bmask = RFB._convertButtonMask(ev.buttons);
let dX = ev.deltaX;
let dY = ev.deltaY;

Expand All @@ -1196,26 +1221,27 @@ export default class RFB extends EventTargetMixin {
this._accumulatedWheelDeltaX += dX;
this._accumulatedWheelDeltaY += dY;


// Generate a mouse wheel step event when the accumulated delta
// for one of the axes is large enough.
if (Math.abs(this._accumulatedWheelDeltaX) >= WHEEL_STEP) {
if (this._accumulatedWheelDeltaX < 0) {
this._handleMouseButton(pos.x, pos.y, true, 1 << 5);
this._handleMouseButton(pos.x, pos.y, false, 1 << 5);
this._handleMouseButton(pos.x, pos.y, true, bmask | 1 << 5);
this._handleMouseButton(pos.x, pos.y, false, bmask);
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Do we have unit tests that make sure that buttons + wheel work at the same time?

} else if (this._accumulatedWheelDeltaX > 0) {
this._handleMouseButton(pos.x, pos.y, true, 1 << 6);
this._handleMouseButton(pos.x, pos.y, false, 1 << 6);
this._handleMouseButton(pos.x, pos.y, true, bmask | 1 << 6);
this._handleMouseButton(pos.x, pos.y, false, bmask);
}

this._accumulatedWheelDeltaX = 0;
}
if (Math.abs(this._accumulatedWheelDeltaY) >= WHEEL_STEP) {
if (this._accumulatedWheelDeltaY < 0) {
this._handleMouseButton(pos.x, pos.y, true, 1 << 3);
this._handleMouseButton(pos.x, pos.y, false, 1 << 3);
this._handleMouseButton(pos.x, pos.y, true, bmask | 1 << 3);
this._handleMouseButton(pos.x, pos.y, false, bmask);
} else if (this._accumulatedWheelDeltaY > 0) {
this._handleMouseButton(pos.x, pos.y, true, 1 << 4);
this._handleMouseButton(pos.x, pos.y, false, 1 << 4);
this._handleMouseButton(pos.x, pos.y, true, bmask | 1 << 4);
this._handleMouseButton(pos.x, pos.y, false, bmask);
}

this._accumulatedWheelDeltaY = 0;
Expand Down Expand Up @@ -1255,7 +1281,7 @@ export default class RFB extends EventTargetMixin {

this._fakeMouseMove(this._gestureFirstDoubleTapEv, pos.x, pos.y);
this._handleMouseButton(pos.x, pos.y, true, bmask);
this._handleMouseButton(pos.x, pos.y, false, bmask);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
}

_handleGesture(ev) {
Expand Down Expand Up @@ -1307,33 +1333,36 @@ export default class RFB extends EventTargetMixin {
case 'longpress':
this._fakeMouseMove(ev, pos.x, pos.y);
break;
case 'twodrag':
case 'twodrag': {
let bmask = RFB._convertButtonMask(ev.detail.buttons);
Copy link
Member

Choose a reason for hiding this comment

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

There is no buttons attribute for gesture events. I'm concerned that this passes the unit tests. Something must be missing/wrong there.

// Always scroll in the same position.
// We don't know if the mouse was moved so we need to move it
// every update.
this._fakeMouseMove(ev, pos.x, pos.y);
while ((ev.detail.magnitudeY - this._gestureLastMagnitudeY) > GESTURE_SCRLSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x8);
this._handleMouseButton(pos.x, pos.y, false, 0x8);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x8);
Copy link
Member

Choose a reason for hiding this comment

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

You should probably rather keep using the known mouse state. I.e. this._mouseButtonMask.

this._handleMouseButton(pos.x, pos.y, false, 0x0);
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here.

this._gestureLastMagnitudeY += GESTURE_SCRLSENS;
}
while ((ev.detail.magnitudeY - this._gestureLastMagnitudeY) < -GESTURE_SCRLSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x10);
this._handleMouseButton(pos.x, pos.y, false, 0x10);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x10);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
this._gestureLastMagnitudeY -= GESTURE_SCRLSENS;
}
while ((ev.detail.magnitudeX - this._gestureLastMagnitudeX) > GESTURE_SCRLSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x20);
this._handleMouseButton(pos.x, pos.y, false, 0x20);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x20);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
this._gestureLastMagnitudeX += GESTURE_SCRLSENS;
}
while ((ev.detail.magnitudeX - this._gestureLastMagnitudeX) < -GESTURE_SCRLSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x40);
this._handleMouseButton(pos.x, pos.y, false, 0x40);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x40);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
this._gestureLastMagnitudeX -= GESTURE_SCRLSENS;
}
break;
case 'pinch':
}
case 'pinch': {
let bmask = RFB._convertButtonMask(ev.detail.buttons);
// Always scroll in the same position.
// We don't know if the mouse was moved so we need to move it
// every update.
Expand All @@ -1342,18 +1371,19 @@ export default class RFB extends EventTargetMixin {
if (Math.abs(magnitude - this._gestureLastMagnitudeX) > GESTURE_ZOOMSENS) {
this._handleKeyEvent(KeyTable.XK_Control_L, "ControlLeft", true);
while ((magnitude - this._gestureLastMagnitudeX) > GESTURE_ZOOMSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x8);
this._handleMouseButton(pos.x, pos.y, false, 0x8);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x8);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
this._gestureLastMagnitudeX += GESTURE_ZOOMSENS;
}
while ((magnitude - this._gestureLastMagnitudeX) < -GESTURE_ZOOMSENS) {
this._handleMouseButton(pos.x, pos.y, true, 0x10);
this._handleMouseButton(pos.x, pos.y, false, 0x10);
this._handleMouseButton(pos.x, pos.y, true, bmask | 0x10);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
this._gestureLastMagnitudeX -= GESTURE_ZOOMSENS;
}
}
this._handleKeyEvent(KeyTable.XK_Control_L, "ControlLeft", false);
break;
}
}
break;

Expand All @@ -1367,11 +1397,11 @@ export default class RFB extends EventTargetMixin {
break;
case 'drag':
this._fakeMouseMove(ev, pos.x, pos.y);
this._handleMouseButton(pos.x, pos.y, false, 0x1);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
break;
case 'longpress':
this._fakeMouseMove(ev, pos.x, pos.y);
this._handleMouseButton(pos.x, pos.y, false, 0x4);
this._handleMouseButton(pos.x, pos.y, false, 0x0);
break;
}
break;
Expand Down
Loading
Loading