From 009bd4726d81c8ac90b432c07363f9391edff77d Mon Sep 17 00:00:00 2001 From: quickiwiki Date: Fri, 18 Jul 2025 14:37:13 +0500 Subject: [PATCH] Bugfix/vnc 130 ime race condition (#137) * VNC-130 Added IME support for composition updates, and optimized key event handling --- app/ui.js | 15 +-- core/input/keyboard.js | 240 ++++++++++++++++++---------------------- core/input/keysymdef.js | 2 +- core/rfb.js | 2 +- 4 files changed, 116 insertions(+), 143 deletions(-) diff --git a/app/ui.js b/app/ui.js index 79553d9e..d3f004fb 100644 --- a/app/ui.js +++ b/app/ui.js @@ -657,17 +657,12 @@ const UI = { * ------v------*/ // Ignore clicks that are propogated from child elements in sub panels isControlPanelItemClick(e) { - if (!(e && e.target && e.target.classList && e.target.parentNode && - ( - e.target.classList.contains('noVNC_button') && e.target.parentNode.id !== 'noVNC_modifiers' || - e.target.classList.contains('noVNC_button_div') || - e.target.classList.contains('noVNC_heading') - ) - )) { + if (!e?.target?.classList || !e?.target?.parentNode) return false; - } - return true; + return e.target.classList.contains('noVNC_button') && e.target.parentNode?.id !== 'noVNC_modifiers' || + e.target.classList.contains('noVNC_button_div') || + e.target.classList.contains('noVNC_heading'); }, // Disable/enable controls depending on connection state @@ -1417,6 +1412,7 @@ const UI = { * ------v------*/ connect(event, password) { + Log.Debug("UI.connect"); // Ignore when rfb already exists if (typeof UI.rfb !== 'undefined') { @@ -1711,6 +1707,7 @@ const UI = { //receive message from parent window receiveMessage(event) { if (event.data && event.data.action) { + Log.Debug("Received message from parent window: " + event.data.action); switch (event.data.action) { case 'clipboardsnd': if (UI.rfb && UI.rfb.clipboardUp) { diff --git a/core/input/keyboard.js b/core/input/keyboard.js index 169c59b6..ae8c0851 100644 --- a/core/input/keyboard.js +++ b/core/input/keyboard.js @@ -11,12 +11,12 @@ import KeyTable from "./keysym.js"; import keysyms from "./keysymdef.js"; import imekeys from "./imekeys.js"; import * as browser from "../util/browser.js"; -import { isChromiumBased } from '../util/browser.js'; // // Keyboard event handler // +const thresholdTime = 16; export default class Keyboard { constructor(screenInput, touchInput) { this._screenInput = screenInput; @@ -26,22 +26,25 @@ export default class Keyboard { // (even if they are happy) this._altGrArmed = false; // Windows AltGr detection + this._rfbKeyQueue = []; + this._lastSendTime = 0; + // keep these here so we can refer to them later this._eventHandlers = { 'keyup': this._handleKeyUp.bind(this), 'keydown': this._handleKeyDown.bind(this), 'blur': this._allKeysUp.bind(this), - 'compositionstart' : this._handleCompositionStart.bind(this), - 'compositionend' : this._handleCompositionEnd.bind(this), - 'input' : this._handleInput.bind(this) + 'compositionstart': this._handleCompositionStart.bind(this), + 'compositionend': this._handleCompositionEnd.bind(this), + 'compositionupdate': this._handleCompositionUpdate.bind(this), + 'input': this._handleInput.bind(this) }; // ===== EVENT HANDLERS ===== this.onkeyevent = () => {}; // Handler for key press/release this._enableIME = false; - this._imeHold = false; - this._imeInProgress = false; + this._imeStarted = false; this._lastKeyboardInput = null; this._defaultKeyboardInputLen = 100; this._keyboardInputReset(); @@ -51,8 +54,8 @@ export default class Keyboard { // ===== PUBLIC METHODS ===== get enableIME() { return this._enableIME; } - set enableIME(val) { - this._enableIME = val; + set enableIME(val) { + this._enableIME = val; this.focus(); } @@ -64,12 +67,11 @@ export default class Keyboard { clearKeysDown(event) { // On some Operating systems, the browser will lose key up events when a shortcut key combination triggers something // on the OS that is outside the scope of the browser. For example, MacOS Cmd+Shift+Ctrl+4 brings up a screen capture - // tool and the browser only recieves some of the key down events, but not the key up events. This leaves the server + // tool and the browser only recieves some of the key down events, but not the key up events. This leaves the server // out of sync, with cetain keys stuck down. This attempts to discover and fix these occurances in a OS nuetral way if (event) { - for (const [key, value] of Object.entries(this._keyDownList)) { - switch(key) { + switch (key) { case "ControlLeft": case "ControlRight": if (!event.ctrlKey) { @@ -103,6 +105,29 @@ export default class Keyboard { } } + _scheduleRfbKeySend() { + if (this._rfbKeyQueue.length === 0) return; + + const process = (timestamp) => { + const elapsed = timestamp - this._lastSendTime; + if (elapsed > thresholdTime) { + while (this._rfbKeyQueue.length > 0) { + const event = this._rfbKeyQueue.shift(); + Log.Debug("onkeyevent " + (event.down ? "down" : "up") + + ", keysym: " + event.keysym, ", code: " + event.code); + this.onkeyevent(event.keysym, event.code, event.down); + } + this._lastSendTime = timestamp; + } + + if (this._rfbKeyQueue.length > 0) { + requestAnimationFrame(process); + } + }; + + requestAnimationFrame(process); + } + _sendKeyEvent(keysym, code, down) { if (down) { this._keyDownList[code] = keysym; @@ -114,9 +139,13 @@ export default class Keyboard { delete this._keyDownList[code]; } - Log.Debug("onkeyevent " + (down ? "down" : "up") + - ", keysym: " + keysym, ", code: " + code); - this.onkeyevent(keysym, code, down); + this._rfbKeyQueue.push({keysym: keysym, code: code, down: down}); + this._scheduleRfbKeySend(); + } + + _sendKeyStroke(keySym, code) { + this._sendKeyEvent(keySym, code, true); + this._sendKeyEvent(keySym, code, false); } _getKeyCode(e) { @@ -137,11 +166,11 @@ export default class Keyboard { // is not layout independent, so it is as bad as using keyCode if (e.keyIdentifier) { // Non-character key? - if (e.keyIdentifier.substr(0, 2) !== 'U+') { + if (!e.keyIdentifier.startsWith('U+')) { return e.keyIdentifier; } - const codepoint = parseInt(e.keyIdentifier.substr(2), 16); + const codepoint = parseInt(e.keyIdentifier.substring(2), 16); const char = String.fromCharCode(codepoint).toUpperCase(); return 'Platform' + char.charCodeAt(); @@ -151,134 +180,85 @@ export default class Keyboard { } _handleCompositionStart(e) { - Log.Debug("composition started"); - if (this._enableIME) { - this._imeHold = true; - this._imeInProgress = true; + Log.Debug("Composition started: " + e.data); + this._imeStarted = true; + this._lastKeyboardInput = ""; + } + + _handleCompositionUpdate(e) { + Log.Debug("Composition update: " + e.data); + const oldValue = this._lastKeyboardInput; + const newValue = e.data; + let diffStart = 0; + + if (this._imeStarted) { + this._sendKeyStroke(keysyms.lookup(newValue.charCodeAt(0)), 'Unidentified'); + this._imeStarted = false; + } else { + //find position where difference starts + for (let i = 0; i < Math.min(oldValue.length, newValue.length); i++) { + if (newValue.charAt(i) !== oldValue.charAt(i)) { + break; + } + diffStart++; + } + + //send backspaces if needed + Log.Debug("Backspace diffStart: " + diffStart); + Log.Debug("Old value: " + oldValue + " Old value length: " + oldValue.length + " New value: " + newValue); + for (let bs = oldValue.length - diffStart; bs > 0; bs--) { + this._sendKeyStroke(KeyTable.XK_BackSpace, "Backspace"); + } + + //send new keys + for (let i = diffStart; i < newValue.length; i++) { + this._sendKeyStroke(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified'); + } } + this._lastKeyboardInput = newValue; + //this._touchInput.focus(); } _handleCompositionEnd(e) { - Log.Debug("Composition ended"); - if (this._enableIME) { this._imeInProgress = false; } - if (isChromiumBased()) { - this._imeHold = false; - } + Log.Debug("Composition ended: " + e.data); + this._touchInput.value = ''; } _handleInput(e) { //input event occurs only when keyup keydown events don't prevent default - //IME events will make this happen, for example + //IME events will make this happen, for example //IME changes can back out old characters and replace, thus send differential if IME //otherwise send new characters - if (this._enableIME && this._imeHold) { - Log.Debug("IME input change, sending differential"); - if (!this._imeInProgress) { - this._imeHold = false; //Firefox fires compisitionend before last input change - } - - const oldValue = this._lastKeyboardInput; - const newValue = e.target.value; - let diff_start = 0; - - //find position where difference starts - for (let i = 0; i < Math.min(oldValue.length, newValue.length); i++) { - if (newValue.charAt(i) != oldValue.charAt(i)) { - break; - } - diff_start++; - } - - //send backspaces if needed - for (let bs = oldValue.length - diff_start; bs > 0; bs--) { - this._sendKeyEvent(KeyTable.XK_BackSpace, "Backspace", true); - this._sendKeyEvent(KeyTable.XK_BackSpace, "Backspace", false); - } - - //send new keys - for (let i = diff_start; i < newValue.length; i++) { - this._sendKeyEvent(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified', true); - this._sendKeyEvent(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified', false); - } - this._lastKeyboardInput = newValue; - } else { + Log.Debug("Current buffer: " + this._touchInput.value + " Input: " + e.data + " isComposing: " + e.isComposing + " input.type: " + e.inputType); + if (!e.isComposing && e.inputType !== "insertCompositionText") { Log.Debug("Non-IME input change, sending new characters"); - const newValue = e.target.value; + const newValue = e.data; - if (!this._lastKeyboardInput) { - this._keyboardInputReset(); + for (let i = 0; i < newValue.length; i++) { + this._sendKeyStroke(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified'); } - const oldValue = this._lastKeyboardInput; - let newLen; - - try { - // Try to check caret position since whitespace at the end - // will not be considered by value.length in some browsers - newLen = Math.max(e.target.selectionStart, newValue.length); - } catch (err) { - // selectionStart is undefined in Google Chrome - newLen = newValue.length; - } - const oldLen = oldValue.length; - - let inputs = newLen - oldLen; - let backspaces = inputs < 0 ? -inputs : 0; - - // Compare the old string with the new to account for - // text-corrections or other input that modify existing text - for (let i = 0; i < Math.min(oldLen, newLen); i++) { - if (newValue.charAt(i) != oldValue.charAt(i)) { - inputs = newLen - i; - backspaces = oldLen - i; - break; - } - } - - // Send the key events - for (let i = 0; i < backspaces; i++) { - this._sendKeyEvent(KeyTable.XK_BackSpace, "Backspace", true); - this._sendKeyEvent(KeyTable.XK_BackSpace, "Backspace", false); - } - for (let i = newLen - inputs; i < newLen; i++) { - this._sendKeyEvent(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified', true); - this._sendKeyEvent(keysyms.lookup(newValue.charCodeAt(i)), 'Unidentified', false); - } - - // Control the text content length in the keyboardinput element - if (newLen > 2 * this._defaultKeyboardInputLen) { - this._keyboardInputReset(); - } else if (newLen < 1) { - // There always have to be some text in the keyboardinput - // element with which backspace can interact. - this._keyboardInputReset(); - // This sometimes causes the keyboard to disappear for a second - // but it is required for the android keyboard to recognize that - // text has been added to the field - e.target.blur(); - // This has to be ran outside of the input handler in order to work - setTimeout(e.target.focus.bind(e.target), 0); - } else { - this._lastKeyboardInput = newValue; - } + this._touchInput.value = ''; } } _keyboardInputReset() { - this._touchInput.value = new Array(this._defaultKeyboardInputLen).join("_"); + this._touchInput.value = ""; this._lastKeyboardInput = this._touchInput.value; } _handleKeyDown(e) { + Log.Debug("Key Down: " + e.keyCode + " isComposing: " + e.isComposing); + if (e.isComposing || e.keyCode === 229) { + //skip event if IME related + Log.Debug("Skipping keydown, IME interaction, keycode: " + e.keyCode); + return; + } + const code = this._getKeyCode(e); let keysym = KeyboardUtil.getKeysym(e); this.clearKeysDown(e); - - if (this._isIMEInteraction(e)) { - //skip event if IME related - Log.Debug("Skipping keydown, IME interaction, code: " + code + " keysym: " + keysym + " keycode: " + e.keyCode); - return; - } + Log.Debug("Key Down: " + e.keyCode + " code: " + code + " keysym: " + keysym); // Windows doesn't have a proper AltGr, but handles it using // fake Ctrl+Alt. However the remote end might not be Windows, @@ -401,13 +381,13 @@ export default class Keyboard { } _handleKeyUp(e) { - const code = this._getKeyCode(e); - - if (this._isIMEInteraction(e)) { + Log.Debug("Key Up: " + e.keyCode + " Buffer: " + this._touchInput.value); + if (e.isComposing || e.keyCode === 229) { //skip IME related events - Log.Debug("Skipping keyup, IME interaction, code: " + code + " keycode: " + e.keyCode); + Log.Debug("Skipping keyup, IME interaction, keycode: " + e.keyCode); return; } + const code = this._getKeyCode(e); stopEvent(e); // We can't get a release in the middle of an AltGr sequence, so @@ -459,10 +439,10 @@ export default class Keyboard { _isIMEInteraction(e) { //input must come from touchinput (textarea) and ime must be enabled - if (e.target != this._touchInput || !this._enableIME) { return false; } + if (e.target !== this._touchInput || !this._enableIME) { return false; } //keyCode of 229 is IME composition - if (e.keyCode == 229) { + if (e.keyCode === 229) { return true; } @@ -471,11 +451,7 @@ export default class Keyboard { //we can't do that with none character keys though //Firefox does not seem to fire key events for IME interaction but Chrome does //TODO: potentially skip this for Firefox browsers, needs more testing with different IME types - if (e.keyCode in imekeys) { - return true; - } - - return false; + return e.keyCode in imekeys; } // ===== PUBLIC METHODS ===== @@ -498,13 +474,13 @@ export default class Keyboard { grab() { //Log.Debug(">> Keyboard.grab"); - this._screenInput.addEventListener('keydown', this._eventHandlers.keydown); this._screenInput.addEventListener('keyup', this._eventHandlers.keyup); this._touchInput.addEventListener('keydown', this._eventHandlers.keydown); this._touchInput.addEventListener('keyup', this._eventHandlers.keyup); this._touchInput.addEventListener('compositionstart', this._eventHandlers.compositionstart); + this._touchInput.addEventListener('compositionupdate', this._eventHandlers.compositionupdate); this._touchInput.addEventListener('compositionend', this._eventHandlers.compositionend); this._touchInput.addEventListener('input', this._eventHandlers.input); @@ -516,13 +492,13 @@ export default class Keyboard { ungrab() { //Log.Debug(">> Keyboard.ungrab"); - this._screenInput.removeEventListener('keydown', this._eventHandlers.keydown); this._screenInput.removeEventListener('keyup', this._eventHandlers.keyup); this._touchInput.removeEventListener('keydown', this._eventHandlers.keydown); this._touchInput.removeEventListener('keyup', this._eventHandlers.keyup); this._touchInput.removeEventListener('compositionstart', this._eventHandlers.compositionstart); + this._touchInput.removeEventListener('compositionupdate', this._eventHandlers.compositionupdate); this._touchInput.removeEventListener('compositionend', this._eventHandlers.compositionend); this._touchInput.removeEventListener('input', this._eventHandlers.input); diff --git a/core/input/keysymdef.js b/core/input/keysymdef.js index 951cacab..d2a7455a 100644 --- a/core/input/keysymdef.js +++ b/core/input/keysymdef.js @@ -682,7 +682,7 @@ export default { return keysym; } - // General mapping as final fallback + // General mapping as the final fallback return 0x01000000 | u; }, }; diff --git a/core/rfb.js b/core/rfb.js index d4229390..6727b5c2 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -399,6 +399,7 @@ export default class RFB extends EventTargetMixin { get viewOnly() { return this._viewOnly; } set viewOnly(viewOnly) { + Log.Debug("Setting viewOnly to " + viewOnly); this._viewOnly = viewOnly; if (this._rfbConnectionState === "connecting" || @@ -986,7 +987,6 @@ export default class RFB extends EventTargetMixin { if (!keysym) { return; } - Log.Info("Sending keysym (" + (down ? "down" : "up") + "): " + keysym); if (this._isPrimaryDisplay) { RFB.messages.keyEvent(this._sock, keysym, down ? 1 : 0); } else {