Use MouseEvent.buttons for button state tracking #1921
Conversation
Instead of keeping track of button states ourselves by looking at MouseEvent.button, we can use the MouseEvent.buttons which already contains the state of all buttons.
|
|
||
| this._sendMouse(x, y, this._mouseButtonMask); | ||
| this._sendMouse(x, y, bmask); | ||
| this._mouseButtonMask = bmask; |
There was a problem hiding this comment.
If you change the order of these two, then you can retain that _sendMouse() is always called with this._mouseButtonMask.
| 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, this._mouseButtonMask | 1 << 5); | ||
| this._handleMouseButton(pos.x, pos.y, false, this._mouseButtonMask & ~(1 << 5)); |
There was a problem hiding this comment.
Do we really need the unset of the bit? Shouldn't this method be in full control of the wheel button bits?
I feel it might imply that the wheel bits might be set in other places as well.
| 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); |
There was a problem hiding this comment.
It's a bit weird keeping the down argument now that we have a full mask. But that's a cleanup I suppose we can do later.
| @@ -2941,6 +2936,35 @@ export default class RFB extends EventTargetMixin { | |||
| "raw", passwordChars, { name: "DES-ECB" }, false, ["encrypt"]); | |||
| return legacyCrypto.encrypt({ name: "DES-ECB" }, key, challenge); | |||
| } | |||
|
|
|||
| static convertButtonMask(buttons) { | |||
There was a problem hiding this comment.
This is an internal method, so it should have a _ prefix.
| @@ -2941,6 +2936,35 @@ export default class RFB extends EventTargetMixin { | |||
| "raw", passwordChars, { name: "DES-ECB" }, false, ["encrypt"]); | |||
| return legacyCrypto.encrypt({ name: "DES-ECB" }, key, challenge); | |||
| } | |||
|
|
|||
| static convertButtonMask(buttons) { | |||
There was a problem hiding this comment.
Given that it's an internal function, as is _handleMouse(), can't we put them next to each other to make it easier to read?
| @@ -3508,6 +3508,9 @@ describe('Remote Frame Buffer protocol client', function () { | |||
| // client coordinate calculations | |||
| client.focusOnClick = false; | |||
|
|
|||
| // We need to keep track of MouseEvent.buttons state | |||
| client._buttonsState = 0; | |||
There was a problem hiding this comment.
This feels like it obfuscating a lot of what the tests are supposed to do. Can't we make sure the tests provide this?
Instead of keeping track of button states ourselves by looking at
MouseEvent.button, we can use theMouseEvent.buttonswhich already contains the state of all buttons.Note that in
MouseEvent.button, button numbers are mapped as follows:But when looking at the
MouseEvent.buttonsproperty, the bits middle and right are switched, which is confusing:With this change, #1919 could be modified to work with Safari as well.
Unfortunately, the internal
_mouseButtonStateMaskstate is not removed in this PR as it would require a large rewrite of the general architecture.Tested on: