fix(files): Show confirmation dialog on delete hotkey press

apps/files/src/actions/deleteAction.spec.ts

@@ -4,6 +4,7 @@
  */
 
 import type { IView } from '@nextcloud/files'
+import type { TriggeredActionContext } from '../utils/actionUtils.ts'
 
 import axios from '@nextcloud/axios'
 import * as capabilities from '@nextcloud/capabilities'
@@ -336,6 +337,110 @@ describe('Delete action execute tests', () => {
 		expect(eventBus.emit).toBeCalledWith('files:node:deleted', file)
 	})
 
+	test('Delete action triggered by hotkey asks for confirmation', async () => {
+		// The confirmation dialog is disabled
+		expect(shouldAskForConfirmation()).toBe(false)
+
+		vi.spyOn(axios, 'delete')
+		vi.spyOn(eventBus, 'emit')
+
+		// Emulate the confirmation dialog to always confirm
+		const confirmMock = vi.fn().mockImplementation((a, b, c, resolve) => resolve(true))
+		window.OC = { dialogs: { confirmDestructive: confirmMock } }
+
+		const file = new File({
+			id: 1,
+			source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt',
+			owner: 'test',
+			mime: 'text/plain',
+			permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE,
+			root: '/files/test',
+		})
+
+		const exec = await action.exec({
+			nodes: [file],
+			view,
+			folder: {} as Folder,
+			contents: [],
+			trigger: 'hotkey',
+		} as TriggeredActionContext)
+
+		expect(confirmMock).toBeCalledTimes(1)
+
+		expect(exec).toBe(true)
+		expect(axios.delete).toBeCalledTimes(1)
+		expect(axios.delete).toBeCalledWith('https://cloud.domain.com/remote.php/dav/files/test/foobar.txt')
+
+		expect(eventBus.emit).toBeCalledTimes(1)
+		expect(eventBus.emit).toBeCalledWith('files:node:deleted', file)
+	})
+
+	test('Delete action triggered by hotkey is cancelled', async () => {
+		vi.spyOn(axios, 'delete')
+		vi.spyOn(eventBus, 'emit')
+
+		// Emulate the confirmation dialog to always cancel
+		const confirmMock = vi.fn().mockImplementation((a, b, c, resolve) => resolve(false))
+		window.OC = { dialogs: { confirmDestructive: confirmMock } }
+
+		const file = new File({
+			id: 1,
+			source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt',
+			owner: 'test',
+			mime: 'text/plain',
+			permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE,
+			root: '/files/test',
+		})
+
+		const exec = await action.exec({
+			nodes: [file],
+			view,
+			folder: {} as Folder,
+			contents: [],
+			trigger: 'hotkey',
+		} as TriggeredActionContext)
+
+		expect(confirmMock).toBeCalledTimes(1)
+
+		expect(exec).toBe(null)
+		expect(axios.delete).toBeCalledTimes(0)
+		expect(eventBus.emit).toBeCalledTimes(0)
+	})
+
+	test('Delete action triggered from the menu does not ask for confirmation', async () => {
+		// The confirmation dialog is disabled
+		expect(shouldAskForConfirmation()).toBe(false)
+
+		vi.spyOn(axios, 'delete')
+		vi.spyOn(eventBus, 'emit')
+
+		const confirmMock = vi.fn()
+		window.OC = { dialogs: { confirmDestructive: confirmMock } }
+
+		const file = new File({
+			id: 1,
+			source: 'https://cloud.domain.com/remote.php/dav/files/test/foobar.txt',
+			owner: 'test',
+			mime: 'text/plain',
+			permissions: Permission.READ | Permission.UPDATE | Permission.WRITE | Permission.DELETE,
+			root: '/files/test',
+		})
+
+		const exec = await action.exec({
+			nodes: [file],
+			view,
+			folder: {} as Folder,
+			contents: [],
+			trigger: 'menu',
+		} as TriggeredActionContext)
+
+		expect(confirmMock).toBeCalledTimes(0)
+
+		expect(exec).toBe(true)
+		expect(axios.delete).toBeCalledTimes(1)
+		expect(axios.delete).toBeCalledWith('https://cloud.domain.com/remote.php/dav/files/test/foobar.txt')
+	})
+
 	test('Delete action batch', async () => {
 		vi.spyOn(axios, 'delete')
 		vi.spyOn(eventBus, 'emit')

apps/files/src/actions/deleteAction.ts

@@ -4,6 +4,7 @@
  */
 
 import type { IFileAction } from '@nextcloud/files'
+import type { TriggeredActionContext } from '../utils/actionUtils.ts'
 
 import CloseSvg from '@mdi/svg/svg/close.svg?raw'
 import NetworkOffSvg from '@mdi/svg/svg/network-off.svg?raw'
@@ -51,17 +52,12 @@ export const action: IFileAction = {
 			.every((permission) => (permission & Permission.DELETE) !== 0)
 	},
 
-	async exec({ nodes, view }) {
+	async exec({ nodes, view, trigger }: TriggeredActionContext) {
 		try {
 			let confirm = true
 
-			// Trick to detect if the action was called from a keyboard event
-			// we need to make sure the method calling have its named containing 'keydown'
-			// here we use `onKeydown` method from the FileEntryActions component
-			const callStack = new Error().stack || ''
-			const isCalledFromEventListener = callStack.toLocaleLowerCase().includes('keydown')
-
-			if (shouldAskForConfirmation() || isCalledFromEventListener) {
+			// Deleting via the hotkey is easy to trigger by accident, so always confirm it
+			if (shouldAskForConfirmation() || trigger === 'hotkey') {
 				confirm = await askConfirmation([nodes[0]], view)
 			}
 

apps/files/src/components/FileEntry/FileEntryActions.vue

@@ -338,7 +338,7 @@ export default defineComponent({
 			this.activeStore.activeNode = this.source
 
 			// Execute the action
-			await executeAction(action)
+			await executeAction(action, 'menu')
 		},
 
 		onKeyDown(event: KeyboardEvent) {

apps/files/src/composables/useHotKeys.spec.ts

@@ -145,6 +145,17 @@ describe('HotKeysService testing', () => {
 		expect(deleteAction.exec).toHaveBeenCalledOnce()
 	})
 
+	it('passes the hotkey trigger to the action', () => {
+		component.destroy()
+		registerFileAction(deleteAction)
+		component = mount(TestComponent)
+
+		dispatchEvent({ key: 'Delete', code: 'Delete' })
+
+		expect(deleteAction.exec).toHaveBeenCalledOnce()
+		expect(deleteAction.exec).toHaveBeenCalledWith(expect.objectContaining({ trigger: 'hotkey' }))
+	})
+
 	// actions implemented by the composable
 
 	it('Pressing alt+up should go to parent directory', () => {

apps/files/src/composables/useHotKeys.ts

@@ -33,7 +33,7 @@ export function useHotKeys(): void {
 			: action.hotkey.key
 
 		logger.debug(`Register hotkey for action "${action.id}"`)
-		useHotKey(key, () => executeAction(action), {
+		useHotKey(key, () => executeAction(action, 'hotkey'), {
 			stop: true,
 			prevent: true,
 			alt: action.hotkey.alt,

apps/files/src/utils/actionUtils.ts

@@ -11,12 +11,26 @@ import Vue from 'vue'
 import { useActiveStore } from '../store/active.ts'
 import { logger } from '../utils/logger.ts'
 
+/**
+ * How the execution of an action was triggered.
+ * `hotkey` means the action was triggered by a keyboard shortcut,
+ * `menu` means it was triggered from the actions menu or an inline action button.
+ */
+export type ActionTrigger = 'hotkey' | 'menu'
+
+/**
+ * Action context enriched with the trigger that started the execution.
+ * The trigger is undefined if the action was executed programmatically.
+ */
+export type TriggeredActionContext = ActionContextSingle & { trigger?: ActionTrigger }
+
 /**
  * Execute an action on the current active node
  *
  * @param action The action to execute
+ * @param trigger How the execution was triggered
  */
-export async function executeAction(action: IFileAction) {
+export async function executeAction(action: IFileAction, trigger?: ActionTrigger) {
 	const activeStore = useActiveStore()
 	const currentFolder = activeStore.activeFolder
 	const currentNode = activeStore.activeNode
@@ -39,7 +53,8 @@ export async function executeAction(action: IFileAction) {
 		view: currentView,
 		folder: currentFolder,
 		contents,
-	} as ActionContextSingle
+		trigger,
+	} as TriggeredActionContext
 
 	if (!action.enabled!(context)) {
 		logger.debug('Action is not not available for the current context', { action, node: currentNode, view: currentView })

tests/playwright/e2e/files/hotkeys.spec.ts

@@ -62,13 +62,31 @@ test.describe('Files hotkey handling', () => {
 		await expect(filesListPage.getFavoriteIconForFile('abcd')).toHaveCount(0)
 	})
 
-	test('Pressing DELETE should delete the folder', async ({ page, filesListPage }) => {
+	test('Pressing DELETE should delete the folder after confirmation', async ({ page, filesListPage }) => {
 		await filesListPage.getFilesList().press('ArrowDown')
 		await expect(page).toHaveURL(/\/apps\/files\/files\/\d+/)
 		await expect(filesListPage.getRows()).toHaveCount(2)
 
 		await filesListPage.getFilesList().press('Delete')
 
+		await page.getByRole('dialog', { name: 'Confirm deletion' })
+			.getByRole('button', { name: 'Delete folder' })
+			.click()
+
 		await expect(filesListPage.getRows()).toHaveCount(1)
 	})
+
+	test('Cancelling the confirmation of the DELETE hotkey keeps the folder', async ({ page, filesListPage }) => {
+		await filesListPage.getFilesList().press('ArrowDown')
+		await expect(page).toHaveURL(/\/apps\/files\/files\/\d+/)
+		await expect(filesListPage.getRows()).toHaveCount(2)
+
+		await filesListPage.getFilesList().press('Delete')
+
+		const dialog = page.getByRole('dialog', { name: 'Confirm deletion' })
+		await dialog.getByRole('button', { name: 'Cancel' }).click()
+
+		await expect(dialog).toBeHidden()
+		await expect(filesListPage.getRows()).toHaveCount(2)
+	})
 })