refactor(KeyboardShortcuts): use key codes over key names

As per #1746 (comment)

specifyweb/frontend/js_src/lib/components/Keyboard/__tests__/UserDefinitions.test.ts

@@ -1,9 +1,10 @@
-import type { KeyboardShortcuts, ModifierKey } from '../context';
-import { exportsForTests, keyJoinSymbol } from '../context';
 import type { GenericPreferences } from '../../Preferences/types';
 import { userPreferenceDefinitions } from '../../Preferences/UserDefinitions';
+import type { KeyboardShortcuts, ModifierKey } from '../config';
+import { keyLocalizations, modifierKeys, specialKeyboardKeys } from '../config';
+import { exportsForTests, keyJoinSymbol } from '../context';
 
-const { keysToString, modifierKeys, specialKeys } = exportsForTests;
+const { keysToString } = exportsForTests;
 
 test('Validate default keyboard shortcuts in userPreferenceDefinitions', () => {
   Object.entries(userPreferenceDefinitions as GenericPreferences).forEach(
@@ -51,9 +52,6 @@ function validateShortcut(
   const parts = shortcut.split(keyJoinSymbol);
   if (parts.length === 0) return 'unexpected empty shortcut';
 
-  /*
-   * FIXME: add test against default preferences containing non-existing shortcuts
-   */
   const shortcutModifierKeys = parts
     .map((part) => part as ModifierKey)
     .filter((part) => modifierKeys.includes(part))
@@ -69,7 +67,9 @@ function validateShortcut(
     return `shortcut is not normalized: ${normalizedShortcut} (expected keys to be sorted, with modifier keys before non-modifiers)`;
 
   if (shortcutModifierKeys.length === 0) {
-    const specialKey = nonModifierKeys.find((key) => specialKeys.has(key));
+    const specialKey = nonModifierKeys.find((key) =>
+      specialKeyboardKeys.has(key)
+    );
     if (typeof specialKey === 'string')
       return `can't use special reserved keys as default shortcuts, unless prefixed with a modifier key. Found ${specialKey}`;
   }
@@ -80,6 +80,10 @@ function validateShortcut(
   if (nonModifierKeys.length === 0)
     return 'shortcut must contain at least one non-modifier key';
 
+  const disallowedKey = nonModifierKeys.find((key) => !allowed.has(key));
+  if (typeof disallowedKey === 'string')
+    return `found an unknown key code: ${disallowedKey}. Make sure you are using key code rather than key name (e.g. use "KeyA" instead of "a"). To test, see what "keydown" event gives you for event.code (rather than event.key). If you are sure you have the key code, then you can extend the list of allowed keys in this test`;
+
   return undefined;
 }
 
@@ -93,41 +97,18 @@ function validateShortcut(
  */
 const allowed = new Set([
   // Not including modifier keys as the above check will only check non-modifiers
-  ...Array.from(specialKeys),
-  'ArrowDown',
-  'ArrowLeft',
-  'ArrowRight',
-  'ArrowUp',
-  'End',
-  'Home',
-  'PageDown',
-  'PageUp',
-  'Insert',
-  'F1',
-  'F2',
-  'F3',
-  'F4',
-  'F5',
-  'F6',
-  'F7',
-  'F8',
-  'F9',
-  'F10',
-  'F11',
-  'F12',
+  ...Array.from(specialKeyboardKeys),
+  ...Object.keys(keyLocalizations),
   'BrowserBack',
   'BrowserForward',
   'BrowserHome',
   'BrowserRefresh',
-  'BrowserrSearch',
+  'BrowserSearch',
   'Add',
   'Multiply',
   'Subtract',
   'Decimal',
   'Divide',
-  ...'01234567890-=qwertyuiop[]asdfghjkl;\'zxcvbnm,./\\`~!@#$%^&*()_+QWERTYUIOP{}|ASDFGHJKL:"ZXCVBNM<>?'.split(
-    ''
-  ),
   /*
    * This list doesn't include more obscure keys, but we probably shouldn't be
    * using those in key shortcuts anyway

specifyweb/frontend/js_src/lib/components/Keyboard/config.ts

@@ -1,5 +1,5 @@
 import { preferencesText } from '../../localization/preferences';
-import type { RA, RR } from '../../utils/types';
+import type { IR, RA, RR } from '../../utils/types';
 
 /**
  * Because operating systems, browsers and browser extensions define many
@@ -29,12 +29,18 @@ export const keyboardPlatform: KeyboardPlatform =
     ? 'windows'
     : 'other';
 
-const modifierKeys = ['Alt', 'Ctrl', 'Meta', 'Shift'] as const;
+export const modifierKeys = ['Alt', 'Ctrl', 'Meta', 'Shift'] as const;
 export type ModifierKey = typeof modifierKeys[number];
 export const allModifierKeys = new Set([
   ...modifierKeys,
   'AltGraph',
   'CapsLock',
+  'MetaLeft',
+  'MetaRight',
+  'ShiftLeft',
+  'ShiftRight',
+  'AltLeft',
+  'AltRight',
 ]);
 
 export const keyboardModifierLocalization: RR<ModifierKey, string> = {
@@ -66,7 +72,78 @@ export const keyboardModifierLocalization: RR<ModifierKey, string> = {
 export const specialKeyboardKeys = new Set([
   'Enter',
   'Tab',
-  ' ',
+  'Space',
   'Escape',
   'Backspace',
 ]);
+
+/**
+ * Because we are listening to key codes that correspond to US English letters,
+ * we should show keys in the UI in US English to avoid confusion.
+ * (otherwise Cmd+O is ambiguous as it's not clear if it refers to English O or
+ * local language О).
+ * See https://github.com/specify/specify7/issues/1746#issuecomment-2227113839
+ *
+ * For some keys, it is less confusing to see a symbol (like arrow keys), rather
+ * than 'ArrowUp', thus symbols are used for those keys.
+ * See http://xahlee.info/comp/unicode_computing_symbols.html
+ *
+ * Try not to define keyboard shortcuts for keys that may be in a different
+ * place in other keyboard layouts (the positions of special symbols wary a lot)
+ */
+export const keyLocalizations: IR<string> = {
+  ArrowDown: '↓',
+  ArrowLeft: '←',
+  ArrowRight: '→',
+  ArrowUp: '↑',
+  Backquote: '`',
+  Backslash: '\\',
+  Backspace: '⌫',
+  BracketLeft: '[',
+  BracketRight: ']',
+  Comma: ',',
+  Digit0: '0',
+  Digit1: '1',
+  Digit2: '2',
+  Digit3: '3',
+  Digit4: '4',
+  Digit5: '5',
+  Digit6: '6',
+  Digit7: '7',
+  Digit8: '8',
+  Digit9: '9',
+  // "return" key is used over Enter on macOS keyboards
+  Enter: keyboardPlatform === 'mac' ? '↵' : 'Enter',
+  Equal: '=',
+  KeyA: 'a',
+  KeyB: 'b',
+  KeyC: 'c',
+  KeyD: 'd',
+  KeyE: 'e',
+  KeyF: 'f',
+  KeyG: 'g',
+  KeyH: 'h',
+  KeyI: 'i',
+  KeyJ: 'j',
+  KeyK: 'k',
+  KeyL: 'l',
+  KeyM: 'm',
+  KeyN: 'n',
+  KeyO: 'o',
+  KeyP: 'p',
+  KeyQ: 'q',
+  KeyR: 'r',
+  KeyS: 's',
+  KeyT: 't',
+  KeyU: 'u',
+  KeyV: 'v',
+  KeyW: 'w',
+  KeyX: 'x',
+  KeyY: 'y',
+  KeyZ: 'z',
+  Minus: '-',
+  Period: '.',
+  Quote: "'",
+  Semicolon: ';',
+  Slash: '/',
+};

specifyweb/frontend/js_src/lib/components/Keyboard/context.tsx

@@ -62,8 +62,8 @@ const pressedKeys: string[] = [];
 document.addEventListener('keydown', (event) => {
   if (shouldIgnoreKeyPress(event)) return;
 
-  if (!pressedKeys.includes(event.key)) {
-    pressedKeys.push(event.key);
+  if (!pressedKeys.includes(event.code)) {
+    pressedKeys.push(event.code);
     pressedKeys.sort();
   }
 
@@ -73,7 +73,7 @@ document.addEventListener('keydown', (event) => {
   // Ignore shortcuts that result in printed characters when in an input field
   const ignore = isPrintable && isEntering;
   if (ignore) return;
-  if (modifiers.length === 0 && specialKeyboardKeys.has(event.key)) return;
+  if (modifiers.length === 0 && specialKeyboardKeys.has(event.code)) return;
 
   const keyString = keysToString(modifiers, pressedKeys);
   const handler = interceptor ?? listeners.get(keyString);
@@ -96,7 +96,7 @@ function shouldIgnoreKeyPress(event: KeyboardEvent): boolean {
   // See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#value
   if (key === 'Dead' || key === 'Unidentified') return true;
 
-  // Do not allow binding a key shortcut to a modifier key only
+  // Do not allow binding a key shortcut directly to a modifier key
   const isModifier = allModifierKeys.has(event.key);
 
   return !isModifier;

specifyweb/frontend/js_src/lib/components/Keyboard/shortcuts.tsx

@@ -27,10 +27,6 @@ import { localizeKeyboardShortcut, resolvePlatformShortcuts } from './utils';
  * those in the UI if present on the page
  *
  * FIXME: open key shortcut viewer on cmd+/
- *
- * FIXME: localize some key shortcuts (arrow keys, home, etc)
- *
- * FIXME: add a mapping of allowed default keyboard shortcuts and use that in tests
  */
 
 export function KeyboardShortcutPreferenceItem({

specifyweb/frontend/js_src/lib/components/Keyboard/utils.ts

@@ -3,15 +3,24 @@ import type { LocalizedString } from 'typesafe-i18n';
 import type { RA } from '../../utils/types';
 import { localized } from '../../utils/types';
 import type { KeyboardShortcuts, ModifierKey } from './config';
-import { keyboardModifierLocalization, keyboardPlatform } from './config';
+import {
+  keyboardModifierLocalization,
+  keyboardPlatform,
+  keyLocalizations,
+} from './config';
 import { keyJoinSymbol } from './context';
 
 const localizedKeyJoinSymbol = ' + ';
 export const localizeKeyboardShortcut = (shortcut: string): LocalizedString =>
   localized(
     shortcut
       .split(keyJoinSymbol)
-      .map((key) => keyboardModifierLocalization[key as ModifierKey] ?? key)
+      .map(
+        (key) =>
+          keyboardModifierLocalization[key as ModifierKey] ??
+          keyLocalizations[key] ??
+          key
+      )
       .join(localizedKeyJoinSymbol)
   );
 
@@ -41,11 +50,11 @@ export function resolvePlatformShortcuts(
 const replaceCtrlWithMeta = (shortcut: string): string =>
   shortcut
     .split(keyJoinSymbol)
-    .map((key) => (key === 'ctrl' ? 'meta' : key))
+    .map((key) => (key === 'Ctrl' ? 'Meta' : key))
     .join(keyJoinSymbol);
 
 const replaceMetaWithCtrl = (shortcut: string): string =>
   shortcut
     .split(keyJoinSymbol)
-    .map((key) => (key === 'meta' ? 'ctrl' : key))
+    .map((key) => (key === 'Meta' ? 'Ctrl' : key))
     .join(keyJoinSymbol);

specifyweb/frontend/js_src/lib/components/Preferences/BasePreferences.tsx

@@ -18,11 +18,11 @@ import {
   contextUnlockedPromise,
   foreverFetch,
 } from '../InitialContext';
+import { bindKeyboardShortcut } from '../Keyboard/context';
 import {
-  bindKeyboardShortcut,
+  localizeKeyboardShortcut,
   resolvePlatformShortcuts,
-} from '../Keyboard/context';
-import { localizeKeyboardShortcut } from '../Keyboard/shortcuts';
+} from '../Keyboard/utils';
 import { formatUrl } from '../Router/queryString';
 import type { GenericPreferences, PreferenceItem } from './types';
 

specifyweb/frontend/js_src/lib/components/Preferences/UserDefinitions.tsx

@@ -37,7 +37,7 @@ import type { TableFields } from '../DataModel/helperTypes';
 import { genericTables } from '../DataModel/tables';
 import type { Collection, Tables } from '../DataModel/types';
 import { error, softError } from '../Errors/assert';
-import type { KeyboardShortcuts } from '../Keyboard/context';
+import type { KeyboardShortcuts } from '../Keyboard/config';
 import { KeyboardShortcutPreferenceItem } from '../Keyboard/shortcuts';
 import type { StatLayout } from '../Statistics/types';
 import {
@@ -82,6 +82,14 @@ const tableLabel = (tableName: keyof Tables): LocalizedString =>
 
 const defineKeyboardShortcut = (
   title: LocalizedString,
+  /**
+   * If defined a keyboard shortcut for one platform, it will be automatically
+   * transformed (`ctrl -> cmd`) for the other platforms.
+   *
+   * Thus, you should define keyboard shortcuts for the "other" platform only,
+   * unless you actually want to use different keyboard shortcuts on different
+   * systems.
+   */
   defaultValue: KeyboardShortcuts | string
 ): PreferenceItem<KeyboardShortcuts> =>
   definePref<KeyboardShortcuts>({

specifyweb/frontend/js_src/lib/utils/types.ts

@@ -215,11 +215,13 @@ export const ensure =
  * empty string is used. However, empty string is not a LocalizedString, so
  * this hack is needed
  *
- * @example Valid use case
+ * @example
+ * // Valid use case
  * localized('')
  *
- * @example Invalid use case
+ * @example
  * ```ts
+ * // Invalid use case
  * // Wrong:
  * localized(table.name)
  * // Should use table label instead (unless there is a good reason to use