-
Notifications
You must be signed in to change notification settings - Fork 653
Fix "Markdown" and "Text Preview" widgets mouse events #7907
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
base: main
Are you sure you want to change the base?
Changes from all commits
cd8c441
36d1174
c88b91b
1b5a093
c121f17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ function addMarkdownWidget( | |
| inputEl.classList.add('comfy-markdown') | ||
| const textarea = document.createElement('textarea') | ||
| inputEl.append(textarea) | ||
| const editorDom: HTMLElement = editor.view.dom | ||
|
|
||
| const widget = node.addDOMWidget(name, 'MARKDOWN', inputEl, { | ||
| getValue(): string { | ||
|
|
@@ -91,6 +92,42 @@ function addMarkdownWidget( | |
| } | ||
| }) | ||
|
|
||
| inputEl.addEventListener('wheel', (event: WheelEvent) => { | ||
| const deltaX = event.deltaX | ||
| const deltaY = event.deltaY | ||
|
|
||
| const canScrollYMarkdown = editorDom.scrollHeight > editorDom.clientHeight | ||
| const canScrollYTextarea = textarea.scrollHeight > textarea.clientHeight | ||
| const isHorizontal = Math.abs(deltaX) > Math.abs(deltaY) | ||
|
|
||
| // Prevent pinch zoom from zooming the page | ||
| if (event.ctrlKey) { | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| app.canvas.processMouseWheel(event) | ||
| return | ||
| } | ||
|
|
||
| // When gestures disabled: horizontal always goes to canvas (no horizontal scroll in textarea) | ||
| if (isHorizontal) { | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| app.canvas.processMouseWheel(event) | ||
| return | ||
| } | ||
|
|
||
| // Vertical scrolling when gestures disabled: let textarea scroll if scrollable | ||
| const isEditing = inputEl.classList.contains('editing') | ||
| if (isEditing ? canScrollYTextarea : canScrollYMarkdown) { | ||
| event.stopPropagation() | ||
| return | ||
| } | ||
|
|
||
| // If textarea can't scroll vertically, pass to canvas | ||
| event.preventDefault() | ||
| app.canvas.processMouseWheel(event) | ||
| }) | ||
|
Comment on lines
+95
to
+129
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider refactoring the wheel handler for better maintainability. The wheel event handler implements sophisticated logic that handles pinch-zoom, trackpad detection, horizontal/vertical scrolling, and canvas delegation. While the logic appears correct, the 50-line handler with multiple conditional branches could benefit from extraction into smaller, testable helper functions. Consider extracting decision logic into focused helper functions: ♻️ Suggested refactoring approach// Helper functions for wheel event routing decisions
function shouldPreventPinchZoom(event: WheelEvent): boolean {
return event.ctrlKey
}
function isLikelyTrackpadGesture(deltaX: number, deltaY: number): boolean {
return Math.abs(deltaX) > 0 || Math.abs(deltaY) < TRACKPAD_DETECTION_THRESHOLD
}
function shouldRouteToCanvas(
event: WheelEvent,
gesturesEnabled: boolean,
isEditing: boolean,
canScrollYMarkdown: boolean,
canScrollYTextarea: boolean
): { route: boolean; allowDefault: boolean } {
const { deltaX, deltaY } = event
const isHorizontal = Math.abs(deltaX) > Math.abs(deltaY)
const isLikelyTrackpad = isLikelyTrackpadGesture(deltaX, deltaY)
// Pinch zoom always goes to canvas
if (event.ctrlKey) {
return { route: true, allowDefault: false }
}
// Trackpad gestures when enabled
if (gesturesEnabled && isLikelyTrackpad) {
return { route: true, allowDefault: false }
}
// Horizontal scrolling
if (isHorizontal) {
return { route: true, allowDefault: false }
}
// Vertical: allow local scroll if possible
const canScroll = isEditing ? canScrollYTextarea : canScrollYMarkdown
if (canScroll) {
return { route: false, allowDefault: true }
}
// Default: route to canvas
return { route: true, allowDefault: false }
}
// Then in the event handler:
inputEl.addEventListener('wheel', (event: WheelEvent) => {
const gesturesEnabled = useSettingStore().get('LiteGraph.Pointer.TrackpadGestures')
const canScrollYMarkdown = editorDom.scrollHeight > editorDom.clientHeight
const canScrollYTextarea = textarea.scrollHeight > textarea.clientHeight
const isEditing = inputEl.classList.contains('editing')
const { route, allowDefault } = shouldRouteToCanvas(
event,
gesturesEnabled,
isEditing,
canScrollYMarkdown,
canScrollYTextarea
)
if (!allowDefault) {
event.preventDefault()
}
event.stopPropagation()
if (route) {
app.canvas.processMouseWheel(event)
}
})Benefits:
🤖 Prompt for AI Agents |
||
|
|
||
| return widget | ||
| } | ||
|
|
||
|
|
||
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.
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 600
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 122
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 238
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5069
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3749
In legacy navigation mode, wheel events are unconditionally forwarded to canvas, blocking local scroll despite
overflow-y-auto.The
handleWheelfunction forwards all wheel events to the canvas in legacy mode (line 50-52), callingpreventDefault()unconditionally. Since TextPreviewWidget doesn't use the[data-capture-wheel="true"]opt-out attribute, users cannot scroll long previews in legacy mode. Text selection/copy are unaffected (pointer events allow normal selection unless space+dragging).Consider either: (1) adding
data-capture-wheel="true"to the preview div to preserve scroll in all modes, or (2) adjusting canvas wheel handling to not block preview scrolling.🤖 Prompt for AI Agents