From 14d94da52b12089500fe8f1afe647a07e7a739cb Mon Sep 17 00:00:00 2001 From: Johnpaul Chiwetelu <49923152+Myestery@users.noreply.github.com> Date: Wed, 19 Nov 2025 20:32:20 +0100 Subject: [PATCH] Fix Node Event Handlers for Shift Click (#6262) This pull request refactors the node selection and pointer interaction logic in the Vue node graph editor to improve multi-selection behavior, clarify event handling, and enhance test coverage. The main change is to defer multi-select toggle actions (such as ctrl+click for selection/deselection) from pointer down to pointer up, preventing premature selection state changes and making drag interactions more robust. The drag initiation logic is also refined to only start dragging after the pointer moves beyond a threshold, and new composable methods are introduced for granular node selection control. **Node selection and pointer event handling improvements:** * Refactored multi-select (ctrl/cmd/shift+click) logic in `useNodeEventHandlersIndividual`: selection toggling is now deferred to pointer up, and pointer down only brings the node to front without changing selection state. The previous `hasMultipleNodesSelected` function and related logic were removed for clarity. [[1]](diffhunk://#diff-8d3820a1ca9c569bce00671fdd6290af81315ae11b8f3d6f29a5a9d30379d084L18-L35) [[2]](diffhunk://#diff-8d3820a1ca9c569bce00671fdd6290af81315ae11b8f3d6f29a5a9d30379d084L57-L73) [[3]](diffhunk://#diff-89bfc2a05201c6ff7116578efa45f96097594eb346f18446c70aa7125ab1811aL112-L116) [[4]](diffhunk://#diff-89bfc2a05201c6ff7116578efa45f96097594eb346f18446c70aa7125ab1811aR127-R143) * Added new composable methods `deselectNode` and `toggleNodeSelectionAfterPointerUp` to `useNodeEventHandlersIndividual` for more granular control over node selection, and exposed them in the returned API. [[1]](diffhunk://#diff-8d3820a1ca9c569bce00671fdd6290af81315ae11b8f3d6f29a5a9d30379d084R210-R245) [[2]](diffhunk://#diff-8d3820a1ca9c569bce00671fdd6290af81315ae11b8f3d6f29a5a9d30379d084L251-R259) **Pointer interaction and drag behavior changes:** * Updated `useNodePointerInteractions` to track pointer down/up state and only start dragging after the pointer moves beyond a pixel threshold. Multi-select toggling is now handled on pointer up, not pointer down, and selection state is read from the actual node manager for accuracy. [[1]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1R6-R10) [[2]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1R33-R34) [[3]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1R44-R53) [[4]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1R76-R110) [[5]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1R122-R123) [[6]](diffhunk://#diff-b50f38fec4f988dcbee7b7adf2b3425ae1e40a7ff10439ecbcb380dfa0a05ee1L131-R175) **Test suite enhancements:** * Improved and expanded tests for pointer interactions and selection logic, including new cases for ctrl+click selection toggling on pointer up, drag threshold behavior, and mocking of new composable methods. [[1]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdR9-R11) [[2]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdR35-R56) [[3]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdR100-R102) [[4]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdL144-R181) [[5]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdL155-R196) [[6]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdL196-R247) [[7]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdL276-R336) [[8]](diffhunk://#diff-8d94b444c448b346f5863e859c75f67267439a56a02baf44b385af1c6945effdR348-R423) * Updated test setup and assertions for node event handlers, ensuring selection changes are only triggered at the correct event phase and that drag and multi-select logic is covered. [[1]](diffhunk://#diff-89bfc2a05201c6ff7116578efa45f96097594eb346f18446c70aa7125ab1811aL4-R7) [[2]](diffhunk://#diff-89bfc2a05201c6ff7116578efa45f96097594eb346f18446c70aa7125ab1811aR92) These changes make node selection more predictable and user-friendly, and ensure drag and multi-select actions behave consistently in both the UI and the test suite. fix https://github.com/Comfy-Org/ComfyUI_frontend/issues/6128 https://github.com/user-attachments/assets/582804d0-1d21-4ba0-a161-6582fb379352 --- .../canvas/useSelectionToolboxPosition.ts | 22 +- .../composables/useNodeEventHandlers.ts | 123 ++++++--- .../useNodePointerInteractions.test.ts | 217 +++++++++++++++- .../composables/useNodePointerInteractions.ts | 64 ++++- .../vueNodes/utils/selectionUtils.ts | 10 + .../composables/useNodeEventHandlers.test.ts | 239 ++++++++++++++++-- 6 files changed, 600 insertions(+), 75 deletions(-) create mode 100644 src/renderer/extensions/vueNodes/utils/selectionUtils.ts diff --git a/src/composables/canvas/useSelectionToolboxPosition.ts b/src/composables/canvas/useSelectionToolboxPosition.ts index 426604f824..8296ee955a 100644 --- a/src/composables/canvas/useSelectionToolboxPosition.ts +++ b/src/composables/canvas/useSelectionToolboxPosition.ts @@ -66,6 +66,14 @@ export function useSelectionToolboxPosition( lgCanvas.canvas ) + // Unified dragging state - combines both LiteGraph and Vue node dragging + const isDragging = computed((): boolean => { + const litegraphDragging = canvasStore.canvas?.state?.draggingItems ?? false + const vueNodeDragging = + shouldRenderVueNodes.value && layoutStore.isDraggingVueNodes.value + return litegraphDragging || vueNodeDragging + }) + /** * Update position based on selection */ @@ -77,6 +85,12 @@ export function useSelectionToolboxPosition( return } + // Don't show toolbox while dragging + if (isDragging.value) { + visible.value = false + return + } + visible.value = true // Get bounds for all selected items @@ -241,14 +255,6 @@ export function useSelectionToolboxPosition( }) } - // Unified dragging state - combines both LiteGraph and Vue node dragging - const isDragging = computed((): boolean => { - const litegraphDragging = canvasStore.canvas?.state?.draggingItems ?? false - const vueNodeDragging = - shouldRenderVueNodes.value && layoutStore.isDraggingVueNodes.value - return litegraphDragging || vueNodeDragging - }) - watch(isDragging, handleDragStateChange) onUnmounted(() => { diff --git a/src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts b/src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts index 05373c5223..b5a425378a 100644 --- a/src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts +++ b/src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts @@ -15,24 +15,7 @@ import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle' import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' import { useCanvasInteractions } from '@/renderer/core/canvas/useCanvasInteractions' import { useNodeZIndex } from '@/renderer/extensions/vueNodes/composables/useNodeZIndex' -import { isLGraphNode } from '@/utils/litegraphUtil' - -/** - * Check if multiple nodes are selected - * Optimized to return early when 2+ nodes found - */ -function hasMultipleNodesSelected(selectedItems: unknown[]): boolean { - let count = 0 - for (let i = 0; i < selectedItems.length; i++) { - if (isLGraphNode(selectedItems[i])) { - count++ - if (count >= 2) { - return true - } - } - } - return false -} +import { isMultiSelectKey } from '@/renderer/extensions/vueNodes/utils/selectionUtils' function useNodeEventHandlersIndividual() { const canvasStore = useCanvasStore() @@ -52,24 +35,19 @@ function useNodeEventHandlersIndividual() { const node = nodeManager.value.getNode(nodeData.id) if (!node) return - const isMultiSelect = event.ctrlKey || event.metaKey || event.shiftKey + const multiSelect = isMultiSelectKey(event) + const selectedItemsCount = canvasStore.selectedItems.length + const preserveExistingSelection = + !multiSelect && node.selected && selectedItemsCount > 1 - if (isMultiSelect) { - // Ctrl/Cmd+click -> toggle selection - if (node.selected) { - canvasStore.canvas.deselect(node) - } else { - canvasStore.canvas.select(node) - } - } else { - const selectedMultipleNodes = hasMultipleNodesSelected( - canvasStore.selectedItems - ) - if (!selectedMultipleNodes) { - // Single-select the node - canvasStore.canvas.deselectAll() + if (multiSelect) { + if (!node.selected) { canvasStore.canvas.select(node) } + } else if (!preserveExistingSelection) { + // Regular click -> single select + canvasStore.canvas.deselectAll() + canvasStore.canvas.select(node) } // Bring node to front when clicked (similar to LiteGraph behavior) @@ -219,6 +197,32 @@ function useNodeEventHandlersIndividual() { canvasStore.updateSelectedItems() } + /** + * Ensure node is selected for shift-drag operations + * Handles special logic for promoting a node to selection when shift-dragging + * @param event - The pointer event (for multi-select key detection) + * @param nodeData - The node data for the node being dragged + * @param wasSelectedAtPointerDown - Whether the node was selected when pointer-down occurred + */ + const ensureNodeSelectedForShiftDrag = ( + event: PointerEvent, + nodeData: VueNodeData, + wasSelectedAtPointerDown: boolean + ) => { + if (wasSelectedAtPointerDown) return + + const multiSelectKeyPressed = isMultiSelectKey(event) + if (!multiSelectKeyPressed) return + + if (!canvasStore.canvas || !nodeManager.value) return + const node = nodeManager.value.getNode(nodeData.id) + if (!node || node.selected) return + + const selectionCount = canvasStore.selectedItems.length + const addToSelection = selectionCount > 0 + selectNodes([nodeData.id], addToSelection) + } + /** * Deselect specific nodes */ @@ -229,14 +233,58 @@ function useNodeEventHandlersIndividual() { nodeIds.forEach((nodeId) => { const node = nodeManager.value?.getNode(nodeId) - if (node) { - node.selected = false + if (node && canvasStore.canvas) { + canvasStore.canvas.deselect(node) } }) canvasStore.updateSelectedItems() } + const deselectNode = (nodeId: string) => { + const node = nodeManager.value?.getNode(nodeId) + if (node) { + canvasStore.canvas?.deselect(node) + canvasStore.updateSelectedItems() + } + } + + const toggleNodeSelectionAfterPointerUp = ( + nodeId: string, + { + wasSelectedAtPointerDown, + multiSelect + }: { + wasSelectedAtPointerDown: boolean + multiSelect: boolean + } + ) => { + if (!shouldHandleNodePointerEvents.value) return + + if (!canvasStore.canvas || !nodeManager.value) return + + const node = nodeManager.value.getNode(nodeId) + if (!node) return + + if (!multiSelect) { + const multipleSelected = canvasStore.selectedItems.length > 1 + if (multipleSelected && wasSelectedAtPointerDown) { + canvasStore.canvas.deselectAll() + canvasStore.canvas.select(node) + canvasStore.updateSelectedItems() + } + return + } + + if (wasSelectedAtPointerDown) { + canvasStore.canvas.deselect(node) + canvasStore.updateSelectedItems() + } + + // No action needed when the node was not previously selected since the pointer-down + // handler already added it to the selection. + } + return { // Core event handlers handleNodeSelect, @@ -248,7 +296,10 @@ function useNodeEventHandlersIndividual() { // Batch operations selectNodes, - deselectNodes + deselectNodes, + deselectNode, + ensureNodeSelectedForShiftDrag, + toggleNodeSelectionAfterPointerUp } } diff --git a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts index b4bd1d8394..0874573dcc 100644 --- a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts +++ b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts @@ -6,6 +6,11 @@ import type { VueNodeData } from '@/composables/graph/useGraphNodeManager' import { useNodePointerInteractions } from '@/renderer/extensions/vueNodes/composables/useNodePointerInteractions' const forwardEventToCanvasMock = vi.fn() +const deselectNodeMock = vi.fn() +const selectNodesMock = vi.fn() +const toggleNodeSelectionAfterPointerUpMock = vi.fn() +const ensureNodeSelectedForShiftDragMock = vi.fn() +const selectedItemsState: { items: Array<{ id?: string }> } = { items: [] } // Mock the dependencies vi.mock('@/renderer/core/canvas/useCanvasInteractions', () => ({ @@ -29,6 +34,37 @@ vi.mock('@/renderer/core/layout/store/layoutStore', () => ({ } })) +vi.mock('@/renderer/core/canvas/canvasStore', () => ({ + useCanvasStore: () => ({ + get selectedItems() { + return selectedItemsState.items + } + }) +})) + +vi.mock( + '@/renderer/extensions/vueNodes/composables/useNodeEventHandlers', + () => ({ + useNodeEventHandlers: () => ({ + deselectNode: deselectNodeMock, + selectNodes: selectNodesMock, + toggleNodeSelectionAfterPointerUp: toggleNodeSelectionAfterPointerUpMock, + ensureNodeSelectedForShiftDrag: ensureNodeSelectedForShiftDragMock + }) + }) +) + +vi.mock('@/composables/graph/useVueNodeLifecycle', () => ({ + useVueNodeLifecycle: () => ({ + nodeManager: ref({ + getNode: vi.fn((id: string) => ({ + id, + selected: false // Default to not selected + })) + }) + }) +})) + const createMockVueNodeData = ( overrides: Partial = {} ): VueNodeData => ({ @@ -72,6 +108,7 @@ const createMouseEvent = ( describe('useNodePointerInteractions', () => { beforeEach(async () => { vi.clearAllMocks() + selectedItemsState.items = [] setActivePinia(createPinia()) // Reset layout store state between tests const { layoutStore } = await import( @@ -141,9 +178,16 @@ describe('useNodePointerInteractions', () => { ) // Test pointer cancel - selection happens on pointer down - pointerHandlers.onPointerdown(createPointerEvent('pointerdown')) + pointerHandlers.onPointerdown( + createPointerEvent('pointerdown', { clientX: 100, clientY: 100 }) + ) expect(mockOnNodeSelect).toHaveBeenCalledTimes(1) + // Simulate drag by moving pointer beyond threshold + pointerHandlers.onPointermove( + createPointerEvent('pointermove', { clientX: 110, clientY: 110 }) + ) + pointerHandlers.onPointercancel(createPointerEvent('pointercancel')) // Selection should have been called on pointer down only @@ -152,7 +196,13 @@ describe('useNodePointerInteractions', () => { mockOnNodeSelect.mockClear() // Test context menu during drag prevents default - pointerHandlers.onPointerdown(createPointerEvent('pointerdown')) + pointerHandlers.onPointerdown( + createPointerEvent('pointerdown', { clientX: 100, clientY: 100 }) + ) + // Simulate drag by moving pointer beyond threshold + pointerHandlers.onPointermove( + createPointerEvent('pointermove', { clientX: 110, clientY: 110 }) + ) const contextMenuEvent = createMouseEvent('contextmenu') const preventDefaultSpy = vi.spyOn(contextMenuEvent, 'preventDefault') @@ -193,8 +243,16 @@ describe('useNodePointerInteractions', () => { mockOnNodeSelect ) - // Start drag - pointerHandlers.onPointerdown(createPointerEvent('pointerdown')) + // Pointer down alone shouldn't set dragging state + pointerHandlers.onPointerdown( + createPointerEvent('pointerdown', { clientX: 100, clientY: 100 }) + ) + expect(layoutStore.isDraggingVueNodes.value).toBe(false) + + // Move pointer beyond threshold to start drag + pointerHandlers.onPointermove( + createPointerEvent('pointermove', { clientX: 110, clientY: 110 }) + ) await nextTick() expect(layoutStore.isDraggingVueNodes.value).toBe(true) @@ -273,14 +331,17 @@ describe('useNodePointerInteractions', () => { expect(mockOnNodeSelect).toHaveBeenCalledWith(downEvent, mockNodeData) expect(mockOnNodeSelect).toHaveBeenCalledTimes(1) - // Dragging state should be active - expect(layoutStore.isDraggingVueNodes.value).toBe(true) + // Dragging state should NOT be active yet + expect(layoutStore.isDraggingVueNodes.value).toBe(false) - // Move the pointer (start dragging) + // Move the pointer beyond threshold (start dragging) pointerHandlers.onPointermove( createPointerEvent('pointermove', { clientX: 150, clientY: 150 }) ) + // Now dragging state should be active + expect(layoutStore.isDraggingVueNodes.value).toBe(true) + // Selection should still only have been called once (on pointer down) expect(mockOnNodeSelect).toHaveBeenCalledTimes(1) @@ -292,4 +353,146 @@ describe('useNodePointerInteractions', () => { // Selection should still only have been called once expect(mockOnNodeSelect).toHaveBeenCalledTimes(1) }) + + it('on ctrl+click: calls toggleNodeSelectionAfterPointerUp on pointer up (not pointer down)', async () => { + const mockNodeData = createMockVueNodeData() + const mockOnNodeSelect = vi.fn() + + const { pointerHandlers } = useNodePointerInteractions( + ref(mockNodeData), + mockOnNodeSelect + ) + + // Pointer down with ctrl + const downEvent = createPointerEvent('pointerdown', { + ctrlKey: true, + clientX: 100, + clientY: 100 + }) + pointerHandlers.onPointerdown(downEvent) + + // On pointer down: toggle handler should NOT be called yet + expect(toggleNodeSelectionAfterPointerUpMock).not.toHaveBeenCalled() + + // Pointer up with ctrl (no drag - same position) + const upEvent = createPointerEvent('pointerup', { + ctrlKey: true, + clientX: 100, + clientY: 100 + }) + pointerHandlers.onPointerup(upEvent) + + // On pointer up: toggle handler IS called with correct params + expect(toggleNodeSelectionAfterPointerUpMock).toHaveBeenCalledWith( + mockNodeData.id, + { + wasSelectedAtPointerDown: false, + multiSelect: true + } + ) + }) + + it('on ctrl+drag: does NOT call toggleNodeSelectionAfterPointerUp', async () => { + const mockNodeData = createMockVueNodeData() + const mockOnNodeSelect = vi.fn() + + const { pointerHandlers } = useNodePointerInteractions( + ref(mockNodeData), + mockOnNodeSelect + ) + + // Pointer down with ctrl + const downEvent = createPointerEvent('pointerdown', { + ctrlKey: true, + clientX: 100, + clientY: 100 + }) + pointerHandlers.onPointerdown(downEvent) + + // Move beyond drag threshold + pointerHandlers.onPointermove( + createPointerEvent('pointermove', { + ctrlKey: true, + clientX: 110, + clientY: 110 + }) + ) + + // Pointer up after drag + const upEvent = createPointerEvent('pointerup', { + ctrlKey: true, + clientX: 110, + clientY: 110 + }) + pointerHandlers.onPointerup(upEvent) + + // When dragging: toggle handler should NOT be called + expect(toggleNodeSelectionAfterPointerUpMock).not.toHaveBeenCalled() + }) + + it('selects node when shift drag starts without multi selection', async () => { + selectedItemsState.items = [] + const mockNodeData = createMockVueNodeData() + const mockOnNodeSelect = vi.fn() + + const { pointerHandlers } = useNodePointerInteractions( + ref(mockNodeData), + mockOnNodeSelect + ) + + const pointerDownEvent = createPointerEvent('pointerdown', { + clientX: 0, + clientY: 0, + shiftKey: true + }) + + pointerHandlers.onPointerdown(pointerDownEvent) + + const pointerMoveEvent = createPointerEvent('pointermove', { + clientX: 10, + clientY: 10, + shiftKey: true + }) + + pointerHandlers.onPointermove(pointerMoveEvent) + + expect(ensureNodeSelectedForShiftDragMock).toHaveBeenCalledWith( + pointerMoveEvent, + mockNodeData, + false + ) + }) + + it('still ensures selection when shift drag starts with existing multi select', async () => { + selectedItemsState.items = [{ id: 'a' }, { id: 'b' }] + const mockNodeData = createMockVueNodeData() + const mockOnNodeSelect = vi.fn() + + const { pointerHandlers } = useNodePointerInteractions( + ref(mockNodeData), + mockOnNodeSelect + ) + + const pointerDownEvent = createPointerEvent('pointerdown', { + clientX: 0, + clientY: 0, + shiftKey: true + }) + + pointerHandlers.onPointerdown(pointerDownEvent) + + const pointerMoveEvent = createPointerEvent('pointermove', { + clientX: 10, + clientY: 10, + shiftKey: true + }) + + pointerHandlers.onPointermove(pointerMoveEvent) + + expect(ensureNodeSelectedForShiftDragMock).toHaveBeenCalledWith( + pointerMoveEvent, + mockNodeData, + false + ) + }) }) diff --git a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts index e9ca19e97b..3a9c452ea6 100644 --- a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts +++ b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts @@ -3,9 +3,12 @@ import type { MaybeRefOrGetter } from 'vue' import { isMiddlePointerInput } from '@/base/pointerUtils' import type { VueNodeData } from '@/composables/graph/useGraphNodeManager' +import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle' import { useCanvasInteractions } from '@/renderer/core/canvas/useCanvasInteractions' import { layoutStore } from '@/renderer/core/layout/store/layoutStore' import { useNodeLayout } from '@/renderer/extensions/vueNodes/layout/useNodeLayout' +import { useNodeEventHandlers } from '@/renderer/extensions/vueNodes/composables/useNodeEventHandlers' +import { isMultiSelectKey } from '@/renderer/extensions/vueNodes/utils/selectionUtils' export function useNodePointerInteractions( nodeDataMaybe: MaybeRefOrGetter, @@ -28,6 +31,9 @@ export function useNodePointerInteractions( // Use canvas interactions for proper wheel event handling and pointer event capture control const { forwardEventToCanvas, shouldHandleNodePointerEvents } = useCanvasInteractions() + const { toggleNodeSelectionAfterPointerUp, ensureNodeSelectedForShiftDrag } = + useNodeEventHandlers() + const { nodeManager } = useVueNodeLifecycle() const forwardMiddlePointerIfNeeded = (event: PointerEvent) => { if (!isMiddlePointerInput(event)) return false @@ -37,6 +43,8 @@ export function useNodePointerInteractions( // Drag state for styling const isDragging = ref(false) + const isPointerDown = ref(false) + const wasSelectedAtPointerDown = ref(false) // Track if node was selected when pointer down occurred const dragStyle = computed(() => { if (nodeData.value?.flags?.pinned) { return { cursor: 'default' } @@ -44,6 +52,7 @@ export function useNodePointerInteractions( return { cursor: isDragging.value ? 'grabbing' : 'grab' } }) const startPosition = ref({ x: 0, y: 0 }) + const DRAG_THRESHOLD = 3 // pixels const handlePointerDown = (event: PointerEvent) => { if (!nodeData.value) { @@ -66,8 +75,10 @@ export function useNodePointerInteractions( return } - // Record position for drag threshold calculation - startPosition.value = { x: event.clientX, y: event.clientY } + // Track if node was selected before this pointer down + // IMPORTANT: Read from actual LGraphNode, not nodeData, to get correct state + const lgNode = nodeManager.value?.getNode(nodeData.value.id) + wasSelectedAtPointerDown.value = lgNode?.selected ?? false onNodeSelect(event, nodeData.value) @@ -75,18 +86,35 @@ export function useNodePointerInteractions( return } - // Start drag using layout system - isDragging.value = true - - // Set Vue node dragging state for selection toolbox - layoutStore.isDraggingVueNodes.value = true + // Record position for drag threshold calculation + startPosition.value = { x: event.clientX, y: event.clientY } + isPointerDown.value = true + // Don't start drag yet - wait for pointer move to exceed threshold startDrag(event) } const handlePointerMove = (event: PointerEvent) => { if (forwardMiddlePointerIfNeeded(event)) return + // Check if we should start dragging (pointer moved beyond threshold) + if (isPointerDown.value && !isDragging.value) { + const dx = event.clientX - startPosition.value.x + const dy = event.clientY - startPosition.value.y + const distance = Math.sqrt(dx * dx + dy * dy) + + if (distance > DRAG_THRESHOLD && nodeData.value) { + // Start drag + isDragging.value = true + layoutStore.isDraggingVueNodes.value = true + ensureNodeSelectedForShiftDrag( + event, + nodeData.value, + wasSelectedAtPointerDown.value + ) + } + } + if (isDragging.value) { void handleDrag(event) } @@ -98,6 +126,8 @@ export function useNodePointerInteractions( */ const cleanupDragState = () => { isDragging.value = false + isPointerDown.value = false + wasSelectedAtPointerDown.value = false layoutStore.isDraggingVueNodes.value = false } @@ -128,12 +158,28 @@ export function useNodePointerInteractions( const handlePointerUp = (event: PointerEvent) => { if (forwardMiddlePointerIfNeeded(event)) return - if (isDragging.value) { + const wasDragging = isDragging.value + const multiSelect = isMultiSelectKey(event) + const canHandlePointer = shouldHandleNodePointerEvents.value + + if (wasDragging) { handleDragTermination(event, 'drag end') + } else { + // Clean up pointer state even if not dragging + isPointerDown.value = false + const wasSelected = wasSelectedAtPointerDown.value + wasSelectedAtPointerDown.value = false + + if (nodeData.value && canHandlePointer) { + toggleNodeSelectionAfterPointerUp(nodeData.value.id, { + wasSelectedAtPointerDown: wasSelected, + multiSelect + }) + } } // Don't handle pointer events when canvas is in panning mode - forward to canvas instead - if (!shouldHandleNodePointerEvents.value) { + if (!canHandlePointer) { forwardEventToCanvas(event) return } diff --git a/src/renderer/extensions/vueNodes/utils/selectionUtils.ts b/src/renderer/extensions/vueNodes/utils/selectionUtils.ts new file mode 100644 index 0000000000..21ae852a63 --- /dev/null +++ b/src/renderer/extensions/vueNodes/utils/selectionUtils.ts @@ -0,0 +1,10 @@ +/** + * Checks if a pointer/mouse event has multi-select modifier keys pressed. + * Multi-select keys are: Ctrl (Windows/Linux), Cmd (Mac), or Shift + * + * @param event - The pointer or mouse event to check + * @returns true if any multi-select modifier key is pressed + */ +export function isMultiSelectKey(event: PointerEvent | MouseEvent): boolean { + return event.ctrlKey || event.metaKey || event.shiftKey +} diff --git a/tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts b/tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts index f19104f0fa..c5d5032262 100644 --- a/tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts +++ b/tests-ui/tests/renderer/extensions/vueNodes/composables/useNodeEventHandlers.test.ts @@ -1,10 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { computed, shallowRef } from 'vue' -import { - type GraphNodeManager, - type VueNodeData, - useGraphNodeManager +import { useGraphNodeManager } from '@/composables/graph/useGraphNodeManager' +import type { + GraphNodeManager, + VueNodeData } from '@/composables/graph/useGraphNodeManager' import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle' import type { @@ -16,6 +16,8 @@ import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' import { useLayoutMutations } from '@/renderer/core/layout/operations/layoutMutations' import { useNodeEventHandlers } from '@/renderer/extensions/vueNodes/composables/useNodeEventHandlers' +const canvasSelectedItems = vi.hoisted(() => [] as Array<{ id?: string }>) + vi.mock('@/renderer/core/canvas/canvasStore', () => { const canvas: Partial = { select: vi.fn(), @@ -23,12 +25,13 @@ vi.mock('@/renderer/core/canvas/canvasStore', () => { deselectAll: vi.fn() } const updateSelectedItems = vi.fn() + const canvasStoreInstance = { + canvas: canvas as LGraphCanvas, + updateSelectedItems, + selectedItems: canvasSelectedItems + } return { - useCanvasStore: vi.fn(() => ({ - canvas: canvas as LGraphCanvas, - updateSelectedItems, - selectedItems: [] - })) + useCanvasStore: vi.fn(() => canvasStoreInstance) } }) @@ -89,6 +92,8 @@ describe('useNodeEventHandlers', () => { beforeEach(async () => { vi.restoreAllMocks() + vi.clearAllMocks() + canvasSelectedItems.length = 0 }) describe('handleNodeSelect', () => { @@ -109,11 +114,10 @@ describe('useNodeEventHandlers', () => { expect(updateSelectedItems).toHaveBeenCalledOnce() }) - it('should toggle selection on ctrl+click', () => { + it('on pointer down with ctrl+click: selects node immediately', () => { const { handleNodeSelect } = useNodeEventHandlers() const { canvas } = useCanvasStore() - // Test selecting unselected node with ctrl mockNode!.selected = false const ctrlClickEvent = new PointerEvent('pointerdown', { @@ -124,16 +128,23 @@ describe('useNodeEventHandlers', () => { handleNodeSelect(ctrlClickEvent, testNodeData) + // On pointer down with multi-select: bring to front + expect(mockLayoutMutations.bringNodeToFront).toHaveBeenCalledWith( + 'node-1' + ) + + // Selection happens immediately so dragging includes this node expect(canvas?.deselectAll).not.toHaveBeenCalled() expect(canvas?.select).toHaveBeenCalledWith(mockNode) + expect(canvas?.deselect).not.toHaveBeenCalled() }) - it('should deselect on ctrl+click of selected node', () => { + it('on pointer down with ctrl+click of selected node: brings node to front only', () => { const { handleNodeSelect } = useNodeEventHandlers() const { canvas } = useCanvasStore() - // Test deselecting selected node with ctrl mockNode!.selected = true + mockNode!.flags.pinned = false const ctrlClickEvent = new PointerEvent('pointerdown', { bubbles: true, @@ -143,15 +154,22 @@ describe('useNodeEventHandlers', () => { handleNodeSelect(ctrlClickEvent, testNodeData) - expect(canvas?.deselect).toHaveBeenCalledWith(mockNode) + // On pointer down: bring to front + expect(mockLayoutMutations.bringNodeToFront).toHaveBeenCalledWith( + 'node-1' + ) + + // But don't deselect yet (deferred to pointer up) + expect(canvas?.deselect).not.toHaveBeenCalled() expect(canvas?.select).not.toHaveBeenCalled() }) - it('should handle meta key (Cmd) on Mac', () => { + it('on pointer down with meta key (Cmd): selects node immediately', () => { const { handleNodeSelect } = useNodeEventHandlers() const { canvas } = useCanvasStore() mockNode!.selected = false + mockNode!.flags.pinned = false const metaClickEvent = new PointerEvent('pointerdown', { bubbles: true, @@ -161,8 +179,59 @@ describe('useNodeEventHandlers', () => { handleNodeSelect(metaClickEvent, testNodeData) + // On pointer down with meta key: bring to front + expect(mockLayoutMutations.bringNodeToFront).toHaveBeenCalledWith( + 'node-1' + ) + + // Selection happens immediately expect(canvas?.select).toHaveBeenCalledWith(mockNode) expect(canvas?.deselectAll).not.toHaveBeenCalled() + expect(canvas?.deselect).not.toHaveBeenCalled() + }) + + it('on pointer down with shift key: selects node immediately', () => { + const { handleNodeSelect } = useNodeEventHandlers() + const { canvas } = useCanvasStore() + + mockNode!.selected = false + mockNode!.flags.pinned = false + + const shiftClickEvent = new PointerEvent('pointerdown', { + bubbles: true, + shiftKey: true + }) + + handleNodeSelect(shiftClickEvent, testNodeData) + + // On pointer down with shift: bring to front + expect(mockLayoutMutations.bringNodeToFront).toHaveBeenCalledWith( + 'node-1' + ) + + // Selection happens immediately for shift-click as well + expect(canvas?.select).toHaveBeenCalledWith(mockNode) + expect(canvas?.deselectAll).not.toHaveBeenCalled() + expect(canvas?.deselect).not.toHaveBeenCalled() + }) + + it('keeps existing multi-selection when dragging selected node without modifiers', () => { + const { handleNodeSelect } = useNodeEventHandlers() + const { canvas } = useCanvasStore() + + mockNode!.selected = true + canvasSelectedItems.push({ id: 'node-1' }, { id: 'node-2' }) + + const event = new PointerEvent('pointerdown', { + bubbles: true, + ctrlKey: false, + metaKey: false + }) + + handleNodeSelect(event, testNodeData) + + expect(canvas?.deselectAll).not.toHaveBeenCalled() + expect(canvas?.select).not.toHaveBeenCalled() }) it('should bring node to front when not pinned', () => { @@ -189,4 +258,144 @@ describe('useNodeEventHandlers', () => { expect(mockLayoutMutations.bringNodeToFront).not.toHaveBeenCalled() }) }) + + describe('toggleNodeSelectionAfterPointerUp', () => { + it('on pointer up with multi-select: deselects node that was selected at pointer down', () => { + const { toggleNodeSelectionAfterPointerUp } = useNodeEventHandlers() + const { canvas, updateSelectedItems } = useCanvasStore() + + mockNode!.selected = true + + toggleNodeSelectionAfterPointerUp('node-1', { + wasSelectedAtPointerDown: true, + multiSelect: true + }) + + expect(canvas?.deselect).toHaveBeenCalledWith(mockNode) + expect(updateSelectedItems).toHaveBeenCalledOnce() + }) + + it('on pointer up with multi-select and node not previously selected: no-op', () => { + const { toggleNodeSelectionAfterPointerUp } = useNodeEventHandlers() + const { canvas, updateSelectedItems } = useCanvasStore() + + mockNode!.selected = true + + toggleNodeSelectionAfterPointerUp('node-1', { + wasSelectedAtPointerDown: false, + multiSelect: true + }) + + expect(canvas?.select).not.toHaveBeenCalled() + expect(updateSelectedItems).not.toHaveBeenCalled() + }) + + it('on pointer up without multi-select: collapses multi-selection to clicked node', () => { + const { toggleNodeSelectionAfterPointerUp } = useNodeEventHandlers() + const { canvas, updateSelectedItems } = useCanvasStore() + + mockNode!.selected = true + canvasSelectedItems.push({ id: 'node-1' }, { id: 'node-2' }) + + toggleNodeSelectionAfterPointerUp('node-1', { + wasSelectedAtPointerDown: true, + multiSelect: false + }) + + expect(canvas?.deselectAll).toHaveBeenCalledOnce() + expect(canvas?.select).toHaveBeenCalledWith(mockNode) + expect(updateSelectedItems).toHaveBeenCalledOnce() + }) + + it('on pointer up without multi-select: keeps single selection intact', () => { + const { toggleNodeSelectionAfterPointerUp } = useNodeEventHandlers() + const { canvas, updateSelectedItems } = useCanvasStore() + + mockNode!.selected = true + canvasSelectedItems.push({ id: 'node-1' }) + + toggleNodeSelectionAfterPointerUp('node-1', { + wasSelectedAtPointerDown: true, + multiSelect: false + }) + + expect(canvas?.deselectAll).not.toHaveBeenCalled() + expect(canvas?.select).not.toHaveBeenCalled() + expect(updateSelectedItems).not.toHaveBeenCalled() + }) + }) + + describe('ensureNodeSelectedForShiftDrag', () => { + it('does nothing when multi-select key is not pressed', () => { + const { ensureNodeSelectedForShiftDrag } = useNodeEventHandlers() + const { canvas } = useCanvasStore() + + const event = new PointerEvent('pointermove', { shiftKey: false }) + + ensureNodeSelectedForShiftDrag(event, testNodeData, false) + + expect(canvas?.select).not.toHaveBeenCalled() + expect(canvas?.deselectAll).not.toHaveBeenCalled() + }) + + it('selects node and clears existing selection when shift-dragging with no other selections', () => { + const { ensureNodeSelectedForShiftDrag } = useNodeEventHandlers() + const { canvas } = useCanvasStore() + + mockNode!.selected = false + + const event = new PointerEvent('pointermove', { shiftKey: true }) + + ensureNodeSelectedForShiftDrag(event, testNodeData, false) + + expect(canvas?.deselectAll).toHaveBeenCalledOnce() + expect(canvas?.select).toHaveBeenCalledWith(mockNode) + }) + + it('adds node to existing multi-selection without clearing other nodes', () => { + const { ensureNodeSelectedForShiftDrag } = useNodeEventHandlers() + const { canvas, selectedItems } = useCanvasStore() + + // Create mock Positionable objects for existing selection + const mockExisting1 = { + id: 'existing-1', + pos: [0, 0] as [number, number], + move: vi.fn(), + snapToGrid: vi.fn(), + boundingRect: vi.fn(() => [0, 0, 100, 100] as const) + } as unknown as LGraphNode + const mockExisting2 = { + id: 'existing-2', + pos: [0, 0] as [number, number], + move: vi.fn(), + snapToGrid: vi.fn(), + boundingRect: vi.fn(() => [0, 0, 100, 100] as const) + } as unknown as LGraphNode + selectedItems.push(mockExisting1, mockExisting2) + mockNode!.selected = false + if (canvas?.select) vi.mocked(canvas.select).mockClear() + if (canvas?.deselectAll) vi.mocked(canvas.deselectAll).mockClear() + + const event = new PointerEvent('pointermove', { shiftKey: true }) + + ensureNodeSelectedForShiftDrag(event, testNodeData, false) + + expect(canvas?.deselectAll).not.toHaveBeenCalled() + expect(canvas?.select).toHaveBeenCalledWith(mockNode) + }) + + it('does nothing if node is already selected (selection happened on pointer down)', () => { + const { ensureNodeSelectedForShiftDrag } = useNodeEventHandlers() + const { canvas } = useCanvasStore() + + mockNode!.selected = true + + const event = new PointerEvent('pointermove', { shiftKey: true }) + + ensureNodeSelectedForShiftDrag(event, testNodeData, false) + + expect(canvas?.select).not.toHaveBeenCalled() + expect(canvas?.deselectAll).not.toHaveBeenCalled() + }) + }) })