-
Notifications
You must be signed in to change notification settings - Fork 563
fix: stop event propagation on keybinding match to prevent dropdown expansion #11886
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,115 @@ | ||
| import { createTestingPinia } from '@pinia/testing' | ||
| import { setActivePinia } from 'pinia' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { useKeybindingService } from '@/platform/keybindings/keybindingService' | ||
| import { useCommandStore } from '@/stores/commandStore' | ||
| import { useDialogStore } from '@/stores/dialogStore' | ||
|
|
||
| vi.mock('@/platform/settings/settingStore', () => ({ | ||
| useSettingStore: vi.fn(() => ({ | ||
| get: vi.fn(() => []) | ||
| })) | ||
| })) | ||
|
|
||
| vi.mock('@/stores/dialogStore', () => ({ | ||
| useDialogStore: vi.fn(() => ({ | ||
| dialogStack: [] | ||
| })) | ||
| })) | ||
|
|
||
| function createKeyboardEvent( | ||
| key: string, | ||
| options: { | ||
| target?: Element | ||
| ctrlKey?: boolean | ||
| metaKey?: boolean | ||
| shiftKey?: boolean | ||
| altKey?: boolean | ||
| } = {} | ||
| ): KeyboardEvent { | ||
| const { target = document.body, ...modifiers } = options | ||
| const event = new KeyboardEvent('keydown', { | ||
| key, | ||
| code: key === 'Enter' ? 'Enter' : key, | ||
| bubbles: true, | ||
| cancelable: true, | ||
| ...modifiers | ||
| }) | ||
| event.preventDefault = vi.fn() | ||
| event.stopPropagation = vi.fn() | ||
| event.composedPath = vi.fn(() => [target]) | ||
| return event | ||
| } | ||
|
|
||
| describe('keybindingService - event propagation', () => { | ||
| let keybindingService: ReturnType<typeof useKeybindingService> | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| setActivePinia(createTestingPinia({ stubActions: false })) | ||
|
|
||
| const commandStore = useCommandStore() | ||
| commandStore.execute = vi.fn() | ||
|
|
||
| vi.mocked(useDialogStore).mockReturnValue({ | ||
| dialogStack: [] | ||
| } as Partial<ReturnType<typeof useDialogStore>> as ReturnType< | ||
| typeof useDialogStore | ||
| >) | ||
|
|
||
| keybindingService = useKeybindingService() | ||
| keybindingService.registerCoreKeybindings() | ||
| }) | ||
|
|
||
| it('stops propagation when Ctrl+Enter fires with a non-input element focused', async () => { | ||
| // Simulates a dropdown (combobox div) being focused when Ctrl+Enter is pressed. | ||
| // Without stopPropagation the event reaches the dropdown handler and expands it. | ||
| const dropdown = document.createElement('div') | ||
| dropdown.setAttribute('role', 'combobox') | ||
|
|
||
| const event = createKeyboardEvent('Enter', { | ||
| ctrlKey: true, | ||
| target: dropdown | ||
| }) | ||
|
|
||
| await keybindingService.keybindHandler(event) | ||
|
|
||
| expect(vi.mocked(useCommandStore().execute)).toHaveBeenCalledWith( | ||
| 'Comfy.QueuePrompt', | ||
| expect.any(Object) | ||
| ) | ||
| expect(event.stopPropagation).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not stop propagation when no keybinding matches', async () => { | ||
| const event = createKeyboardEvent('F13') // no binding for this key | ||
|
|
||
| await keybindingService.keybindHandler(event) | ||
|
|
||
| expect(event.stopPropagation).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not stop propagation when key is reserved by text input and target is textarea', async () => { | ||
| const textarea = document.createElement('textarea') | ||
| const event = createKeyboardEvent('Enter', { target: textarea }) | ||
|
|
||
| await keybindingService.keybindHandler(event) | ||
|
|
||
| expect(event.stopPropagation).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not stop propagation for bare Escape even when a keybinding matches', async () => { | ||
| // ExitSubgraph is bound to bare Escape. Element-level handlers (Reka UI | ||
| // menus, PrimeVue dialogs, BuilderFooterToolbar) need the event to reach | ||
| // them so they can close — stopPropagation would block them. | ||
| const event = createKeyboardEvent('Escape') | ||
|
|
||
| await keybindingService.keybindHandler(event) | ||
|
|
||
| expect(vi.mocked(useCommandStore().execute)).toHaveBeenCalledWith( | ||
| 'Comfy.Graph.ExitSubgraph' | ||
| ) | ||
| expect(event.stopPropagation).not.toHaveBeenCalled() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,16 @@ export function useKeybindingService() { | |
| } | ||
|
|
||
| event.preventDefault() | ||
| // Bare Escape must keep propagating so element-level handlers (Reka UI | ||
| // menus, PrimeVue dialogs, BuilderFooterToolbar) can still close themselves. | ||
| const isBareEscape = | ||
| event.key === 'Escape' && | ||
| !event.ctrlKey && | ||
| !event.altKey && | ||
| !event.metaKey | ||
| if (!isBareEscape) { | ||
|
Collaborator
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. issue: Bare Escape still executes the global keybinding before element-level handlers get a chance to consume it. Because |
||
| event.stopPropagation() | ||
| } | ||
| const runCommandIds = new Set([ | ||
| 'Comfy.QueuePrompt', | ||
| 'Comfy.QueuePromptFront', | ||
|
|
||
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.
isBareEscapemissingshiftKeycheck allows unintended propagationLow Severity
The
isBareEscapecondition checks!event.ctrlKey,!event.altKey, and!event.metaKeybut omits!event.shiftKey. If a user registers a customShift+Escapekeybinding, the check would still evaluate totrue, skippingstopPropagation(). This is the exact same class of bug this PR fixes forCtrl+Enter— a matched keybinding failing to prevent element-level handlers from firing — just for a hypotheticalShift+Escapebinding instead.Reviewed by Cursor Bugbot for commit 2e6ec6c. Configure here.