mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-21 21:09:00 +00:00
fix: harden pointer capture release and drag state for anomalous Mac events
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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}']
|
||||
|
||||
@@ -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
|
||||
|
||||
199
src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts
Normal file
199
src/lib/litegraph/src/LGraphCanvas.pointerCapture.test.ts
Normal file
@@ -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<typeof vi.fn>
|
||||
hasPointerCapture: ReturnType<typeof vi.fn>
|
||||
} {
|
||||
const el = document.createElement('canvas')
|
||||
el.width = 800
|
||||
el.height = 600
|
||||
|
||||
// Track captured pointer IDs
|
||||
const capturedPointers = new Set<number>()
|
||||
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<CanvasRenderingContext2D>
|
||||
|
||||
el.getContext = vi
|
||||
.fn()
|
||||
.mockReturnValue(fromAny<CanvasRenderingContext2D, unknown>(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<PointerEventInit> & { 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<typeof vi.fn>
|
||||
|
||||
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()
|
||||
})
|
||||
})
|
||||
@@ -3784,8 +3784,19 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
|
||||
* 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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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[] = []
|
||||
|
||||
Reference in New Issue
Block a user