Skip to content

Fix arrow keys not working in browser surfaces and IME candidate selection #1423

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

Closed
jtdoth wants to merge 1 commit into from
Closed

Fix arrow keys not working in browser surfaces and IME candidate selection #1423

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 46 additions & 19 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,18 @@ private func cmuxOwningGhosttyView(for view: NSView) -> GhosttyNSView? {
return nil
}

/// Returns true when the responder (or one of its view-hierarchy ancestors)
/// is a WKWebView. Used to detect that the first responder is inside browser
/// web content (e.g. a textarea) rather than the address bar.
private func cmuxResponderIsInsideWebView(_ responder: NSResponder) -> Bool {
guard var current = responder as? NSView else { return false }
while true {
if current is WKWebView { return true }
guard let parent = current.superview else { return false }
current = parent
}
}

#if DEBUG
func browserZoomShortcutTraceCandidate(
flags: NSEvent.ModifierFlags,
Expand Down Expand Up @@ -8015,20 +8027,25 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
}

// Guard against stale browserAddressBarFocusedPanelId after focus transitions
// (e.g., split that doesn't properly blur the address bar). If the first responder
// is a terminal surface, the address bar can't be focused.
// (e.g., split that doesn't properly blur the address bar, or clicking into a
// textarea/input inside the WebView). If the first responder is a terminal surface
// or inside a WKWebView, the address bar can't be focused.
if browserAddressBarFocusedPanelId != nil,
cmuxOwningGhosttyView(for: NSApp.keyWindow?.firstResponder) != nil {
let firstResponder = NSApp.keyWindow?.firstResponder {
let isTerminalResponder = cmuxOwningGhosttyView(for: firstResponder) != nil
if isTerminalResponder || cmuxResponderIsInsideWebView(firstResponder) {
#if DEBUG
let stalePanelToken = browserAddressBarFocusedPanelId.map { String($0.uuidString.prefix(5)) } ?? "nil"
let firstResponderType = NSApp.keyWindow?.firstResponder.map { String(describing: type(of: $0)) } ?? "nil"
dlog(
"browser.focus.addressBar.staleClear panel=\(stalePanelToken) " +
"reason=terminal_first_responder fr=\(firstResponderType)"
)
let stalePanelToken = browserAddressBarFocusedPanelId.map { String($0.uuidString.prefix(5)) } ?? "nil"
let firstResponderType = String(describing: type(of: firstResponder))
let reason = isTerminalResponder ? "terminal_first_responder" : "webview_first_responder"
dlog(
"browser.focus.addressBar.staleClear panel=\(stalePanelToken) " +
"reason=\(reason) fr=\(firstResponderType)"
)
#endif
browserAddressBarFocusedPanelId = nil
stopBrowserOmnibarSelectionRepeat()
browserAddressBarFocusedPanelId = nil
stopBrowserOmnibarSelectionRepeat()
}
}

// Keep Cmd+P/Cmd+N inside the focused browser omnibar for Chrome-like
Expand Down Expand Up @@ -11130,16 +11147,13 @@ private extension NSWindow {
Self.cmuxOwningWebView(for: $0, in: self, event: event)
}
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
// process it. Cmd-based shortcuts should still work during composition since
// Cmd is never part of IME input sequences.
if ghosttyView.hasMarkedText(), !event.modifierFlags.intersection(.deviceIndependentFlagsMask).contains(.command) {
return cmux_performKeyEquivalent(with: event)
}

let flags = event.modifierFlags.intersection(.deviceIndependentFlagsMask)
if !flags.contains(.command) {
// Route non-Command keys directly to the terminal, bypassing the
// SwiftUI content view hierarchy. This covers both normal typing
// and IME composition (where GhosttyNSView.performKeyEquivalent
// returns false so the event flows through keyDown → interpretKeyEvents
// to the input method).
let result = ghosttyView.performKeyEquivalent(with: event)
#if DEBUG
dlog(" → ghostty direct: \(result)")
Expand Down Expand Up @@ -11198,6 +11212,19 @@ private extension NSWindow {
return true
}

// SwiftUI hosting view interception fix for browser surfaces: after app-level
// shortcuts have been checked above, route remaining keys directly to the
// WebView. Without this, arrow keys (and Cmd+arrow for jump-to-start/end)
// in web textareas are swallowed by SwiftUI's broken focus state after a
// browser panel has been in the responder chain.
if let webView = firstResponderWebView {
let result = webView.performKeyEquivalent(with: event)
#if DEBUG
dlog(" → webview direct: \(result)")
#endif
return result
}
Comment on lines +11220 to +11226

@coderabbitai coderabbitai Bot Mar 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fall through when WebKit declines a Command equivalent.

Line 11225 returns result unconditionally. If WKWebView.performKeyEquivalent(with:) returns false for a Cmd shortcut it does not own, the original NSWindow.performKeyEquivalent(with:) never runs, so window/menu handlers lose their chance. That can bypass repo-specific handlers like browser-popup Cmd+W, and native fallbacks such as window cycling.

💡 Suggested fix 
         if let webView = firstResponderWebView {
             let result = webView.performKeyEquivalent(with: event)
 `#if` DEBUG
             dlog("  → webview direct: \(result)")
 `#endif`
-            return result
+            if result || !event.modifierFlags
+                .intersection(.deviceIndependentFlagsMask)
+                .contains(.command) {
+                return result
+            }
         }

Based on learnings: BrowserPopupPanel.performKeyEquivalent is the primary handler to close popups.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 11220 - 11226,
firstResponderWebView's handling should only short-circuit when it actually
handled the key event; change the logic around firstResponderWebView and
webView.performKeyEquivalent(with:) so you only return the result when it's
true, otherwise allow the method to fall through and let
NSWindow.performKeyEquivalent (and BrowserPopupPanel.performKeyEquivalent) run;
locate the block referencing firstResponderWebView and
WKWebView.performKeyEquivalent(with:) and adjust the conditional to return only
on true so menu/window handlers still get a chance on false.

@jtdoth jtdoth Mar 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is a false positive — CmuxWebView.performKeyEquivalent already handles Command key routing internally:

  1. shouldRouteCommandEquivalentDirectlyToMainMenu → tries NSApp.mainMenu.performKeyEquivalent (line 155)
  2. Falls back to handleBrowserSurfaceKeyEquivalent (line 164)
  3. Falls through to super.performKeyEquivalent (WKWebView, line 171)

All menu items and app shortcuts are resolved inside the CmuxWebView override before it returns. If it still returns false, it means no handler claimed the event — the same outcome the original NSWindow.performKeyEquivalent walk would produce, except the original walk also risks hitting the broken SwiftUI hosting view path (which is exactly the bug this PR fixes).

The only Command key where shouldRouteCommandEquivalentDirectlyToMainMenu returns false is Cmd+` (keyCode 50), which is intentionally excluded (confirmed by existing test testCmdBacktickDoesNotRouteDirectlyToMainMenuWhenWebViewIsFirstResponder). In that case, super.performKeyEquivalent returns false, and our code correctly propagates that — same as the old path.

@coderabbitai coderabbitai Bot Mar 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

@coderabbit-user Thanks for the added context. I’ll verify your claim against the repo and get back with a short confirmation or tweak if anything looks off.

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find CmuxWebView and performKeyEquivalent overrides =="
rg -nP --no-ignore-vcs -C3 '(class|extension)\s+CmuxWebView\b' --glob '**/*.swift' || true
rg -n --no-ignore-vcs -C5 'performKeyEquivalent\s*\(\s*with:' --glob '**/*.swift' || true

echo
echo "== Search for handleBrowserSurfaceKeyEquivalent =="
rg -n --no-ignore-vcs -C3 '\bhandleBrowserSurfaceKeyEquivalent\b' --glob '**/*.swift' || true

echo
echo "== Search BrowserPopupPanel and its performKeyEquivalent =="
rg -nP --no-ignore-vcs -C4 '(class|extension)\s+BrowserPopupPanel\b' --glob '**/*.swift' || true
rg -n --no-ignore-vcs -C5 'BrowserPopupPanel.*performKeyEquivalent|extension\s+BrowserPopupPanel|performKeyEquivalent\s*\(\s*with:' Sources/Panels/**/*.swift || true

echo
echo "== Find string identifier used for browser popups to cross-check learning =="
rg -n --no-ignore-vcs -C2 'cmux\.browser-popup' --glob '**/*.swift' || true

Length of output: 26926

@coderabbit-user Agreed — this was a false positive. I verified the routing:

  • In NSWindow.cmux_performKeyEquivalent, we first give the app-level handler a chance: handleBrowserSurfaceKeyEquivalent (Sources/AppDelegate.swift:11208–11213). Only then do we call firstResponderWebView.performKeyEquivalent and return its result (Lines 11220–11226).

  • CmuxWebView.performKeyEquivalent itself handles Command routing:

    • If shouldRouteCommandEquivalentDirectlyToMainMenu(event) is true, it tries NSApp.mainMenu.performKeyEquivalent first (Sources/Panels/CmuxWebView.swift:155–160).
    • Then falls back to AppDelegate.shared?.handleBrowserSurfaceKeyEquivalent (Lines 164–168).
    • Then to super.performKeyEquivalent (Lines 171–175).
    • For non-Command, it directly calls super (Lines 139–144).

  • Tests cover this: Cmd+N/W/R are consumed via the menu when WebView is first responder, while Return / Cmd+Return are not, and Cmd+` is explicitly excluded from direct-menu routing (cmuxTests/CmuxWebViewKeyEquivalentTests.swift:184–209, 220–246, 814–820).

Given this, allowing fall-through to the original NSWindow.performKeyEquivalent when the WebView returns false would just re-enter the SwiftUI hierarchy we’re intentionally bypassing for this bug, without adding any valid handler opportunity. Also, there’s no BrowserPopupPanel or “cmux.browser-popup” identifier in this repo, so that prior guideline doesn’t apply here.

I’m retracting the concern; no change needed.

✏️ Learnings added 
Learnt from: jt-hsiao
Repo: manaflow-ai/cmux PR: 1423
File: Sources/AppDelegate.swift:11220-11226
Timestamp: 2026-03-14T07:05:52.379Z
Learning: Repo: manaflow-ai/cmux — In Sources/Panels/CmuxWebView.swift, performKeyEquivalent(with:) handles Command-key routing end-to-end: (1) if allowed, route to NSApp.mainMenu.performKeyEquivalent; (2) fall back to AppDelegate.shared?.handleBrowserSurfaceKeyEquivalent(event) for non–menu-backed app shortcuts; (3) fall back to super.performKeyEquivalent. For non-Command keys it calls super directly. Therefore, in NSWindow.cmux_performKeyEquivalent (Sources/AppDelegate.swift), it is correct to call firstResponderWebView.performKeyEquivalent and return its Bool unconditionally to avoid re-entering SwiftUI’s performKeyEquivalent path that can swallow keys after WKWebView focus.

Learnt from: jt-hsiao
Repo: manaflow-ai/cmux PR: 1423
File: Sources/AppDelegate.swift:11220-11226
Timestamp: 2026-03-14T07:05:52.379Z
Learning: Repo: manaflow-ai/cmux — Command-backtick (Cmd+`, keyCode 50) is intentionally excluded from direct menu routing by shouldRouteCommandEquivalentDirectlyToMainMenu; tests assert this behavior. Do not add a window/main-menu bypass for Cmd+`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 1150
File: Sources/AppDelegate.swift:7394-7401
Timestamp: 2026-03-10T10:24:14.017Z
Learning: Repo: manaflow-ai/cmux — For Cmd+W behavior with browser popups: BrowserPopupPanel.performKeyEquivalent is the primary handler to close popups. AppDelegate.handleCustomShortcut(_:), in its Cmd+W fallback, must check both NSApp.keyWindow and event.window for identifier "cmux.browser-popup" and close it if found, before routing to workspace/settings close logic.

Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-12T21:54:38.291Z
Learning: Applies to Sources/**/*TerminalWindowPortal.swift : In `WindowTerminalHostView.hitTest()` in `TerminalWindowPortal.swift`: all divider/sidebar/drag routing must be gated to pointer events only. Do not add work outside the `isPointerEvent` guard to avoid typing latency.

Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 803
File: Sources/Panels/BrowserPopupWindowController.swift:36-38
Timestamp: 2026-03-04T05:11:56.373Z
Learning: In Sources/Panels/BrowserPopupWindowController.swift and Sources/Panels/BrowserPanel.swift, `webView.isInspectable = true` (guarded by `#available(macOS 13.3, *)`) is intentionally enabled in all builds — not just DEBUG — because cmux is a developer tool and full Web Inspector access is desired in production builds as well. Do not flag this as a security concern.

Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-12T21:54:38.291Z
Learning: Applies to Sources/**/*.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`. Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.

Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.

Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*TerminalView.swift : Terminal find layering contract: SurfaceSearchOverlay must be mounted from GhosttySurfaceScrollView in Sources/GhosttyTerminalView.swift (AppKit portal layer), not from SwiftUI panel containers

Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*.swift : Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection

Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 762
File: Sources/ContentView.swift:5784-5788
Timestamp: 2026-03-06T02:00:30.333Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), ShortcutHintModifierPolicy.shouldShowHints is Command-only: it returns true only when modifierFlags normalized to [.command]. Control-hold must not show sidebar/titlebar shortcut hints, and close-button suppression in TabItemView must remain tied to showsModifierShortcutHints (Command hold) or the always-show debug flag.

Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.

Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).

Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:37.952Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus. Do not change its sync‑seeding behavior within feature‑scoped PRs; treat brief initial flashes as consistent with existing submenus. Any UX improvement (e.g., synchronous seeding on forced corpus refresh) should be implemented and tested globally in a dedicated follow‑up PR.

Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/TerminalController.swift:3180-3193
Timestamp: 2026-03-09T02:08:14.574Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceClearTags(params:) must only clear all tags when the "source" key is absent. If "source" is present but blank or non-string (v2String(...) returns nil), the API should return invalid_params. Current implementation uses hasSourceKey = params.keys.contains("source") and guards with if hasSourceKey && source == nil { return .err(...)}.

Learnt from: 0xble
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-09T21:53:05.451Z
Learning: Repo: manaflow-ai/cmux — For browser portals, visibility resync is centralized in BrowserWindowPortalRegistry.updateEntryVisibleInUI(for: WKWebView, visibleInUI: Bool), which immediately synchronizes the WKWebView on visibility changes. Call sites (e.g., Workspace.reconcilePanelPortalVisibilityForCurrentLayout()) should use this helper instead of ad‑hoc resync logic.

Learnt from: mdsakalu
Repo: manaflow-ai/cmux PR: 510
File: Sources/GhosttyTerminalView.swift:2276-2336
Timestamp: 2026-03-11T04:48:32.420Z
Learning: In cmux Sources/GhosttyTerminalView.swift, when implementing initial-present recovery for Ghostty terminals, avoid ghostty_surface_draw loops. Re-read self.surface after view.forceRefreshSurface() (since the surface may be torn down/reparented), then use occlusion toggle (ghostty_surface_set_occlusion false/true) plus ghostty_surface_refresh to wake the renderer. Cancel retries once the layer has contents or the retry budget is exhausted.

Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:12.296Z
Learning: In PRs affecting this repository, limit the scope of localization (i18n) changes to Japanese translations for the file Sources/AppDelegate.swift. Do not include UX enhancements (e.g., preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels) in this PR. Open a separate follow-up issue to address any UX-related changes to avoid scope creep and keep localization review focused.

Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.

Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.

Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.


// When the terminal is focused, skip the full NSWindow.performKeyEquivalent
// (which walks the SwiftUI content view hierarchy) and dispatch Command-key
// events directly to the main menu. This avoids the broken SwiftUI focus path.
Expand Down