Compare commits

...

3 Commits

Author SHA1 Message Date
Glary-Bot
7cfaeb33f8 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.
2026-05-16 03:17:23 +00:00
Glary-Bot
1e381cccf8 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.
2026-05-16 03:12:15 +00:00
Glary-Bot
16dc701f4c 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.
2026-05-15 20:39:05 +00:00
9 changed files with 322 additions and 11 deletions

View File

@@ -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()

View File

@@ -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 })

View File

@@ -0,0 +1,52 @@
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[]
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', () => {
menus.push(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'], {})
menus.push(parent)
calls.length = 0
new ContextMenu(['Sub Item'], { parentMenu: parent })
expect(calls).toHaveLength(0)
})
})

View File

@@ -56,6 +56,7 @@ export class ContextMenu<TValue = unknown> {
root: ContextMenuDivElement<TValue>
current_submenu?: ContextMenu<TValue>
lock?: boolean
ownerDocument: Document
controller: AbortController = new AbortController()
@@ -105,7 +106,13 @@ export class ContextMenu<TValue = unknown> {
options.event = undefined
}
const root: ContextMenuDivElement<TValue> = document.createElement('div')
const ownerDocument =
(options.event?.target as Node | null | undefined)?.ownerDocument ??
document
this.ownerDocument = ownerDocument
const root: ContextMenuDivElement<TValue> =
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<TValue = unknown> {
const eventOptions = { capture: true, signal }
if (!this.parentMenu) {
document.addEventListener(
ownerDocument.addEventListener(
'pointerdown',
(e) => {
if (e.target instanceof Node && !this.containsNode(e.target)) {
@@ -126,6 +133,16 @@ export class ContextMenu<TValue = unknown> {
},
eventOptions
)
ownerDocument.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
@@ -179,13 +196,9 @@ export class ContextMenu<TValue = unknown> {
}
// 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
@@ -200,7 +213,7 @@ export class ContextMenu<TValue = unknown> {
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(
@@ -219,6 +232,14 @@ export class ContextMenu<TValue = unknown> {
if (LiteGraph.context_menu_scaling && options.scale) {
root.style.transform = `scale(${Math.round(options.scale * 4) * 0.25})`
}
if (!parent) {
ownerDocument.dispatchEvent(
new CustomEvent('litegraph:contextmenu', {
detail: { type: 'open', menu: this }
})
)
}
}
/**
@@ -402,6 +423,13 @@ export class ContextMenu<TValue = unknown> {
}
}
this.current_submenu?.close(e, true)
if (!this.parentMenu) {
this.ownerDocument.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). */

View File

@@ -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'))

View File

@@ -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
}
}

View File

@@ -0,0 +1,34 @@
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 Map<object, symbol>()
const handler = (event: Event) => {
const detail = (event as CustomEvent<LiteGraphContextMenuEventDetail>)
.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)
for (const id of idsByMenu.values()) store.close(id)
idsByMenu.clear()
})
}

View File

@@ -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)
})
})

View File

@@ -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<RaisedSurfaceEntry[]>([])
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<boolean>
): 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)
}