diff --git a/.github/actions/find/tests/findForUrl.test.ts b/.github/actions/find/tests/findForUrl.test.ts index 387e2360..cbbdca70 100644 --- a/.github/actions/find/tests/findForUrl.test.ts +++ b/.github/actions/find/tests/findForUrl.test.ts @@ -186,6 +186,20 @@ describe('findForUrl', () => { expect(playwrightMocks.contextClose).toHaveBeenCalledTimes(1) expect(playwrightMocks.browserClose).toHaveBeenCalledTimes(1) }) + + it('preserves caller logging and partial-result behavior when reflow fails', async () => { + const error = new Error('overflow evaluation timed out') + loadedPlugins = [{name: 'reflow-scan', default: vi.fn().mockRejectedValue(error)}] + actionInput = JSON.stringify(['reflow-scan', 'axe']) + clearAll() + + await expect(findForUrl({url: 'test.com'})).resolves.toEqual([]) + + expect(core.error).toHaveBeenCalledWith(`Error during accessibility scan: ${error}`) + expect(AxeBuilder.prototype.analyze).not.toHaveBeenCalled() + expect(playwrightMocks.contextClose).toHaveBeenCalledTimes(1) + expect(playwrightMocks.browserClose).toHaveBeenCalledTimes(1) + }) }) describe('when no scans list is provided', () => { diff --git a/.github/actions/find/tests/reflowScan.test.ts b/.github/actions/find/tests/reflowScan.test.ts new file mode 100644 index 00000000..88f797b7 --- /dev/null +++ b/.github/actions/find/tests/reflowScan.test.ts @@ -0,0 +1,156 @@ +import {afterEach, describe, expect, it, vi} from 'vitest' +import reflowScan, {FINDING_TIMEOUT_MS, OPERATION_TIMEOUT_MS} from '../../../scanner-plugins/reflow-scan/index.js' + +const originalViewport = {width: 1280, height: 720} + +function createPage({scrollWidth = 320, clientWidth = 320} = {}) { + return { + viewportSize: vi.fn<() => typeof originalViewport | null>(() => originalViewport), + url: vi.fn(() => 'https://example.com'), + setViewportSize: vi.fn().mockResolvedValue(undefined), + evaluate: vi.fn().mockResolvedValue({scrollWidth, clientWidth}), + } +} + +afterEach(() => { + vi.useRealTimers() +}) + +describe('reflow scan', () => { + it('restores the original viewport when no overflow is found', async () => { + const page = createPage() + const addFinding = vi.fn() + + await reflowScan({page, addFinding}) + + expect(page.setViewportSize).toHaveBeenNthCalledWith(1, {width: 320, height: 256}) + expect(page.setViewportSize).toHaveBeenNthCalledWith(2, originalViewport) + expect(addFinding).not.toHaveBeenCalled() + }) + + it('preserves the overflow finding and restores the original viewport', async () => { + const page = createPage({scrollWidth: 400, clientWidth: 320}) + const addFinding = vi.fn().mockResolvedValue(undefined) + + await reflowScan({page, addFinding}) + + expect(addFinding).toHaveBeenCalledWith( + expect.objectContaining({ + scannerType: 'reflow-scan', + url: 'https://example.com', + problemShort: 'needs review: page presents a horizontal scrollbar at a 320px wide viewport', + solutionLong: expect.stringContaining('scroll width of 400px but a client width of only 320px'), + }), + ) + expect(page.setViewportSize).toHaveBeenLastCalledWith(originalViewport) + }) + + it.each([ + [ + 'viewport resize', + (page: ReturnType) => page.setViewportSize.mockRejectedValueOnce(new Error('resize')), + ], + [ + 'overflow evaluation', + (page: ReturnType) => page.evaluate.mockRejectedValueOnce(new Error('evaluate')), + ], + [ + 'finding callback', + (page: ReturnType) => + page.evaluate.mockResolvedValueOnce({scrollWidth: 400, clientWidth: 320}), + ], + ])('reports a rejected %s and attempts restoration', async (operation, configurePage) => { + const page = createPage() + configurePage(page) + const addFinding = vi.fn().mockRejectedValue(new Error('finding')) + + await expect(reflowScan({page, addFinding})).rejects.toMatchObject({ + name: 'ReflowScanError', + operation, + }) + + expect(page.setViewportSize).toHaveBeenCalledTimes(2) + }) + + it('reports restoration failure after a successful check', async () => { + const page = createPage() + page.setViewportSize.mockResolvedValueOnce(undefined).mockRejectedValueOnce(new Error('restore')) + + await expect(reflowScan({page, addFinding: vi.fn()})).rejects.toMatchObject({ + name: 'ReflowScanError', + operation: 'viewport restoration', + pageStateUnsafe: true, + }) + }) + + it('preserves the scan error when viewport restoration also fails', async () => { + const page = createPage() + const scanError = new Error('resize') + const restoreError = new Error('restore') + page.setViewportSize.mockRejectedValueOnce(scanError).mockRejectedValueOnce(restoreError) + + await expect(reflowScan({page, addFinding: vi.fn()})).rejects.toMatchObject({ + name: 'ReflowScanError', + operation: 'viewport resize', + cause: expect.objectContaining({cause: scanError}), + cleanupError: expect.objectContaining({cause: restoreError}), + pageStateUnsafe: true, + }) + }) + + it.each(['viewport resize', 'overflow evaluation', 'finding callback', 'viewport restoration'])( + 'bounds a non-returning %s operation', + async operation => { + vi.useFakeTimers() + const page = createPage({scrollWidth: 400, clientWidth: 320}) + const never = () => new Promise(() => {}) + + if (operation === 'viewport resize') { + page.setViewportSize.mockImplementationOnce(never) + } else if (operation === 'overflow evaluation') { + page.evaluate.mockImplementationOnce(never) + } else if (operation === 'finding callback') { + page.evaluate.mockResolvedValueOnce({scrollWidth: 400, clientWidth: 320}) + } else { + page.setViewportSize.mockResolvedValueOnce(undefined).mockImplementationOnce(never) + } + + const addFinding = operation === 'finding callback' ? vi.fn(never) : vi.fn().mockResolvedValue(undefined) + const scan = reflowScan({page, addFinding}) + const rejection = expect(scan).rejects.toMatchObject({ + name: 'ReflowScanError', + operation, + pageStateUnsafe: true, + }) + + await vi.advanceTimersByTimeAsync(operation === 'finding callback' ? FINDING_TIMEOUT_MS : OPERATION_TIMEOUT_MS) + + await rejection + }, + ) + + it('allows a healthy finding callback to take longer than the viewport-operation budget', async () => { + vi.useFakeTimers() + const page = createPage({scrollWidth: 400}) + const addFinding = vi.fn(() => new Promise(resolve => setTimeout(resolve, 31000))) + const scan = reflowScan({page, addFinding}) + + await vi.advanceTimersByTimeAsync(31000) + await expect(scan).resolves.toBeUndefined() + + expect(page.setViewportSize).toHaveBeenLastCalledWith(originalViewport) + expect(vi.getTimerCount()).toBe(0) + }) + + it('does not change the page when its original viewport is unavailable', async () => { + const page = createPage() + page.viewportSize.mockReturnValue(null) + + await expect(reflowScan({page, addFinding: vi.fn()})).rejects.toMatchObject({ + name: 'ReflowScanError', + operation: 'viewport capture', + }) + + expect(page.setViewportSize).not.toHaveBeenCalled() + }) +}) diff --git a/.github/scanner-plugins/reflow-scan/index.ts b/.github/scanner-plugins/reflow-scan/index.ts index 7f6a5e31..f8d0c206 100644 --- a/.github/scanner-plugins/reflow-scan/index.ts +++ b/.github/scanner-plugins/reflow-scan/index.ts @@ -1,32 +1,155 @@ -export default async function reflowScan({page, addFinding} = {}) { - const originalViewport = page.viewportSize() +import type {PluginDefaultParams} from '../../actions/find/src/pluginManager/types.js' + +// Cooperative wait budgets only: timers must run, and browser teardown is not included. +export const OPERATION_TIMEOUT_MS = 5000 +export const FINDING_TIMEOUT_MS = 35000 + +class ReflowOperationError extends Error { + constructor( + readonly operation: string, + message: string, + readonly timedOut: boolean, + options?: ErrorOptions, + ) { + super(`${operation} ${message}`, options) + this.name = 'ReflowOperationError' + } +} + +export class ReflowScanError extends Error { + constructor( + readonly operation: string, + cause: ReflowOperationError, + readonly cleanupError: ReflowOperationError | undefined, + readonly pageStateUnsafe: boolean, + ) { + const cleanupMessage = cleanupError ? `; viewport restoration also failed: ${cleanupError.message}` : '' + const unsafeMessage = pageStateUnsafe ? '; page state may be unsafe and will not be reused' : '' + super(`${cause.message}${cleanupMessage}${unsafeMessage}`, {cause}) + this.name = 'ReflowScanError' + } +} + +export default async function reflowScan({ + page, + addFinding, +}: { + page: Pick + addFinding: PluginDefaultParams['addFinding'] +}) { + let originalViewport + try { + originalViewport = page.viewportSize() + } catch (error) { + throw new ReflowScanError( + 'viewport capture', + new ReflowOperationError('viewport capture', `failed: ${errorMessage(error)}`, false, {cause: error}), + undefined, + false, + ) + } + + if (!originalViewport) { + throw new ReflowScanError( + 'viewport capture', + new ReflowOperationError('viewport capture', 'is unavailable; cannot safely change the viewport', false), + undefined, + false, + ) + } + const url = page.url() - // Check for horizontal scrolling at 320 viewport + let resizeAttempted = false + let operationError: ReflowOperationError | undefined + let cleanupError: ReflowOperationError | undefined + try { - await page.setViewportSize({width: 320, height: 256}) - const scrollWidth = await page.evaluate(() => document.documentElement.scrollWidth) - const clientWidth = await page.evaluate(() => document.documentElement.clientWidth) + resizeAttempted = true + await runBounded('viewport resize', () => page.setViewportSize({width: 320, height: 256})) + + const {scrollWidth, clientWidth} = await runBounded('overflow evaluation', () => + page.evaluate(() => ({ + scrollWidth: document.documentElement.scrollWidth, + clientWidth: document.documentElement.clientWidth, + })), + ) - // If horizontal scroll is required (with 1px tolerance for rounding) if (scrollWidth > clientWidth + 1) { - await addFinding({ - scannerType: 'reflow-scan', - url, - problemShort: 'needs review: page presents a horizontal scrollbar at a 320px wide viewport', - problemUrl: 'https://www.w3.org/WAI/WCAG22/Understanding/reflow.html', - solutionShort: - 'verify if sections of content can be viewed within the 320px wide viewport without needing to scroll in two dimensions to read the content of an individual section', - solutionLong: `The page has a scroll width of ${scrollWidth}px but a client width of only ${clientWidth}px at a 320px wide viewport, resulting in a horizontal scrollbar. Ensure that multi-line text does not require scrolling in two-dimensions to read, as this would be a violation of WCAG Success Criterion 1.4.10 (Reflow).`, - }) + await runBounded( + 'finding callback', + () => + addFinding({ + scannerType: 'reflow-scan', + url, + problemShort: 'needs review: page presents a horizontal scrollbar at a 320px wide viewport', + problemUrl: 'https://www.w3.org/WAI/WCAG22/Understanding/reflow.html', + solutionShort: + 'verify if sections of content can be viewed within the 320px wide viewport without needing to scroll in two dimensions to read the content of an individual section', + solutionLong: `The page has a scroll width of ${scrollWidth}px but a client width of only ${clientWidth}px at a 320px wide viewport, resulting in a horizontal scrollbar. Ensure that multi-line text does not require scrolling in two-dimensions to read, as this would be a violation of WCAG Success Criterion 1.4.10 (Reflow).`, + }), + FINDING_TIMEOUT_MS, + ) } - } catch (e) { - console.error('Error checking horizontal scroll:', e) + } catch (error) { + operationError = + error instanceof ReflowOperationError + ? error + : new ReflowOperationError('reflow scan', `failed: ${errorMessage(error)}`, false, {cause: error}) } finally { - // Restore original viewport so subsequent scans (e.g. Axe) aren't affected - if (originalViewport) { - await page.setViewportSize(originalViewport) + if (resizeAttempted) { + try { + await runBounded('viewport restoration', () => page.setViewportSize(originalViewport)) + } catch (error) { + cleanupError = + error instanceof ReflowOperationError + ? error + : new ReflowOperationError('viewport restoration', `failed: ${errorMessage(error)}`, false, { + cause: error, + }) + } } } + + const failure = operationError ?? cleanupError + if (failure) { + throw new ReflowScanError( + failure.operation, + failure, + operationError ? cleanupError : undefined, + failure.timedOut || cleanupError !== undefined, + ) + } +} + +async function runBounded( + operation: string, + action: () => Promise, + timeoutMs = OPERATION_TIMEOUT_MS, +): Promise { + let timer: ReturnType | undefined + const timeout = new Promise((_, reject) => { + timer = setTimeout( + () => reject(new ReflowOperationError(operation, `timed out after ${timeoutMs}ms`, true)), + timeoutMs, + ) + }) + + try { + return await Promise.race([ + Promise.resolve() + .then(action) + .catch(cause => { + throw new ReflowOperationError(operation, `failed: ${errorMessage(cause)}`, false, {cause}) + }), + timeout, + ]) + } finally { + if (timer) clearTimeout(timer) + } +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) } export const name = 'reflow-scan' diff --git a/PLUGINS.md b/PLUGINS.md index 7dd702df..8b66b070 100644 --- a/PLUGINS.md +++ b/PLUGINS.md @@ -5,6 +5,18 @@ The plugin system allows teams to create custom scans/tests to run on their page Some first-party plugins come built-in with the scanner repository and can be enabled via [actions inputs](https://github.com/github/accessibility-scanner/tree/main/action.yml#L48-L50). There are also some [allowlisted](https://github.com/github/accessibility-scanner/blob/7ec4b73ca2fdb471ad6f71a48a88e7b1a84ab3ad/.github/actions/find/src/pluginManager/index.ts#L92) first-party plugins hosted in a different repository; we also have the ability to support loading approved third-party plugins [from NPM](#loading-plugins-from-npm-packages). +## Built-in reflow scan + +Enable `reflow-scan` in the `scans` input to check for horizontal overflow at a 320 x 256 viewport. It keeps the existing 1px rounding tolerance and restores the original viewport after checking. If the original viewport is unavailable, it fails without resizing the page. + +Viewport resize, overflow evaluation, and viewport restoration each have a **5-second cooperative wait budget**. The finding callback has a separate **35-second budget**, allowing the screenshot library's existing 30-second timeout plus headroom for saving and recording the finding. This avoids reducing normal screenshot capture time to five seconds, but a callback exceeding 35 seconds now fails the reflow check instead of waiting indefinitely. + +The total plugin wait budget is 50 seconds when all phases run (15 seconds without an overflow finding). These are not hard wall-clock guarantees: timers need a responsive JavaScript event loop, timed-out Playwright operations are not cancelled, and context/browser teardown is outside these budgets. A blocked controller or browser teardown still requires runtime-level recovery. + +Reflow propagates incomplete-check errors to its caller. Restoration failures retain the original scan error with cleanup context; timeouts and failed restoration mark the page state as unsafe even if a timed-out operation later settles. The current caller logs plugin errors, skips remaining scans on that page, attempts context/browser cleanup, and returns partial findings before continuing to later URLs. + +**Logging and continuing does not guarantee complete scan coverage or exclude partial findings from issue filing.** The separate configurable caller-policy change will offer opt-in abort behavior to stop incomplete plugin work before findings publication, while retaining log-and-continue as the compatibility default. This plugin change does not alter the scanner's existing error-handling policy. + ## How to create plugins ### Default function API (all plugins)