Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent handling of keyboard shortcuts with caps lock on #1565

Closed
nicolas-p opened this issue Mar 16, 2016 · 24 comments
Closed

Inconsistent handling of keyboard shortcuts with caps lock on #1565

nicolas-p opened this issue Mar 16, 2016 · 24 comments

Comments

@nicolas-p
Copy link
Contributor

I have defined a keyboard shortcut like this:

environment-gadget "general" f {
    { T{ key-up f f "w" } add-word }
} define-command-map

On a Mac, it doesn't matter if caps lock is tuned on or off, pressing W always triggers the action. On Windows, it only works if caps lock is turned off.

I don't know what is the expected behaviour (in this particular case, I prefer the Mac behaviour), but this inconsistency across platforms is strange.

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 16, 2016

So make Ctrl-w and Ctrl-W or w and W do the same thing?

@nicolas-p
Copy link
Contributor Author

Yes, obviously I can just write:

environment-gadget "general" f {
    { T{ key-up f f "w" } add-word }
    { T{ key-up f f "W" } add-word }
} define-command-map

But maybe it would be worth looking at how Factor handles keyboard gestures to see it there isn't a bug somewhere.

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 19, 2016

@nicolas-p wait, I didn't read the bug report properly -- did you say in Factor on the mac that W and w both trigger the action?

@nicolas-p
Copy link
Contributor Author

Yes, this is what happens. I guess you can easily check it yourself.

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 19, 2016

Okay, so I don't use caps lock much (I rebind it to control), but I did some testing and found that apparently when the caps lock key is on the charactersIgnoringModifiers does not uppercase the key (unlike when the shift key is pressed).

This "fixes" it, but I don't want to apply this change without understanding when and when not to uppercase the characters:

diff --git a/basis/ui/backend/cocoa/views/views.factor b/basis/ui/backend/cocoa/views/views.factor
index 25b4359..6bea6ca 100644
--- a/basis/ui/backend/cocoa/views/views.factor
+++ b/basis/ui/backend/cocoa/views/views.factor
@@ -68,8 +68,10 @@ CONSTANT: key-codes
     }

 : key-code ( event -- string ? )
-    dup -> keyCode key-codes at
-    [ t ] [ -> charactersIgnoringModifiers CF>string f ] ?if ;
+    dup -> keyCode key-codes at [ t ] [
+        [ -> charactersIgnoringModifiers CF>string ]
+        [ -> modifierFlags NSAlphaShiftKeyMask bitand 0 > [ >upper ] when ] bi f
+    ] ?if ;

 : event-modifiers ( event -- modifiers )
     -> modifierFlags modifiers modifier ;

@nicolas-p
Copy link
Contributor Author

On my side I had to do this.

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 19, 2016

It works with Shift, just not Caps lock.

@alex-ilin
Copy link
Member

On Windows the CapsLock state should not make a difference. It should be the same as on Mac.

@bjourne
Copy link
Member

bjourne commented Jun 20, 2016

I don't think w and W should be the same. In vim they aren't and maybe you are writing a vim clone in Factor. :) And ctrl+s needs to be different from ctrl+shift+s too because the first is "Save" and the other "Save As.." in almost all programs.

@catb0t
Copy link
Contributor

catb0t commented Jun 21, 2016

But ctrl-shift-s should not be the same as ctrl-s when caps is pressed.

@catb0t
Copy link
Contributor

catb0t commented Feb 19, 2018

By the way, at least on Linux (and probably on Windows too) in the Listener UI, keyboard shortcuts don't work when capslock is on, like ctrl+a+caps doesn't "select all", for the same reason as noted upthread.

It's a little bug that should probably be fixed before like, 0.99 but I only notice it because I press caps accidentally sometimes.

@mrjbq7
Copy link
Member

mrjbq7 commented Feb 19, 2018

For what it's worth, you can get a good idea of what events are being generated using gesture-logger:

IN: scratchpad USE: gesture-logger

IN: scratchpad run-gesture-logger

I see, for example, a select-all-action with just Ctrl-A:

T{ key-down { mods { C+ } } { sym "a" } }
select-all-action
T{ key-up { mods { C+ } } { sym "a" } }

@catb0t
Copy link
Contributor

catb0t commented Feb 19, 2018

For me it's like T{ key-up { mods { C+ } } { sym "A" } } when capslock is on so it's the very same thing as above. 🤷‍♀️

@alex-ilin
Copy link
Member

Wow, gesture-logger is a very cool tool, I didn't know it existed!
It seems that currently we don't consider Shift as a modifier. Instead of having

T{ key-up { mods { C+ } } { sym "A" } }

we should really have

T{ key-up { mods { C+ S+ } } { sym "a" } }

And the character should always be lower-case independent of the CapsLock state. This would make the behavior consistent with all the (normal) Windows apps out there.
Compare the FireFox behavior with Ctrl+H versus Ctrl+Shift+H. The two hotkeys have distinct functions, both independent of the CapsLock state.

@alex-ilin
Copy link
Member

I've pushed a patch here: https://github.com/AlexIljin/factor/tree/fix-1565
This is some preliminary work, not thoroughly tested. The code I modified is ancient, committed by Slava in 2008 and not documented. The commit message didn't state what kind of problems he was trying to solve at the time. I didn't have time to fully understand the intent of the code I changed and the context of what was being done there, so I'm not submitting it as a pull request yet. But it makes what I'd expect to happen.

Pressing "a" without Caps Lock:

T{ key-down { sym "a" } }
T{ key-down { sym "a" } }
User input: a
T{ key-up { sym "a" } }

Pressing "S+a" without Caps Lock:

T{ key-down { mods { S+ } } { sym "a" } }
T{ key-down { sym "A" } }
User input: A
T{ key-up { mods { S+ } } { sym "a" } }

