-
Notifications
You must be signed in to change notification settings - Fork 74
[Fix] Stop global shortcuts from hijacking Ctrl/Cmd+P #545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||||||||||||||||||
| import type { PanelShortcutLeft, PanelShortcutRight } from '../stores/ui-store.js'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export type GlobalShortcutAction = | ||||||||||||||||||||||
| | 'new-task' | ||||||||||||||||||||||
| | 'open-files' | ||||||||||||||||||||||
| | 'toggle-command-palette' | ||||||||||||||||||||||
| | 'toggle-left-nav' | ||||||||||||||||||||||
| | 'toggle-right-panel'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export interface GlobalShortcutConfig { | ||||||||||||||||||||||
| leftNavShortcut: PanelShortcutLeft; | ||||||||||||||||||||||
| rightPanelShortcut: PanelShortcutRight; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const TEXT_INPUT_SELECTOR = | ||||||||||||||||||||||
| 'input:not([type=checkbox]):not([type=radio]):not([type=button]):not([type=submit]):not([type=reset]), textarea, select, [contenteditable=""], [contenteditable="true"]'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type SelectorCapableElement = EventTarget & { | ||||||||||||||||||||||
| matches: (selector: string) => boolean; | ||||||||||||||||||||||
| closest: (selector: string) => Element | null; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function isElementWithSelectorSupport(target: EventTarget | null): target is SelectorCapableElement { | ||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| typeof target === 'object' && | ||||||||||||||||||||||
| target !== null && | ||||||||||||||||||||||
| 'matches' in target && | ||||||||||||||||||||||
| typeof (target as { matches?: unknown }).matches === 'function' && | ||||||||||||||||||||||
| 'closest' in target && | ||||||||||||||||||||||
| typeof (target as { closest?: unknown }).closest === 'function' | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function isTextInputElement(target: EventTarget | null): boolean { | ||||||||||||||||||||||
| if (!isElementWithSelectorSupport(target)) return false; | ||||||||||||||||||||||
| if (target.matches(TEXT_INPUT_SELECTOR)) return true; | ||||||||||||||||||||||
| return Boolean(target.closest('[contenteditable="true"], [contenteditable=""]')); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+34
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fully support
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function shouldDeferGlobalShortcut( | ||||||||||||||||||||||
| event: Pick<KeyboardEvent, 'metaKey' | 'ctrlKey' | 'shiftKey' | 'code' | 'target'>, | ||||||||||||||||||||||
| ): boolean { | ||||||||||||||||||||||
| if (!isTextInputElement(event.target)) return false; | ||||||||||||||||||||||
| const meta = event.metaKey || event.ctrlKey; | ||||||||||||||||||||||
| if (meta && !event.shiftKey && event.code === 'KeyK') return false; | ||||||||||||||||||||||
| return true; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+40
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation of export function shouldDeferGlobalShortcut(
event: Pick<KeyboardEvent, 'metaKey' | 'ctrlKey' | 'shiftKey' | 'code' | 'target'>,
): boolean {
if (!isTextInputElement(event.target)) return false;
const meta = event.metaKey || event.ctrlKey;
if (meta) {
if (!event.shiftKey && event.code === 'KeyK') return false;
if (event.shiftKey && (event.code === 'KeyO' || event.code === 'KeyF')) return false;
}
return true;
} |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function resolveGlobalShortcutAction( | ||||||||||||||||||||||
| event: Pick<KeyboardEvent, 'metaKey' | 'ctrlKey' | 'shiftKey' | 'code'>, | ||||||||||||||||||||||
| config: GlobalShortcutConfig, | ||||||||||||||||||||||
| ): GlobalShortcutAction | null { | ||||||||||||||||||||||
| const meta = event.metaKey || event.ctrlKey; | ||||||||||||||||||||||
| if (!meta) return null; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (event.shiftKey && event.code === 'KeyO') return 'new-task'; | ||||||||||||||||||||||
| if (event.shiftKey && event.code === 'KeyF') return 'open-files'; | ||||||||||||||||||||||
| if (!event.shiftKey && event.code === 'KeyK') return 'toggle-command-palette'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (!event.shiftKey && config.leftNavShortcut !== 'None' && event.code === config.leftNavShortcut) { | ||||||||||||||||||||||
| return 'toggle-left-nav'; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (!event.shiftKey && config.rightPanelShortcut !== 'None' && event.code === config.rightPanelShortcut) { | ||||||||||||||||||||||
| return 'toggle-right-panel'; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return null; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function hasPreferencesShortcutConflict(leftNavShortcut: PanelShortcutLeft, platform: string): boolean { | ||||||||||||||||||||||
| return platform === 'darwin' && leftNavShortcut === 'Comma'; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { | ||
| hasPreferencesShortcutConflict, | ||
| isTextInputElement, | ||
| resolveGlobalShortcutAction, | ||
| shouldDeferGlobalShortcut, | ||
| } from '../src/lib/global-shortcuts.js'; | ||
|
|
||
| function keyEvent( | ||
| partial: Partial<KeyboardEvent> & Pick<KeyboardEvent, 'code'>, | ||
| target: EventTarget | null = null, | ||
| ): KeyboardEvent { | ||
| return { | ||
| metaKey: false, | ||
| ctrlKey: false, | ||
| shiftKey: false, | ||
| target, | ||
| ...partial, | ||
| } as KeyboardEvent; | ||
| } | ||
|
|
||
| function mockTextInput(): EventTarget { | ||
| return { | ||
| matches: () => true, | ||
| closest: () => null, | ||
| } as unknown as EventTarget; | ||
| } | ||
|
|
||
| function mockNonInput(): EventTarget { | ||
| return { | ||
| matches: () => false, | ||
| closest: () => null, | ||
| } as unknown as EventTarget; | ||
| } | ||
|
|
||
| const defaultConfig = { | ||
| leftNavShortcut: 'Comma' as const, | ||
| rightPanelShortcut: 'Period' as const, | ||
| }; | ||
|
|
||
| describe('resolveGlobalShortcutAction', () => { | ||
| it('does not bind Ctrl/Cmd+P', () => { | ||
| expect(resolveGlobalShortcutAction(keyEvent({ ctrlKey: true, code: 'KeyP' }), defaultConfig)).toBeNull(); | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'KeyP' }), defaultConfig)).toBeNull(); | ||
| }); | ||
|
|
||
| it('resolves built-in shortcuts', () => { | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, shiftKey: true, code: 'KeyO' }), defaultConfig)).toBe( | ||
| 'new-task', | ||
| ); | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, shiftKey: true, code: 'KeyF' }), defaultConfig)).toBe( | ||
| 'open-files', | ||
| ); | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'KeyK' }), defaultConfig)).toBe( | ||
| 'toggle-command-palette', | ||
| ); | ||
| }); | ||
|
|
||
| it('respects configurable panel shortcuts', () => { | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'Comma' }), defaultConfig)).toBe( | ||
| 'toggle-left-nav', | ||
| ); | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'Period' }), defaultConfig)).toBe( | ||
| 'toggle-right-panel', | ||
| ); | ||
| }); | ||
|
|
||
| it('skips panel shortcuts when set to None', () => { | ||
| const disabled = { leftNavShortcut: 'None' as const, rightPanelShortcut: 'None' as const }; | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'Comma' }), disabled)).toBeNull(); | ||
| expect(resolveGlobalShortcutAction(keyEvent({ metaKey: true, code: 'Period' }), disabled)).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('shouldDeferGlobalShortcut', () => { | ||
| it('defers panel shortcuts while typing in a text field', () => { | ||
| expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, code: 'Comma' }, mockTextInput()))).toBe(true); | ||
| }); | ||
|
|
||
| it('still allows Cmd/Ctrl+K inside text fields', () => { | ||
| expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, code: 'KeyK' }, mockTextInput()))).toBe(false); | ||
| }); | ||
|
|
||
| it('does not defer shortcuts outside text inputs', () => { | ||
| expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, code: 'Comma' }, mockNonInput()))).toBe(false); | ||
| }); | ||
|
|
||
| it('defers Ctrl/Cmd+P inside text fields so print can pass through', () => { | ||
| expect(shouldDeferGlobalShortcut(keyEvent({ ctrlKey: true, code: 'KeyP' }, mockTextInput()))).toBe(true); | ||
| }); | ||
|
Comment on lines
+80
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add tests to verify that standard application shortcuts like it('still allows Cmd/Ctrl+K inside text fields', () => {
expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, code: 'KeyK' }, mockTextInput()))).toBe(false);
});
it('still allows Cmd/Ctrl+Shift+O and Cmd/Ctrl+Shift+F inside text fields', () => {
expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, shiftKey: true, code: 'KeyO' }, mockTextInput()))).toBe(false);
expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, shiftKey: true, code: 'KeyF' }, mockTextInput()))).toBe(false);
});
it('does not defer shortcuts outside text inputs', () => {
expect(shouldDeferGlobalShortcut(keyEvent({ metaKey: true, code: 'Comma' }, mockNonInput()))).toBe(false);
});
it('defers Ctrl/Cmd+P inside text fields so print can pass through', () => {
expect(shouldDeferGlobalShortcut(keyEvent({ ctrlKey: true, code: 'KeyP' }, mockTextInput()))).toBe(true);
}); |
||
| }); | ||
|
|
||
| describe('isTextInputElement', () => { | ||
| it('detects editable targets', () => { | ||
| expect(isTextInputElement(mockTextInput())).toBe(true); | ||
| expect(isTextInputElement(mockNonInput())).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hasPreferencesShortcutConflict', () => { | ||
| it('flags Cmd+, on macOS', () => { | ||
| expect(hasPreferencesShortcutConflict('Comma', 'darwin')).toBe(true); | ||
| expect(hasPreferencesShortcutConflict('BracketLeft', 'darwin')).toBe(false); | ||
| expect(hasPreferencesShortcutConflict('Comma', 'win32')).toBe(false); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
TEXT_INPUT_SELECTORonly matchescontenteditable="true"andcontenteditable="". However, modern rich text editors and chat inputs frequently usecontenteditable="plaintext-only"to allow auto-resizing while preventing formatted rich-text pasting. We should update the selector to match any element with acontenteditableattribute that is not explicitly set to"false".