browser inputs: dispatch pointerenter non-bubbling to match mouseenter and spec - #5958
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR expands browser automation: adds CLI routing and helpers for new browser verbs (react-grab, devtools, focus-mode, zoom, history), extends TabManager with react-grab surface override API, centralizes injected JS input helpers and keyboard/text event handling, implements browser-action utilities and handlers, and updates docs. ChangesBrowser Command Automation Expansion
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Parser
participant RouteHelpers as Route Helpers
participant TerminalController as TerminalController
participant TabManager as TabManager
participant BrowserJS as Browser JS
CLI->>RouteHelpers: extract verb, workspace, surface from args
RouteHelpers->>RouteHelpers: resolve workspace/window via fallback
RouteHelpers->>TerminalController: dispatch browser action
TerminalController->>TabManager: toggleReactGrab(surface overrides)
TabManager->>BrowserJS: trigger React Grab pasteback
BrowserJS->>BrowserJS: return terminal panel ID
TerminalController->>BrowserJS: dispatch keyboard/mouse via __cmuxKey/__cmuxClick
BrowserJS->>BrowserJS: synthesize event sequence and honor beforeinput cancellation
sequenceDiagram
participant Action as Browser Action
participant Util as Resolution Util
participant Surface as Browser Surface
participant Store as History Store
Action->>Util: resolve browser surface (surface_id precedence)
alt surface_id provided
Util->>Surface: use explicit surface
else no explicit surface
Util->>Surface: use focused or sole browser
end
alt history.clear action
Action->>Store: clear default profile history (force=true required)
end
Util->>Action: return standardized payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 3 warnings)
✅ Passed checks (14 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| for key in keys where v2HasNonNullParam(params, key) && v2UUID(params, key) == nil { | ||
| return .err(code: "invalid_params", message: "Unresolved \(key)", data: nil) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Stale workspace UUID not rejected
Medium Severity
New v2RejectUnresolvedHandles treats workspace_id as resolved whenever v2UUID parses a UUID string, without checking that workspace exists in the target window. A stale but syntactically valid workspace_id yields a generic not-found instead of an explicit unresolved-handle error.
Reviewed by Cursor Bugbot for commit d66f0fc. Configure here.
Greptile SummaryThis commit is a single-focus correctness fix:
Confidence Score: 5/5This is a targeted one-line correctness fix; no regressions are expected and existing callers correctly receive the old default behavior. The change adds a No files require special attention; the change is confined to a single JavaScript helper string inside Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as cmuxClick/cmuxHover
participant P as __cmuxPointer
participant DOM as DOM Element
Caller->>P: "pointerover (bubbles default=true)"
P->>DOM: dispatchEvent(pointerover, bubbles:true)
Caller->>P: "pointerenter (bubbles=false) FIXED"
P->>DOM: dispatchEvent(pointerenter, bubbles:false)
Note over DOM: Only target receives event, ancestors skipped
Caller->>P: "pointermove (bubbles default=true)"
P->>DOM: dispatchEvent(pointermove, bubbles:true)
Reviews (2): Last reviewed commit: "browser inputs: dispatch pointerenter no..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| v2BrowserSelectorAction(params: params, actionName: "click") { selectorLiteral in | ||
| """ | ||
| (() => { | ||
| \(Self.browserInputHelpers) | ||
| const el = document.querySelector(\(selectorLiteral)); | ||
| if (!el) return { ok: false, error: 'not_found' }; | ||
| if (el.disabled) return { ok: false, error: 'disabled' }; | ||
| el.scrollIntoView({ block: 'nearest', inline: 'nearest' }); | ||
| if (typeof el.click === 'function') { | ||
| el.click(); | ||
| } else { | ||
| el.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, view: window, detail: 1 })); | ||
| } | ||
| __cmuxClick(el); | ||
| return { ok: true }; | ||
| })() |
There was a problem hiding this comment.
pointerenter incorrectly bubbles
__cmuxPointer hardcodes bubbles: true for all pointer events, including pointerenter. Unlike pointermove/pointerdown, pointerenter does not bubble in the spec — and this is already handled correctly for its mouse counterpart (__cmuxMouse(el, 'mouseenter', c, 0, 0, false)). When pointerenter bubbles, every ancestor that has a pointerenter listener receives a spurious hover-enter signal, which can open parent menus, trigger parent tooltip logic, or corrupt hover state in React/Vue pointer-tracking components. The fix is to pass bubbles: false when dispatching pointerenter (and pointerleave, if added later).
| /// vanilla handlers all fire. Define them once at the top of an injected snippet, then call | ||
| /// `__cmuxClick(el)`, `__cmuxHover(el)`, `__cmuxSetChecked(el, desired)`, and `__cmuxKey(t,type,key)`. | ||
| private static let browserInputHelpers = """ | ||
| function __cmuxCenter(el){const r=el.getBoundingClientRect();return {x:Math.floor(r.left+Math.min(r.width,r.width/2)),y:Math.floor(r.top+Math.min(r.height,r.height/2))};} |
There was a problem hiding this comment.
Math.min(r.width, r.width/2) is always r.width/2
getBoundingClientRect() always returns non-negative width/height, so r.width/2 ≤ r.width is always true and Math.min adds no semantic value. The simpler form is clearer and avoids a head-scratch moment for any reader trying to understand why there's a min here.
| function __cmuxCenter(el){const r=el.getBoundingClientRect();return {x:Math.floor(r.left+Math.min(r.width,r.width/2)),y:Math.floor(r.top+Math.min(r.height,r.height/2))};} | |
| function __cmuxCenter(el){const r=el.getBoundingClientRect();return {x:Math.floor(r.left+r.width/2),y:Math.floor(r.top+r.height/2)};} |
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!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c86dba6. Configure here.
| __cmuxPointer(el,'pointerdown',c,1);__cmuxMouse(el,'mousedown',c,1,1); | ||
| if(typeof el.focus==='function'){try{el.focus({preventScroll:true});}catch(e){try{el.focus();}catch(e2){}}} | ||
| __cmuxPointer(el,'pointerup',c,0);__cmuxMouse(el,'mouseup',c,0,1); | ||
| if(typeof el.click==='function'){el.click();}else{__cmuxMouse(el,'click',c,0,1);} |
There was a problem hiding this comment.
Click helper fires events twice
High Severity
The new __cmuxClick helper dispatches a full synthetic pointer and mouse down/up sequence and then calls el.click(). In WebKit that typically runs another activation, so browser click (and dblclick, which calls it twice) can invoke listeners twice and leave toggle controls back in their original state.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c86dba6. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Line 32773: Update the usage/help string that currently reads "browser
focus-mode enter|exit|toggle [--surface <id>]" to also advertise the accepted
aliases "on" and "off" (e.g., "browser focus-mode enter|exit|toggle|on|off
[--surface <id>]") so users see all supported modes; locate the command/usage
definition in CLI/cmux.swift (the focus-mode usage string) and modify that
literal so the help output includes the on/off aliases.
- Around line 11827-11835: The history-clear code path currently ignores routing
flags; before calling client.sendV2("browser.history.clear", ...), detect any
supplied routing flags (e.g., --surface, --workspace, --window via hasFlag and
browserActionVerbArgs()) and either (preferred) pass them through the existing
handle-validation/resolution helper (e.g., resolve/validate routing handles) and
throw a CLIError if any supplied handle cannot be resolved, or (fallback) reject
the command immediately when any of those flags are present by throwing a
CLIError explaining they are unsupported for this destructive action; ensure the
check happens before the hasFlag(--force) guard and before client.sendV2 is
invoked.
In `@Sources/TabManager.swift`:
- Around line 6280-6292: The logic that computes returnTerminalPanelId
incorrectly clears a route-derived return when browserSurfaceId is provided;
change the branching in the block that sets returnTerminalPanelId so that: if
returnTerminalSurfaceId is non-nil use it (as now); else if
route?.returnTerminalPanelId is non-nil and workspace.panels[thatId]?.panelType
== .terminal then use the route-derived ID (independent of browserSurfaceId);
otherwise if browserSurfaceId is nil fall back to nil; ensure you reference
returnTerminalSurfaceId, browserSurfaceId, route?.returnTerminalPanelId and
workspace.panels when making the checks so the route-derived terminal is
preserved unless explicitly overridden by --return-to or is invalid.
In `@Sources/TerminalController.swift`:
- Around line 13281-13290: v2RejectUnresolvedHandles currently only checks that
a param parses to a UUID via v2UUID; update it to also verify the UUID actually
resolves to the correct resource and context (surface, return_to type,
workspace, window) so supplied-but-wrong-workspace/type handles are rejected.
For keys like "surface_id", "return_to", "workspace_id", "window_id" call the
appropriate resolver (e.g.
v2ResolveSurface/v2ResolveReturnTo/v2ResolveWorkspace/v2ResolveWindow or the
central handle-resolver used elsewhere) instead of just v2UUID, and if
resolution fails or the resolved object is of the wrong type/owner return
.err(code: "invalid_params", message: "Unresolved <key>", data: nil); keep the
existing presence check using v2HasNonNullParam and reuse v2UUID only to obtain
the candidate id before resolution.
- Around line 12646-12660: The textContent/contenteditable branch currently
skips dispatching a cancelable beforeinput and thus ignores cancellation; before
mutating el.textContent in the else branch, dispatch a cancelable
InputEvent('beforeinput', { bubbles: true, cancelable: true, inputType:
'insertText', data: chunk }) (wrap in try/catch), check its boolean return (as
done in the 'value' branch) and if it returns false return { ok: false, error:
'input_rejected' }; only then perform el.textContent = ... and dispatch the
input/change events as already implemented so contenteditable targets honor
beforeinput cancellation (mirror the behavior around the 'value' in el path and
Self.reactCompatibleSetValue).
- Around line 12713-12718: kdNotPrevented records whether the synthetic keydown
was canceled but code still always dispatches keypress for printable keys;
change the logic so __cmuxKey(target, 'keypress', k) is only invoked when
kdNotPrevented is true (i.e. when the keydown was not prevented) — update the
kpNotPrevented assignment to check kdNotPrevented before calling __cmuxKey (keep
the existing printable-key/Enter condition), leaving __cmuxKey(target, 'keyup',
k) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: befb4ffc-4266-43a0-b469-b49db5f1bd64
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
CLI/cmux.swiftSources/TabManager.swiftSources/TerminalController.swiftweb/app/[locale]/docs/browser-automation/page.tsx
| if subcommand == "history" { | ||
| let verb = browserActionVerbArgs().first?.lowercased() ?? "clear" | ||
| guard verb == "clear" else { | ||
| throw CLIError(message: "Unsupported browser history subcommand: \(verb) (expected: clear)") | ||
| } | ||
| guard hasFlag(subArgs, name: "--force") || hasFlag(subArgs, name: "--yes") else { | ||
| throw CLIError(message: "browser history clear permanently deletes the default browser profile's history (same as the View menu's Clear Browser History); pass --force to confirm") | ||
| } | ||
| let payload = try client.sendV2(method: "browser.history.clear", params: ["force": true]) |
There was a problem hiding this comment.
Reject or validate routing flags before clearing history.
browser history clear bypasses the new handle-validation path entirely. If a caller supplies --surface, --workspace, or --window, those values are silently ignored here, so malformed explicit handles do not fail fast and the command still clears the default profile history. That breaks the new “supplied-but-unresolvable handles are hard errors” contract and is risky on a destructive action.
Suggested fix
if subcommand == "history" {
let verb = browserActionVerbArgs().first?.lowercased() ?? "clear"
guard verb == "clear" else {
throw CLIError(message: "Unsupported browser history subcommand: \(verb) (expected: clear)")
}
guard hasFlag(subArgs, name: "--force") || hasFlag(subArgs, name: "--yes") else {
throw CLIError(message: "browser history clear permanently deletes the default browser profile's history (same as the View menu's Clear Browser History); pass --force to confirm")
}
+ if surfaceRaw != nil || parseOption(subArgs, name: "--workspace").0 != nil || parseOption(subArgs, name: "--window").0 != nil {
+ throw CLIError(message: "browser history clear does not accept --surface, --workspace, or --window")
+ }
let payload = try client.sendV2(method: "browser.history.clear", params: ["force": true])
output(payload, fallback: "OK")
return
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/cmux.swift` around lines 11827 - 11835, The history-clear code path
currently ignores routing flags; before calling
client.sendV2("browser.history.clear", ...), detect any supplied routing flags
(e.g., --surface, --workspace, --window via hasFlag and browserActionVerbArgs())
and either (preferred) pass them through the existing
handle-validation/resolution helper (e.g., resolve/validate routing handles) and
throw a CLIError if any supplied handle cannot be resolved, or (fallback) reject
the command immediately when any of those flags are present by throwing a
CLIError explaining they are unsupported for this destructive action; ensure the
check happens before the hasFlag(--force) guard and before client.sendV2 is
invoked.
| browser back|forward|reload [--snapshot-after] | ||
| browser react-grab toggle [--surface <id>] [--return-to <terminal-surface>] | ||
| browser devtools toggle|console [--surface <id>] | ||
| browser focus-mode enter|exit|toggle [--surface <id>] |
There was a problem hiding this comment.
Document the on / off focus-mode aliases here.
The parser accepts enter, exit, toggle, on, and off, but this usage text only advertises the first three. That makes two supported modes effectively undiscoverable from cmux browser --help.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/cmux.swift` at line 32773, Update the usage/help string that currently
reads "browser focus-mode enter|exit|toggle [--surface <id>]" to also advertise
the accepted aliases "on" and "off" (e.g., "browser focus-mode
enter|exit|toggle|on|off [--surface <id>]") so users see all supported modes;
locate the command/usage definition in CLI/cmux.swift (the focus-mode usage
string) and modify that literal so the help output includes the on/off aliases.
| // Return terminal: an explicit return surface is authoritative (must be a terminal in | ||
| // this workspace, no fallback) so pasteback never silently goes to the wrong terminal. | ||
| // With no explicit return, adopt the route's terminal only when the browser also came | ||
| // from the route (matching shortcut semantics). | ||
| let returnTerminalPanelId: UUID? | ||
| if let explicit = returnTerminalSurfaceId { | ||
| guard workspace.panels[explicit]?.panelType == .terminal else { return nil } | ||
| returnTerminalPanelId = explicit | ||
| } else if browserSurfaceId == nil { | ||
| returnTerminalPanelId = route?.returnTerminalPanelId | ||
| } else { | ||
| returnTerminalPanelId = nil | ||
| } |
There was a problem hiding this comment.
Keep the route-derived return terminal when only the browser target is overridden.
This branch makes an explicit browser target clear the implicit returnTerminalPanelId, so browser react-grab toggle --surface <browser> loses pasteback to the caller’s terminal unless --return-to is also passed. The PR contract describes --surface and --return-to as independent overrides, so overriding the browser target should not suppress an otherwise valid route-derived return target.
Proposed fix
- } else if browserSurfaceId == nil {
- returnTerminalPanelId = route?.returnTerminalPanelId
- } else {
- returnTerminalPanelId = nil
+ } else {
+ returnTerminalPanelId = route?.returnTerminalPanelId
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TabManager.swift` around lines 6280 - 6292, The logic that computes
returnTerminalPanelId incorrectly clears a route-derived return when
browserSurfaceId is provided; change the branching in the block that sets
returnTerminalPanelId so that: if returnTerminalSurfaceId is non-nil use it (as
now); else if route?.returnTerminalPanelId is non-nil and
workspace.panels[thatId]?.panelType == .terminal then use the route-derived ID
(independent of browserSurfaceId); otherwise if browserSurfaceId is nil fall
back to nil; ensure you reference returnTerminalSurfaceId, browserSurfaceId,
route?.returnTerminalPanelId and workspace.panels when making the checks so the
route-derived terminal is preserved unless explicitly overridden by --return-to
or is invalid.
| if ('value' in el) { | ||
| const newValue = (el.value || '') + chunk; | ||
| // beforeinput is cancelable; honor a page that rejects the edit (input masks, | ||
| // controlled editors) instead of forcing the value and drifting from app state. | ||
| let proceed = true; | ||
| try { proceed = el.dispatchEvent(new InputEvent('beforeinput', { bubbles: true, cancelable: true, inputType: 'insertText', data: chunk })); } catch (e) {} | ||
| if (!proceed) return { ok: false, error: 'input_rejected' }; | ||
| \(Self.reactCompatibleSetValue) | ||
| el.dispatchEvent(new Event('input', { bubbles: true })); | ||
| try { el.dispatchEvent(new InputEvent('input', { bubbles: true, inputType: 'insertText', data: chunk })); } | ||
| catch (e) { el.dispatchEvent(new Event('input', { bubbles: true })); } | ||
| el.dispatchEvent(new Event('change', { bubbles: true })); | ||
| } else { | ||
| el.textContent = (el.textContent || '') + chunk; | ||
| try { el.dispatchEvent(new InputEvent('input', { bubbles: true, inputType: 'insertText', data: chunk })); } catch (e) {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code around the referenced lines.
sed -n '12620,12710p' Sources/TerminalController.swift | nl -ba | sed -n '1,120p'
# Also extract a bit wider to catch surrounding context where input_rejected is handled.
sed -n '12580,12780p' Sources/TerminalController.swift | nl -ba | sed -n '1,220p'
# Search within the file for the rejection sentinel and the two branches.
rg -n "input_rejected|beforeinput|reactCompatibleSetValue|value' in el|textContent" Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Helper: print with 1-based absolute line numbers
print_range () {
local start="$1" end="$2"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d %s\n", NR, $0}' "Sources/TerminalController.swift"
}
print_range 12620 12710
print_range 12580 12780
rg -n "input_rejected|beforeinput|reactCompatibleSetValue|value' in el|textContent" Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 22241
Honor canceled beforeinput for textContent/contenteditable targets (type + fill)
v2BrowserType/v2BrowserFill only return { ok: false, error: 'input_rejected' } when 'value' in el; the else path (el.textContent = ...) never dispatches a cancelable beforeinput nor respects its cancellation, so editors that land in the textContent branch can still be force-mutated and drift from controlled state (e.g., around Sources/TerminalController.swift lines 12646-12660 and 12680-12692).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 12646 - 12660, The
textContent/contenteditable branch currently skips dispatching a cancelable
beforeinput and thus ignores cancellation; before mutating el.textContent in the
else branch, dispatch a cancelable InputEvent('beforeinput', { bubbles: true,
cancelable: true, inputType: 'insertText', data: chunk }) (wrap in try/catch),
check its boolean return (as done in the 'value' branch) and if it returns false
return { ok: false, error: 'input_rejected' }; only then perform el.textContent
= ... and dispatch the input/change events as already implemented so
contenteditable targets honor beforeinput cancellation (mirror the behavior
around the 'value' in el path and Self.reactCompatibleSetValue).
| const kdNotPrevented = __cmuxKey(target, 'keydown', k); | ||
| // keypress historically fires for character-producing keys, which includes Enter and | ||
| // Space; many pages still bind submit/search to keypress for Enter. | ||
| let kpNotPrevented = true; | ||
| if (k.length === 1 || k === 'Enter') { kpNotPrevented = __cmuxKey(target, 'keypress', k); } | ||
| __cmuxKey(target, 'keyup', k); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact snippet in TerminalController.swift
rg -n "kdNotPrevented|kpNotPrevented|__cmuxKey\\(target, 'keypress'" Sources/TerminalController.swift
# Print the surrounding block (around the reported lines)
sed -n '12700,12760p' Sources/TerminalController.swift
# Also search for related keydown/keypress flow elsewhere in the file (cheap sanity check)
rg -n "__cmuxKey\\(target, 'keydown'|__cmuxKey\\(target, 'keypress'|__cmuxKey\\(target, 'keyup'" Sources/TerminalController.swift | head -n 50Repository: manaflow-ai/cmux
Length of output: 4615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "browserInputHelpers|__cmuxKey\\b|function __cmuxKey|const __cmuxKey|__cmuxKey\\s*=|kpNotPrevented|kdNotPrevented" Sources/TerminalController.swift
# Show the definition of browserInputHelpers
rg -n "browserInputHelpers" Sources/TerminalController.swift -n
python3 - <<'PY'
import re,sys,subprocess,os, textwrap, json
PY
# Print a wider chunk around browserInputHelpers definition (based on line numbers from search output)
# We'll fetch around the first occurrence for visibility.
first=$(rg -n "browserInputHelpers" Sources/TerminalController.swift | head -n1 | cut -d: -f1)
start=$((first-60))
end=$((first+220))
sed -n "${start},${end}p" Sources/TerminalController.swift
# Also inspect other nearby key event mappings (full coverage around v2BrowserPress + v2BrowserKeyDown)
rg -n "func v2BrowserPress|func v2BrowserKeyDown" Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 16930
🌐 Web query:
In browser, does calling preventDefault() on a keydown event prevent the subsequent keypress event from firing for printable keys/Enter?
💡 Result:
Yes, calling preventDefault on a keydown event typically prevents the subsequent keypress event from firing in modern web browsers [1][2][3]. When you call preventDefault on a keydown event, you are preventing the default action of that event [1]. Because the dispatch of a keypress event is often considered a default action of the preceding keydown event for printable characters, stopping the keydown event prevents the browser from firing the keypress event [1][2]. Important considerations: 1. Deprecation: The keypress event itself is deprecated and is no longer recommended for use [4][5][6]. Modern web development practices suggest using the beforeinput or keydown events instead [4][6]. 2. Browser Behavior: While this behavior is standard in modern browsers, it was not always consistent in older browser versions [2][7]. Some older implementations might have dispatched the keypress event regardless of whether the keydown event was prevented [2][8]. 3. Event Flow: Because keypress is a legacy event, relying on the specific sequence of keydown and keypress—or attempting to manipulate the browser's default behavior through these events—is discouraged in favor of more modern input handling methods [4][5].
Citations:
- 1: https://stackoverflow.com/questions/57400729/does-preventdefault-in-keydown-event-prevent-a-following-keypress-event
- 2: http://docs.w3cub.com/dom_events/keydown
- 3: https://code.cablelabs.com/App_Technologies/webkit/-/blob/b62310ff0a210e4423a9ef62b9440496f01e167e/LayoutTests/fast/events/keydown-keypress-preventDefault.html
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Element/keypress_event
- 5: https://webplatform.github.io/docs/dom/KeyboardEvent/keypress/
- 6: https://stackoverflow.com/questions/52882144/replacement-for-deprecated-keypress-dom-event
- 7: https://bugzilla.mozilla.org/show_bug.cgi?id=501496
- 8: https://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-October/066149.html
Suppress keypress when keydown was canceled.
kdNotPrevented captures whether the synthetic keydown event was canceled (__cmuxKey returns dispatchEvent’s cancel status), but keypress is still dispatched for printable keys / Enter regardless. In normal browser behavior, canceling keydown (preventDefault) prevents the follow-on keypress, so pages that block keydown can still receive a synthetic keypress here and run duplicate/forbidden handlers.
Suggested direction
- let kpNotPrevented = true;
- if (k.length === 1 || k === 'Enter') { kpNotPrevented = __cmuxKey(target, 'keypress', k); }
+ let kpNotPrevented = true;
+ if (kdNotPrevented && (k.length === 1 || k === 'Enter')) {
+ kpNotPrevented = __cmuxKey(target, 'keypress', k);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 12713 - 12718, kdNotPrevented
records whether the synthetic keydown was canceled but code still always
dispatches keypress for printable keys; change the logic so __cmuxKey(target,
'keypress', k) is only invoked when kdNotPrevented is true (i.e. when the
keydown was not prevented) — update the kpNotPrevented assignment to check
kdNotPrevented before calling __cmuxKey (keep the existing printable-key/Enter
condition), leaving __cmuxKey(target, 'keyup', k) unchanged.
| /// Returns an error if any of the given handle params is SUPPLIED but does not resolve. | ||
| /// v2UUID returns nil for both an absent param and a present-but-unresolvable handle (e.g. a | ||
| /// stale `surface:2`/`workspace:99` ref), so a supplied target must not be treated as omitted | ||
| /// and silently fall back to the focused/selected context. Returns nil when all are valid. | ||
| private func v2RejectUnresolvedHandles(_ params: [String: Any], _ keys: [String]) -> V2CallResult? { | ||
| // Use v2HasNonNullParam (not v2String) for presence: v2String trims empties to nil, so an | ||
| // empty/whitespace explicit handle would otherwise look absent and silently fall back. | ||
| for key in keys where v2HasNonNullParam(params, key) && v2UUID(params, key) == nil { | ||
| return .err(code: "invalid_params", message: "Unresolved \(key)", data: nil) | ||
| } |
There was a problem hiding this comment.
Explicit handle validation is still too weak for the new authoritative-routing contract.
v2RejectUnresolvedHandles only verifies that a supplied value can be turned into a UUID. A valid handle from the wrong workspace/window — or a return_to surface of the wrong type — still passes here and then degrades into the generic .not_found path in the new browser handlers. That breaks the PR’s new “supplied-but-unresolvable handles are hard errors” behavior for explicit surface_id / return_to / workspace_id / window_id.
Also applies to: 13294-13405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 13281 - 13290,
v2RejectUnresolvedHandles currently only checks that a param parses to a UUID
via v2UUID; update it to also verify the UUID actually resolves to the correct
resource and context (surface, return_to type, workspace, window) so
supplied-but-wrong-workspace/type handles are rejected. For keys like
"surface_id", "return_to", "workspace_id", "window_id" call the appropriate
resolver (e.g.
v2ResolveSurface/v2ResolveReturnTo/v2ResolveWorkspace/v2ResolveWindow or the
central handle-resolver used elsewhere) instead of just v2UUID, and if
resolution fails or the resolved object is of the wrong type/owner return
.err(code: "invalid_params", message: "Unresolved <key>", data: nil); keep the
existing presence check using v2HasNonNullParam and reuse v2UUID only to obtain
the candidate id before resolution.
…r and spec PointerEvent 'pointerenter' (like 'mouseenter') must not bubble, but __cmuxPointer hardcoded bubbles:true while __cmuxMouse already honored a bubbles flag and dispatched mouseenter non-bubbling. A bubbling pointerenter fires spurious enter signals on ancestor elements and can corrupt hover state (hover menus, pointer-tracking) in nested components when __cmuxClick/__cmuxHover run. Add a bubbles parameter to __cmuxPointer mirroring __cmuxMouse and pass false at the pointerenter call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c86dba6 to
4ecc5a7
Compare
lawrencecchen
changed the title
…r and spec (manaflow-ai#5958) PointerEvent 'pointerenter' (like 'mouseenter') must not bubble, but __cmuxPointer hardcoded bubbles:true while __cmuxMouse already honored a bubbles flag and dispatched mouseenter non-bubbling. A bubbling pointerenter fires spurious enter signals on ancestor elements and can corrupt hover state (hover menus, pointer-tracking) in nested components when __cmuxClick/__cmuxHover run. Add a bubbles parameter to __cmuxPointer mirroring __cmuxMouse and pass false at the pointerenter call sites. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
The browser view CLI actions this branch was named for (
react-grab,devtools,console,focus-mode,zoom,history) already landed on main via #5766, with follow-up refinements in #5870 and #5778. This PR is now reduced to the one fix from that work that was not yet on main.PointerEventpointerenter(likemouseenter) must not bubble.__cmuxMousealready honored abubblesflag and dispatchedmouseenternon-bubbling, but__cmuxPointerhardcodedbubbles:true, sopointerenterbubbled. That fires spurious enter signals on ancestor elements and can corrupt hover state (hover menus, pointer tracking) in nested components when__cmuxClick/__cmuxHoverrun. Fix adds abubblesparameter to__cmuxPointermirroring__cmuxMouseand passesfalseat the twopointerentercall sites.pointerover/move/down/upkeep the defaultbubbles:true, which is correct per spec.🤖 Generated with Claude Code
Note
Low Risk
Small change to injected JS input simulation in
TerminalController; no auth, data, or routing impact.Overview
Fixes synthetic click and hover gestures in injected
browserInputHelperssopointerentermatches real DOM behavior and existingmouseenterhandling.__cmuxPointernow accepts an optionalbubblesflag (default on).pointerenteris dispatched withbubbles: falsein__cmuxClickand__cmuxHover, aligning the pointer + mouse sequence frameworks expect for enter events.Reviewed by Cursor Bugbot for commit 4ecc5a7. Bugbot is set up for automated code reviews on this repo. Configure here.