FIameCaster / prism-code-editor

Lightweight, extensible code editor component for the web using Prism
https://prism-code-editor.netlify.app
MIT License
58 stars 8 forks source link

Most Default Commands don’t work on a non-QWERTY Keyboard #28

Open amxmln opened 3 months ago

amxmln commented 3 months ago

I have a QWERTZ keyboard and am unable to use the following default commands:

While the first four simply do not work (even when using the physical keys representing the keyCodes used in the source, probably because native features override them), the last one does add a block comment, but instead of the text it only contains the letter Å, so for example language: 'html', becomes /* Å */.

The first undo restores the original text, but commented out, the second undo removes the comment.

These issues are inherent to using KeyboardEvent.code / KeyboardEvent.keyCode instead of KeyboardEvent.key. Unfortunately, KeyboardEvent.key isn’t reliable as soon as the Shift- or Option-keys on Mac are involved, as that triggers different return values for KeyboardEvent.key, so I’m not quite sure how that could be fixed.

Unfortunately, I also couldn’t find an option to easily remap most of the default commands, as they’re added via a keydown listener, not via the keyCommandMap.

FIameCaster commented 3 months ago

I don't know which QWERTZ keyboard layout you have, but I tried the German QWERTZ layout in Windows and every command worked in Google Chrome. In Firefox, none of the keys on my keyboard had the corresponding keyCode for the Outdent line, Indent line, and Toggle comment commands, so those commands weren't possible to execute. The same commands also don't work in VSCode in Firefox with the German QWERTZ layout, so they're probably using keyCode under the hood as well. I can hardly execute any commands in CodeMirror with this layout.

Unfortunately, KeyboardEvent.key isn’t reliable as soon as the Shift- or Option-keys on Mac are involved, as that triggers different return values for KeyboardEvent.key, so I’m not quite sure how that could be fixed.

Yup, using KeyboardEvent.key for commands is not a great idea since you might have to press extra modifier keys. With a Norwegian keyboard layout, you have to press Shift + 7 for / for example, which would make the comment toggling command impossible to execute if we used key.

I don't think it's possible to make commands consistent across multiple browsers and keyboard layouts. We could use KeyboardEvent.code, which means the same physical key would be used regardless of keyboard layout for a command, but this is not what other editors do.

the last one does add a block comment, but instead of the text it only contains the letter Å, so for example language: 'html', becomes / Å /.

I've forgotten to call Event.preventDefault here, so this can easily be fixed.

amxmln commented 3 months ago

Thanks for your quick responses!

I’m on a German QWERTZ keyboard on a Mac, I’ve tested on Linux (so Windows QWERTZ layout) as well.

The other commands seem to be working on Linux, as far as I could tell.

Out of curiousity, I tested Codemirror as well and all commands in the default keymap seemed to work on Linux, but with the actual keys on the keyboard (so the actual [-Key for example, which is AltGr + 8), leading me to initially believe they were using event.key, however on Mac, hardly any of the commands worked?

So I looked into the code and at least according to this it looks like they’re using event.key, however, I also did find a reference to event.keyCode here.

To me it makes a lot of sense to use event.key because then at least the bindings are more likely to work, even if they might not be the most ergonomic. Pressing Cmd + Shift + 7 (resulting in Cmd + /) to toggle a line comment matches what I do in VS Code as well (although there I have also a custom binding to Cmd + # for convenience).

I think for the reduced scope of this project, it would also be fine, if we could set up / overwrite our own bindings for all the commands using the existing keyCommandMap. Is that something that would be more likely to be supported?

panoply commented 3 months ago

I have Swedish keyboard and do not have issues. FWIW you can use the existing keyCommandMap to set your own logic. Here is how I establish cmd + s in a project:

  const MetaKey = editor.keyCommandMap.Meta;
  let mk: boolean = false;

  editor.keyCommandMap.Meta = (e, selection, value) => {
    mk = true;
    return MetaKey?.(e, selection, value);
  };

  editor.keyCommandMap.s = (_event, _selection, value) => {
    if (!mk) return;
    mk = false;
    for (const [ cb, scope ] of events.onsave) {
      cb.call(assign(scope, { 
        get editor () { 
          return instance; 
         } 
       }), value);
    }
    return true;
  };

IIRC you can already override existing maps by mutating the command map, it should be in docs or readme.

amxmln commented 3 months ago

Thank you for your example, however, most of the functionality of the Commands-extension is not enabled through the keyCommandMap, so there seems to be no way to overwrite it.

FIameCaster commented 2 months ago

By the way, I did add the missing Event.preventDefaults, so as long as MacOS doesn't consume the commands before they reach the browser, they should all work. You can test it in the autocomplete demo.

Some of the commands don't use keyCommandMap because if it's used you're forced to use KeyboardEvent.key. Currently, the keys needed to execute commands are identical to the defaults in VSCode across all keyboard layouts I've tested. This wouldn't be possible if we used the keyCommandMap.

The defaultCommands extension doesn't use anything internal apart from some simple utilities, so it would be possible for anyone to copy its source code and change when commands are executed in their own projects. Not a great solution, but a possible one.

amxmln commented 2 months ago

I see, that’s what I was thinking of doing, but yeah, it increases the cost of maintenance as the project evolves. 😅

Maybe adding a note about these commands not being overwriteable via the keyCommandMap and the suggestion to manually re-implement the commands to the README would be good? 🤔

FIameCaster commented 2 months ago

Maybe adding a note about these commands not being overwriteable via the keyCommandMap and the suggestion to manually re-implement the commands to the README would be good? 🤔

I think it might be better to put that information in the JSDoc for the extension.