feat: Share shortcut formatting between help/menu/messages (#408)

* Share shortcut formatting between help/menu/messages

Update "toast" message on enter to reference help which we think is a better
option based on user testing.

Partly motivated by wanting to rebind list_shortcuts in MakeCode and have the
correct shortcut be shown by the toast/dialog.

* fix: retain the short format for menus

* chore: rename shortcut formatting functions

Author: microbit-matt-hillsdon

src/actions/clipboard.ts

@@ -18,6 +18,7 @@ import * as Constants from '../constants';
 import type {BlockSvg, WorkspaceSvg} from 'blockly';
 import {Navigation} from '../navigation';
 import {ScopeWithConnection} from './action_menu';
+import {getShortActionShortcut} from '../shortcut_formatting';
 
 const KeyCodes = blocklyUtils.KeyCodes;
 const createSerializedKey = ShortcutRegistry.registry.createSerializedKey.bind(
@@ -100,7 +101,7 @@ export class Clipboard {
    */
   private registerCutContextMenuAction() {
     const cutAction: ContextMenuRegistry.RegistryItem = {
-      displayText: (scope) => `Cut (${this.getPlatformPrefix()}X)`,
+      displayText: (scope) => `Cut (${getShortActionShortcut('cut')})`,
       preconditionFn: (scope) => {
         const ws = scope.block?.workspace;
         if (!ws) return 'hidden';
@@ -195,7 +196,7 @@ export class Clipboard {
    */
   private registerCopyContextMenuAction() {
     const copyAction: ContextMenuRegistry.RegistryItem = {
-      displayText: (scope) => `Copy (${this.getPlatformPrefix()}C)`,
+      displayText: (scope) => `Copy (${getShortActionShortcut('copy')})`,
       preconditionFn: (scope) => {
         const ws = scope.block?.workspace;
         if (!ws) return 'hidden';
@@ -304,7 +305,7 @@ export class Clipboard {
    */
   private registerPasteContextMenuAction() {
     const pasteAction: ContextMenuRegistry.RegistryItem = {
-      displayText: (scope) => `Paste (${this.getPlatformPrefix()}V)`,
+      displayText: (scope) => `Paste (${getShortActionShortcut('paste')})`,
       preconditionFn: (scope: ScopeWithConnection) => {
         const block = scope.block ?? scope.connection?.getSourceBlock();
         const ws = block?.workspace as WorkspaceSvg | null;
@@ -372,16 +373,4 @@ export class Clipboard {
     Events.setGroup(false);
     return false;
   }
-
-  /**
-   * Check the platform and return a prefix for the keyboard shortcut.
-   * TODO: https://github.com/google/blockly-keyboard-experimentation/issues/155
-   * This will eventually be the responsibility of the action code ib
-   * Blockly core.
-   *
-   * @returns A platform-appropriate string for the meta key.
-   */
-  private getPlatformPrefix() {
-    return navigator.platform.startsWith('Mac') ? '⌘' : 'Ctrl + ';
-  }
 }

src/actions/enter.ts

@@ -22,6 +22,7 @@ import type {
 
 import * as Constants from '../constants';
 import type {Navigation} from '../navigation';
+import {getShortActionShortcut} from '../shortcut_formatting';
 import {Mover} from './mover';
 
 const KeyCodes = BlocklyUtils.KeyCodes;
@@ -103,14 +104,9 @@ export class EnterAction {
     } else if (nodeType === ASTNode.types.BLOCK) {
       const block = curNode.getLocation() as Block;
       if (!this.tryShowFullBlockFieldEditor(block)) {
-        const metaKey = navigator.platform.startsWith('Mac') ? 'Cmd' : 'Ctrl';
-        const canMoveInHint = `Press right arrow to move in or ${metaKey} + Enter for more options`;
-        const genericHint = `Press ${metaKey} + Enter for options`;
-        const hint =
-          curNode.in()?.getSourceBlock() === block
-            ? canMoveInHint
-            : genericHint;
-        dialog.alert(hint);
+        const shortcut = getShortActionShortcut('list_shortcuts');
+        const message = `Press ${shortcut} for help on keyboard controls`;
+        dialog.alert(message);
       }
     } else if (curNode.isConnection() || nodeType === ASTNode.types.WORKSPACE) {
       this.navigation.openToolboxOrFlyout(workspace);

src/keynames.ts

@@ -19,7 +19,7 @@
  *
  * Copied from goog.events.keynames
  */
-const keyNames: Record<string, string> = {
+export const keyNames: Record<string, string> = {
   /* eslint-disable @typescript-eslint/naming-convention */
   8: 'backspace',
   9: 'tab',
@@ -121,70 +121,3 @@ const keyNames: Record<string, string> = {
   224: 'win',
   /* eslint-enable @typescript-eslint/naming-convention */
 };
-
-const modifierKeys = ['control', 'alt', 'meta'];
-
-/**
- * Assign the appropriate class names for the key.
- * Modifier keys are indicated so they can be switched to a platform specific
- * key.
- *
- * @param keyName The key name.
- */
-function getKeyClassName(keyName: string) {
-  return modifierKeys.includes(keyName.toLowerCase()) ? 'key modifier' : 'key';
-}
-
-/**
- * Naive title case conversion. Uppercases first and lowercases remainder.
- *
- * @param str String.
- * @returns The string in title case.
- */
-export function toTitleCase(str: string) {
-  return str.charAt(0).toUpperCase() + str.substring(1).toLowerCase();
-}
-
-/**
- * Convert from a serialized key code to a HTML string.
- * This should be the inverse of ShortcutRegistry.createSerializedKey, but
- * should also convert ascii characters to strings.
- *
- * @param keycode The key code as a string of characters separated
- *     by the + character.
- * @param index Which key code this is in sequence.
- * @returns A single string representing the key code.
- */
-function keyCodeToString(keycode: string, index: number) {
-  let result = `<span class="shortcut-combo shortcut-combo-${index}">`;
-  const pieces = keycode.split('+');
-
-  let piece = pieces[0];
-  let strrep = keyNames[piece] ?? piece;
-
-  for (let i = 0; i < pieces.length; i++) {
-    piece = pieces[i];
-    strrep = keyNames[piece] ?? piece;
-    const className = getKeyClassName(strrep);
-    if (i > 0) {
-      result += '+';
-    }
-    result += `<span class="${className}">${toTitleCase(strrep)}</span>`;
-  }
-  result += '</span>';
-  return result;
-}
-
-/**
- * Convert an array of key codes into a comma-separated list of strings.
- *
- * @param keycodeArr The array of key codes to convert.
- * @returns The input array as a comma-separated list of
- *     human-readable strings wrapped in HTML.
- */
-export function keyCodeArrayToString(keycodeArr: string[]): string {
-  const stringified = keycodeArr.map((keycode, index) =>
-    keyCodeToString(keycode, index),
-  );
-  return stringified.join('<span class="separator">/</span>');
-}

src/shortcut_dialog.ts

@@ -7,7 +7,10 @@
 import * as Blockly from 'blockly/core';
 import * as Constants from './constants';
 import {ShortcutRegistry} from 'blockly/core';
-import {keyCodeArrayToString, toTitleCase} from './keynames';
+import {
+  getLongActionShortcutsAsKeys,
+  upperCaseFirst,
+} from './shortcut_formatting';
 
 /**
  * Class for handling the shortcuts dialog.
@@ -51,28 +54,14 @@ export class ShortcutDialog {
   /**
    * Update the modifier key to the user's specific platform.
    */
-  updatePlatformModifier() {
+  updatePlatformName() {
     const platform = this.getPlatform();
     const platformEl = this.outputDiv
       ? this.outputDiv.querySelector('.platform')
       : null;
-
-    // Update platform string
     if (platformEl) {
       platformEl.textContent = platform;
     }
-
-    if (this.shortcutDialog) {
-      const modifierKeys =
-        this.shortcutDialog.querySelectorAll('.key.modifier');
-
-      if (modifierKeys.length > 0 && platform) {
-        for (const key of modifierKeys) {
-          key.textContent =
-            this.getPlatform() === 'macOS' ? '⌘ Command' : 'Ctrl';
-        }
-      }
-    }
   }
 
   toggle() {
@@ -93,7 +82,7 @@ export class ShortcutDialog {
    * @returns A title case version of the name.
    */
   getReadableShortcutName(shortcutName: string) {
-    return toTitleCase(shortcutName.replace(/_/gi, ' '));
+    return upperCaseFirst(shortcutName.replace(/_/gi, ' '));
   }
 
   /**
@@ -123,20 +112,10 @@ export class ShortcutDialog {
           `;
 
       for (const keyboardShortcut of categoryShortcuts) {
-        const codeArray =
-          ShortcutRegistry.registry.getKeyCodesByShortcutName(keyboardShortcut);
-        if (codeArray.length > 0) {
-          // Only show the first shortcut if there are many
-          const prettyPrinted =
-            codeArray.length > 2
-              ? keyCodeArrayToString(codeArray.slice(0, 1))
-              : keyCodeArrayToString(codeArray);
-
-          modalContents += `
+        modalContents += `
               <td>${this.getReadableShortcutName(keyboardShortcut)}</td>
-              <td>${prettyPrinted}</td>
+              <td>${this.actionShortcutsToHTML(keyboardShortcut)}</td>
               </tr>`;
-        }
       }
       modalContents += '</tr></tbody></table>';
     }
@@ -149,7 +128,7 @@ export class ShortcutDialog {
       this.modalContainer = this.outputDiv.querySelector('.modal-container');
       this.shortcutDialog = this.outputDiv.querySelector('.shortcut-modal');
       this.closeButton = this.outputDiv.querySelector('.close-modal');
-      this.updatePlatformModifier();
+      this.updatePlatformName();
       // Can we also intercept the Esc key to dismiss.
       if (this.closeButton) {
         this.closeButton.addEventListener('click', (e) => {
@@ -159,6 +138,22 @@ export class ShortcutDialog {
     }
   }
 
+  private actionShortcutsToHTML(action: string) {
+    const shortcuts = getLongActionShortcutsAsKeys(action);
+    return shortcuts.map((keys) => this.actionShortcutToHTML(keys)).join(' / ');
+  }
+
+  private actionShortcutToHTML(keys: string[]) {
+    const separator = navigator.platform.startsWith('Mac') ? '' : ' + ';
+    return [
+      `<span class="shortcut-combo">`,
+      ...keys.map((key, index) => {
+        return `<span class="key">${key}</span>${index < keys.length - 1 ? separator : ''}`;
+      }),
+      `</span>`,
+    ].join('');
+  }
+
   /**
    * Registers an action to list shortcuts with the shortcut registry.
    */
@@ -304,7 +299,7 @@ Blockly.Css.register(`
   border: 1px solid var(--key-border-color);
   border-radius: 8px;
   display: inline-block;
-  margin: 0 8px;
+  margin: 0 4px;
   min-width: 2em;
   padding: .3em .5em;
   text-align: center;

src/shortcut_formatting.ts

@@ -0,0 +1,99 @@
+import {ShortcutRegistry} from 'blockly';
+import {keyNames} from './keynames';
+
+const isMacPlatform = navigator.platform.startsWith('Mac');
+
+/**
+ * Find the primary shortcut for this platform and return it as single string
+ * in a short user facing format.
+ *
+ * @param action The action name, e.g. "cut".
+ * @returns The formatted shortcut.
+ */
+export function getShortActionShortcut(action: string): string {
+  const parts = getActionShortcutsAsKeys(action, shortModifierNames)[0];
+  return parts.join(isMacPlatform ? ' ' : ' + ');
+}
+
+/**
+ * Find the relevant shortcuts for the given action for the current platform.
+ * Keys are returned in a long user facing format.
+ *
+ * @param action The action name, e.g. "cut".
+ * @returns The formatted shortcuts as individual keys.
+ */
+export function getLongActionShortcutsAsKeys(action: string): string[][] {
+  return getActionShortcutsAsKeys(action, longModifierNames);
+}
+
+const longModifierNames: Record<string, string> = {
+  'Control': 'Ctrl',
+  'Meta': '⌘ Command',
+  'Alt': isMacPlatform ? '⌥ Option' : 'Alt',
+};
+
+const shortModifierNames: Record<string, string> = {
+  'Control': 'Ctrl',
+  'Meta': '⌘',
+  'Alt': isMacPlatform ? '⌥' : 'Alt',
+};
+
+/**
+ * Find the relevant shortcuts for the given action for the current platform.
+ * Keys are returned in a user facing format.
+ *
+ * This could be considerably simpler if we only bound shortcuts relevant to the
+ * current platform or tagged them with a platform.
+ *
+ * @param action The action name, e.g. "cut".
+ * @param modifierNames The names to use for the Meta/Control/Alt modifiers.
+ * @returns The formatted shortcuts.
+ */
+function getActionShortcutsAsKeys(
+  action: string,
+  modifierNames: Record<string, string>,
+): string[][] {
+  const shortcuts = ShortcutRegistry.registry.getKeyCodesByShortcutName(action);
+  // See ShortcutRegistry.createSerializedKey for the starting format.
+  const named = shortcuts.map((shortcut) => {
+    return shortcut
+      .split('+')
+      .map((maybeNumeric) => keyNames[maybeNumeric] ?? maybeNumeric)
+      .map((k) => upperCaseFirst(modifierNames[k] ?? k));
+  });
+
+  const command = modifierNames['Meta'];
+  const option = modifierNames['Alt'];
+  const control = modifierNames['Control'];
+  // Needed to prefer Command to Option where we've bound Alt.
+  named.sort((a, b) => {
+    const aValue = a.includes(command) ? 1 : 0;
+    const bValue = b.includes(command) ? 1 : 0;
+    return bValue - aValue;
+  });
+  let currentPlatform = named.filter((shortcut) => {
+    const isMacShortcut =
+      shortcut.includes(command) || shortcut.includes(option);
+    return isMacShortcut === isMacPlatform;
+  });
+  currentPlatform = currentPlatform.length === 0 ? named : currentPlatform;
+
+  // If there are modifiers return only one shortcut on the assumption they are
+  // intended for different platforms. Otherwise assume they are alternatives.
+  const hasModifiers = currentPlatform.some((shortcut) =>
+    shortcut.some(
+      (key) => command === key || option === key || control === key,
+    ),
+  );
+  return hasModifiers ? [currentPlatform[0]] : currentPlatform;
+}
+
+/**
+ * Convert the first character to uppercase.
+ *
+ * @param str String.
+ * @returns The string in title case.
+ */
+export function upperCaseFirst(str: string) {
+  return str.charAt(0).toUpperCase() + str.substring(1);
+}