Add customizable shortcuts panel

[ollimeier]

This fixes #1594, fixes #1589, fixes #721, and fixes #1642.

Known issues:

• event.preventDefault() does not work for default shortcuts like cmd+N (event.stopImmediatePropagation() does not work either) -> some default shortcuts cannot be overwritten, please see: bugs.chromium -> Good comment related to this issue

[justvanrossum]

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

[ollimeier]

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

I agree. I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files It's related to collapseSubTools.

[justvanrossum]

I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files

Ah, but that PR got merged into a branch which is now abandoned, right?

[ollimeier]

I fixed it here already: https://github.com/googlefonts/fontra/pull/1604/files

Ah, but that PR got merged into a branch which is now abandoned, right?

yes.

[justvanrossum]

Regarding the event.key vs event.code vs event.keyCode issue: the solution is in the Keyboard API, but this isn't (yet?) available in Safari and Firefox.

We should either:

• use the Keyboard API if it is available, else fall back to the deprecated event.keyCode property
• use the deprecated event.keyCode property, and start using the Keyboard API later, once it is more available

Spec: https://wicg.github.io/keyboard-map/#keyboard-interface Explainer: https://github.com/wicg/keyboard-map/blob/main/explainer.md

[justvanrossum]

I added a helper func named getBaseKeyFromKeyEvent(event) to actions.js. We should use its return value to:

• match against what it currently called keyOrCode, but should perhaps then be called baseKey
• display the "base" key as part of a menu item shortcut string, and in the custom shortcut UI

[ollimeier]

I added a helper func named getBaseKeyFromKeyEvent(event) to actions.js. We should use its return value to:

• match against what it currently called keyOrCode, but should perhaps then be called baseKey
• display the "base" key as part of a menu item shortcut string, and in the custom shortcut UI

Thanks a lot for the helper function. I used it in the shortcuts panel to record the correct keyOrCode. But I don't fully understand your two bullet points. I don't know what else I should do with it.

For example: match against what it currently calledkeyOrCode, but should perhaps then be calledbaseKey` -> Do you want me to replacekeyOrCodewithbaseKeyeverywhere in the code? Or do you have a specific line of code mind which can be improved withgetBaseKeyFromKeyEvent`?

display the "base" key as part of a menu item shortcut string, and in the custom shortcut UI -> In my opinion, this is already happening, isn't it? The keyOrCode (which should be the same as the baseKey) is used to get the correct icon/symbol via shortCutKeyMap.

I apologize that I am having trouble understanding these instructions.

[ollimeier]

I added a helper func named getBaseKeyFromKeyEvent(event) to actions.js. We should use its return value to:

• match against what it currently called keyOrCode, but should perhaps then be called baseKey
• display the "base" key as part of a menu item shortcut string, and in the custom shortcut UI

I added a helper func named getBaseKeyFromKeyEvent(event) to actions.js. We should use its return value to:

• match against what it currently called keyOrCode, but should perhaps then be called baseKey
• display the "base" key as part of a menu item shortcut string, and in the custom shortcut UI

Done.

[ollimeier]

Some feedback, it's not a complete review.

This doesn't support multiple shortcuts for a single action yet, right?

Some feedback, it's not a complete review.

This doesn't support multiple shortcuts for a single action yet, right?

There is no UI for adding multiple shortcuts to a single action, BUT you can add multiple shortcuts to a json file and import it. It will then support multiple shortcuts (but it will only display the first one).

Is this enough for now or do you want me to add a UI for being able to add and visualize multiple shortcuts? Or is this better for a follow up?

[ollimeier]

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

I noticed that if you assign a shortcut to the ellipse tool, the tool will switch, but the icon doesn't. This is technically unrelated to this PR, but perhaps it stll makes sense to fix it here.

done.

[justvanrossum]

We have a layout issue with long labels:

I don't think we can use the content size, as this will break alignment across sections. But:

• The label could wrap
• We could make the label column wider

(I think we should do both)

[ollimeier]

We have a layout issue with long labels:

I don't think we can use the content size, as this will break alignment across sections. But:

• The label could wrap
• We could make the label column wider

(I think we should do both)

Thanks for the feedback. done.