Pressing "a" with Caps Lock:

T{ key-down { sym "a" } }
T{ key-down { sym "A" } }
User input: A
T{ key-up { sym "a" } }

Pressing "S+a" with Caps Lock:

T{ key-down { mods { S+ } } { sym "a" } }
T{ key-down { sym "a" } }
User input: a
T{ key-up { mods { S+ } } { sym "a" } }

The above behavior is exactly what I would expect, as it would make sure that C+S+a would be distinct from C+a, and both would work consistently and without interference from the Caps Lock state. This is confirmed by my tests: in the listener the C+a, C+c, C+v, C+p, etc. now work regardless of the Caps Lock state, but aren't triggered if Shift is depressed.

The only remaining issue is the duplicated key-down notification. I'm not sure if any part of the system will break if I remove it. Do we need the { sym "A" } to be reported when Caps Lock is on, or is { sym "a" } sufficient for all intents and purposes?

Please, have a look and tell me what you think.

@kusumotonorio
Copy link
Contributor

I think my code which was showed on the Factor mailing list solves the duplicated key-down notification issue.

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 31, 2019

Seems like we need to make sure all platforms are doing the same thing.

So, either keep all keys as lowercase, with their modifiers.

Or uppercase, with or without modifiers.

Or, some other thing.

Patches and investigation welcome!

@mrjbq7
Copy link
Member

mrjbq7 commented Mar 31, 2019

Previous comment code from the mailing list, for tracking.

CONSTANT: wm-keydown-codes
    H{
      { 48 "0" } ! <- added
      { 49 "1" } ! <- added
      { 50 "2" } ! <- added

      { 65 "a" } ! <- added
      { 66 "b" } ! <- added
      { 67 "c" } ! <- added


        { 8 "BACKSPACE" }
        { 9 "TAB" }
        { 13 "RET" }
        { 27 "ESC" }
        { 33 "PAGE_UP" }
        { 34 "PAGE_DOWN" }
        { 35 "END" }
        { 36 "HOME" }
        { 37 "LEFT" }
        { 38 "UP" }
        { 39 "RIGHT" }
        { 40 "DOWN" }
        { 45 "INSERT" }
        { 46 "DELETE" }
        { 112 "F1" }
        { 113 "F2" }
        { 114 "F3" }
        { 115 "F4" }
        { 116 "F5" }
        { 117 "F6" }
        { 118 "F7" }
        { 119 "F8" }
        { 120 "F9" }
        { 121 "F10" }
        { 122 "F11" }
        { 123 "F12" }
        { 190 "PERIOD" } ! <- added
    }


:: key-sym ( wParam -- string/f action? )    ! : -> ::
    ! {
    !     {
    !         [ dup LETTER? ]
    !         [ shift? caps-lock? xor [ CHAR: a + CHAR: A - ] unless 1string f ]
    !     }
    !     { [ dup digit? ] [ 1string f ] }
    !     [ wm-keydown-codes at t ]
    ! } cond ;

    wParam wm-keydown-codes at [ t ] [
       wParam
       {
           {
               [ dup LETTER? ]
               [ shift? caps-lock? xor [ CHAR: a + CHAR: A - ] unless 1string f ]
             }
             { [ dup digit? ] [ 1string f ] }
             [ wm-keydown-codes at t ]
             } cond
     ] if* ;

:: handle-wm-keydown ( hWnd uMsg wParam lParam -- )
    wParam exclude-key-wm-keydown? [
        wParam key-sym over [
            dup ctrl? alt? xor or [                          ! neccesary?
                hWnd send-key-down
            ] [ 2drop ] if
        ] [ 2drop ] if
    ] unless ;

:: handle-wm-char ( hWnd uMsg wParam lParam -- )
    wParam exclude-key-wm-char? [
        ctrl? alt? xor [
            wParam 1string
!            [ f hWnd send-key-down ]
            [ drop ]
            [ hWnd window user-input ] bi
        ] unless
    ] unless ;

@alex-ilin
Copy link
Member

I've created the PR #2126, which incorporates the key-down event deduplication. I've also updated all the key-down events like { T{ key-down f { A+ } "H" } com-home } (using the capital letter to designate Shift/Caps Lock) to { T{ key-down f { S+ A+ } "h" } com-home }, so that they require the Shift modifier, since after the patch they are no longer affected by the Caps Lock state.

@alex-ilin
Copy link
Member

Going back to the original posting, this fix makes the behavior consistent between Mac and Windows regarding the Caps Lock state.

@kusumotonorio
Copy link
Contributor

Hmm... I wrote wm-keydown-codes which had most of keys. And I understood my idea was too simple. The problem is there are various international keyboards. For example, to input "+” with US keyboard is shift & "=". But, with Japanese Keyboard, it is shift & "-". If they want to zoom a document, should I wrote the code as { T{ key-down f { S+ } "=" } com-zoom } ? Or as { T{ key-down f { S+ } "-" } com-zoom } ?

@kusumotonorio
Copy link
Contributor

I wrote the code for "Plan 2". It is more conservative, put emphasis on compatibility.
It ignores caps-lock at key-down and key-up events. And number keys on the numeric pad, ",", ".", etc provide key-up events.

https://github.com/kusumotonorio/factor/blob/windows-kn-plan2/basis/ui/backend/windows/windows.factor

master...kusumotonorio:windows-kn-plan2

@kusumotonorio
Copy link
Contributor

kusumotonorio commented Apr 4, 2019

I found a bug in it. I'll fix it tonight. <-- I did it.

@erg
Copy link
Member

erg commented May 2, 2019

Hopefully resolved in 08aa27a, please reopen if not.

@erg erg closed this as completed May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants