mike-jumper commented on code in PR #895:
URL: https://github.com/apache/guacamole-client/pull/895#discussion_r1245585866
##########
guacamole-common-js/src/main/webapp/modules/Keyboard.js:
##########
@@ -1362,6 +1362,15 @@ Guacamole.Keyboard = function Keyboard(element) {
e.preventDefault();
+ // 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)
+ if (e.keyCode == 20 && quirks.capsLockKeyupUnreliable) {
+ eventLog.push(new KeydownEvent(e));
+ interpret_events();
+ return;
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
