-
Notifications
You must be signed in to change notification settings - Fork 598
✨(frontend) add keyboard shortcut to toggle presenter mode #2426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { renderHook } from '@testing-library/react'; | ||
| import { describe, expect, test, vi } from 'vitest'; | ||
|
|
||
| import { usePresenterToggleShortcut } from '../hooks/usePresenterToggleShortcut'; | ||
|
|
||
| const renderShortcut = (enabled?: boolean) => { | ||
| const onToggle = vi.fn(); | ||
| renderHook(() => usePresenterToggleShortcut(onToggle, enabled)); | ||
| return onToggle; | ||
| }; | ||
|
|
||
| const press = (init: KeyboardEventInit) => { | ||
| const event = new KeyboardEvent('keydown', { ...init, cancelable: true }); | ||
| window.dispatchEvent(event); | ||
| return event; | ||
| }; | ||
|
|
||
| describe('usePresenterToggleShortcut', () => { | ||
| test('Ctrl+Alt+P and Meta+Alt+P call onToggle', () => { | ||
| const onToggle = renderShortcut(); | ||
| press({ code: 'KeyP', ctrlKey: true, altKey: true }); | ||
| press({ code: 'KeyP', metaKey: true, altKey: true }); | ||
| expect(onToggle).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| test('Mod+Alt+P prevents default', () => { | ||
| renderShortcut(); | ||
| const event = press({ code: 'KeyP', ctrlKey: true, altKey: true }); | ||
| expect(event.defaultPrevented).toBe(true); | ||
| }); | ||
|
|
||
| test('KeyP without the full modifier combination is ignored', () => { | ||
| const onToggle = renderShortcut(); | ||
| press({ code: 'KeyP' }); | ||
| press({ code: 'KeyP', ctrlKey: true }); | ||
| press({ code: 'KeyP', altKey: true }); | ||
| press({ code: 'KeyP', ctrlKey: true, altKey: true, shiftKey: true }); | ||
| expect(onToggle).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('repeat events are ignored', () => { | ||
| const onToggle = renderShortcut(); | ||
| press({ code: 'KeyP', ctrlKey: true, altKey: true, repeat: true }); | ||
| expect(onToggle).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('does nothing when disabled', () => { | ||
| const onToggle = renderShortcut(false); | ||
| press({ code: 'KeyP', ctrlKey: true, altKey: true }); | ||
| expect(onToggle).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||||||||||||||||||||||||||
| import { useEffect } from 'react'; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Toggles the presenter on Mod+Alt+P (Ctrl+Alt+P / ⌘⌥P). | ||||||||||||||||||||||||||||||
| * Matching on `event.code` keeps the shortcut independent of the | ||||||||||||||||||||||||||||||
| * keyboard layout, and requiring Ctrl/Meta makes it safe to fire | ||||||||||||||||||||||||||||||
| * while typing in the editor. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| export const usePresenterToggleShortcut = ( | ||||||||||||||||||||||||||||||
| onToggle: () => void, | ||||||||||||||||||||||||||||||
| enabled = true, | ||||||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||
| if (!enabled) { | ||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const handleKeyDown = (event: KeyboardEvent) => { | ||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||
| event.code === 'KeyP' && | ||||||||||||||||||||||||||||||
| (event.ctrlKey || event.metaKey) && | ||||||||||||||||||||||||||||||
| event.altKey && | ||||||||||||||||||||||||||||||
| !event.shiftKey && | ||||||||||||||||||||||||||||||
| !event.repeat | ||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against AltGraph false positives in the shortcut predicate. On international layouts, AltGr commonly sets both Suggested patch if (
event.code === 'KeyP' &&
(event.ctrlKey || event.metaKey) &&
event.altKey &&
+ !event.getModifierState('AltGraph') &&
!event.shiftKey &&
!event.repeat
) {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||
| onToggle(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| window.addEventListener('keydown', handleKeyDown); | ||||||||||||||||||||||||||||||
| return () => { | ||||||||||||||||||||||||||||||
| window.removeEventListener('keydown', handleKeyDown); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| }, [onToggle, enabled]); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a regression test to ensure AltGraph does not trigger presenter toggle.
Please add a non-trigger case so keyboard-layout regressions are caught when
ctrlKey + altKeycomes fromAltGraph.Suggested test addition
test('does nothing when disabled', () => { const onToggle = renderShortcut(false); press({ code: 'KeyP', ctrlKey: true, altKey: true }); expect(onToggle).not.toHaveBeenCalled(); }); + + test('AltGraph+P is ignored', () => { + const onToggle = renderShortcut(); + const event = press({ code: 'KeyP', ctrlKey: true, altKey: true }); + Object.defineProperty(event, 'getModifierState', { + value: (key: string) => key === 'AltGraph', + }); + window.dispatchEvent(event); + expect(onToggle).not.toHaveBeenCalled(); + }); });🤖 Prompt for AI Agents