Skip to content

Conversation

@christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Sep 15, 2025

Summary

Integrated Vue node components with canvas panning mode to prevent UI interference during navigation.

Changes

  • What: Added canCapturePointerEvents computed property to useCanvasInteractions composable that checks canvas read-only state
  • What: Modified Vue node components (LGraphNode, NodeWidgets) to conditionally handle pointer events based on canvas navigation mode
  • What: Updated node event handlers to respect panning mode and forward events to canvas when appropriate

Review Focus

Event forwarding logic in panning mode and pointer event capture state management across Vue node hierarchy.

graph TD
    A[User Interaction] --> B{Canvas in Panning Mode?}
    B -->|Yes| C[Forward to Canvas]
    B -->|No| D[Handle in Vue Component]
    C --> E[Canvas Navigation]
    D --> F[Node Selection/Widget Interaction]

    G[canCapturePointerEvents] --> H{read_only === false}
    H -->|Yes| I[Allow Vue Events]
    H -->|No| J[Block Vue Events]

    style A fill:#f9f9f9,stroke:#333,color:#000
    style E fill:#f9f9f9,stroke:#333,color:#000
    style F fill:#f9f9f9,stroke:#333,color:#000
    style I fill:#e1f5fe,stroke:#01579b,color:#000
    style J fill:#ffebee,stroke:#c62828,color:#000
Loading

Screenshots

2025-09-14.20-27-33_processed_20250914_202818.mp4

┆Issue is synchronized with this Notion page by Unito

@christian-byrne christian-byrne requested a review from a team as a code owner September 15, 2025 03:52
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 15, 2025
@github-actions
Copy link

github-actions bot commented Sep 15, 2025

🎭 Playwright Test Results

All tests passed!

⏰ Completed at: 09/18/2025, 10:28:34 PM UTC

📈 Summary

  • Total Tests: 450
  • Passed: 421 ✅
  • Failed: 0
  • Flaky: 0
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 414 / ❌ 0 / ⚠️ 0 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@christian-byrne christian-byrne changed the title Vue node panning mode integration Make Vue nodes read-only when in panning mode Sep 15, 2025
@christian-byrne christian-byrne requested a review from a team as a code owner September 15, 2025 04:31
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 15, 2025
@christian-byrne christian-byrne force-pushed the feat/vue-nodes-panning-mode-integration branch from 2c35c25 to 3c48f03 Compare September 15, 2025 05:38
@github-actions
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

DrJKL
DrJKL previously approved these changes Sep 16, 2025
* Whether Vue components can capture pointer events.
* Returns false when canvas is in read-only/panning mode.
*/
const canCapturePointerEvents = computed(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor naming concern, it's unclear from just the name and the fact that this is in useCanvasInteractions which elements can capture pointer events and whether that means they will capture pointer events

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to shouldHandleNodePointerEvents for better clarity in 690f6d2

}
// Don't capture pointer events when canvas is in panning mode - forward to canvas instead
if (!canCapturePointerEvents.value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's more do capture than can capture?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! Renamed to shouldHandleNodePointerEvents in 690f6d2

<div v-else class="lg-node-widgets flex flex-col gap-2 pr-4">
<div
v-else
:class="[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:class="[
:class="cn(
'lg-node-widgets flex flex-col gap-2 pr-4',
canCapturePointerEvents ? 'pointer-events-auto' : 'pointer-events-none'
)"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use cn() utility in 690f6d2

nodeData: VueNodeData,
wasDragging: boolean
) => {
if (!canCapturePointerEvents.value) return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally when a boolean value is only ever used negated, I think it's !(backwards setting a expressing)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe disableNodePointerEvents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Renamed to shouldHandleNodePointerEvents to better reflect the intent in 690f6d2

Vue nodes now respect canvas panning mode and disable interactions during:
- Hand tool activation (Comfy.Canvas.Lock command)
- Space key temporary panning
- Canvas read-only state

This ensures Vue nodes behave consistently with native LiteGraph nodes by:
- Disabling pointer event capture via CSS pointer-events
- Forwarding events to canvas for proper panning functionality
- Preventing widget interactions (SelectWidget, InputNumber, InputText, etc.)
- Disabling hover effects during panning mode
- Blocking node selection, collapse, title editing, and slot connections

Implementation centralizes panning state logic in useCanvasInteractions composable
and applies event masking at both node and widget container levels.
Add mock for useCanvasInteractions dependency that was introduced
in the panning mode integration. The test now properly mocks
canCapturePointerEvents to allow testing of existing functionality.
Move useCanvasInteractions from src/composables/graph/ to src/renderer/core/canvas/
to follow Domain-Driven Design principles. Canvas interactions are specifically
about canvas behavior and should be grouped with other canvas-related functionality.

Also move the corresponding test file to maintain the same directory structure.

Updated all import paths across:
- Vue components (GraphCanvas, GraphCanvasMenu, SelectionToolbox, etc.)
- Vue node components (LGraphNode, NodeWidgets)
- Composables (useNodeEventHandlers, useNodeImage)
- Test files and mocks
@christian-byrne christian-byrne force-pushed the feat/vue-nodes-panning-mode-integration branch from 690f6d2 to 5444718 Compare September 18, 2025 22:17
@github-actions
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@christian-byrne christian-byrne merged commit bc85d4e into main Sep 18, 2025
2 checks passed
@christian-byrne christian-byrne deleted the feat/vue-nodes-panning-mode-integration branch September 18, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:node-interaction area:vue-migration size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants