-
Notifications
You must be signed in to change notification settings - Fork 368
Fix clicks near DOMWidgets being swallowed #5253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎭 Playwright Test Results✅ All tests passed across all browsers! ⏰ Completed at: 08/31/2025, 03:48:01 AM UTC 📊 Test Reports by Browser🎉 Your tests are passing across all browsers! |
pointer.onDragStart = () => this.#startDraggingItems(node, pointer) | ||
pointer.onDragEnd = (e) => this.#processDraggedItems(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance this could be clobbering something?
@@ -2851,6 +2851,12 @@ export class LGraphCanvas | |||
|
|||
this.dirty_canvas = true | |||
} | |||
passPointerEvent(pointer: CanvasPointer, node: LGraphNode): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's an intentional mutation, I usually like to note that in the function name.
Something like augmentPointerDragEvents
maybe?
src/scripts/domWidget.ts
Outdated
pointer: CanvasPointer, | ||
node: LGraphNode, | ||
canvas: LGraphCanvas | ||
): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete side-note, what does the return boolean indicate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It indicates that the pointer event is handled and no more processing should be performed. Here, it just skips a bunch of legacy mouse event handling code and prevents a warning message from being printed in the console about the legacy code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it kind of emulates preventDefault or cancel on a true event?
src/scripts/domWidget.ts
Outdated
node: LGraphNode, | ||
canvas: LGraphCanvas | ||
): boolean { | ||
return canvas.passPointerEvent(pointer, node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to add listeners as a result of a different event here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to say this change is not good in its own right, but I wonder if there's also a bonafide bug/regression in the hit-testing? (Or maybe the cause is stated elsewhere and I am missing it -- apologies if so.)
It seems this line is using 6px:
ComfyUI_frontend/src/lib/litegraph/src/LGraphNode.ts
Lines 2140 to 2143 in c3c2681
if ( | |
widget.last_y !== undefined && | |
isInRectangle(x, y, 6, widget.last_y, w - 12, h) | |
) { |
Would the issue be solved by doing something like this:
getWidgetOnPos(canvasX: number, canvasY: number, includeDisabled = false): IBaseWidget | undefined {
// ... existing code ...
for (const widget of widgets) {
// ... visibility checks ...
// Use DOM widget margin instead of hardcoded 6px
const margin = widget.margin ?? 6 // DOM widgets have margin property
// ... other code ..
if (
widget.last_y !== undefined &&
isInRectangle(x, y, margin, widget.last_y, w - margin * 2, h)
) {
return widget
}
}
}
Will do some testing. I would also prefer this as I'm not completely confident about custom nodes using both domWidget + legacy mouse method. Code search was inconclusive. IIRC, the space between widgets will always be assigned to a widget, but the primary annoyance was clicks above the first widget. |
The removal of gap between widgets was intentional; it always felt like it was a bug to me. The glitchy feeling of it intensified when we added "drag link on widget => auto-convert to input" - caused flickering when you dragged a link over a node. After I added snap highlighting on the widgets, it -really- needed to go. |
bafdab8
to
506219d
Compare
🎨 Storybook Build Status❌ Build failed! ⏰ Completed at: 08/31/2025, 03:24:52 AM UTC 🔗 Links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
DOMWidgets do not perfectly align with their allocated space on a node.
Clicks on the element proper are never seen by litegraph, but there's a small border region where the LegacyWidget onClick method is called (See LGraphNode.getWidgetOnPos). To end users who will not see the console warning, it appears the click just gets swallowed.
This PR addresses the issue in two parts:It provides a method on canvas to set a pointer event to be handled as a passthroughIt sets DOMWidgets to use this method for clicks that are not handled by the element itself┆Issue is synchronized with this Notion page by Unito