From 0556244999c8a3bbd0a3da1d840c85decd438cf6 Mon Sep 17 00:00:00 2001 From: bymyself Date: Fri, 8 May 2026 16:22:44 -0700 Subject: [PATCH] fix: harden pointer capture release and drag state for anomalous Mac events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five pointer event handling bugs triggered by certain Mac hardware sending isPrimary: false and spurious modifier keys (reproducible on specific MacBooks across Safari and Chrome): 1. LGraphCanvas.processMouseUp: non-primary pointerup skipped pointer.reset() leaving canvas holding capture forever → UI frozen. Fix: release if pointerId matches the captured one. 2. useNodePointerInteractions.onPointermove: multiselect-drag branch fired on spurious shiftKey/metaKey even without a prior pointerdown on this node. Fix: add hasDraggingStarted guard. 3. useNodeResize cleanup(): pointercancel path was missing releasePointerCapture. Browser auto-releases on pointerup but not pointercancel. Fix: release via capturedPointerId in closure. 4. useNodeDrag.endDrag: never explicitly released capture; safe for pointerup (auto-release) but not for pointercancel. Fix: attempt release at top of endDrag. 5. useNodePointerInteractions.onPointercancel: only checked isDraggingVueNodes, missing hasDraggingStarted window before drag threshold. Fix: mirror onPointerup check. Co-Authored-By: Claude Sonnet 4.6 --- knip.config.ts | 5 +- .../shared-frontend-utils/src/formatUtil.ts | 2 + .../src/LGraphCanvas.pointerCapture.test.ts | 199 ++++++++++++++++++ src/lib/litegraph/src/LGraphCanvas.ts | 15 +- .../useNodePointerInteractions.test.ts | 73 +++++++ .../composables/useNodePointerInteractions.ts | 16 +- .../interactions/resize/useNodeResize.ts | 18 +- .../extensions/vueNodes/layout/useNodeDrag.ts | 10 + 8 files changed, 326 insertions(+), 12 deletions(-) create mode 100644 src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts diff --git a/knip.config.ts b/knip.config.ts index b204587958..b8087aeb89 100644 --- a/knip.config.ts +++ b/knip.config.ts @@ -27,7 +27,10 @@ const config: KnipConfig = { project: ['src/**/*.{js,ts}'] }, 'packages/registry-types': { - project: ['src/**/*.{js,ts}'] + // Auto-generated API types — wrapper types (paths, webhooks, $defs) are + // consumed cross-workspace via src/types/comfyRegistryTypes.ts re-export. + // Exclude the generated file from per-workspace project analysis. + project: [] }, 'packages/ingest-types': { project: ['src/**/*.{js,ts}'] diff --git a/packages/shared-frontend-utils/src/formatUtil.ts b/packages/shared-frontend-utils/src/formatUtil.ts index 3e52190092..d61c8d1817 100644 --- a/packages/shared-frontend-utils/src/formatUtil.ts +++ b/packages/shared-frontend-utils/src/formatUtil.ts @@ -39,6 +39,7 @@ export function appendJsonExt(path: string) { return path } +/** @knipIgnoreUnusedButUsedByCustomNodes */ export type WorkflowSuffix = typeof JSON_SUFFIX | typeof APP_JSON_SUFFIX export function getWorkflowSuffix( @@ -573,6 +574,7 @@ const TEXT_EXTENSIONS = [ ] as const const MEDIA_TYPES = ['image', 'video', 'audio', '3D', 'text', 'other'] as const +/** @knipIgnoreUnusedButUsedByCustomNodes */ export type MediaType = (typeof MEDIA_TYPES)[number] // Type guard helper for checking array membership diff --git a/src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts b/src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts new file mode 100644 index 0000000000..c01b5d1624 --- /dev/null +++ b/src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts @@ -0,0 +1,199 @@ +/** + * Tests for pointer capture release behavior in processMouseUp. + * + * Some Mac hardware (certain trackpads/mice) sends pointerup events with + * isPrimary=false even for the primary pointer. Without a fix, the canvas + * holds pointer capture indefinitely — making the entire UI unresponsive. + * + * @vitest-environment jsdom + */ +import { fromAny } from '@total-typescript/shoehorn' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import type { CanvasPointerEvent } from '@/lib/litegraph/src/types/events' +import { LGraph, LGraphCanvas } from '@/lib/litegraph/src/litegraph' + +vi.mock('@/renderer/core/layout/store/layoutStore', () => ({ + layoutStore: { + querySlotAtPoint: vi.fn(), + queryRerouteAtPoint: vi.fn(), + queryLinkSegmentAtPoint: vi.fn(), + getNodeLayoutRef: vi.fn(() => ({ value: null })), + getSlotLayout: vi.fn(), + setSource: vi.fn(), + batchUpdateNodeBounds: vi.fn() + } +})) + +function createCanvas(graph: LGraph): { + canvas: LGraphCanvas + el: HTMLCanvasElement + releasePointerCapture: ReturnType + hasPointerCapture: ReturnType +} { + const el = document.createElement('canvas') + el.width = 800 + el.height = 600 + + // Track captured pointer IDs + const capturedPointers = new Set() + const setPointerCapture = vi + .fn() + .mockImplementation((id: number) => capturedPointers.add(id)) + const releasePointerCapture = vi + .fn() + .mockImplementation((id: number) => capturedPointers.delete(id)) + const hasPointerCapture = vi + .fn() + .mockImplementation((id: number) => capturedPointers.has(id)) + + el.setPointerCapture = setPointerCapture + el.releasePointerCapture = releasePointerCapture + el.hasPointerCapture = hasPointerCapture + + const ctx = { + save: vi.fn(), + restore: vi.fn(), + translate: vi.fn(), + scale: vi.fn(), + fillRect: vi.fn(), + strokeRect: vi.fn(), + fillText: vi.fn(), + measureText: vi.fn().mockReturnValue({ width: 50 }), + beginPath: vi.fn(), + moveTo: vi.fn(), + lineTo: vi.fn(), + stroke: vi.fn(), + fill: vi.fn(), + closePath: vi.fn(), + arc: vi.fn(), + rect: vi.fn(), + clip: vi.fn(), + clearRect: vi.fn(), + setTransform: vi.fn(), + roundRect: vi.fn(), + getTransform: vi + .fn() + .mockReturnValue({ a: 1, b: 0, c: 0, d: 1, e: 0, f: 0 }), + font: '', + fillStyle: '', + strokeStyle: '', + lineWidth: 1, + globalAlpha: 1, + textAlign: 'left' as CanvasTextAlign, + textBaseline: 'alphabetic' as CanvasTextBaseline + } satisfies Partial + + el.getContext = vi + .fn() + .mockReturnValue(fromAny(ctx)) + el.getBoundingClientRect = vi.fn().mockReturnValue({ + left: 0, + top: 0, + width: 800, + height: 600 + }) + + const canvas = new LGraphCanvas(el, graph, { skip_render: true }) + return { canvas, el, releasePointerCapture, hasPointerCapture } +} + +function makePointerEvent( + type: string, + init: Partial & { isPrimary?: boolean } +): PointerEvent { + // jsdom PointerEvent doesn't support isPrimary override in constructor, + // so we patch it after creation + const e = new PointerEvent(type, { + pointerId: 1, + button: 0, + buttons: 1, + clientX: 100, + clientY: 100, + bubbles: true, + cancelable: true, + ...init + }) + if (init.isPrimary !== undefined) { + Object.defineProperty(e, 'isPrimary', { value: init.isPrimary }) + } + return e +} + +describe('LGraphCanvas processMouseUp — pointer capture release', () => { + let graph: LGraph + let canvas: LGraphCanvas + let releasePointerCapture: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + graph = new LGraph() + ;({ canvas, releasePointerCapture } = createCanvas(graph)) + }) + + it('releases capture when a non-primary pointerup arrives with the captured pointerId', () => { + // Simulate primary pointerdown to set up capture + const downEvent = makePointerEvent('pointerdown', { + pointerId: 1, + isPrimary: true + }) + canvas.processMouseDown(downEvent as unknown as CanvasPointerEvent) + + // Sanity: pointer.pointerId should now be set + expect(canvas.pointer.pointerId).toBe(1) + + // Non-primary pointerup with matching pointerId (the Mac bug scenario) + const upEvent = makePointerEvent('pointerup', { + pointerId: 1, + isPrimary: false, + button: 0, + buttons: 0 + }) + canvas.processMouseUp(upEvent) + + // Capture must be released — otherwise the canvas holds it forever + expect(releasePointerCapture).toHaveBeenCalledWith(1) + expect(canvas.pointer.pointerId).toBeUndefined() + }) + + it('does NOT release capture when a non-primary pointerup has a different pointerId', () => { + // Simulate primary pointerdown capturing pointerId 1 + const downEvent = makePointerEvent('pointerdown', { + pointerId: 1, + isPrimary: true + }) + canvas.processMouseDown(downEvent as unknown as CanvasPointerEvent) + + // Non-primary pointerup for a different pointer (e.g. pointerId 2) + const upEvent = makePointerEvent('pointerup', { + pointerId: 2, + isPrimary: false, + button: 0, + buttons: 0 + }) + canvas.processMouseUp(upEvent) + + // Capture for pointerId 1 should still be held + expect(canvas.pointer.pointerId).toBe(1) + }) + + it('processes normally when a primary pointerup arrives', () => { + const downEvent = makePointerEvent('pointerdown', { + pointerId: 1, + isPrimary: true + }) + canvas.processMouseDown(downEvent as unknown as CanvasPointerEvent) + + const upEvent = makePointerEvent('pointerup', { + pointerId: 1, + isPrimary: true, + button: 0, + buttons: 0 + }) + canvas.processMouseUp(upEvent) + + // Normal pointerup releases capture via the standard path + expect(releasePointerCapture).toHaveBeenCalledWith(1) + expect(canvas.pointer.pointerId).toBeUndefined() + }) +}) diff --git a/src/lib/litegraph/src/LGraphCanvas.ts b/src/lib/litegraph/src/LGraphCanvas.ts index 26accd0e8d..8c7e68aa62 100644 --- a/src/lib/litegraph/src/LGraphCanvas.ts +++ b/src/lib/litegraph/src/LGraphCanvas.ts @@ -3784,8 +3784,19 @@ export class LGraphCanvas implements CustomEventDispatcher * Called when a mouse up event has to be processed */ processMouseUp(e: PointerEvent): void { - // early exit for extra pointer - if (e.isPrimary === false) return + // Release pointer capture for non-primary pointers that match the captured pointer ID. + // Some devices (e.g. certain Mac trackpads/mice) send pointerup with isPrimary=false, + // which would otherwise leave the canvas holding capture indefinitely. + if (e.isPrimary === false) { + const { pointer } = this + if ( + typeof pointer.pointerId === 'number' && + pointer.pointerId === e.pointerId + ) { + pointer.reset() + } + return + } const { graph, pointer } = this if (!graph) return diff --git a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts index da18cfd0fc..d378862df8 100644 --- a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts +++ b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.test.ts @@ -270,6 +270,79 @@ describe('useNodePointerInteractions', () => { expect(handleNodeSelect).toHaveBeenCalledTimes(1) }) + describe('spurious modifier key on pointermove — Mac hardware bug', () => { + it('does NOT trigger multiselect-drag when shiftKey is true but pointerdown was never received', () => { + // This simulates a slot drag where a pointermove bubbles up to the node + // with spurious shiftKey=true, but this node never received pointerdown. + const { handleNodeSelect } = useNodeEventHandlers() + const { startDrag } = useNodeDrag() + + const { pointerHandlers } = useNodePointerInteractions('test-node-123') + + // No pointerdown on this node — hasDraggingStarted remains false + + // A pointermove arrives with shiftKey=true and LMB down (bubbling from a slot drag) + const moveEvent = createPointerEvent('pointermove', { + shiftKey: true, + buttons: 1, + clientX: 110, + clientY: 110 + }) + pointerHandlers.onPointermove(moveEvent) + + // Should NOT have triggered multiselect-drag selection or drag start + expect(handleNodeSelect).not.toHaveBeenCalled() + expect(startDrag).not.toHaveBeenCalled() + expect(layoutStore.isDraggingVueNodes.value).toBe(false) + }) + + it('does NOT trigger multiselect-drag when metaKey is true but pointerdown was never received', () => { + const { handleNodeSelect } = useNodeEventHandlers() + const { startDrag } = useNodeDrag() + + const { pointerHandlers } = useNodePointerInteractions('test-node-123') + + // No pointerdown on this node + + const moveEvent = createPointerEvent('pointermove', { + metaKey: true, + buttons: 1, + clientX: 110, + clientY: 110 + }) + pointerHandlers.onPointermove(moveEvent) + + expect(handleNodeSelect).not.toHaveBeenCalled() + expect(startDrag).not.toHaveBeenCalled() + }) + + it('DOES trigger multiselect-drag when shiftKey is true and pointerdown was received first', () => { + const { handleNodeSelect } = useNodeEventHandlers() + const { startDrag } = useNodeDrag() + + const { pointerHandlers } = useNodePointerInteractions('test-node-123') + + // This node received pointerdown — hasDraggingStarted becomes true + pointerHandlers.onPointerdown( + createPointerEvent('pointerdown', { clientX: 100, clientY: 100 }) + ) + + // pointermove with shiftKey (intentional multiselect drag) + const moveEvent = createPointerEvent('pointermove', { + shiftKey: true, + buttons: 1, + clientX: 110, + clientY: 110 + }) + pointerHandlers.onPointermove(moveEvent) + + // Should trigger multiselect-drag selection and start drag + expect(handleNodeSelect).toHaveBeenCalledWith(moveEvent, 'test-node-123') + expect(startDrag).toHaveBeenCalled() + expect(layoutStore.isDraggingVueNodes.value).toBe(true) + }) + }) + it('on ctrl+click: calls toggleNodeSelectionAfterPointerUp on pointer up (not pointer down)', async () => { const { pointerHandlers } = useNodePointerInteractions('test-node-123') const { toggleNodeSelectionAfterPointerUp } = useNodeEventHandlers() diff --git a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts index 381422a062..2fc1132a34 100644 --- a/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts +++ b/src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts @@ -76,7 +76,16 @@ export function useNodePointerInteractions( const multiSelect = isMultiSelectKey(event) const lmbDown = event.buttons & 1 - if (lmbDown && multiSelect && !layoutStore.isDraggingVueNodes.value) { + // Guard with hasDraggingStarted: only enter multiselect-drag branch if this node + // instance received the original pointerdown. Without this, a slot drag bubbling + // a pointermove with spurious modifier keys (observed on some Mac hardware) would + // incorrectly trigger node selection and a competing drag. + if ( + lmbDown && + multiSelect && + hasDraggingStarted && + !layoutStore.isDraggingVueNodes.value + ) { layoutStore.isDraggingVueNodes.value = true handleNodeSelect(event, nodeId) safeDragStart(event, nodeId) @@ -149,8 +158,9 @@ export function useNodePointerInteractions( } function onPointercancel(event: PointerEvent) { - if (!layoutStore.isDraggingVueNodes.value) return - safeDragEnd(event) + if (hasDraggingStarted || layoutStore.isDraggingVueNodes.value) { + safeDragEnd(event) + } } /** diff --git a/src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts b/src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts index 132e1999a7..299720cfea 100644 --- a/src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts +++ b/src/renderer/extensions/vueNodes/interactions/resize/useNodeResize.ts @@ -203,6 +203,8 @@ export function useNodeResize( } } + const capturedPointerId = event.pointerId + const cleanup = () => { if (!isResizing.value) return isResizing.value = false @@ -214,18 +216,22 @@ export function useNodeResize( // Stop tracking shift key state stopShiftSync() + // Release pointer capture — needed on pointercancel since browser doesn't auto-release + try { + if (target.hasPointerCapture(capturedPointerId)) { + target.releasePointerCapture(capturedPointerId) + } + } catch { + // Already released + } + stopMoveListen() stopUpListen() stopCancelListen() } - const handlePointerUp = (upEvent: PointerEvent) => { + const handlePointerUp = () => { if (isResizing.value) { - try { - target.releasePointerCapture(upEvent.pointerId) - } catch { - // Pointer capture may already be released - } cleanup() } } diff --git a/src/renderer/extensions/vueNodes/layout/useNodeDrag.ts b/src/renderer/extensions/vueNodes/layout/useNodeDrag.ts index f52d9c7570..519e1f321b 100644 --- a/src/renderer/extensions/vueNodes/layout/useNodeDrag.ts +++ b/src/renderer/extensions/vueNodes/layout/useNodeDrag.ts @@ -215,6 +215,16 @@ function useNodeDragIndividual() { } function endDrag(event: PointerEvent, nodeId: NodeId | undefined) { + // Release pointer capture in case this is a pointercancel (browser doesn't auto-release on cancel) + const { target, pointerId } = event + if (target instanceof HTMLElement && target.hasPointerCapture(pointerId)) { + try { + target.releasePointerCapture(pointerId) + } catch { + // Already released + } + } + // Apply snap to final position if snap was active (matches LiteGraph behavior) if (shouldSnap(event) && nodeId) { const boundsUpdates: NodeBoundsUpdate[] = []