-
Notifications
You must be signed in to change notification settings - Fork 223
feat(ui): Enhances virtual keyboard with sticky modifier key support #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@ym @adamshiervani This is ready for review and works really well ;) I am not sure about the CSS style of the "depressed" buttons, I made something up and welcome other suggestions. |
2020b6c
to
642f0a5
Compare
Rebased and ready for review @ym |
47c45b5
to
6c3ee8c
Compare
4d5eaf5
to
77d302e
Compare
Rebased and squashed. Ready again |
70e4c7f
to
acbdd23
Compare
acbdd23
to
84e4b44
Compare
@ym and I just reviewed this PR. Overall, it looks good - but the underlying keyboard state management is broken. There are two main issues. First, the data source is split: for keyboard LED state, we fetch from the device; for all other keyboard state, we rely on JavaScript variables - unless the device doesn’t support LED state. In that case, we should theoretically fall back to the JS state. The problem is that the current codebase handles this inconsistency poorly. Some parts assume the keyboard LED state is always available from the device, while other parts assume it may not be. This results in a confusing and brittle implementation that’s hard to follow and maintain. Our suggestion is to take a more declarative approach: treat the device as the single source of truth for the entire keyboard state, including LED status. Let that state flow into the UI via messages over WebRTC. That would mean introducing a unified “super keyboard state” on the device side - one that includes all the elements we want to track. The device can manage any conflicts internally (e.g. between actual LED status and internal state) and then broadcast a consistent, resolved state to the UI. What do you think? |
Thanks @IDisposable - really appreciate the thoughtful breakdown! Yes, treating the device as the single source of truth for the entire keyboard state - including both modifier keys and LED status - makes the most sense long term for me. Any internal conflicts (for example, between LED state and modifier press/release timing) should be resolved on the device side. From there, the device can emit a clean, unified “super keyboard state” that flows into the UI via WebRTC. On the LED state specifically: even if it arrives with some delay, it should always take precedence and override whatever the UI thought earlier. That’s consistent with how the remote host behaves - if it says Caps Lock is off, then it’s off, regardless of what the browser previously assumed. I mean, in whatever the browser thinks, as far as I see, will be irrelevant, if the LED state says something else. If LED state isn’t available from the device, we’ll just fall back to JS-tracked state and, well, depending on your level of atheism, pray that it's in "sync" with the remote host. As for the event flow: both WebRTCVideo and VirtualKeyboard should send all relevant modifier key events to the device. In the case of WebRTCVideo, we’d send them with sticky = false. As for the Virtual Keyboard, should we have a sticky/record+release button in the Virtual Keyboard, or what do you think? I'm not super dogmatic on a certain approach. After that, we can drop all local state and just react to whatever comes back from the device. Regarding macro playback - that’s a great point. For now, I'd say we treat macros as stupidly as possible. We’ll forward the modifier key events to the device and let it manage state updates as usual. No need to ignore LED state during playback or resync afterward for now. If we end up needing tighter control (e.g. to reset before/after execution), we can always add a toggle to the macro creator later. Lastly, I think we can safely remove the LED tracking mode setting (Host Only, Browser Only, Automatic). Let’s simplify and remove the settings. Always let the device decide what the correct state is, and if LED data is available, just merge that in and go with it. This should reduce ambiguity and eliminate some of the weird edge cases we’ve been dealing with. Let me know if that makes sense or if you’d like to hash out any specifics further. Happy to help shape the unified state logic. Lastly, sorry for the delay on this one. Totally get if you're frustrated with the latency, and don't want to spend time on this anymore. If so, totally understand, just let me know <3. |
Hoping I'm being helpful instead of annoying. Please just let me know (e.g. via email IDisposable @ gmail . com) if you ever need me to do something else or shush.
Yes. That's so much more simple. I think the big thing it drives is that when someone presses a modifier key, we need to just send the modifier key through to the device, let it reply with the current modifier state, track that browser-side ONLY for the purposes of showing the correct thing on the modifier (lower right) and keyboard state.
I love this, it makes it trivial to forward key-up/down events to the device, and trusting the replied state in the browser. I wonder if adding a (optional) overlay on the full-screen (e.g. lower right corner) that has the modifier state in visible form (Shft /Ctrl/Alt/AltGr or something) would help users on the browser side see what's active on the device's last reply
Can we implement that as fall back to the device-tracked state. e.g. send the current state from the device to the browser... and then let the user do the press-release dance on keys if the current state doesn't reflect what they think it should be based on the indicators on the lower right? That way, the device is always authoritative (and would be tracking or not the LED-status from the host if enabled... but that's completely opaque to the browser).
Gotcha, so we send what we know (from the last mod-status reply). For macros, do we want to send a "resync" message at the start to get back the current state?
Hmmm, so the point of virtual keyboard, this sticky-modification... is that pressing the Shift key (or other normally not a toggle modifier) would send the key-down event for the modifier, but NOT send the key up until you click the key a second time, right? So you could click
... sorta... I think we're ALWAYS sending just the Key-Down and Key-Up events, and always trusting the device's state management... the only thing that's odd is that when pressing the physical key we send the key down, and when it's released we sent the key up... but for the Virtual Key "chips", one click presses down the Shift key, which is sent as a Key Down to device, device changes the keyboard state, which when received causes the Virtual Keyboard layout to change to the "Shifted" state... which shows the shift-keys "down"/highlighted... and they stay that way until you click the shift chip AGAIN... when it sends a Shift-Key-Up event to the device, and trusts the reply's keyboard state which will now indicate the keyboard is in unshifted state... so the Virtual Keyboard layout reverts to the unshifted state... and that means the shift-key chips are "up"/unhighlighted.
I think we need the reset as always present and ensure the macro to have steps for down and up of the modifier keys... I will have to circle back to that when I get things cleaned up on the virtual key side and then will focus on the macro details.
YES. I love this. So we need to assume a state device side at connection (possibly sending an eight-zeros keyboard report) at first connection, and any time the controlled-machine replies with an LED state, trust that as truth.
I think I'm in sync now 😄 but let me know if my clarifications are wrong...
I am NOT frustrated by the latency, just concerned that I am not helping, which is my primary motivation. I don't want to waste your time on changes that aren't in line with your plans. Just point me the right direction and I'll happily work at the pace my real job allows... if I anticipate a delay, I'll let you know. |
I'm going to leave this PR open until I've floated a new one implementing things discussed, we can then evaluate that one without all the distractions here (and close this one then). That okay? |
I'd be wary to add more noise to the main UI, but don't we already show this implicitly by showing the key presses?
Yep, totally agree! To clarify, I misspoke earlier. Where I wrote “fallback to JS-tracked state,” I meant “device-tracked state.” Otherwise we’re missing the entire point of this rewrite 😅
I’d vote to leave macros out of scope for this change unless you see a blocker. I’d prefer to preserve the current behavior and revisit improvements later, once the core input pipeline is stable.
Yes - agreed on all points here. The only unique bit is the sticky behavior from the Virtual Keyboard, where clicks simulate holding modifiers until toggled off. All good. Do you have any UX ideas on how to best support sticky combinations across different keyboard layouts and languages? That’s the one tricky bit I want to get right.
Mate, you’ve been an incredible help. Your engagement, whether it’s PRs, feature work, or comments on issues, has been consistently thoughtful and impactful. Genuinely appreciate the collaboration! And yes, sounds great to leave this PR open, until the new one is ready. |
Regarding the keyboard layouts, I am planning on using something (like a keyboard.info) to source layouts and tie them to the keyboard languages/formats and tweak or replace the on screen keyboard layouts to be driven by those in the selected locale... initially going to float en-US 102 key and at least Intl-102 for POC and pull in the others in short order. As for on-screen was thinking about making the keycaps indicate the characters available and have them shift appearance as the user stickies the modifiers. Ideally, would be cool to just support uploading a definition file from keyboard.info, but that might be a stretch, what do you think? |
Something like keyboard.info sounds great, but let's start small. My biggest concern is feature creep of the keyboard rewrite. Let's keep it mainly about changing the underlying mechanism, and with that in place we can easily make more significant changes to consumers of the new data flow. |
Work started with doing away with the LED State tracking option and adding the key press/release handling to the device (go) code. I'll get some more done this weekend. |
Awesome. Actually looking forward! <3 |
In reviewing these notes now that #725 is about to merge... I realize that in that code I don't know that I deal with the state where the host doesn't send LED status back to the device. We possibly will want to add that for the rare host that doesn't send LED messages... so when the device is connected to the USB port, it should initialize to something and then track the client/browser-sent keypresses to keep track of CapsLock/NumLock states which gets tossed if an LED state comes in from the host. It's really a minor edge case, because the only place I would ever expect not to get LED states from the host is in boot mode. |
Adds support for Shift, Ctrl, Alt, Meta, and AltGr keys to the virtual keyboard, treating them as "sticky" keys when entered on the Virtual Keyboard. This allows the user to click the Shift or Ctrl or Alt or Meta or AltGr button and then click another button to emit the combo (e.g. Ctrl-Alt-Del would be done using exactly that click sequence).
Fixes #396