Skip to content

fix(browser): keep IME Enter on composition path #2108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 2 commits into from
Mar 25, 2026
Merged

fix(browser): keep IME Enter on composition path #2108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
118d5ae
test: cover browser IME Enter composition routing
austinywang Mar 25, 2026
7d0dc4e
fix(browser): keep IME Enter on composition path
austinywang Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1504,12 +1504,31 @@ func browserOmnibarShouldSubmitOnReturn(flags: NSEvent.ModifierFlags) -> Bool {
return normalizedFlags == [] || normalizedFlags == [.shift]
}

func browserResponderHasMarkedText(_ responder: NSResponder?) -> Bool {
guard let responder else { return false }

// During IME composition, Return/Enter belongs to the text system so the
// candidate list can commit or confirm the marked text.
if let textInputClient = responder as? NSTextInputClient {
return textInputClient.hasMarkedText()
}

if let textField = responder as? NSTextField,
let editor = textField.currentEditor() as? NSTextView {
return editor.hasMarkedText()
}
Comment on lines +1516 to +1519

@greptile-apps greptile-apps Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.


return false
}

func shouldDispatchBrowserReturnViaFirstResponderKeyDown(
keyCode: UInt16,
firstResponderIsBrowser: Bool,
firstResponderHasMarkedText: Bool = false,
flags: NSEvent.ModifierFlags
) -> Bool {
guard firstResponderIsBrowser else { return false }
guard !firstResponderHasMarkedText else { return false }
guard keyCode == 36 || keyCode == 76 else { return false }
// Keep browser Return forwarding narrow: only plain/Shift Return should be
// treated as submit-intent. Command-modified Return is reserved for app shortcuts
Expand Down Expand Up @@ -12391,6 +12410,7 @@ private extension NSWindow {
let firstResponderWebView = self.firstResponder.flatMap {
Self.cmuxOwningWebView(for: $0, in: self, event: event)
}
let firstResponderHasMarkedText = browserResponderHasMarkedText(self.firstResponder)
if let ghosttyView = firstResponderGhosttyView {
// If the IME is composing and the key has no Cmd modifier, don't intercept —
// let it flow through normal AppKit event dispatch so the input method can
Expand Down Expand Up @@ -12434,6 +12454,7 @@ private extension NSWindow {
if shouldDispatchBrowserReturnViaFirstResponderKeyDown(
keyCode: event.keyCode,
firstResponderIsBrowser: firstResponderWebView != nil,
firstResponderHasMarkedText: firstResponderHasMarkedText,
flags: event.modifierFlags
) {
// Forwarding keyDown can re-enter performKeyEquivalent in WebKit/AppKit internals.
Expand Down
138 changes: 138 additions & 0 deletions cmuxTests/BrowserConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ func installCmuxUnitTestInspectorOverride() {
cmuxUnitTestInspectorOverrideInstalled = true
}

private final class BrowserMarkedTextProbeTextView: NSTextView {
var hasMarkedTextForTesting = false
private(set) var keyDownEvents: [NSEvent] = []

override var acceptsFirstResponder: Bool { true }

override func hasMarkedText() -> Bool {
hasMarkedTextForTesting
}

override func keyDown(with event: NSEvent) {
keyDownEvents.append(event)
}
}

final class CmuxWebViewKeyEquivalentTests: XCTestCase {
private final class ActionSpy: NSObject {
private(set) var invoked: Bool = false
Expand Down Expand Up @@ -2455,6 +2470,107 @@ final class BrowserOmnibarCommandNavigationTests: XCTestCase {
}


final class BrowserIMEKeyDownRoutingTests: XCTestCase {
@MainActor
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(
Comment on lines +2475 to 2576

@greptile-apps greptile-apps Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Duplicated integration test setup — consider a shared helper

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!

Expand Down Expand Up @@ -2496,6 +2612,28 @@ final class BrowserReturnKeyDownRoutingTests: XCTestCase {
)
}

func testDoesNotRouteReturnWhenBrowserFirstResponderHasMarkedText() {
XCTAssertFalse(
shouldDispatchBrowserReturnViaFirstResponderKeyDown(
keyCode: 36,
firstResponderIsBrowser: true,
firstResponderHasMarkedText: true,
flags: []
)
)
}

func testDoesNotRouteKeypadEnterWhenBrowserFirstResponderHasMarkedText() {
XCTAssertFalse(
shouldDispatchBrowserReturnViaFirstResponderKeyDown(
keyCode: 76,
firstResponderIsBrowser: true,
firstResponderHasMarkedText: true,
flags: []
)
)
}

func testRoutesForShiftReturnWhenBrowserFirstResponder() {
XCTAssertTrue(
shouldDispatchBrowserReturnViaFirstResponderKeyDown(
Expand Down
Loading