From 212a249fe1b2f048b7317aa4300fbd25509162aa Mon Sep 17 00:00:00 2001 From: Holger Benl Date: Tue, 3 Nov 2026 18:06:47 +0100 Subject: [PATCH 1/3] Bug 2081620 - Keep dialogs open until explicitly accepted or dismissed --- src/firefox/core.ts | 4 ++ .../firefox/connect-existing.test.ts | 16 ++++- tests/integration/mcp/dialog.test.ts | 72 +++++++++++++++++++ tests/unit/firefox/connect-existing.test.ts | 3 + tests/unit/firefox/core-prefs.test.ts | 1 + tests/unit/firefox/core.test.ts | 3 + 6 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 tests/integration/mcp/dialog.test.ts diff --git a/src/firefox/core.ts b/src/firefox/core.ts index 596c87b4..3481976b 100644 --- a/src/firefox/core.ts +++ b/src/firefox/core.ts @@ -3,6 +3,7 @@ */ import { Builder, Browser, Capabilities, WebDriver } from 'selenium-webdriver'; +import { UserPromptHandler } from 'selenium-webdriver/lib/capabilities.js'; import firefox from 'selenium-webdriver/firefox.js'; import { mkdirSync, @@ -290,6 +291,7 @@ export class FirefoxCore { if (this.options.acceptInsecureCerts) { caps.set('acceptInsecureCerts', true); } + caps.setAlertBehavior(UserPromptHandler.IGNORE); const serviceBuilder = new firefox.ServiceBuilder(geckodriverPath); this.driver = firefox.Driver.createSession(caps, serviceBuilder.build()); @@ -364,6 +366,7 @@ export class FirefoxCore { // session in a locked state that blocks reconnection. const caps = new Capabilities(); caps.set('webSocketUrl', true); + caps.setAlertBehavior(UserPromptHandler.IGNORE); // createSession() returns synchronously; the session is established async under the hood. // Passing geckodriverPath to ServiceBuilder prevents getBinaryPaths() from running, @@ -409,6 +412,7 @@ export class FirefoxCore { // Standard path: launch a new Firefox via selenium-webdriver const firefoxOptions = new firefox.Options(); firefoxOptions.enableBidi(); + firefoxOptions.setAlertBehavior(UserPromptHandler.IGNORE); if (this.options.headless) { firefoxOptions.addArguments('-headless'); diff --git a/tests/integration/firefox/connect-existing.test.ts b/tests/integration/firefox/connect-existing.test.ts index a02c51ab..e6ae17b4 100644 --- a/tests/integration/firefox/connect-existing.test.ts +++ b/tests/integration/firefox/connect-existing.test.ts @@ -59,7 +59,7 @@ function findFirefoxBinary(): string { return 'firefox'; } -describe('Connect Existing Viewport Tests', () => { +describe('Connect Existing Viewport and Dialog Tests', () => { let firefoxProcess: ChildProcess | undefined; let profile: string; let marionettePort: number; @@ -143,4 +143,18 @@ describe('Connect Existing Viewport Tests', () => { await closeFirefox(second); } }, 60000); + + it('should keep a prompt open until it is accepted', async () => { + const firefox = await createTestFirefox({ connectExisting: true, marionettePort }); + + await firefox.evaluate( + 'window.result = "pending"; setTimeout(() => { window.result = prompt("foo"); });' + ); + await new Promise((resolve) => setTimeout(resolve, 500)); + expect(await firefox.evaluate('window.result')).toBe('pending'); + + await firefox.acceptDialog('bar'); + await waitFor(async () => (await firefox.evaluate('window.result')) !== 'pending'); + expect(await firefox.evaluate('window.result')).toBe('bar'); + }); }); diff --git a/tests/integration/mcp/dialog.test.ts b/tests/integration/mcp/dialog.test.ts new file mode 100644 index 00000000..05390a5e --- /dev/null +++ b/tests/integration/mcp/dialog.test.ts @@ -0,0 +1,72 @@ +/** + * Integration tests for accept_dialog and dismiss_dialog, called through the MCP server + */ + +import { describe, expect } from 'vitest'; +import { mcpTest, type McpTestServer } from '../../helpers/mcp.js'; +import { waitFor } from '../../helpers/firefox.js'; + +async function getResult(mcp: McpTestServer) { + return (await mcp.callTool('evaluate_script', { function: '() => window.result' })).text; +} + +describe('Dialog handlers', () => { + describe.each([ + { + method: 'alert', + acceptExpectation: 'undefined', + dismissExpectation: 'undefined', + }, + { + method: 'confirm', + acceptExpectation: 'true', + dismissExpectation: 'false', + }, + { + method: 'prompt', + acceptExpectation: '"bar"', + dismissExpectation: 'null', + }, + ])('$method dialog', ({ method, acceptExpectation, dismissExpectation }) => { + mcpTest.beforeEach(async ({ mcp }) => { + await mcp.callTool('evaluate_script', { + function: `() => { window.result = 'pending'; setTimeout(() => { window.result = ${method}('foo'); }); }`, + }); + await new Promise((resolve) => setTimeout(resolve, 500)); + expect(await getResult(mcp)).toContain('"pending"'); + }); + + mcpTest('should be left open until it is accepted', async ({ mcp }) => { + const result = await mcp.callTool( + 'accept_dialog', + method === 'prompt' ? { promptText: 'bar' } : undefined + ); + expect(result.isError).toBe(false); + expect(result.text).toContain('Accepted'); + + await waitFor(async () => !(await getResult(mcp)).includes('"pending"')); + expect(await getResult(mcp)).toContain(acceptExpectation); + }); + + mcpTest('should be left open until it is dismissed', async ({ mcp }) => { + const result = await mcp.callTool('dismiss_dialog'); + expect(result.isError).toBe(false); + expect(result.text).toContain('Dismissed'); + + await waitFor(async () => !(await getResult(mcp)).includes('"pending"')); + expect(await getResult(mcp)).toContain(dismissExpectation); + }); + }); + + mcpTest('accept_dialog should fail if there is no dialog', async ({ mcp }) => { + const result = await mcp.callTool('accept_dialog'); + expect(result.isError).toBe(true); + expect(result.text).toContain('Failed to accept dialog'); + }); + + mcpTest('dismiss_dialog should fail if there is no dialog', async ({ mcp }) => { + const result = await mcp.callTool('dismiss_dialog'); + expect(result.isError).toBe(true); + expect(result.text).toContain('Failed to dismiss dialog'); + }); +}); diff --git a/tests/unit/firefox/connect-existing.test.ts b/tests/unit/firefox/connect-existing.test.ts index 4c9fa888..bbaae9b0 100644 --- a/tests/unit/firefox/connect-existing.test.ts +++ b/tests/unit/firefox/connect-existing.test.ts @@ -141,6 +141,7 @@ describe('FirefoxCore connect() BiDi endpoint check', () => { // Mocks for the launch path (used by the launch-mode test only) const mockEnableBidi = vi.fn(); + const mockSetAlertBehavior = vi.fn(); const mockOptionsAddArguments = vi.fn(); // Fake socket for the Marionette port probe: emits `events` in order on the @@ -199,6 +200,7 @@ describe('FirefoxCore connect() BiDi endpoint check', () => { default: { Options: class { enableBidi = mockEnableBidi; + setAlertBehavior = mockSetAlertBehavior; addArguments = mockOptionsAddArguments; windowSize = vi.fn(); setBinary = vi.fn(); @@ -218,6 +220,7 @@ describe('FirefoxCore connect() BiDi endpoint check', () => { vi.doMock('selenium-webdriver', () => ({ Capabilities: class { set = mockCapabilitiesSet; + setAlertBehavior = mockSetAlertBehavior; }, Builder: class { forBrowser = vi.fn().mockReturnThis(); diff --git a/tests/unit/firefox/core-prefs.test.ts b/tests/unit/firefox/core-prefs.test.ts index 8668c516..78c413de 100644 --- a/tests/unit/firefox/core-prefs.test.ts +++ b/tests/unit/firefox/core-prefs.test.ts @@ -27,6 +27,7 @@ describe('FirefoxCore prefs via firefoxOptions', () => { default: { Options: class { enableBidi = vi.fn(); + setAlertBehavior = vi.fn(); addArguments = vi.fn(); setBinary = vi.fn(); setAcceptInsecureCerts = vi.fn(); diff --git a/tests/unit/firefox/core.test.ts b/tests/unit/firefox/core.test.ts index 26685234..c3920296 100644 --- a/tests/unit/firefox/core.test.ts +++ b/tests/unit/firefox/core.test.ts @@ -313,6 +313,7 @@ describe('FirefoxCore connect() profile handling', () => { const mockAddArguments = vi.fn(); const mockSetProfile = vi.fn(); const mockEnableBidi = vi.fn(); + const mockSetAlertBehavior = vi.fn(); const mockSetBinary = vi.fn(); const mockWindowSize = vi.fn(); const mockSetAcceptInsecureCerts = vi.fn(); @@ -327,6 +328,7 @@ describe('FirefoxCore connect() profile handling', () => { default: { Options: class { enableBidi = mockEnableBidi; + setAlertBehavior = mockSetAlertBehavior; addArguments = mockAddArguments; setProfile = mockSetProfile; setBinary = mockSetBinary; @@ -453,6 +455,7 @@ describe('FirefoxCore connect() binary lookup failure', () => { default: { Options: class { enableBidi = vi.fn(); + setAlertBehavior = vi.fn(); addArguments = vi.fn(); setBinary = vi.fn(); windowSize = vi.fn(); From 68e0aaeabe0c4495e088ea0036c3e275bbf3d07b Mon Sep 17 00:00:00 2001 From: Holger Benl Date: Thu, 5 Nov 2026 13:09:36 +0100 Subject: [PATCH 2/3] Bug 2081600 - Return early when a dialog is opened during evaluate_script or click_by_uid --- src/firefox/events/dialogs.ts | 53 ++++++ src/firefox/events/index.ts | 1 + src/firefox/index.ts | 27 ++- src/tools/input.ts | 15 +- src/tools/script.ts | 161 +++++++++--------- src/tools/utilities.ts | 9 + tests/integration/mcp/dialog.test.ts | 34 +++- .../events/race-against-dialog.test.ts | 92 ++++++++++ 8 files changed, 304 insertions(+), 88 deletions(-) create mode 100644 src/firefox/events/dialogs.ts create mode 100644 tests/unit/firefox/events/race-against-dialog.test.ts diff --git a/src/firefox/events/dialogs.ts b/src/firefox/events/dialogs.ts new file mode 100644 index 00000000..6446dd64 --- /dev/null +++ b/src/firefox/events/dialogs.ts @@ -0,0 +1,53 @@ +/** + * Dialog events. + */ + +import type { BrowsingContext } from 'webdriver-bidi-protocol'; +import type { BiDiFacade } from '../bidi.js'; +import { logDebug } from '../../utils/logger.js'; + +export class DialogEvents { + private subscribed = false; + + constructor(private bidi: BiDiFacade) {} + + /** + * Subscribe to BiDi dialog events. + */ + async subscribe(): Promise { + if (this.subscribed) { + return; + } + + await this.bidi.subscribe(['browsingContext.userPromptOpened']); + + this.subscribed = true; + logDebug('Dialog listener ready'); + } + + async raceAgainstDialogOpened( + fn: () => Promise, + dialogOpenedResult: (dialog: BrowsingContext.UserPromptOpenedParameters) => T + ) { + let removeListener!: () => void; + const dialogOpened = new Promise((resolve) => { + const listener = (dialog: BrowsingContext.UserPromptOpenedParameters) => + resolve(dialogOpenedResult(dialog)); + this.bidi.on('browsingContext.userPromptOpened', listener); + removeListener = () => this.bidi.off('browsingContext.userPromptOpened', listener); + }); + try { + const promise = fn(); + void dialogOpened.then(() => + promise.catch((error) => + logDebug( + `Tool call errored after its handler returned early due to a dialog opening: ${String(error)}` + ) + ) + ); + return await Promise.race([promise, dialogOpened]); + } finally { + removeListener(); + } + } +} diff --git a/src/firefox/events/index.ts b/src/firefox/events/index.ts index 685e1427..9c2bf242 100644 --- a/src/firefox/events/index.ts +++ b/src/firefox/events/index.ts @@ -5,4 +5,5 @@ export { ConsoleEvents, type ConsoleEventsOptions } from './console.js'; export { NetworkEvents, type NetworkEventsOptions } from './network.js'; export { DebuggingEvents } from './debugging.js'; +export { DialogEvents } from './dialogs.js'; export { DownloadEvents } from './downloads.js'; diff --git a/src/firefox/index.ts b/src/firefox/index.ts index 5d7a1997..d195c5ee 100644 --- a/src/firefox/index.ts +++ b/src/firefox/index.ts @@ -8,7 +8,13 @@ import { FirefoxCore } from './core.js'; import { BiDiFacade, type FirefoxExtension } from './bidi.js'; import { listExtensions, type ListExtensionsFilters } from './webextension.js'; import { logDebug } from '../utils/logger.js'; -import { ConsoleEvents, NetworkEvents, DebuggingEvents, DownloadEvents } from './events/index.js'; +import { + ConsoleEvents, + NetworkEvents, + DebuggingEvents, + DownloadEvents, + DialogEvents, +} from './events/index.js'; import type { NetworkBodyResult } from './events/network.js'; import { DomInteractions } from './dom.js'; import { PageManagement } from './pages.js'; @@ -25,6 +31,7 @@ export class FirefoxClient { private consoleEvents: ConsoleEvents | null = null; private networkEvents: NetworkEvents | null = null; private debuggingEvents: DebuggingEvents | null = null; + private dialogEvents: DialogEvents | null = null; private downloadEvents: DownloadEvents | null = null; private dom: DomInteractions | null = null; private pages: PageManagement | null = null; @@ -106,6 +113,14 @@ export class FirefoxClient { this.debuggingEvents = null; } + this.dialogEvents = new DialogEvents(this.bidi); + try { + await this.dialogEvents.subscribe(); + } catch { + logDebug('Unable to subscribe to dialog events'); + this.dialogEvents = null; + } + this.downloadEvents = new DownloadEvents(this.bidi); try { await this.downloadEvents.subscribe(); @@ -239,6 +254,16 @@ export class FirefoxClient { return await this.pages.setViewportSize(width, height); } + async raceAgainstDialogOpened( + fn: () => Promise, + dialogOpenedResult: (dialog: BrowsingContext.UserPromptOpenedParameters) => T + ) { + if (!this.dialogEvents) { + throw new Error('Dialog events not available'); + } + return await this.dialogEvents.raceAgainstDialogOpened(fn, dialogOpenedResult); + } + async acceptDialog(promptText?: string): Promise { if (!this.pages) { throw new Error('Not connected'); diff --git a/src/tools/input.ts b/src/tools/input.ts index a2424fb6..8d04f53d 100644 --- a/src/tools/input.ts +++ b/src/tools/input.ts @@ -6,6 +6,7 @@ import { successResponse } from '../utils/response-helpers.js'; import { handleUidError } from '../utils/uid-helpers.js'; import { defineModule, defineToolHandler, type ToolDefinition } from './module.js'; +import { dialogOpenedResponse } from './utilities.js'; import type { McpToolResponse } from '../types/common.js'; // Tool definitions @@ -213,12 +214,14 @@ export const handleClickByUid = defineToolHandler( const { getFirefox } = await import('../index.js'); const firefox = await getFirefox(); - try { - await firefox.clickByUid(uid, dblClick); - return successResponse(`${dblClick ? 'dblclick' : 'click'} ${uid}`); - } catch (error) { - throw handleUidError(error as Error, uid); - } + return await firefox.raceAgainstDialogOpened(async () => { + try { + await firefox.clickByUid(uid, dblClick); + return successResponse(`${dblClick ? 'dblclick' : 'click'} ${uid}`); + } catch (error) { + throw handleUidError(error as Error, uid); + } + }, dialogOpenedResponse); } ); diff --git a/src/tools/script.ts b/src/tools/script.ts index c655e335..e2fc7131 100644 --- a/src/tools/script.ts +++ b/src/tools/script.ts @@ -8,6 +8,7 @@ import { remoteValueToNative } from '../utils/remote-value.js'; import { validateFunction } from '../utils/js-validation.js'; import { saveOutput } from '../utils/save-output.js'; import { defineModule, defineToolHandler, type ToolDefinition } from './module.js'; +import { dialogOpenedResponse } from './utilities.js'; import type { McpToolResponse } from '../types/common.js'; import { BiDiTimeoutError } from '../utils/errors.js'; @@ -76,98 +77,100 @@ export const handleEvaluateScript = defineToolHandler const { getFirefox } = await import('../index.js'); const firefox = await getFirefox(); - const scriptTimeout = timeout ?? DEFAULT_TIMEOUT; + return await firefox.raceAgainstDialogOpened(async () => { + const scriptTimeout = timeout ?? DEFAULT_TIMEOUT; - // Prepare arguments: resolve UIDs to references shared ids if provided - const resolvedArgs: Script.LocalValue[] = []; - if (fnArgs && fnArgs.length > 0) { - for (const arg of fnArgs) { - try { - resolvedArgs.push(await firefox.resolveUidToElement(arg.uid)); - } catch (error) { - const errorMsg = (error as Error).message; + // Prepare arguments: resolve UIDs to references shared ids if provided + const resolvedArgs: Script.LocalValue[] = []; + if (fnArgs && fnArgs.length > 0) { + for (const arg of fnArgs) { + try { + resolvedArgs.push(await firefox.resolveUidToElement(arg.uid)); + } catch (error) { + const errorMsg = (error as Error).message; - // Provide friendly error for stale UIDs - if ( - errorMsg.includes('stale') || - errorMsg.includes('Snapshot') || - errorMsg.includes('UID') - ) { - throw new Error( - `UID "${arg.uid}" is invalid or from an old snapshot.\n\n` + - 'The page may have changed since the snapshot was taken.\n' + - 'Please call take_snapshot to get fresh UIDs and try again.' - ); - } + // Provide friendly error for stale UIDs + if ( + errorMsg.includes('stale') || + errorMsg.includes('Snapshot') || + errorMsg.includes('UID') + ) { + throw new Error( + `UID "${arg.uid}" is invalid or from an old snapshot.\n\n` + + 'The page may have changed since the snapshot was taken.\n' + + 'Please call take_snapshot to get fresh UIDs and try again.' + ); + } - throw new Error(`Failed to resolve UID "${arg.uid}": ${errorMsg}`); + throw new Error(`Failed to resolve UID "${arg.uid}": ${errorMsg}`); + } } } - } - // Execute with resolved args (empty array if no args) - const context = firefox.getCurrentContextId(); - if (!context) { - throw new Error('No active browsing context'); - } - let result; - try { - result = await firefox.sendBiDiCommand( - 'script.callFunction', - { - functionDeclaration: fnString, - awaitPromise: true, - arguments: resolvedArgs, - target: { - context, - ...(sandbox !== undefined && { sandbox }), + // Execute with resolved args (empty array if no args) + const context = firefox.getCurrentContextId(); + if (!context) { + throw new Error('No active browsing context'); + } + let result; + try { + result = await firefox.sendBiDiCommand( + 'script.callFunction', + { + functionDeclaration: fnString, + awaitPromise: true, + arguments: resolvedArgs, + target: { + context, + ...(sandbox !== undefined && { sandbox }), + }, }, - }, - { timeout: scriptTimeout } - ); - } catch (error) { - if (error instanceof BiDiTimeoutError) { - return errorResponse( - new Error( - `Script execution timed out (exceeded ${scriptTimeout}ms).\n\n` + - 'The function may contain an infinite loop or be waiting for a slow operation.\n' + - 'Try simplifying the script or increasing the timeout parameter.' - ) + { timeout: scriptTimeout } ); + } catch (error) { + if (error instanceof BiDiTimeoutError) { + return errorResponse( + new Error( + `Script execution timed out (exceeded ${scriptTimeout}ms).\n\n` + + 'The function may contain an infinite loop or be waiting for a slow operation.\n' + + 'Try simplifying the script or increasing the timeout parameter.' + ) + ); + } + throw error; } - throw error; - } - if (result.type === 'success') { - // JSON.stringify returns undefined for an undefined script result - const json = JSON.stringify(remoteValueToNative(result.result), null, 2) ?? 'undefined'; + if (result.type === 'success') { + // JSON.stringify returns undefined for an undefined script result + const json = JSON.stringify(remoteValueToNative(result.result), null, 2) ?? 'undefined'; - if (saveTo) { - const saved = await saveOutput( - json, - saveTo === true ? undefined : saveTo, - 'evaluate-script' - ); - let output = `Script ran on page. Result saved to: ${saved.path} (${(saved.bytes / 1024).toFixed(1)}KB)`; - const excerpt = previewExcerpt(json, preview); - if (excerpt) { - output += '\nPreview:\n```json\n' + excerpt + '\n```'; + if (saveTo) { + const saved = await saveOutput( + json, + saveTo === true ? undefined : saveTo, + 'evaluate-script' + ); + let output = `Script ran on page. Result saved to: ${saved.path} (${(saved.bytes / 1024).toFixed(1)}KB)`; + const excerpt = previewExcerpt(json, preview); + if (excerpt) { + output += '\nPreview:\n```json\n' + excerpt + '\n```'; + } + return successResponse(output); } - return successResponse(output); - } - return successResponse('Script ran on page and returned:\n```json\n' + json + '\n```'); - } else { - const exceptionDetails = result.exceptionDetails; - return errorResponse( - new Error( - `Script execution failed: ${exceptionDetails.text}\n\n` + - '```json\n' + - JSON.stringify(remoteValueToNative(exceptionDetails.exception), null, 2) + - '\n```' - ) - ); - } + return successResponse('Script ran on page and returned:\n```json\n' + json + '\n```'); + } else { + const exceptionDetails = result.exceptionDetails; + return errorResponse( + new Error( + `Script execution failed: ${exceptionDetails.text}\n\n` + + '```json\n' + + JSON.stringify(remoteValueToNative(exceptionDetails.exception), null, 2) + + '\n```' + ) + ); + } + }, dialogOpenedResponse); } ); diff --git a/src/tools/utilities.ts b/src/tools/utilities.ts index 0bca7467..e8182844 100644 --- a/src/tools/utilities.ts +++ b/src/tools/utilities.ts @@ -4,6 +4,7 @@ import { successResponse } from '../utils/response-helpers.js'; import { defineModule, defineToolHandler, type ToolDefinition } from './module.js'; +import type { BrowsingContext } from 'webdriver-bidi-protocol'; import type { McpToolResponse } from '../types/common.js'; // Tool definitions - Dialogs @@ -102,6 +103,14 @@ export const handleDismissDialog = defineToolHandler( } ); +export function dialogOpenedResponse(dialog: BrowsingContext.UserPromptOpenedParameters) { + const defaultValue = + dialog.defaultValue !== undefined ? ` with default value "${dialog.defaultValue}"` : ''; + return successResponse( + `Dialog opened while tool was running: ${dialog.type} "${dialog.message}"${defaultValue}. Use accept_dialog or dismiss_dialog.` + ); +} + // Handlers - History export const handleNavigateHistory = defineToolHandler( async function handleNavigateHistory(args): Promise { diff --git a/tests/integration/mcp/dialog.test.ts b/tests/integration/mcp/dialog.test.ts index 05390a5e..98a8c891 100644 --- a/tests/integration/mcp/dialog.test.ts +++ b/tests/integration/mcp/dialog.test.ts @@ -2,9 +2,9 @@ * Integration tests for accept_dialog and dismiss_dialog, called through the MCP server */ -import { describe, expect } from 'vitest'; +import { assert, describe, expect } from 'vitest'; import { mcpTest, type McpTestServer } from '../../helpers/mcp.js'; -import { waitFor } from '../../helpers/firefox.js'; +import { fixtureUrl, waitFor } from '../../helpers/firefox.js'; async function getResult(mcp: McpTestServer) { return (await mcp.callTool('evaluate_script', { function: '() => window.result' })).text; @@ -69,4 +69,34 @@ describe('Dialog handlers', () => { expect(result.isError).toBe(true); expect(result.text).toContain('Failed to dismiss dialog'); }); + + mcpTest('evaluate_script should return when a dialog opens', async ({ mcp }) => { + const result = await mcp.callTool('evaluate_script', { + function: `() => alert('foo')`, + }); + expect(result.isError).toBe(false); + expect(result.text).toContain('Dialog opened while tool was running: alert "foo"'); + + const accepted = await mcp.callTool('accept_dialog'); + expect(accepted.isError).toBe(false); + expect(accepted.text).toContain('Accepted'); + }); + + mcpTest('click_by_uid should return when a dialog opens', async ({ mcp }) => { + await mcp.callTool('navigate_page', { url: fixtureUrl('simple.html') }); + await mcp.callTool('evaluate_script', { + function: `() => document.getElementById('clickBtn').addEventListener('click', () => alert('foo'))`, + }); + const snapshot = await mcp.callTool('take_snapshot'); + const uid = /uid=(\S+) button "Click Me"/.exec(snapshot.text)?.[1]; + assert(uid, 'snapshot contains the button uid'); + + const result = await mcp.callTool('click_by_uid', { uid }); + expect(result.isError).toBe(false); + expect(result.text).toContain('Dialog opened while tool was running: alert "foo"'); + + const accepted = await mcp.callTool('accept_dialog'); + expect(accepted.isError).toBe(false); + expect(accepted.text).toContain('Accepted'); + }); }); diff --git a/tests/unit/firefox/events/race-against-dialog.test.ts b/tests/unit/firefox/events/race-against-dialog.test.ts new file mode 100644 index 00000000..028d8136 --- /dev/null +++ b/tests/unit/firefox/events/race-against-dialog.test.ts @@ -0,0 +1,92 @@ +import EventEmitter from 'node:events'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { BrowsingContext } from 'webdriver-bidi-protocol'; +import { DialogEvents } from '../../../../src/firefox/events/dialogs.js'; +import type { BiDiFacade } from '../../../../src/firefox/bidi.js'; +import { logDebug } from '../../../../src/utils/logger.js'; + +vi.mock('../../../../src/utils/logger.js', () => ({ logDebug: vi.fn() })); + +const EVENT = 'browsingContext.userPromptOpened'; + +const DIALOG = { + context: 'ctx-1', + handler: 'ignore', + message: 'Hello', + type: 'alert', +} as BrowsingContext.UserPromptOpenedParameters; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +describe('DialogEvents.raceAgainstDialogOpened', () => { + let bidi: EventEmitter; + let events: DialogEvents; + + beforeEach(() => { + vi.mocked(logDebug).mockClear(); + bidi = new EventEmitter(); + events = new DialogEvents(bidi as unknown as BiDiFacade); + }); + + function race(fnResult: Promise) { + return events.raceAgainstDialogOpened( + () => fnResult, + (dialog) => `dialog: ${dialog.message}` + ); + } + + it('returns the result of fn if it resolves before a dialog opens', async () => { + const fn = deferred(); + const result = race(fn.promise); + fn.resolve('done'); + + await expect(result).resolves.toBe('done'); + expect(bidi.listenerCount(EVENT)).toBe(0); + expect(logDebug).not.toHaveBeenCalled(); + }); + + it('throws if fn throws before a dialog opens, without logging the error', async () => { + const fn = deferred(); + const result = race(fn.promise); + fn.reject(new Error('boom')); + + await expect(result).rejects.toThrow('boom'); + expect(bidi.listenerCount(EVENT)).toBe(0); + expect(logDebug).not.toHaveBeenCalled(); + }); + + it('returns dialogOpenedResult if a dialog opens before fn resolves', async () => { + const fn = deferred(); + const result = race(fn.promise); + bidi.emit(EVENT, DIALOG); + + await expect(result).resolves.toBe('dialog: Hello'); + expect(bidi.listenerCount(EVENT)).toBe(0); + + fn.resolve('done'); + await new Promise((resolve) => setImmediate(resolve)); + expect(logDebug).not.toHaveBeenCalled(); + }); + + it('logs the error if fn throws after a dialog opened', async () => { + const fn = deferred(); + const result = race(fn.promise); + bidi.emit(EVENT, DIALOG); + + await expect(result).resolves.toBe('dialog: Hello'); + expect(bidi.listenerCount(EVENT)).toBe(0); + + fn.reject(new Error('boom')); + await new Promise((resolve) => setImmediate(resolve)); + expect(logDebug).toHaveBeenCalledTimes(1); + expect(vi.mocked(logDebug).mock.calls[0][0]).toContain('Error: boom'); + }); +}); From 3854254ac934ca3d6627601c0b2193558a913e05 Mon Sep 17 00:00:00 2001 From: Holger Benl Date: Thu, 5 Nov 2026 13:56:02 +0100 Subject: [PATCH 3/3] fix: track open dialogs and use a prompt's default value in accept_dialog --- src/firefox/events/dialogs.ts | 21 +++++++++++- src/firefox/index.ts | 7 ++++ src/tools/utilities.ts | 12 +++++-- tests/integration/mcp/dialog.test.ts | 51 ++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/firefox/events/dialogs.ts b/src/firefox/events/dialogs.ts index 6446dd64..7158cede 100644 --- a/src/firefox/events/dialogs.ts +++ b/src/firefox/events/dialogs.ts @@ -7,6 +7,10 @@ import type { BiDiFacade } from '../bidi.js'; import { logDebug } from '../../utils/logger.js'; export class DialogEvents { + private dialogs = new Map< + BrowsingContext.BrowsingContext, + BrowsingContext.UserPromptOpenedParameters + >(); private subscribed = false; constructor(private bidi: BiDiFacade) {} @@ -19,12 +23,27 @@ export class DialogEvents { return; } - await this.bidi.subscribe(['browsingContext.userPromptOpened']); + await this.bidi.subscribe([ + 'browsingContext.userPromptOpened', + 'browsingContext.userPromptClosed', + ]); + + this.bidi.on('browsingContext.userPromptOpened', (dialog) => { + this.dialogs.set(dialog.context, dialog); + }); + + this.bidi.on('browsingContext.userPromptClosed', ({ context }) => { + this.dialogs.delete(context); + }); this.subscribed = true; logDebug('Dialog listener ready'); } + getDialog(context: BrowsingContext.BrowsingContext) { + return this.dialogs.get(context); + } + async raceAgainstDialogOpened( fn: () => Promise, dialogOpenedResult: (dialog: BrowsingContext.UserPromptOpenedParameters) => T diff --git a/src/firefox/index.ts b/src/firefox/index.ts index d195c5ee..6522af30 100644 --- a/src/firefox/index.ts +++ b/src/firefox/index.ts @@ -264,6 +264,13 @@ export class FirefoxClient { return await this.dialogEvents.raceAgainstDialogOpened(fn, dialogOpenedResult); } + getCurrentDialog() { + if (!this.dialogEvents) { + throw new Error('Dialog events not available'); + } + return this.dialogEvents.getDialog(this.getContext()); + } + async acceptDialog(promptText?: string): Promise { if (!this.pages) { throw new Error('Not connected'); diff --git a/src/tools/utilities.ts b/src/tools/utilities.ts index e8182844..d8e7b99d 100644 --- a/src/tools/utilities.ts +++ b/src/tools/utilities.ts @@ -87,9 +87,15 @@ export const handleAcceptDialog = defineToolHandler( const { getFirefox } = await import('../index.js'); const firefox = await getFirefox(); - - await firefox.acceptDialog(promptText); - return successResponse(promptText ? `Accepted: "${promptText}"` : 'Accepted'); + const dialog = firefox.getCurrentDialog(); + const value = !dialog + ? promptText + : (dialog.type as string) === 'prompt' + ? (promptText ?? dialog.defaultValue) + : undefined; + + await firefox.acceptDialog(value); + return successResponse(value !== undefined ? `Accepted: "${value}"` : 'Accepted'); } ); diff --git a/tests/integration/mcp/dialog.test.ts b/tests/integration/mcp/dialog.test.ts index 98a8c891..a1ccbfd9 100644 --- a/tests/integration/mcp/dialog.test.ts +++ b/tests/integration/mcp/dialog.test.ts @@ -99,4 +99,55 @@ describe('Dialog handlers', () => { expect(accepted.isError).toBe(false); expect(accepted.text).toContain('Accepted'); }); + + mcpTest( + `accept_dialog without promptText should use a prompt's default value`, + async ({ mcp }) => { + await mcp.callTool('evaluate_script', { + function: `() => window.result = prompt('foo', 'default value')`, + }); + + const accepted = await mcp.callTool('accept_dialog'); + expect(accepted.isError).toBe(false); + expect(accepted.text).toBe('Accepted: "default value"'); + + const result = await mcp.callTool('evaluate_script', { + function: '() => window.result', + }); + expect(result.isError).toBe(false); + expect(result.text).toContain('default value'); + } + ); + + mcpTest( + `accept_dialog should override a prompt's default value with an empty string`, + async ({ mcp }) => { + await mcp.callTool('evaluate_script', { + function: `() => window.result = prompt('foo', 'default value')`, + }); + + const accepted = await mcp.callTool('accept_dialog', { promptText: '' }); + expect(accepted.isError).toBe(false); + expect(accepted.text).toBe('Accepted: ""'); + + const result = await mcp.callTool('evaluate_script', { + function: '() => window.result', + }); + expect(result.isError).toBe(false); + expect(result.text).toContain('""'); + } + ); + + mcpTest( + 'accept_dialog should ignore promptText if the current dialog is not a prompt', + async ({ mcp }) => { + await mcp.callTool('evaluate_script', { + function: `() => alert('foo')`, + }); + + const accepted = await mcp.callTool('accept_dialog', { promptText: 'bar' }); + expect(accepted.isError).toBe(false); + expect(accepted.text).toBe('Accepted'); + } + ); });