Execute all matching key binding variations #8

Closed
ocavue wants to merge 2 commits from ocavue-group-map into master
ocavue commented 2025-12-03 15:38:03 +01:00 (Migrated from github.com)

When users send two commands with the same key string to the keymap() plugin, it's expected that only one will be executed:

keymap({"c-Space": cmd1, "c-Space": cmd2})
// Only `cmd2` is actually passed to `keymap`, so it's normal for `cmd1` to be ignored.

However, when two commands with different variations of the same key are provided, prosemirror-keymap still executes only one of them, which I found to be a bit surprising.

keymap({"Ctrl-Space": cmd1, "c-Space": cmd2})
// Both `cmd1` and `cmd2` are passed to `keymap`, but only `cmd2` will be called.

Modifiers can be given in any order. Shift- (or s-), Alt- (or a-), Ctrl- (or c- or Control-) and Cmd- (or m- or Meta-) are recognized.
...
You can use Mod- as a shorthand for Cmd- on Mac and Ctrl- on other platforms.

Some examples of similar variations include:

  • Ctrl-Space vs Control-Space: different variations of the same modifier
  • Ctrl-Shift-Space vs Shift-Ctrl-Space: different order of modifiers
  • Mod-a vs mod-a: Undocumented modifier. Although the document never stated that lower case mod- is a valid modifier, it works find.

This PR addresses the issue by allowing different variations of the same key.


An alternative approach would be for prosemirror-keymap to expose normalizeKeyName as a public API, allowing me to accomplish the same task externally.

When users send two commands with the same key string to the `keymap()` plugin, it's expected that only one will be executed: ```ts keymap({"c-Space": cmd1, "c-Space": cmd2}) // Only `cmd2` is actually passed to `keymap`, so it's normal for `cmd1` to be ignored. ``` However, when two commands with **different variations** of the **same key** are provided, `prosemirror-keymap` still executes only one of them, which I found to be a bit surprising. ```ts keymap({"Ctrl-Space": cmd1, "c-Space": cmd2}) // Both `cmd1` and `cmd2` are passed to `keymap`, but only `cmd2` will be called. ``` > Modifiers can be given in any order. Shift- (or s-), Alt- (or a-), Ctrl- (or c- or Control-) and Cmd- (or m- or Meta-) are recognized. > ... > You can use Mod- as a shorthand for Cmd- on Mac and Ctrl- on other platforms. Some examples of similar variations include: - `Ctrl-Space` vs `Control-Space`: different variations of the same modifier - `Ctrl-Shift-Space` vs `Shift-Ctrl-Space`: different order of modifiers - `Mod-a` vs `mod-a`: Undocumented modifier. Although the document never stated that lower case `mod-` is a valid modifier, it works find. This PR addresses the issue by allowing different variations of the same key. --- An alternative approach would be for `prosemirror-keymap` to expose `normalizeKeyName` as a public API, allowing me to accomplish the same task externally.
ocavue commented 2025-12-03 15:46:37 +01:00 (Migrated from github.com)

Notice that two existing tests are failing on the master branch. The new added test is passed.

npm test
npm warn config ignoring workspace config at /Users/ocavue/code/github/prosemirror/keymap/.npmrc

> prosemirror-keymap@1.2.3 test
> pm-runtests



  keymap
    ✔ calls the correct handler
    ✔ distinguishes between modifiers
    ✔ passes the state, dispatch, and view
    1) tries both shifted key and base with shift modifier
    ✔ tries keyCode when modifier active
    2) tries keyCode for non-ASCII characters
    ✔ tries all keybindings for a key with different variations


  5 passing (3ms)
  2 failing

  1) keymap
       tries both shifted key and base with shift modifier:
     0 != 1 at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:47:9)
  Error: 0 != 1
      at new ist.Failure (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:42:11)
      at ist (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:34:13)
      at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:47:9)

  2) keymap
       tries keyCode for non-ASCII characters:
     0 != 1 at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:57:9)
  Error: 0 != 1
      at new ist.Failure (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:42:11)
      at ist (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:34:13)
      at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:57:9)



