fix(ui): handle IME prefixes and select shortcut conflicts
This commit is contained in:
@@ -135,6 +135,7 @@ export function DraftTargetSelectors(props: DraftTargetProps) {
|
||||
open={openPicker === 'project'}
|
||||
onOpenChange={(open) => setOpenPicker(open ? 'project' : null)}
|
||||
onValueChange={handleProjectChange}
|
||||
disableGlobalShortcuts
|
||||
>
|
||||
<SelectTrigger
|
||||
ref={projectTriggerRef}
|
||||
@@ -160,6 +161,7 @@ export function DraftTargetSelectors(props: DraftTargetProps) {
|
||||
open={openPicker === 'worktree'}
|
||||
onOpenChange={(open) => setOpenPicker(open ? 'worktree' : null)}
|
||||
onValueChange={handleDirectoryChange}
|
||||
disableGlobalShortcuts
|
||||
>
|
||||
<SelectTrigger
|
||||
ref={worktreeTriggerRef}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { expect, test } from 'bun:test';
|
||||
import { getDropdownMenuNavigationKey } from './dropdown-menu-keyboard';
|
||||
import { getDropdownNavigationKey } from './dropdown-navigation';
|
||||
|
||||
function keyEvent(key: string, modifiers: Partial<Pick<KeyboardEvent, 'ctrlKey' | 'metaKey' | 'altKey' | 'shiftKey'>> = {}) {
|
||||
return {
|
||||
@@ -13,11 +13,11 @@ function keyEvent(key: string, modifiers: Partial<Pick<KeyboardEvent, 'ctrlKey'
|
||||
}
|
||||
|
||||
test('maps only exact Ctrl+N and Ctrl+P to menu navigation keys', () => {
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('n', { ctrlKey: true }))).toBe('ArrowDown');
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('p', { ctrlKey: true }))).toBe('ArrowUp');
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('N', { ctrlKey: true }))).toBe('ArrowDown');
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('n'))).toBe(null);
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('n', { ctrlKey: true, shiftKey: true }))).toBe(null);
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('p', { ctrlKey: true, altKey: true }))).toBe(null);
|
||||
expect(getDropdownMenuNavigationKey(keyEvent('p', { ctrlKey: true, metaKey: true }))).toBe(null);
|
||||
expect(getDropdownNavigationKey(keyEvent('n', { ctrlKey: true }))).toBe('ArrowDown');
|
||||
expect(getDropdownNavigationKey(keyEvent('p', { ctrlKey: true }))).toBe('ArrowUp');
|
||||
expect(getDropdownNavigationKey(keyEvent('N', { ctrlKey: true }))).toBe('ArrowDown');
|
||||
expect(getDropdownNavigationKey(keyEvent('n'))).toBe(null);
|
||||
expect(getDropdownNavigationKey(keyEvent('n', { ctrlKey: true, shiftKey: true }))).toBe(null);
|
||||
expect(getDropdownNavigationKey(keyEvent('p', { ctrlKey: true, altKey: true }))).toBe(null);
|
||||
expect(getDropdownNavigationKey(keyEvent('p', { ctrlKey: true, metaKey: true }))).toBe(null);
|
||||
});
|
||||
|
||||
@@ -4,7 +4,8 @@ import { Menu as BaseMenu } from "@base-ui/react/menu"
|
||||
import { cn } from "@/lib/utils"
|
||||
import { Icon } from "@/components/icon/Icon";
|
||||
import { shortcutRegistry } from "@/lib/shortcuts";
|
||||
import { getDropdownMenuNavigationKey } from "./dropdown-menu-keyboard";
|
||||
import { isIMECompositionEvent } from "@/lib/ime";
|
||||
import { getDropdownNavigationKey } from "./dropdown-navigation";
|
||||
import { dropdownMenuItemClass, dropdownMenuPopupClass, dropdownMenuSeparatorClass, dropdownMenuSubTriggerClass } from "./dropdown-menu.styles";
|
||||
|
||||
type AsChildProps = { asChild?: boolean };
|
||||
@@ -141,8 +142,8 @@ function DropdownMenuContent({
|
||||
|
||||
const handleKeyDown: NonNullable<React.ComponentProps<typeof BaseMenu.Popup>['onKeyDown']> = (event) => {
|
||||
onKeyDown?.(event);
|
||||
if (event.defaultPrevented || event.isPropagationStopped() || event.nativeEvent.isComposing) return;
|
||||
const navigationKey = getDropdownMenuNavigationKey(event);
|
||||
if (event.defaultPrevented || event.isPropagationStopped() || isIMECompositionEvent(event)) return;
|
||||
const navigationKey = getDropdownNavigationKey(event);
|
||||
if (!navigationKey) return;
|
||||
|
||||
event.currentTarget.dispatchEvent(new KeyboardEvent('keydown', {
|
||||
|
||||
+1
-1
@@ -1,4 +1,4 @@
|
||||
export function getDropdownMenuNavigationKey(event: Pick<KeyboardEvent, 'key' | 'ctrlKey' | 'metaKey' | 'altKey' | 'shiftKey'>): 'ArrowDown' | 'ArrowUp' | null {
|
||||
export function getDropdownNavigationKey(event: Pick<KeyboardEvent, 'key' | 'ctrlKey' | 'metaKey' | 'altKey' | 'shiftKey'>): 'ArrowDown' | 'ArrowUp' | null {
|
||||
if (!event.ctrlKey || event.metaKey || event.altKey || event.shiftKey) return null;
|
||||
if (event.key.toLowerCase() === 'n') return 'ArrowDown';
|
||||
if (event.key.toLowerCase() === 'p') return 'ArrowUp';
|
||||
@@ -8,6 +8,9 @@ import { cn } from "@/lib/utils"
|
||||
import { dropdownTriggerVariants } from "@/components/ui/dropdown-trigger"
|
||||
import { ScrollableOverlay } from "@/components/ui/ScrollableOverlay";
|
||||
import { Icon } from "@/components/icon/Icon";
|
||||
import { shortcutRegistry } from "@/lib/shortcuts";
|
||||
import { isIMECompositionEvent } from "@/lib/ime";
|
||||
import { getDropdownNavigationKey } from "./dropdown-navigation";
|
||||
|
||||
type AsChildProps = { asChild?: boolean };
|
||||
type AsChildRenderProps = {
|
||||
@@ -36,14 +39,21 @@ type SelectRootProps<Value extends string = string> = Omit<
|
||||
value?: Value;
|
||||
defaultValue?: Value;
|
||||
onValueChange?: (value: Value, eventDetails: SelectRootChangeEventDetails) => void;
|
||||
disableGlobalShortcuts?: boolean;
|
||||
};
|
||||
|
||||
function Select<Value extends string = string>({
|
||||
onValueChange,
|
||||
modal = false,
|
||||
disableGlobalShortcuts = false,
|
||||
open,
|
||||
defaultOpen,
|
||||
onOpenChange,
|
||||
...props
|
||||
}: SelectRootProps<Value>) {
|
||||
const [portalContainer, setPortalContainer] = React.useState<HTMLElement | null>(null);
|
||||
const [uncontrolledOpen, setUncontrolledOpen] = React.useState(defaultOpen ?? false);
|
||||
const isOpen = open ?? uncontrolledOpen;
|
||||
const portalContextValue = React.useMemo<SelectPortalContextValue>(() => ({
|
||||
portalContainer,
|
||||
setPortalContainer,
|
||||
@@ -58,9 +68,26 @@ function Select<Value extends string = string>({
|
||||
[onValueChange]
|
||||
);
|
||||
|
||||
React.useLayoutEffect(() => {
|
||||
if (!disableGlobalShortcuts || !isOpen) return;
|
||||
return shortcutRegistry.suspend();
|
||||
}, [disableGlobalShortcuts, isOpen]);
|
||||
|
||||
const handleOpenChange: NonNullable<React.ComponentProps<typeof BaseSelect.Root>['onOpenChange']> = (nextOpen, eventDetails) => {
|
||||
if (open === undefined) setUncontrolledOpen(nextOpen);
|
||||
onOpenChange?.(nextOpen, eventDetails);
|
||||
};
|
||||
|
||||
return (
|
||||
<SelectPortalContext.Provider value={portalContextValue}>
|
||||
<BaseSelect.Root {...props} modal={modal} onValueChange={handleValueChange} />
|
||||
<BaseSelect.Root
|
||||
{...props}
|
||||
modal={modal}
|
||||
open={open}
|
||||
defaultOpen={defaultOpen}
|
||||
onOpenChange={handleOpenChange}
|
||||
onValueChange={handleValueChange}
|
||||
/>
|
||||
</SelectPortalContext.Provider>
|
||||
)
|
||||
}
|
||||
@@ -174,12 +201,28 @@ function SelectContent({
|
||||
sideOffset,
|
||||
side,
|
||||
align,
|
||||
onKeyDown,
|
||||
...props
|
||||
}: React.ComponentProps<typeof BaseSelect.Popup> & SelectContentExtra) {
|
||||
const portalContext = React.useContext(SelectPortalContext);
|
||||
const alignItemWithTrigger = position === "item-aligned";
|
||||
const portalContainer = portalContext?.portalContainer ?? null;
|
||||
|
||||
const handleKeyDown: NonNullable<React.ComponentProps<typeof BaseSelect.Popup>['onKeyDown']> = (event) => {
|
||||
onKeyDown?.(event);
|
||||
if (event.defaultPrevented || event.isPropagationStopped() || isIMECompositionEvent(event)) return;
|
||||
const navigationKey = getDropdownNavigationKey(event);
|
||||
if (!navigationKey) return;
|
||||
|
||||
event.currentTarget.dispatchEvent(new KeyboardEvent('keydown', {
|
||||
key: navigationKey,
|
||||
bubbles: true,
|
||||
cancelable: true,
|
||||
}));
|
||||
event.preventDefault();
|
||||
event.stopPropagation();
|
||||
};
|
||||
|
||||
return (
|
||||
<BaseSelect.Portal container={portalToBody ? undefined : portalContainer || undefined}>
|
||||
<BaseSelect.Positioner
|
||||
@@ -203,6 +246,7 @@ function SelectContent({
|
||||
className
|
||||
)}
|
||||
{...props}
|
||||
onKeyDown={handleKeyDown}
|
||||
>
|
||||
<ScrollableOverlay
|
||||
outerClassName={cn(
|
||||
@@ -248,7 +292,7 @@ function SelectItem({
|
||||
<BaseSelect.Item
|
||||
data-slot="select-item"
|
||||
className={cn(
|
||||
"data-[highlighted]:bg-interactive-hover hover:bg-interactive-hover data-[selected]:bg-interactive-selection data-[selected]:text-interactive-selection-foreground [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-pointer items-center gap-2 rounded-lg py-1.5 pr-8 pl-2 typography-ui-label outline-none select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2",
|
||||
"data-[highlighted]:bg-interactive-hover hover:bg-interactive-hover [&_svg:not([class*='text-'])]:text-muted-foreground relative flex w-full cursor-pointer items-center gap-2 rounded-lg py-1.5 pr-8 pl-2 typography-ui-label outline-none select-none data-[disabled]:pointer-events-none data-[disabled]:opacity-50 [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4 *:[span]:last:flex *:[span]:last:items-center *:[span]:last:gap-2",
|
||||
className
|
||||
)}
|
||||
{...props}
|
||||
|
||||
@@ -35,11 +35,11 @@ The settings recorder also stops at two chords. It keeps the recording local unt
|
||||
|
||||
# Dispatching
|
||||
|
||||
`ShortcutDispatcher` is DOM-independent. It invokes only currently registered handlers, resolves bindings when dispatching, and holds an active sequence prefix for 1500ms. The application keydown route clears that prefix on window blur and consumes Escape only when it cancels a prefix. A handler returns `false` to leave the completed binding unconsumed. When a sequence prefix is active, only its second key is dispatched during window capture so local input handlers cannot block it; unconsumed keys retain local input behavior, while consumed keys are prevented and stopped. Normal application shortcuts remain window-bubble listeners.
|
||||
`ShortcutDispatcher` is DOM-independent. It invokes only currently registered handlers, resolves bindings when dispatching, and holds an active sequence prefix for 1500ms. The application keydown route clears that prefix on window blur and consumes Escape only when it cancels a prefix. A handler returns `false` to leave the completed binding unconsumed. When a sequence prefix is active, only its second key is dispatched during window capture so local input handlers cannot block it; an exact second key remains eligible during IME composition and is prevented when handled, while an IME mismatch clears the prefix and retains normal composition input. Normal application shortcuts remain window-bubble listeners.
|
||||
|
||||
`shortcutRegistry.suspend()` disables all application handlers and returns an idempotent cleanup. Suspensions nest; handlers resume only after the final cleanup. Starting or ending a suspension invalidates every pending dispatcher prefix, so stale second keys and Escape cannot consume it.
|
||||
|
||||
Shared `DropdownMenu` can opt into this boundary with `disableGlobalShortcuts`; it suspends while open for both controlled and uncontrolled menus and resumes on close or unmount.
|
||||
Shared `DropdownMenu` and `Select` can opt into this boundary with `disableGlobalShortcuts`; they suspend while open for both controlled and uncontrolled popups and resume on close or unmount.
|
||||
|
||||
Terminal capture, Escape abort priming, and the shifted reverse-agent chord are input-boundary exceptions. They preserve their target-specific semantics and invoke the registered application handler rather than duplicating command behavior.
|
||||
|
||||
|
||||
@@ -163,6 +163,34 @@ describe('ShortcutDispatcher', () => {
|
||||
expect(calls).toEqual(['sequence']);
|
||||
});
|
||||
|
||||
test('consumes a matching captured prefix key during IME composition', () => {
|
||||
for (const compositionState of [{ isComposing: true }, { keyCode: 229 }]) {
|
||||
const registry = new ShortcutRegistry();
|
||||
const calls: string[] = [];
|
||||
registry.register('open_session_list', () => { calls.push('sequence'); });
|
||||
const dispatcher = new ShortcutDispatcher({ registry, getBinding: () => 'mod+s l' });
|
||||
const secondKey = key('l', compositionState);
|
||||
|
||||
expect(dispatcher.dispatch(key('s', { ctrlKey: true }))).toBe(true);
|
||||
expect(dispatcher.dispatchActivePrefix(secondKey)).toBe(true);
|
||||
expect(dispatcher.consumeCapturedPrefixEvent(secondKey)).toBe(true);
|
||||
expect(calls).toEqual(['sequence']);
|
||||
}
|
||||
});
|
||||
|
||||
test('clears an active prefix but preserves an unmatched IME key', () => {
|
||||
const registry = new ShortcutRegistry();
|
||||
const calls: string[] = [];
|
||||
registry.register('open_session_list', () => { calls.push('sequence'); });
|
||||
const dispatcher = new ShortcutDispatcher({ registry, getBinding: () => 'mod+s l' });
|
||||
const secondKey = key('x', { isComposing: true });
|
||||
|
||||
dispatcher.dispatch(key('s', { ctrlKey: true }));
|
||||
expect(dispatcher.dispatchActivePrefix(secondKey)).toBe(false);
|
||||
expect(dispatcher.hasActivePrefix()).toBe(false);
|
||||
expect(calls).toEqual([]);
|
||||
});
|
||||
|
||||
test('stops after the first handler that accepts a conflicting binding', () => {
|
||||
const registry = new ShortcutRegistry();
|
||||
const calls: string[] = [];
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
} from './bindings';
|
||||
import { type ShortcutHandler, ShortcutRegistry } from './registry';
|
||||
import type { ShortcutActionId } from './schema';
|
||||
import { isIMECompositionEvent } from '../ime';
|
||||
|
||||
const SEQUENCE_TIMEOUT_MS = 1500;
|
||||
const MODIFIER_KEYS = new Set(['alt', 'control', 'meta', 'shift']);
|
||||
@@ -38,7 +39,7 @@ export class ShortcutDispatcher {
|
||||
}
|
||||
|
||||
dispatch(event: KeyboardEvent): boolean {
|
||||
if (event.repeat || event.isComposing || MODIFIER_KEYS.has(event.key.toLowerCase())) {
|
||||
if (event.repeat || isIMECompositionEvent(event) || MODIFIER_KEYS.has(event.key.toLowerCase())) {
|
||||
return false;
|
||||
}
|
||||
if (event.key === 'Escape' && this.hasActivePrefix()) {
|
||||
@@ -48,11 +49,7 @@ export class ShortcutDispatcher {
|
||||
|
||||
const matches = this.getMatches();
|
||||
if (this.prefix) {
|
||||
const pending = matches.filter((match) => (
|
||||
match.chords.length === 2
|
||||
&& match.chords[0] === this.prefix
|
||||
&& eventMatchesShortcut(event, match.chords[1])
|
||||
));
|
||||
const pending = this.getPrefixMatches(matches, event);
|
||||
if (pending.length > 0) {
|
||||
this.clear();
|
||||
return this.invoke(pending, event);
|
||||
@@ -109,6 +106,14 @@ export class ShortcutDispatcher {
|
||||
|
||||
dispatchActivePrefix(event: KeyboardEvent): boolean {
|
||||
this.capturedPrefixEvents.add(event);
|
||||
if (isIMECompositionEvent(event)) {
|
||||
if (event.repeat || MODIFIER_KEYS.has(event.key.toLowerCase()) || !this.hasActivePrefix()) {
|
||||
return false;
|
||||
}
|
||||
const pending = this.getPrefixMatches(this.getMatches(), event);
|
||||
this.clear();
|
||||
return pending.length > 0 ? this.invoke(pending, event) : false;
|
||||
}
|
||||
return this.dispatch(event);
|
||||
}
|
||||
|
||||
@@ -127,6 +132,14 @@ export class ShortcutDispatcher {
|
||||
return false;
|
||||
}
|
||||
|
||||
private getPrefixMatches(matches: BindingMatch[], event: KeyboardEvent): BindingMatch[] {
|
||||
return matches.filter((match) => (
|
||||
match.chords.length === 2
|
||||
&& match.chords[0] === this.prefix
|
||||
&& eventMatchesShortcut(event, match.chords[1])
|
||||
));
|
||||
}
|
||||
|
||||
private getMatches(): BindingMatch[] {
|
||||
const matches: BindingMatch[] = [];
|
||||
for (const actionId of this.options.registry.actionIds()) {
|
||||
|
||||
Reference in New Issue
Block a user