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

New keybinding attribute to indicate shortcuts which prevent default actions #688

Closed
krassowski opened this issue Mar 5, 2024 · 2 comments · Fixed by #689
Closed

New keybinding attribute to indicate shortcuts which prevent default actions #688

krassowski opened this issue Mar 5, 2024 · 2 comments · Fixed by #689
Labels

Comments

@krassowski
Copy link
Member

Problem

  • Currently lumino shortcuts can prevent user from typing characters on certain keyboards (because it always calls preventDefault() and stopPropagation() on matched keyboard events)
  • JupyterLab 4.1.3+ solves it by delaying command execution until it knows if a character would have been typed (Prevent command shortcuts from preventing user input jupyterlab#15790)
  • However, some shortcuts need to be executed immediately to prevent default browser action
  • Currently there is no way to distinguish the situation in which the current keyboard event would invoke a shortcut which needs such immediately handling

Proposed Solution

  1. Add new attribute preventDefault: bool = true to IKeyBindingOptions

  2. Add new public method on commands to check if the default action on the event should be prevented, say:

        willPreventDefault(event) {
            // Bail immediately if playing back keystrokes.
            if (this._replaying || CommandRegistry.isModifierKeyPressed(event)) {
                return;
            }
            // Get the normalized keystroke for the event.
            let keystroke = CommandRegistry.keystrokeForKeydownEvent(event);
            // If the keystroke is not valid for the keyboard layout, replay
            // any suppressed events and clear the pending state.
            if (!keystroke) {
                this._replayKeydownEvents();
                this._clearPendingState();
                return;
            }
            // Add the keystroke to the current key sequence.
            const keystrokes = [*this._keystrokes, keystroke];
            // Find the exact and partial matches for the key sequence.
            let { exact } = Private.matchKeyBinding(this._keyBindings, keystrokes, event);
    
            if (exact.preventDefault || partial.preventDefault) {
              return true;
            }
            return false;
        }
  3. Change processKeydownEvent to only preventDefault when needed, this is change:

    // Stop propagation of the event. If there is only a partial match,
    // the event will be replayed if a final exact match never occurs.
    event.preventDefault();
    event.stopPropagation();

    to:

    if (exact.preventDefault || partial.preventDefault) {
       event.preventDefault(); 
       event.stopPropagation(); 
    }
  4. In JupyterLab:
    a) mark all keybindings as preventDefault=false by default, and flick the few ones which should prevent default manually to true
    b) rework the evtKeydown handler to call willPreventDefault(event) and depending on the result decide whether to process the event immediately or not

  5. In a future major lumino version, consider changing preventDefault default to false, and adopting the refined evtKeydown implementation from JupyterLab

Additional context

@krassowski
Copy link
Member Author

Here are some more thoughts:

  • willPreventDefault is a bit single-use case method which needs to be called in a very specific way (after previous processKeydownEvent but before next processKeydownEvent). I don't like it from the API design perspective.
  • the script on the lab side is wrong with respect to chord because it just skips call to processKeydownEvent if it find that user input arises from a new key event; this can still result in a command being dispatched if it was a partial match; we need a better solution on lumino level

@krassowski
Copy link
Member Author

Currently thinking about adding some kind of a hook in _executeKeyBinding but don't see a clean API for it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant