[backport core/1.51] fix: keep Ctrl/Cmd shortcuts from falling through to the browser (#15478)

Backport of #15066 to `core/1.51`

Automatically created by backport workflow.

Co-authored-by: Christian Byrne <cbyrne@comfy.org>
Co-authored-by: Connor Byrne <c.byrne@comfy.org>
Co-authored-by: Alexander Brown <drjkl@comfy.org>

Author: 4 people

browser_tests/tests/defaultKeybindings.spec.ts

@@ -236,6 +236,33 @@ test.describe('Default Keybindings', { tag: '@keyboard' }, () => {
       await comfyPage.keyboard.press('Escape')
     })
 
+    for (const { label, key } of [
+      { label: 'Ctrl+s', key: 'Control+s' },
+      { label: 'Cmd+s', key: 'Meta+s' }
+    ]) {
+      test(`'${label}' does not fall through to the browser while a dialog is open`, async ({
+        comfyPage
+      }) => {
+        await comfyPage.keyboard.press('Control+s')
+        await expect(comfyPage.page.getByRole('dialog')).toBeVisible()
+
+        await comfyPage.page.evaluate(() => {
+          window.TestCommand = false
+          window.addEventListener('keydown', (event: KeyboardEvent) => {
+            if (event.key === 's') window.TestCommand = event.defaultPrevented
+          })
+        })
+
+        await comfyPage.keyboard.press(key)
+
+        await expect
+          .poll(() => comfyPage.page.evaluate(() => window.TestCommand))
+          .toBe(true)
+
+        await comfyPage.keyboard.press('Escape')
+      })
+    }
+
     test("'Ctrl+o' triggers open workflow", async ({ comfyPage }) => {
       // Ctrl+o calls app.ui.loadFile() which clicks a hidden file input.
       // Detect the file input click via an event listener.

src/platform/keybindings/keybindingService.dialog.test.ts

@@ -58,14 +58,15 @@ describe('keybindingService - dialog gate', () => {
 
   function createKeyboardEvent(
     key: string,
-    target: HTMLElement = document.body
+    target: HTMLElement = document.body,
+    modifiers: { ctrlKey?: boolean; metaKey?: boolean } = {}
   ): KeyboardEvent {
     const event = new KeyboardEvent('keydown', {
       key,
       bubbles: true,
-      cancelable: true
+      cancelable: true,
+      ...modifiers
     })
-    event.preventDefault = vi.fn()
     event.composedPath = vi.fn(() => [target])
     return event
   }
@@ -104,7 +105,7 @@ describe('keybindingService - dialog gate', () => {
       await keybindingService.keybindHandler(event)
 
       expect(mockCommandExecute).not.toHaveBeenCalled()
-      expect(event.preventDefault).not.toHaveBeenCalled()
+      expect(event.defaultPrevented).toBe(false)
     } finally {
       document.body.removeChild(dialog)
     }
@@ -121,7 +122,7 @@ describe('keybindingService - dialog gate', () => {
       await keybindingService.keybindHandler(event)
 
       expect(mockCommandExecute).not.toHaveBeenCalled()
-      expect(event.preventDefault).not.toHaveBeenCalled()
+      expect(event.defaultPrevented).toBe(false)
     } finally {
       document.body.removeChild(dialog)
     }
@@ -138,12 +139,32 @@ describe('keybindingService - dialog gate', () => {
       await keybindingService.keybindHandler(event)
 
       expect(mockCommandExecute).not.toHaveBeenCalled()
-      expect(event.preventDefault).not.toHaveBeenCalled()
+      expect(event.defaultPrevented).toBe(false)
     } finally {
       document.body.removeChild(dialog)
     }
   })
 
+  it.for([
+    { label: 'Ctrl+S', modifiers: { ctrlKey: true } },
+    { label: 'Meta+S', modifiers: { metaKey: true } }
+  ] as {
+    label: string
+    modifiers: { ctrlKey?: boolean; metaKey?: boolean }
+  }[])(
+    'still suppresses the browser default for $label while a dialog is open',
+    async ({ modifiers }) => {
+      const dialogStore = useDialogStore()
+      dialogStore.dialogStack.push(createTestDialogInstance('templates-dialog'))
+
+      const event = createKeyboardEvent('s', document.body, modifiers)
+      await keybindingService.keybindHandler(event)
+
+      expect(mockCommandExecute).not.toHaveBeenCalled()
+      expect(event.defaultPrevented).toBe(true)
+    }
+  )
+
   it('executes a global keybinding while a reka popover is open', async () => {
     const popper = document.createElement('div')
     popper.setAttribute('data-reka-popper-content-wrapper', '')

src/platform/keybindings/keybindingService.ts

@@ -51,6 +51,10 @@ export function useKeybindingService() {
         }
       }
       if (isModalOpen(dialogStore.dialogStack.length)) {
+        // Bare keys still have to reach inputs inside the dialog.
+        if (keyCombo.ctrl) {
+          event.preventDefault()
+        }
         return
       }