mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-22 11:04:42 -07:00
fix(browser): keep input blocker active across navigations (#22562)
Co-authored-by: cynthialong0-0 <82900738+cynthialong0-0@users.noreply.github.com>
This commit is contained in:
@@ -9,6 +9,7 @@ import { BrowserManager } from './browserManager.js';
|
|||||||
import { makeFakeConfig } from '../../test-utils/config.js';
|
import { makeFakeConfig } from '../../test-utils/config.js';
|
||||||
import type { Config } from '../../config/config.js';
|
import type { Config } from '../../config/config.js';
|
||||||
import { injectAutomationOverlay } from './automationOverlay.js';
|
import { injectAutomationOverlay } from './automationOverlay.js';
|
||||||
|
import { injectInputBlocker } from './inputBlocker.js';
|
||||||
import { coreEvents } from '../../utils/events.js';
|
import { coreEvents } from '../../utils/events.js';
|
||||||
|
|
||||||
// Mock the MCP SDK
|
// Mock the MCP SDK
|
||||||
@@ -54,6 +55,13 @@ vi.mock('./automationOverlay.js', () => ({
|
|||||||
injectAutomationOverlay: vi.fn().mockResolvedValue(undefined),
|
injectAutomationOverlay: vi.fn().mockResolvedValue(undefined),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
vi.mock('./inputBlocker.js', () => ({
|
||||||
|
injectInputBlocker: vi.fn().mockResolvedValue(undefined),
|
||||||
|
removeInputBlocker: vi.fn().mockResolvedValue(undefined),
|
||||||
|
suspendInputBlocker: vi.fn().mockResolvedValue(undefined),
|
||||||
|
resumeInputBlocker: vi.fn().mockResolvedValue(undefined),
|
||||||
|
}));
|
||||||
|
|
||||||
vi.mock('node:fs', async (importOriginal) => {
|
vi.mock('node:fs', async (importOriginal) => {
|
||||||
const actual = await importOriginal<typeof import('node:fs')>();
|
const actual = await importOriginal<typeof import('node:fs')>();
|
||||||
return {
|
return {
|
||||||
@@ -78,6 +86,7 @@ describe('BrowserManager', () => {
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
vi.resetAllMocks();
|
vi.resetAllMocks();
|
||||||
vi.mocked(injectAutomationOverlay).mockClear();
|
vi.mocked(injectAutomationOverlay).mockClear();
|
||||||
|
vi.mocked(injectInputBlocker).mockClear();
|
||||||
vi.spyOn(coreEvents, 'emitFeedback').mockImplementation(() => {});
|
vi.spyOn(coreEvents, 'emitFeedback').mockImplementation(() => {});
|
||||||
|
|
||||||
// Re-establish consent mock after resetAllMocks
|
// Re-establish consent mock after resetAllMocks
|
||||||
@@ -692,21 +701,66 @@ describe('BrowserManager', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe('overlay re-injection in callTool', () => {
|
describe('overlay re-injection in callTool', () => {
|
||||||
it('should re-inject overlay after click in non-headless mode', async () => {
|
it('should re-inject overlay and input blocker after click in non-headless mode when input disabling is enabled', async () => {
|
||||||
|
// Enable input disabling in config
|
||||||
|
mockConfig = makeFakeConfig({
|
||||||
|
agents: {
|
||||||
|
overrides: {
|
||||||
|
browser_agent: {
|
||||||
|
enabled: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
browser: {
|
||||||
|
headless: false,
|
||||||
|
disableUserInput: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
const manager = new BrowserManager(mockConfig);
|
const manager = new BrowserManager(mockConfig);
|
||||||
await manager.callTool('click', { uid: '1_2' });
|
await manager.callTool('click', { uid: '1_2' });
|
||||||
|
|
||||||
expect(injectAutomationOverlay).toHaveBeenCalledWith(manager, undefined);
|
expect(injectAutomationOverlay).toHaveBeenCalledWith(manager, undefined);
|
||||||
|
expect(injectInputBlocker).toHaveBeenCalledWith(manager, undefined);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should re-inject overlay after navigate_page in non-headless mode', async () => {
|
it('should re-inject overlay and input blocker after navigate_page in non-headless mode when input disabling is enabled', async () => {
|
||||||
|
mockConfig = makeFakeConfig({
|
||||||
|
agents: {
|
||||||
|
overrides: {
|
||||||
|
browser_agent: {
|
||||||
|
enabled: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
browser: {
|
||||||
|
headless: false,
|
||||||
|
disableUserInput: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
const manager = new BrowserManager(mockConfig);
|
const manager = new BrowserManager(mockConfig);
|
||||||
await manager.callTool('navigate_page', { url: 'https://example.com' });
|
await manager.callTool('navigate_page', { url: 'https://example.com' });
|
||||||
|
|
||||||
expect(injectAutomationOverlay).toHaveBeenCalledWith(manager, undefined);
|
expect(injectAutomationOverlay).toHaveBeenCalledWith(manager, undefined);
|
||||||
|
expect(injectInputBlocker).toHaveBeenCalledWith(manager, undefined);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should re-inject overlay after click_at, new_page, press_key, handle_dialog', async () => {
|
it('should re-inject overlay and input blocker after click_at, new_page, press_key, handle_dialog when input disabling is enabled', async () => {
|
||||||
|
mockConfig = makeFakeConfig({
|
||||||
|
agents: {
|
||||||
|
overrides: {
|
||||||
|
browser_agent: {
|
||||||
|
enabled: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
browser: {
|
||||||
|
headless: false,
|
||||||
|
disableUserInput: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
const manager = new BrowserManager(mockConfig);
|
const manager = new BrowserManager(mockConfig);
|
||||||
for (const tool of [
|
for (const tool of [
|
||||||
'click_at',
|
'click_at',
|
||||||
@@ -715,12 +769,15 @@ describe('BrowserManager', () => {
|
|||||||
'handle_dialog',
|
'handle_dialog',
|
||||||
]) {
|
]) {
|
||||||
vi.mocked(injectAutomationOverlay).mockClear();
|
vi.mocked(injectAutomationOverlay).mockClear();
|
||||||
|
vi.mocked(injectInputBlocker).mockClear();
|
||||||
await manager.callTool(tool, {});
|
await manager.callTool(tool, {});
|
||||||
expect(injectAutomationOverlay).toHaveBeenCalledTimes(1);
|
expect(injectAutomationOverlay).toHaveBeenCalledTimes(1);
|
||||||
|
expect(injectInputBlocker).toHaveBeenCalledTimes(1);
|
||||||
|
expect(injectInputBlocker).toHaveBeenCalledWith(manager, undefined);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should NOT re-inject overlay after read-only tools', async () => {
|
it('should NOT re-inject overlay or input blocker after read-only tools', async () => {
|
||||||
const manager = new BrowserManager(mockConfig);
|
const manager = new BrowserManager(mockConfig);
|
||||||
for (const tool of [
|
for (const tool of [
|
||||||
'take_snapshot',
|
'take_snapshot',
|
||||||
@@ -729,8 +786,10 @@ describe('BrowserManager', () => {
|
|||||||
'fill',
|
'fill',
|
||||||
]) {
|
]) {
|
||||||
vi.mocked(injectAutomationOverlay).mockClear();
|
vi.mocked(injectAutomationOverlay).mockClear();
|
||||||
|
vi.mocked(injectInputBlocker).mockClear();
|
||||||
await manager.callTool(tool, {});
|
await manager.callTool(tool, {});
|
||||||
expect(injectAutomationOverlay).not.toHaveBeenCalled();
|
expect(injectAutomationOverlay).not.toHaveBeenCalled();
|
||||||
|
expect(injectInputBlocker).not.toHaveBeenCalled();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -215,6 +215,10 @@ export class BrowserManager {
|
|||||||
// Re-inject the automation overlay and input blocker after tools that
|
// Re-inject the automation overlay and input blocker after tools that
|
||||||
// can cause a full-page navigation. chrome-devtools-mcp emits no MCP
|
// can cause a full-page navigation. chrome-devtools-mcp emits no MCP
|
||||||
// notifications, so callTool() is the only interception point.
|
// notifications, so callTool() is the only interception point.
|
||||||
|
//
|
||||||
|
// The input blocker injection is idempotent: the injected function
|
||||||
|
// reuses the existing DOM element when present and only recreates
|
||||||
|
// it when navigation has actually replaced the page DOM.
|
||||||
if (
|
if (
|
||||||
!result.isError &&
|
!result.isError &&
|
||||||
POTENTIALLY_NAVIGATING_TOOLS.has(toolName) &&
|
POTENTIALLY_NAVIGATING_TOOLS.has(toolName) &&
|
||||||
@@ -224,17 +228,8 @@ export class BrowserManager {
|
|||||||
if (this.shouldInjectOverlay) {
|
if (this.shouldInjectOverlay) {
|
||||||
await injectAutomationOverlay(this, signal);
|
await injectAutomationOverlay(this, signal);
|
||||||
}
|
}
|
||||||
// Only re-inject the input blocker for tools that *reliably*
|
if (this.shouldDisableInput) {
|
||||||
// replace the page DOM (navigate_page, new_page, select_page).
|
await injectInputBlocker(this, signal);
|
||||||
// click/click_at are handled by pointer-events suspend/resume
|
|
||||||
// in mcpToolWrapper — no full re-inject roundtrip needed.
|
|
||||||
// press_key/handle_dialog only sometimes navigate.
|
|
||||||
const reliableNavigation =
|
|
||||||
toolName === 'navigate_page' ||
|
|
||||||
toolName === 'new_page' ||
|
|
||||||
toolName === 'select_page';
|
|
||||||
if (this.shouldDisableInput && reliableNavigation) {
|
|
||||||
await injectInputBlocker(this);
|
|
||||||
}
|
}
|
||||||
} catch {
|
} catch {
|
||||||
// Never let overlay/blocker failures interrupt the tool result
|
// Never let overlay/blocker failures interrupt the tool result
|
||||||
|
|||||||
@@ -5,7 +5,12 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||||
import { injectInputBlocker, removeInputBlocker } from './inputBlocker.js';
|
import {
|
||||||
|
injectInputBlocker,
|
||||||
|
removeInputBlocker,
|
||||||
|
suspendInputBlocker,
|
||||||
|
resumeInputBlocker,
|
||||||
|
} from './inputBlocker.js';
|
||||||
import type { BrowserManager } from './browserManager.js';
|
import type { BrowserManager } from './browserManager.js';
|
||||||
|
|
||||||
describe('inputBlocker', () => {
|
describe('inputBlocker', () => {
|
||||||
@@ -28,6 +33,7 @@ describe('inputBlocker', () => {
|
|||||||
{
|
{
|
||||||
function: expect.stringContaining('__gemini_input_blocker'),
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
},
|
},
|
||||||
|
undefined,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -77,6 +83,29 @@ describe('inputBlocker', () => {
|
|||||||
injectInputBlocker(mockBrowserManager),
|
injectInputBlocker(mockBrowserManager),
|
||||||
).resolves.toBeUndefined();
|
).resolves.toBeUndefined();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should be safe to call multiple times (idempotent injection)', async () => {
|
||||||
|
await injectInputBlocker(mockBrowserManager);
|
||||||
|
await injectInputBlocker(mockBrowserManager);
|
||||||
|
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenCalledTimes(2);
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenNthCalledWith(
|
||||||
|
1,
|
||||||
|
'evaluate_script',
|
||||||
|
expect.objectContaining({
|
||||||
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
|
}),
|
||||||
|
undefined,
|
||||||
|
);
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenNthCalledWith(
|
||||||
|
2,
|
||||||
|
'evaluate_script',
|
||||||
|
expect.objectContaining({
|
||||||
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
|
}),
|
||||||
|
undefined,
|
||||||
|
);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('removeInputBlocker', () => {
|
describe('removeInputBlocker', () => {
|
||||||
@@ -88,6 +117,7 @@ describe('inputBlocker', () => {
|
|||||||
{
|
{
|
||||||
function: expect.stringContaining('__gemini_input_blocker'),
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
},
|
},
|
||||||
|
undefined,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -110,4 +140,38 @@ describe('inputBlocker', () => {
|
|||||||
).resolves.toBeUndefined();
|
).resolves.toBeUndefined();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('suspendInputBlocker and resumeInputBlocker', () => {
|
||||||
|
it('should not throw when blocker element is missing', async () => {
|
||||||
|
// Simulate evaluate_script resolving successfully even if the DOM element is absent.
|
||||||
|
mockBrowserManager.callTool = vi.fn().mockResolvedValue({
|
||||||
|
content: [{ type: 'text', text: 'Script ran on page and returned:' }],
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
suspendInputBlocker(mockBrowserManager),
|
||||||
|
).resolves.toBeUndefined();
|
||||||
|
await expect(
|
||||||
|
resumeInputBlocker(mockBrowserManager),
|
||||||
|
).resolves.toBeUndefined();
|
||||||
|
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenCalledTimes(2);
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenNthCalledWith(
|
||||||
|
1,
|
||||||
|
'evaluate_script',
|
||||||
|
expect.objectContaining({
|
||||||
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
|
}),
|
||||||
|
undefined,
|
||||||
|
);
|
||||||
|
expect(mockBrowserManager.callTool).toHaveBeenNthCalledWith(
|
||||||
|
2,
|
||||||
|
'evaluate_script',
|
||||||
|
expect.objectContaining({
|
||||||
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
|
}),
|
||||||
|
undefined,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -198,11 +198,14 @@ const RESUME_BLOCKER_FUNCTION = `() => {
|
|||||||
*/
|
*/
|
||||||
export async function injectInputBlocker(
|
export async function injectInputBlocker(
|
||||||
browserManager: BrowserManager,
|
browserManager: BrowserManager,
|
||||||
|
signal?: AbortSignal,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
try {
|
try {
|
||||||
await browserManager.callTool('evaluate_script', {
|
await browserManager.callTool(
|
||||||
function: INPUT_BLOCKER_FUNCTION,
|
'evaluate_script',
|
||||||
});
|
{ function: INPUT_BLOCKER_FUNCTION },
|
||||||
|
signal,
|
||||||
|
);
|
||||||
debugLogger.log('Input blocker injected successfully');
|
debugLogger.log('Input blocker injected successfully');
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
// Log but don't throw - input blocker is a UX enhancement, not critical functionality
|
// Log but don't throw - input blocker is a UX enhancement, not critical functionality
|
||||||
@@ -222,11 +225,14 @@ export async function injectInputBlocker(
|
|||||||
*/
|
*/
|
||||||
export async function removeInputBlocker(
|
export async function removeInputBlocker(
|
||||||
browserManager: BrowserManager,
|
browserManager: BrowserManager,
|
||||||
|
signal?: AbortSignal,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
try {
|
try {
|
||||||
await browserManager.callTool('evaluate_script', {
|
await browserManager.callTool(
|
||||||
function: REMOVE_BLOCKER_FUNCTION,
|
'evaluate_script',
|
||||||
});
|
{ function: REMOVE_BLOCKER_FUNCTION },
|
||||||
|
signal,
|
||||||
|
);
|
||||||
debugLogger.log('Input blocker removed successfully');
|
debugLogger.log('Input blocker removed successfully');
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
// Log but don't throw - removal failure is not critical
|
// Log but don't throw - removal failure is not critical
|
||||||
@@ -244,11 +250,14 @@ export async function removeInputBlocker(
|
|||||||
*/
|
*/
|
||||||
export async function suspendInputBlocker(
|
export async function suspendInputBlocker(
|
||||||
browserManager: BrowserManager,
|
browserManager: BrowserManager,
|
||||||
|
signal?: AbortSignal,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
try {
|
try {
|
||||||
await browserManager.callTool('evaluate_script', {
|
await browserManager.callTool(
|
||||||
function: SUSPEND_BLOCKER_FUNCTION,
|
'evaluate_script',
|
||||||
});
|
{ function: SUSPEND_BLOCKER_FUNCTION },
|
||||||
|
signal,
|
||||||
|
);
|
||||||
} catch {
|
} catch {
|
||||||
// Non-critical — tool call will still attempt to proceed
|
// Non-critical — tool call will still attempt to proceed
|
||||||
}
|
}
|
||||||
@@ -260,11 +269,14 @@ export async function suspendInputBlocker(
|
|||||||
*/
|
*/
|
||||||
export async function resumeInputBlocker(
|
export async function resumeInputBlocker(
|
||||||
browserManager: BrowserManager,
|
browserManager: BrowserManager,
|
||||||
|
signal?: AbortSignal,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
try {
|
try {
|
||||||
await browserManager.callTool('evaluate_script', {
|
await browserManager.callTool(
|
||||||
function: RESUME_BLOCKER_FUNCTION,
|
'evaluate_script',
|
||||||
});
|
{ function: RESUME_BLOCKER_FUNCTION },
|
||||||
|
signal,
|
||||||
|
);
|
||||||
} catch {
|
} catch {
|
||||||
// Non-critical
|
// Non-critical
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -224,6 +224,7 @@ describe('mcpToolWrapper', () => {
|
|||||||
expect.objectContaining({
|
expect.objectContaining({
|
||||||
function: expect.stringContaining('__gemini_input_blocker'),
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
}),
|
}),
|
||||||
|
expect.any(AbortSignal),
|
||||||
);
|
);
|
||||||
|
|
||||||
// Second call: click
|
// Second call: click
|
||||||
@@ -241,6 +242,7 @@ describe('mcpToolWrapper', () => {
|
|||||||
expect.objectContaining({
|
expect.objectContaining({
|
||||||
function: expect.stringContaining('__gemini_input_blocker'),
|
function: expect.stringContaining('__gemini_input_blocker'),
|
||||||
}),
|
}),
|
||||||
|
expect.any(AbortSignal),
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -129,7 +129,7 @@ class McpToolInvocation extends BaseToolInvocation<
|
|||||||
// chrome-devtools-mcp's interactability checks pass.
|
// chrome-devtools-mcp's interactability checks pass.
|
||||||
// Only toggles pointer-events CSS — no DOM change, no flicker.
|
// Only toggles pointer-events CSS — no DOM change, no flicker.
|
||||||
if (this.needsBlockerSuspend) {
|
if (this.needsBlockerSuspend) {
|
||||||
await suspendInputBlocker(this.browserManager);
|
await suspendInputBlocker(this.browserManager, signal);
|
||||||
}
|
}
|
||||||
|
|
||||||
const result: McpToolCallResult = await this.browserManager.callTool(
|
const result: McpToolCallResult = await this.browserManager.callTool(
|
||||||
@@ -155,7 +155,7 @@ class McpToolInvocation extends BaseToolInvocation<
|
|||||||
|
|
||||||
// Resume input blocker after interactive tool completes.
|
// Resume input blocker after interactive tool completes.
|
||||||
if (this.needsBlockerSuspend) {
|
if (this.needsBlockerSuspend) {
|
||||||
await resumeInputBlocker(this.browserManager);
|
await resumeInputBlocker(this.browserManager, signal);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (result.isError) {
|
if (result.isError) {
|
||||||
@@ -181,7 +181,7 @@ class McpToolInvocation extends BaseToolInvocation<
|
|||||||
|
|
||||||
// Resume on error path too so the blocker is always restored
|
// Resume on error path too so the blocker is always restored
|
||||||
if (this.needsBlockerSuspend) {
|
if (this.needsBlockerSuspend) {
|
||||||
await resumeInputBlocker(this.browserManager).catch(() => {});
|
await resumeInputBlocker(this.browserManager, signal).catch(() => {});
|
||||||
}
|
}
|
||||||
|
|
||||||
debugLogger.error(`MCP tool ${this.toolName} failed: ${errorMsg}`);
|
debugLogger.error(`MCP tool ${this.toolName} failed: ${errorMsg}`);
|
||||||
|
|||||||
Reference in New Issue
Block a user