Ignore keydown events for modifier keys when accumulating key sequence

[ph-ph]

This change addresses JupyterLab issue: jupyterlab/jupyterlab#7702 .

It follows the strategy proposed by @jasongrout in the issue comments:

• adds isIgnoredKey method to keyboard layout interface
• adds modifier keys to en-us layout, but marks them as ignored
• skips processing of 'keydown' event for ignored keys.

I can see several reasons for why the change might be concerning:

• default keyboard layout now has new valid keys, which may break some downstream dependency
• we rely on keyCode to detect modifier keys, which is probably not the best way in the long run
• skipping processing of ignored keys actually changes the order in which keydown events are processed. E.g. if user presses "Shift", "Shift C", "Ctrl", "Ctrl P", and there's no "Shift C Ctrl P" shortcut, the events will be processed in the order "Shift", "Ctrl" (ignored events that we let propagate), "Shift C", "Ctrl P" (replayed events). Probably ok for modifier keys, but may cause weird behavior if we start ignoring non-modifier keys in the future.

Let me know if you think there's a better way.

How it was tested

• Added new specs for commands and keyboard packages
• Locally modified dockpanel example to have shortcuts like "Shift C Ctrl P", verified that the shortcuts worked as expected.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[blink1073]

Hi @ph-ph, given the concerns you raise, I'd be in favor of only ignoring modifier key codes with the name modifierCodes, and allowing a keyboard layout to optionally provide them. We'd use isModifierCode as the test function name. As for keycode, we're stuck with that until there is broader support for KeyboardEvent.code. We haven't tested it in a few years, but that is a separate effort/discussion.

[blink1073]

cc @jasongrout who made the original suggestions for handling the issue

[ph-ph]

Thank you, @blink1073 . I renamed isIgnoredKey to isModifierKey (chose not to use "Code" in the layout interface since it's an implementation detail). Please take another look when you can.

[blink1073]

Thanks! I'm currently stuck in a release quagmire. 😄 I will review tomorrow.

[blink1073]

LGTM, thank you!

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️