Conversation
Introduce a new `ignore_keys` setting that allows users to prevent specific keyboard inputs from being forwarded to the remote VNC host while still allowing the browser/client to handle them locally. This addresses common UX issues where browser-level shortcuts (e.g. Escape to exit fullscreen or F11 for fullscreen toggle) are also sent to the VM, potentially interrupting workflows or triggering unintended actions. Key behavior: - Keys listed in `ignore_keys` are handled locally & not sent to the VM - Default value is to not ignore any key events - Supports comma-separated input with whitespace tolerance - Accepts aliases (e.g. "esc", "ctrl", "cmd") mapped to canonical codes - Matching is case-insensitive and normalized Implementation details: - Centralized supported keys via `supportedIgnoreKeys` - Added `normalizeIgnoreKey()` to map aliases to canonical codes - Introduced `wrapRfbSendKey()` to intercept and filter outgoing key events - Simplified `keyEvent()` to delegate to `rfb.sendKey` - Added validation for user input with visual feedback on invalid entries - Added dynamic tooltip and placeholder generation from supported key list UI changes: - Added `ignore_keys` input to settings panel - Added tooltip with supported key examples - Added inline validation styling for invalid entries Tests: - Added coverage for `shouldIgnoreKey()` including aliases, normalization, whitespace handling, and edge cases - Added tests for wrapped `sendKey` behavior to ensure filtering works - Updated `keyEvent()` tests to reflect pass-through behavior - Added validation and helper function tests This change improves usability when interacting with fullscreen mode and other browser-level shortcuts, while remaining backward compatible.
samhed
left a comment
There was a problem hiding this comment.
Thank you for contributing! I think this could be a useful feature in some cases.
I tested it in Chrome 145 on Fedora 43. Functionally, it works as advertised.
The code needs some work though, see the comments.
|
|
||
| #noVNC_setting_ignore_keys.noVNC_invalid, | ||
| #noVNC_setting_ignore_keys.noVNC_invalid:focus { | ||
| border-color: var(--novnc-red); |
There was a problem hiding this comment.
I don't see why we need our own color for this, regular "red" looks fine.
There was a problem hiding this comment.
Agreed, there was no need for it. I dropped both the --novnc-red-rgb and --novnc-red variables and used plain red instead. I also removed the box-shadow, the border colour on its own is enough to signal an invalid entry.
| } | ||
| } else { | ||
| ctrl.value = value; | ||
| ctrl.value = value ?? ''; |
There was a problem hiding this comment.
Left over from an earlier iteration where the setting could still be undefined. It serves no purpose now, so I reverted it back to ctrl.value = value.
| UI.rfb.sendKey(keysym, code, down); | ||
| }, | ||
|
|
||
| wrapRfbSendKey() { |
There was a problem hiding this comment.
Renamed to filterIgnoredKeys(). While doing so I also dropped the _ignoreKeysWrapped guard, since the function only runs immediately after a fresh RFB object is created and so there was never anything to guard against.
| UI.addSettingChangeHandler('reconnect_delay'); | ||
| UI.addSettingChangeHandler('ignore_keys'); | ||
| const input = document.getElementById('noVNC_setting_ignore_keys'); | ||
| if (input && !input.dataset.validationBound) { |
There was a problem hiding this comment.
Why have you added this validationBound attribute? I can't see any reason for why these two handlers need such a guard if no others do.
We should probably get rid of the "if" and the variable as well and follow the style of the other handlers.
There was a problem hiding this comment.
You are right, there was no reason for it. I removed the guard, the variable and the two manual listeners. Validation is now registered with UI.addSettingChangeHandler("ignore_keys", UI.validateIgnoreKeysInput), exactly like the other settings.
| const ctrl = document.getElementById('noVNC_setting_' + name); | ||
| // Update cookie and form control setting. If value is not set, then | ||
| // updates from control to current cookie setting. | ||
| saveSetting(nameOrEvent) { |
There was a problem hiding this comment.
The changes to this function makes no sense to me - saveSetting() doesn't ever seem to be called with an event?
There was a problem hiding this comment.
Correct, it never is. This was left over from the earlier validation handling, which at one point passed the event through. Since that is gone, I reverted saveSetting() and updateSetting() back to their original form.
| </label> | ||
| </li> | ||
| <li><hr></li> | ||
| <li class="noVNC_setting"> |
There was a problem hiding this comment.
Most users won't need this, I think it fits best under the "advanced" section of the settings.
There was a problem hiding this comment.
Agreed, moved it into the "Advanced" section, just below the Repeater ID field.
| <div class="noVNC_setting_with_help"> | ||
| <label for="noVNC_setting_ignore_keys">Ignored Keys:</label> | ||
|
|
||
| <button id="noVNC_ignore_keys_help_button" type="button">?</button> |
There was a problem hiding this comment.
This large circled question mark doesn't fit the rest of our interface. I'd also like to avoid infrastructure for tooltips.
Couldn't we simply use the html "title" attribute to get almost the same? (With 
 we can have line breaks in the title attribute.)
There was a problem hiding this comment.
Done. The help button, the tooltip element, its CSS and the JavaScript that built it are all gone, replaced by a plain title attribute on the input using 
 for the line breaks. That also let me remove initIgnoreKeysTooltip(), buildIgnoreKeysTooltipText() and buildIgnoreKeysPlaceholder(). The supported keys are now listed directly in vnc.html, along with a short placeholder on the input itself.
- Move the setting into the "Advanced" section, where it fits better since most users won't need it. - Replace the custom tooltip (help button, CSS, and JS) with a plain "title" attribute, using &novnc#13; for line breaks. - Use plain "red" for the invalid input border instead of adding a dedicated colour variable. - Rename wrapRfbSendKey() to filterIgnoredKeys() and drop the unnecessary guard. - Revert the unrelated changes to saveSetting() and updateSetting(). - Register the validation handler via addSettingChangeHandler(), like every other setting, instead of a custom guarded listener.
Overview
Introduce a new
ignore_keyssetting that allows users to prevent specific keyboard inputs from being forwarded to the remote VNC host, while still allowing the browser to handle them locally.This addresses common UX issues where browser level shortcuts (for example Escape to exit fullscreen) are also sent to the VM, potentially interrupting workflows or triggering unintended actions.
Key behavior:
ignore_keysare handled locally and never sent to the serverImplementation details:
supportedIgnoreKeyscentralizes the keys that can be ignored, along with their aliasesnormalizeIgnoreKey()maps an alias or label to its canonical formshouldIgnoreKey()checks a key code against the current settingfilterIgnoredKeys()wrapsrfb.sendKey()when a connection is set up, so that ignored keys are dropped before reaching the servervalidateIgnoreKeysInput()flags unrecognized entries as the setting is changedUI changes:
titleattribute, using
for line breaksTests:
tests/test.ui.jscoveringshouldIgnoreKey(), verifying that aliases resolve to the right key and that keys outsidesupportedIgnoreKeysare never ignoredapp/ui.jsadded to the files served by karma, so that it can be imported from testsKnown limitation: filtering happens inside
rfb.sendKey(), which is also the path used by the keyboard toolbar buttons. Ignoringesctherefore also disables the toolbar Esc button, and ignoringctrl,altordelbreaks the Ctrl+Alt+Del button. Filtering inKeyboard._handleKeyEvent()instead would avoid this, at the cost of a change in core.This change improves usability when interacting with fullscreen mode and other browser level shortcuts, while remaining backward compatible.