npm error Lifecycle script `test` failed with error:
npm error code 1
npm error path /Users/ocavue/code/github/prosemirror/keymap
npm error workspace prosemirror-keymap@1.2.3
npm error location /Users/ocavue/code/github/prosemirror/keymap
npm error command failed
npm error command sh -c pm-runtests
Notice that two existing tests are failing on the master branch. The new added test is passed. ``` npm test npm warn config ignoring workspace config at /Users/ocavue/code/github/prosemirror/keymap/.npmrc > prosemirror-keymap@1.2.3 test > pm-runtests keymap ✔ calls the correct handler ✔ distinguishes between modifiers ✔ passes the state, dispatch, and view 1) tries both shifted key and base with shift modifier ✔ tries keyCode when modifier active 2) tries keyCode for non-ASCII characters ✔ tries all keybindings for a key with different variations 5 passing (3ms) 2 failing 1) keymap tries both shifted key and base with shift modifier: 0 != 1 at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:47:9) Error: 0 != 1 at new ist.Failure (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:42:11) at ist (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:34:13) at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:47:9) 2) keymap tries keyCode for non-ASCII characters: 0 != 1 at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:57:9) Error: 0 != 1 at new ist.Failure (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:42:11) at ist (file:///Users/ocavue/code/github/prosemirror/node_modules/ist/ist.js:34:13) at Context.<anonymous> (file:///Users/ocavue/code/github/prosemirror/keymap/test/test-keymap.js:57:9) npm error Lifecycle script `test` failed with error: npm error code 1 npm error path /Users/ocavue/code/github/prosemirror/keymap npm error workspace prosemirror-keymap@1.2.3 npm error location /Users/ocavue/code/github/prosemirror/keymap npm error command failed npm error command sh -c pm-runtests ```
marijnh commented 2025-12-03 18:11:09 +01:00 (Migrated from github.com)

I don't think this is a useful enough thing to complicate the package's data structures for. It would be more sensible to raise an error when someone does this. Just use chainCommands to bind multiple commands to a single key.

I don't think this is a useful enough thing to complicate the package's data structures for. It would be more sensible to raise an error when someone does this. Just use [`chainCommands`](https://prosemirror.net/docs/ref/#commands.chainCommands) to bind multiple commands to a single key.
ocavue commented 2025-12-03 18:26:34 +01:00 (Migrated from github.com)

@marijnh Thanks for your reply! Would you be open to exporting normalizeKeyName as a public API? I can group multiple commands with the same key after normalization on my end.

@marijnh Thanks for your reply! Would you be open to exporting [`normalizeKeyName`](https://github.com/prosemirror/prosemirror-keymap/blob/081f24f65b6a394174a4bdf07b9a55a00361f1b7/src/keymap.ts#L8) as a public API? I can group multiple commands with the same key _**after**_ normalization on my end.
marijnh commented 2025-12-03 18:29:19 +01:00 (Migrated from github.com)

Are you aware that you can add multiple keymaps to a single editor? There's no need to combine them in code.

Are you aware that you can add multiple keymaps to a single editor? There's no need to combine them in code.
ocavue commented 2025-12-03 18:44:33 +01:00 (Migrated from github.com)

Yes, I understand that I can have multiple plugins returned by keymap().

The challenge is that sometimes I want to update the keybindings dynamically after the editor has been initialized. For example, I might want to change some keyboard shortcuts based on user settings.

I can use view.setProps to update the plugins list, but that would trigger a state update, which seems unnecessary since neither state.doc nor state.selection changes in this case.

So, my current solution is to register a single plugin with the handleKeyDown prop. Inside that handler, I use event delegation and the keydownHandler to manage keybindings. Therefore, I want to pass all keybindings as one large keymap object. That's when I found that two keybindings with different key strings could override each other.

Yes, I understand that I can have multiple plugins returned by [`keymap()`](https://prosemirror.net/docs/ref/#keymap.keymap). The challenge is that sometimes I want to update the keybindings dynamically after the editor has been initialized. For example, I might want to change some keyboard shortcuts based on user settings. I can use [`view.setProps`](https://prosemirror.net/docs/ref/#view.EditorView.setProps) to update the plugins list, but that would trigger a state update, which seems unnecessary since neither `state.doc` nor `state.selection` changes in this case. So, my current solution is to register a single plugin with the [handleKeyDown](https://prosemirror.net/docs/ref/#view.EditorProps.handleKeyDown) prop. Inside that handler, I use event delegation and the [keydownHandler](https://prosemirror.net/docs/ref/#keymap.keydownHandler) to manage keybindings. Therefore, I want to pass all keybindings as one large keymap object. That's when I found that two keybindings with different key strings could override each other.
marijnh commented 2025-12-04 07:51:09 +01:00 (Migrated from github.com)

Therefore, I want to pass all keybindings as one large keymap object.

In this situation you can also loop over a list of keydown handlers. Which means there's still no need to combine keymap objects.

> Therefore, I want to pass all keybindings as one large keymap object. In this situation you can also loop over a list of keydown handlers. Which means there's still no need to combine keymap objects.
ocavue commented 2025-12-05 12:08:55 +01:00 (Migrated from github.com)

That totally makes sense! I guess calling several dozen keydown handlers for every key press shouldn't significantly impact performance. Thank you for your response!

That totally makes sense! I guess calling several dozen keydown handlers for every key press shouldn't significantly impact performance. Thank you for your response!
marijnh commented 2025-12-05 12:15:25 +01:00 (Migrated from github.com)

Attached patch adds a check for this, so it doesn't cause accidental issues.

Attached patch adds a check for this, so it doesn't cause accidental issues.

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
prosemirror/prosemirror-keymap!8
No description provided.