From 16dc701f4c5c608cb19ef88b33e226405c3be731 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Fri, 15 May 2026 20:39:05 +0000 Subject: [PATCH 1/3] fix: prevent escape from exiting subgraph while a context menu is open Pressing Escape while a right-click context menu is open inside a subgraph used to fire the global Comfy.Graph.ExitSubgraph keybinding, exiting the subgraph while leaving the menu open. The keybinding service only suppressed Escape when a Pinia dialog was open, so other raised surfaces leaked the event to window-level handlers. Introduce a raisedSurfaceStore that tracks open popovers, context menus, and top-level modals as a single source of truth, and consult it from the keybinding service alongside the existing dialog check. The legacy LiteGraph ContextMenu now closes itself on Escape (mirroring its existing outside-pointerdown handler) and reports open/close lifecycle via document events so the store stays in sync without monkey patches. NodeContextMenu registers itself via the new useRaisedSurface composable. Fixes the bug demonstrated in the attached screencast. --- src/components/graph/GraphCanvas.vue | 2 + src/components/graph/NodeContextMenu.vue | 3 + src/lib/litegraph/src/ContextMenu.test.ts | 48 ++++++++++ src/lib/litegraph/src/ContextMenu.ts | 25 ++++++ .../keybindingService.escape.test.ts | 26 ++++++ src/platform/keybindings/keybindingService.ts | 7 +- .../raisedSurfaceLiteGraphBridge.ts | 32 +++++++ .../keybindings/raisedSurfaceStore.test.ts | 89 +++++++++++++++++++ .../keybindings/raisedSurfaceStore.ts | 72 +++++++++++++++ 9 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 src/lib/litegraph/src/ContextMenu.test.ts create mode 100644 src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts create mode 100644 src/platform/keybindings/raisedSurfaceStore.test.ts create mode 100644 src/platform/keybindings/raisedSurfaceStore.ts diff --git a/src/components/graph/GraphCanvas.vue b/src/components/graph/GraphCanvas.vue index 67ef64d7445..d5f5a695a55 100644 --- a/src/components/graph/GraphCanvas.vue +++ b/src/components/graph/GraphCanvas.vue @@ -152,6 +152,7 @@ import { useGlobalLitegraph } from '@/composables/useGlobalLitegraph' import { usePaste } from '@/composables/usePaste' import { useVueFeatureFlags } from '@/composables/useVueFeatureFlags' import { LiteGraph } from '@/lib/litegraph/src/litegraph' +import { useLiteGraphContextMenuTracking } from '@/platform/keybindings/raisedSurfaceLiteGraphBridge' import { useLitegraphSettings } from '@/platform/settings/composables/useLitegraphSettings' import { CORE_SETTINGS } from '@/platform/settings/constants/coreSettings' import { useSettingStore } from '@/platform/settings/settingStore' @@ -460,6 +461,7 @@ useNodeBadge() useGlobalLitegraph() useContextMenuTranslation() +useLiteGraphContextMenuTracking() useCopy() usePaste() useWorkflowAutoSave() diff --git a/src/components/graph/NodeContextMenu.vue b/src/components/graph/NodeContextMenu.vue index 6bd285fd8c5..70cf98a27b6 100644 --- a/src/components/graph/NodeContextMenu.vue +++ b/src/components/graph/NodeContextMenu.vue @@ -52,6 +52,7 @@ import type { MenuOption, SubMenuOption } from '@/composables/graph/useMoreOptionsMenu' +import { useRaisedSurface } from '@/platform/keybindings/raisedSurfaceStore' import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' import ColorPickerMenu from './selectionToolbox/ColorPickerMenu.vue' @@ -69,6 +70,8 @@ const isOpen = ref(false) const { menuOptions, bump } = useMoreOptionsMenu() const canvasStore = useCanvasStore() +useRaisedSurface('context-menu', isOpen) + // World position (canvas coordinates) where menu was opened const worldPosition = ref({ x: 0, y: 0 }) diff --git a/src/lib/litegraph/src/ContextMenu.test.ts b/src/lib/litegraph/src/ContextMenu.test.ts new file mode 100644 index 00000000000..8a99e46b478 --- /dev/null +++ b/src/lib/litegraph/src/ContextMenu.test.ts @@ -0,0 +1,48 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { ContextMenu } from '@/lib/litegraph/src/ContextMenu' + +describe('ContextMenu lifecycle events', () => { + let listener: (event: Event) => void + let calls: Event[] + + beforeEach(() => { + document.body.innerHTML = '' + calls = [] + listener = (event) => calls.push(event) + document.addEventListener('litegraph:contextmenu', listener) + }) + + afterEach(() => { + document.removeEventListener('litegraph:contextmenu', listener) + }) + + it('dispatches an "open" event when a top-level menu is constructed', () => { + new ContextMenu(['Item A', 'Item B'], {}) + + expect(calls).toHaveLength(1) + const detail = (calls[0] as CustomEvent).detail + expect(detail.type).toBe('open') + }) + + it('dispatches a "close" event when a top-level menu closes', () => { + const menu = new ContextMenu(['Item A'], {}) + calls.length = 0 + + menu.close() + + expect(calls).toHaveLength(1) + const detail = (calls[0] as CustomEvent).detail + expect(detail.type).toBe('close') + expect(detail.menu).toBe(menu) + }) + + it('does not dispatch lifecycle events for submenus', () => { + const parent = new ContextMenu(['Item A'], {}) + calls.length = 0 + + new ContextMenu(['Sub Item'], { parentMenu: parent }) + + expect(calls).toHaveLength(0) + }) +}) diff --git a/src/lib/litegraph/src/ContextMenu.ts b/src/lib/litegraph/src/ContextMenu.ts index 90450522fe5..a12dd594cf5 100644 --- a/src/lib/litegraph/src/ContextMenu.ts +++ b/src/lib/litegraph/src/ContextMenu.ts @@ -126,6 +126,16 @@ export class ContextMenu { }, eventOptions ) + document.addEventListener( + 'keydown', + (e) => { + if (e.key !== 'Escape' || e.ctrlKey || e.altKey || e.metaKey) return + this.close() + e.preventDefault() + e.stopPropagation() + }, + eventOptions + ) } // this prevents the default context browser menu to open in case this menu was created when pressing right button @@ -219,6 +229,14 @@ export class ContextMenu { if (LiteGraph.context_menu_scaling && options.scale) { root.style.transform = `scale(${Math.round(options.scale * 4) * 0.25})` } + + if (!parent) { + document.dispatchEvent( + new CustomEvent('litegraph:contextmenu', { + detail: { type: 'open', menu: this } + }) + ) + } } /** @@ -402,6 +420,13 @@ export class ContextMenu { } } this.current_submenu?.close(e, true) + if (!this.parentMenu) { + document.dispatchEvent( + new CustomEvent('litegraph:contextmenu', { + detail: { type: 'close', menu: this } + }) + ) + } } /** @deprecated Likely unused, however code search was inconclusive (too many results to check by hand). */ diff --git a/src/platform/keybindings/keybindingService.escape.test.ts b/src/platform/keybindings/keybindingService.escape.test.ts index 42b946a13a5..b235fffdc69 100644 --- a/src/platform/keybindings/keybindingService.escape.test.ts +++ b/src/platform/keybindings/keybindingService.escape.test.ts @@ -7,6 +7,7 @@ import { KeyComboImpl } from '@/platform/keybindings/keyCombo' import { KeybindingImpl } from '@/platform/keybindings/keybinding' import { useKeybindingService } from '@/platform/keybindings/keybindingService' import { useKeybindingStore } from '@/platform/keybindings/keybindingStore' +import { useRaisedSurfaceStore } from '@/platform/keybindings/raisedSurfaceStore' import { useCommandStore } from '@/stores/commandStore' import type { DialogInstance } from '@/stores/dialogStore' import { useDialogStore } from '@/stores/dialogStore' @@ -108,6 +109,31 @@ describe('keybindingService - Escape key handling', () => { expect(mockCommandExecute).not.toHaveBeenCalled() }) + it('should NOT execute Escape keybinding when a raised surface is open', async () => { + const raisedSurfaceStore = useRaisedSurfaceStore() + raisedSurfaceStore.open('context-menu') + + keybindingService = useKeybindingService() + + const event = createKeyboardEvent('Escape') + await keybindingService.keybindHandler(event) + + expect(mockCommandExecute).not.toHaveBeenCalled() + }) + + it('should resume executing Escape keybinding after a raised surface closes', async () => { + const raisedSurfaceStore = useRaisedSurfaceStore() + const id = raisedSurfaceStore.open('context-menu') + raisedSurfaceStore.close(id) + + keybindingService = useKeybindingService() + + const event = createKeyboardEvent('Escape') + await keybindingService.keybindHandler(event) + + expect(mockCommandExecute).toHaveBeenCalledWith('Comfy.Graph.ExitSubgraph') + }) + it('should execute Escape keybinding with modifiers regardless of dialog state', async () => { const dialogStore = useDialogStore() dialogStore.dialogStack.push(createTestDialogInstance('test-dialog')) diff --git a/src/platform/keybindings/keybindingService.ts b/src/platform/keybindings/keybindingService.ts index b11d6831a7d..f78572e1d59 100644 --- a/src/platform/keybindings/keybindingService.ts +++ b/src/platform/keybindings/keybindingService.ts @@ -7,12 +7,14 @@ import { CORE_KEYBINDINGS } from './defaults' import { KeyComboImpl } from './keyCombo' import { KeybindingImpl } from './keybinding' import { useKeybindingStore } from './keybindingStore' +import { useRaisedSurfaceStore } from './raisedSurfaceStore' export function useKeybindingService() { const keybindingStore = useKeybindingStore() const commandStore = useCommandStore() const settingStore = useSettingStore() const dialogStore = useDialogStore() + const raisedSurfaceStore = useRaisedSurfaceStore() async function keybindHandler(event: KeyboardEvent) { const keyCombo = KeyComboImpl.fromEvent(event) @@ -50,7 +52,10 @@ export function useKeybindingService() { !event.altKey && !event.metaKey ) { - if (dialogStore.dialogStack.length > 0) { + if ( + raisedSurfaceStore.isAnyOpen || + dialogStore.dialogStack.length > 0 + ) { return } } diff --git a/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts b/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts new file mode 100644 index 00000000000..3778a2d68e2 --- /dev/null +++ b/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts @@ -0,0 +1,32 @@ +import { onScopeDispose } from 'vue' + +import { useRaisedSurfaceStore } from './raisedSurfaceStore' + +interface LiteGraphContextMenuEventDetail { + type: 'open' | 'close' + menu: object +} + +export function useLiteGraphContextMenuTracking(): void { + const store = useRaisedSurfaceStore() + const idsByMenu = new WeakMap() + + const handler = (event: Event) => { + const detail = (event as CustomEvent) + .detail + if (detail.type === 'open') { + idsByMenu.set(detail.menu, store.open('context-menu')) + return + } + const id = idsByMenu.get(detail.menu) + if (id !== undefined) { + store.close(id) + idsByMenu.delete(detail.menu) + } + } + + document.addEventListener('litegraph:contextmenu', handler) + onScopeDispose(() => { + document.removeEventListener('litegraph:contextmenu', handler) + }) +} diff --git a/src/platform/keybindings/raisedSurfaceStore.test.ts b/src/platform/keybindings/raisedSurfaceStore.test.ts new file mode 100644 index 00000000000..2590a0b0dcb --- /dev/null +++ b/src/platform/keybindings/raisedSurfaceStore.test.ts @@ -0,0 +1,89 @@ +import { createPinia, setActivePinia } from 'pinia' +import { effectScope, ref } from 'vue' +import { beforeEach, describe, expect, it } from 'vitest' + +import { useRaisedSurface, useRaisedSurfaceStore } from './raisedSurfaceStore' + +describe('raisedSurfaceStore', () => { + beforeEach(() => { + setActivePinia(createPinia()) + }) + + it('is empty by default', () => { + const store = useRaisedSurfaceStore() + expect(store.isAnyOpen).toBe(false) + expect(store.stack).toHaveLength(0) + }) + + it('tracks open/close and reports isAnyOpen', () => { + const store = useRaisedSurfaceStore() + const id = store.open('context-menu') + expect(store.isAnyOpen).toBe(true) + expect(store.stack).toHaveLength(1) + store.close(id) + expect(store.isAnyOpen).toBe(false) + expect(store.stack).toHaveLength(0) + }) + + it('supports concurrent surfaces and closes by id (LIFO not required)', () => { + const store = useRaisedSurfaceStore() + const a = store.open('context-menu') + const b = store.open('popover') + expect(store.stack).toHaveLength(2) + store.close(a) + expect(store.stack).toHaveLength(1) + expect(store.stack[0].kind).toBe('popover') + store.close(b) + expect(store.isAnyOpen).toBe(false) + }) + + it('close is a no-op for unknown ids', () => { + const store = useRaisedSurfaceStore() + store.open('modal') + store.close(Symbol('stale')) + expect(store.stack).toHaveLength(1) + }) +}) + +describe('useRaisedSurface', () => { + beforeEach(() => { + setActivePinia(createPinia()) + }) + + it('opens when reactive flag turns true and closes when it turns false', () => { + const store = useRaisedSurfaceStore() + const isOpen = ref(false) + const scope = effectScope() + scope.run(() => useRaisedSurface('context-menu', isOpen)) + + expect(store.isAnyOpen).toBe(false) + isOpen.value = true + expect(store.isAnyOpen).toBe(true) + isOpen.value = false + expect(store.isAnyOpen).toBe(false) + scope.stop() + }) + + it('does not double-register when flag is toggled true twice', () => { + const store = useRaisedSurfaceStore() + const isOpen = ref(false) + const scope = effectScope() + scope.run(() => useRaisedSurface('context-menu', isOpen)) + + isOpen.value = true + isOpen.value = true + expect(store.stack).toHaveLength(1) + scope.stop() + }) + + it('releases the surface when the owning scope is disposed', () => { + const store = useRaisedSurfaceStore() + const isOpen = ref(true) + const scope = effectScope() + scope.run(() => useRaisedSurface('context-menu', isOpen)) + + expect(store.isAnyOpen).toBe(true) + scope.stop() + expect(store.isAnyOpen).toBe(false) + }) +}) diff --git a/src/platform/keybindings/raisedSurfaceStore.ts b/src/platform/keybindings/raisedSurfaceStore.ts new file mode 100644 index 00000000000..86f81c2f029 --- /dev/null +++ b/src/platform/keybindings/raisedSurfaceStore.ts @@ -0,0 +1,72 @@ +import { defineStore } from 'pinia' +import { computed, onScopeDispose, ref, toValue, watch } from 'vue' +import type { MaybeRefOrGetter } from 'vue' + +/** + * Tracks open "raised surfaces" — popovers, context menus, and top-level + * modals — that should suppress global keybindings while they are open. + * + * UX axiom: standard keybindings work in every context EXCEPT when a raised + * surface is open. Consumers register/unregister surfaces here; the keybinding + * service consults {@link isAnyOpen} as its single source of truth. + */ +type RaisedSurfaceKind = 'context-menu' | 'popover' | 'modal' + +interface RaisedSurfaceEntry { + id: symbol + kind: RaisedSurfaceKind +} + +export const useRaisedSurfaceStore = defineStore('raisedSurface', () => { + const stack = ref([]) + const isAnyOpen = computed(() => stack.value.length > 0) + + function open(kind: RaisedSurfaceKind): symbol { + const id = Symbol(kind) + stack.value.push({ id, kind }) + return id + } + + function close(id: symbol): void { + const index = stack.value.findIndex((entry) => entry.id === id) + if (index !== -1) stack.value.splice(index, 1) + } + + return { stack, isAnyOpen, open, close } +}) + +/** + * Bind a surface's reactive open-state to the raised-surface registry. + * + * @example + * const isOpen = ref(false) + * useRaisedSurface('context-menu', isOpen) + */ +export function useRaisedSurface( + kind: RaisedSurfaceKind, + isOpen: MaybeRefOrGetter +): void { + const store = useRaisedSurfaceStore() + let id: symbol | null = null + + function release() { + if (id !== null) { + store.close(id) + id = null + } + } + + watch( + () => toValue(isOpen), + (open) => { + if (open && id === null) { + id = store.open(kind) + } else if (!open) { + release() + } + }, + { immediate: true, flush: 'sync' } + ) + + onScopeDispose(release) +} From 1e381cccf81866badf52dee2b12a2dbdf594874b Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Sat, 16 May 2026 03:12:15 +0000 Subject: [PATCH 2/3] fix: address review feedback - ContextMenu: use the menu's owner document (event target ownerDocument or fallback) for listeners, DOM insertion, and lifecycle events, so menus opened in fullscreen or alternate documents stay consistent. - raisedSurfaceLiteGraphBridge: track active context-menu ids and release them on scope dispose, so a menu open during unmount doesn't leak a stale store entry. - ContextMenu tests: close all top-level menus in afterEach to prevent document-level listeners leaking between tests. --- src/lib/litegraph/src/ContextMenu.test.ts | 6 ++++- src/lib/litegraph/src/ContextMenu.ts | 27 ++++++++++--------- .../raisedSurfaceLiteGraphBridge.ts | 4 ++- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/lib/litegraph/src/ContextMenu.test.ts b/src/lib/litegraph/src/ContextMenu.test.ts index 8a99e46b478..9dcf32a75a7 100644 --- a/src/lib/litegraph/src/ContextMenu.test.ts +++ b/src/lib/litegraph/src/ContextMenu.test.ts @@ -5,20 +5,23 @@ import { ContextMenu } from '@/lib/litegraph/src/ContextMenu' describe('ContextMenu lifecycle events', () => { let listener: (event: Event) => void let calls: Event[] + let menus: ContextMenu[] beforeEach(() => { document.body.innerHTML = '' calls = [] + menus = [] listener = (event) => calls.push(event) document.addEventListener('litegraph:contextmenu', listener) }) afterEach(() => { + for (const menu of menus) menu.close() document.removeEventListener('litegraph:contextmenu', listener) }) it('dispatches an "open" event when a top-level menu is constructed', () => { - new ContextMenu(['Item A', 'Item B'], {}) + menus.push(new ContextMenu(['Item A', 'Item B'], {})) expect(calls).toHaveLength(1) const detail = (calls[0] as CustomEvent).detail @@ -39,6 +42,7 @@ describe('ContextMenu lifecycle events', () => { it('does not dispatch lifecycle events for submenus', () => { const parent = new ContextMenu(['Item A'], {}) + menus.push(parent) calls.length = 0 new ContextMenu(['Sub Item'], { parentMenu: parent }) diff --git a/src/lib/litegraph/src/ContextMenu.ts b/src/lib/litegraph/src/ContextMenu.ts index a12dd594cf5..2c8f61ca32a 100644 --- a/src/lib/litegraph/src/ContextMenu.ts +++ b/src/lib/litegraph/src/ContextMenu.ts @@ -56,6 +56,7 @@ export class ContextMenu { root: ContextMenuDivElement current_submenu?: ContextMenu lock?: boolean + ownerDocument: Document controller: AbortController = new AbortController() @@ -105,7 +106,13 @@ export class ContextMenu { options.event = undefined } - const root: ContextMenuDivElement = document.createElement('div') + const ownerDocument = + (options.event?.target as Node | null | undefined)?.ownerDocument ?? + document + this.ownerDocument = ownerDocument + + const root: ContextMenuDivElement = + ownerDocument.createElement('div') let classes = 'litegraph litecontextmenu litemenubar-panel' if (options.className) classes += ` ${options.className}` root.className = classes @@ -117,7 +124,7 @@ export class ContextMenu { const eventOptions = { capture: true, signal } if (!this.parentMenu) { - document.addEventListener( + ownerDocument.addEventListener( 'pointerdown', (e) => { if (e.target instanceof Node && !this.containsNode(e.target)) { @@ -126,7 +133,7 @@ export class ContextMenu { }, eventOptions ) - document.addEventListener( + ownerDocument.addEventListener( 'keydown', (e) => { if (e.key !== 'Escape' || e.ctrlKey || e.altKey || e.metaKey) return @@ -189,13 +196,9 @@ export class ContextMenu { } // insert before checking position - const ownerDocument = (options.event?.target as Node | null | undefined) - ?.ownerDocument - const root_document = ownerDocument || document - - if (root_document.fullscreenElement) - root_document.fullscreenElement.append(root) - else root_document.body.append(root) + if (ownerDocument.fullscreenElement) + ownerDocument.fullscreenElement.append(root) + else ownerDocument.body.append(root) // compute best position let left = options.left || 0 @@ -231,7 +234,7 @@ export class ContextMenu { } if (!parent) { - document.dispatchEvent( + ownerDocument.dispatchEvent( new CustomEvent('litegraph:contextmenu', { detail: { type: 'open', menu: this } }) @@ -421,7 +424,7 @@ export class ContextMenu { } this.current_submenu?.close(e, true) if (!this.parentMenu) { - document.dispatchEvent( + this.ownerDocument.dispatchEvent( new CustomEvent('litegraph:contextmenu', { detail: { type: 'close', menu: this } }) diff --git a/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts b/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts index 3778a2d68e2..0f15641c34b 100644 --- a/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts +++ b/src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts @@ -9,7 +9,7 @@ interface LiteGraphContextMenuEventDetail { export function useLiteGraphContextMenuTracking(): void { const store = useRaisedSurfaceStore() - const idsByMenu = new WeakMap() + const idsByMenu = new Map() const handler = (event: Event) => { const detail = (event as CustomEvent) @@ -28,5 +28,7 @@ export function useLiteGraphContextMenuTracking(): void { document.addEventListener('litegraph:contextmenu', handler) onScopeDispose(() => { document.removeEventListener('litegraph:contextmenu', handler) + for (const id of idsByMenu.values()) store.close(id) + idsByMenu.clear() }) } From 7cfaeb33f86dc19280ed245dd1e2335fab0dfd75 Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Sat, 16 May 2026 03:17:23 +0000 Subject: [PATCH 3/3] fix: clamp ContextMenu position against ownerDocument body Last remaining global `document.body` reference in the positioning block: switch to `ownerDocument.body` so menus opened inside iframes, fullscreen elements, or alternate documents are clamped against the correct viewport. --- src/lib/litegraph/src/ContextMenu.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/litegraph/src/ContextMenu.ts b/src/lib/litegraph/src/ContextMenu.ts index 2c8f61ca32a..284d12199e3 100644 --- a/src/lib/litegraph/src/ContextMenu.ts +++ b/src/lib/litegraph/src/ContextMenu.ts @@ -213,7 +213,7 @@ export class ContextMenu { left = rect.left + rect.width } - const body_rect = document.body.getBoundingClientRect() + const body_rect = ownerDocument.body.getBoundingClientRect() const root_rect = root.getBoundingClientRect() if (body_rect.height == 0) console.error(