Fix Commands button to show correct shortcut on Windows and Linux (#3625)

* Fix Commands button to show correct shortcut

* Fix onboarding to use the same shortcut reference

* Rename test file to be more general

* Add test for commands button text

* Remove outdated reference to Ctrl+/

* Change shortcut separator to be + and no spaces

* Add JSDocs and improve comments

* Add unit tests

* Change control modifier to regular ASCII caret

* Add browser test and fix platform detection

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest)

* Add useful debug info to the error message

* Fix to display metaKey as Super on Linux

* Revert "A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest)"

This reverts commit f8da90d5d2.

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest)

* Approve snapshots

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jess Frazelle <jessfraz@users.noreply.github.com>

e2e/playwright/app-header-tests.spec.ts

@@ -6,7 +6,39 @@ test.afterEach(async ({ page }, testInfo) => {
   await tearDown(page, testInfo)
 })
 
-test.describe('Electron user sidebar menu tests', () => {
+test.describe('Electron app header tests', () => {
+  test(
+    'Open Command Palette button has correct shortcut',
+    { tag: '@electron' },
+    async ({ browserName }, testInfo) => {
+      const { electronApp, page } = await setupElectron({
+        testInfo,
+        folderSetupFn: async () => {},
+      })
+
+      await page.setViewportSize({ width: 1200, height: 500 })
+
+      // No space before the shortcut since it checks textContent.
+      let text
+      switch (process.platform) {
+        case 'darwin':
+          text = 'Commands⌘K'
+          break
+        case 'win32':
+          text = 'CommandsCtrl+K'
+          break
+        default: // 'linux' etc.
+          text = 'CommandsCtrl+K'
+          break
+      }
+      const commandsButton = page.getByRole('button', { name: 'Commands' })
+      await expect(commandsButton).toBeVisible()
+      await expect(commandsButton).toHaveText(text)
+
+      await electronApp.close()
+    }
+  )
+
   test(
     'User settings has correct shortcut',
     { tag: '@electron' },

e2e/playwright/testing-settings.spec.ts

@@ -122,6 +122,36 @@ test.describe('Testing settings', () => {
     ).not.toBeChecked()
   })
 
+  test('Keybindings display the correct hotkey for Command Palette', async ({
+    page,
+  }) => {
+    const u = await getUtils(page)
+    await page.setViewportSize({ width: 1200, height: 500 })
+    await u.waitForAuthSkipAppStart()
+
+    await test.step('Open keybindings settings', async () => {
+      // Open the settings modal with the browser keyboard shortcut
+      await page.keyboard.press('ControlOrMeta+Shift+,')
+
+      // Go to Keybindings tab.
+      const keybindingsTab = page.getByRole('radio', { name: 'Keybindings' })
+      await keybindingsTab.click()
+    })
+
+    // Go to the hotkey for Command Palette.
+    const commandPalette = page.getByText('Toggle Command Palette')
+    await commandPalette.scrollIntoViewIfNeeded()
+
+    // The heading is above it and should be in view now.
+    const commandPaletteHeading = page.getByRole('heading', {
+      name: 'Command Palette',
+    })
+    // The hotkey is in a kbd element next to the heading.
+    const hotkey = commandPaletteHeading.locator('+ div kbd')
+    const text = process.platform === 'darwin' ? 'Command+K' : 'Control+K'
+    await expect(hotkey).toHaveText(text)
+  })
+
   test('Project and user settings can be reset', async ({ page }) => {
     const u = await getUtils(page)
     await page.setViewportSize({ width: 1200, height: 500 })

src/components/CommandBar/CommandBar.tsx

@@ -9,6 +9,8 @@ import useHotkeyWrapper from 'lib/hotkeyWrapper'
 import { CustomIcon } from 'components/CustomIcon'
 import Tooltip from 'components/Tooltip'
 
+export const COMMAND_PALETTE_HOTKEY = 'mod+k'
+
 export const CommandBar = () => {
   const { pathname } = useLocation()
   const { commandBarState, commandBarSend } = useCommandsContext()
@@ -24,7 +26,7 @@ export const CommandBar = () => {
   }, [pathname])
 
   // Hook up keyboard shortcuts
-  useHotkeyWrapper(['mod+k'], () => {
+  useHotkeyWrapper([COMMAND_PALETTE_HOTKEY], () => {
     if (commandBarState.context.commands.length === 0) return
     if (commandBarState.matches('Closed')) {
       commandBarSend({ type: 'Open' })

src/components/CommandBarOpenButton.tsx

@@ -1,5 +1,7 @@
 import { useCommandsContext } from 'hooks/useCommandsContext'
 import usePlatform from 'hooks/usePlatform'
+import { hotkeyDisplay } from 'lib/hotkeyWrapper'
+import { COMMAND_PALETTE_HOTKEY } from './CommandBar/CommandBar'
 
 export function CommandBarOpenButton() {
   const { commandBarSend } = useCommandsContext()
@@ -12,7 +14,7 @@ export function CommandBarOpenButton() {
     >
       <span>Commands</span>
       <kbd className="bg-primary/10 dark:bg-chalkboard-80 dark:group-hover:bg-primary font-mono rounded-sm dark:text-inherit inline-block px-1 border-primary dark:border-chalkboard-90">
-        {platform === 'macos' ? '⌘K' : '^/'}
+        {hotkeyDisplay(COMMAND_PALETTE_HOTKEY, platform)}
       </kbd>
     </button>
   )

src/lib/hotkeyWrapper.test.ts

@@ -0,0 +1,35 @@
+import { hotkeyDisplay } from './hotkeyWrapper'
+
+describe('hotkeyDisplay', () => {
+  it('displays mod', async () => {
+    expect(hotkeyDisplay('mod+c', 'macos')).toEqual('⌘C')
+    expect(hotkeyDisplay('mod+c', 'windows')).toEqual('Ctrl+C')
+    expect(hotkeyDisplay('mod+c', 'linux')).toEqual('Ctrl+C')
+  })
+  it('displays shift', async () => {
+    expect(hotkeyDisplay('shift+c', 'macos')).toEqual('⬆C')
+    expect(hotkeyDisplay('shift+c', 'windows')).toEqual('Shift+C')
+    expect(hotkeyDisplay('shift+c', 'linux')).toEqual('Shift+C')
+  })
+  it('displays meta', async () => {
+    expect(hotkeyDisplay('meta+c', 'macos')).toEqual('⌘C')
+    expect(hotkeyDisplay('meta+c', 'windows')).toEqual('Win+C')
+    // That's correct.  What browsers call meta is actually super.
+    expect(hotkeyDisplay('meta+c', 'linux')).toEqual('Super+C')
+  })
+  it('displays alt', async () => {
+    expect(hotkeyDisplay('alt+c', 'macos')).toEqual('⌥C')
+    expect(hotkeyDisplay('alt+c', 'windows')).toEqual('Alt+C')
+    expect(hotkeyDisplay('alt+c', 'linux')).toEqual('Alt+C')
+  })
+  it('displays ctrl', async () => {
+    expect(hotkeyDisplay('ctrl+c', 'macos')).toEqual('^C')
+    expect(hotkeyDisplay('ctrl+c', 'windows')).toEqual('Ctrl+C')
+    expect(hotkeyDisplay('ctrl+c', 'linux')).toEqual('Ctrl+C')
+  })
+  it('displays multiple modifiers', async () => {
+    expect(hotkeyDisplay('shift+alt+ctrl+c', 'windows')).toEqual(
+      'Shift+Alt+Ctrl+C'
+    )
+  })
+})

src/lib/hotkeyWrapper.ts

@@ -1,9 +1,10 @@
 import { Options, useHotkeys } from 'react-hotkeys-hook'
 import { useEffect } from 'react'
 import { codeManager } from './singletons'
+import { Platform } from './utils'
 
 // Hotkey wrapper wraps hotkeys for the app (outside of the editor)
-// With hotkeys inside the editor.
+// with hotkeys inside the editor.
 // This way we can have hotkeys defined in one place and not have to worry about
 // conflicting hotkeys, or them only being implemented for the app but not
 // inside the editor.
@@ -37,3 +38,48 @@ function mapHotkeyToCodeMirrorHotkey(hotkey: string): string {
     .replaceAll('shift', 'Shift')
     .replaceAll('alt', 'Alt')
 }
+
+const LOWER_CASE_LETTER = /[a-z]/
+const WHITESPACE = /\s+/g
+
+/**
+ * Convert hotkey to display text.
+ *
+ * TODO: We should handle capitalized single letter hotkeys like K as Shift+K,
+ * but we don't.
+ */
+export function hotkeyDisplay(hotkey: string, platform: Platform): string {
+  const isMac = platform === 'macos'
+  const isWindows = platform === 'windows'
+  // Browsers call it metaKey, but that's a misnomer.
+  const meta = isWindows ? 'Win' : 'Super'
+  const outputSeparator = isMac ? '' : '+'
+  const display = hotkey
+    // Capitalize letters.  We want Ctrl+K, not Ctrl+k, since Shift should be
+    // shown as a separate modifier.
+    .split('+')
+    .map((word) => {
+      if (word.length === 1 && LOWER_CASE_LETTER.test(word)) {
+        return word.toUpperCase()
+      }
+      return word
+    })
+    .join(outputSeparator)
+    // Collapse multiple spaces into one.
+    .replaceAll(WHITESPACE, ' ')
+    .replaceAll('mod', isMac ? '⌘' : 'Ctrl')
+    .replaceAll('meta', isMac ? '⌘' : meta)
+    // This is technically the wrong arrow for control, but it's more visible
+    // and recognizable.  May want to change this in the future.
+    //
+    // The correct arrow is ⌃ "UP ARROWHEAD" Unicode: U+2303
+    .replaceAll('ctrl', isMac ? '^' : 'Ctrl')
+    // This is technically the wrong arrow for shift, but it's more visible and
+    // recognizable.  May want to change this in the future.
+    //
+    // The correct arrow is ⇧ "UPWARDS WHITE ARROW" Unicode: U+21E7
+    .replaceAll('shift', isMac ? '⬆' : 'Shift')
+    .replaceAll('alt', isMac ? '⌥' : 'Alt')
+
+  return display
+}

src/lib/settings/initialKeybindings.ts

@@ -61,7 +61,7 @@ export const interactionMap: Record<
       name: 'toggle-command-palette',
       sequence: `${PRIMARY}+K`,
       title: 'Toggle Command Palette',
-      description: 'Always available. Use Ctrl+/ on Windows/Linux.',
+      description: 'Always available.',
     },
   ],
   Panes: [

src/lib/utils.ts

@@ -151,6 +151,32 @@ export function platform(): Platform {
         return ''
     }
   }
+
+  // navigator.platform is deprecated, but many browsers still support it, and
+  // it's more accurate than userAgent and userAgentData in Playwright.
+  if (
+    navigator.platform?.indexOf('Mac') === 0 ||
+    navigator.platform === 'iPhone'
+  ) {
+    return 'macos'
+  }
+  if (navigator.platform === 'Win32') {
+    return 'windows'
+  }
+
+  // Chrome only, but more accurate than userAgent.
+  let userAgentDataPlatform: unknown
+  if (
+    'userAgentData' in navigator &&
+    navigator.userAgentData &&
+    typeof navigator.userAgentData === 'object' &&
+    'platform' in navigator.userAgentData
+  ) {
+    userAgentDataPlatform = navigator.userAgentData.platform
+    if (userAgentDataPlatform === 'macOS') return 'macos'
+    if (userAgentDataPlatform === 'Windows') return 'windows'
+  }
+
   if (navigator.userAgent.indexOf('Mac') !== -1) {
     return 'macos'
   } else if (navigator.userAgent.indexOf('Win') !== -1) {
@@ -158,7 +184,12 @@ export function platform(): Platform {
   } else if (navigator.userAgent.indexOf('Linux') !== -1) {
     return 'linux'
   }
-  console.error('Unknown platform userAgent:', navigator.userAgent)
+  console.error(
+    'Unknown platform userAgent:',
+    navigator.platform,
+    userAgentDataPlatform,
+    navigator.userAgent
+  )
   return ''
 }
 

src/routes/Onboarding/CmdK.tsx

@@ -2,6 +2,8 @@ import usePlatform from 'hooks/usePlatform'
 import { OnboardingButtons, kbdClasses, useDismiss, useNextClick } from '.'
 import { onboardingPaths } from 'routes/Onboarding/paths'
 import { useModelingContext } from 'hooks/useModelingContext'
+import { hotkeyDisplay } from 'lib/hotkeyWrapper'
+import { COMMAND_PALETTE_HOTKEY } from 'components/CommandBar/CommandBar'
 
 export default function CmdK() {
   const { context } = useModelingContext()
@@ -20,15 +22,9 @@ export default function CmdK() {
         <h2 className="text-2xl font-bold">Command Bar</h2>
         <p className="my-4">
           Press{' '}
-          {platformName === 'macos' ? (
+          <kbd className={kbdClasses}>
-            <>
+            {hotkeyDisplay(COMMAND_PALETTE_HOTKEY, platformName)}
-              <kbd className={kbdClasses}>⌘K</kbd>
+          </kbd>{' '}
-            </>
-          ) : (
-            <>
-              <kbd className={kbdClasses}>Ctrl + /</kbd>
-            </>
-          )}{' '}
           to open the command bar. Try changing your theme with it.
         </p>
         <p className="my-4">