On Windows/Linux, Ctrl+P opens the proofread/replace rules but also triggers the browser print dialog, since the selection shortcut handlers return undefined and never preventDefault. Add a print-free `alt+p` binding for Proofread Selection alongside ctrl+p/cmd+p. Also fix Shift+P being unable to exit paragraph mode: the paragraph overlay attaches a capture-phase keydown listener that calls stopImmediatePropagation() on every key while visible, so the global toggle shortcut never reached useShortcuts. Honor the configured "Toggle Paragraph Mode" shortcut directly in the overlay so the same shortcut that enters paragraph mode also exits it. Extract the shared shortcut event-matching into matchesShortcut() in utils/shortcutKeys.ts and reuse it from useShortcuts instead of its private duplicate. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -56,6 +56,14 @@ describe('TTS navigation shortcuts', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('Proofread selection shortcut (#4717)', () => {
|
||||
it('binds alt+p alongside ctrl+p/cmd+p so it avoids the print conflict', async () => {
|
||||
const shortcuts = await getDefaults();
|
||||
expect(shortcuts.onProofreadSelection.keys).toContain('alt+p');
|
||||
expect(shortcuts.onProofreadSelection.keys).toEqual(['ctrl+p', 'cmd+p', 'alt+p']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('No identical keybinding lists across actions (#3675)', () => {
|
||||
// Pre-existing pairs where two actions intentionally share the exact
|
||||
// same key list — both handlers guard on runtime context.
|
||||
|
||||
@@ -381,6 +381,24 @@ describe('paragraph mode', () => {
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('exits when the toggle paragraph mode shortcut (Shift+P) is pressed (#4717)', async () => {
|
||||
const onClose = vi.fn();
|
||||
await renderVisibleOverlay(onClose);
|
||||
|
||||
fireEvent.keyDown(document.body, { key: 'P', shiftKey: true });
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('does not exit on an unrelated key while visible', async () => {
|
||||
const onClose = vi.fn();
|
||||
await renderVisibleOverlay(onClose);
|
||||
|
||||
fireEvent.keyDown(document.body, { key: 'x' });
|
||||
|
||||
expect(onClose).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('paragraph mode TTS sync', () => {
|
||||
|
||||
@@ -1,5 +1,22 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { formatKeyForDisplay, filterPlatformKeys } from '../../utils/shortcutKeys';
|
||||
import { formatKeyForDisplay, filterPlatformKeys, matchesShortcut } from '../../utils/shortcutKeys';
|
||||
|
||||
const evt = (
|
||||
key: string,
|
||||
modifiers: Partial<{
|
||||
ctrlKey: boolean;
|
||||
altKey: boolean;
|
||||
metaKey: boolean;
|
||||
shiftKey: boolean;
|
||||
}> = {},
|
||||
) => ({
|
||||
key,
|
||||
ctrlKey: false,
|
||||
altKey: false,
|
||||
metaKey: false,
|
||||
shiftKey: false,
|
||||
...modifiers,
|
||||
});
|
||||
|
||||
describe('formatKeyForDisplay', () => {
|
||||
describe('Mac platform', () => {
|
||||
@@ -127,3 +144,40 @@ describe('filterPlatformKeys', () => {
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('matchesShortcut', () => {
|
||||
it('matches a shift+letter shortcut against an uppercased key event', () => {
|
||||
expect(matchesShortcut(evt('P', { shiftKey: true }), ['shift+p'])).toBe(true);
|
||||
});
|
||||
|
||||
it('does not match when a required modifier is missing', () => {
|
||||
expect(matchesShortcut(evt('p'), ['shift+p'])).toBe(false);
|
||||
});
|
||||
|
||||
it('does not match when an extra modifier is pressed', () => {
|
||||
expect(matchesShortcut(evt('P', { shiftKey: true, ctrlKey: true }), ['shift+p'])).toBe(false);
|
||||
});
|
||||
|
||||
it('treats alt and opt as the same modifier', () => {
|
||||
expect(matchesShortcut(evt('p', { altKey: true }), ['alt+p'])).toBe(true);
|
||||
expect(matchesShortcut(evt('p', { altKey: true }), ['opt+p'])).toBe(true);
|
||||
});
|
||||
|
||||
it('treats cmd and meta as the same modifier', () => {
|
||||
expect(matchesShortcut(evt('p', { metaKey: true }), ['cmd+p'])).toBe(true);
|
||||
expect(matchesShortcut(evt('p', { metaKey: true }), ['meta+p'])).toBe(true);
|
||||
});
|
||||
|
||||
it('matches when any key in the list matches', () => {
|
||||
expect(matchesShortcut(evt('p', { altKey: true }), ['ctrl+p', 'cmd+p', 'alt+p'])).toBe(true);
|
||||
});
|
||||
|
||||
it('matches symbol and space base keys', () => {
|
||||
expect(matchesShortcut(evt(']', { ctrlKey: true }), ['ctrl+]'])).toBe(true);
|
||||
expect(matchesShortcut(evt(' ', { shiftKey: true }), ['shift+ '])).toBe(true);
|
||||
});
|
||||
|
||||
it('returns false for an empty key list', () => {
|
||||
expect(matchesShortcut(evt('p', { shiftKey: true }), [])).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -13,6 +13,8 @@ import {
|
||||
ParagraphPresentation,
|
||||
} from '@/utils/paragraphPresentation';
|
||||
import { getTextSubRange } from '@/services/tts/wordHighlight';
|
||||
import { loadShortcuts } from '@/helpers/shortcuts';
|
||||
import { matchesShortcut } from '@/utils/shortcutKeys';
|
||||
import TTSFollowIndicator, { TtsSyncStatus } from '../tts/TTSFollowIndicator';
|
||||
import { buildTtsHighlightCssText } from './paragraphTts';
|
||||
|
||||
@@ -342,6 +344,16 @@ const ParagraphOverlay: React.FC<ParagraphOverlayProps> = ({
|
||||
return;
|
||||
}
|
||||
|
||||
// The overlay swallows every keydown in the capture phase, so the global
|
||||
// toggle shortcut (Shift+P by default) never reaches useShortcuts. Honor
|
||||
// it here so the same shortcut that enters paragraph mode also exits it
|
||||
// (#4717).
|
||||
if (matchesShortcut(e, loadShortcuts().onToggleParagraphMode.keys)) {
|
||||
e.preventDefault();
|
||||
onCloseRef.current?.();
|
||||
return;
|
||||
}
|
||||
|
||||
const action = getParagraphActionForKey(e.key, activePresentation ?? viewSettings);
|
||||
if (action === 'next') {
|
||||
e.preventDefault();
|
||||
|
||||
@@ -134,7 +134,9 @@ const DEFAULT_SHORTCUTS = {
|
||||
section: 'Selection',
|
||||
},
|
||||
onProofreadSelection: {
|
||||
keys: ['ctrl+p', 'cmd+p'],
|
||||
// alt+p is a print-free alternative on Windows/Linux, where ctrl+p is
|
||||
// intercepted by the browser's print dialog (#4717).
|
||||
keys: ['ctrl+p', 'cmd+p', 'alt+p'],
|
||||
description: _('Proofread Selection'),
|
||||
section: 'Selection',
|
||||
},
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { useEffect, useState } from 'react';
|
||||
import { loadShortcuts, ShortcutConfig } from '../helpers/shortcuts';
|
||||
import { matchesShortcut, ShortcutEventLike } from '../utils/shortcutKeys';
|
||||
|
||||
export type KeyActionHandlers = {
|
||||
[K in keyof ShortcutConfig]?: (event?: KeyboardEvent | MessageEvent) => void;
|
||||
@@ -17,45 +18,9 @@ const useShortcuts = (actions: KeyActionHandlers, dependencies: React.Dependency
|
||||
return () => window.removeEventListener('shortcutUpdate', handleShortcutUpdate);
|
||||
}, []);
|
||||
|
||||
const parseShortcut = (shortcut: string) => {
|
||||
const keys = shortcut.toLowerCase().split('+');
|
||||
return {
|
||||
ctrlKey: keys.includes('ctrl'),
|
||||
altKey: keys.includes('alt') || keys.includes('opt'),
|
||||
metaKey: keys.includes('meta') || keys.includes('cmd'),
|
||||
shiftKey: keys.includes('shift'),
|
||||
key: keys.find((k) => !['ctrl', 'alt', 'opt', 'meta', 'cmd', 'shift'].includes(k)),
|
||||
};
|
||||
};
|
||||
|
||||
const isShortcutMatch = (
|
||||
shortcut: string,
|
||||
key: string,
|
||||
ctrlKey: boolean,
|
||||
altKey: boolean,
|
||||
metaKey: boolean,
|
||||
shiftKey: boolean,
|
||||
) => {
|
||||
const parsedShortcut = parseShortcut(shortcut);
|
||||
return (
|
||||
parsedShortcut.key === key.toLowerCase() &&
|
||||
parsedShortcut.ctrlKey === ctrlKey &&
|
||||
parsedShortcut.altKey === altKey &&
|
||||
parsedShortcut.metaKey === metaKey &&
|
||||
parsedShortcut.shiftKey === shiftKey
|
||||
);
|
||||
};
|
||||
|
||||
const processKeyEvent = (
|
||||
key: string,
|
||||
ctrlKey: boolean,
|
||||
altKey: boolean,
|
||||
metaKey: boolean,
|
||||
shiftKey: boolean,
|
||||
event: KeyboardEvent | MessageEvent,
|
||||
) => {
|
||||
const processKeyEvent = (eventLike: ShortcutEventLike, event: KeyboardEvent | MessageEvent) => {
|
||||
// FIXME: This is a temporary fix to disable Back button navigation
|
||||
if (key === 'backspace') return true;
|
||||
if (eventLike.key.toLowerCase() === 'backspace') return true;
|
||||
for (const [actionName, actionHandler] of Object.entries(actions)) {
|
||||
const shortcutKey = actionName as keyof ShortcutConfig;
|
||||
const handler = actionHandler as
|
||||
@@ -63,12 +28,7 @@ const useShortcuts = (actions: KeyActionHandlers, dependencies: React.Dependency
|
||||
| undefined;
|
||||
const shortcutEntry = shortcuts[shortcutKey as keyof ShortcutConfig];
|
||||
// console.log('Checking action:', shortcutKey);
|
||||
if (
|
||||
handler &&
|
||||
shortcutEntry?.keys?.some((shortcut) =>
|
||||
isShortcutMatch(shortcut, key, ctrlKey, altKey, metaKey, shiftKey),
|
||||
)
|
||||
) {
|
||||
if (handler && shortcutEntry?.keys && matchesShortcut(eventLike, shortcutEntry.keys)) {
|
||||
if (handler(event)) {
|
||||
return true;
|
||||
}
|
||||
@@ -103,7 +63,7 @@ const useShortcuts = (actions: KeyActionHandlers, dependencies: React.Dependency
|
||||
event.preventDefault();
|
||||
}
|
||||
|
||||
const handled = processKeyEvent(key.toLowerCase(), ctrlKey, altKey, metaKey, shiftKey, event);
|
||||
const handled = processKeyEvent({ key, ctrlKey, altKey, metaKey, shiftKey }, event);
|
||||
// console.log('Key event handled:', key, handled);
|
||||
if (handled) event.preventDefault();
|
||||
} else if (
|
||||
@@ -112,7 +72,7 @@ const useShortcuts = (actions: KeyActionHandlers, dependencies: React.Dependency
|
||||
event.data.type === 'iframe-keydown'
|
||||
) {
|
||||
const { key, ctrlKey, altKey, metaKey, shiftKey } = event.data;
|
||||
processKeyEvent(key.toLowerCase(), ctrlKey, altKey, metaKey, shiftKey, event);
|
||||
processKeyEvent({ key, ctrlKey, altKey, metaKey, shiftKey }, event);
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
@@ -64,6 +64,38 @@ export const formatKeyForDisplay = (key: string, isMac: boolean): string => {
|
||||
return [...modifiers, displayKey].join('+');
|
||||
};
|
||||
|
||||
export type ShortcutEventLike = Pick<
|
||||
KeyboardEvent,
|
||||
'key' | 'ctrlKey' | 'altKey' | 'metaKey' | 'shiftKey'
|
||||
>;
|
||||
|
||||
const parseShortcut = (shortcut: string) => {
|
||||
const keys = shortcut.toLowerCase().split('+');
|
||||
return {
|
||||
ctrlKey: keys.includes('ctrl'),
|
||||
altKey: keys.includes('alt') || keys.includes('opt'),
|
||||
metaKey: keys.includes('meta') || keys.includes('cmd'),
|
||||
shiftKey: keys.includes('shift'),
|
||||
key: keys.find((k) => !MODIFIERS.has(k)),
|
||||
};
|
||||
};
|
||||
|
||||
// Whether a keyboard event matches any of the given shortcut strings. `alt`/`opt`
|
||||
// and `cmd`/`meta` are treated as equivalent, matching how shortcuts are authored.
|
||||
export const matchesShortcut = (event: ShortcutEventLike, keys: string[]): boolean => {
|
||||
const key = event.key.toLowerCase();
|
||||
return keys.some((shortcut) => {
|
||||
const parsed = parseShortcut(shortcut);
|
||||
return (
|
||||
parsed.key === key &&
|
||||
parsed.ctrlKey === event.ctrlKey &&
|
||||
parsed.altKey === event.altKey &&
|
||||
parsed.metaKey === event.metaKey &&
|
||||
parsed.shiftKey === event.shiftKey
|
||||
);
|
||||
});
|
||||
};
|
||||
|
||||
const MAC_MODIFIERS = new Set(['cmd', 'opt']);
|
||||
const OTHER_MODIFIERS = new Set(['ctrl', 'alt']);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user