You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by flangelo <gi...@git.apache.org> on 2017/11/09 20:32:24 UTC

[GitHub] incubator-guacamole-client pull request #209: GUACAMOLE-161: Add missing pre...

GitHub user flangelo opened a pull request:

    https://github.com/apache/incubator-guacamole-client/pull/209

    GUACAMOLE-161: Add missing press and release CapsLock events on Mac O…

    …S. Add CapsLock to the no_repeat key list.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/flangelo/incubator-guacamole-client fix_mac_os_capslock

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-guacamole-client/pull/209.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #209
    
----
commit ae479895981179771e0ccee2e0efade1e17f1372
Author: Frode Langelo <fr...@apache.org>
Date:   2017-11-09T20:25:39Z

    GUACAMOLE-161: Add missing press and release CapsLock events on Mac OS. Add CapsLock to the no_repeat key list.

----


---

[GitHub] incubator-guacamole-client pull request #209: GUACAMOLE-161: Add missing pre...

Posted by flangelo <gi...@git.apache.org>.
Github user flangelo commented on a diff in the pull request:

    https://github.com/apache/incubator-guacamole-client/pull/209#discussion_r150694859
  
    --- Diff: guacamole-common-js/src/main/webapp/modules/Keyboard.js ---
    @@ -984,8 +1003,15 @@ Guacamole.Keyboard = function(element) {
                 // Release specific key if known
                 var keysym = first.keysym;
                 if (keysym) {
    +
    +                // Press CapsLock first if Keyup event is alone.
    +                // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Special_cases
    +                if (keysym === 0xFFE5)
    +                    guac_keyboard.press(keysym);
    --- End diff --
    
    You are correct; looking for the Keydown and Keyup events have no effect:
    ```
    3.key,5.65509,1.1;	18	15:08:08.502
    3.key,5.65509,1.0;	18	15:08:08.503
    3.key,5.65509,1.1;	18	15:08:08.513
    3.key,5.65509,1.0;	18	15:08:08.513
    ```
    This leaves relying only on assuming Mac OS is quirky; which may be true until the behavior is 
    fixed in the browser(s) and we'd have to handle that later.. Are we OK with that though.


---

[GitHub] incubator-guacamole-client pull request #209: GUACAMOLE-161: Add missing pre...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/incubator-guacamole-client/pull/209#discussion_r150925860
  
    --- Diff: guacamole-common-js/src/main/webapp/modules/Keyboard.js ---
    @@ -984,8 +1003,15 @@ Guacamole.Keyboard = function(element) {
                 // Release specific key if known
                 var keysym = first.keysym;
                 if (keysym) {
    +
    +                // Press CapsLock first if Keyup event is alone.
    +                // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Special_cases
    +                if (keysym === 0xFFE5)
    +                    guac_keyboard.press(keysym);
    --- End diff --
    
    Generally, yes, that seems the only safe option - always releasing after handling keydown for Caps Lock, and then leaving the keyup behavior unmodified. There's no need to inspect the keyboard object's event queue to determine whether a keyup event is already present, since if so that event will be handled immediately after the keydown handling is complete, and the keyup event will simply be ignored if the corresponding key has already been marked as released.
    
    Alternatively, if you want to browser sniff the quirk, I think that would be fine as well given the docs you've cited, but we should look to avoid repeating the sniff and to make the representation of the quirk(s) more semantic. From the iterations so far, it looks like this may even be as simple as just adding a call to `release()` if Caps Lock is known to be unreliable based on the platform test.


---

[GitHub] incubator-guacamole-client pull request #209: GUACAMOLE-161: Add missing pre...

Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/incubator-guacamole-client/pull/209#discussion_r150676272
  
    --- Diff: guacamole-common-js/src/main/webapp/modules/Keyboard.js ---
    @@ -984,8 +1003,15 @@ Guacamole.Keyboard = function(element) {
                 // Release specific key if known
                 var keysym = first.keysym;
                 if (keysym) {
    +
    +                // Press CapsLock first if Keyup event is alone.
    +                // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Special_cases
    +                if (keysym === 0xFFE5)
    +                    guac_keyboard.press(keysym);
    --- End diff --
    
    Given the following events, happening in order:
    
    1. `keydown` Caps Lock
    2. `keyup` Caps Lock
    
    due to a single key press, would this then result in the following series of Guacamole keyboard events:
    
    1. `keydown` Caps Lock (due to original `keydown`)
    2. `keyup` Caps Lock (due to unreliable `keydown`)
    3. `keydown` Caps Lock (due to unreliable `keyup`)
    4. `keyup` Caps Lock (due to original `keyup`)
    
    effectively doubling things on the remote end?


---