fix(tao): drive macOS press-and-hold from replacementRange, not heuristics - #617
Merged
Merged
Conversation
Nucleus forced `ApplePressAndHoldEnabled` on in four domains — app, argument volatile, CFPreferences, registration — from both `+load` and runtime, then guessed when the accent picker was active from a state machine (`g_letter_key_down`, `g_press_and_hold_queried`, `g_base_text`). Both halves were wrong: - Forcing the default suppresses key repeat, but does not guarantee the picker engages. Where it cannot (non-Apple keyboards, Karabiner virtual devices), a held letter did nothing at all — no repeat, no picker, strictly worse than either OS behaviour and invisible to the user (#612). - The heuristics misread ordinary fast typing: with the previous key still physically down, a `selectedRange` query armed the picker flag and the next commit was routed into the replace path, deleting the character that had just been committed ("Xcode" landing as "Xcde") (#611). Chromium does neither. `ApplePressAndHoldEnabled` has zero occurrences in its tree and its history; the picker works because RenderWidgetHostViewCocoa answers `selectedRange` / `attributedSubstringForProposedRange` over a cached window of committed text and honors `replacementRange` in `insertText:`. Recording AppKit against an `NSTextView` and a document-backed `NSTextInputClient` confirms the protocol: the accent pick arrives as `insertText:"é" replacementRange:{caret-1, 1}` — UTF-16, document-absolute — and the range carries everything. No client-side detection is involved. Same model here: - Never read, set or register the user default. The OS decides whether a held letter repeats or opens the picker. - The host pushes the focused field's committed text (a bounded window), its document-absolute offset and the selection through `nativeSetImeDocument`, mirroring Chromium's renderer-to-browser selection + surrounding-text push. The swizzled getters serve document-absolute answers from that cache; the marked-text anchor is maintained optimistically in a `setMarkedText:` swizzle, the same fallback chain Chromium uses for `_markedRange`. - `insert_text` routes a valid `replacementRange` outside a composition to `WindowEvent::ImeReplaceCommit`, and `TaoImeSession.replaceCommit` applies it select-then-insert — Blink's `ReplaceTextAndKeepSelection` semantics — instead of a blind `deleteSurroundingTextInCodePoints(1, 0)`. Routing the pick through tao's event queue also removes a re-entrant `EVENT_CALLBACK` lock: the previous direct upcall from the AppKit callout deadlocked when the JVM re-entered native on the same thread. Repeat keyDowns still reach `interpretKeyEvents:` (unchanged), which is what lets the picker engage where the OS allows it. Closes #612.
`TaoPressAndHoldE2ETest` (opt-in, `NUCLEUS_TAO_SMOKE=1`) replays real
keyboard traffic — CGEvents at the session tap, `kCGKeyboardEventAutorepeat`
stamped so AppKit sees a held key — against a focused Compose field. Each
scenario runs in its own child JVM so `ApplePressAndHoldEnabled` can be
set per scenario through the process argument domain, and because
`taoApplication` ends in `exitProcess(0)`.
The expectations are not invented: they are the behaviour recorded from
AppKit driving a reference `NSTextView` and a document-backed
`NSTextInputClient` on the same machine — what Chrome, Notes and TextEdit
do.
- press-and-hold disabled by the user: a held letter repeats
- press-and-hold enabled: the held letter never goes dead and the next
keystroke is not eaten
- the picker's `insertText:"é" replacementRange:{0, 1}` replaces the base
letter
- key roll-over while typing `xcode` is not misread as an accent pick
All four fail on the previous implementation: 'e' instead of 'eeeeee',
'x' instead of 'e…x', a watchdog timeout on the replacement commit, and
'xcde' instead of 'xcode'.
Stage-1 coverage for the replacement commit itself lands in
`TaoSceneImeTest`: in-range replacement, surrounding text left intact
with the caret after the accent, and an out-of-bounds range clamped
rather than thrown.
Two traps worth knowing, both hit while writing this:
- A scenario that engages the real picker must dismiss it with Escape
before the next keystroke and at scenario end — the bubble is a system
window and outlives the child process, so it steals the following
scenario's keys.
- CGEvent posting inherits the machine keyboard layout, so the probe
sanity-checks the first keystroke and reports `skip=layout` instead of
failing on a non-Latin layout.
`nucleus_tao_post_key_to_view` gained an autorepeat flag and
`nucleus_tao_inject_insert_text` a replacement range; both default to the
previous behaviour for existing callers.
Follow-ups from review of the two commits above. Correctness: - The `ImeReplaceCommit` branch did not set `key_triggered_ime`, so the key that picked the accent was also delivered as a raw key event. An app shortcut bound to a digit fired while the user was only choosing an accent. Both sibling IME branches already set it; the #595 invariant is that a key the input method consumed is not double-delivered. - `attributedSubstringForProposedRange:` could underflow: `NSMaxRange` wraps on an overflowing proposed range, slips past both guards, and `end - loc` became a huge length, raising `NSRangeException` from inside an AppKit callout. - Document-cache invalidation was global, so focus moving between windows (the old session tears down after the new one starts) wiped the cache the newly focused field had just installed. It is now scoped to the owning view, resets the marked-text anchor, and `detach()` invalidates explicitly so a freed view's address cannot be inherited by a later view allocated at the same address. - A stale `markedText` ivar pinned the marked-text anchor from a previous field; an invalid cache now forces a re-anchor. - `replaceCommit` ends the composition before committing: `commitText` replaces the composing region when one exists, which would have silently ignored the selection it had just set. - The no-session fallback hardcoded a single Backspace regardless of the replaced length; it now deletes what the range asked for, bounded. Fidelity and cost, both on the keystroke hot path: - `nativeSetImeDocument` read the JVM string through modified UTF-8 and a lossy decode — two extra copies, and a length that can drift from the offsets the JVM computed (unpaired surrogates). It now reads the UTF-16 directly via `GetStringRegion`, as its comment always claimed. - The pushed window drops from ±512 to ±128 UTF-16 units, closer to the ±100 Chromium ships. The only reader is `attributedSubstringForProposedRange:`, which is only ever asked near the caret. - `dispatch_ime_replace_commit` reuses the shared JNI helper instead of duplicating its attach/exception handling. Tests: - The probe left `e` held down when the layout-sanity check bailed out. Key state lives in the window server and outlives the child process, so that leaked a stuck key into the next scenario and the rest of the session. - The repeat scenario asserted an exact six characters; the claim is that the key repeats, not how many synthetic autorepeats survive coalescing.
Contributor
|
Tested this branch (
The |
Collaborator
Author
|
@takke Thank you very much, I was going to ask you to check but I fell asleep before I could :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
macOS press-and-hold is rewritten to the contract every document-backed AppKit client follows — Chrome, Notes, TextEdit — replacing the forced user default and the picker-detection state machine.
Two bugs shared one root cause: Nucleus was guessing when the accent picker was active instead of listening to the protocol.
ApplePressAndHoldEnabledwas forced on in four domains (app, argument volatile, CFPreferences, registration) from both+loadand runtime. Forcing it suppresses key repeat but cannot make the picker engage; where it cannot (non-Apple keyboards, Karabiner virtual devices) a held letter did nothing at all — no repeat, no picker. Strictly worse than either OS behaviour, and invisible: every other app on that machine repeats, the Nucleus one just stops.g_letter_key_down,g_press_and_hold_queried,g_base_text) misread ordinary fast typing. With the previous key still physically down, aselectedRangequery armed the picker flag and the next commit was routed into the replace path, deleting the character just committed:Xcode→Xcde.What Chrome actually does
ApplePressAndHoldEnabledhas zero occurrences in the Chromium tree and its entire commit history — the OS decides. The picker works becauseRenderWidgetHostViewCocoaanswersselectedRange/attributedSubstringForProposedRangeover a cached window of committed text (the renderer pushes selection ±100 chars) and honorsreplacementRangeininsertText:. There is no press-and-hold state machine anywhere incontent/.Recording AppKit against a reference
NSTextViewand a custom document-backedNSTextInputClientconfirms the wire protocol: the accent pick arrives asinsertText:"é" replacementRange:{caret-1, 1}— UTF-16, document-absolute. The range carries everything; no detection is needed.This PR
nativeSetImeDocument; the swizzled getters serve document-absolute answers from that cache. The marked-text anchor is maintained optimistically in asetMarkedText:swizzle — Chromium's_markedRangefallback chain.insert_textroutes a validreplacementRangeoutside a composition to a newWindowEvent::ImeReplaceCommit, andTaoImeSession.replaceCommitapplies it select-then-insert (Blink'sReplaceTextAndKeepSelection) instead of a blinddeleteSurroundingTextInCodePoints(1, 0).EVENT_CALLBACKlock: the previous direct upcall from the AppKit callout deadlocked when the JVM re-entered native on the same thread.Repeat keyDowns still reach
interpretKeyEvents:(unchanged) — that is what lets the picker engage where the OS allows it.Everything touched is
internal; no ABI change (apiCheckclean). GraalVM reachability metadata updated for the new callback signature.Test plan
TaoPressAndHoldE2ETest(opt-in,NUCLEUS_TAO_SMOKE=1) posts real CGEvents at the session tap withkCGKeyboardEventAutorepeatstamped, against a focused Compose field. One child JVM per scenario, soApplePressAndHoldEnabledis controlled through the process argument domain. Expectations are the behaviour recorded from AppKit drivingNSTextView/ a document-backed client on the same machine.e— dead key (#612)eeeeeexx— held letter eaten^e+x$insertText:"é" rr:{0,1}éxcodexcde(#611)xcodemain, pass after the fix#595Kotoeri headful cases pass — the IME cross-checks exactly theNSTextInputClientanswers rewritten here:decorated-window-tao:testgreen, incl. 3 new stage-1 cases inTaoSceneImeTest(in-range replacement, surrounding text intact, out-of-bounds range clamped)ktlintCheck/detekt/apiCheckgreenNotes
Supersedes #611 — that PR patches
nucleus_note_press_and_hold_query()andnucleus_insert_text(), functions this branch deletes along with the rest of the state machine. Its roll-over scenario is preserved as a permanent e2e case. @takke's diagnosis is what pinned the real problem: guarding an architecture that guesses, rather than reading the range AppKit already provides.With the user default no longer forced, the opt-out requested in #612 is unnecessary — Nucleus now behaves like every other macOS app.