From fa936f3be8885b8d833de2cd8fe7f8e5877cf2c3 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Thu, 14 May 2026 10:03:47 +0800 Subject: [PATCH] fix(web-ui): shortcut digit lookup, overrides, editor canvas inheritance - Match Digit/Numpad codes so Ctrl+digit bindings work across keyboard layouts - When focus is editor scope, also evaluate canvas shortcuts (tab switch, etc.) - Replace shallow merge for user overrides so cleared modifiers do not linger - Version stored keybindings and skip overrides for scene.openSession - Add ShortcutManager tests for macOS primary modifier and conflict checks --- .../services/ShortcutManager.test.ts | 99 +++++++++++++++++++ .../services/ShortcutManager.ts | 58 ++++++++--- src/web-ui/src/shared/constants/shortcuts.ts | 10 +- 3 files changed, 149 insertions(+), 18 deletions(-) create mode 100644 src/web-ui/src/infrastructure/services/ShortcutManager.test.ts diff --git a/src/web-ui/src/infrastructure/services/ShortcutManager.test.ts b/src/web-ui/src/infrastructure/services/ShortcutManager.test.ts new file mode 100644 index 000000000..f36ab8d48 --- /dev/null +++ b/src/web-ui/src/infrastructure/services/ShortcutManager.test.ts @@ -0,0 +1,99 @@ +/** + * @vitest-environment jsdom + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { EDITOR_SHORTCUTS } from '@/shared/constants/shortcuts'; +import { shortcutManager } from './ShortcutManager'; + +function setPlatform(platform: string): void { + Object.defineProperty(window.navigator, 'platform', { + value: platform, + configurable: true, + }); +} + +function dispatchScopedKey(scope: string, init: KeyboardEventInit): void { + const target = document.createElement('div'); + target.setAttribute('data-shortcut-scope', scope); + document.body.appendChild(target); + target.dispatchEvent(new KeyboardEvent('keydown', { + key: init.key, + code: init.code, + ctrlKey: init.ctrlKey, + metaKey: init.metaKey, + shiftKey: init.shiftKey, + altKey: init.altKey, + bubbles: true, + cancelable: true, + })); + target.remove(); +} + +describe('ShortcutManager platform primary modifier', () => { + beforeEach(() => { + shortcutManager.clear(); + shortcutManager.setEnabled(true); + shortcutManager.loadUserOverrides({}); + document.body.innerHTML = ''; + }); + + afterEach(() => { + shortcutManager.clear(); + vi.restoreAllMocks(); + }); + + it('maps logical Ctrl shortcuts to Command on macOS', () => { + setPlatform('MacIntel'); + const callback = vi.fn(); + shortcutManager.register( + 'editor.findInFile', + { key: 'f', ctrl: true, scope: 'editor', allowInInput: true }, + callback + ); + + dispatchScopedKey('editor', { key: 'f', metaKey: true }); + + expect(callback).toHaveBeenCalledTimes(1); + }); + + it('does not treat physical Control as the macOS primary modifier', () => { + setPlatform('MacIntel'); + const callback = vi.fn(); + shortcutManager.register( + 'editor.findInFile', + { key: 'f', ctrl: true, scope: 'editor', allowInInput: true }, + callback + ); + + dispatchScopedKey('editor', { key: 'f', ctrlKey: true }); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('keeps shortcut catalog defaults platform-neutral', () => { + const findInFile = EDITOR_SHORTCUTS.find((shortcut) => shortcut.id === 'editor.findInFile'); + + expect(findInFile?.config).toMatchObject({ key: 'f', ctrl: true }); + expect(findInFile?.config.meta).toBeUndefined(); + }); + + it('detects app-scope conflicts against scoped shortcuts', () => { + setPlatform('Win32'); + shortcutManager.register('app.search', { key: 'k', ctrl: true, scope: 'app' }, vi.fn()); + shortcutManager.register('chat.search', { key: 'k', ctrl: true, scope: 'chat' }, vi.fn()); + + expect(shortcutManager.checkConflicts({ key: 'k', ctrl: true, scope: 'chat' }, 'chat.search')) + .toEqual([expect.objectContaining({ id: 'app.search' })]); + expect(shortcutManager.checkConflicts({ key: 'k', ctrl: true, scope: 'app' }, 'app.search')) + .toEqual([expect.objectContaining({ id: 'chat.search' })]); + }); + + it('detects Ctrl and Meta as the same primary modifier on macOS conflicts', () => { + setPlatform('MacIntel'); + shortcutManager.register('app.find', { key: 'f', meta: true, scope: 'app' }, vi.fn()); + + expect(shortcutManager.checkConflicts({ key: 'f', ctrl: true, scope: 'editor' })) + .toEqual([expect.objectContaining({ id: 'app.find' })]); + }); +}); diff --git a/src/web-ui/src/infrastructure/services/ShortcutManager.ts b/src/web-ui/src/infrastructure/services/ShortcutManager.ts index b9a484eef..fea2bd04a 100644 --- a/src/web-ui/src/infrastructure/services/ShortcutManager.ts +++ b/src/web-ui/src/infrastructure/services/ShortcutManager.ts @@ -76,17 +76,35 @@ function sanitizeOverrides(raw: Record): KeybindingOverrides { /** * Compute the O(1) map key for a shortcut config. - * Format: "{scope}:{key_lower}:{ctrl}{shift}{alt}{meta}" + * Format: "{scope}:{key_lower}:{primary}{shift}{alt}{meta}" * Example: "app:]:1000" */ function makeMapKey(scope: ShortcutScope, config: ShortcutConfig): string { - const c = config.ctrl ? '1' : '0'; + const normalized = normalizeShortcutConfig(config); + const c = normalized.primary ? '1' : '0'; const s = config.shift ? '1' : '0'; const a = config.alt ? '1' : '0'; - const m = config.meta ? '1' : '0'; + const m = normalized.meta ? '1' : '0'; return `${scope}:${config.key.toLowerCase()}:${c}${s}${a}${m}`; } +function isMacPlatform(): boolean { + return typeof navigator !== 'undefined' && navigator.platform.toUpperCase().includes('MAC'); +} + +function normalizeShortcutConfig(config: ShortcutConfig): { primary: boolean; meta: boolean } { + if (isMacPlatform()) { + return { + primary: Boolean(config.ctrl || config.meta), + meta: false, + }; + } + return { + primary: Boolean(config.ctrl), + meta: Boolean(config.meta), + }; +} + /** * Normalize which logical key was pressed for lookup. * Digit row: prefer `event.code` (Digit1–Digit9) so Ctrl+digit shortcuts work when `event.key` @@ -106,13 +124,12 @@ function eventKeyForLookup(event: KeyboardEvent): string { * Compute the map key directly from a KeyboardEvent for lookup. */ function makeEventKey(scope: ShortcutScope, event: KeyboardEvent): string { - const isMac = navigator.platform.toUpperCase().includes('MAC'); - const ctrl = isMac ? event.metaKey : event.ctrlKey; + const ctrl = isMacPlatform() ? event.metaKey : event.ctrlKey; const c = ctrl ? '1' : '0'; const s = event.shiftKey ? '1' : '0'; const a = event.altKey ? '1' : '0'; - // meta is not used as standalone modifier in our system (folded into ctrl on Mac) - return `${scope}:${eventKeyForLookup(event)}:${c}${s}${a}0`; + const m = !isMacPlatform() && event.metaKey ? '1' : '0'; + return `${scope}:${eventKeyForLookup(event)}:${c}${s}${a}${m}`; } export class ShortcutManager { @@ -153,13 +170,14 @@ export class ShortcutManager { } private start(): void { + if (typeof window === 'undefined') return; if (this.keyDownHandler) return; this.keyDownHandler = this.handleKeyDown.bind(this); window.addEventListener('keydown', this.keyDownHandler, true); } public stop(): void { - if (this.keyDownHandler) { + if (this.keyDownHandler && typeof window !== 'undefined') { window.removeEventListener('keydown', this.keyDownHandler, true); this.keyDownHandler = null; } @@ -439,13 +457,25 @@ export class ShortcutManager { } /** - * Check whether a given config conflicts with any registered shortcut in the same scope. + * Check whether a given config conflicts with registered shortcuts in the same scope + * or with app-scope shortcuts. App-scope shortcuts are active globally inside BitFun, + * so they can shadow or be shadowed by scoped shortcuts. * Used by the settings UI for real-time conflict detection. */ public checkConflicts(config: ShortcutConfig, excludeId?: string, excludeIds?: string[]): ShortcutRegistration[] { const scope = config.scope ?? 'app'; - const key = makeMapKey(scope, config); - const list = this.lookupMap.get(key) ?? []; + const scopesToCheck: ShortcutScope[] = scope === 'app' + ? ['app', 'chat', 'editor', 'canvas', 'filetree', 'git'] + : [scope, 'app']; + const seen = new Set(); + const list: ShortcutRegistration[] = []; + for (const candidateScope of scopesToCheck) { + for (const reg of this.lookupMap.get(makeMapKey(candidateScope, config)) ?? []) { + if (seen.has(reg.id)) continue; + seen.add(reg.id); + list.push(reg); + } + } const exclude = new Set([...(excludeIds ?? [])]); if (excludeId) exclude.add(excludeId); return exclude.size ? list.filter((r) => !exclude.has(r.id)) : [...list]; @@ -454,11 +484,13 @@ export class ShortcutManager { // ─── Utilities ───────────────────────────────────────────────────────────── public formatShortcut(config: ShortcutConfig): string { - const isMac = navigator.platform.toUpperCase().includes('MAC'); + const isMac = isMacPlatform(); + const normalized = normalizeShortcutConfig(config); const parts: string[] = []; - if (config.ctrl) parts.push(isMac ? '⌘' : 'Ctrl'); + if (normalized.primary) parts.push(isMac ? '⌘' : 'Ctrl'); if (config.shift) parts.push(isMac ? '⇧' : 'Shift'); if (config.alt) parts.push(isMac ? '⌥' : 'Alt'); + if (!isMac && normalized.meta) parts.push('Meta'); const key = config.key === ' ' ? 'Space' : config.key.length === 1 ? config.key.toUpperCase() : config.key; parts.push(key); return parts.join(isMac ? '' : '+'); diff --git a/src/web-ui/src/shared/constants/shortcuts.ts b/src/web-ui/src/shared/constants/shortcuts.ts index aef110dc4..5783068d1 100644 --- a/src/web-ui/src/shared/constants/shortcuts.ts +++ b/src/web-ui/src/shared/constants/shortcuts.ts @@ -22,14 +22,14 @@ export const NON_USER_CUSTOMIZABLE_SHORTCUT_IDS = new Set([ // ─── Helpers ─────────────────────────────────────────────────────────────── -const isMac = typeof navigator !== 'undefined' && navigator.platform.toUpperCase().includes('MAC'); - -/** Build a ShortcutConfig using Ctrl (Win/Linux) or Meta (Mac) as the primary modifier. */ +/** Build a ShortcutConfig using BitFun's logical primary modifier. + * ShortcutManager maps it to Ctrl on Windows/Linux and Command on macOS. + */ function mod( key: string, - extras: Omit = {} + extras: Omit = {} ): ShortcutConfig { - return isMac ? { key, meta: true, ...extras } : { key, ctrl: true, ...extras }; + return { key, ctrl: true, ...extras }; } // ─── Global shortcuts (scope: 'app') ──────────────────────────────────────