mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(ime): stop the app dispatcher stripping the IME markers off a chord
Guarding the shared matcher was not sufficient. App.tsx rebuilt the matcher input field-by-field from the DOM event and copied only key, code, and the modifier flags, so `isComposing` and `keyCode` never reached the matcher and it had nothing to refuse on. A composing Mod+Alt+W the matcher rejected when handed the event directly still resolved `tab.closeAll` once it had been through the adapter, and the plugin-command guard was blind on that path for the same reason. The adapter is now a named module with its own tests rather than an inline object literal, because the defect was invisible at the call site and no test of the matcher could ever catch it. It refuses to build an input at all for an IME-owned keystroke, and copies the markers verbatim when it does, so the matcher can still refuse independently. Composing keydown and keyup no longer reach the double-tap detector either. The detector is stateful across the pair, and the synthetic input a detection produces carries no key or modifier flags, so there would be nothing left downstream to refuse on. Also exempts the xterm source patch from the whitespace checks: a unified diff prefixes context lines with a space, so a blank source line is a line holding exactly one space, and stripping those corrupts the patch. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
+4
-1
@@ -16,4 +16,7 @@
|
||||
# its minified bundle lines also make a diff nobody can read. Review the hand-written
|
||||
# source patch under xterm-src/ instead.
|
||||
/config/patches/@xterm__xterm@*.patch -diff -text
|
||||
/config/patches/xterm-src/*.patch text eol=lf
|
||||
# A unified-diff context line is prefixed with a space, so a blank source line is a line
|
||||
# holding exactly one space. Stripping it corrupts the patch, so exempt these from the
|
||||
# whitespace checks rather than leave `git diff --check` reporting damage as a defect.
|
||||
/config/patches/xterm-src/*.patch text eol=lf -whitespace
|
||||
|
||||
+22
-36
@@ -25,6 +25,12 @@ import { SYNC_FIT_PANES_EVENT, TOGGLE_TERMINAL_PANE_EXPAND_EVENT } from '@/const
|
||||
import { syncZoomCSSVar } from '@/lib/ui-zoom'
|
||||
import { resolveLeftSidebarStyleVariables } from '@/lib/left-sidebar-appearance'
|
||||
import { canShowRightSidebarForView } from '@/lib/right-sidebar-visibility'
|
||||
import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event'
|
||||
import {
|
||||
toDoubleTapShortcutDispatchInput,
|
||||
toShortcutDispatchInput,
|
||||
type ShortcutDispatchInput
|
||||
} from '@/lib/app-shortcut-dispatch-input'
|
||||
import {
|
||||
isPairedWebClientWindow,
|
||||
shouldRenderDesktopWindowChrome
|
||||
@@ -180,8 +186,7 @@ import {
|
||||
keybindingMatchesAction,
|
||||
type KeybindingActionId,
|
||||
type KeybindingContext,
|
||||
type KeybindingMatchOptions,
|
||||
type PhysicalModifierToken
|
||||
type KeybindingMatchOptions
|
||||
} from '../../shared/keybindings'
|
||||
import { PLUGIN_COMMAND_ALIAS_ACTION_IDS } from '../../shared/plugins/plugin-command-actions'
|
||||
import { registerAppCommandDispatcher } from '@/lib/app-command-dispatch'
|
||||
@@ -246,20 +251,6 @@ function getKeybindingContext(target: EventTarget | null): KeybindingContext {
|
||||
: 'app'
|
||||
}
|
||||
|
||||
// Abstraction over a real KeyboardEvent and a synthetic double-tap gesture so one dispatch path serves both; KeybindingInput-compatible.
|
||||
type ShortcutDispatchInput = {
|
||||
key?: string
|
||||
code?: string
|
||||
altKey?: boolean
|
||||
metaKey?: boolean
|
||||
ctrlKey?: boolean
|
||||
shiftKey?: boolean
|
||||
doubleTapModifier?: PhysicalModifierToken
|
||||
target: EventTarget | null
|
||||
defaultPrevented: boolean
|
||||
preventDefault: () => void
|
||||
}
|
||||
|
||||
// Why: Windows and Linux both remove the native title bar, so we render our own min/max/close buttons (Fluent/Win11-style SVGs).
|
||||
function WindowControls(): React.JSX.Element {
|
||||
const [maximized, setMaximized] = useState(false)
|
||||
@@ -1920,6 +1911,11 @@ function App(): React.JSX.Element {
|
||||
}
|
||||
|
||||
const onKeyDown = (e: KeyboardEvent): void => {
|
||||
// Ahead of the detector: a composing chord must not arm a double-tap either, and the
|
||||
// synthetic input a detection produces carries no markers to be refused by later.
|
||||
if (isImeCompositionKeyDown(e)) {
|
||||
return
|
||||
}
|
||||
const detected = doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyDown',
|
||||
@@ -1936,30 +1932,20 @@ function App(): React.JSX.Element {
|
||||
if (e.repeat) {
|
||||
return
|
||||
}
|
||||
if (detected) {
|
||||
// Synthetic input: no key/modifier flags, so only DoubleTap bindings match.
|
||||
dispatchShortcutInput({
|
||||
doubleTapModifier: detected.modifier,
|
||||
target: e.target,
|
||||
defaultPrevented: e.defaultPrevented,
|
||||
preventDefault: () => e.preventDefault()
|
||||
})
|
||||
return
|
||||
const input = detected
|
||||
? toDoubleTapShortcutDispatchInput(e, detected.modifier)
|
||||
: toShortcutDispatchInput(e)
|
||||
if (input) {
|
||||
dispatchShortcutInput(input)
|
||||
}
|
||||
dispatchShortcutInput({
|
||||
key: e.key,
|
||||
code: e.code,
|
||||
altKey: e.altKey,
|
||||
metaKey: e.metaKey,
|
||||
ctrlKey: e.ctrlKey,
|
||||
shiftKey: e.shiftKey,
|
||||
target: e.target,
|
||||
defaultPrevented: e.defaultPrevented,
|
||||
preventDefault: () => e.preventDefault()
|
||||
})
|
||||
}
|
||||
|
||||
const onKeyUp = (e: KeyboardEvent): void => {
|
||||
// The detector is stateful across keydown/keyup, so a composing release must not
|
||||
// complete a tap pair the matching press was already withheld from.
|
||||
if (isImeCompositionKeyDown(e)) {
|
||||
return
|
||||
}
|
||||
doubleTapDetector.process(
|
||||
toModifierDoubleTapEvent({
|
||||
type: 'keyUp',
|
||||
|
||||
@@ -0,0 +1,66 @@
|
||||
// @vitest-environment happy-dom
|
||||
// Guarding the shared matcher is not enough on its own: this adapter rebuilds the input
|
||||
// field-by-field, so any field it forgets is a field the matcher can never refuse on. The
|
||||
// regression these pin is a composing Mod+Alt+W that the matcher rejected directly but that
|
||||
// resolved `tab.closeAll` once it had been through the adapter.
|
||||
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { keybindingMatchesAction } from '../../../shared/keybindings'
|
||||
import {
|
||||
toDoubleTapShortcutDispatchInput,
|
||||
toShortcutDispatchInput
|
||||
} from './app-shortcut-dispatch-input'
|
||||
|
||||
function closeAllTabsEvent(over: Partial<KeyboardEventInit> = {}): KeyboardEvent {
|
||||
return new KeyboardEvent('keydown', {
|
||||
altKey: true,
|
||||
code: 'KeyW',
|
||||
key: 'w',
|
||||
metaKey: true,
|
||||
...over
|
||||
})
|
||||
}
|
||||
|
||||
describe('toShortcutDispatchInput', () => {
|
||||
it('carries a real chord through to a matching action', () => {
|
||||
const input = toShortcutDispatchInput(closeAllTabsEvent())
|
||||
|
||||
expect(input).not.toBeNull()
|
||||
expect(keybindingMatchesAction('tab.closeAll', input!, 'darwin')).toBe(true)
|
||||
})
|
||||
|
||||
it.each([
|
||||
['isComposing', { isComposing: true }],
|
||||
['keyCode 229', { keyCode: 229 }]
|
||||
])('refuses to build an input for a chord marked by %s', (_marker, over) => {
|
||||
expect(toShortcutDispatchInput(closeAllTabsEvent(over))).toBeNull()
|
||||
})
|
||||
|
||||
// Defense in depth, and the field-level assertion the null-return above cannot make: the
|
||||
// copy must carry the markers verbatim so the matcher can refuse on its own. Dropping them
|
||||
// here is the original defect, and it is invisible to every test that only checks the null.
|
||||
it('copies the IME markers verbatim rather than dropping them', () => {
|
||||
const input = toShortcutDispatchInput(closeAllTabsEvent())!
|
||||
|
||||
expect(input.isComposing).toBe(false)
|
||||
expect(input.keyCode).toBe(closeAllTabsEvent().keyCode)
|
||||
expect(Object.hasOwn(input, 'isComposing')).toBe(true)
|
||||
expect(Object.hasOwn(input, 'keyCode')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('toDoubleTapShortcutDispatchInput', () => {
|
||||
it('builds a gesture from an ordinary release', () => {
|
||||
const input = toDoubleTapShortcutDispatchInput(new KeyboardEvent('keydown'), 'Shift')
|
||||
|
||||
expect(input?.doubleTapModifier).toBe('Shift')
|
||||
})
|
||||
|
||||
// A composing chord must not arm a double-tap: the synthetic input it produces carries no
|
||||
// key or modifier flags, so there is nothing left downstream for the matcher to refuse on.
|
||||
it('refuses to build a gesture while an IME owns the keystroke', () => {
|
||||
expect(
|
||||
toDoubleTapShortcutDispatchInput(new KeyboardEvent('keydown', { isComposing: true }), 'Shift')
|
||||
).toBeNull()
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,68 @@
|
||||
import type { PhysicalModifierToken } from '../../../shared/keybindings'
|
||||
import { isImeCompositionKeyDown } from '@/lib/ime-composition-keyboard-event'
|
||||
|
||||
/**
|
||||
* Abstraction over a real KeyboardEvent and a synthetic double-tap gesture so one dispatch
|
||||
* path serves both; KeybindingInput-compatible.
|
||||
*
|
||||
* The IME markers are part of the contract. The shared matcher refuses a chord an IME owns by
|
||||
* reading them off this object, so an adapter that builds it field-by-field and omits them
|
||||
* re-opens the hole no matter what the matcher does — which is exactly how a composing
|
||||
* Mod+Alt+W reached `tab.closeAll` after the matcher was already guarded.
|
||||
*/
|
||||
export type ShortcutDispatchInput = {
|
||||
key?: string
|
||||
code?: string
|
||||
altKey?: boolean
|
||||
metaKey?: boolean
|
||||
ctrlKey?: boolean
|
||||
shiftKey?: boolean
|
||||
isComposing?: boolean
|
||||
keyCode?: number
|
||||
doubleTapModifier?: PhysicalModifierToken
|
||||
target: EventTarget | null
|
||||
defaultPrevented: boolean
|
||||
preventDefault: () => void
|
||||
}
|
||||
|
||||
/** Null when an IME owns the keystroke, so the window listener has nothing to dispatch. */
|
||||
export function toShortcutDispatchInput(event: KeyboardEvent): ShortcutDispatchInput | null {
|
||||
if (isImeCompositionKeyDown(event)) {
|
||||
return null
|
||||
}
|
||||
return {
|
||||
altKey: event.altKey,
|
||||
code: event.code,
|
||||
ctrlKey: event.ctrlKey,
|
||||
defaultPrevented: event.defaultPrevented,
|
||||
isComposing: event.isComposing,
|
||||
key: event.key,
|
||||
keyCode: event.keyCode,
|
||||
metaKey: event.metaKey,
|
||||
preventDefault: () => event.preventDefault(),
|
||||
shiftKey: event.shiftKey,
|
||||
target: event.target
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The double-tap gesture carries no key or modifier flags, so only DoubleTap bindings match.
|
||||
* It still passes the markers through: the gesture is only as trustworthy as the release that
|
||||
* completed it.
|
||||
*/
|
||||
export function toDoubleTapShortcutDispatchInput(
|
||||
event: KeyboardEvent,
|
||||
doubleTapModifier: PhysicalModifierToken
|
||||
): ShortcutDispatchInput | null {
|
||||
if (isImeCompositionKeyDown(event)) {
|
||||
return null
|
||||
}
|
||||
return {
|
||||
defaultPrevented: event.defaultPrevented,
|
||||
doubleTapModifier,
|
||||
isComposing: event.isComposing,
|
||||
keyCode: event.keyCode,
|
||||
preventDefault: () => event.preventDefault(),
|
||||
target: event.target
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user