GUACAMOLE-1823: Fix capslock on macos chrome

[m1k1o]

Chrome on MacOS sends only a single keydown event when turning on caps-lock, and only a single keyup event when turning off caps-lock.

Firefox on MacOS sends always only a single keydown that is properly handled in quirks as capsLockKeyupUnreliable.

Steps to reproduce:

In my tests I always clicked CapsLock twice, while initial state was not activated. So I activated and deactivated caps lock in Firefox and Chrome on MacOS while watching how guacamole keybaord behaves and also what events are actually fired:

Firefox MacOS:

Chrome MacOS:

Solution

When quirk capsLockKeyupUnreliable is active, we are never handling any keyup event because we don't expect it to happen. I extended the code to handle a case, when we only get a single keyup event, and we convert it to keydown event. This evenutally changes meaning of capsLockKeyupUnreliable to capsLockKeyUnreliable (because its either up or down).

[mike-jumper]

Please also see our contribution guidelines:

https://github.com/apache/guacamole-client/blob/master/CONTRIBUTING

In particular:

1. All changes need a corresponding issue in JIRA. If this does not already have an issue, you'll need to create one. If you don't have an account in JIRA, you'll have to click the link to request one.
2. All commits need to be tagged with the JIRA issue (see established git history for how this is formatted).

[mike-jumper]

If unreliable caps lock was pressed and event was not marked, then we need to pretend that this is a keydown event because we obviously did not receive it (issue on macos with chrome)

This doesn't look correct:

1. It's unclear what is meant by "event was not marked", but this doesn't appear to align with the logic in the if that follows.

2. From the description in the PR:

   Chrome on MacOS sends only a single keydown event when turning on caps-lock, and only a single keyup event when turning off caps-lock.

   That would mean that we should send our own keydown for Caps Lock if we receive a keyup without a corresponding keydown.

3. The intent of the structure of eventLog and these various event types is to prevent having too much logic directly within the JS event handlers. Such logic should instead be in the portion of the keyboard code that retrospectively inspects the contents of eventLog to produce its final interpretation.

As the keyboard already tracks modifier states, attempting to synchronize them whenever they change externally, I think a better approach would be to:

1. Track and synchronize lock states in the same way.
2. Synchronize lock states when inspecting the lone KeyUpEvent event in the eventLog.

[m1k1o]

 It's unclear what is meant by "event was not marked",

Just 4 lines before that if, there is if (!markEvent(e)) return; that ignores events which have already been handled. I also find that naming unclear, but I followed the convetion of "marked" events.

That would mean that we should send our own keydown for Caps Lock if we receive a keyup without a corresponding keydown.

Exactly, that is what I wanted to achieve, sice we don't get any keydown but it need to be emulated for the receiveing end.

The intent of the structure of eventLog and these various event types is to prevent having too much logic directly within the JS event handlers. Such logic should instead be in the portion of the keyboard code that retrospectively inspects the contents of eventLog to produce its final interpretation.

I agree with this statement, but since we never get the corresponsing keydown, we would need to emit keydown from keyup event. And since there is already code that handles unreliable keyup events, I found it cleaner to just call this existing code from native event handler.

Synchronize lock states when inspecting the lone KeyUpEvent event in the eventLog.

I am not sure if there is any other need for different lock states, since only CapsLock seems to be problematic, hence the quirk capsLockKeyupUnreliable. I'd argue that such added complexity should be justified first, otherwise YAGNI principle should be followed.

[mike-jumper]

OK, the issues I see with the current approach here are:

• Adding events that did not occur directly to eventLog, rather than through retrospective inspection of the eventLog, goes against the core design of Guacamole.Keyboard. This correction should be made in a way that works with that core design, unless there is a solid technical reason that said design needs to be revisited.

  The contents of eventLog have a 1:1 correspondence with events received from JS, with Guacamole's further interpretation of those events coming later through inspection of eventLog. Interpretation beyond that 1:1 correspondence should not happen directly within the low-level event handler.

