Ensure shifted keys are released

[gbrian]

When presssing a SHIFT + any_key combination and releasing the SHIFT key before any_key, the any_key code is not sent.
This change will ensure any_key code is relesed before the SHIFT to ensure target system receives the right keyup event

[mike-jumper]

Wouldn't this be incorrect behavior? If the following key events occur locally:

1. Press SHIFT
2. Press A
3. Release SHIFT
4. Release A

Then the events sent to the remote desktop should be exactly that. It would be incorrect to reorder the events to:

1. Press SHIFT
2. Press A
3. Release A
4. Release SHIFT

[m1k1o]

@mike-jumper the problem we are trying to solve are different key codes with and without SHIFT modifier.

Lets say we have 1 key that sends code x when no modifier is pressed and y when SHIFT is pressed. Under normal circumstances it would be like:

1. Press SHIFT
2. Press 1 -> sends keydown x.
3. Release 1 -> sends keyup x.
4. Release SHIFT

But when we do it in a different order, there is a race condition. There are two posibilities:

1. Press SHIFT
2. Press 1 -> sends keydown x.
3. Release SHIFT
4. Release 1 -> sends keyup x.

This is expected behavior, however when doing this relatively slowly we can end up with following state:

1. Press SHIFT
2. Press 1 -> sends keydown x.
3. Release SHIFT
4. Release 1 -> sends keydown + keyup y.

This means, that there is no sent keyup for x.

[mike-jumper]

What do you see when you press the key sequence in question on the key event tester:

https://guacamole.apache.org/pub/tests/guac/keyboard-test.html

[m1k1o]

First press is fast, second is the same but a little bit slower:

[mike-jumper]

And what keyboard layout are you using locally?

[m1k1o]

@mike-jumper US keyboard. Also tried it with Slovak keyboard (different key), same results:

[everflux]

Is there an associated JIRA issue to join the discussion?

I observed that reset() does not seem to work in all situations, usual case is a user using ctrl-tab or alt-tab, switching local browser tabs or windows, when refocusing the guacamole session the alt key is often stuck, even when reset() is called. We use a custom web client using guacamole-client, german keyboard layout de-de-qwertz.

[mike-jumper]

@m1k1o - The issue you're seeing appears due to key repeat behavior. I don't think we should solve that behavior by reordering key events, but instead investigate and correct whatever is causing the shifted version of the key to not be released as its unshifted counterpart repeats. It may be a browser quirk that the keyboard support needs to work around, or it may be a bug in the client-side tracking of key state.

@everflux - What you describe sounds like something different and may not be a bug in Guacamole. If you encounter the same behavior in the latest release of the standard, unmodified Guacamole stack (1.4.0), then yes please open a JIRA. If you only encounter the issue with your modified webapp, then the issue lies with your webapp and you should request assistance in troubleshooting your app on the mailing list.

[monken]

@everflux we also created our own web client and ran into similar issues. We connect to a Linux server using XRDP and needed support for shortcuts such as Ctrl + Alt + T which open a terminal on a Linux Desktop. The keyboard implementation from Guacamole doesn't forward the t key correctly. Here is a simplified version of our keyboard implementation which handles the shift key correctly (keyup events match the keydown event), repeat keys and supports the before mentioned key combinations: https://41t3yq.csb.app/ (code at https://codesandbox.io/s/41t3yq)

[mike-jumper]

@monken:

1. we also created our own web client and ...

   Please be sure to test against the mainline Apache Guacamole webapp before reporting an issue as a bug in Guacamole. See: https://guacamole.apache.org/faq/#test-against-latest-version

2. If what you describe is reproducible against mainline Apache Guacamole, the issue ("t" not being sent correctly if Ctrl+Alt are held) does not sound related to this at all (a key repeat issue specifically tied to Shift) except for the fact that the keyboard is involved. If you think you've found a bug in the keyboard handling, please open an issue in our JIRA. If you also think you have a solution, please open a PR.

3. I do not see the behavior you describe testing against https://guacamole.apache.org/pub/tests/guac/keyboard-test.html:

   

[monken]

@mike-jumper thanks for chiming in. I'm not seeing the same output. I'm testing this on Chrome on MacOS