Reset modifier button's state #12187
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where modifier keys (Shift, Ctrl, Alt, Windows) remain stuck in the pressed state on VMware VMs when the browser tab/window closes unexpectedly, particularly when using a websocket reverse proxy. The solution adds event handlers to detect tab closure and release all pressed modifier keys before the connection is terminated.
Key changes:
- Introduces a centralized
_modifierKeysconfiguration object mapping modifier keys to their keysyms, codes, and button IDs - Adds
beforeunloadandpagehideevent listeners to detect tab/window closure - Implements helper methods to release individual or all pressed modifier keys
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| UI.idleControlbar(); | ||
| }, | ||
|
|
||
| _sendKeyUp(keysym, code) { |
There was a problem hiding this comment.
Missing null check for UI.rfb before calling sendKey(). If the RFB connection is not initialized, this will throw a runtime error. Add a check like if (!UI.rfb) return; at the beginning of the method, consistent with other methods in this file (e.g., lines 1386, 1402, 1444).
| _sendKeyUp(keysym, code) { | |
| _sendKeyUp(keysym, code) { | |
| if (!UI.rfb) return; |
| _releaseModifierKey(keyName) { | ||
| const keyConfig = UI._modifierKeys[keyName]; | ||
| if (!keyConfig) return false; | ||
|
|
||
| const btn = document.getElementById(keyConfig.buttonId); | ||
| if (!btn || !btn.classList.contains("noVNC_selected")) { | ||
| return false; | ||
| } | ||
|
|
||
| UI._sendKeyUp(keyConfig.keysym, keyConfig.code); | ||
| btn.classList.remove("noVNC_selected"); | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
Missing null check for UI.rfb before calling _sendKeyUp(). Although _releaseAllModifierKeys() checks for UI.rfb existence, this method can be called independently and should be defensive. Add a check like if (!UI.rfb) return false; at the beginning of the method.
| return keysReleased; | ||
| }, | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line. Remove one of the blank lines to maintain consistency with the rest of the codebase, which uses single blank lines between methods.
systemvm/agent/noVNC/app/ui.js
Outdated
|
|
||
| // Release all currently pressed modifier keys | ||
| _releaseAllModifierKeys() { | ||
| if (!UI.rfb || UI.rfb._rfbConnectionState !== 'connected') { |
There was a problem hiding this comment.
Accessing the private property _rfbConnectionState directly breaks encapsulation. The RFB class may change this internal implementation in the future. Consider using a public API method to check the connection state, or checking if UI.connected is true instead, which is already used throughout the codebase to track connection status.
| if (!UI.rfb || UI.rfb._rfbConnectionState !== 'connected') { | |
| if (!UI.connected) { |
systemvm/agent/noVNC/app/ui.js
Outdated
| if (UI.rfb._shiftPressed) { | ||
| const shiftConfig = UI._modifierKeys.shift; | ||
| UI._sendKeyUp(shiftConfig.keysym, shiftConfig.code); | ||
| keysReleased = true; | ||
| } |
There was a problem hiding this comment.
Accessing the private property _shiftPressed directly breaks encapsulation. This internal RFB state may not be reliable or could change in future versions. Additionally, there's already logic to release shift via the button state check above, making this additional check potentially redundant.
| if (UI.rfb._shiftPressed) { | |
| const shiftConfig = UI._modifierKeys.shift; | |
| UI._sendKeyUp(shiftConfig.keysym, shiftConfig.code); | |
| keysReleased = true; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12187 +/- ##
============================================
+ Coverage 3.58% 17.56% +13.97%
- Complexity 0 15545 +15545
============================================
Files 445 5910 +5465
Lines 37536 529128 +491592
Branches 6905 64636 +57731
============================================
+ Hits 1346 92919 +91573
- Misses 36024 425753 +389729
- Partials 166 10456 +10290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15916 |
2 participants
Description
Fixes #9940
Copilot Generated summary
This pull request enhances the reliability of modifier key handling in the noVNC UI, especially for VMware VMs using a websocket reverse proxy. It introduces logic to ensure all pressed modifier keys are released when the user closes the browser tab or navigates away, preventing stuck keys on the remote VM. The changes are grouped into modifier key management and event handling improvements.
Modifier key management:
_modifierKeysconfiguration object to track modifier keys (Shift, Ctrl, Alt, Windows) and their associated keysyms, codes, and button IDs in theUIobject._sendKeyUp,_releaseModifierKey, and_releaseAllModifierKeysmethods to send key release events for individual or all modifier keys, updating UI state accordingly.Event handling improvements:
beforeunloadandpagehideevent listeners to trigger modifier key release logic when the user closes the tab or navigates away, ensuring the remote VM does not retain stuck modifier keys.handleBeforeUnloadandhandlePageHidemethods to execute modifier key release logic on relevant browser events.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?