Repository navigation
fix(browser): keep IME Enter on composition path - #2108
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughThe changes add IME composition detection to prevent Return/Enter key dispatch to the browser first responder when marked text is active. A new helper function Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes issue #1814 where IME confirmation (Return/keypad Enter during marked-text composition) was being intercepted and force-dispatched to the browser's The fix adds a
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["NSWindow.performKeyEquivalent(event)"] --> B["Compute firstResponderWebView"]
A --> C["Compute firstResponderHasMarkedText\nbrowserResponderHasMarkedText(firstResponder)"]
C --> C1{"responder as? NSTextInputClient"}
C1 -- yes --> C2["return textInputClient.hasMarkedText()"]
C1 -- no --> C3{"responder as? NSTextField\n& currentEditor as? NSTextView"}
C3 -- yes --> C4["return editor.hasMarkedText()"]
C3 -- no --> C5["return false"]
B --> D["shouldDispatchBrowserReturnViaFirstResponderKeyDown(...)"]
C --> D
D --> E{"firstResponderIsBrowser?"}
E -- no --> F["return false (no-op)"]
E -- yes --> G{"firstResponderHasMarkedText?"}
G -- yes --> H["return false → IME keeps event\n(fix for issue #1814)"]
G -- no --> I{"keyCode == 36 or 76?"}
I -- no --> F
I -- yes --> J{"plain/Shift Return?"}
J -- yes --> K["firstResponder.keyDown(event)\nreturn true (form submit)"]
J -- no --> F
Reviews (1): Last reviewed commit: "fix(browser): keep IME Enter on composit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let textField = responder as? NSTextField, | ||
| let editor = textField.currentEditor() as? NSTextView { | ||
| return editor.hasMarkedText() | ||
| } |
There was a problem hiding this comment.
NSTextField branch appears unreachable in normal AppKit flow
In standard AppKit event dispatch, when a user is actively editing an NSTextField, the field editor (NSTextView) — not the NSTextField itself — becomes the window's first responder. Since NSTextView conforms to NSTextInputClient, it is already handled by the first branch above. The NSTextField branch here would only be reached if NSTextField is the first responder while currentEditor() is simultaneously non-nil, which does not occur in the normal AppKit responder promotion sequence.
This is harmless dead code, but if the intent is to guard against some custom NSTextField subclass scenario, a brief comment explaining the edge case would help future readers understand why both paths exist.
| func testWindowPerformKeyEquivalentDoesNotForwardReturnDuringMarkedTextComposition() { | ||
| _ = NSApplication.shared | ||
| AppDelegate.installWindowResponderSwizzlesForTesting() | ||
|
|
||
| let window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), | ||
| styleMask: [.titled, .closable], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| let container = NSView(frame: window.contentRect(forFrameRect: window.frame)) | ||
| window.contentView = container | ||
|
|
||
| let webView = CmuxWebView(frame: container.bounds, configuration: WKWebViewConfiguration()) | ||
| webView.autoresizingMask = [.width, .height] | ||
| container.addSubview(webView) | ||
|
|
||
| let responder = BrowserMarkedTextProbeTextView(frame: NSRect(x: 0, y: 0, width: 32, height: 20)) | ||
| responder.hasMarkedTextForTesting = true | ||
| webView.addSubview(responder) | ||
|
|
||
| window.makeKeyAndOrderFront(nil) | ||
| defer { window.orderOut(nil) } | ||
|
|
||
| XCTAssertTrue(window.makeFirstResponder(responder)) | ||
| guard let event = NSEvent.keyEvent( | ||
| with: .keyDown, | ||
| location: .zero, | ||
| modifierFlags: [], | ||
| timestamp: ProcessInfo.processInfo.systemUptime, | ||
| windowNumber: window.windowNumber, | ||
| context: nil, | ||
| characters: "\r", | ||
| charactersIgnoringModifiers: "\r", | ||
| isARepeat: false, | ||
| keyCode: 36 | ||
| ) else { | ||
| XCTFail("Failed to construct Return event") | ||
| return | ||
| } | ||
|
|
||
| let consumed = window.performKeyEquivalent(with: event) | ||
|
|
||
| XCTAssertFalse(consumed, "Return should stay in the IME path while marked text is active") | ||
| XCTAssertTrue(responder.hasMarkedText(), "Marked text should still be active until the input method commits it") | ||
| XCTAssertEqual(responder.keyDownEvents.count, 0, "Return should not be force-forwarded to the browser responder during IME composition") | ||
| } | ||
|
|
||
| @MainActor | ||
| func testWindowPerformKeyEquivalentDoesNotForwardKeypadEnterDuringMarkedTextComposition() { | ||
| _ = NSApplication.shared | ||
| AppDelegate.installWindowResponderSwizzlesForTesting() | ||
|
|
||
| let window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), | ||
| styleMask: [.titled, .closable], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| let container = NSView(frame: window.contentRect(forFrameRect: window.frame)) | ||
| window.contentView = container | ||
|
|
||
| let webView = CmuxWebView(frame: container.bounds, configuration: WKWebViewConfiguration()) | ||
| webView.autoresizingMask = [.width, .height] | ||
| container.addSubview(webView) | ||
|
|
||
| let responder = BrowserMarkedTextProbeTextView(frame: NSRect(x: 0, y: 0, width: 32, height: 20)) | ||
| responder.hasMarkedTextForTesting = true | ||
| webView.addSubview(responder) | ||
|
|
||
| window.makeKeyAndOrderFront(nil) | ||
| defer { window.orderOut(nil) } | ||
|
|
||
| XCTAssertTrue(window.makeFirstResponder(responder)) | ||
| guard let event = NSEvent.keyEvent( | ||
| with: .keyDown, | ||
| location: .zero, | ||
| modifierFlags: [], | ||
| timestamp: ProcessInfo.processInfo.systemUptime, | ||
| windowNumber: window.windowNumber, | ||
| context: nil, | ||
| characters: "\r", | ||
| charactersIgnoringModifiers: "\r", | ||
| isARepeat: false, | ||
| keyCode: 76 | ||
| ) else { | ||
| XCTFail("Failed to construct keypad Enter event") | ||
| return | ||
| } | ||
|
|
||
| let consumed = window.performKeyEquivalent(with: event) | ||
|
|
||
| XCTAssertFalse(consumed, "Keypad Enter should stay in the IME path while marked text is active") | ||
| XCTAssertTrue(responder.hasMarkedText(), "Marked text should still be active until the input method commits it") | ||
| XCTAssertEqual(responder.keyDownEvents.count, 0, "Keypad Enter should not be force-forwarded to the browser responder during IME composition") | ||
| } | ||
| } | ||
|
|
||
|
|
||
| final class BrowserReturnKeyDownRoutingTests: XCTestCase { | ||
| func testRoutesForReturnWhenBrowserFirstResponder() { | ||
| XCTAssertTrue( |
There was a problem hiding this comment.
testWindowPerformKeyEquivalentDoesNotForwardReturnDuringMarkedTextComposition and testWindowPerformKeyEquivalentDoesNotForwardKeypadEnterDuringMarkedTextComposition share identical window/webView/responder setup (≈35 lines each), differing only in keyCode (36 vs 76) and the assertion messages. Extracting the shared scaffolding into a private helper would make it straightforward to add coverage for future key codes without duplicating boilerplate:
private func runIMEEnterTest(keyCode: UInt16, label: String) { … }Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
1524-1528: MakefirstResponderHasMarkedTextmandatory.Line 1527 makes the new safeguard opt-in.
cmuxTests/BrowserConfigTests.swift:2577-2611already has callers omitting this argument, so future production call sites could silently fall back to the pre-fix behavior.♻️ Proposed change
func shouldDispatchBrowserReturnViaFirstResponderKeyDown( keyCode: UInt16, firstResponderIsBrowser: Bool, - firstResponderHasMarkedText: Bool = false, + firstResponderHasMarkedText: Bool, flags: NSEvent.ModifierFlags ) -> Bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 1524 - 1528, The function shouldDispatchBrowserReturnViaFirstResponderKeyDown currently makes firstResponderHasMarkedText optional by providing a default; remove the default so callers must explicitly pass firstResponderHasMarkedText to opt into the safeguard — update the function signature in shouldDispatchBrowserReturnViaFirstResponderKeyDown to remove the "= false" default and then update all call sites (including tests like cmuxTests/BrowserConfigTests.swift) to pass an explicit Bool for firstResponderHasMarkedText.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 1524-1528: The function
shouldDispatchBrowserReturnViaFirstResponderKeyDown currently makes
firstResponderHasMarkedText optional by providing a default; remove the default
so callers must explicitly pass firstResponderHasMarkedText to opt into the
safeguard — update the function signature in
shouldDispatchBrowserReturnViaFirstResponderKeyDown to remove the "= false"
default and then update all call sites (including tests like
cmuxTests/BrowserConfigTests.swift) to pass an explicit Bool for
firstResponderHasMarkedText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b1d14b-0535-46f7-ae7e-3a400949c058
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/BrowserConfigTests.swift
* test: cover browser IME Enter composition routing * fix(browser): keep IME Enter on composition path
|
首先感謝你們針對中文輸入法的改善努力，不過上次的修復還是有一點問題： Still reproducible after #2108Environment
What still happensAfter #2108 was merged, the issue persists. Pressing Enter after 注音 candidate selection still submits the web form instead of committing the composition. Why the fix likely still has a race condition#2108 guards against dispatch by calling This is the same timing issue as the web-layer Confirmed workaround (JS layer, tested)Injecting the following via DevTools Console fully resolves the issue: (function() {
let composing = false;
document.addEventListener('compositionstart', () => composing = true, true);
document.addEventListener('compositionend', () => {
setTimeout(() => composing = false, 30);
}, true);
document.addEventListener('keydown', (e) => {
if (e.key === 'Enter' && composing) {
e.preventDefault();
e.stopImmediatePropagation();
}
}, true);
})();The key insight is the 30ms delay on Suggested fix directionThe same delay-based approach could be applied at the native layer: instead of checking |
This branch was successfully deployed
2 participants
Summary
Testing
./scripts/reload.sh --tag issue-1814-browser-ime-enterDemo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Keep Return and keypad Enter on the IME composition path while marked text is active, so confirming candidates doesn’t submit web forms in the embedded browser. Closes #1814.
NSTextInputClient/NSTextViewand passfirstResponderHasMarkedTextinto Return/Enter routing.Written for commit 7d0dc4e. Summary will update on new commits.
Summary by CodeRabbit