Accept individual modifier keys as valid keybindings

[g547315]

References

#15090

• Trigger functionalities on key down and key up of Alt or Alt + Shift

Code changes

Allow the key down event listener to return a mod key or combination of mods to be processed and matched to its keybinding and the keyboard shortcut associated with it

Tests have been changed to reflect the new functionality of mod keys. As mod keys now have additional functionality, pressing a mod key will break the sequence when entering a key sequence i.e. [Shift K, Shift L]. The test 'should ignore modifier keys pressed in the middle of key sequence' has been changed to reflect this. A similar change has been made to 'should process key sequences that use different modifier keys' to remove unnecessary keystrokes.

'should return nothing for keys that are marked as modifier in keyboard layout' has been modified as mod keys now return keyboard events.

To see the full interned functionality please combine this PR with:

• Added and styled overlay UI element #633

User-facing changes

None

Backwards-incompatible changes

None

[g547315]

@krassowski
@tonyfast
@brichet

[brichet]

Thanks for working on this @g547315.

I have some comments below.

Would it be possible to wait for a delay before returning the modifier keys combination (when there are only modifier keys pressed) ?
That could avoid triggering the event linked to that modifier key, while only using the modifier in a shortcut.

[tonyfast]

should these modify combinations be configurable?

[g547315]

should these modify combinations be configurable?

could you elaborate please
@tonyfast

[tonyfast]

should these modify combinations be configurable?

modifier keys for assistive technology vary with locale, operating system, and vendor. there might be edge cases where this specific modifier combination is challenge for certain users. the most assistive solution would allow for configurable modifier key codes with Alt + Shift as the default.

[krassowski]

The failure is relevant:

Run for file in review/api/application.api.md review/api/commands.api.md; do
review/api/application.api.md
review/api/commands.api.md

Needs running yarn run api and committing changes

[g547315]

@brichet Thanks for the second pair of eyes and the contributions to the PR.
Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.

Mod keys bindings are being detected and matched, however they are not executed consistently or at all

• Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

[brichet]

@brichet Thanks for the second pair of eyes and the contributions to the PR. Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.

Mod keys bindings are being detected and matched, however they are not executed consistently or at all

• Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

This is nice for testing @g547315.

It works well on my side, when holding the key for 1s (it currently uses the CHORD_TIMEOUT variable).

Let's create a new variable with a shorter delay for this feature.

[g547315]

@brichet Thanks for the second pair of eyes and the contributions to the PR. Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.
Mod keys bindings are being detected and matched, however they are not executed consistently or at all

• Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

This is nice for testing @g547315.

It works well on my side, when holding the key for 1s (it currently uses the CHORD_TIMEOUT variable).

Let's create a new variable with a shorter delay for this feature.

New variable added and set to 500ms

[g547315]

This pull request should to be ready for merging. However, I am unable to continue working on it after March 28, 2024. If further refinements are identified, I welcome any future contributors to pick up and progress the work

[brichet]

Thanks again @g547315 for working on this.

Since I worked on it, it would be nice to have another opinion before merging.
@krassowski do you think we can merge this PR ?

[krassowski]

Thanks for the ping @brichet!

Should the modifier timer also be stopped if an exact match for a chord is found?

For example if the sequence is Ctrl Z (not Ctrl + Z but Ctrl followed by Z) we would get two events (one for Ctrl, one for Z). My impression from just reading the code (I could be wrong) is that currently it would trigger both the handler for Ctrl and for the Ctrl followed by Z chord.

In other words, should _clearModifierTimer be called in _clearPendingState too?

[brichet]

You are right, but this behavior seems OK if this feature is only used to display overlays.
You may want to keep the modifier key(s) pressed (and the overlays visible) to trigger another shortcut.

If we keep it that way, we should probably document it somewhere, to avoid using this modifier key(s) for unexpected action.

[krassowski]

You may want to keep the modifier key(s) pressed (and the overlays visible) to trigger another shortcut.

Yes, but I also do not want to have the overlay visible ("sticky") after I already pressed the combo I was interested in. I think if user is pressing two shortcuts and they need overlays, say Ctrl + C and Ctrl + V it is fine it they do that in sequence:

• Ctrl: shows overlay
• Ctrl + C: overlay hides immediately
• Ctrl: shows overlay again
• Ctrl + V: overlay hides immediately again

[brichet]

Actually, the overlay visible or not depends on the command that will be triggered.
Here it is only about the timeout before triggering the command, so you're right we better clear it if an other command is triggered. BTW the timeout is also cleared on key up event with this PR.
I updated it.

[krassowski]

Thank you!