From 9e6f71cb753405507f8977ed8cb64f11189ac4c2 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Fri, 27 Jan 2017 10:49:04 +0100 Subject: [PATCH] Remove modifier synchronisation The fields provided cannot tell us if it is the left or right version of the key that's pressed, so they are inherently unreliable. It is also not a huge problem in practice as we'll get in sync on the next press or release of the modifier. --- core/input/devices.js | 17 ----- core/input/util.js | 15 +--- core/rfb.js | 3 +- tests/test.helper.js | 161 ----------------------------------------- tests/test.keyboard.js | 23 ------ 5 files changed, 3 insertions(+), 216 deletions(-) diff --git a/core/input/devices.js b/core/input/devices.js index 979756dd..2308be9c 100644 --- a/core/input/devices.js +++ b/core/input/devices.js @@ -120,10 +120,6 @@ Keyboard.prototype = { //Log.Debug(">> Keyboard.ungrab"); }, - - sync: function (e) { - this._handler.syncModifiers(e); - } }; make_properties(Keyboard, [ @@ -178,10 +174,6 @@ Mouse.prototype = { _handleMouseButton: function (e, down) { if (!this._focused) { return; } - if (this._notify) { - this._notify(e); - } - var pos = this._getMousePosition(e); var bmask; @@ -248,10 +240,6 @@ Mouse.prototype = { _handleMouseWheel: function (e) { if (!this._focused) { return; } - if (this._notify) { - this._notify(e); - } - var pos = this._getMousePosition(e); if (this._onMouseButton) { @@ -278,10 +266,6 @@ Mouse.prototype = { _handleMouseMove: function (e) { if (! this._focused) { return; } - if (this._notify) { - this._notify(e); - } - var pos = this._getMousePosition(e); if (this._onMouseMove) { this._onMouseMove(pos.x, pos.y); @@ -373,7 +357,6 @@ Mouse.prototype = { make_properties(Mouse, [ ['target', 'ro', 'dom'], // DOM element that captures mouse input - ['notify', 'ro', 'func'], // Function to call to notify whenever a mouse event is received ['focused', 'rw', 'bool'], // Capture and send mouse clicks/movement ['onMouseButton', 'rw', 'func'], // Handler for mouse button click/release diff --git a/core/input/util.js b/core/input/util.js index 7dea0c09..e5363a52 100644 --- a/core/input/util.js +++ b/core/input/util.js @@ -100,8 +100,6 @@ export function ModifierSync(charModifier) { // sync on the appropriate keyboard event keydown: function(evt) { return syncKeyEvent(evt, true);}, keyup: function(evt) { return syncKeyEvent(evt, false);}, - // Call this with a non-keyboard event (such as mouse events) to use its modifier state to synchronize anyway - syncAny: function(evt) { return sync(evt);}, // if a char modifier is down, return the keys it consists of, otherwise return null activeCharModifier: function() { return hasCharModifier(charModifier, state) ? charModifier : null; } @@ -260,16 +258,10 @@ export function getKeysym(evt){ // Takes a DOM keyboard event and: // - determines which keysym it represents // - determines a code identifying the key that was pressed (corresponding to the code/keyCode properties on the DOM event) -// - synthesizes events to synchronize modifier key state between which modifiers are actually down, and which we thought were down // - marks each event with an 'escape' property if a modifier was down which should be "escaped" // This information is collected into an object which is passed to the next() function. (one call per event) export function KeyEventDecoder (modifierState, next) { "use strict"; - function sendAll(evts) { - for (var i = 0; i < evts.length; ++i) { - next(evts[i]); - } - } function process(evt, type) { var result = {type: type}; var code = getKeycode(evt); @@ -322,19 +314,16 @@ export function KeyEventDecoder (modifierState, next) { return { keydown: function(evt) { - sendAll(modifierState.keydown(evt)); + modifierState.keydown(evt); return process(evt, 'keydown'); }, keypress: function(evt) { return process(evt, 'keypress'); }, keyup: function(evt) { - sendAll(modifierState.keyup(evt)); + modifierState.keyup(evt); return process(evt, 'keyup'); }, - syncModifiers: function(evt) { - sendAll(modifierState.syncAny(evt)); - }, releaseAll: function() { next({type: 'releaseall'}); } }; }; diff --git a/core/rfb.js b/core/rfb.js index c82dd23e..430fa798 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -204,8 +204,7 @@ export default function RFB(defaults) { this._mouse = new Mouse({target: this._target, onMouseButton: this._handleMouseButton.bind(this), - onMouseMove: this._handleMouseMove.bind(this), - notify: this._keyboard.sync.bind(this._keyboard)}); + onMouseMove: this._handleMouseMove.bind(this)}); this._sock = new Websock(); this._sock.on('message', this._handle_message.bind(this)); diff --git a/tests/test.helper.js b/tests/test.helper.js index ce742f0a..f705cff0 100644 --- a/tests/test.helper.js +++ b/tests/test.helper.js @@ -155,165 +155,4 @@ describe('Helpers', function() { }); }); }); - - describe('Modifier Sync', function() { // return a list of fake events necessary to fix modifier state - describe('Toggle all modifiers', function() { - var sync = KeyboardUtil.ModifierSync(); - it ('should do nothing if all modifiers are up as expected', function() { - expect(sync.keydown({ - code: 'KeyA', - ctrlKey: false, - altKey: false, - altGraphKey: false, - shiftKey: false, - metaKey: false}) - ).to.have.lengthOf(0); - }); - it ('should synthesize events if all keys are unexpectedly down', function() { - var result = sync.keydown({ - code: 'KeyA', - ctrlKey: true, - altKey: true, - altGraphKey: true, - shiftKey: true, - metaKey: true - }); - expect(result).to.have.lengthOf(5); - var keysyms = {}; - for (var i = 0; i < result.length; ++i) { - keysyms[result[i].keysym] = (result[i].type == 'keydown'); - } - expect(keysyms[0xffe3]); - expect(keysyms[0xffe9]); - expect(keysyms[0xfe03]); - expect(keysyms[0xffe1]); - expect(keysyms[0xffe7]); - }); - it ('should do nothing if all modifiers are down as expected', function() { - expect(sync.keydown({ - code: 'KeyA', - ctrlKey: true, - altKey: true, - altGraphKey: true, - shiftKey: true, - metaKey: true - })).to.have.lengthOf(0); - }); - }); - describe('Toggle Ctrl', function() { - var sync = KeyboardUtil.ModifierSync(); - it('should sync if modifier is suddenly down', function() { - expect(sync.keydown({ - code: 'KeyA', - ctrlKey: true, - })).to.be.deep.equal([{keysym: 0xffe3, type: 'keydown'}]); - }); - it('should sync if modifier is suddenly up', function() { - expect(sync.keydown({ - code: 'KeyA', - ctrlKey: false - })).to.be.deep.equal([{keysym: 0xffe3, type: 'keyup'}]); - }); - }); - describe('Toggle Alt', function() { - var sync = KeyboardUtil.ModifierSync(); - it('should sync if modifier is suddenly down', function() { - expect(sync.keydown({ - code: 'KeyA', - altKey: true, - })).to.be.deep.equal([{keysym: 0xffe9, type: 'keydown'}]); - }); - it('should sync if modifier is suddenly up', function() { - expect(sync.keydown({ - code: 'KeyA', - altKey: false - })).to.be.deep.equal([{keysym: 0xffe9, type: 'keyup'}]); - }); - }); - describe('Toggle AltGr', function() { - var sync = KeyboardUtil.ModifierSync(); - it('should sync if modifier is suddenly down', function() { - expect(sync.keydown({ - code: 'KeyA', - altGraphKey: true, - })).to.be.deep.equal([{keysym: 0xfe03, type: 'keydown'}]); - }); - it('should sync if modifier is suddenly up', function() { - expect(sync.keydown({ - code: 'KeyA', - altGraphKey: false - })).to.be.deep.equal([{keysym: 0xfe03, type: 'keyup'}]); - }); - }); - describe('Toggle Shift', function() { - var sync = KeyboardUtil.ModifierSync(); - it('should sync if modifier is suddenly down', function() { - expect(sync.keydown({ - code: 'KeyA', - shiftKey: true, - })).to.be.deep.equal([{keysym: 0xffe1, type: 'keydown'}]); - }); - it('should sync if modifier is suddenly up', function() { - expect(sync.keydown({ - code: 'KeyA', - shiftKey: false - })).to.be.deep.equal([{keysym: 0xffe1, type: 'keyup'}]); - }); - }); - describe('Toggle Meta', function() { - var sync = KeyboardUtil.ModifierSync(); - it('should sync if modifier is suddenly down', function() { - expect(sync.keydown({ - code: 'KeyA', - metaKey: true, - })).to.be.deep.equal([{keysym: 0xffe7, type: 'keydown'}]); - }); - it('should sync if modifier is suddenly up', function() { - expect(sync.keydown({ - code: 'KeyA', - metaKey: false - })).to.be.deep.equal([{keysym: 0xffe7, type: 'keyup'}]); - }); - }); - describe('Modifier keyevents', function() { - it('should not sync a modifier on its own events', function() { - expect(KeyboardUtil.ModifierSync().keydown({ - code: 'ControlLeft', - ctrlKey: false - })).to.be.deep.equal([]); - expect(KeyboardUtil.ModifierSync().keydown({ - code: 'ControlLeft', - ctrlKey: true - }), 'B').to.be.deep.equal([]); - }) - it('should update state on modifier keyevents', function() { - var sync = KeyboardUtil.ModifierSync(); - sync.keydown({ - code: 'ControlLeft', - }); - expect(sync.keydown({ - code: 'KeyA', - ctrlKey: true, - })).to.be.deep.equal([]); - }); - it('should sync other modifiers on ctrl events', function() { - expect(KeyboardUtil.ModifierSync().keydown({ - code: 'ControlLeft', - altKey: true - })).to.be.deep.equal([{keysym: 0xffe9, type: 'keydown'}]); - }) - }); - describe('sync modifiers on non-key events', function() { - it('should generate sync events when receiving non-keyboard events', function() { - expect(KeyboardUtil.ModifierSync().syncAny({ - altKey: true - })).to.be.deep.equal([{keysym: 0xffe9, type: 'keydown'}]); - }); - }); - describe('do not treat shift as a modifier key', function() { - it('should not treat shift as a char modifier', function() { - expect(KeyboardUtil.hasCharModifier([], {0xffe1 : true})).to.be.false; - }); - }); - }); }); diff --git a/tests/test.keyboard.js b/tests/test.keyboard.js index 19a8a66a..7ecfcaf1 100644 --- a/tests/test.keyboard.js +++ b/tests/test.keyboard.js @@ -26,29 +26,6 @@ describe('Key Event Pipeline Stages', function() { done(); }).keydown({code: 'KeyA', key: 'a'}); }); - it('should not sync modifiers on a keypress', function() { - // Firefox provides unreliable modifier state on keypress events - var count = 0; - KeyboardUtil.KeyEventDecoder(KeyboardUtil.ModifierSync(), function(evt) { - ++count; - }).keypress({code: 'KeyA', key: 'a', ctrlKey: true}); - expect(count).to.be.equal(1); - }); - it('should sync modifiers if necessary', function(done) { - var count = 0; - KeyboardUtil.KeyEventDecoder(KeyboardUtil.ModifierSync(), function(evt) { - switch (count) { - case 0: // fake a ctrl keydown - expect(evt).to.be.deep.equal({keysym: 0xffe3, type: 'keydown'}); - ++count; - break; - case 1: - expect(evt).to.be.deep.equal({code: 'KeyA', type: 'keydown', keysym: 0x61}); - done(); - break; - } - }).keydown({code: 'KeyA', key: 'a', ctrlKey: true}); - }); it('should forward keydown events with the right type', function(done) { KeyboardUtil.KeyEventDecoder(KeyboardUtil.ModifierSync(), function(evt) { expect(evt).to.be.deep.equal({code: 'KeyA', keysym: 0x61, type: 'keydown'});