Accept individual modifier keys as valid keybindings

[g547315]

References

#15090

• [X] 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:

• [X] Added and styled overlay UI element #633

  User-facing changes

  None

  Backwards-incompatible changes

None

[g547315]

@krassowski @tonyfast @brichet

[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.

[g547315]

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.

The combinations are configurable by the user through settings

[g547315]

@brichet please re-review with the added changes and updated PR description

[g547315]

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.

This is now done @brichet

[brichet]

I'll take a look at it in the week. At first glance, I think we should add a delay when there are only mod key. Otherwise, the related action will be triggered every time we want to do a shortcut using mod keys.

EDIT: Sorry, I missed your previous comment, but I couldn't find anything that would allow a delay...

[g547315]

Thanks @g547315 I don't really understand where a delay has been added for modifier keys only...

Is Jupyterlab working with this modification included ? On my side I'm not able to write in a cell for example.

Cell issue should now be fixed.

In terms of how the mod keys are delayed:

Here we don't want to fire the exact matches that are triggered by only modifier key(s)

    // If there is an exact match but no partial match, the exact match
    // can be dispatched immediately. The pending state is cleared so
    // the next key press starts from the default state.
    if (
      exact &&
      !partial &&
      !(
        exact.keys.toString() === 'Alt' ||
        exact.keys.toString() === 'Ctrl' ||
        exact.keys.toString() === 'Shift' ||
        exact.keys.toString() === 'Alt Shift'
      )
    ) {
      this._executeKeyBinding(exact);
      this._clearPendingState();
      return;
    }

The exact match is then stored for future dispatch (delayed)

// If there is both an exact match and a partial match, the exact
    // match is stored for future dispatch in case the timer expires
    // before a more specific match is triggered.
    // which is the case fro 'Alt' and 'Alt Shift'.
    if (exact) {
      this._exactKeyMatch = exact;
    }

    // Store the event for possible playback in the future.
    this._keydownEvents.push(event);

A timer that counts down for 1000 milliseconds (1 second) is started. This timer can be stopped by other exact or partial matches within the 1000 milliseconds.

    // (Re)start the timer to dispatch the most recent exact match
    // in case the partial match fails to result in an exact match.
    this._startTimer();
  }

  /**
   * Start or restart the pending timeout.
   */
  private _startTimer(): void {
    this._clearTimer();
    this._timerID = window.setTimeout(() => {
      this._onPendingTimeout();
    }, Private.CHORD_TIMEOUT);
  }

[brichet]

In terms of how the mod keys are delayed:

thanks for the explanation

[brichet]

In terms of how the mod keys are delayed:

Thinking again about it, I don't think the delay should use the partial/exact mechanism. I think that we can assume that the "mod keys only" will never be used for regular shortcuts, but only to display helpers. In my mind, the related command should be triggered if the key(s) is(are) held for some delay only. The way it is implemented now, if you press and release the 'Alt' key, the overlay will be displayed after one second, which would be a strange behavior in my opinion.

The timer should probably be dedicated to this feature, and deleted on key up, if the key(s) is(are) not held. This way, the "mod keys only" would not be added to _keydownEvents, and it would fix the sequences with combinations (see https://github.com/jupyterlab/lumino/pull/637#discussion_r1392881819).

[g547315]

In terms of how the mod keys are delayed:

Thinking again about it, I don't think the delay should use the partial/exact mechanism. I think that we can assume that the "mod keys only" will never be used for regular shortcuts, but only to display helpers. In my mind, the related command should be triggered if the key(s) is(are) held for some delay only. The way it is implemented now, if you press and release the 'Alt' key, the overlay will be displayed after one second, which would be a strange behavior in my opinion.

The timer should probably be dedicated to this feature, and deleted on key up, if the key(s) is(are) not held. This way, the "mod keys only" would not be added to _keydownEvents, and it would fix the sequences with combinations (see #637 (comment)).

Thanks for the review the suggested changes have now been implemented can you please re-review

[g547315]

Context for this PR came about with a suggestion made here, it suggests mirroring Slack's functionality using a combination of modifier keys and numbers E.g:

• 'alt' would display the overlays for the left sidebar

• 'alt' + '1' would activate the first element on the left sidebar

However, without a clear understanding of the underlying processing and management of chords it is proving difficult to replicate

Upon implementing the proposed changes in two different ways (modifier keys being delayed and held down), I discovered that the test, 'should ignore modifier keys pressed in the middle of key sequence,' is still failing. This test relies on the application returning an empty string or doing nothing when a modifier key is encountered by its self.

To resolve this issue, we can modify the test or overhaul the chord processing logic. Without these modifications, it seems impossible to maintain the current chord implementation, pass the test, and execute bindings that are only modifier keys.

A final possibility is using a combination of modifier keys, characters and numbers E.g:

• 'alt' + '.' would display the overlays for the left sidebar

• 'alt' + '.' + '1' would activate the first element on the left sidebar

This allows for the current implementation of returning an empty string or doing nothing when a modifier key is encountered by its self to persist and resolve the failing test 

Open to any other suggestions that would progress this issue @brichet @krassowski

[brichet]

@g547315 thanks again for working on this, adding this feature without breaking anything is not easy.

I opened https://github.com/g547315/lumino/pull/1 on your branch. It is based on what you've done with the timer, I mostly moved some calls to avoid breaking sequences.

[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 ?