Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/graph/GraphCanvas.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -460,6 +461,7 @@ useNodeBadge()

useGlobalLitegraph()
useContextMenuTranslation()
useLiteGraphContextMenuTracking()
useCopy()
usePaste()
useWorkflowAutoSave()
Expand Down
3 changes: 3 additions & 0 deletions src/components/graph/NodeContextMenu.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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 })

Expand Down
52 changes: 52 additions & 0 deletions src/lib/litegraph/src/ContextMenu.test.ts
Original file line number Diff line number Diff line change
@@ -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)
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
48 changes: 38 additions & 10 deletions src/lib/litegraph/src/ContextMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class ContextMenu<TValue = unknown> {
root: ContextMenuDivElement<TValue>
current_submenu?: ContextMenu<TValue>
lock?: boolean
ownerDocument: Document

controller: AbortController = new AbortController()

Expand Down Expand Up @@ -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
Expand All @@ -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)) {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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 }
})
)
}
}

/**
Expand Down Expand Up @@ -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). */
Expand Down
26 changes: 26 additions & 0 deletions src/platform/keybindings/keybindingService.escape.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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'))
Expand Down
7 changes: 6 additions & 1 deletion src/platform/keybindings/keybindingService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -50,7 +52,10 @@ export function useKeybindingService() {
!event.altKey &&
!event.metaKey
) {
if (dialogStore.dialogStack.length > 0) {
if (
raisedSurfaceStore.isAnyOpen ||
dialogStore.dialogStack.length > 0
) {
return
}
}
Expand Down
34 changes: 34 additions & 0 deletions src/platform/keybindings/raisedSurfaceLiteGraphBridge.ts
Original file line number Diff line number Diff line change
@@ -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()
})
}
89 changes: 89 additions & 0 deletions src/platform/keybindings/raisedSurfaceStore.test.ts
Original file line number Diff line number Diff line change
@@ -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)
})
})
Loading
Loading