fix(windows): caps lock stores to work in compliant applications#15771
fix(windows): caps lock stores to work in compliant applications#15771
Conversation
This change also adds a check in aiTIP.cpp to check the thread data stored last key press and scan code. Before if the scan code had been cleared then we would process the synthasized caps lock key presses which was just for the system and not for the current Keyman app.
also update external processsCaps Lock call to handle the Updateable change
Return early if caps lock state is unchanged.
The SCAN_FLAG_KEYMAN_KEY_EVENT was checked in OnKey pressess as well as an early return option in _KeymanProcessKeystroke. This change removes the extra and allows _KeymanProcessKeystroke, to be the central place for code readablitiy and maintainablity. It also adds a processToggleChange call in kmhook_getmessage as it is sometimes called before the TIP hook. the call is also idempotent
User Test ResultsTest specification and instructions 🟥 SUITE_CAPSLOCK:
Retesting TemplateTest Artifacts |
Added a Key transition member to the thread globals. This help so better insure we actually matching the key value that may have been cleared by kmhook_getmessage to the right key. More importantly makes sure we are not matching it to the wrong key press. This was happening when a synthazised capslock key press was followed by and actuall capslock key press.
|
Who improvements to matching the Keyman generated scancode The two we are focused on for issues with this PR are This problem is again observed with the There is no timestamp in the call back informaion in either Update LastScanCode in Keyman TIP
The solution is to make to set the |
|
Remove Duplication |
User TestingOn Windows 11 Non-compliant On Windows 10 Test Test in Word, Firefox for compliant applications Non-compliant SUITE_CAPSLOCK:
Caps LockThe test keyboard layouts are found in the keyman repo at The test cases below expect the usage of the Prerequisites before each test
Test casesclick to expand
CapsAlwaysOffFor these tests, use a keyboard with the Any keyboard with that store set will work; if you don't have one at hand you can use the Note: When testing in a virtual machine, use an on-screen keyboard (in VirtualBox: Input/Keyboard/Soft Keyboard) and observe the caps lock indicator of the on-screen keyboard. Using the hardware keyboard might show side effects with caps lock. Prerequisites before each test
Test casesclick to expand
SHIFT: CapsOnOnly/ShiftFreesCapsFor these tests, use a keyboard with the Any keyboard with these stores set will work; if you don't have one at hand you can use the The shift_frees_caps.kmp keyboard will enable caps lock by pressing the Note: When testing in a virtual machine, use an on-screen keyboard (in VirtualBox: Input/Keyboard/Soft Keyboard) and observe the caps lock indicator of the on-screen keyboard. Using the hardware keyboard might show side effects with caps lock. Except for TEST_CAPSONLY-5 which can only be reliably tested on a hardware keyboard on host OS (not a VM). For windows 10 and windows 11 with a virtual box vm-onscreen keyboard, the following happens. The VM soft keyboard does NOT actually send the Shift Shift Key Stroke through but rather will change the keys pressed for example if an Prerequisites before each test
Test casesclick to expand
Regression testcases
TEST_KM_NON_COMPLIANT:
|
Update the lastkey, lastscankey and lasttransition in aaitp. This ensures that when the pfEaten is FALSE and the get_message_hook does not recieve the key press the "last" value is correct.
Test Specs
Test ResultsSUITE_CAPSLOCK:GROUP_WIN10:
|
|
@sze2st Can you please test the Windows 11 cases on one of your machines? |
Test Specs
Test ResultsSUITE_CAPSLOCK:GROUP_WIN11:
Conclusion: keyboard
|
|
@sze2st why did you close this PR? |
mcdurdin
left a comment
There was a problem hiding this comment.
Given there are still test failures, I won't approve changes yet, as they'll need a revisit
Suggested title: fix(windows): remove assumptions about order that hooks and TIP events will be called or something like that? If I am understanding the changes correctly, a lot relates to that. But there's a bit more in there that perhaps we should discuss.
| * @param _td Theread data to update | ||
| * @param wParam WPARAM of the key event | ||
| * @param scan Scan code of the key event | ||
| * @param keyTransition Transition state of the key event (0 = key down, 1 = repeat, 3 = key up) |
There was a problem hiding this comment.
| * @param keyTransition Transition state of the key event (0 = key down, 1 = repeat, 3 = key up) | |
| * @param keyTransition Transition state of the key event (0 = key down, 1 = repeat, 2 = key up) |
Can we use an enumeration to clarify this?
| BOOL isUp = keyFlags & KF_UP ? TRUE : FALSE; | ||
| BOOL extended = keyFlags & KF_EXTENDED ? TRUE : FALSE; | ||
| BYTE scan = keyFlags & 0xFF; | ||
| BYTE keyTransition = (BYTE)((HIWORD(lParam) & (KF_UP | KF_REPEAT)) >> 14); |
There was a problem hiding this comment.
KEYMSG_FLAG_TRANSITION macro is defined, can we use that here?
There was a problem hiding this comment.
| BYTE keyTransition = (BYTE)((HIWORD(lParam) & (KF_UP | KF_REPEAT)) >> 14); | |
| BYTE keyTransition = KEYMSG_FLAG_TRANSITION(lParam); |
| _td->LastScanCode = scan; | ||
| _td->LastKey = mp->wParam; | ||
| _td->LastTransition = keyTransitionEvent; |
There was a problem hiding this comment.
You created a function UpdateLastKeyCache so should use that here too.
| _td->LastScanCode = scan; | |
| _td->LastKey = mp->wParam; | |
| _td->LastTransition = keyTransitionEvent; | |
| UpdateLastKeyCache(_td, mp->wParam, scan, keyTransitionEvent); |
| if (!Updateable || caps_state_change == KM_CORE_CAPS_UNCHANGED) | ||
| { |
There was a problem hiding this comment.
| if (!Updateable || caps_state_change == KM_CORE_CAPS_UNCHANGED) | |
| { | |
| if (!Updateable || caps_state_change == KM_CORE_CAPS_UNCHANGED) { |
nit only
Good catch. This PR was originally a child of this PR title fix(windows): caps lock stores to work in compliant applications #15730. |
|
I have created a seperate issue for addressing the capslock indicator turning off when selecting a Keyman keyboard. |
|
@sze2st I see how TEST_CAPSOFF-3 is failing. The fact that the indicator turns off is ok in this scenario of Caps always off and the output is as expected. |
Fixes: #15594
and
Fixes: #5507
To assist reviewer I have inserted this a check list of changes. More details of each change are through out this PR.
ScanCodeandTransitionCheck to aiTip.cpp. As well as updatingLastKeyandLastScanCodein aiTip.LastTransitionthread variableLastScanCode and Key
This change also adds a check in aiTIP.cpp to check the thread data stored last key press and scan code. Before it could process the key event the scan code had been cleared then we would process the synthasized capslock key as a user press.
Code maintenance
The SCAN_FLAG_KEYMAN_KEY_EVENT was checked in OnKey pressess as well as an early return option in _KeymanProcessKeystroke. This change removes the extra and allows _KeymanProcessKeystroke, to be the central place for code readablitiy and maintainablity.
It also adds a processToggleChange call in kmhook_getmessage as it is sometimes called before the TIP hook. the call is also idempotent
fTestEatenBuf
It doesn't seem correct the
OnTestKeyUpuses the buffer for pfEaten when it is only set by OnKeyDown and not OnTestKeyDown*pfEaten = fEatenBuf[wParam];Because if OnKeyDown never occured in the last processing of a particular key then it will be the wrong value, it will be from the previous press key.
This was observed with shift frees caps. Take the following sequence
Using the CapsOnOnly and ShiftFreesCaps keyboard test keyboard.
In a non-complaint app where there will be two passes the non-updateable also refered to as
OnTest, and if and only if the pfEaten bool parameter is retruned as TRUE will a second updateableOnKeyupdateable pass be called for the key stroke.Caps LockCaps LockShift
The caps lock does not turn off.
A second press of shift turns capslock off
Caps LockCaps LockShiftShift
Also
Caps LockShift
Here is the problem with the current method of recording pfeaten only in OnKeyDown.
If there is say key like the caps lock key where it is processed pfEaten = true then
it the key stroke will process throught the following 4 function calls.
OnTestKeyDown
OnKeyDown -> here is where the buffer
fEatenBufresult is stored.OnKeyUp
OnTestKeyUP
Then when you get a caps lock key where it is not processed pfEaten = False in the OnTestKeyDown
then OnKeyDown and OnKeyUp will not be called. The OnTestKeyUp then returns buffer value from the pervious call.
OnTestKeyDown
OnTestKeyUp <- uses the buffer which returns previous OnKeyDown which is not the current Key.
A second buffer has been added for
fTestEatenBufthis will store the result of the OnTestKeysOnTestKeyDown > here we store
fTestEatenBufOnKeyDown -> here is where the buffer
fEatenBufresult is stored.OnKeyUp
OnTestKeyUP
There is a question about whether we need to store and use this result on the key Up directions instead of the value returned from
_KeymanProcessKeystroke. It might be the case that now the key processing buisness logic has been moved to Keyman core that it is no longer necessary. I think though that is an investigation for another day.see more in comments below
Build-bot: release:windows