m1k1o commented on code in PR #895:
URL: https://github.com/apache/guacamole-client/pull/895#discussion_r1244389062
##########
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:
> 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](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) principle
should be followed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