• The solution (inject a keydown event for every keyup received if capsLockKeyupUnreliable) is potentially brittle / does not align with the semantics of the system:

  • capsLockkeyupUnreliable is specific to keyup behavior, not the lack of a keydown (nor Mac+Chrome's use of keydown to signify Caps Lock is active and keyup to signify Caps Lock is inactive).
  • The adding of a keydown event for each Caps Lock keyup does not actually test whether such an event has already been processed. The assumption is made that a keydown is always missing. It may be that doubling up on keydowns will not ultimately result in duplicate events reaching guacd due to other checks within Guacamole.Keyboard, but a solution to an issue should ideally not itself add another issue / technical debt (in this case, event noise that Guacamole.Keyboard must process around).

I suggested tracking locks similarly to modifiers because it seems an elegant solution to the problem at hand:

• It fits the established design of Guacamole.Keyboard.
• It inherently ensures Caps Lock events are dispatched correctly.
• It has the happy side effect of additionally ensuring Caps Lock is automatically synchronized even if its state changes externally.

If you have a simpler alternative to the above, I would not be against it so long as:

1. It is in line with the intent of eventLog.
2. It is in line with semantics (either through embracing those semantics or changing them).
3. It solves the problem (incorrect behavior of Caps Lock on Mac OS) without introducing a different problem (duplicate keydown events) that happens to already be dealt with internally.

[m1k1o]

I am not fully familiar with core design of Guacamole.Keyboard since this is my first contibution, but thanks for explaining to me and I can move my code inside eventLog to match the design principle. I'll submit my changes throughout the next days.

capsLockkeyupUnreliable is specific to keyup behavior, not the lack of a keydown

It could be renamed to capsLockUnreliable to match the semantics.

The adding of a keydown event for each Caps Lock keyup does not actually test whether such an event has already been processed. The assumption is made that a keydown is always missing.

That assumption is coming directly from capsLockkeyupUnreliable, what processes custom keyup event if there is keydown event. Even if there would be keyup event with said quirk enabled, it would not pass through if (!markEvent(e)) return; condition. But I agree, that this logic might be implicit and not transparent.

tracking locks similarly to modifiers

I might look into that, but I would prefer to keep this refactoring separated to a different PR and focus now on fixing the bug according to the contribution guideline Avoid commits which cover multiple, distinct goals that could (and should) be handled separately. .

[mike-jumper]

I might look into that, but I would prefer to keep this refactoring separated to a different PR and focus now on fixing the bug according to the contribution guideline ...

Sounds good.

To clarify: there is no requirement that all bugfixes first enter the codebase as minimal patches/hacks before a final, cleaner solution is later contributed - that'd be technical debt. The "avoid commits which cover multiple, distinct goals" refers to preferring to split changes across separate commits where the separation makes sense, rather than contributing everything as a single, monolithic commit. Large PRs consisting of a single, monolithic commit can cause trouble down the line when tracing things back with git blame. I don't anticipate this particular PR being that huge, but all that requirement means is that the PR itself should contain multiple commits where doing so makes logical sense.

[mike-jumper]

OK, the issues I see with the current approach here are:

• Adding events that did not occur directly to eventLog, rather than through retrospective inspection of the eventLog, goes against the core design of Guacamole.Keyboard. This correction should be made in a way that works with that core design, unless there is a solid technical reason that said design needs to be revisited.

  The contents of eventLog have a 1:1 correspondence with events received from JS, with Guacamole's further interpretation of those events coming later through inspection of eventLog. Interpretation beyond that 1:1 correspondence should not happen directly within the low-level event handler.

• The solution (inject a keydown event for every keyup received if capsLockKeyupUnreliable) is potentially brittle / does not align with the semantics of the system:

  • capsLockkeyupUnreliable is specific to keyup behavior, not the lack of a keydown (nor Mac+Chrome's use of keydown to signify Caps Lock is active and keyup to signify Caps Lock is inactive).
  • The adding of a keydown event for each Caps Lock keyup does not actually test whether such an event has already been processed. The assumption is made that a keydown is always missing. It may be that doubling up on keydowns will not ultimately result in duplicate events reaching guacd due to other checks within Guacamole.Keyboard, but a solution to an issue should ideally not itself add another issue / technical debt (in this case, event noise that Guacamole.Keyboard must process around).

I suggested tracking locks similarly to modifiers because it seems an elegant solution to the problem at hand:

• It fits the established design of Guacamole.Keyboard.
• It inherently ensures Caps Lock events are dispatched correctly.
• It has the happy side effect of additionally ensuring Caps Lock is automatically synchronized even if its state changes externally.

If you have a simpler alternative to the above, I would not be against it so long as:

1. It is in line with the intent of eventLog.
2. It is in line with semantics (either through embracing those semantics or changing them).
3. It solves the problem (incorrect behavior of Caps Lock on Mac OS) without introducing a different problem (duplicate keydown events) that happens to already be dealt with internally.

[mike-jumper]

It's probably also worth re-targeting this against the staging/1.5.3 branch so that it can be included in the 1.5.3 bugfix release.

[m1k1o]

Re-targetted against staging/1.5.3 branch and moved code to KeyupEvent function.

Renamed to capsLockKeyEventUnreliable, since now it applies to keyup and keydown events.

[mike-jumper]

Did not mean to auto-close this. I'll create a staging/1.5.4 branch shortly after closing out the 1.5.3 release process.

[necouchman]

@m1k1o This is now targeted at the 1.5.5 release, if you want to try to get it into the next bugfix release.