Configurable keyboard shortcuts

[holloway]

Per the instructions in #57 (specifically this comment) I've made the keys configurable.

There's an optional prop keybinding that you can provide an object that looks like,

export const DEFAULT_KEYBINDING: Keybinding = {
  Backspace: "Delete",
  Tab: "FocusOutsideNext",
  "Tab+shift": "FocusOutsidePrev",
  "ArrowDown+meta": "SelectAndActivate",
  ArrowDown: "Next",
  ArrowUp: "Prev",
  ArrowRight: "Right",
  ArrowLeft: "Left",
  "a+meta": "SelectAll",
  a: "CreateLeaf",
  A: "CreateInternal",
  Home: "FocusFirst",
  End: "FocusLast",
  Enter: "Rename",
  " ": "SelectOrToggle",
  "*": "OpenSiblings",
  PageUp: "PageUp",
  PageDown: "PageDown",
};

The keys of this object are arbitrary strings referring to keyboard buttons, and the values are restricted (in TS) to the names of valid commands. Simultaneous keyboard button presses can be expressed with the "tab+shift" syntax in the key.

I've made all keys configurable so that people can remap arrow keys to WASD if they prefer (maybe they're using arrow keys for other things in their app...not sure if we can presume they're not 🤷).

The user provided keybinding prop isn't merged with DEFAULT_KEYBINDING and this is intentional. This makes it very explicit and clear, but it has some DX ramifications (pros and cons). This makes it easy to understand the full set of keys being used, and to exclude (eg) the ' ' (space) key by simply not including that in the keybinding prop. Unfortunately this means that if a user configures one key they need to configure them all, however there's an exported DEFAULT_KEYBINDING that they can spread to reuse defaults like this,

keybinding={{
   's': 'Next',
   ...DEFAULT_KEYBINDING,
}}

This way it's explicit without default per-key being applied.

We could also export WCAG_KEYBINDING so that a user could use it like, keybinding={WCAG_KEYBINDING} or merge like keybinding={ {'s': 'Next', ...WCAG_KEYBINDING }}. This is a good reason not to have defaults per-key, because applying WCAG_KEYBINDING while also undoing DEFAULT_KEYBINDING would be more complex.

Keen for feedback on anything but specifically would like feedback on

• file layout and names of functions / variables
  • Command names I made up... eg "FocusOutsidePrev" and "SelectAndActivate" so some suggestions would be appreciated.
• Where can I cache/memoise the call to parseKeybinding?
• The Command takes the Tree as you suggested but also a 2nd arg of a e keyboard event. Is this ok? As you'll see in commands.ts some commands need an e.currentTarget.
• Key matching syntax is case insensitive and currently it supports + or 'space' as separator keys. Hopefully this makes writing the key matches easier.
  • If the key matching syntax was case sensitive then for WASD they'd need to have (eg) both "a" and "A" as separate keys in the object. Case insensitive means that "a" and "A" are the same. If they want to refer specifically to the uppercase 'A' while excluding 'a' they can write "a+shift". Is this ok? Seems like it's a better DX and probably what most devs want.
• Does VSCode keybinding syntax (ie `tab+shift' as a string) have any features we want to copy?
• Do we need more docs?

Last thing...this needs testing. Although it seems to work for arrow keys, tab, shift+tab, etc. I haven't tried the more obscure keyboard shortcuts.

[jameskerr]

Awesome! Thank you! A detailed review coming soon.

[holloway]

I use Linux and my filesystem (like most Linux filesystems) is case-sensitive so it needs to match the filename.

[jameskerr]

Thanks so much for your work on this @holloway. I've thought about it a lot and think we should follow the example of VSCode's Keyboard Rules.

Instead of making the keybinding argument an object, I think it should be an array with items that are objects of the type:

{key: string, command: string, when?: string}

Key Property

The "key" value will follow the form of VSCode's Accepted Keys. Examples: "down", "up", "shift+down", "ctrl+a", "cmd+a". Although I think we should add another one called "mod" which maps to either "ctrl" on windows and linux but "cmd" on mac.

Command Property

Here are the names I'd like to give the commands:

type TreeCommand =
  | "tabNext"
  | "tabPrev"

  | "pageUp"
  | "pageDown"

  | "focusNext"
  | "focusPrev"
  | "focusParent"
  | "focusFirst"
  | "focusLast"
  
  | "createLeaf"
  | "createInternal"
  | "rename"
  | "delete"
  | "activate"

  | "open"
  | "openSiblings"
  | "close"
  | "closeSiblings"
  | "toggle"
  | "toggleSiblings"
  
  | "select"
  | "selectAll"
  | "selectContiguousNext"
  | "selectContiguousPrev"
  | "selectMulti";

When Property

You'll notice that none of these commands have the words "or/and" in them. We'll take care of conditions in the "when" clause of the keybinding object. The states we need in the when clause are currently: "isLeaf", "isInternal", "isOpen", "isClosed". For example, the "left" key needs to either focus the parent or close the focused node depending on the state of the focused node.

[
  {key: "left", command: "focusParent", when: "isLeaf || isClosed"},
  {key: "left", command: "close", when: "isOpen"}
]

We'll need to make a miniature language parser for the when clause string. Those states will always be from the focused node. If there is no when clause, it will always run that command.

File Locations

The new files should be:

src/commands.ts (all the command functions live here)
src/keyboard/when-parser.ts (exports function to parse a when clause)
src/keyboard/match-rule.ts (exports a function to find the proper rule given the Keyboard event and array of rules)
src/keyboard/default-rules.ts (exports the default array of rules)

Other Notes

• I'd like the prop passed to the Tree component to be keybinding "overrides". They should not replace them all. And it should not be required to ...merge the defaults into them. To remove a default, we can pass empty string "" to the "command" property at that key.
• When finding a keybinding, we should search the overrides first for a match of the key and the when clause, then search the defaults.
• As for style, name all the functions with a "camelCase" name.

Thanks for diving in on this!

[holloway]

Hi @jameskerr really sorry about this but I have to work on other things at work now so I won't be able to progress this further. Hopefully someone else can pick it up. Again, sorry about this.

[jameskerr]

No problem at all! Thank you for getting the ball rolling. I like the direction it's headed.

[jameskerr]

I'm going to close this and revisit when it becomes a priority again.

[joshmarnold]

@jameskerr, what's the plan for this feature? I'll be needing it at some point, but like @holloway, don't have the time to work on it myself.

[jameskerr]

If no one else jumps on this, I'll probably start working on it not in the next few weeks, but perhaps in the next few months.

[muscal]

'a' is commonly used as part of 'ctrl+a' shortcut for selecting all. It would make more sense for keybinding the '+' character for creation of leafs.

[jameskerr]

Yes, that does seem like a good shortcut.

[muscal]

@jameskerr would you be able to spin up a new PR for this change?

[jameskerr]

I'm excited to work on this, but it's not in my immediate queue.

[HeavensRegent]

The proposed keyboard shortcut changes seem to be an improvement over the current settings. No customization vs customization. This flexibility would enable us, as developers, to modify the shortcuts to our preference.

Although the proposed changes may not be perfect, they could serve as a beneficial temporary solution. Would it be reasonable to implement these changes for the time being?

[jameskerr]

Configuration will be added in version 4. You can track progress here #235