Fix Clicking on "Save Video" Node Video widget and press delete will trigger the entire graph to delete.

[BenraouaneSoufiane]

Hello, this pull fix this issue: #4216, I found the issue does not related with commandStore, keybindingService or even with node selection
the main reason that cause this issue is the video element capture the del press (keydown' event) instead litegraph.js, so I've remove the selected node manually when clicking the video then press del
Here's a video about the fix (sorry screenrec not works as expected this time)
https://youtu.be/z4YFia6YniY

[christian-byrne]

Thanks for working on this fix! I was looking at the issue and noticed that we already have keyboard event filtering logic in the keybinding service.

Would it be possible to handle this more simply by adding a check in src/services/keybindingService.ts after line 39? Something like:

// Prevent Delete key on media elements
if (event.key === 'Delete' && (target.tagName === 'VIDEO' || target.tagName === 'AUDIO')) {
  return
}

This would follow the same pattern we use for text inputs and would be just a few lines instead of the current approach. It seems like the keybinding service is already the central place where we filter keyboard events before they reach LiteGraph.

Also, it looks like the useContextMenuOverride composable might be unrelated to the video delete issue - could that potentially be moved to a separate PR?

[christian-byrne]

Also make sure to install with npm install and run some of the scripts like npm run lint:fix and npm run format

[BenraouaneSoufiane]

Thanks for working on this fix! I was looking at the issue and noticed that we already have keyboard event filtering logic in the keybinding service.

Would it be possible to handle this more simply by adding a check in src/services/keybindingService.ts after line 39? Something like:

// Prevent Delete key on media elements
if (event.key === 'Delete' && (target.tagName === 'VIDEO' || target.tagName === 'AUDIO')) {
  return
}

This would follow the same pattern we use for text inputs and would be just a few lines instead of the current approach. It seems like the keybinding service is already the central place where we filter keyboard events before they reach LiteGraph.

Also, it looks like the useContextMenuOverride composable might be unrelated to the video delete issue - could that potentially be moved to a separate PR?

Gooday, thank you for your reply/feedback, please keep in mind your suggested code prevent the deletion at all, do you need this? (I think it should delete only selected node, not stop it all), what do you think? remove my approache & use the suggested edit, or keeps it & ignore suggestion?

Sorry for the useContextMenuOverride, it was from other fix, anyway I've deleted it & undo changes concern graphcanvas.vue
Thanks

[BenraouaneSoufiane]

Also make sure to install with npm install and run some of the scripts like npm run lint:fix and npm run format

Concern the linting of my edits, I did't found ts errors, concern the formatting, I'm using prettier for formating is that good?

[christian-byrne]

You make a great point! You're right that my suggestion would prevent deletion entirely when focused on a video element.

Looking at the original issue more carefully, it seems the problem is that pressing Delete while focused on the video widget causes the entire graph to be deleted (not just selected nodes). This sounds like an unexpected behavior.

Could you clarify what the expected behavior should be when pressing Delete while focused on a video element? I see a few options:

1. Delete key does nothing when video has focus (my original suggestion)
2. Delete key removes only the selected nodes, not the entire graph (what your original code was doing)
3. Delete key should work on the video element itself somehow (though I'm not sure what that would mean for a video)

For option 2, if we want to preserve the normal node deletion behavior, we could let the event propagate to LiteGraph but first ensure the canvas has proper focus:

// In keybindingService.ts
if (event.key === 'Delete' && target.tagName === 'VIDEO') {
  // Blur the video to ensure LiteGraph handles deletion properly
  (target as HTMLVideoElement).blur()
  // Let the event continue to LiteGraph
}

What do you think is the best approach? Also, thanks for removing the unrelated context menu changes!

[BenraouaneSoufiane]

You make a great point! You're right that my suggestion would prevent deletion entirely when focused on a video element.

Looking at the original issue more carefully, it seems the problem is that pressing Delete while focused on the video widget causes the entire graph to be deleted (not just selected nodes). This sounds like an unexpected behavior.

Could you clarify what the expected behavior should be when pressing Delete while focused on a video element? I see a few options:

1. Delete key does nothing when video has focus (my original suggestion)
2. Delete key removes only the selected nodes, not the entire graph (what your original code was doing)
3. Delete key should work on the video element itself somehow (though I'm not sure what that would mean for a video)

For option 2, if we want to preserve the normal node deletion behavior, we could let the event propagate to LiteGraph but first ensure the canvas has proper focus:

// In keybindingService.ts
if (event.key === 'Delete' && target.tagName === 'VIDEO') {
  // Blur the video to ensure LiteGraph handles deletion properly
  (target as HTMLVideoElement).blur()
  // Let the event continue to LiteGraph
}

What do you think is the best approach? Also, thanks for removing the unrelated context menu changes!

Gooday, thank you for your reply, in fact there are no issue with deletion in litgraph because when debug the selected node when clicking the video inside save video or even load video, it shows that the relative node are selected, so the video prevent the element to arrive to the litegraph (the video player uses e.stopPropagation() somehow), before arrive at this point I tried blur() that let me to confirm what happening (tried to simulate the keydown manually, & other things...)
I think to keep my approach because I didn't found other way to resolve this issue

[BenraouaneSoufiane]

@benceruleanlu can you take a look/merge?

[kevinpaik1]

Hi @BenraouaneSoufiane as mentioned in email to you, I'll be closing this PR. Thank you for your effort and interest in our bounty program.