make new browser tab focus on address bar (#474)

* Fix browser tab focus and grab shortcut behavior

* fix: resolve focus stealing and iframe grabbing edge cases

- Move post-navigation focus handoff to the start of programmatic navigations to prevent stealing focus if the user clicks the address bar while the page is loading.
- Add IFRAME to the isEditable check in the grab shortcut guest forwarder to prevent stealing native copy from cross-origin iframes.
This commit is contained in:
Jinjing 2026-04-11 05:36:09 -07:00 committed by GitHub
parent 7e58514ba5
commit 8ccab252ea
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 375 additions and 11 deletions

View File

@ -14,7 +14,9 @@ const {
guestIsDestroyedMock,
guestGetZoomFactorMock,
guestCapturePageMock,
menuBuildFromTemplateMock
menuBuildFromTemplateMock,
rendererSendMock,
rendererIsDestroyedMock
} = vi.hoisted(() => ({
webContentsFromIdMock: vi.fn(),
guestOnMock: vi.fn(),
@ -25,7 +27,9 @@ const {
guestIsDestroyedMock: vi.fn(() => false),
guestGetZoomFactorMock: vi.fn(() => 1),
guestCapturePageMock: vi.fn(),
menuBuildFromTemplateMock: vi.fn()
menuBuildFromTemplateMock: vi.fn(),
rendererSendMock: vi.fn(),
rendererIsDestroyedMock: vi.fn(() => false)
}))
vi.mock('electron', () => ({
@ -65,7 +69,18 @@ describe('browserManager grab operations', () => {
browserManager.unregisterAll()
guest = makeGuest(101)
webContentsFromIdMock.mockReturnValue(guest)
webContentsFromIdMock.mockImplementation((id: number) => {
if (id === 101) {
return guest
}
if (id === rendererWebContentsId) {
return {
isDestroyed: rendererIsDestroyedMock,
send: rendererSendMock
}
}
return null
})
browserManager.attachGuestPolicies(guest)
browserManager.registerGuest({
@ -127,6 +142,48 @@ describe('browserManager grab operations', () => {
})
})
describe('grab shortcut forwarding', () => {
it('forwards cmd/ctrl+c from the guest when the page is not using copy', async () => {
const handler = guestOnMock.mock.calls.find(
([eventName]) => eventName === 'before-input-event'
)?.[1]
expect(handler).toBeTypeOf('function')
guestExecuteJavaScriptMock.mockResolvedValueOnce(true)
const preventDefault = vi.fn()
handler?.(
{ preventDefault } as never,
{ type: 'keyDown', meta: true, control: true, shift: false, alt: false, key: 'c' } as never
)
await Promise.resolve()
await Promise.resolve()
expect(preventDefault).toHaveBeenCalledTimes(1)
expect(rendererSendMock).toHaveBeenCalledWith('browser:grabModeToggle', 'tab-1')
})
it('does not forward cmd/ctrl+c when the guest reports native copy should win', async () => {
const handler = guestOnMock.mock.calls.find(
([eventName]) => eventName === 'before-input-event'
)?.[1]
expect(handler).toBeTypeOf('function')
guestExecuteJavaScriptMock.mockResolvedValueOnce(false)
const preventDefault = vi.fn()
handler?.(
{ preventDefault } as never,
{ type: 'keyDown', meta: true, control: true, shift: false, alt: false, key: 'c' } as never
)
await Promise.resolve()
await Promise.resolve()
expect(preventDefault).not.toHaveBeenCalled()
expect(rendererSendMock).not.toHaveBeenCalled()
})
})
describe('hasActiveGrabOp', () => {
it('returns false when no grab is active', () => {
expect(browserManager.hasActiveGrabOp('tab-1')).toBe(false)

View File

@ -222,6 +222,7 @@ class BrowserManager {
private readonly webContentsIdByTabId = new Map<string, number>()
private readonly rendererWebContentsIdByTabId = new Map<string, number>()
private readonly contextMenuCleanupByTabId = new Map<string, () => void>()
private readonly grabShortcutCleanupByTabId = new Map<string, () => void>()
private readonly policyAttachedGuestIds = new Set<number>()
private readonly pendingLoadFailuresByGuestId = new Map<
number,
@ -317,6 +318,7 @@ class BrowserManager {
this.rendererWebContentsIdByTabId.set(browserTabId, rendererWebContentsId)
this.setupContextMenu(browserTabId, guest)
this.setupGrabShortcut(browserTabId, guest)
this.flushPendingLoadFailure(browserTabId, webContentsId)
}
@ -331,6 +333,11 @@ class BrowserManager {
cleanup()
this.contextMenuCleanupByTabId.delete(browserTabId)
}
const shortcutCleanup = this.grabShortcutCleanupByTabId.get(browserTabId)
if (shortcutCleanup) {
shortcutCleanup()
this.grabShortcutCleanupByTabId.delete(browserTabId)
}
this.webContentsIdByTabId.delete(browserTabId)
this.rendererWebContentsIdByTabId.delete(browserTabId)
}
@ -794,6 +801,79 @@ class BrowserManager {
})
}
// Why: browser grab mode intentionally uses Cmd/Ctrl+C as its entry
// gesture, but a focused webview guest is a separate Chromium process so
// the renderer's window-level keydown handler never sees that shortcut.
// Only forward the chord when Chromium would not perform a normal copy:
// no editable element is focused and there is no selected text. That keeps
// native page copy working while still making the grab shortcut reachable
// from focused web content.
private setupGrabShortcut(browserTabId: string, guest: Electron.WebContents): void {
const previousCleanup = this.grabShortcutCleanupByTabId.get(browserTabId)
if (previousCleanup) {
previousCleanup()
this.grabShortcutCleanupByTabId.delete(browserTabId)
}
const handler = (event: Electron.Event, input: Electron.Input): void => {
if (input.type !== 'keyDown') {
return
}
const isMod = process.platform === 'darwin' ? input.meta : input.control
if (!isMod || input.shift || input.alt || input.key.toLowerCase() !== 'c') {
return
}
void guest
.executeJavaScript(`(() => {
const active = document.activeElement
const tag = active?.tagName
const isEditable =
active instanceof HTMLInputElement ||
active instanceof HTMLTextAreaElement ||
active?.isContentEditable === true ||
tag === 'SELECT' ||
tag === 'IFRAME'
if (isEditable) {
return false
}
const selection = window.getSelection()
return Boolean(selection && selection.type === 'Range' && selection.toString().trim().length > 0)
? false
: true
})()`)
.then((shouldToggle) => {
if (!shouldToggle) {
return
}
event.preventDefault()
const rendererWcId = this.rendererWebContentsIdByTabId.get(browserTabId)
if (!rendererWcId) {
return
}
const rendererWc = webContents.fromId(rendererWcId)
if (!rendererWc || rendererWc.isDestroyed()) {
return
}
rendererWc.send('browser:grabModeToggle', browserTabId)
})
.catch(() => {
// Why: shortcut forwarding is best-effort. Guest teardown or a
// transient executeJavaScript failure should not break normal copy.
})
}
guest.on('before-input-event', handler)
this.grabShortcutCleanupByTabId.set(browserTabId, () => {
try {
guest.off('before-input-event', handler)
} catch {
// Why: browser tabs can outlive the guest webContents briefly during
// teardown. Cleanup should be best-effort.
}
})
}
private forwardOrQueueGuestLoadFailure(
guestWebContentsId: number,
loadError: { code: number; description: string; validatedUrl: string }

View File

@ -76,6 +76,7 @@ export type BrowserApi = {
args: BrowserCaptureSelectionScreenshotArgs
) => Promise<BrowserCaptureSelectionScreenshotResult>
extractHoverPayload: (args: BrowserExtractHoverArgs) => Promise<BrowserExtractHoverResult>
onGrabModeToggle: (callback: (browserTabId: string) => void) => () => void
}
export type PreflightStatus = {

View File

@ -355,7 +355,14 @@ const api = {
extractHoverPayload: (args: {
browserTabId: string
}): Promise<{ ok: true; payload: unknown } | { ok: false; reason: string }> =>
ipcRenderer.invoke('browser:extractHoverPayload', args)
ipcRenderer.invoke('browser:extractHoverPayload', args),
onGrabModeToggle: (callback: (browserTabId: string) => void): (() => void) => {
const listener = (_event: Electron.IpcRendererEvent, browserTabId: string) =>
callback(browserTabId)
ipcRenderer.on('browser:grabModeToggle', listener)
return () => ipcRenderer.removeListener('browser:grabModeToggle', listener)
}
},
hooks: {

View File

@ -1,5 +1,6 @@
/* eslint-disable max-lines */
import { useCallback, useEffect, useRef, useState } from 'react'
import { cn } from '@/lib/utils'
import {
ArrowLeft,
ArrowRight,
@ -24,6 +25,7 @@ import {
DropdownMenuShortcut,
DropdownMenuTrigger
} from '@/components/ui/dropdown-menu'
import { useAppStore } from '@/store'
import { ORCA_BROWSER_BLANK_URL, ORCA_BROWSER_PARTITION } from '../../../../shared/constants'
import type { BrowserLoadError, BrowserTab as BrowserTabState } from '../../../../shared/types'
import {
@ -259,6 +261,7 @@ export default function BrowserPane({
onSetUrl: (tabId: string, url: string) => void
}): React.JSX.Element {
const containerRef = useRef<HTMLDivElement | null>(null)
const addressBarInputRef = useRef<HTMLInputElement | null>(null)
const webviewRef = useRef<Electron.WebviewTag | null>(null)
const browserTabIdRef = useRef(browserTab.id)
browserTabIdRef.current = browserTab.id
@ -273,6 +276,8 @@ export default function BrowserPane({
const addressBarValueRef = useRef(browserTab.url)
const [resourceNotice, setResourceNotice] = useState<string | null>(null)
const grab = useGrabMode(browserTab.id)
const consumeAddressBarFocusRequest = useAppStore((s) => s.consumeAddressBarFocusRequest)
const keepAddressBarFocusRef = useRef(false)
// Inline toast that appears near the grabbed element instead of the global
// bottom-right toaster, so feedback feels spatially connected to the action.
@ -374,6 +379,60 @@ export default function BrowserPane({
)
}, [browserTab.id])
const focusAddressBarNow = useCallback(() => {
const input = addressBarInputRef.current
if (!input) {
return false
}
webviewRef.current?.blur()
input.focus()
input.select()
return document.activeElement === input
}, [])
const focusWebviewNow = useCallback(() => {
const webview = webviewRef.current
if (!webview) {
return false
}
addressBarInputRef.current?.blur()
webview.focus()
return document.activeElement === webview
}, [])
useEffect(() => {
if (!consumeAddressBarFocusRequest(browserTab.id)) {
return
}
keepAddressBarFocusRef.current = true
// Why: terminal activation restores xterm focus on a later animation frame
// when the surface changes. A single address-bar focus attempt can lose
// that race, leaving the new browser tab on <body>. Retry briefly across a
// few frames so a freshly opened blank tab still lands in the location bar,
// but keep the request one-shot so revisiting the tab later does not steal
// focus back from the user.
let cancelled = false
let frameId = 0
let attempts = 0
const focusAddressBar = (): void => {
if (cancelled) {
return
}
focusAddressBarNow()
attempts += 1
if (attempts < 6) {
frameId = window.requestAnimationFrame(focusAddressBar)
} else {
keepAddressBarFocusRef.current = false
}
}
frameId = window.requestAnimationFrame(focusAddressBar)
return () => {
cancelled = true
window.cancelAnimationFrame(frameId)
}
}, [browserTab.id, consumeAddressBarFocusRequest, focusAddressBarNow])
useEffect(() => {
onUpdatePageStateRef.current = onUpdatePageState
onSetUrlRef.current = onSetUrl
@ -440,6 +499,9 @@ export default function BrowserPane({
})
}
syncNavigationState(webview)
if (keepAddressBarFocusRef.current) {
focusAddressBarNow()
}
}
const handleDidStartLoading = (): void => {
@ -497,6 +559,11 @@ export default function BrowserPane({
rememberLiveBrowserUrl(browserTab.id, currentUrl)
setAddressBarValue(toDisplayUrl(currentUrl))
onSetUrlRef.current(browserTab.id, currentUrl)
if (keepAddressBarFocusRef.current && currentUrl === ORCA_BROWSER_BLANK_URL) {
focusAddressBarNow()
} else {
keepAddressBarFocusRef.current = false
}
onUpdatePageStateRef.current(browserTab.id, {
loading: false,
title: webview.getTitle() || currentUrl,
@ -610,7 +677,7 @@ export default function BrowserPane({
evictParkedWebviews(browserTab.id)
}
}
}, [browserTab.id, syncNavigationState])
}, [browserTab.id, focusAddressBarNow, focusWebviewNow, syncNavigationState])
useEffect(() => {
const webview = webviewRef.current
@ -627,8 +694,14 @@ export default function BrowserPane({
// event so only real navigations, not tab activation churn, show loading UI.
trackNextLoadingEventRef.current = normalizedUrl !== ORCA_BROWSER_BLANK_URL
webview.src = normalizedUrl
if (normalizedUrl !== ORCA_BROWSER_BLANK_URL) {
keepAddressBarFocusRef.current = false
if (document.activeElement === addressBarInputRef.current) {
focusWebviewNow()
}
}
}
}, [browserTab.url])
}, [browserTab.url, focusWebviewNow])
useEffect(() => {
if (!browserTab.loading) {
@ -696,6 +769,18 @@ export default function BrowserPane({
return () => window.removeEventListener('keydown', handleKeyDown)
}, [grab])
// Why: a focused webview guest receives Cmd/Ctrl+C inside Chromium, not the
// host renderer window. Main forwards the chord back only when the page
// would not use it for native copy, so grab mode still toggles from web
// content without stealing real copy from inputs or selections.
useEffect(() => {
return window.api.browser.onGrabModeToggle((tabId) => {
if (tabId === browserTab.id) {
grabRef.current.toggle()
}
})
}, [browserTab.id])
// Why: single-key shortcuts (C / S) let the user copy the hovered element
// without clicking. During 'armed'/'awaiting' state, the shortcut calls the
// extractHoverPayload IPC to read the currently hovered element directly.
@ -821,6 +906,7 @@ export default function BrowserPane({
}, [grab, showGrabToast])
const submitAddressBar = (): void => {
keepAddressBarFocusRef.current = false
const nextUrl = normalizeBrowserNavigationUrl(addressBarValue)
if (!nextUrl) {
onUpdatePageStateRef.current(browserTab.id, {
@ -844,6 +930,9 @@ export default function BrowserPane({
}
trackNextLoadingEventRef.current = nextUrl !== ORCA_BROWSER_BLANK_URL
webview.src = nextUrl
if (nextUrl !== ORCA_BROWSER_BLANK_URL) {
focusWebviewNow()
}
}
// Why: the store initially holds 'about:blank', but once the webview loads
@ -921,6 +1010,7 @@ export default function BrowserPane({
>
<Globe className="size-4 shrink-0 text-muted-foreground" />
<Input
ref={addressBarInputRef}
value={addressBarValue}
onChange={(event) => setAddressBarValue(event.target.value)}
className="h-auto border-0 bg-transparent px-0 text-sm shadow-none focus-visible:ring-0"
@ -973,8 +1063,26 @@ export default function BrowserPane({
</div>
) : null}
{grab.state !== 'idle' ? (
<div className="flex items-center gap-2 border-b border-border/60 bg-muted/40 px-3 py-1.5 text-xs text-muted-foreground">
<Crosshair className="size-3" />
<div
className={cn(
'flex items-center gap-2 border-b border-border/60 px-3 py-1.5 text-xs text-foreground/90',
grab.state === 'error'
? 'bg-destructive/10'
: grab.state === 'confirming'
? 'bg-green-500/10'
: 'bg-blue-500/10'
)}
>
<Crosshair
className={cn(
'size-3 shrink-0',
grab.state === 'error'
? 'text-destructive'
: grab.state === 'confirming'
? 'text-green-500'
: 'text-blue-500'
)}
/>
<span>
{grab.state === 'error'
? `Grab failed: ${grab.error ?? 'Unknown error'}`
@ -983,7 +1091,7 @@ export default function BrowserPane({
: 'Click to copy, or hover and press C. S for screenshot.'}
</span>
<button
className="ml-auto text-muted-foreground hover:text-foreground"
className="ml-auto shrink-0 rounded px-2 py-0.5 text-muted-foreground transition-colors hover:text-foreground"
onClick={grab.cancel}
>
Cancel

View File

@ -22,6 +22,7 @@ export type BrowserSlice = {
browserTabsByWorktree: Record<string, BrowserTab[]>
activeBrowserTabId: string | null
activeBrowserTabIdByWorktree: Record<string, string | null>
pendingAddressBarFocusByTabId: Record<string, true>
createBrowserTab: (
worktreeId: string,
url: string,
@ -29,6 +30,7 @@ export type BrowserSlice = {
) => BrowserTab
closeBrowserTab: (tabId: string) => void
setActiveBrowserTab: (tabId: string) => void
consumeAddressBarFocusRequest: (tabId: string) => boolean
updateBrowserTabPageState: (tabId: string, updates: BrowserTabPageState) => void
setBrowserTabUrl: (tabId: string, url: string) => void
hydrateBrowserSession: (session: WorkspaceSessionState) => void
@ -60,10 +62,11 @@ function getFallbackTabTypeForWorktree(
return 'terminal'
}
export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> = (set) => ({
export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> = (set, get) => ({
browserTabsByWorktree: {},
activeBrowserTabId: null,
activeBrowserTabIdByWorktree: {},
pendingAddressBarFocusByTabId: {},
createBrowserTab: (worktreeId, url, options) => {
const id = globalThis.crypto.randomUUID()
@ -111,6 +114,9 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
const shouldActivate = options?.activate ?? true
const shouldUpdateGlobalActiveSurface = shouldActivate && s.activeWorktreeId === worktreeId
const shouldFocusAddressBar =
shouldUpdateGlobalActiveSurface &&
(normalizedUrl === 'about:blank' || normalizedUrl === ORCA_BROWSER_BLANK_URL)
return {
browserTabsByWorktree: {
...s.browserTabsByWorktree,
@ -133,7 +139,17 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
activeTabType: shouldUpdateGlobalActiveSurface ? 'browser' : s.activeTabType,
activeTabTypeByWorktree: shouldActivate
? { ...s.activeTabTypeByWorktree, [worktreeId]: 'browser' }
: s.activeTabTypeByWorktree
: s.activeTabTypeByWorktree,
// Why: the active BrowserPane remounts on every browser-tab switch, so
// a plain autoFocus would keep stealing focus whenever the user
// revisits an existing tab. Queue a one-shot focus request only for a
// freshly created blank tab, then let BrowserPane consume it once.
pendingAddressBarFocusByTabId: shouldFocusAddressBar
? {
...s.pendingAddressBarFocusByTabId,
[id]: true
}
: s.pendingAddressBarFocusByTabId
}
})
return browserTab
@ -199,6 +215,11 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
activeBrowserTabIdByWorktree: nextActiveBrowserTabIdByWorktree,
tabBarOrderByWorktree: nextTabBarOrder,
activeTabType: nextActiveTabType,
pendingAddressBarFocusByTabId: Object.fromEntries(
Object.entries(s.pendingAddressBarFocusByTabId).filter(
([pendingTabId]) => pendingTabId !== tabId
)
),
activeTabTypeByWorktree: nextActiveTabTypeByWorktree
}
}),
@ -225,6 +246,20 @@ export const createBrowserSlice: StateCreator<AppState, [], [], BrowserSlice> =
}
}),
consumeAddressBarFocusRequest: (tabId) => {
if (!get().pendingAddressBarFocusByTabId[tabId]) {
return false
}
set((s) => {
const next = { ...s.pendingAddressBarFocusByTabId }
delete next[tabId]
return { pendingAddressBarFocusByTabId: next }
})
return true
},
updateBrowserTabPageState: (tabId, updates) =>
set((s) => ({
browserTabsByWorktree: Object.fromEntries(

View File

@ -473,6 +473,82 @@ describe('setActiveWorktree', () => {
expect(s.activeBrowserTabIdByWorktree[backgroundWt]).toBe(browserTab.id)
})
it('queues and consumes a one-shot address-bar focus request for a fresh blank browser tab', () => {
const store = createTestStore()
const wt = 'repo1::/path/wt1'
seedStore(store, {
worktreesByRepo: {
repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt1' })]
},
activeWorktreeId: wt,
activeTabType: 'terminal',
tabsByWorktree: {
[wt]: [makeTab({ id: 'terminal-1', worktreeId: wt })]
}
})
const browserTab = store.getState().createBrowserTab(wt, 'about:blank', { activate: true })
expect(store.getState().pendingAddressBarFocusByTabId[browserTab.id]).toBe(true)
expect(store.getState().consumeAddressBarFocusRequest(browserTab.id)).toBe(true)
expect(store.getState().consumeAddressBarFocusRequest(browserTab.id)).toBe(false)
})
it('does not queue address-bar focus for background or already-navigated browser tabs', () => {
const store = createTestStore()
const activeWt = 'repo1::/path/wt1'
const backgroundWt = 'repo1::/path/wt2'
seedStore(store, {
worktreesByRepo: {
repo1: [
makeWorktree({ id: activeWt, repoId: 'repo1', path: '/path/wt1' }),
makeWorktree({ id: backgroundWt, repoId: 'repo1', path: '/path/wt2' })
]
},
activeWorktreeId: activeWt,
activeTabType: 'terminal',
tabsByWorktree: {
[activeWt]: [makeTab({ id: 'terminal-1', worktreeId: activeWt })],
[backgroundWt]: [makeTab({ id: 'terminal-2', worktreeId: backgroundWt })]
}
})
const backgroundBlankTab = store
.getState()
.createBrowserTab(backgroundWt, 'about:blank', { activate: true })
const activeNavigatedTab = store
.getState()
.createBrowserTab(activeWt, 'https://example.com', { activate: true })
expect(store.getState().pendingAddressBarFocusByTabId[backgroundBlankTab.id]).toBeUndefined()
expect(store.getState().pendingAddressBarFocusByTabId[activeNavigatedTab.id]).toBeUndefined()
})
it('drops a pending address-bar focus request when the new browser tab closes before mount', () => {
const store = createTestStore()
const wt = 'repo1::/path/wt1'
seedStore(store, {
worktreesByRepo: {
repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt1' })]
},
activeWorktreeId: wt,
activeTabType: 'terminal',
tabsByWorktree: {
[wt]: [makeTab({ id: 'terminal-1', worktreeId: wt })]
}
})
const browserTab = store.getState().createBrowserTab(wt, 'about:blank', { activate: true })
expect(store.getState().pendingAddressBarFocusByTabId[browserTab.id]).toBe(true)
store.getState().closeBrowserTab(browserTab.id)
expect(store.getState().pendingAddressBarFocusByTabId[browserTab.id]).toBeUndefined()
})
it('restores terminal surface when switching to a worktree that was last on a terminal tab with open files', () => {
const store = createTestStore()
const wt = 'repo1::/path/wt1'