diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index d199b65fc9a..4f60e29ca25 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -156,6 +156,8 @@ const { blocks: [] as unknown[], messages: [] as unknown[], latestChatEditorProps: null as ChatEditorTestProps | null, + latestToolApprovalKeyboardActive: null as boolean | null, + latestAskUserQuestionKeyboardActive: null as boolean | null, latestScheduledTasksProps: null as { onRunPrompt?: ( prompt: string, @@ -766,8 +768,30 @@ mockComponent('./components/dialogs/RewindDialog', 'RewindDialog'); mockComponent('./components/messages/AgentsMessage', 'AgentsMessage'); mockComponent('./components/messages/MemoryMessage', 'MemoryMessage'); mockComponent('./components/messages/AuthMessage', 'AuthMessage'); -mockComponent('./components/messages/ToolApproval', 'ToolApproval'); -mockComponent('./components/messages/AskUserQuestion', 'AskUserQuestion'); +// Record keyboardActive so app-level tests can assert the overlay is told to +// grab focus when it becomes topmost (the actual focus lives in the real +// components, covered by their own unit tests). +vi.doMock('./components/messages/ToolApproval', async () => { + const React = await import('react'); + return { + ToolApproval: (props: { keyboardActive?: boolean }) => { + testState.latestToolApprovalKeyboardActive = props.keyboardActive ?? null; + return React.createElement('div', { + 'data-web-shell-permission-panel': '', + }); + }, + }; +}); +vi.doMock('./components/messages/AskUserQuestion', async () => { + const React = await import('react'); + return { + AskUserQuestion: (props: { keyboardActive?: boolean }) => { + testState.latestAskUserQuestionKeyboardActive = + props.keyboardActive ?? null; + return React.createElement('div', { 'data-web-shell-ask-panel': '' }); + }, + }; +}); mockComponent('./components/messages/TasksStatusMessage', 'TasksStatusMessage'); mockComponent('./components/messages/BtwMessage', 'BtwMessage'); mockComponent('./components/QueuedPromptDisplay', 'QueuedPromptDisplay'); @@ -904,6 +928,8 @@ beforeEach(() => { testState.blocks = []; testState.messages = []; testState.latestChatEditorProps = null; + testState.latestToolApprovalKeyboardActive = null; + testState.latestAskUserQuestionKeyboardActive = null; testState.latestScheduledTasksProps = null; testState.latestGoalsProps = null; sidebarTokens.length = 0; @@ -1350,10 +1376,14 @@ describe('App session callbacks', () => { editorFocus.mockClear(); act(() => vi.runOnlyPendingTimers()); + // The editor isn't refocused while an approval is pending; instead the app + // tells the approval overlay to take focus (keyboardActive), so a stray + // keystroke can't send a message past the pending approval. expect(editorFocus).not.toHaveBeenCalled(); - expect(document.activeElement).toBe( + expect( document.querySelector('[data-testid="approval-overlay"]'), - ); + ).not.toBeNull(); + expect(testState.latestToolApprovalKeyboardActive).toBe(true); }); it('does not show missing-session state for non-404/410 errors', async () => { @@ -4178,7 +4208,10 @@ describe('App session callbacks', () => { expect(mockSessionActions.sendPrompt).not.toHaveBeenCalled(); }); - it('moves focus to the approval overlay when it appears', async () => { + it('marks the approval overlay keyboard-active when it appears', async () => { + // Focus itself is owned by ToolApproval/AskUserQuestion (covered by their + // own tests); the app's job is to render the overlay and tell it to grab + // focus (keyboardActive) once it's the topmost surface. const { rerender } = renderApp(); await flush(); @@ -4188,9 +4221,31 @@ describe('App session callbacks', () => { await Promise.resolve(); }); - const overlay = document.querySelector('[data-testid="approval-overlay"]'); - expect(overlay).not.toBeNull(); - expect(document.activeElement).toBe(overlay); + expect( + document.querySelector('[data-testid="approval-overlay"]'), + ).not.toBeNull(); + expect(testState.latestToolApprovalKeyboardActive).toBe(true); + }); + + it('marks the ask-user question overlay keyboard-active when it appears', async () => { + // Symmetric to the ToolApproval case: guards against askUserOverlayVisible + // being mis-derived (e.g. from pendingToolApproval) so the question overlay + // would never pull focus. + const { rerender } = renderApp(); + await flush(); + + await act(async () => { + testState.blocks = [ + makePendingPermissionBlock({ toolName: 'ask_user_question' }), + ]; + rerender(); + await Promise.resolve(); + }); + + expect( + document.querySelector('[data-testid="approval-overlay"]'), + ).not.toBeNull(); + expect(testState.latestAskUserQuestionKeyboardActive).toBe(true); }); it('closes the panel on Escape from outside the sidebar', async () => { diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 236d1a00226..720c3699b00 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -2536,30 +2536,24 @@ export function App({ showApprovalModeDialog, mainView, ]); - // Once the effect above uncovers the approval, the overlay is the topmost - // surface but the just-unmounted panel Back button dropped focus to . - // Move focus onto the overlay when it becomes visible so keyboard/AT users - // land on it. Only for ToolApproval: it drives keyboard entirely through a - // window listener, so focusing its (tabindex=-1) wrapper is safe and gives AT - // a landing spot without confirming (Enter arms first, confirms second — a - // focused button would confirm on the first press). AskUserQuestion instead - // manages its own focus across its options/input, so stealing focus to the - // wrapper would break its arrow-key navigation. - const approvalOverlayRef = useRef(null); + // Whether each approval overlay is the topmost (visible, uncovered) one. The + // overlay components consume this as `keyboardActive`: when it flips true — on + // appearance, or once a panel/dialog that was covering it closes — they pull + // keyboard focus to their own safe-default option. Focus handling now lives in + // ToolApproval/AskUserQuestion (their keyboard handling is focus-scoped), so + // the app no longer focuses the wrapper element directly. const toolApprovalOverlayVisible = pendingToolApproval !== null && !activePanel && modelDialogMode === null && !showApprovalModeDialog && mainView === 'chat'; - const prevToolApprovalOverlayVisibleRef = useRef(toolApprovalOverlayVisible); - useEffect(() => { - const wasVisible = prevToolApprovalOverlayVisibleRef.current; - prevToolApprovalOverlayVisibleRef.current = toolApprovalOverlayVisible; - if (toolApprovalOverlayVisible && !wasVisible) { - approvalOverlayRef.current?.focus(); - } - }, [toolApprovalOverlayVisible]); + const askUserOverlayVisible = + pendingAskUserApproval !== null && + !activePanel && + modelDialogMode === null && + !showApprovalModeDialog && + mainView === 'chat'; const [showMemoryDialog, setShowMemoryDialog] = useState(false); const [showAuthDialog, setShowAuthDialog] = useState(false); const showAuthDialogRef = useRef(showAuthDialog); @@ -7061,13 +7055,11 @@ export function App({ )} {/* Only render the outer session's approval on the chat view. Under a full-page view (split / scheduled tasks) - it would sit hidden yet still own global keyboard - shortcuts — a keypress could confirm an unseen - approval. Each split pane surfaces its own approval. */} + it would sit hidden and unreachable. Each split pane + surfaces its own approval. `keyboardActive` tells the + overlay to grab focus only when it's the topmost one. */} {pendingToolApproval && mainView === 'chat' && (
@@ -7075,13 +7067,12 @@ export function App({ request={pendingToolApproval} onConfirm={handleConfirm} variant="floating" + keyboardActive={toolApprovalOverlayVisible} />
)} {pendingAskUserApproval && mainView === 'chat' && (
@@ -7089,6 +7080,7 @@ export function App({ request={pendingAskUserApproval} onConfirm={handleConfirm} variant="floating" + keyboardActive={askUserOverlayVisible} />
)} diff --git a/packages/web-shell/client/components/ChatPane.test.tsx b/packages/web-shell/client/components/ChatPane.test.tsx index bafcf96c2e2..75903dcb6f3 100644 --- a/packages/web-shell/client/components/ChatPane.test.tsx +++ b/packages/web-shell/client/components/ChatPane.test.tsx @@ -205,6 +205,7 @@ vi.mock('./messages/AskUserQuestion', () => ({ AskUserQuestion: (props: any) => ( ); })} @@ -341,15 +515,14 @@ export function AskUserQuestion({ className={`${styles.option} ${ isCustomActive ? styles.optionActive : '' } ${hasCustomValue ? styles.optionSelected : ''}`} - onClick={() => { - setSelectedIdx(current.options.length); - focusCustomInput(); - }} - onMouseEnter={() => - setSelectedIdx(current.options.length) - } + // The whole row is clickable (it carries cursor:pointer via + // styles.option), so clicks on the padding — not just the + // inner trigger/input — activate the "Other" option. The + // trigger button has no onClick of its own; its click (and + // native Enter/Space activation) bubbles up to here. + onClick={() => chooseOption(current.options.length)} > - +