Use MouseEvent.buttons for button state tracking #1921
Conversation
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
278742b to
0925be1
Compare
|
I've updated my branch and addressed all comments |
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.
CendioHalim
force-pushed
the
mouse-buttonstate-remove
branch
from
0925be1 to
19c2b83
Compare
| // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This drag handling might be on the wrong layer. Perhaps it should be moved up one level in the stack.
| 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); |
There was a problem hiding this comment.
Looks good. Do we have unit tests that make sure that buttons + wheel work at the same time?
| @@ -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); | |||
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
You should probably rather keep using the known mouse state. I.e. this._mouseButtonMask.
| 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); |
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: