From d441a60c9f8a95fe0676c91f20df760991b32fad Mon Sep 17 00:00:00 2001 From: Matthew Merrill Date: Fri, 16 Oct 2026 15:00:25 -0600 Subject: [PATCH] Bug 2081620 - Keep page dialogs open until accept_dialog or dismiss_dialog handles them The sessions never set unhandledPromptBehavior, so the WebDriver Classic default "dismiss and notify" applied and Firefox closed each alert, confirm and prompt as soon as it opened. confirm() always returned false and prompt() always returned null before the tools could act. All three session paths (launch, --connect-existing and Android) now request unhandledPromptBehavior "ignore". Marionette reports a missing dialog as NoSuchAlertError with an empty message, and acceptDialog and dismissDialog wrapped it in "Failed to accept dialog: ". The tools looked for "no such alert" in the message, so "No active dialog" never showed. PageManagement now keeps that error as it is and the tools check error.name. --- src/firefox/core.ts | 6 + src/firefox/pages.ts | 20 +++- src/tools/utilities.ts | 8 +- tests/firefox/connect-existing.test.ts | 32 ++++++ tests/firefox/core-prefs.test.ts | 1 + tests/firefox/core.test.ts | 3 + tests/firefox/pages.test.ts | 50 +++++++++ tests/integration/dialog.integration.test.ts | 111 +++++++++++++++++++ tests/tools/utilities.test.ts | 55 ++++++++- 9 files changed, 273 insertions(+), 13 deletions(-) create mode 100644 tests/integration/dialog.integration.test.ts diff --git a/src/firefox/core.ts b/src/firefox/core.ts index 032085fe..19c31471 100644 --- a/src/firefox/core.ts +++ b/src/firefox/core.ts @@ -157,6 +157,9 @@ const BINARY_NOT_FOUND_HINT = 'Unable to detect Firefox binary automatically, please provide the full path via ' + '--firefox-path'; +/** Leave prompts open until accept_dialog or dismiss_dialog handles them. */ +const UNHANDLED_PROMPT_BEHAVIOR = 'ignore'; + export class FirefoxCore { private currentContextId: string | null = null; private detectedBinaryPath: string | undefined; @@ -218,6 +221,7 @@ export class FirefoxCore { const caps = new Capabilities(); caps.set('webSocketUrl', true); + caps.set('unhandledPromptBehavior', UNHANDLED_PROMPT_BEHAVIOR); caps.set('moz:firefoxOptions', mozOptions); if (this.options.acceptInsecureCerts) { caps.set('acceptInsecureCerts', true); @@ -275,6 +279,7 @@ export class FirefoxCore { // session in a locked state that blocks reconnection. const caps = new Capabilities(); caps.set('webSocketUrl', true); + caps.set('unhandledPromptBehavior', UNHANDLED_PROMPT_BEHAVIOR); // createSession() returns synchronously; the session is established async under the hood. // Passing geckodriverPath to ServiceBuilder prevents getBinaryPaths() from running, @@ -317,6 +322,7 @@ export class FirefoxCore { // Standard path: launch a new Firefox via selenium-webdriver const firefoxOptions = new firefox.Options(); firefoxOptions.enableBidi(); + firefoxOptions.setAlertBehavior(UNHANDLED_PROMPT_BEHAVIOR); if (this.options.headless) { firefoxOptions.addArguments('-headless'); diff --git a/src/firefox/pages.ts b/src/firefox/pages.ts index 48fec917..182cd9ce 100644 --- a/src/firefox/pages.ts +++ b/src/firefox/pages.ts @@ -36,6 +36,18 @@ export function isReadinessState(value: unknown): value is BrowsingContext.Readi return READINESS_STATES.includes(value as BrowsingContext.ReadinessState); } +/** + * Keep NoSuchAlertError as is so callers can tell it apart: Marionette sends it with an empty message. + */ +function dialogError(action: 'accept' | 'dismiss', error: unknown): Error { + if (error instanceof Error && error.name === 'NoSuchAlertError') { + return error; + } + return new Error( + `Failed to ${action} dialog: ${error instanceof Error ? error.message : String(error)}` + ); +} + export class PageManagement { constructor( private driver: WebDriver, @@ -108,9 +120,7 @@ export class PageManagement { } await alert.accept(); } catch (error) { - throw new Error( - `Failed to accept dialog: ${error instanceof Error ? error.message : String(error)}` - ); + throw dialogError('accept', error); } } @@ -122,9 +132,7 @@ export class PageManagement { const alert = await this.driver.switchTo().alert(); await alert.dismiss(); } catch (error) { - throw new Error( - `Failed to dismiss dialog: ${error instanceof Error ? error.message : String(error)}` - ); + throw dialogError('dismiss', error); } } diff --git a/src/tools/utilities.ts b/src/tools/utilities.ts index 372239e7..4dbcd82e 100644 --- a/src/tools/utilities.ts +++ b/src/tools/utilities.ts @@ -92,10 +92,8 @@ export const handleAcceptDialog = defineToolHandler(async function handleAcceptD await firefox.acceptDialog(promptText); return successResponse(promptText ? `Accepted: "${promptText}"` : 'Accepted'); } catch (error) { - const errorMsg = (error as Error).message; - // Concise error for no active dialog - if (errorMsg.includes('no such alert') || errorMsg.includes('No dialog')) { + if ((error as Error).name === 'NoSuchAlertError') { throw new Error('No active dialog'); } @@ -113,10 +111,8 @@ export const handleDismissDialog = defineToolHandler(async function handleDismis await firefox.dismissDialog(); return successResponse('Dismissed'); } catch (error) { - const errorMsg = (error as Error).message; - // Concise error for no active dialog - if (errorMsg.includes('no such alert') || errorMsg.includes('No dialog')) { + if ((error as Error).name === 'NoSuchAlertError') { throw new Error('No active dialog'); } diff --git a/tests/firefox/connect-existing.test.ts b/tests/firefox/connect-existing.test.ts index 50ce1136..412fda06 100644 --- a/tests/firefox/connect-existing.test.ts +++ b/tests/firefox/connect-existing.test.ts @@ -136,6 +136,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 `event` on the next tick. @@ -183,6 +184,7 @@ describe('FirefoxCore connect() BiDi endpoint check', () => { default: { Options: class { enableBidi = mockEnableBidi; + setAlertBehavior = mockSetAlertBehavior; addArguments = mockOptionsAddArguments; windowSize = vi.fn(); setBinary = vi.fn(); @@ -373,4 +375,34 @@ describe('FirefoxCore connect() BiDi endpoint check', () => { expect(mockCreateSession).not.toHaveBeenCalled(); expect(core.getCurrentContextId()).toBe('mock-context-id'); }); + + describe('unhandled prompt behavior', () => { + it('should keep prompts open in launch mode', async () => { + const { FirefoxCore } = await import('@/firefox/core.js'); + + await new FirefoxCore({ headless: true }).connect(); + + expect(mockSetAlertBehavior).toHaveBeenCalledWith('ignore'); + }); + + it('should keep prompts open in connect-existing mode', async () => { + mockCreateSession.mockReturnValue( + makeDriver({ browserVersion: '142.0', webSocketUrl: 'ws://127.0.0.1:9222/session/abc' }) + ); + const { FirefoxCore } = await import('@/firefox/core.js'); + + await new FirefoxCore({ connectExisting: true, marionettePort: 2828 }).connect(); + + expect(mockCapabilitiesSet).toHaveBeenCalledWith('unhandledPromptBehavior', 'ignore'); + }); + + it('should keep prompts open in Android mode', async () => { + mockCreateSession.mockReturnValue(makeDriver({ browserVersion: '142.0' })); + const { FirefoxCore } = await import('@/firefox/core.js'); + + await new FirefoxCore({ androidDevice: 'auto', androidWipeAppData: true }).connect(); + + expect(mockCapabilitiesSet).toHaveBeenCalledWith('unhandledPromptBehavior', 'ignore'); + }); + }); }); diff --git a/tests/firefox/core-prefs.test.ts b/tests/firefox/core-prefs.test.ts index 9fdb9617..66792650 100644 --- a/tests/firefox/core-prefs.test.ts +++ b/tests/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/firefox/core.test.ts b/tests/firefox/core.test.ts index 26685234..c3920296 100644 --- a/tests/firefox/core.test.ts +++ b/tests/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(); diff --git a/tests/firefox/pages.test.ts b/tests/firefox/pages.test.ts index dcf6727a..5f34582e 100644 --- a/tests/firefox/pages.test.ts +++ b/tests/firefox/pages.test.ts @@ -464,4 +464,54 @@ describe('PageManagement', () => { expect(driver.close).not.toHaveBeenCalled(); }); }); + + describe('dialogs', () => { + function createDialogMocks(alert: Record) { + const driver = { + switchTo: vi.fn(() => ({ alert: vi.fn().mockResolvedValue(alert) })), + } as any; + const pages = new PageManagement(driver, vi.fn(), vi.fn(), vi.fn()); + return { pages, driver }; + } + + function noSuchAlertError(): Error { + const error = new Error(''); + error.name = 'NoSuchAlertError'; + return error; + } + + it('types the prompt text before accepting', async () => { + const alert = { sendKeys: vi.fn(), accept: vi.fn(), dismiss: vi.fn() }; + const { pages } = createDialogMocks(alert); + + await pages.acceptDialog('hello'); + + expect(alert.sendKeys).toHaveBeenCalledWith('hello'); + expect(alert.accept).toHaveBeenCalledOnce(); + }); + + it('keeps NoSuchAlertError unchanged when there is no dialog', async () => { + const error = noSuchAlertError(); + const alert = { + sendKeys: vi.fn(), + accept: vi.fn().mockRejectedValue(error), + dismiss: vi.fn().mockRejectedValue(error), + }; + const { pages } = createDialogMocks(alert); + + await expect(pages.acceptDialog()).rejects.toBe(error); + await expect(pages.dismissDialog()).rejects.toBe(error); + }); + + it('wraps other errors with the action that failed', async () => { + const alert = { + accept: vi.fn().mockRejectedValue(new Error('boom')), + dismiss: vi.fn().mockRejectedValue(new Error('bang')), + }; + const { pages } = createDialogMocks(alert); + + await expect(pages.acceptDialog()).rejects.toThrow('Failed to accept dialog: boom'); + await expect(pages.dismissDialog()).rejects.toThrow('Failed to dismiss dialog: bang'); + }); + }); }); diff --git a/tests/integration/dialog.integration.test.ts b/tests/integration/dialog.integration.test.ts new file mode 100644 index 00000000..15de435a --- /dev/null +++ b/tests/integration/dialog.integration.test.ts @@ -0,0 +1,111 @@ +/** + * Integration tests for accept_dialog and dismiss_dialog + * Tests with real Firefox browser in headless mode. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { McpToolResponse } from '@/types/common.js'; +import type { FirefoxClient } from '@/firefox/index.js'; +import { handleAcceptDialog, handleDismissDialog } from '@/tools/utilities.js'; +import { createTestFirefox, closeFirefox, waitFor } from '../helpers/firefox.js'; + +// The dialog tools resolve the browser through the server's global getFirefox(), +// so it is redirected to the Firefox instance launched by this test. +const testState = vi.hoisted(() => ({ firefox: null as FirefoxClient | null })); + +vi.mock('../../src/index.js', () => ({ + getFirefox: async () => testState.firefox, +})); + +const PAGE = 'data:text/html,

dialogs

'; + +function responseText(response: McpToolResponse): string { + return response.content.map((item) => (item.text as string) ?? '').join('\n'); +} + +describe('Dialog Integration Tests', () => { + let firefox: FirefoxClient; + + beforeAll(async () => { + firefox = await createTestFirefox(); + testState.firefox = firefox; + }); + + afterAll(async () => { + testState.firefox = null; + await closeFirefox(firefox); + }); + + async function evaluateString(expression: string): Promise { + return String(await firefox.evaluate(expression)); + } + + /** + * Opens a dialog from a timer so the evaluation returns, then checks that + * the page is still blocked in the dialog after the browser had time to + * close it on its own. + */ + async function openDialog(call: string): Promise { + await firefox.navigate(PAGE); + await firefox.evaluate( + `window.result = 'pending'; setTimeout(() => { window.result = ${call}; }, 50); 1` + ); + await new Promise((resolve) => setTimeout(resolve, 500)); + expect(await evaluateString('String(window.result)')).toBe('pending'); + } + + async function resultOnceSettled(): Promise { + await waitFor(async () => (await evaluateString('String(window.result)')) !== 'pending', 5000); + return evaluateString('JSON.stringify(window.result)'); + } + + it('should leave a confirm open until it is accepted', async () => { + await openDialog("confirm('Proceed?')"); + + const response = await handleAcceptDialog({}); + + expect(response.isError).toBeUndefined(); + expect(responseText(response)).toBe('Accepted'); + expect(await resultOnceSettled()).toBe('true'); + }, 30000); + + it('should leave a confirm open until it is dismissed', async () => { + await openDialog("confirm('Proceed?')"); + + const response = await handleDismissDialog({}); + + expect(response.isError).toBeUndefined(); + expect(responseText(response)).toBe('Dismissed'); + expect(await resultOnceSettled()).toBe('false'); + }, 30000); + + it('should return the text given to accept_dialog from a prompt', async () => { + await openDialog("prompt('Name?')"); + + const response = await handleAcceptDialog({ promptText: 'Ada' }); + + expect(response.isError).toBeUndefined(); + expect(await resultOnceSettled()).toBe('"Ada"'); + }, 30000); + + it('should return null from a prompt that is dismissed', async () => { + await openDialog("prompt('Name?')"); + + const response = await handleDismissDialog({}); + + expect(response.isError).toBeUndefined(); + expect(await resultOnceSettled()).toBe('null'); + }, 30000); + + it('should report no active dialog when none is open', async () => { + await firefox.navigate(PAGE); + + const accepted = await handleAcceptDialog({}); + const dismissed = await handleDismissDialog({}); + + expect(accepted.isError).toBe(true); + expect(responseText(accepted)).toBe('Error: No active dialog'); + expect(dismissed.isError).toBe(true); + expect(responseText(dismissed)).toBe('Error: No active dialog'); + }, 30000); +}); diff --git a/tests/tools/utilities.test.ts b/tests/tools/utilities.test.ts index 8f3235b8..f2b993ab 100644 --- a/tests/tools/utilities.test.ts +++ b/tests/tools/utilities.test.ts @@ -2,9 +2,11 @@ * Unit tests for utilities tools */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { acceptDialogTool, + handleAcceptDialog, + handleDismissDialog, dismissDialogTool, navigateHistoryTool, setViewportSizeTool, @@ -59,4 +61,55 @@ describe('Utilities Tools', () => { expect(required).toContain('height'); }); }); + + describe('Dialog handlers', () => { + const acceptDialog = vi.fn(); + const dismissDialog = vi.fn(); + + function errorText(result: { content: Array<{ type: string; text?: string }> }): string { + return result.content[0]?.text ?? ''; + } + + beforeEach(() => { + acceptDialog.mockReset(); + dismissDialog.mockReset(); + vi.doMock('../../src/index.js', () => ({ + getFirefox: vi.fn().mockResolvedValue({ acceptDialog, dismissDialog }), + })); + }); + + it('accept_dialog reports no active dialog for NoSuchAlertError with an empty message', async () => { + const error = new Error(''); + error.name = 'NoSuchAlertError'; + acceptDialog.mockRejectedValue(error); + + const result = await handleAcceptDialog({}); + + expect(result.isError).toBe(true); + expect(errorText(result)).toBe('Error: No active dialog'); + }); + + it('dismiss_dialog reports no active dialog for NoSuchAlertError with an empty message', async () => { + const error = new Error(''); + error.name = 'NoSuchAlertError'; + dismissDialog.mockRejectedValue(error); + + const result = await handleDismissDialog({}); + + expect(result.isError).toBe(true); + expect(errorText(result)).toBe('Error: No active dialog'); + }); + + it('does not treat other errors as a missing dialog', async () => { + acceptDialog.mockRejectedValue(new Error('Failed to accept dialog: no such alert')); + dismissDialog.mockRejectedValue(new Error('Failed to dismiss dialog: boom')); + + expect(errorText(await handleAcceptDialog({}))).toBe( + 'Error: Failed to accept dialog: no such alert' + ); + expect(errorText(await handleDismissDialog({}))).toBe( + 'Error: Failed to dismiss dialog: boom' + ); + }); + }); });