fix(core): don't hijack Ctrl+F in editors or behind modals

core/src/tests/components/UnifiedSearch.spec.ts

@@ -31,21 +31,55 @@ function factory() {
 }
 
 /**
- * Dispatch Ctrl+<key> on the window and report whether the handler claimed it.
+ * Dispatch Ctrl+<key> from an element and report whether the handler claimed it.
+ * The guard inspects `event.target`, so this cannot dispatch on the window.
+ *
+ * @param key The key to press
+ * @param target The element the keystroke originates from
  */
-function pressCtrl(key = 'k') {
+function pressCtrl(key = 'k', target: EventTarget = document.body) {
 	const event = new KeyboardEvent('keydown', { key, ctrlKey: true, bubbles: true, cancelable: true })
 	const prevented = vi.spyOn(event, 'preventDefault')
-	window.dispatchEvent(event)
+	target.dispatchEvent(event)
 	return prevented
 }
 
+/**
+ * jsdom has no layout, so the mask's visibility has to be faked.
+ *
+ * @param parent Element to append the mask to
+ */
+function addVisibleModalMask(parent: Element = document.body) {
+	const mask = document.createElement('div')
+	mask.classList.add('modal-mask')
+	parent.appendChild(mask)
+	Element.prototype.checkVisibility = () => true
+	return mask
+}
+
+/**
+ * jsdom does not derive isContentEditable from the attribute.
+ */
+function addContentEditable() {
+	const editor = document.createElement('div')
+	Object.defineProperty(editor, 'isContentEditable', { value: true })
+	document.body.appendChild(editor)
+	return editor
+}
+
 beforeEach(() => {
 	mobile.value = false
 	location.value = { pathname: '/' }
-	window.OCP = { Accessibility: { disableKeyboardShortcuts: () => true } }
+	// useHotKey reads the accessibility opt-out once, when its module is first imported,
+	// so it cannot be toggled per test. Opting out is the library's behaviour, not ours.
+	window.OCP = { Accessibility: { disableKeyboardShortcuts: () => false } }
+})
+afterEach(() => {
+	vi.clearAllMocks()
+	document.body.replaceChildren()
+	// @ts-expect-error Restore the jsdom default (absent).
+	delete Element.prototype.checkVisibility
 })
-afterEach(() => vi.clearAllMocks())
 
 describe('UnifiedSearch open-state model', () => {
 	it('desktop: typing opens, clearing closes', async () => {
@@ -85,13 +119,8 @@ describe('UnifiedSearch open-state model', () => {
 })
 
 describe('UnifiedSearch focus shortcut (Ctrl/Cmd+K)', () => {
-	function mountWithShortcuts() {
-		window.OCP = { Accessibility: { disableKeyboardShortcuts: () => false } }
-		return factory()
-	}
-
 	it('desktop: focuses the header input and claims the key', () => {
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
 		const prevented = pressCtrl()
@@ -103,67 +132,93 @@ describe('UnifiedSearch focus shortcut (Ctrl/Cmd+K)', () => {
 
 	it('mobile: opens the modal instead (no header input to focus)', () => {
 		mobile.value = true
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 
 		pressCtrl()
 
 		expect(wrapper.vm.showUnifiedSearch).toBe(true)
 		wrapper.destroy()
 	})
 
-	it('is not bound when the user disabled keyboard shortcuts', () => {
-		const wrapper = factory() // beforeEach leaves shortcuts disabled
+	// The allowlist means the page owns Ctrl+F, not Ctrl+K. Nothing on those pages binds
+	// Ctrl+K, so bailing out would hand it to the browser (Firefox opens its search bar).
+	it('still claims the key on pages that own Ctrl+F', () => {
+		location.value = { pathname: '/settings/users' }
+		const wrapper = factory()
+		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+
+		const prevented = pressCtrl()
+
+		expect(focusInput).toHaveBeenCalled()
+		expect(prevented).toHaveBeenCalled()
+		wrapper.destroy()
+	})
+
+	it('unbinds the shortcut when the component is torn down', () => {
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
+		wrapper.destroy()
 		pressCtrl()
 
 		expect(focusInput).not.toHaveBeenCalled()
+	})
+
+	// Under Caps Lock / Shift, event.key is 'K'. The shortcut must still fire.
+	it('fires regardless of key case', () => {
+		const wrapper = factory()
+		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+
+		pressCtrl('K')
+
+		expect(focusInput).toHaveBeenCalled()
 		wrapper.destroy()
 	})
 
-	it('stays out of the way on pages that own the search shortcut', () => {
-		location.value = { pathname: '/settings/users' }
-		const wrapper = mountWithShortcuts()
+	it('leaves the key to the editor the user is typing in', () => {
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
-		const prevented = pressCtrl()
+		const prevented = pressCtrl('k', addContentEditable())
 
 		expect(focusInput).not.toHaveBeenCalled()
 		expect(prevented).not.toHaveBeenCalled()
 		wrapper.destroy()
 	})
 
-	it('unbinds the shortcut when the component is torn down', () => {
-		const wrapper = mountWithShortcuts()
+	it('stays quiet behind an open modal from another app', () => {
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+		addVisibleModalMask()
 
-		wrapper.destroy()
-		pressCtrl()
+		const prevented = pressCtrl('k')
 
 		expect(focusInput).not.toHaveBeenCalled()
+		expect(prevented).not.toHaveBeenCalled()
+		wrapper.destroy()
 	})
 
-	// Under Caps Lock / Shift, event.key is 'K'. The shortcut must still fire.
-	it('fires regardless of key case', () => {
-		const wrapper = mountWithShortcuts()
+	// useHotKey has no way to exempt a component's own scrim, so the open results panel
+	// silences Ctrl+K. Re-focusing an already-focused input is a no-op, so this is only
+	// a papercut: the browser gets the key instead.
+	it('gives up the key behind its own results scrim', () => {
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+		document.body.appendChild(wrapper.vm.$el)
+		addVisibleModalMask(wrapper.vm.$el)
 
-		pressCtrl('K')
+		const prevented = pressCtrl('k')
 
-		expect(focusInput).toHaveBeenCalled()
+		expect(focusInput).not.toHaveBeenCalled()
+		expect(prevented).not.toHaveBeenCalled()
 		wrapper.destroy()
 	})
 })
 
 describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
-	function mountWithShortcuts() {
-		window.OCP = { Accessibility: { disableKeyboardShortcuts: () => false } }
-		return factory()
-	}
-
 	// Ctrl+F used to open the modal on an empty query; it now mirrors Ctrl+K.
 	it('desktop: focuses the input instead of opening an empty modal', () => {
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
 		const prevented = pressCtrl('f')
@@ -176,7 +231,7 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 
 	it('mobile: opens the modal (no header input to focus)', () => {
 		mobile.value = true
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 
 		pressCtrl('f')
 
@@ -186,7 +241,7 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 
 	it('stays out of the way on pages that own the search shortcut', () => {
 		location.value = { pathname: '/settings/users' }
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
 		const prevented = pressCtrl('f')
@@ -200,7 +255,7 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 	// must not steal the key there. This replaces the local search bar we used to render.
 	it('leaves Ctrl+F to Deck, which filters in its own board input', () => {
 		location.value = { pathname: '/apps/deck' }
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 
 		const prevented = pressCtrl('f')
@@ -214,7 +269,7 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 	// Once search is engaged, Ctrl+F belongs to the browser again: a second press must
 	// reach the native find bar instead of being swallowed to re-focus what is already focused.
 	it('falls through to the browser once the results are open', () => {
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 		wrapper.vm.showUnifiedSearch = true
 
@@ -226,7 +281,7 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 	})
 
 	it('falls through to the browser while the header input already holds focus', () => {
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 		// The engaged check tests the real focused element against the input's DOM subtree,
 		// so it needs a focusable node that is actually in the document.
@@ -245,10 +300,46 @@ describe('UnifiedSearch find shortcut (Ctrl+F) aligns with Ctrl+K', () => {
 		wrapper.destroy()
 	})
 
+	it('leaves the key to the editor the user is typing in', () => {
+		const wrapper = factory()
+		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+
+		const prevented = pressCtrl('f', addContentEditable())
+
+		expect(focusInput).not.toHaveBeenCalled()
+		expect(prevented).not.toHaveBeenCalled()
+		wrapper.destroy()
+	})
+
+	it('leaves the key to an input the user is typing in', () => {
+		const wrapper = factory()
+		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+		const field = document.createElement('input')
+		document.body.appendChild(field)
+
+		const prevented = pressCtrl('f', field)
+
+		expect(focusInput).not.toHaveBeenCalled()
+		expect(prevented).not.toHaveBeenCalled()
+		wrapper.destroy()
+	})
+
+	it('stays quiet behind an open modal from another app', () => {
+		const wrapper = factory()
+		const focusInput = vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
+		addVisibleModalMask()
+
+		const prevented = pressCtrl('f')
+
+		expect(focusInput).not.toHaveBeenCalled()
+		expect(prevented).not.toHaveBeenCalled()
+		wrapper.destroy()
+	})
+
 	// Only Ctrl+F defers to the browser. Ctrl+K has no native meaning worth preserving
 	// (in Firefox it focuses the address bar), so it stays claimed even when engaged.
 	it('does not make Ctrl+K fall through as well', () => {
-		const wrapper = mountWithShortcuts()
+		const wrapper = factory()
 		vi.spyOn(wrapper.vm, 'focusInput').mockImplementation(() => {})
 		wrapper.vm.showUnifiedSearch = true
 

core/src/views/UnifiedSearch.vue

@@ -32,6 +32,7 @@
 <script lang="ts">
 import { emit, subscribe } from '@nextcloud/event-bus'
 import { t } from '@nextcloud/l10n'
+import { useHotKey } from '@nextcloud/vue/composables/useHotKey'
 import { useIsSmallMobile } from '@nextcloud/vue/composables/useIsMobile'
 import { useBrowserLocation } from '@vueuse/core'
 import debounce from 'debounce'
@@ -124,10 +125,24 @@ export default defineComponent({
 	},
 
 	mounted() {
-		// register keyboard listener for search shortcut
-		if (window.OCP.Accessibility.disableKeyboardShortcuts() === false) {
-			window.addEventListener('keydown', this.onKeyDown)
-		}
+		// useHotKey owns the accessibility opt-out and the guards that keep shortcuts out
+		// of editors, inputs and open modals. The key filter runs before it calls
+		// preventDefault, so returning false there leaves the key to the browser.
+		this.stopHotKeys = [
+			useHotKey(
+				(event) => event.key.toLowerCase() === 'f'
+					&& !this.appHandlesSearchShortcut
+					// A second press belongs to the browser's native find.
+					&& !this.isSearchEngaged(),
+				() => this.focusSearch(),
+				{ ctrl: true, prevent: true },
+			),
+			useHotKey(
+				(event) => event.key.toLowerCase() === 'k',
+				() => this.focusSearch(),
+				{ ctrl: true, prevent: true },
+			),
+		]
 
 		// Allow external reset of the search
 		subscribe('nextcloud:unified-search:reset', () => {
@@ -147,47 +162,12 @@ export default defineComponent({
 	},
 
 	// Vue 2.7 only recognises beforeDestroy/destroyed as Options lifecycle hooks;
-	// a beforeUnmount() option is silently ignored, so the listener must be removed here.
+	// a beforeUnmount() option is silently ignored, so the listeners must be removed here.
 	beforeDestroy() {
-		// keep in mind to remove the event listener
-		window.removeEventListener('keydown', this.onKeyDown)
+		this.stopHotKeys.forEach((stop) => stop())
 	},
 
 	methods: {
-		/**
-		 * Handle the key down event to open search on `ctrl + F`
-		 *
-		 * @param event The keyboard event
-		 */
-		onKeyDown(event: KeyboardEvent) {
-			// Match on the lowercased key so Caps Lock / Shift (event.key === 'F'/'K')
-			// still triggers the shortcut instead of silently falling through.
-			const key = event.key.toLowerCase()
-			if (event.ctrlKey && key === 'f') {
-				// Skip on pages that handle Ctrl+F themselves (e.g. a dedicated search input).
-				if (this.appHandlesSearchShortcut) {
-					return
-				}
-				// Otherwise behave like Ctrl+K: focus the input (desktop) / open the
-				// modal (mobile). Once search is already engaged, let a second press fall
-				// through to the browser's native find instead of claiming Ctrl+F again.
-				if (this.isSearchEngaged()) {
-					return
-				}
-				event.preventDefault()
-				this.focusSearch()
-			} else if ((event.metaKey || event.ctrlKey) && key === 'k') {
-				// Global focus shortcut. Same opt-out as Ctrl+F: leave pages that own the
-				// shortcut alone. preventDefault only when we act (Ctrl+K also focuses the
-				// browser address bar in Firefox, so we must claim it here).
-				if (this.appHandlesSearchShortcut) {
-					return
-				}
-				event.preventDefault()
-				this.focusSearch()
-			}
-		},
-
 		/**
 		 * Bring the user into search: focus the header input on desktop, or open the
 		 * results modal on mobile. Shared by the Ctrl+F and Ctrl+K shortcuts.