From acbe92d717fc23eaf7d8f85f972234693bbafa48 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Thu, 22 Aug 2024 19:13:27 -0400 Subject: [PATCH] Fix keyboard shortcuts to use Control on Windows and Linux (#3620) * Fix keyboard shortcuts to use Control instead of Meta on Windows and Linux * Convert more tests to use Playwright built-in --- e2e/playwright/command-bar-tests.spec.ts | 6 ++-- e2e/playwright/copilot-ghost-test.spec.ts | 23 ++++++-------- e2e/playwright/editor-tests.spec.ts | 13 +++----- e2e/playwright/projects.spec.ts | 13 +++----- e2e/playwright/test-utils.ts | 11 ++----- e2e/playwright/testing-settings.spec.ts | 4 +-- e2e/playwright/text-to-cad-tests.spec.ts | 22 ++++++-------- e2e/playwright/various.spec.ts | 19 +++--------- src/Router.tsx | 2 +- src/components/FileTree.tsx | 4 +-- src/hooks/usePlatform.ts | 37 ++--------------------- src/lib/hotkeyWrapper.ts | 2 +- src/lib/settings/initialKeybindings.ts | 12 ++++++-- src/lib/utils.ts | 36 ++++++++++++++++++++++ 14 files changed, 92 insertions(+), 112 deletions(-) diff --git a/e2e/playwright/command-bar-tests.spec.ts b/e2e/playwright/command-bar-tests.spec.ts index 55b5a5eeef..3d4a6ca7ea 100644 --- a/e2e/playwright/command-bar-tests.spec.ts +++ b/e2e/playwright/command-bar-tests.spec.ts @@ -124,7 +124,7 @@ const extrude001 = extrude(-10, sketch001)` await expect(cmdSearchBar).not.toBeVisible() // Now try the same, but with the keyboard shortcut, check focus - await page.keyboard.press('Meta+K') + await page.keyboard.press('ControlOrMeta+K') await expect(cmdSearchBar).toBeVisible() await expect(cmdSearchBar).toBeFocused() @@ -185,7 +185,7 @@ const extrude001 = extrude(-10, sketch001)` await page.locator('.cm-content').click() // Now try the same, but with the keyboard shortcut, check focus - await page.keyboard.press('Meta+K') + await page.keyboard.press('ControlOrMeta+K') let cmdSearchBar = page.getByPlaceholder('Search commands') await expect(cmdSearchBar).toBeVisible() @@ -250,7 +250,7 @@ const extrude001 = extrude(-10, sketch001)` await page.getByRole('button', { name: 'Extrude' }).isEnabled() let cmdSearchBar = page.getByPlaceholder('Search commands') - await page.keyboard.press('Meta+K') + await page.keyboard.press('ControlOrMeta+K') await expect(cmdSearchBar).toBeVisible() // Search for extrude command and choose it diff --git a/e2e/playwright/copilot-ghost-test.spec.ts b/e2e/playwright/copilot-ghost-test.spec.ts index 8520dfaa1f..2ba2ac4079 100644 --- a/e2e/playwright/copilot-ghost-test.spec.ts +++ b/e2e/playwright/copilot-ghost-test.spec.ts @@ -332,7 +332,6 @@ test.describe('Copilot ghost text', () => { await page.setViewportSize({ width: 1200, height: 500 }) await u.waitForAuthSkipAppStart() - const CtrlKey = process.platform === 'darwin' ? 'Meta' : 'Control' await u.codeLocator.click() await expect(page.locator('.cm-content')).toHaveText(``) @@ -349,10 +348,10 @@ test.describe('Copilot ghost text', () => { ) // Going elsewhere in the code should hide the ghost text. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.down('Shift') await page.keyboard.press('KeyZ') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.up('Shift') await expect(page.locator('.cm-ghostText').first()).not.toBeVisible() @@ -368,8 +367,6 @@ test.describe('Copilot ghost text', () => { await u.waitForAuthSkipAppStart() - const CtrlKey = process.platform === 'darwin' ? 'Meta' : 'Control' - await page.waitForTimeout(800) await u.codeLocator.click() await expect(page.locator('.cm-content')).toHaveText(``) @@ -382,17 +379,17 @@ test.describe('Copilot ghost text', () => { await page.waitForTimeout(800) // Ctrl+z - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyZ') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await expect(page.locator('.cm-content')).toHaveText(``) // Ctrl+shift+z - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.down('Shift') await page.keyboard.press('KeyZ') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.up('Shift') await expect(page.locator('.cm-content')).toHaveText(`{thing: "blah"}`) @@ -411,14 +408,14 @@ test.describe('Copilot ghost text', () => { ) // Once for the enter. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyZ') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') // Once for the text. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyZ') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await expect(page.locator('.cm-ghostText').first()).not.toBeVisible() diff --git a/e2e/playwright/editor-tests.spec.ts b/e2e/playwright/editor-tests.spec.ts index fda4cc36da..80c5cbb7f2 100644 --- a/e2e/playwright/editor-tests.spec.ts +++ b/e2e/playwright/editor-tests.spec.ts @@ -16,7 +16,6 @@ test.describe('Editor tests', () => { await page.setViewportSize({ width: 1000, height: 500 }) await u.waitForAuthSkipAppStart() - const CtrlKey = process.platform === 'darwin' ? 'Meta' : 'Control' // check no error to begin with await expect(page.locator('.cm-lint-marker-error')).not.toBeVisible() @@ -29,9 +28,9 @@ test.describe('Editor tests', () => { |> line([-20, 0], %) |> close(%)`) - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('/') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await expect(page.locator('.cm-content')) .toHaveText(`const sketch001 = startSketchOn('XY') @@ -42,9 +41,9 @@ test.describe('Editor tests', () => { // |> close(%)`) // uncomment the code - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('/') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await expect(page.locator('.cm-content')) .toHaveText(`const sketch001 = startSketchOn('XY') @@ -148,9 +147,7 @@ test.describe('Editor tests', () => { // Delete all the code. await page.locator('.cm-content').click() // Select all - await page.keyboard.press('Control+A') - await page.keyboard.press('Backspace') - await page.keyboard.press('Meta+A') + await page.keyboard.press('ControlOrMeta+A') await page.keyboard.press('Backspace') await expect(page.locator('.cm-content')).toHaveText(``) diff --git a/e2e/playwright/projects.spec.ts b/e2e/playwright/projects.spec.ts index 7cb5bd5a9e..1fcc868678 100644 --- a/e2e/playwright/projects.spec.ts +++ b/e2e/playwright/projects.spec.ts @@ -1233,18 +1233,13 @@ test( await page.getByText('mike_stress_test').click() - const modifier = - process.platform === 'win32' || process.platform === 'linux' - ? 'Control' - : 'Meta' - await test.step('select all in code editor, check its length', async () => { await u.codeLocator.click() // expect u.codeLocator to have some text await expect(u.codeLocator).toContainText('line(') - await page.keyboard.down(modifier) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyA') - await page.keyboard.up(modifier) + await page.keyboard.up('ControlOrMeta') // check the length of the selected text const selectedText = await page.evaluate(() => { @@ -1260,9 +1255,9 @@ test( await test.step('delete all the text, select again and verify there are no characters left', async () => { await page.keyboard.press('Backspace') - await page.keyboard.down(modifier) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyA') - await page.keyboard.up(modifier) + await page.keyboard.up('ControlOrMeta') // check the length of the selected text const selectedText = await page.evaluate(() => { diff --git a/e2e/playwright/test-utils.ts b/e2e/playwright/test-utils.ts index 7e8e8778e0..39617874e7 100644 --- a/e2e/playwright/test-utils.ts +++ b/e2e/playwright/test-utils.ts @@ -9,7 +9,6 @@ import { test, } from '@playwright/test' import { EngineCommand } from 'lang/std/artifactGraph' -import os from 'os' import fsp from 'fs/promises' import fsSync from 'fs' import { join } from 'path' @@ -78,11 +77,10 @@ async function waitForPageLoad(page: Page) { } async function removeCurrentCode(page: Page) { - const hotkey = process.platform === 'darwin' ? 'Meta' : 'Control' await page.locator('.cm-content').click() - await page.keyboard.down(hotkey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('a') - await page.keyboard.up(hotkey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.press('Backspace') await expect(page.locator('.cm-content')).toHaveText('') } @@ -745,11 +743,6 @@ export const doExport = async ( } } -/** - * Gets the appropriate modifier key for the platform. - */ -export const metaModifier = os.platform() === 'darwin' ? 'Meta' : 'Control' - export async function tearDown(page: Page, testInfo: TestInfo) { if (testInfo.status === 'skipped') return if (testInfo.status === 'failed') return diff --git a/e2e/playwright/testing-settings.spec.ts b/e2e/playwright/testing-settings.spec.ts index 57669ac276..bca8a4e978 100644 --- a/e2e/playwright/testing-settings.spec.ts +++ b/e2e/playwright/testing-settings.spec.ts @@ -72,7 +72,7 @@ test.describe('Testing settings', () => { const inputLocator = page.locator('input[name="modeling-showDebugPanel"]') // Open the settings modal with the browser keyboard shortcut - await page.keyboard.press('Meta+Shift+,') + await page.keyboard.press('ControlOrMeta+Shift+,') await expect(headingLocator).toBeVisible() await page.locator('#showDebugPanel').getByText('OffOn').click() @@ -82,7 +82,7 @@ test.describe('Testing settings', () => { await test.step('Open settings with keyboard shortcut', async () => { await page.getByTestId('settings-close-button').click() await page.locator('.cm-content').click() - await page.keyboard.press('Meta+Shift+,') + await page.keyboard.press('ControlOrMeta+Shift+,') await expect(headingLocator).toBeVisible() }) diff --git a/e2e/playwright/text-to-cad-tests.spec.ts b/e2e/playwright/text-to-cad-tests.spec.ts index 640acc6241..162d8bf052 100644 --- a/e2e/playwright/text-to-cad-tests.spec.ts +++ b/e2e/playwright/text-to-cad-tests.spec.ts @@ -9,8 +9,6 @@ test.afterEach(async ({ page }, testInfo) => { await tearDown(page, testInfo) }) -const CtrlKey = process.platform === 'darwin' ? 'Meta' : 'Control' - test.describe('Text-to-CAD tests', () => { test('basic lego happy case', async ({ page }) => { const u = await getUtils(page) @@ -298,9 +296,9 @@ test.describe('Text-to-CAD tests', () => { await expect(page.locator('textarea')).toContainText(badPrompt) // Select all and start a new prompt. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyA') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.type('a 2x4 lego') // Submit the new prompt. @@ -520,9 +518,9 @@ test.describe('Text-to-CAD tests', () => { await page.locator('.cm-content').click({ position: { x: 10, y: 10 } }) // Paste the code. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyV') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') // Expect the code to be pasted. await expect(page.locator('.cm-content')).toContainText(`2x8`) @@ -549,13 +547,13 @@ test.describe('Text-to-CAD tests', () => { await page.locator('.cm-content').click({ position: { x: 10, y: 10 } }) // Paste the code. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyA') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.press('Backspace') - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyV') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') // Expect the code to be pasted. await expect(page.locator('.cm-content')).toContainText(`2x4`) @@ -636,9 +634,9 @@ test.describe('Text-to-CAD tests', () => { await page.locator('.cm-content').click({ position: { x: 10, y: 10 } }) // Paste the code. - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('KeyV') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') // Expect the code to be pasted. await expect(page.locator('.cm-content')).toContainText(`2x4`) diff --git a/e2e/playwright/various.spec.ts b/e2e/playwright/various.spec.ts index 8ef9abf0b5..fb947eca6a 100644 --- a/e2e/playwright/various.spec.ts +++ b/e2e/playwright/various.spec.ts @@ -1,13 +1,6 @@ import { test, expect } from '@playwright/test' -import { - doExport, - getUtils, - makeTemplate, - metaModifier, - setup, - tearDown, -} from './test-utils' +import { doExport, getUtils, makeTemplate, setup, tearDown } from './test-utils' test.beforeEach(async ({ context, page }) => { await setup(context, page) @@ -17,8 +10,6 @@ test.afterEach(async ({ page }, testInfo) => { await tearDown(page, testInfo) }) -const CtrlKey = process.platform === 'darwin' ? 'Meta' : 'Control' - test('Units menu', async ({ page }) => { const u = await getUtils(page) await page.setViewportSize({ width: 1200, height: 500 }) @@ -157,7 +148,7 @@ test('Paste should not work unless an input is focused', async ({ // Paste without the code pane focused await codeEditorText.blur() - await page.keyboard.press(`${metaModifier}+KeyV`) + await page.keyboard.press('ControlOrMeta+KeyV') // Show that the paste didn't work but typing did await expect(codeEditorText).not.toContainText(pasteContent) @@ -166,7 +157,7 @@ test('Paste should not work unless an input is focused', async ({ // Paste with the code editor focused // Following this guidance: https://github.com/microsoft/playwright/issues/8114 await codeEditorText.focus() - await page.keyboard.press(`${metaModifier}+KeyV`) + await page.keyboard.press('ControlOrMeta+KeyV') await expect( await page.evaluate( () => document.querySelector('.cm-content')?.textContent @@ -380,9 +371,9 @@ test('Basic default modeling and sketch hotkeys work', async ({ page }) => { await test.step(`Type code with sketch hotkeys, shouldn't fire`, async () => { // Since there's code now, we have to get to the end of the line await page.locator('.cm-line').last().click() - await page.keyboard.down(CtrlKey) + await page.keyboard.down('ControlOrMeta') await page.keyboard.press('ArrowRight') - await page.keyboard.up(CtrlKey) + await page.keyboard.up('ControlOrMeta') await page.keyboard.press('Enter') await page.keyboard.type('//') diff --git a/src/Router.tsx b/src/Router.tsx index 6ffde25343..a7a80af973 100644 --- a/src/Router.tsx +++ b/src/Router.tsx @@ -190,7 +190,7 @@ function CoreDump() { () => new CoreDumpManager(engineCommandManager, codeManager, token), [] ) - useHotkeyWrapper(['meta + shift + .'], () => { + useHotkeyWrapper(['mod + shift + .'], () => { toast.promise( coreDump(coreDumpManager, true), { diff --git a/src/components/FileTree.tsx b/src/components/FileTree.tsx index cc1b2c32d4..13ec6ca757 100644 --- a/src/components/FileTree.tsx +++ b/src/components/FileTree.tsx @@ -396,8 +396,8 @@ export const FileTreeMenu = () => { }) } - useHotkeyWrapper(['meta + n'], createFile) - useHotkeyWrapper(['meta + shift + n'], createFolder) + useHotkeyWrapper(['mod + n'], createFile) + useHotkeyWrapper(['mod + shift + n'], createFolder) return ( <> diff --git a/src/hooks/usePlatform.ts b/src/hooks/usePlatform.ts index cd751f013a..d74944745f 100644 --- a/src/hooks/usePlatform.ts +++ b/src/hooks/usePlatform.ts @@ -1,44 +1,11 @@ -import { isDesktop } from 'lib/isDesktop' +import { Platform, platform } from 'lib/utils' import { useEffect, useState } from 'react' -export type Platform = 'macos' | 'windows' | 'linux' | '' - export default function usePlatform() { const [platformName, setPlatformName] = useState('') useEffect(() => { - function getPlatform(): Platform { - const platform = window.electron.platform ?? '' - // https://nodejs.org/api/process.html#processplatform - switch (platform) { - case 'darwin': - return 'macos' - case 'win32': - return 'windows' - // We don't currently care to distinguish between these. - case 'android': - case 'freebsd': - case 'linux': - case 'openbsd': - case 'sunos': - return 'linux' - default: - console.error('Unknown platform:', platform) - return '' - } - } - - if (isDesktop()) { - setPlatformName(getPlatform()) - } else { - if (navigator.userAgent.indexOf('Mac') !== -1) { - setPlatformName('macos') - } else if (navigator.userAgent.indexOf('Win') !== -1) { - setPlatformName('windows') - } else if (navigator.userAgent.indexOf('Linux') !== -1) { - setPlatformName('linux') - } - } + setPlatformName(platform()) }, [setPlatformName]) return platformName diff --git a/src/lib/hotkeyWrapper.ts b/src/lib/hotkeyWrapper.ts index 11c10a637e..13b4c9ede2 100644 --- a/src/lib/hotkeyWrapper.ts +++ b/src/lib/hotkeyWrapper.ts @@ -31,7 +31,7 @@ function mapHotkeyToCodeMirrorHotkey(hotkey: string): string { return hotkey .replaceAll('+', '-') .replaceAll(' ', '') - .replaceAll('mod', 'Meta') + .replaceAll('mod', 'Mod') .replaceAll('meta', 'Meta') .replaceAll('ctrl', 'Ctrl') .replaceAll('shift', 'Shift') diff --git a/src/lib/settings/initialKeybindings.ts b/src/lib/settings/initialKeybindings.ts index 9c51bfdc30..dda9c1a8eb 100644 --- a/src/lib/settings/initialKeybindings.ts +++ b/src/lib/settings/initialKeybindings.ts @@ -1,4 +1,5 @@ import { isDesktop } from 'lib/isDesktop' +import { platform } from 'lib/utils' export type InteractionMapItem = { name: string @@ -24,6 +25,11 @@ export const interactionMapCategories = [ type InteractionMapCategory = (typeof interactionMapCategories)[number] +/** + * Primary modifier key for the current platform. + */ +const PRIMARY = platform() === 'macos' ? 'Command' : 'Control' + /** * A temporary implementation of the interaction map for * display purposes only. @@ -38,7 +44,7 @@ export const interactionMap: Record< Settings: [ { name: 'toggle-settings', - sequence: isDesktop() ? 'Meta+,' : 'Shift+Meta+,', + sequence: isDesktop() ? `${PRIMARY}+,` : `Shift+${PRIMARY}+,`, title: 'Toggle Settings', description: 'Opens the settings dialog. Always available.', }, @@ -53,7 +59,7 @@ export const interactionMap: Record< 'Command Palette': [ { name: 'toggle-command-palette', - sequence: 'Meta+K', + sequence: `${PRIMARY}+K`, title: 'Toggle Command Palette', description: 'Always available. Use Ctrl+/ on Windows/Linux.', }, @@ -159,7 +165,7 @@ export const interactionMap: Record< }, { name: 'delete-file', - sequence: 'Meta+Backspace', + sequence: `${PRIMARY}+Backspace`, title: 'Delete File/Folder', description: 'Available when a file or folder is selected in the file tree.', diff --git a/src/lib/utils.ts b/src/lib/utils.ts index b9085d6d6b..454686ba82 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -1,6 +1,7 @@ import { SourceRange } from '../lang/wasm' import { v4 } from 'uuid' +import { isDesktop } from './isDesktop' export const uuidv4 = v4 @@ -126,6 +127,41 @@ export function getNormalisedCoordinates({ } } +// TODO: Remove the empty platform type. +export type Platform = 'macos' | 'windows' | 'linux' | '' + +export function platform(): Platform { + if (isDesktop()) { + const platform = window.electron.platform ?? '' + // https://nodejs.org/api/process.html#processplatform + switch (platform) { + case 'darwin': + return 'macos' + case 'win32': + return 'windows' + // We don't currently care to distinguish between these. + case 'android': + case 'freebsd': + case 'linux': + case 'openbsd': + case 'sunos': + return 'linux' + default: + console.error('Unknown platform:', platform) + return '' + } + } + if (navigator.userAgent.indexOf('Mac') !== -1) { + return 'macos' + } else if (navigator.userAgent.indexOf('Win') !== -1) { + return 'windows' + } else if (navigator.userAgent.indexOf('Linux') !== -1) { + return 'linux' + } + console.error('Unknown platform userAgent:', navigator.userAgent) + return '' +} + export function isReducedMotion(): boolean { return ( typeof window !== 'undefined' &&