From c2c8019b0edbe0c5d58a174ccfb989fef299215f Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Thu, 15 Oct 2026 20:04:52 +0800 Subject: [PATCH 1/9] feat(core): let the runtime report carry warnings DoneRuntimeVerifier now returns { errors, warnings } instead of a bare error list. Warnings never change the done status, so a false positive cannot start a repair round; they are listed under "Non-blocking warnings" with the existing metadata warnings. When a layout warning is present the stop instruction changes: the agent may fix one it agrees with and check it with preview, but is told not to call done again. The preview result gains an optional layoutIssues list, capped at 20 and never part of ok. done:verify:v1 keeps its errors field and gains warnings and a schemaVersion. The desktop verifier reports no warnings yet. --- apps/desktop/src/main/done-verify.ts | 8 +- .../src/main/done-verify.workspace.test.ts | 9 +- .../desktop/src/main/harness-document.test.ts | 4 +- apps/desktop/src/main/ipc/generate.ts | 6 +- .../ipc/generate.workspace-rename.test.ts | 22 +++- apps/desktop/src/main/ipc/tool-log.test.ts | 2 +- apps/desktop/src/preload/index.ts | 2 + .../core/src/agent.native-lifecycle.test.ts | 12 +- packages/core/src/agent.test.ts | 4 +- packages/core/src/index.ts | 4 + packages/core/src/tools/done.test.ts | 117 +++++++++++++++--- packages/core/src/tools/done.ts | 54 +++++--- packages/core/src/tools/preview.test.ts | 43 +++++++ packages/core/src/tools/preview.ts | 22 ++++ 14 files changed, 258 insertions(+), 51 deletions(-) diff --git a/apps/desktop/src/main/done-verify.ts b/apps/desktop/src/main/done-verify.ts index fa316ce49..3535cf418 100644 --- a/apps/desktop/src/main/done-verify.ts +++ b/apps/desktop/src/main/done-verify.ts @@ -19,7 +19,7 @@ import { mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath, pathToFileURL, URL } from 'node:url'; -import type { DoneError, DoneRuntimeVerifier } from '@open-codesign/core'; +import type { DoneError, DoneRuntimeResult, DoneRuntimeVerifier } from '@open-codesign/core'; import { findSystemChrome } from '@open-codesign/exporters'; import { buildSrcdoc } from '@open-codesign/runtime'; import type { Browser, ConsoleMessage, HTTPRequest, Page } from 'puppeteer-core'; @@ -181,7 +181,7 @@ async function verifyWithSystemChrome( html: string, workspaceRoot?: string, signal?: AbortSignal, -): Promise { +): Promise { signal?.throwIfAborted(); const executablePath = await findSystemChrome(); const puppeteer = (await import('puppeteer-core')).default; @@ -259,11 +259,11 @@ async function verifyWithSystemChrome( await rm(userDataDir, { recursive: true, force: true }); } - return errors; + return { errors, warnings: [] }; } export function makeRuntimeVerifier(options?: { workspaceRoot: string }): DoneRuntimeVerifier { - return async (artifactSource, context): Promise => { + return async (artifactSource, context): Promise => { context?.signal?.throwIfAborted(); const workspaceRoot = options ? resolve(options.workspaceRoot) : undefined; const srcdoc = workspaceRoot diff --git a/apps/desktop/src/main/done-verify.workspace.test.ts b/apps/desktop/src/main/done-verify.workspace.test.ts index 831c82ded..cbd8937b2 100644 --- a/apps/desktop/src/main/done-verify.workspace.test.ts +++ b/apps/desktop/src/main/done-verify.workspace.test.ts @@ -40,7 +40,10 @@ describe.skipIf(!chromeAvailable)('done workspace assets in system Chrome', () = const prefix = path.startsWith('screens/') ? '../' : ''; const source = sourceFor(`${prefix}logo.svg`, `${prefix}art/poster.svg`); await writeFile(join(workspace, path), source); - expect(await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { path })).toEqual([]); + expect(await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { path })).toEqual({ + errors: [], + warnings: [], + }); const preview = await runPreview({ workspaceRoot: workspace, path, vision: false }); expect(preview.ok).toBe(true); expect(preview.assetErrors).toEqual([]); @@ -49,7 +52,7 @@ describe.skipIf(!chromeAvailable)('done workspace assets in system Chrome', () = it('reports a missing relative asset with its URL', async () => { const source = sourceFor('logo.svg', 'missing.svg'); await writeFile(join(workspace, 'App.jsx'), source); - const errors = await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { + const { errors } = await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { path: 'App.jsx', }); expect(errors.some((error) => error.message.includes('missing.svg'))).toBe(true); @@ -60,7 +63,7 @@ describe.skipIf(!chromeAvailable)('done workspace assets in system Chrome', () = await writeFile(outside, svg); const source = sourceFor('logo.svg', pathToFileURL(outside).href); await writeFile(join(workspace, 'App.jsx'), source); - const errors = await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { + const { errors } = await makeRuntimeVerifier({ workspaceRoot: workspace })(source, { path: 'App.jsx', }); expect( diff --git a/apps/desktop/src/main/harness-document.test.ts b/apps/desktop/src/main/harness-document.test.ts index 612719a4e..3cfc69ddf 100644 --- a/apps/desktop/src/main/harness-document.test.ts +++ b/apps/desktop/src/main/harness-document.test.ts @@ -74,12 +74,12 @@ function onlyNavigation() { describe('harness documents for sandboxed system Chrome (#455)', () => { it('serves the done verifier document from memory instead of a shared temp file', async () => { - const errors = await makeRuntimeVerifier()('
ok
', { + const result = await makeRuntimeVerifier()('
ok
', { path: 'index.html', }); const navigation = onlyNavigation(); - expect(errors).toEqual([]); + expect(result).toEqual({ errors: [], warnings: [] }); expect(navigation.url).toMatch(/^file:.*verify\.html$/); expect(navigation.existedOnDisk).toBe(false); expect(navigation.request.continue).not.toHaveBeenCalled(); diff --git a/apps/desktop/src/main/ipc/generate.ts b/apps/desktop/src/main/ipc/generate.ts index dfc71f566..484569733 100644 --- a/apps/desktop/src/main/ipc/generate.ts +++ b/apps/desktop/src/main/ipc/generate.ts @@ -1093,8 +1093,10 @@ export function registerGenerateIpc({ db, getMainWindow }: RegisterGenerateIpcDe ) { throw new CodesignError('done:verify:v1 expects { artifact: string }', 'IPC_BAD_INPUT'); } - const errors = await sharedRuntimeVerifier((raw as { artifact: string }).artifact); - return { errors }; + const { errors, warnings } = await sharedRuntimeVerifier( + (raw as { artifact: string }).artifact, + ); + return { schemaVersion: 1, errors, warnings }; }); ipcMain.handle('codesign:v1:generate', async (_e, raw: unknown) => { diff --git a/apps/desktop/src/main/ipc/generate.workspace-rename.test.ts b/apps/desktop/src/main/ipc/generate.workspace-rename.test.ts index b9b8cc21b..3d4000a9e 100644 --- a/apps/desktop/src/main/ipc/generate.workspace-rename.test.ts +++ b/apps/desktop/src/main/ipc/generate.workspace-rename.test.ts @@ -184,7 +184,7 @@ vi.mock('../memory-ipc', () => ({ })); vi.mock('../done-verify', () => ({ - makeRuntimeVerifier: vi.fn(() => async () => []), + makeRuntimeVerifier: vi.fn(() => async () => ({ errors: [], warnings: [] })), })); vi.mock('../preview-runtime', () => ({ @@ -420,6 +420,26 @@ describe('generate IPC workspace rename coordination', () => { expect(vi.mocked(generateViaAgent)).toHaveBeenCalledOnce(); }); + it('returns verifier errors and warnings from done:verify:v1 with a schema version', async () => { + const error = { message: 'ReferenceError: x is not defined', source: 'pageerror' }; + const warning = { + message: 'The page is 132 px wider than a 390 px viewport', + source: 'layout', + }; + vi.mocked(makeRuntimeVerifier).mockReturnValueOnce(async () => ({ + errors: [error], + warnings: [warning], + })); + registerGenerateIpc({ db: initTestDb(), getMainWindow: () => null }); + + await expect( + getHandler('done:verify:v1')(null, { artifact: 'function App(){return null;}' }), + ).resolves.toEqual({ schemaVersion: 1, errors: [error], warnings: [warning] }); + await expect(getHandler('done:verify:v1')(null, { artifact: 42 })).rejects.toThrow( + 'done:verify:v1 expects { artifact: string }', + ); + }); + it('persists preparation failures and validates recovery cursors', async () => { vi.mocked(preparePromptContext).mockRejectedValueOnce(new Error('Reference unavailable')); const db = initTestDb(); diff --git a/apps/desktop/src/main/ipc/tool-log.test.ts b/apps/desktop/src/main/ipc/tool-log.test.ts index 1d027ea39..4f9610ebe 100644 --- a/apps/desktop/src/main/ipc/tool-log.test.ts +++ b/apps/desktop/src/main/ipc/tool-log.test.ts @@ -84,7 +84,7 @@ describe('done diagnostic durability', () => { it('keeps warnings visible without changing successful completion to failure', () => { const result = { - content: [{ type: 'text', text: 'ok\nNon-blocking design metadata warnings' }], + content: [{ type: 'text', text: 'ok\nNon-blocking warnings' }], details: { status: 'ok', path: 'App.jsx', diff --git a/apps/desktop/src/preload/index.ts b/apps/desktop/src/preload/index.ts index b698674da..ebd90c93c 100644 --- a/apps/desktop/src/preload/index.ts +++ b/apps/desktop/src/preload/index.ts @@ -462,7 +462,9 @@ const api = { ipcRenderer.invoke('codesign:detect-provider', key) as Promise, doneVerify: (artifact: string) => ipcRenderer.invoke('done:verify:v1', { artifact }) as Promise<{ + schemaVersion: 1; errors: Array<{ message: string; source?: string; lineno?: number }>; + warnings: Array<{ message: string; source?: string; lineno?: number }>; }>, generate: (payload: { prompt: string; diff --git a/packages/core/src/agent.native-lifecycle.test.ts b/packages/core/src/agent.native-lifecycle.test.ts index 5790e52be..b89635ef1 100644 --- a/packages/core/src/agent.native-lifecycle.test.ts +++ b/packages/core/src/agent.native-lifecycle.test.ts @@ -222,7 +222,7 @@ describe('production generateViaAgent with installed pi', () => { async (source: string, context?: { path: string; signal?: AbortSignal }) => { expect(context?.signal).toBeInstanceOf(AbortSignal); expect(source).toBe(readFileSync(join(root, 'App.jsx'), 'utf8')); - return []; + return { errors: [], warnings: [] }; }, ); const checks: DoneDetails[] = []; @@ -438,7 +438,10 @@ describe('production generateViaAgent with installed pi', () => { const controller = new AbortController(); const active = new ActiveRunMessages('design', 'run', () => {}, controller.signal); const ended: AgentMessage[][] = []; - const verify = vi.fn(async () => [{ message: 'Broken runtime', source: 'runtime' }]); + const verify = vi.fn(async () => ({ + errors: [{ message: 'Broken runtime', source: 'runtime' }], + warnings: [], + })); script([ [todos()], ...Array.from({ length: 5 }, (_, n) => [ @@ -637,7 +640,10 @@ describe('production generateViaAgent with installed pi', () => { it('leaves repeated done calls to the done repair limit', async () => { seed(); - const verify = vi.fn(async () => [{ message: 'Broken runtime', source: 'runtime' }]); + const verify = vi.fn(async () => ({ + errors: [{ message: 'Broken runtime', source: 'runtime' }], + warnings: [], + })); script([[todos()], ...Array.from({ length: 5 }, () => [done()]), 'Must not complete']); await expect(generateViaAgent(input, { fs, runtimeVerify: verify })).rejects.toMatchObject({ code: 'GENERATION_INCOMPLETE', diff --git a/packages/core/src/agent.test.ts b/packages/core/src/agent.test.ts index 2e5a24c25..4b2201b84 100644 --- a/packages/core/src/agent.test.ts +++ b/packages/core/src/agent.test.ts @@ -1228,8 +1228,8 @@ describe('generateViaAgent()', () => { }; const runtimeVerify = vi .fn() - .mockResolvedValueOnce([{ message: 'Temporary render error' }]) - .mockResolvedValueOnce([]); + .mockResolvedValueOnce({ errors: [{ message: 'Temporary render error' }], warnings: [] }) + .mockResolvedValueOnce({ errors: [], warnings: [] }); const result = await generateViaAgent( { prompt: 'design a meditation app', diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3eeeee862..459b6a70d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -119,7 +119,9 @@ export { export { type DoneDetails, type DoneError, + type DoneRuntimeResult, type DoneRuntimeVerifier, + LAYOUT_WARNING_SOURCE, makeDoneTool, } from './tools/done.js'; export { @@ -137,6 +139,8 @@ export { type WorkspaceInspection, } from './tools/inspect-workspace.js'; export { + type LayoutIssue, + MAX_LAYOUT_ISSUES, makePreviewTool, type PreviewInput, type PreviewResult, diff --git a/packages/core/src/tools/done.test.ts b/packages/core/src/tools/done.test.ts index ab6071b67..8fef03eed 100644 --- a/packages/core/src/tools/done.test.ts +++ b/packages/core/src/tools/done.test.ts @@ -72,7 +72,7 @@ components: `, ), }); - const runtime = vi.fn(async () => []); + const runtime = vi.fn(async () => ({ errors: [], warnings: [] })); const result = await makeDoneTool(fs, runtime).execute('extensions', { path }); expect(result.details.status).toBe('ok'); expect(result.details.errors).toEqual([]); @@ -88,12 +88,84 @@ components: } expect(result.content).toEqual([ expect.objectContaining({ - text: expect.stringContaining('Non-blocking design metadata warnings'), + text: expect.stringContaining('Non-blocking warnings'), }), ]); expect(runtime).toHaveBeenCalledTimes(path === 'App.jsx' ? 1 : 0); }); + describe('runtime warnings', () => { + const layoutWarning = { + source: 'layout', + message: 'The page is 132 px wider than a 390 px viewport (body > main.hero)', + }; + + it('lists layout warnings without failing the run, and lets the agent fix one without calling done again', async () => { + const fs = makeFs({ 'App.jsx': VALID_TASK_APP, 'DESIGN.md': VALID_DESIGN_MD }); + const runtime = vi.fn(async () => ({ errors: [], warnings: [layoutWarning] })); + + const result = await makeDoneTool(fs, runtime).execute('layout', { path: 'App.jsx' }); + + expect(result.details.status).toBe('ok'); + expect(result.details.errors).toEqual([]); + expect(result.details.warnings).toEqual([layoutWarning]); + const text = result.content[0]?.type === 'text' ? result.content[0].text : ''; + expect(text).toContain('Non-blocking warnings:'); + expect(text).toContain(`- layout: ${layoutWarning.message}`); + expect(text).toContain('advice, not errors'); + expect(text).toContain('do not call `done` again'); + expect(text).not.toContain('STOP.'); + }); + + it('keeps the stop instruction when no layout warning is present', async () => { + const fs = makeFs({ + 'App.jsx': VALID_TASK_APP, + 'DESIGN.md': VALID_DESIGN_MD.replace( + '\n---\n', + '\ncomponents:\n card:\n borderRadius: 16px\n---\n', + ), + }); + const runtime = vi.fn(async () => ({ errors: [], warnings: [] })); + + const result = await makeDoneTool(fs, runtime).execute('metadata', { path: 'App.jsx' }); + + const text = result.content[0]?.type === 'text' ? result.content[0].text : ''; + expect(result.details.status).toBe('ok'); + expect(text).toContain('STOP.'); + expect(text).not.toContain('advice, not errors'); + }); + + it('reports layout warnings next to runtime errors', async () => { + const fs = makeFs({ 'App.jsx': VALID_TASK_APP, 'DESIGN.md': VALID_DESIGN_MD }); + const error = { source: 'runtime', message: 'ReferenceError: missingTask is not defined' }; + const runtime = vi.fn(async () => ({ errors: [error], warnings: [layoutWarning] })); + + const result = await makeDoneTool(fs, runtime).execute('both', { path: 'App.jsx' }); + + expect(result.details.status).toBe('has_errors'); + expect(result.details.errors).toEqual([error]); + expect(result.details.warnings).toEqual([layoutWarning]); + const text = result.content[0]?.type === 'text' ? result.content[0].text : ''; + expect(text.startsWith('has_errors')).toBe(true); + expect(text).toContain(`- layout: ${layoutWarning.message}`); + }); + + it('does not report warnings when the verifier failed to run', async () => { + const fs = makeFs({ 'App.jsx': VALID_TASK_APP, 'DESIGN.md': VALID_DESIGN_MD }); + const runtime = vi.fn(async () => { + throw new Error('Chrome is not installed'); + }); + + const result = await makeDoneTool(fs, runtime).execute('failed', { path: 'App.jsx' }); + + expect(result.details.status).toBe('has_errors'); + expect(result.details.errors).toEqual([ + { source: 'runtime', message: 'Runtime verifier failed: Chrome is not installed' }, + ]); + expect(result.details.warnings).toBeUndefined(); + }); + }); + it('does not let metadata warnings hide runtime failures', async () => { const fs = makeFs({ 'App.jsx': VALID_TASK_APP, @@ -102,9 +174,10 @@ components: '\ncomponents:\n card:\n borderRadius: 16px\n---\n', ), }); - const runtime = vi.fn(async () => [ - { source: 'runtime', message: 'ReferenceError: missingTask is not defined' }, - ]); + const runtime = vi.fn(async () => ({ + errors: [{ source: 'runtime', message: 'ReferenceError: missingTask is not defined' }], + warnings: [], + })); const result = await makeDoneTool(fs, runtime).execute('runtime', { path: 'App.jsx' }); expect(result.details.status).toBe('has_errors'); expect(result.details.errors).toEqual([ @@ -275,13 +348,16 @@ ReactDOM.createRoot(document.getElementById('root')).render();`, const fs = makeFs({ 'index.html': '

Hi

', }); - const runtimeVerify = vi.fn(async () => [ - { - message: 'ReferenceError: TWEAK_DEFAULT is not defined', - source: 'console.error', - lineno: 12, - }, - ]); + const runtimeVerify = vi.fn(async () => ({ + errors: [ + { + message: 'ReferenceError: TWEAK_DEFAULT is not defined', + source: 'console.error', + lineno: 12, + }, + ], + warnings: [], + })); const tool = makeDoneTool(fs, runtimeVerify); const res = await tool.execute('id5', { summary: 'shipped' }); expect(runtimeVerify).toHaveBeenCalledOnce(); @@ -298,7 +374,7 @@ ReactDOM.createRoot(document.getElementById('root')).render();`, const fs = makeFs({ 'index.html': '

Hi

', }); - const runtimeVerify = vi.fn(async () => []); + const runtimeVerify = vi.fn(async () => ({ errors: [], warnings: [] })); const tool = makeDoneTool(fs, runtimeVerify); const res = await tool.execute('id6', {}); expect(res.details.status).toBe('ok'); @@ -310,12 +386,15 @@ ReactDOM.createRoot(document.getElementById('root')).render();`, const fs = makeFs({ 'design-brief.md': '# Design brief\n\nA concise handoff document.', }); - const runtimeVerify = vi.fn(async () => [ - { - message: 'This verifier should only run for renderable sources.', - source: 'runtime', - }, - ]); + const runtimeVerify = vi.fn(async () => ({ + errors: [ + { + message: 'This verifier should only run for renderable sources.', + source: 'runtime', + }, + ], + warnings: [], + })); const tool = makeDoneTool(fs, runtimeVerify, { requireDesignMd: true }); const res = await tool.execute('id-markdown-done', { path: 'design-brief.md' }); expect(runtimeVerify).not.toHaveBeenCalled(); diff --git a/packages/core/src/tools/done.ts b/packages/core/src/tools/done.ts index 7e838f0d0..3e12f3cc1 100644 --- a/packages/core/src/tools/done.ts +++ b/packages/core/src/tools/done.ts @@ -26,6 +26,27 @@ const DoneParams = Type.Object({ const DESIGN_MD_ENTRY = 'DESIGN.md'; +/** `source` of the advisory warnings the host's layout checks report. */ +export const LAYOUT_WARNING_SOURCE = 'layout'; + +const STOP_LINES = [ + 'STOP. The design is verified. Your only remaining action is a short', + '2–3 sentence natural-language summary of the design decisions — no', + 'code, no fenced blocks, no `` tags, no file re-emission.', + 'Do NOT call `done` again. Do NOT call any other tool. The host', + 'extracts the artifact from the virtual filesystem automatically;', + "anything else you emit is wasted tokens and pollutes the user's chat.", +]; + +/** Layout warnings are advice, so the agent may fix one it agrees with, but not by looping on `done`. */ +const LAYOUT_ADVICE_LINES = [ + 'The design is verified. The layout warnings below are advice, not errors.', + 'If you agree one is a real defect, fix it with one edit and check it with', + '`preview`; do not call `done` again. Otherwise your only remaining action', + 'is a short 2–3 sentence natural-language summary of the design decisions —', + 'no code, no fenced blocks, no `` tags, no file re-emission.', +]; + export interface DoneError { message: string; source?: string; @@ -117,7 +138,7 @@ function designMdWorkspaceFindings( function formatWarnings(warnings: DoneError[]): string { return warnings.length === 0 ? '' - : `\n\nNon-blocking design metadata warnings:\n${warnings + : `\n\nNon-blocking warnings:\n${warnings .map((warning) => `- ${warning.source}: ${warning.message}`) .join('\n')}`; } @@ -134,13 +155,20 @@ function requiredDesignMdErrors(fs: TextEditorFsCallbacks, activePath: string): ]; } +/** What the host observed when it ran the artifact: console and load `errors` + * that make `done` fail, and advisory `warnings` that never do. */ +export interface DoneRuntimeResult { + errors: DoneError[]; + warnings: DoneError[]; +} + /** Host-injected runtime verifier. Receives the raw artifact source (the - * agent's JSX module, NOT a fully-built srcdoc) and returns any console / - * load errors observed when the host actually executed it. */ + * agent's JSX module, NOT a fully-built srcdoc) and returns what the host + * observed when it actually executed it. */ export type DoneRuntimeVerifier = ( artifactSource: string, context?: { path: string; signal?: AbortSignal }, -) => Promise; +) => Promise; const VOID_ELEMENTS = new Set([ 'area', @@ -528,14 +556,16 @@ export function makeDoneTool( ...(opts.requireDesignMd ? requiredDesignMdErrors(fs, path) : []), ...designFindings.errors, ]; + const warnings: DoneError[] = [...designFindings.warnings]; if (runtimeVerify && isRenderableDesignSourcePath(path)) { try { - const runtimeErrors = await runtimeVerify(file.content, { + const runtime = await runtimeVerify(file.content, { path, ...(signal ? { signal } : {}), }); signal?.throwIfAborted(); - errors.push(...runtimeErrors); + errors.push(...runtime.errors); + warnings.push(...runtime.warnings); } catch (err) { signal?.throwIfAborted(); errors.push({ @@ -549,9 +579,10 @@ export function makeDoneTool( status, path, errors, - ...(designFindings.warnings.length > 0 ? { warnings: designFindings.warnings } : {}), + ...(warnings.length > 0 ? { warnings } : {}), ...(params.summary !== undefined ? { summary: params.summary } : {}), }; + const hasLayoutAdvice = warnings.some((warning) => warning.source === LAYOUT_WARNING_SOURCE); const text = status === 'ok' ? [ @@ -559,16 +590,11 @@ export function makeDoneTool( ? 'ok — no syntactic or runtime issues detected.' : 'ok — no syntactic issues detected. (Runtime verification not configured in this host.)', '', - 'STOP. The design is verified. Your only remaining action is a short', - '2–3 sentence natural-language summary of the design decisions — no', - 'code, no fenced blocks, no `` tags, no file re-emission.', - 'Do NOT call `done` again. Do NOT call any other tool. The host', - 'extracts the artifact from the virtual filesystem automatically;', - "anything else you emit is wasted tokens and pollutes the user's chat.", + ...(hasLayoutAdvice ? LAYOUT_ADVICE_LINES : STOP_LINES), ].join('\n') : `has_errors\n${errors.map((e) => `- ${e.source ? `${e.source}: ` : ''}${e.message}${e.lineno ? ` (line ${e.lineno})` : ''}`).join('\n')}`; return { - content: [{ type: 'text', text: text + formatWarnings(designFindings.warnings) }], + content: [{ type: 'text', text: text + formatWarnings(warnings) }], details, }; }, diff --git a/packages/core/src/tools/preview.test.ts b/packages/core/src/tools/preview.test.ts index 9eaab3264..8c1b646c0 100644 --- a/packages/core/src/tools/preview.test.ts +++ b/packages/core/src/tools/preview.test.ts @@ -7,8 +7,10 @@ import { import { CodesignError } from '@open-codesign/shared'; import { describe, expect, it, vi } from 'vitest'; import { + type LayoutIssue, MAX_ASSET_ERRORS, MAX_CONSOLE_ENTRIES, + MAX_LAYOUT_ISSUES, MAX_PREVIEW_STEPS, makePreviewTool, type PreviewInput, @@ -347,6 +349,47 @@ describe('makePreviewTool', () => { expect(JSON.stringify(result.content)).toContain('isolated browser terminated'); }); + describe('layout issues', () => { + const issue = (index: number): LayoutIssue => ({ + check: 'horizontal-overflow', + viewportWidth: 390, + selector: `body > div:nth-of-type(${index})`, + message: `Element ${index} extends past the right edge`, + measured: 420, + limit: 390, + }); + + it('returns layout issues to the agent without failing the preview', async () => { + const runPreview = vi.fn().mockResolvedValue(cannedResult({ layoutIssues: [issue(1)] })); + + const result = await makePreviewTool(runPreview).execute('call', { path: 'App.jsx' }); + + expect(result.details.ok).toBe(true); + expect(result.details.layoutIssues).toEqual([issue(1)]); + expect(result.content[0]).toMatchObject({ text: expect.stringContaining('preview ok') }); + expect(JSON.stringify(result.content)).toContain('Element 1 extends past the right edge'); + }); + + it('caps layout issues to the documented budget', async () => { + const issues = Array.from({ length: MAX_LAYOUT_ISSUES + 10 }, (_, index) => issue(index)); + const runPreview = vi.fn().mockResolvedValue(cannedResult({ layoutIssues: issues })); + + const result = await makePreviewTool(runPreview).execute('call', { path: 'App.jsx' }); + + expect(MAX_LAYOUT_ISSUES).toBe(20); + expect(result.details.layoutIssues).toEqual(issues.slice(0, MAX_LAYOUT_ISSUES)); + }); + + it('leaves the field out when the host did not measure the layout', async () => { + const runPreview = vi.fn().mockResolvedValue(cannedResult()); + + const result = await makePreviewTool(runPreview).execute('call', { path: 'App.jsx' }); + + expect(result.details).not.toHaveProperty('layoutIssues'); + expect(result.content).toHaveLength(1); + }); + }); + it('propagates select steps and accepts empty and maximum-length option values', async () => { const input: PreviewInput = { path: 'App.jsx', diff --git a/packages/core/src/tools/preview.ts b/packages/core/src/tools/preview.ts index efa2e7c02..51c9e4a38 100644 --- a/packages/core/src/tools/preview.ts +++ b/packages/core/src/tools/preview.ts @@ -123,6 +123,22 @@ export const AssetError = Type.Object({ type: Type.Optional(Type.String()), }); +/** Layout findings are advice: they never change `ok` or the `done` status. */ +export const MAX_LAYOUT_ISSUES = 20; + +export const LayoutIssue = Type.Object({ + check: Type.Union([Type.Literal('horizontal-overflow'), Type.Literal('clipped-text')]), + /** Width of the viewport the page was measured at. */ + viewportWidth: Type.Integer(), + /** A short CSS-style path to the element, for the agent to find it in the source. */ + selector: Type.String(), + message: Type.String(), + /** What was measured, in CSS px, and the limit it was compared with. */ + measured: Type.Number(), + limit: Type.Number(), +}); +export type LayoutIssue = Static; + export const PreviewResult = Type.Object({ ok: Type.Boolean(), /** Set only when capabilities.vision === true. */ @@ -139,6 +155,7 @@ export const PreviewResult = Type.Object({ }), reason: Type.Optional(Type.String()), warnings: Type.Optional(Type.Array(Type.String(), { maxItems: 5 })), + layoutIssues: Type.Optional(Type.Array(LayoutIssue, { maxItems: MAX_LAYOUT_ISSUES })), visibleText: Type.Optional(Type.String()), steps: Type.Optional( Type.Array( @@ -163,6 +180,9 @@ export function trimPreviewResult(result: PreviewResult): PreviewResult { ...result, consoleErrors: result.consoleErrors.slice(0, MAX_CONSOLE_ENTRIES), assetErrors: result.assetErrors.slice(0, MAX_ASSET_ERRORS), + ...(result.layoutIssues !== undefined + ? { layoutIssues: result.layoutIssues.slice(0, MAX_LAYOUT_ISSUES) } + : {}), }; } @@ -170,6 +190,7 @@ function previewContent(summary: string, result: PreviewResult): Array = [{ type: 'text', text: summary }]; if ( result.warnings !== undefined || + result.layoutIssues !== undefined || result.steps !== undefined || result.domOutline !== undefined || result.visibleText !== undefined @@ -179,6 +200,7 @@ function previewContent(summary: string, result: PreviewResult): Array Date: Thu, 15 Oct 2026 20:32:42 +0800 Subject: [PATCH 2/9] feat(desktop): check layout in done and preview After a page loads, preview and done now measure it at its current width and again at a 390 px phone width, then put the viewport back. They look for a page that scrolls sideways, naming the outermost elements that cross the edge, and for text that its box clips. A page that hides horizontal overflow, content inside a scroller, fixed elements, ellipsis and line clamps, collapsed panels and running animations are not reported. The page-side function only collects measurements; the decisions are made on plain data, so the thresholds and the budgets (20 findings, 5,000 elements, 3 elements and 5 clipped texts per width) are tested without a browser. preview returns the findings as layoutIssues and done lists them as warnings. A failed measurement is reported as a warning, a cancelled run still stops. --- .../layout-checks-in-done-and-preview.md | 6 + apps/desktop/src/main/done-verify.ts | 11 +- .../desktop/src/main/harness-document.test.ts | 13 +- .../src/main/layout-probe.browser.test.ts | 224 +++++++++++ apps/desktop/src/main/layout-probe.test.ts | 313 +++++++++++++++ apps/desktop/src/main/layout-probe.ts | 367 ++++++++++++++++++ .../src/main/layout-wiring.browser.test.ts | 79 ++++ .../layout-wiring.failure.browser.test.ts | 111 ++++++ apps/desktop/src/main/preview-runtime.ts | 19 +- packages/core/src/tools/preview.test.ts | 19 + packages/core/src/tools/preview.ts | 7 +- 11 files changed, 1160 insertions(+), 9 deletions(-) create mode 100644 .changeset/layout-checks-in-done-and-preview.md create mode 100644 apps/desktop/src/main/layout-probe.browser.test.ts create mode 100644 apps/desktop/src/main/layout-probe.test.ts create mode 100644 apps/desktop/src/main/layout-probe.ts create mode 100644 apps/desktop/src/main/layout-wiring.browser.test.ts create mode 100644 apps/desktop/src/main/layout-wiring.failure.browser.test.ts diff --git a/.changeset/layout-checks-in-done-and-preview.md b/.changeset/layout-checks-in-done-and-preview.md new file mode 100644 index 000000000..a49450c1d --- /dev/null +++ b/.changeset/layout-checks-in-done-and-preview.md @@ -0,0 +1,6 @@ +--- +"@open-codesign/core": patch +"@open-codesign/desktop": patch +--- + +Check layout in `preview` and `done`. After a page loads, a deterministic measurement looks for a page that scrolls sideways and for text that its box clips, at the current width and again at a 390 px phone width. `preview` lists up to 20 findings in `layoutIssues`; `done` shows them under "Non-blocking warnings", and when one is present it lets the agent fix it and check it with `preview` instead of telling it to stop. Findings never change `ok` or the `done` status, so a false positive cannot cost a repair round. `done:verify:v1` now returns `warnings` and a `schemaVersion` next to `errors`. diff --git a/apps/desktop/src/main/done-verify.ts b/apps/desktop/src/main/done-verify.ts index 3535cf418..e46b2cc0f 100644 --- a/apps/desktop/src/main/done-verify.ts +++ b/apps/desktop/src/main/done-verify.ts @@ -23,6 +23,7 @@ import type { DoneError, DoneRuntimeResult, DoneRuntimeVerifier } from '@open-co import { findSystemChrome } from '@open-codesign/exporters'; import { buildSrcdoc } from '@open-codesign/runtime'; import type { Browser, ConsoleMessage, HTTPRequest, Page } from 'puppeteer-core'; +import { inspectLayout, layoutFailureWarning, layoutWarnings } from './layout-probe'; import { boundedPreview } from './preview-interactions'; import { buildWorkspacePreviewDocument, @@ -187,6 +188,7 @@ async function verifyWithSystemChrome( const puppeteer = (await import('puppeteer-core')).default; const userDataDir = await mkdtemp(join(tmpdir(), 'codesign-done-chrome-')); const errors: DoneError[] = []; + const warnings: DoneError[] = []; const seen = new Set(); let browser: Browser | null = null; let page: Page | null = null; @@ -245,6 +247,13 @@ async function verifyWithSystemChrome( signal, ); await boundedPreview(settleAfterLoad(), SETTLE_AFTER_LOAD_MS + 100, signal); + // Layout findings are advice: a failed measurement is reported, never turned into an error. + try { + warnings.push(...layoutWarnings(await inspectLayout(page, signal))); + } catch (err) { + signal?.throwIfAborted(); + warnings.push(layoutFailureWarning(err)); + } } catch (err) { signal?.throwIfAborted(); pushUniqueError( @@ -259,7 +268,7 @@ async function verifyWithSystemChrome( await rm(userDataDir, { recursive: true, force: true }); } - return { errors, warnings: [] }; + return { errors, warnings }; } export function makeRuntimeVerifier(options?: { workspaceRoot: string }): DoneRuntimeVerifier { diff --git a/apps/desktop/src/main/harness-document.test.ts b/apps/desktop/src/main/harness-document.test.ts index 3cfc69ddf..5bf4afb62 100644 --- a/apps/desktop/src/main/harness-document.test.ts +++ b/apps/desktop/src/main/harness-document.test.ts @@ -26,6 +26,7 @@ vi.mock('puppeteer-core', () => ({ launch: async () => { const handlers = new Map void>(); const page = { + viewport: () => ({ width: 1280, height: 800 }), setViewport: async () => {}, setRequestInterception: async () => {}, evaluateOnNewDocument: async () => {}, @@ -47,7 +48,17 @@ vi.mock('puppeteer-core', () => ({ handlers.get('request')?.(request); await vi.waitFor(() => expect(request.respond).toHaveBeenCalled()); }, - evaluate: async () => ({ nodes: 1, width: 1280, height: 800 }), + evaluate: async (fn: { name: string }) => + fn.name === 'collectLayoutSamples' + ? { + viewportWidth: 1280, + documentWidth: 1280, + rootClipsX: false, + overflowing: [], + clipped: [], + truncated: false, + } + : { nodes: 1, width: 1280, height: 800 }, close: async () => {}, }; return { newPage: async () => page, close: async () => {} }; diff --git a/apps/desktop/src/main/layout-probe.browser.test.ts b/apps/desktop/src/main/layout-probe.browser.test.ts new file mode 100644 index 000000000..3a088f9a5 --- /dev/null +++ b/apps/desktop/src/main/layout-probe.browser.test.ts @@ -0,0 +1,224 @@ +import { randomUUID } from 'node:crypto'; +import { mkdirSync, rmSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { findSystemChrome } from '@open-codesign/exporters'; +import puppeteer, { type Browser, type Page } from 'puppeteer-core'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { inspectLayout, MAX_PROBE_ELEMENTS, PHONE_VIEWPORT } from './layout-probe'; + +const chrome = await findSystemChrome().catch(() => null); +const DESKTOP = { width: 1280, height: 800 }; +const LONG_TEXT = 'Long text that wraps over several lines and is cut off by its box. '.repeat(6); + +describe.skipIf(!chrome)('layout checks in system Chrome', () => { + let browser: Browser; + let page: Page; + let profile: string; + + beforeAll(async () => { + if (!chrome) throw new Error('System Chrome unavailable'); + profile = resolve(`.layout-probe-browser-${randomUUID()}`); + mkdirSync(profile); + browser = await puppeteer.launch({ + executablePath: chrome, + headless: true, + userDataDir: profile, + args: ['--disable-background-networking'], + }); + }, 30_000); + + afterAll(async () => { + try { + await browser?.close(); + } finally { + if (profile) rmSync(profile, { recursive: true, force: true }); + } + }, 30_000); + + beforeEach(async () => { + page = await browser.newPage(); + await page.setViewport(DESKTOP); + await page.setRequestInterception(true); + page.on('request', (request) => void request.abort()); + }); + + afterEach(async () => { + await page?.close(); + }); + + const mount = (body: string, head = '') => + page.setContent( + `${body}`, + ); + + describe('horizontal overflow', () => { + it('names the element that scrolls the page sideways, at phone width only', async () => { + await mount( + '
PlanPrice
', + ); + + const { issues, notes } = await inspectLayout(page); + + expect(notes).toEqual([]); + expect(issues).toHaveLength(1); + expect(issues[0]).toMatchObject({ + check: 'horizontal-overflow', + viewportWidth: PHONE_VIEWPORT.width, + selector: 'table#prices', + measured: 800, + limit: PHONE_VIEWPORT.width, + }); + expect(issues[0]?.message).toContain('table#prices reaches 800 px'); + }); + + it('puts the viewport back after measuring', async () => { + await mount('
Hello
'); + + await inspectLayout(page); + + expect(page.viewport()).toMatchObject(DESKTOP); + }); + + it('reports nothing for a page that reflows to fit', async () => { + await mount( + '
One
Two
Three
', + '.row{display:flex;flex-wrap:wrap}.card{width:200px;height:80px}', + ); + + expect((await inspectLayout(page)).issues).toEqual([]); + }); + + it('does not blame content inside a container that scrolls on its own', async () => { + await mount( + '
Wide strip
', + ); + + expect((await inspectLayout(page)).issues).toEqual([]); + }); + + it('names the element that really overflows, not a wider one inside a scroller', async () => { + await mount( + `
Wide strip
+ `, + ); + + const { issues } = await inspectLayout(page); + + expect(issues.map((issue) => issue.selector)).toEqual(['div#banner']); + }); + + it('does not report overflow the page hides from the user', async () => { + await mount('
Wide
', 'body{overflow-x:hidden}'); + + expect((await inspectLayout(page)).issues).toEqual([]); + }); + + it('ignores a fixed-position element, which cannot scroll the page', async () => { + await mount( + `
+ `, + ); + + const { issues } = await inspectLayout(page); + + expect(issues.map((issue) => issue.selector)).toEqual(['div#banner']); + }); + + it('names at most three elements and keeps the widest first', async () => { + const boxes = [500, 900, 600, 700, 800] + .map((width) => `
`) + .join(''); + await mount(`
${boxes}
`); + + const { issues } = await inspectLayout(page); + + expect(issues.map((issue) => issue.measured)).toEqual([900, 800, 700]); + }); + }); + + describe('clipped text', () => { + const clippedAt = async (markup: string, head = '') => { + await mount(markup, head); + return (await inspectLayout(page)).issues.filter((issue) => issue.check === 'clipped-text'); + }; + + it('reports text that a fixed-height box cuts off, with a path to the element', async () => { + const issues = await clippedAt( + `
${LONG_TEXT}
`, + ); + + expect(issues.length).toBeGreaterThan(0); + expect(issues[0]).toMatchObject({ + check: 'clipped-text', + viewportWidth: DESKTOP.width, + selector: 'section > div.note.highlighted', + }); + expect(issues[0]?.measured).toBeGreaterThan(issues[0]?.limit ?? Number.POSITIVE_INFINITY); + }); + + it('ends the path at an element with an id and otherwise keeps three levels', async () => { + const issues = await clippedAt( + `

${LONG_TEXT}

`, + ); + + expect(issues[0]?.selector).toBe('p#deep'); + const unnamed = await clippedAt( + `

${LONG_TEXT}

`, + ); + expect(unnamed[0]?.selector).toBe('… > div > div > p'); + }); + + it('leaves text alone that fits its box', async () => { + expect( + await clippedAt('
Short
'), + ).toEqual([]); + }); + + it.each([ + [ + 'an ellipsis', + `
${LONG_TEXT}
`, + ], + [ + 'a line clamp', + `

${LONG_TEXT}

`, + ], + ['a collapsed panel', '
Answer behind a toggle
'], + [ + 'a running animation', + `
${LONG_TEXT}
`, + ], + ])('treats %s as intended', async (_label, markup) => { + expect( + await clippedAt( + markup, + '@keyframes slide{from{transform:translateX(0)}to{transform:translateX(-100px)}}', + ), + ).toEqual([]); + }); + + it('names at most five clipped texts per viewport', async () => { + const boxes = Array.from( + { length: 8 }, + (_, index) => + `
${LONG_TEXT}
`, + ).join(''); + + const issues = await clippedAt(boxes); + + // Five at each of the two widths. + expect(issues).toHaveLength(10); + expect(issues.filter((issue) => issue.viewportWidth === DESKTOP.width)).toHaveLength(5); + }); + }); + + it('says so when the page has more elements than it measures', async () => { + await mount(Array.from({ length: MAX_PROBE_ELEMENTS + 100 }, () => '').join('')); + + const { notes } = await inspectLayout(page); + + expect(notes).toEqual([ + `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, + ]); + }); +}); diff --git a/apps/desktop/src/main/layout-probe.test.ts b/apps/desktop/src/main/layout-probe.test.ts new file mode 100644 index 000000000..1c48c82af --- /dev/null +++ b/apps/desktop/src/main/layout-probe.test.ts @@ -0,0 +1,313 @@ +import { LAYOUT_WARNING_SOURCE, type LayoutIssue, MAX_LAYOUT_ISSUES } from '@open-codesign/core'; +import type { Page } from 'puppeteer-core'; +import { describe, expect, it } from 'vitest'; +import { + analyzeLayoutSamples, + collectLayoutSamples, + inspectLayout, + LAYOUT_PROBE_LIMITS, + type LayoutSamples, + layoutFailureWarning, + layoutWarnings, + MAX_PROBE_ELEMENTS, + OVERFLOW_TOLERANCE_PX, + PHONE_VIEWPORT, +} from './layout-probe'; + +function samples(overrides: Partial = {}): LayoutSamples { + return { + viewportWidth: 390, + documentWidth: 390, + rootClipsX: false, + overflowing: [], + clipped: [], + truncated: false, + ...overrides, + }; +} + +describe('analyzeLayoutSamples', () => { + it('reports a page that fits, or is off by less than the tolerance, as clean', () => { + expect(analyzeLayoutSamples(samples())).toEqual([]); + expect(analyzeLayoutSamples(samples({ documentWidth: 390 + OVERFLOW_TOLERANCE_PX }))).toEqual( + [], + ); + }); + + it('names each element that makes the page scroll sideways, with the numbers', () => { + const issues = analyzeLayoutSamples( + samples({ + documentWidth: 1412, + overflowing: [ + { selector: 'table#prices', right: 1412 }, + { selector: 'main > div.hero', right: 960 }, + ], + }), + ); + + expect(issues).toEqual([ + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'table#prices', + message: + 'table#prices reaches 1,412 px, past the 390 px viewport edge, so the page scrolls sideways.', + measured: 1412, + limit: 390, + }, + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'main > div.hero', + message: + 'main > div.hero reaches 960 px, past the 390 px viewport edge, so the page scrolls sideways.', + measured: 960, + limit: 390, + }, + ]); + }); + + it('still reports the overflow when no single element was identified', () => { + expect(analyzeLayoutSamples(samples({ documentWidth: 520 }))).toEqual([ + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'html', + message: + 'The page is 520 px wide at a 390 px viewport, so it scrolls sideways; no single element was identified.', + measured: 520, + limit: 390, + }, + ]); + }); + + it('does not report overflow the page hides, because it cannot scroll sideways', () => { + expect( + analyzeLayoutSamples( + samples({ + documentWidth: 900, + rootClipsX: true, + overflowing: [{ selector: 'div.wide', right: 900 }], + }), + ), + ).toEqual([]); + }); + + it('reports text a box clips on the side that sticks out', () => { + const issues = analyzeLayoutSamples( + samples({ + viewportWidth: 1280, + documentWidth: 1280, + clipped: [ + { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, + { selector: 'p.teaser', textRight: 200, boxRight: 300, textBottom: 190, boxBottom: 120 }, + ], + }), + ); + + expect(issues).toEqual([ + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'div#card', + message: + 'div#card hides part of its text: the text reaches 340 px but the box ends at 300 px (viewport 1,280 px).', + measured: 340, + limit: 300, + }, + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'p.teaser', + message: + 'p.teaser hides part of its text: the text reaches 190 px from the top but the box ends at 120 px (viewport 1,280 px).', + measured: 190, + limit: 120, + }, + ]); + }); + + it('lists overflow before clipped text, and reports clipped text on a page that hides overflow', () => { + const issues = analyzeLayoutSamples( + samples({ + documentWidth: 600, + overflowing: [{ selector: 'div.wide', right: 600 }], + clipped: [ + { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, + ], + }), + ); + expect(issues.map((issue) => issue.check)).toEqual(['horizontal-overflow', 'clipped-text']); + + const hidden = analyzeLayoutSamples( + samples({ + documentWidth: 600, + rootClipsX: true, + clipped: [ + { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, + ], + }), + ); + expect(hidden.map((issue) => issue.check)).toEqual(['clipped-text']); + }); +}); + +describe('layout warnings', () => { + it('shows each finding and note as a layout warning', () => { + const issue: LayoutIssue = { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'html', + message: 'The page scrolls sideways.', + measured: 520, + limit: 390, + }; + + expect(layoutWarnings({ issues: [issue], notes: ['Stopped after 5,000 elements.'] })).toEqual([ + { source: LAYOUT_WARNING_SOURCE, message: 'The page scrolls sideways.' }, + { source: LAYOUT_WARNING_SOURCE, message: 'Stopped after 5,000 elements.' }, + ]); + expect(layoutWarnings({ issues: [], notes: [] })).toEqual([]); + }); + + it('says why the checks did not run', () => { + expect(layoutFailureWarning(new Error('Preview operation timed out'))).toEqual({ + source: LAYOUT_WARNING_SOURCE, + message: 'Layout checks did not run: Preview operation timed out', + }); + expect(layoutFailureWarning('boom').message).toBe('Layout checks did not run: boom'); + }); +}); + +interface FakePage { + page: Page; + viewports: Array<{ width: number; height: number }>; +} + +/** A page whose measurements come from a script, one per call of the collector. */ +function fakePage( + original: { width: number; height: number } | null, + measurements: Array, +): FakePage { + const viewports: Array<{ width: number; height: number }> = []; + const queue = [...measurements]; + const page = { + viewport: () => original, + setViewport: async (viewport: { width: number; height: number }) => { + viewports.push(viewport); + }, + evaluate: async (fn: unknown) => { + if (fn !== collectLayoutSamples) return undefined; + const next = queue.shift(); + if (next === undefined) throw new Error('Unexpected extra measurement'); + if (next instanceof Error) throw next; + return next; + }, + }; + return { page: page as unknown as Page, viewports }; +} + +const desktop = { width: 1280, height: 800 }; +const overflowAtPhone = samples({ + documentWidth: 700, + overflowing: [{ selector: 'table#prices', right: 700 }], +}); + +describe('inspectLayout', () => { + it('measures at the current width and at phone width, then restores the viewport', async () => { + const { page, viewports } = fakePage(desktop, [ + samples({ viewportWidth: 1280, documentWidth: 1280 }), + overflowAtPhone, + ]); + + const inspection = await inspectLayout(page); + + expect(inspection.issues.map((issue) => [issue.check, issue.viewportWidth])).toEqual([ + ['horizontal-overflow', 390], + ]); + expect(inspection.notes).toEqual([]); + expect(viewports).toEqual([{ ...desktop, ...PHONE_VIEWPORT }, desktop]); + }); + + it('does not resize a page that is already phone width', async () => { + const { page, viewports } = fakePage({ width: PHONE_VIEWPORT.width, height: 700 }, [ + overflowAtPhone, + ]); + + const inspection = await inspectLayout(page); + + expect(inspection.issues).toHaveLength(1); + expect(viewports).toEqual([]); + }); + + it('restores the viewport and rejects when the phone measurement fails', async () => { + const { page, viewports } = fakePage(desktop, [ + samples({ viewportWidth: 1280, documentWidth: 1280 }), + new Error('Execution context was destroyed'), + ]); + + await expect(inspectLayout(page)).rejects.toThrow('Execution context was destroyed'); + + expect(viewports.at(-1)).toEqual(desktop); + }); + + it('rejects when the page has no viewport to measure', async () => { + const { page } = fakePage(null, []); + + await expect(inspectLayout(page)).rejects.toThrow('The page has no viewport to measure'); + }); + + it('keeps the first findings and says so when there are more than the budget', async () => { + const clipped = Array.from({ length: MAX_LAYOUT_ISSUES }, (_, index) => ({ + selector: `p.item-${index}`, + textRight: 400, + boxRight: 300, + textBottom: 20, + boxBottom: 40, + })); + const { page } = fakePage(desktop, [ + samples({ viewportWidth: 1280, documentWidth: 1280, clipped }), + overflowAtPhone, + ]); + + const inspection = await inspectLayout(page); + + expect(inspection.issues).toHaveLength(MAX_LAYOUT_ISSUES); + expect(inspection.issues.every((issue) => issue.check === 'clipped-text')).toBe(true); + expect(inspection.notes).toEqual([ + `Layout checks found ${MAX_LAYOUT_ISSUES + 1} issues; only the first ${MAX_LAYOUT_ISSUES} are listed.`, + ]); + }); + + it('says when the page was only measured in part', async () => { + const { page } = fakePage(desktop, [ + samples({ viewportWidth: 1280, documentWidth: 1280, truncated: true }), + samples(), + ]); + + const inspection = await inspectLayout(page); + + expect(inspection.issues).toEqual([]); + expect(inspection.notes).toEqual([ + `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, + ]); + }); + + it('stops when the run is cancelled', async () => { + const controller = new AbortController(); + controller.abort(); + const { page } = fakePage(desktop, [samples(), samples()]); + + await expect(inspectLayout(page, controller.signal)).rejects.toThrow('Preview cancelled'); + }); + + it('passes the documented limits to the page', () => { + expect(LAYOUT_PROBE_LIMITS).toEqual({ + maxElements: 5000, + overflowTolerance: 1, + clipTolerance: 2, + maxOverflowElements: 3, + maxClippedTexts: 5, + }); + }); +}); diff --git a/apps/desktop/src/main/layout-probe.ts b/apps/desktop/src/main/layout-probe.ts new file mode 100644 index 000000000..e64bcc831 --- /dev/null +++ b/apps/desktop/src/main/layout-probe.ts @@ -0,0 +1,367 @@ +/** + * Deterministic layout checks for the pages `preview` and `done` already open + * in Chrome. The page-side function only collects raw measurements; the + * decisions (what counts as overflow, how many findings to keep) are made here + * on plain data so they can be tested without a browser. + * + * Findings are advice. They never change `ok` or the `done` status, because a + * heuristic can be wrong and a false positive must not cost a repair round. + */ + +import { + type DoneError, + LAYOUT_WARNING_SOURCE, + type LayoutIssue, + MAX_LAYOUT_ISSUES, +} from '@open-codesign/core'; +import type { Page } from 'puppeteer-core'; +import { boundedPreview } from './preview-interactions'; + +// collectLayoutSamples is serialized into the page. Node's tsconfig has no DOM +// types, so these describe only the browser globals it touches. +interface PageStyle { + display: string; + position: string; + overflowX: string; + overflowY: string; + textOverflow: string; + webkitLineClamp?: string; + animationName: string; +} +interface PageRect { + left: number; + right: number; + top: number; + bottom: number; + width: number; + height: number; +} +interface PageNode { + nodeType: number; + textContent: string | null; +} +interface PageElement extends PageNode { + tagName: string; + id: string; + classList: ArrayLike; + parentElement: PageElement | null; + children: ArrayLike; + childNodes: ArrayLike; + getBoundingClientRect(): PageRect; + querySelectorAll(selector: string): ArrayLike; +} +declare const document: { + documentElement: PageElement & { clientWidth: number; scrollWidth: number }; + body: PageElement | null; + createRange(): { selectNodeContents(node: PageNode): void; getBoundingClientRect(): PageRect }; +}; +declare const Node: { TEXT_NODE: number }; +declare function getComputedStyle(element: PageElement): PageStyle; +declare function requestAnimationFrame(callback: () => void): number; + +/** The narrow viewport every page is also measured at. */ +export const PHONE_VIEWPORT = { width: 390, height: 844 } as const; +/** Most elements one measurement visits; a larger page is measured only in part. */ +export const MAX_PROBE_ELEMENTS = 5000; +/** Sub-pixel rounding is not overflow. */ +export const OVERFLOW_TOLERANCE_PX = 1; +/** Text may stick out of a clipping box by this much before it counts as clipped. */ +export const CLIP_TOLERANCE_PX = 2; +/** Elements named per viewport when the page scrolls sideways. */ +export const MAX_OVERFLOW_ELEMENTS = 3; +/** Clipped texts named per viewport. */ +export const MAX_CLIPPED_TEXTS = 5; +/** Time allowed for both measurements together. */ +export const LAYOUT_BUDGET_MS = 5000; + +export interface OverflowingElement { + selector: string; + /** Right edge of the element, in CSS px from the left of the viewport. */ + right: number; +} + +export interface ClippedText { + selector: string; + textRight: number; + boxRight: number; + textBottom: number; + boxBottom: number; +} + +/** What the page-side function returns. Plain data, so it survives serialization. */ +export interface LayoutSamples { + viewportWidth: number; + documentWidth: number; + /** The page cannot scroll sideways, because the root or body hides horizontal overflow. */ + rootClipsX: boolean; + overflowing: OverflowingElement[]; + clipped: ClippedText[]; + /** The element limit was reached, so the rest of the page was not measured. */ + truncated: boolean; +} + +export interface LayoutProbeLimits { + maxElements: number; + overflowTolerance: number; + clipTolerance: number; + maxOverflowElements: number; + maxClippedTexts: number; +} + +export const LAYOUT_PROBE_LIMITS: LayoutProbeLimits = { + maxElements: MAX_PROBE_ELEMENTS, + overflowTolerance: OVERFLOW_TOLERANCE_PX, + clipTolerance: CLIP_TOLERANCE_PX, + maxOverflowElements: MAX_OVERFLOW_ELEMENTS, + maxClippedTexts: MAX_CLIPPED_TEXTS, +}; + +/** + * Runs inside the page: plain DOM, `getComputedStyle` and `getBoundingClientRect`. + * It must stay self-contained, because Puppeteer serializes it to source. + */ +export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { + const root = document.documentElement; + const body = document.body; + const clipsX = (value: string) => value === 'hidden' || value === 'clip'; + const rootOverflowX = getComputedStyle(root).overflowX; + const bodyOverflowX = body ? getComputedStyle(body).overflowX : 'visible'; + const rootClipsX = + clipsX(rootOverflowX) || (rootOverflowX === 'visible' && clipsX(bodyOverflowX)); + const viewportWidth = root.clientWidth; + const documentWidth = root.scrollWidth; + const samples: LayoutSamples = { + viewportWidth, + documentWidth, + rootClipsX, + overflowing: [], + clipped: [], + truncated: false, + }; + if (!body) return samples; + + const selectorOf = (element: PageElement): string => { + const parts: string[] = []; + let node: PageElement | null = element; + while (node && node !== body && parts.length < 3) { + let part = node.tagName.toLowerCase(); + if (node.id) { + parts.unshift(`${part}#${node.id}`); + node = null; + break; + } + const classes = Array.from(node.classList).slice(0, 2); + if (classes.length > 0) part += `.${classes.join('.')}`; + parts.unshift(part); + node = node.parentElement; + } + const path = parts.join(' > '); + return (node === body || node === null ? path : `… > ${path}`).slice(0, 120); + }; + + const sideways = documentWidth - viewportWidth > limits.overflowTolerance && !rootClipsX; + const extending = new Set(); + const elements = body.querySelectorAll('*'); + const visited = Math.min(elements.length, limits.maxElements); + samples.truncated = elements.length > limits.maxElements; + + const clippedByAncestor = (element: PageElement): boolean => { + for (let node = element.parentElement; node && node !== body; node = node.parentElement) { + if (getComputedStyle(node).overflowX !== 'visible') return true; + } + return false; + }; + + for (let index = 0; index < visited; index += 1) { + const element = elements[index]; + if (!element) continue; + const style = getComputedStyle(element); + if (style.display === 'none') continue; + const box = element.getBoundingClientRect(); + + if (sideways && style.position !== 'fixed' && box.width > 0) { + if (box.right > viewportWidth + limits.overflowTolerance && !clippedByAncestor(element)) { + extending.add(element); + } + } + + const clipsText = clipsX(style.overflowX) || clipsX(style.overflowY); + const intentional = + style.textOverflow === 'ellipsis' || + (style.webkitLineClamp !== undefined && style.webkitLineClamp !== 'none') || + style.animationName !== 'none'; + if ( + !clipsText || + intentional || + box.width < 1 || + box.height < 1 || + samples.clipped.length >= limits.maxClippedTexts + ) { + continue; + } + for (const child of Array.from(element.childNodes)) { + if (child.nodeType !== Node.TEXT_NODE || !(child.textContent ?? '').trim()) continue; + const range = document.createRange(); + range.selectNodeContents(child); + const text = range.getBoundingClientRect(); + if (text.width < 1 || text.height < 1) continue; + const tolerance = limits.clipTolerance; + if ( + text.right > box.right + tolerance || + text.bottom > box.bottom + tolerance || + text.left < box.left - tolerance || + text.top < box.top - tolerance + ) { + samples.clipped.push({ + selector: selectorOf(element), + textRight: Math.round(text.right), + boxRight: Math.round(box.right), + textBottom: Math.round(text.bottom), + boxBottom: Math.round(box.bottom), + }); + break; + } + } + } + + // Name the outermost elements that cross the edge: along each chain, the first box + // that goes past it is where the overflow starts. + const outermost: OverflowingElement[] = []; + for (const element of extending) { + if (element.parentElement && extending.has(element.parentElement)) continue; + outermost.push({ + selector: selectorOf(element), + right: Math.round(element.getBoundingClientRect().right), + }); + } + outermost.sort((a, b) => b.right - a.right); + samples.overflowing = outermost.slice(0, limits.maxOverflowElements); + return samples; +} + +const formatPx = (value: number) => `${value.toLocaleString('en-US')} px`; + +/** Turns one measurement into findings. Pure, so the thresholds are testable without Chrome. */ +export function analyzeLayoutSamples( + samples: LayoutSamples, + limits: Pick = LAYOUT_PROBE_LIMITS, +): LayoutIssue[] { + const issues: LayoutIssue[] = []; + const { viewportWidth, documentWidth } = samples; + + if (!samples.rootClipsX && documentWidth - viewportWidth > limits.overflowTolerance) { + if (samples.overflowing.length === 0) { + issues.push({ + check: 'horizontal-overflow', + viewportWidth, + selector: 'html', + message: `The page is ${formatPx(documentWidth)} wide at a ${formatPx(viewportWidth)} viewport, so it scrolls sideways; no single element was identified.`, + measured: documentWidth, + limit: viewportWidth, + }); + } + for (const element of samples.overflowing) { + issues.push({ + check: 'horizontal-overflow', + viewportWidth, + selector: element.selector, + message: `${element.selector} reaches ${formatPx(element.right)}, past the ${formatPx(viewportWidth)} viewport edge, so the page scrolls sideways.`, + measured: element.right, + limit: viewportWidth, + }); + } + } + + for (const text of samples.clipped) { + const horizontal = text.textRight > text.boxRight; + issues.push({ + check: 'clipped-text', + viewportWidth, + selector: text.selector, + message: horizontal + ? `${text.selector} hides part of its text: the text reaches ${formatPx(text.textRight)} but the box ends at ${formatPx(text.boxRight)} (viewport ${formatPx(viewportWidth)}).` + : `${text.selector} hides part of its text: the text reaches ${formatPx(text.textBottom)} from the top but the box ends at ${formatPx(text.boxBottom)} (viewport ${formatPx(viewportWidth)}).`, + measured: horizontal ? text.textRight : text.textBottom, + limit: horizontal ? text.boxRight : text.boxBottom, + }); + } + return issues; +} + +export interface LayoutInspection { + issues: LayoutIssue[]; + /** Things the agent should know about the measurement itself, such as a cap that was hit. */ + notes: string[]; +} + +/** Waits two frames so a resized page has laid out again. */ +async function settleLayout(page: Page): Promise { + await page.evaluate( + () => + new Promise((resolve) => { + requestAnimationFrame(() => requestAnimationFrame(() => resolve())); + }), + ); +} + +async function measure(page: Page): Promise { + return page.evaluate(collectLayoutSamples, LAYOUT_PROBE_LIMITS); +} + +/** + * Measures the open page at its current viewport and at phone width, then puts + * the viewport back. A failure rejects, so the caller reports it instead of + * silently reporting a clean page. + */ +export async function inspectLayout(page: Page, signal?: AbortSignal): Promise { + return boundedPreview(runInspection(page), LAYOUT_BUDGET_MS, signal); +} + +async function runInspection(page: Page): Promise { + const original = page.viewport(); + if (original === null) throw new Error('The page has no viewport to measure'); + const widths = original.width === PHONE_VIEWPORT.width ? [] : [PHONE_VIEWPORT]; + const issues: LayoutIssue[] = []; + let truncated = false; + const take = (samples: LayoutSamples) => { + issues.push(...analyzeLayoutSamples(samples)); + truncated ||= samples.truncated; + }; + + take(await measure(page)); + try { + for (const viewport of widths) { + await page.setViewport({ ...original, ...viewport }); + await settleLayout(page); + take(await measure(page)); + } + } finally { + if (widths.length > 0) await page.setViewport(original); + } + + const notes: string[] = []; + if (truncated) { + notes.push( + `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, + ); + } + if (issues.length > MAX_LAYOUT_ISSUES) { + notes.push( + `Layout checks found ${issues.length} issues; only the first ${MAX_LAYOUT_ISSUES} are listed.`, + ); + } + return { issues: issues.slice(0, MAX_LAYOUT_ISSUES), notes }; +} + +/** What `done` shows for a measurement: each finding and note as a non-blocking warning. */ +export function layoutWarnings(inspection: LayoutInspection): DoneError[] { + return [...inspection.issues.map((issue) => issue.message), ...inspection.notes].map( + (message) => ({ message, source: LAYOUT_WARNING_SOURCE }), + ); +} + +/** What `done` shows when the measurement itself failed. */ +export function layoutFailureWarning(error: unknown): DoneError { + const reason = error instanceof Error ? error.message : String(error); + return { message: `Layout checks did not run: ${reason}`, source: LAYOUT_WARNING_SOURCE }; +} diff --git a/apps/desktop/src/main/layout-wiring.browser.test.ts b/apps/desktop/src/main/layout-wiring.browser.test.ts new file mode 100644 index 000000000..82d3ec0aa --- /dev/null +++ b/apps/desktop/src/main/layout-wiring.browser.test.ts @@ -0,0 +1,79 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { LAYOUT_WARNING_SOURCE, trimPreviewResult } from '@open-codesign/core'; +import { findSystemChrome } from '@open-codesign/exporters'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { makeRuntimeVerifier } from './done-verify'; +import { PHONE_VIEWPORT } from './layout-probe'; +import { runPreview } from './preview-runtime'; + +vi.mock('./logger', () => ({ getLogger: () => ({ warn: vi.fn(), error: vi.fn() }) })); + +const chromeAvailable = await findSystemChrome().then( + () => true, + () => false, +); + +const jsx = (style: string) => + `function App(){return
Quarterly report
;} ReactDOM.createRoot(document.getElementById('root')).render();`; +const FITS = jsx('{ padding: 16 }'); +const TOO_WIDE = jsx('{ width: 900, padding: 16 }'); + +describe.skipIf(!chromeAvailable)('layout checks reach done and preview', () => { + let workspace: string; + beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), 'codesign-layout-wiring-')); + }); + afterEach(async () => { + await rm(workspace, { recursive: true, force: true }); + }); + + it('done lists a page that scrolls sideways at phone width as a warning, not an error', async () => { + const { errors, warnings } = await makeRuntimeVerifier({ workspaceRoot: workspace })(TOO_WIDE, { + path: 'App.jsx', + }); + + expect(errors).toEqual([]); + expect(warnings).toHaveLength(1); + expect(warnings[0]?.source).toBe(LAYOUT_WARNING_SOURCE); + expect(warnings[0]?.message).toMatch( + new RegExp(`past the ${PHONE_VIEWPORT.width} px viewport edge`), + ); + }, 60_000); + + it('done reports no warning for a page that fits at both widths', async () => { + const result = await makeRuntimeVerifier({ workspaceRoot: workspace })(FITS, { + path: 'App.jsx', + }); + + expect(result).toEqual({ errors: [], warnings: [] }); + }, 60_000); + + it('preview returns the same finding as layoutIssues and still reports ok', async () => { + await writeFile(join(workspace, 'App.jsx'), TOO_WIDE); + + const preview = await runPreview({ workspaceRoot: workspace, path: 'App.jsx', vision: false }); + + expect(preview.ok).toBe(true); + expect(preview.layoutIssues).toHaveLength(1); + expect(preview.layoutIssues?.[0]).toMatchObject({ + check: 'horizontal-overflow', + viewportWidth: PHONE_VIEWPORT.width, + }); + expect(trimPreviewResult(preview).layoutIssues).toEqual(preview.layoutIssues); + }, 60_000); + + it('preview leaves layoutIssues out for a page that fits, and measures a vision preview too', async () => { + await writeFile(join(workspace, 'App.jsx'), FITS); + + const text = await runPreview({ workspaceRoot: workspace, path: 'App.jsx', vision: false }); + const vision = await runPreview({ workspaceRoot: workspace, path: 'App.jsx', vision: true }); + + expect(text.layoutIssues).toBeUndefined(); + expect(text.warnings).toBeUndefined(); + expect(vision.layoutIssues).toBeUndefined(); + expect(vision.screenshot).toMatch(/^data:image\/png;base64,/); + expect(vision.metrics.width).toBe(1280); + }, 60_000); +}); diff --git a/apps/desktop/src/main/layout-wiring.failure.browser.test.ts b/apps/desktop/src/main/layout-wiring.failure.browser.test.ts new file mode 100644 index 000000000..00cacbac0 --- /dev/null +++ b/apps/desktop/src/main/layout-wiring.failure.browser.test.ts @@ -0,0 +1,111 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { LAYOUT_WARNING_SOURCE } from '@open-codesign/core'; +import { findSystemChrome } from '@open-codesign/exporters'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { makeRuntimeVerifier } from './done-verify'; +import { inspectLayout } from './layout-probe'; +import { runPreview } from './preview-runtime'; + +vi.mock('./logger', () => ({ getLogger: () => ({ warn: vi.fn(), error: vi.fn() }) })); +vi.mock('./layout-probe', async (importOriginal) => ({ + ...(await importOriginal()), + inspectLayout: vi.fn(), +})); + +const chromeAvailable = await findSystemChrome().then( + () => true, + () => false, +); +const SOURCE = `function App(){return
Hello
;} ReactDOM.createRoot(document.getElementById('root')).render();`; + +describe.skipIf(!chromeAvailable)('a layout measurement that fails or is cancelled', () => { + let workspace: string; + beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), 'codesign-layout-failure-')); + await writeFile(join(workspace, 'App.jsx'), SOURCE); + }); + afterEach(async () => { + vi.mocked(inspectLayout).mockReset(); + await rm(workspace, { recursive: true, force: true }); + }); + + it('is reported by done as a warning and does not fail the run', async () => { + vi.mocked(inspectLayout).mockRejectedValueOnce(new Error('Execution context was destroyed')); + + const result = await makeRuntimeVerifier({ workspaceRoot: workspace })(SOURCE, { + path: 'App.jsx', + }); + + expect(result).toEqual({ + errors: [], + warnings: [ + { + source: LAYOUT_WARNING_SOURCE, + message: 'Layout checks did not run: Execution context was destroyed', + }, + ], + }); + }, 60_000); + + it('is reported by preview as a warning and does not fail the preview', async () => { + vi.mocked(inspectLayout).mockRejectedValueOnce(new Error('Execution context was destroyed')); + + const preview = await runPreview({ workspaceRoot: workspace, path: 'App.jsx', vision: false }); + + expect(preview.ok).toBe(true); + expect(preview.layoutIssues).toBeUndefined(); + expect(preview.warnings).toEqual([ + 'Layout checks did not run: Execution context was destroyed', + ]); + }, 60_000); + + it('carries the notes of a partial measurement into the preview warnings', async () => { + vi.mocked(inspectLayout).mockResolvedValueOnce({ + issues: [], + notes: ['Layout checks stopped after 5,000 elements; the rest of the page was not measured.'], + }); + + const preview = await runPreview({ workspaceRoot: workspace, path: 'App.jsx', vision: false }); + + expect(preview.ok).toBe(true); + expect(preview.warnings).toEqual([ + 'Layout checks stopped after 5,000 elements; the rest of the page was not measured.', + ]); + }, 60_000); + + it('still stops the done verifier when the run was cancelled', async () => { + const controller = new AbortController(); + vi.mocked(inspectLayout).mockImplementationOnce(async () => { + controller.abort(new Error('Stopped')); + throw new Error('Preview cancelled'); + }); + + await expect( + makeRuntimeVerifier({ workspaceRoot: workspace })(SOURCE, { + path: 'App.jsx', + signal: controller.signal, + }), + ).rejects.toThrow('Stopped'); + }, 60_000); + + it('still stops the preview when the run was cancelled', async () => { + const controller = new AbortController(); + vi.mocked(inspectLayout).mockImplementationOnce(async () => { + controller.abort(new Error('Stopped')); + throw new Error('Preview cancelled'); + }); + + const preview = await runPreview({ + workspaceRoot: workspace, + path: 'App.jsx', + vision: false, + signal: controller.signal, + }); + + expect(preview.ok).toBe(false); + expect(preview.reason).toBe('Preview cancelled'); + expect(preview.warnings).toBeUndefined(); + }, 60_000); +}); diff --git a/apps/desktop/src/main/preview-runtime.ts b/apps/desktop/src/main/preview-runtime.ts index caaef532d..891244b83 100644 --- a/apps/desktop/src/main/preview-runtime.ts +++ b/apps/desktop/src/main/preview-runtime.ts @@ -27,6 +27,7 @@ import { resolveArtifactSourceReferencePath, } from '@open-codesign/runtime'; import type { Browser, ConsoleMessage, HTTPRequest, HTTPResponse, Page } from 'puppeteer-core'; +import { inspectLayout, layoutFailureWarning } from './layout-probe'; import { getLogger } from './logger'; import { boundedPreview, collectVisibleEvidence, runPreviewSteps } from './preview-interactions'; import { resolveSafeWorkspaceChildPath } from './workspace-reader'; @@ -253,6 +254,15 @@ export async function runPreview(opts: RunPreviewOptions): Promise 0) result.layoutIssues = inspection.issues; + if (inspection.notes.length > 0) result.warnings = inspection.notes; + } catch (err) { + result.warnings = [layoutFailureWarning(err).message]; + } + if (opts.vision) { const png = await boundedPreview( page.screenshot({ type: 'png', encoding: 'base64' }), @@ -333,12 +343,9 @@ export async function runPreview(opts: RunPreviewOptions): Promise { expect(JSON.stringify(result.content)).toContain('Element 1 extends past the right edge'); }); + it('counts layout issues in the summary line, and only when there are some', async () => { + const summaryOf = async (layoutIssues?: LayoutIssue[]) => { + const runPreview = vi + .fn() + .mockResolvedValue(cannedResult(layoutIssues ? { layoutIssues } : {})); + const result = await makePreviewTool(runPreview).execute('call', { path: 'App.jsx' }); + const first = result.content[0]; + return first?.type === 'text' ? first.text : ''; + }; + + expect(await summaryOf([issue(1)])).toBe( + 'preview ok: 42 nodes, 0 console errors, 0 asset errors, 1 layout issue', + ); + expect(await summaryOf([issue(1), issue(2)])).toBe( + 'preview ok: 42 nodes, 0 console errors, 0 asset errors, 2 layout issues', + ); + expect(await summaryOf()).toBe('preview ok: 42 nodes, 0 console errors, 0 asset errors'); + }); + it('caps layout issues to the documented budget', async () => { const issues = Array.from({ length: MAX_LAYOUT_ISSUES + 10 }, (_, index) => issue(index)); const runPreview = vi.fn().mockResolvedValue(cannedResult({ layoutIssues: issues })); diff --git a/packages/core/src/tools/preview.ts b/packages/core/src/tools/preview.ts index 51c9e4a38..316996fb1 100644 --- a/packages/core/src/tools/preview.ts +++ b/packages/core/src/tools/preview.ts @@ -260,8 +260,13 @@ export function makePreviewTool( validatePreviewInput(params); const raw = await runPreview({ ...params, vision, ...(signal ? { signal } : {}) }); const result = trimPreviewResult(raw); + const layoutCount = result.layoutIssues?.length ?? 0; + const layout = + layoutCount > 0 + ? `, ${layoutCount} layout ${layoutCount === 1 ? 'issue' : 'issues'}` + : ''; const summary = result.ok - ? `preview ok: ${result.metrics.nodes} nodes, ${result.consoleErrors.length} console errors, ${result.assetErrors.length} asset errors` + ? `preview ok: ${result.metrics.nodes} nodes, ${result.consoleErrors.length} console errors, ${result.assetErrors.length} asset errors${layout}` : `preview failed${result.reason ? `: ${result.reason}` : ''} (${result.consoleErrors.length} console errors, ${result.assetErrors.length} asset errors)`; return { content: previewContent(summary, result), From 31bcfd7fafb6381274538791b0346456436bfc85 Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 00:01:37 +0800 Subject: [PATCH 3/9] feat(desktop): check contrast, tap targets and control names The layout probe is split into the shared samples and limits, the page function, and the analysis, which works on plain data. Three checks join overflow and clipped text: - contrast: the text colour over the backgrounds behind it, WCAG 2.x AA (4.5:1, and 3:1 for large text), grouped by colour pair and listed worst first. Text on an image or gradient, or partly transparent, is counted and reported, not guessed. Colours are read through a canvas so oklch() and color-mix() come out as sRGB. - tap targets: a link, button or input under 24 by 24 px, at phone width, unless it is inline in text or has no other target within that square (WCAG 2.2 SC 2.5.8). - names: a control with no text, aria-label, aria-labelledby, label, title, image alt or SVG title. Contrast and names run at the current width, tap targets at phone width. When there are more findings than the budget, one is taken from each check in turn; findings past a per-check limit are counted and the result says so. LayoutIssue gains the three checks, and its measured and limit fields become optional, since a missing name has no number. --- apps/desktop/src/main/layout-analysis.test.ts | 375 ++++++++++++++ apps/desktop/src/main/layout-analysis.ts | 199 ++++++++ apps/desktop/src/main/layout-browser-kit.ts | 63 +++ .../src/main/layout-probe.browser.test.ts | 81 +-- apps/desktop/src/main/layout-probe.test.ts | 317 +++++------- apps/desktop/src/main/layout-probe.ts | 469 ++++++++++++------ apps/desktop/src/main/layout-samples.ts | 157 ++++++ apps/desktop/src/main/layout-test-fixtures.ts | 20 + .../src/main/layout-wiring.browser.test.ts | 2 +- packages/core/src/tools/done.ts | 2 +- packages/core/src/tools/preview.ts | 16 +- 11 files changed, 1284 insertions(+), 417 deletions(-) create mode 100644 apps/desktop/src/main/layout-analysis.test.ts create mode 100644 apps/desktop/src/main/layout-analysis.ts create mode 100644 apps/desktop/src/main/layout-browser-kit.ts create mode 100644 apps/desktop/src/main/layout-samples.ts create mode 100644 apps/desktop/src/main/layout-test-fixtures.ts diff --git a/apps/desktop/src/main/layout-analysis.test.ts b/apps/desktop/src/main/layout-analysis.test.ts new file mode 100644 index 000000000..a8fe009b3 --- /dev/null +++ b/apps/desktop/src/main/layout-analysis.test.ts @@ -0,0 +1,375 @@ +import type { LayoutIssue } from '@open-codesign/core'; +import { describe, expect, it } from 'vitest'; +import { + analyzeLayoutSamples, + compositeOver, + contrastRatio, + measureTextContrast, + relativeLuminance, + selectIssues, + toHex, +} from './layout-analysis'; +import { OVERFLOW_TOLERANCE_PX, type Rgba, type TextGroup } from './layout-samples'; +import { samples } from './layout-test-fixtures'; + +const WHITE: Rgba = [255, 255, 255, 1]; +const group = (overrides: Partial = {}): TextGroup => ({ + selector: 'p.note', + count: 1, + foreground: [119, 119, 119, 1], + layers: [], + large: false, + ...overrides, +}); + +describe('colour maths', () => { + it('computes the relative luminance of black, white and a grey', () => { + expect(relativeLuminance([0, 0, 0])).toBe(0); + expect(relativeLuminance([255, 255, 255])).toBeCloseTo(1, 10); + expect(relativeLuminance([119, 119, 119])).toBeCloseTo(0.1845, 4); + }); + + it('computes WCAG contrast ratios the way the standard lists them', () => { + expect(contrastRatio([0, 0, 0], [255, 255, 255])).toBeCloseTo(21, 10); + expect(contrastRatio([255, 255, 255], [0, 0, 0])).toBeCloseTo(21, 10); + expect(contrastRatio([119, 119, 119], [255, 255, 255])).toBeCloseTo(4.48, 2); + expect(contrastRatio([118, 118, 118], [255, 255, 255])).toBeCloseTo(4.54, 2); + expect(contrastRatio([10, 20, 30], [10, 20, 30])).toBe(1); + }); + + it('lays a translucent colour over an opaque one', () => { + expect(compositeOver([0, 0, 0, 0.5], [255, 255, 255])).toEqual([127.5, 127.5, 127.5]); + expect(compositeOver([10, 20, 30, 1], [200, 200, 200])).toEqual([10, 20, 30]); + expect(compositeOver([10, 20, 30, 0], [200, 200, 200])).toEqual([200, 200, 200]); + }); + + it('writes colours as hex, rounding and keeping them in range', () => { + expect(toHex([127.5, 0, 255])).toBe('#8000ff'); + expect(toHex([-4, 300, 1.2])).toBe('#00ff01'); + }); +}); + +describe('measureTextContrast', () => { + it('measures text on white when no background is set', () => { + const { ratio, text, backdrop } = measureTextContrast({ + foreground: [119, 119, 119, 1], + layers: [], + }); + + expect(backdrop).toEqual([255, 255, 255]); + expect(text).toEqual([119, 119, 119]); + expect(ratio).toBeCloseTo(4.48, 2); + }); + + it('lays the backgrounds over white from the outermost to the innermost', () => { + // The innermost is half-transparent black over an opaque blue. + const { backdrop } = measureTextContrast({ + foreground: WHITE, + layers: [ + [0, 0, 0, 0.5], + [0, 0, 255, 1], + ], + }); + + expect(backdrop).toEqual([0, 0, 127.5]); + }); + + it('lays translucent text over the backdrop it sits on', () => { + const { text, backdrop } = measureTextContrast({ + foreground: [0, 0, 0, 0.5], + layers: [[255, 255, 255, 1]], + }); + + expect(backdrop).toEqual([255, 255, 255]); + expect(text).toEqual([127.5, 127.5, 127.5]); + }); +}); + +describe('analyzeLayoutSamples: overflow and clipped text', () => { + it('reports a page that fits, or is off by less than the tolerance, as clean', () => { + expect(analyzeLayoutSamples(samples())).toEqual([]); + expect(analyzeLayoutSamples(samples({ documentWidth: 390 + OVERFLOW_TOLERANCE_PX }))).toEqual( + [], + ); + }); + + it('names each element that makes the page scroll sideways, with the numbers', () => { + const issues = analyzeLayoutSamples( + samples({ + documentWidth: 1412, + overflowing: [ + { selector: 'table#prices', right: 1412 }, + { selector: 'main > div.hero', right: 960 }, + ], + }), + ); + + expect(issues).toEqual([ + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'table#prices', + message: + 'table#prices reaches 1,412 px, past the 390 px viewport edge, so the page scrolls sideways.', + measured: 1412, + limit: 390, + }, + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'main > div.hero', + message: + 'main > div.hero reaches 960 px, past the 390 px viewport edge, so the page scrolls sideways.', + measured: 960, + limit: 390, + }, + ]); + }); + + it('still reports the overflow when no single element was identified', () => { + expect(analyzeLayoutSamples(samples({ documentWidth: 520 }))).toEqual([ + { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'html', + message: + 'The page is 520 px wide at a 390 px viewport, so it scrolls sideways; no single element was identified.', + measured: 520, + limit: 390, + }, + ]); + }); + + it('does not report overflow the page hides, because it cannot scroll sideways', () => { + expect( + analyzeLayoutSamples( + samples({ + documentWidth: 900, + rootClipsX: true, + overflowing: [{ selector: 'div.wide', right: 900 }], + }), + ), + ).toEqual([]); + }); + + it('reports text a box clips on the side that sticks out', () => { + const issues = analyzeLayoutSamples( + samples({ + viewportWidth: 1280, + documentWidth: 1280, + clipped: [ + { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, + { selector: 'p.teaser', textRight: 200, boxRight: 300, textBottom: 190, boxBottom: 120 }, + ], + }), + ); + + expect(issues).toEqual([ + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'div#card', + message: + 'div#card hides part of its text: the text reaches 340 px but the box ends at 300 px (viewport 1,280 px).', + measured: 340, + limit: 300, + }, + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'p.teaser', + message: + 'p.teaser hides part of its text: the text reaches 190 px from the top but the box ends at 120 px (viewport 1,280 px).', + measured: 190, + limit: 120, + }, + ]); + }); +}); + +describe('analyzeLayoutSamples: contrast', () => { + const contrastOf = (...groups: TextGroup[]) => + analyzeLayoutSamples(samples({ textGroups: groups })).filter( + (issue) => issue.check === 'low-contrast', + ); + + it('reports text just under 4.5:1 and names both colours', () => { + const [issue] = contrastOf(group()); + + expect(issue).toEqual({ + check: 'low-contrast', + viewportWidth: 390, + selector: 'p.note', + message: 'p.note has low text contrast: 4.47:1 for #777777 on #ffffff, but text needs 4.5:1.', + measured: 4.47, + limit: 4.5, + }); + }); + + it('never rounds a failing ratio up to the threshold', () => { + // #777 is 4.478:1; rounded to two decimals it would read 4.48, still short of 4.5. + const [issue] = contrastOf(group()); + + expect(issue?.measured).toBe(4.47); + expect(issue?.measured).toBeLessThan(4.5); + }); + + it('accepts text at or above 4.5:1', () => { + expect(contrastOf(group({ foreground: [118, 118, 118, 1] }))).toEqual([]); + expect(contrastOf(group({ foreground: [0, 0, 0, 1] }))).toEqual([]); + }); + + it('asks only 3:1 of large text', () => { + // #949494 is 3.03:1 on white: enough for large text, not for body text. + const grey: Rgba = [148, 148, 148, 1]; + + expect(contrastOf(group({ foreground: grey, large: true }))).toEqual([]); + const [issue] = contrastOf(group({ foreground: grey })); + expect(issue?.limit).toBe(4.5); + const [largeIssue] = contrastOf(group({ foreground: [170, 170, 170, 1], large: true })); + expect(largeIssue?.limit).toBe(3); + expect(largeIssue?.message).toContain('large text needs 3:1'); + }); + + it('measures against the backgrounds laid over white', () => { + const [issue] = contrastOf( + group({ foreground: WHITE, layers: [[0, 0, 0, 0.5]], selector: 'h2.badge' }), + ); + + expect(issue?.message).toBe( + 'h2.badge has low text contrast: 3.97:1 for #ffffff on #808080, but text needs 4.5:1.', + ); + }); + + it('says how many text elements share a failing pair', () => { + const [issue] = contrastOf(group({ count: 7 })); + + expect(issue?.message).toMatch(/ 7 text elements use this pair\.$/); + expect(contrastOf(group({ count: 1 }))[0]?.message).not.toContain('use this pair'); + }); + + it('lists the worst pair first and every failing pair', () => { + const issues = contrastOf( + group({ selector: 'a', foreground: [150, 150, 150, 1] }), + group({ selector: 'b', foreground: [200, 200, 200, 1] }), + group({ selector: 'c', foreground: [119, 119, 119, 1] }), + ); + + expect(issues.map((issue) => issue.selector)).toEqual(['b', 'a', 'c']); + }); +}); + +describe('analyzeLayoutSamples: small targets and names', () => { + it('reports each small target with its size', () => { + const issues = analyzeLayoutSamples( + samples({ smallTargets: [{ selector: 'button.close', width: 16, height: 20 }] }), + ); + + expect(issues).toEqual([ + { + check: 'small-target', + viewportWidth: 390, + selector: 'button.close', + message: + 'button.close is 16 × 20 px; a tap target should be at least 24 × 24 px, or have that much room around it.', + measured: 16, + limit: 24, + }, + ]); + }); + + it('reports each control with no name, without numbers', () => { + const issues = analyzeLayoutSamples( + samples({ + unnamedControls: [ + { selector: 'button.menu', kind: 'button' }, + { selector: 'div.toggle', kind: 'div role=switch' }, + ], + }), + ); + + expect(issues).toEqual([ + { + check: 'missing-name', + viewportWidth: 390, + selector: 'button.menu', + message: + 'button.menu (button) has no accessible name: it has no text, aria-label, aria-labelledby, label or title.', + }, + { + check: 'missing-name', + viewportWidth: 390, + selector: 'div.toggle', + message: + 'div.toggle (div role=switch) has no accessible name: it has no text, aria-label, aria-labelledby, label or title.', + }, + ]); + expect(issues[0]).not.toHaveProperty('measured'); + }); + + it('lists the checks in one fixed order', () => { + const issues = analyzeLayoutSamples( + samples({ + documentWidth: 600, + overflowing: [{ selector: 'div.wide', right: 600 }], + clipped: [ + { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, + ], + textGroups: [group()], + smallTargets: [{ selector: 'button.close', width: 16, height: 16 }], + unnamedControls: [{ selector: 'button.menu', kind: 'button' }], + }), + ); + + expect(issues.map((issue) => issue.check)).toEqual([ + 'horizontal-overflow', + 'clipped-text', + 'low-contrast', + 'small-target', + 'missing-name', + ]); + }); +}); + +describe('selectIssues', () => { + const issue = (check: LayoutIssue['check'], index: number): LayoutIssue => ({ + check, + viewportWidth: 390, + selector: `${check}-${index}`, + message: `${check} ${index}`, + }); + + it('keeps everything when it fits the budget', () => { + const issues = [issue('low-contrast', 1), issue('missing-name', 1)]; + + expect(selectIssues(issues, 5)).toBe(issues); + expect(selectIssues(issues, 2)).toBe(issues); + }); + + it('takes one from each check in turn and keeps the original order', () => { + const issues = [ + ...[1, 2, 3, 4, 5].map((index) => issue('horizontal-overflow', index)), + ...[1, 2, 3, 4, 5].map((index) => issue('low-contrast', index)), + issue('missing-name', 1), + ]; + + expect(selectIssues(issues, 4).map((item) => item.selector)).toEqual([ + 'horizontal-overflow-1', + 'horizontal-overflow-2', + 'low-contrast-1', + 'missing-name-1', + ]); + }); + + it('still shows a rare check when one check has far more findings than the budget', () => { + const issues = [ + ...Array.from({ length: 40 }, (_, index) => issue('small-target', index)), + issue('low-contrast', 1), + ]; + + const kept = selectIssues(issues, 20); + + expect(kept).toHaveLength(20); + expect(kept.some((item) => item.check === 'low-contrast')).toBe(true); + }); +}); diff --git a/apps/desktop/src/main/layout-analysis.ts b/apps/desktop/src/main/layout-analysis.ts new file mode 100644 index 000000000..901db0c52 --- /dev/null +++ b/apps/desktop/src/main/layout-analysis.ts @@ -0,0 +1,199 @@ +/** + * The decisions behind the layout checks, made on the plain data the page-side + * function collects, so every threshold is testable without a browser. + */ + +import type { LayoutIssue } from '@open-codesign/core'; +import { + LAYOUT_PROBE_LIMITS, + type LayoutSamples, + MIN_CONTRAST_RATIO, + MIN_LARGE_TEXT_CONTRAST_RATIO, + MIN_TARGET_SIZE_PX, + type Rgba, + type TextGroup, +} from './layout-samples'; + +const formatPx = (value: number) => `${value.toLocaleString('en-US')} px`; + +type Rgb = [number, number, number]; + +/** A colour with some transparency, laid over an opaque one. */ +export function compositeOver(top: Rgba, base: Rgb): Rgb { + const [red, green, blue, alpha] = top; + return [ + red * alpha + base[0] * (1 - alpha), + green * alpha + base[1] * (1 - alpha), + blue * alpha + base[2] * (1 - alpha), + ]; +} + +/** WCAG relative luminance of an sRGB colour. */ +export function relativeLuminance([red, green, blue]: Rgb): number { + const linear = (channel: number) => { + const value = channel / 255; + return value <= 0.04045 ? value / 12.92 : ((value + 0.055) / 1.055) ** 2.4; + }; + return 0.2126 * linear(red) + 0.7152 * linear(green) + 0.0722 * linear(blue); +} + +/** WCAG contrast ratio, from 1 to 21. */ +export function contrastRatio(first: Rgb, second: Rgb): number { + const a = relativeLuminance(first); + const b = relativeLuminance(second); + return (Math.max(a, b) + 0.05) / (Math.min(a, b) + 0.05); +} + +export function toHex([red, green, blue]: Rgb): string { + const part = (channel: number) => + Math.round(Math.min(255, Math.max(0, channel))) + .toString(16) + .padStart(2, '0'); + return `#${part(red)}${part(green)}${part(blue)}`; +} + +/** + * The backgrounds behind the text are laid over white, the colour of a page that + * sets none, from the outermost to the innermost; then the text over the result. + */ +export function measureTextContrast(group: Pick): { + ratio: number; + text: Rgb; + backdrop: Rgb; +} { + let backdrop: Rgb = [255, 255, 255]; + for (let index = group.layers.length - 1; index >= 0; index -= 1) { + const layer = group.layers[index]; + if (layer) backdrop = compositeOver(layer, backdrop); + } + const text = compositeOver(group.foreground, backdrop); + return { ratio: contrastRatio(text, backdrop), text, backdrop }; +} + +function overflowIssues(samples: LayoutSamples, tolerance: number): LayoutIssue[] { + const { viewportWidth, documentWidth } = samples; + if (samples.rootClipsX || documentWidth - viewportWidth <= tolerance) return []; + if (samples.overflowing.length === 0) { + return [ + { + check: 'horizontal-overflow', + viewportWidth, + selector: 'html', + message: `The page is ${formatPx(documentWidth)} wide at a ${formatPx(viewportWidth)} viewport, so it scrolls sideways; no single element was identified.`, + measured: documentWidth, + limit: viewportWidth, + }, + ]; + } + return samples.overflowing.map((element) => ({ + check: 'horizontal-overflow', + viewportWidth, + selector: element.selector, + message: `${element.selector} reaches ${formatPx(element.right)}, past the ${formatPx(viewportWidth)} viewport edge, so the page scrolls sideways.`, + measured: element.right, + limit: viewportWidth, + })); +} + +function clippedIssues(samples: LayoutSamples): LayoutIssue[] { + const { viewportWidth } = samples; + return samples.clipped.map((text) => { + const horizontal = text.textRight > text.boxRight; + return { + check: 'clipped-text', + viewportWidth, + selector: text.selector, + message: horizontal + ? `${text.selector} hides part of its text: the text reaches ${formatPx(text.textRight)} but the box ends at ${formatPx(text.boxRight)} (viewport ${formatPx(viewportWidth)}).` + : `${text.selector} hides part of its text: the text reaches ${formatPx(text.textBottom)} from the top but the box ends at ${formatPx(text.boxBottom)} (viewport ${formatPx(viewportWidth)}).`, + measured: horizontal ? text.textRight : text.textBottom, + limit: horizontal ? text.boxRight : text.boxBottom, + }; + }); +} + +function contrastIssues(samples: LayoutSamples): LayoutIssue[] { + const failing = samples.textGroups + .map((group) => { + const { ratio, text, backdrop } = measureTextContrast(group); + return { group, ratio, text, backdrop }; + }) + .filter( + ({ group, ratio }) => + ratio < (group.large ? MIN_LARGE_TEXT_CONTRAST_RATIO : MIN_CONTRAST_RATIO), + ) + .sort((a, b) => a.ratio - b.ratio); + + return failing.map(({ group, ratio, text, backdrop }) => { + const needed = group.large ? MIN_LARGE_TEXT_CONTRAST_RATIO : MIN_CONTRAST_RATIO; + // WCAG does not round up: 4.499 does not pass as 4.5. + const shown = Math.floor(ratio * 100) / 100; + const shared = group.count > 1 ? ` ${group.count} text elements use this pair.` : ''; + return { + check: 'low-contrast', + viewportWidth: samples.viewportWidth, + selector: group.selector, + message: `${group.selector} has low text contrast: ${shown}:1 for ${toHex(text)} on ${toHex(backdrop)}, but ${group.large ? 'large text' : 'text'} needs ${needed}:1.${shared}`, + measured: shown, + limit: needed, + }; + }); +} + +function targetIssues(samples: LayoutSamples): LayoutIssue[] { + return samples.smallTargets.map((target) => ({ + check: 'small-target', + viewportWidth: samples.viewportWidth, + selector: target.selector, + message: `${target.selector} is ${target.width} × ${target.height} px; a tap target should be at least ${MIN_TARGET_SIZE_PX} × ${MIN_TARGET_SIZE_PX} px, or have that much room around it.`, + measured: Math.min(target.width, target.height), + limit: MIN_TARGET_SIZE_PX, + })); +} + +function nameIssues(samples: LayoutSamples): LayoutIssue[] { + return samples.unnamedControls.map((control) => ({ + check: 'missing-name', + viewportWidth: samples.viewportWidth, + selector: control.selector, + message: `${control.selector} (${control.kind}) has no accessible name: it has no text, aria-label, aria-labelledby, label or title.`, + })); +} + +/** Turns one measurement into findings, in the order the checks are listed in the docs. */ +export function analyzeLayoutSamples( + samples: LayoutSamples, + limits: Pick = LAYOUT_PROBE_LIMITS, +): LayoutIssue[] { + return [ + ...overflowIssues(samples, limits.overflowTolerance), + ...clippedIssues(samples), + ...contrastIssues(samples), + ...targetIssues(samples), + ...nameIssues(samples), + ]; +} + +/** + * Keeps at most `max` findings. When there are more, it takes one from each + * check in turn, so a page with fifty tiny buttons still shows its contrast + * problem, and keeps the original order. + */ +export function selectIssues(issues: LayoutIssue[], max: number): LayoutIssue[] { + if (issues.length <= max) return issues; + const queues = new Map(); + for (const issue of issues) { + const queue = queues.get(issue.check); + if (queue) queue.push(issue); + else queues.set(issue.check, [issue]); + } + const picked = new Set(); + while (picked.size < max) { + for (const queue of queues.values()) { + const next = queue.shift(); + if (next) picked.add(next); + if (picked.size === max) break; + } + } + return issues.filter((issue) => picked.has(issue)); +} diff --git a/apps/desktop/src/main/layout-browser-kit.ts b/apps/desktop/src/main/layout-browser-kit.ts new file mode 100644 index 000000000..eff3adafa --- /dev/null +++ b/apps/desktop/src/main/layout-browser-kit.ts @@ -0,0 +1,63 @@ +import { randomUUID } from 'node:crypto'; +import { mkdirSync, rmSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { findSystemChrome } from '@open-codesign/exporters'; +import puppeteer, { type Browser, type Page } from 'puppeteer-core'; +import { afterAll, afterEach, beforeAll, beforeEach } from 'vitest'; + +/** The system Chrome the browser tests drive, or null when there is none and they skip. */ +export const chrome = await findSystemChrome().catch(() => null); + +export const DESKTOP = { width: 1280, height: 800 } as const; + +/** + * One browser for the tests in a `describe`, and a fresh page at desktop size + * for each of them, with every request aborted so the page is only what the test sets. + */ +export function useChromePage(name: string): { + page: () => Page; + mount: (markup: string) => Promise; +} { + let browser: Browser; + let page: Page; + let profile: string; + + beforeAll(async () => { + if (!chrome) throw new Error('System Chrome unavailable'); + profile = resolve(`.${name}-${randomUUID()}`); + mkdirSync(profile); + browser = await puppeteer.launch({ + executablePath: chrome, + headless: true, + userDataDir: profile, + args: ['--disable-background-networking'], + }); + }, 30_000); + + afterAll(async () => { + try { + await browser?.close(); + } finally { + if (profile) rmSync(profile, { recursive: true, force: true }); + } + }, 30_000); + + beforeEach(async () => { + page = await browser.newPage(); + await page.setViewport(DESKTOP); + await page.setRequestInterception(true); + page.on('request', (request) => void request.abort()); + }); + + afterEach(async () => { + await page?.close(); + }); + + return { + page: () => page, + mount: (markup) => + page.setContent( + `${markup}`, + ), + }; +} diff --git a/apps/desktop/src/main/layout-probe.browser.test.ts b/apps/desktop/src/main/layout-probe.browser.test.ts index 3a088f9a5..867ca5c1c 100644 --- a/apps/desktop/src/main/layout-probe.browser.test.ts +++ b/apps/desktop/src/main/layout-probe.browser.test.ts @@ -1,55 +1,14 @@ -import { randomUUID } from 'node:crypto'; -import { mkdirSync, rmSync } from 'node:fs'; -import { resolve } from 'node:path'; -import { findSystemChrome } from '@open-codesign/exporters'; -import puppeteer, { type Browser, type Page } from 'puppeteer-core'; -import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; -import { inspectLayout, MAX_PROBE_ELEMENTS, PHONE_VIEWPORT } from './layout-probe'; - -const chrome = await findSystemChrome().catch(() => null); -const DESKTOP = { width: 1280, height: 800 }; +import { describe, expect, it } from 'vitest'; +import { chrome, DESKTOP, useChromePage } from './layout-browser-kit'; +import { inspectLayout } from './layout-probe'; +import { MAX_PROBE_ELEMENTS, PHONE_VIEWPORT } from './layout-samples'; + const LONG_TEXT = 'Long text that wraps over several lines and is cut off by its box. '.repeat(6); describe.skipIf(!chrome)('layout checks in system Chrome', () => { - let browser: Browser; - let page: Page; - let profile: string; - - beforeAll(async () => { - if (!chrome) throw new Error('System Chrome unavailable'); - profile = resolve(`.layout-probe-browser-${randomUUID()}`); - mkdirSync(profile); - browser = await puppeteer.launch({ - executablePath: chrome, - headless: true, - userDataDir: profile, - args: ['--disable-background-networking'], - }); - }, 30_000); - - afterAll(async () => { - try { - await browser?.close(); - } finally { - if (profile) rmSync(profile, { recursive: true, force: true }); - } - }, 30_000); - - beforeEach(async () => { - page = await browser.newPage(); - await page.setViewport(DESKTOP); - await page.setRequestInterception(true); - page.on('request', (request) => void request.abort()); - }); - - afterEach(async () => { - await page?.close(); - }); - + const { page: pageOf, mount: mountMarkup } = useChromePage('layout-probe-browser'); const mount = (body: string, head = '') => - page.setContent( - `${body}`, - ); + mountMarkup(`${head ? `` : ''}${body}`); describe('horizontal overflow', () => { it('names the element that scrolls the page sideways, at phone width only', async () => { @@ -57,7 +16,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { '
PlanPrice
', ); - const { issues, notes } = await inspectLayout(page); + const { issues, notes } = await inspectLayout(pageOf()); expect(notes).toEqual([]); expect(issues).toHaveLength(1); @@ -74,9 +33,9 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { it('puts the viewport back after measuring', async () => { await mount('
Hello
'); - await inspectLayout(page); + await inspectLayout(pageOf()); - expect(page.viewport()).toMatchObject(DESKTOP); + expect(pageOf().viewport()).toMatchObject(DESKTOP); }); it('reports nothing for a page that reflows to fit', async () => { @@ -85,7 +44,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { '.row{display:flex;flex-wrap:wrap}.card{width:200px;height:80px}', ); - expect((await inspectLayout(page)).issues).toEqual([]); + expect((await inspectLayout(pageOf())).issues).toEqual([]); }); it('does not blame content inside a container that scrolls on its own', async () => { @@ -93,7 +52,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { '
Wide strip
', ); - expect((await inspectLayout(page)).issues).toEqual([]); + expect((await inspectLayout(pageOf())).issues).toEqual([]); }); it('names the element that really overflows, not a wider one inside a scroller', async () => { @@ -102,7 +61,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { `, ); - const { issues } = await inspectLayout(page); + const { issues } = await inspectLayout(pageOf()); expect(issues.map((issue) => issue.selector)).toEqual(['div#banner']); }); @@ -110,7 +69,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { it('does not report overflow the page hides from the user', async () => { await mount('
Wide
', 'body{overflow-x:hidden}'); - expect((await inspectLayout(page)).issues).toEqual([]); + expect((await inspectLayout(pageOf())).issues).toEqual([]); }); it('ignores a fixed-position element, which cannot scroll the page', async () => { @@ -119,7 +78,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { `, ); - const { issues } = await inspectLayout(page); + const { issues } = await inspectLayout(pageOf()); expect(issues.map((issue) => issue.selector)).toEqual(['div#banner']); }); @@ -130,7 +89,7 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { .join(''); await mount(`
${boxes}
`); - const { issues } = await inspectLayout(page); + const { issues } = await inspectLayout(pageOf()); expect(issues.map((issue) => issue.measured)).toEqual([900, 800, 700]); }); @@ -139,7 +98,9 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { describe('clipped text', () => { const clippedAt = async (markup: string, head = '') => { await mount(markup, head); - return (await inspectLayout(page)).issues.filter((issue) => issue.check === 'clipped-text'); + return (await inspectLayout(pageOf())).issues.filter( + (issue) => issue.check === 'clipped-text', + ); }; it('reports text that a fixed-height box cuts off, with a path to the element', async () => { @@ -215,10 +176,10 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { it('says so when the page has more elements than it measures', async () => { await mount(Array.from({ length: MAX_PROBE_ELEMENTS + 100 }, () => '').join('')); - const { notes } = await inspectLayout(page); + const { notes } = await inspectLayout(pageOf()); expect(notes).toEqual([ - `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, + 'Layout checks did not cover the whole page (more than 5,000 elements or 400 controls); the rest was not measured.', ]); }); }); diff --git a/apps/desktop/src/main/layout-probe.test.ts b/apps/desktop/src/main/layout-probe.test.ts index 1c48c82af..3c6c21a23 100644 --- a/apps/desktop/src/main/layout-probe.test.ts +++ b/apps/desktop/src/main/layout-probe.test.ts @@ -2,155 +2,19 @@ import { LAYOUT_WARNING_SOURCE, type LayoutIssue, MAX_LAYOUT_ISSUES } from '@ope import type { Page } from 'puppeteer-core'; import { describe, expect, it } from 'vitest'; import { - analyzeLayoutSamples, collectLayoutSamples, inspectLayout, - LAYOUT_PROBE_LIMITS, - type LayoutSamples, layoutFailureWarning, layoutWarnings, - MAX_PROBE_ELEMENTS, - OVERFLOW_TOLERANCE_PX, - PHONE_VIEWPORT, } from './layout-probe'; - -function samples(overrides: Partial = {}): LayoutSamples { - return { - viewportWidth: 390, - documentWidth: 390, - rootClipsX: false, - overflowing: [], - clipped: [], - truncated: false, - ...overrides, - }; -} - -describe('analyzeLayoutSamples', () => { - it('reports a page that fits, or is off by less than the tolerance, as clean', () => { - expect(analyzeLayoutSamples(samples())).toEqual([]); - expect(analyzeLayoutSamples(samples({ documentWidth: 390 + OVERFLOW_TOLERANCE_PX }))).toEqual( - [], - ); - }); - - it('names each element that makes the page scroll sideways, with the numbers', () => { - const issues = analyzeLayoutSamples( - samples({ - documentWidth: 1412, - overflowing: [ - { selector: 'table#prices', right: 1412 }, - { selector: 'main > div.hero', right: 960 }, - ], - }), - ); - - expect(issues).toEqual([ - { - check: 'horizontal-overflow', - viewportWidth: 390, - selector: 'table#prices', - message: - 'table#prices reaches 1,412 px, past the 390 px viewport edge, so the page scrolls sideways.', - measured: 1412, - limit: 390, - }, - { - check: 'horizontal-overflow', - viewportWidth: 390, - selector: 'main > div.hero', - message: - 'main > div.hero reaches 960 px, past the 390 px viewport edge, so the page scrolls sideways.', - measured: 960, - limit: 390, - }, - ]); - }); - - it('still reports the overflow when no single element was identified', () => { - expect(analyzeLayoutSamples(samples({ documentWidth: 520 }))).toEqual([ - { - check: 'horizontal-overflow', - viewportWidth: 390, - selector: 'html', - message: - 'The page is 520 px wide at a 390 px viewport, so it scrolls sideways; no single element was identified.', - measured: 520, - limit: 390, - }, - ]); - }); - - it('does not report overflow the page hides, because it cannot scroll sideways', () => { - expect( - analyzeLayoutSamples( - samples({ - documentWidth: 900, - rootClipsX: true, - overflowing: [{ selector: 'div.wide', right: 900 }], - }), - ), - ).toEqual([]); - }); - - it('reports text a box clips on the side that sticks out', () => { - const issues = analyzeLayoutSamples( - samples({ - viewportWidth: 1280, - documentWidth: 1280, - clipped: [ - { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, - { selector: 'p.teaser', textRight: 200, boxRight: 300, textBottom: 190, boxBottom: 120 }, - ], - }), - ); - - expect(issues).toEqual([ - { - check: 'clipped-text', - viewportWidth: 1280, - selector: 'div#card', - message: - 'div#card hides part of its text: the text reaches 340 px but the box ends at 300 px (viewport 1,280 px).', - measured: 340, - limit: 300, - }, - { - check: 'clipped-text', - viewportWidth: 1280, - selector: 'p.teaser', - message: - 'p.teaser hides part of its text: the text reaches 190 px from the top but the box ends at 120 px (viewport 1,280 px).', - measured: 190, - limit: 120, - }, - ]); - }); - - it('lists overflow before clipped text, and reports clipped text on a page that hides overflow', () => { - const issues = analyzeLayoutSamples( - samples({ - documentWidth: 600, - overflowing: [{ selector: 'div.wide', right: 600 }], - clipped: [ - { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, - ], - }), - ); - expect(issues.map((issue) => issue.check)).toEqual(['horizontal-overflow', 'clipped-text']); - - const hidden = analyzeLayoutSamples( - samples({ - documentWidth: 600, - rootClipsX: true, - clipped: [ - { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, - ], - }), - ); - expect(hidden.map((issue) => issue.check)).toEqual(['clipped-text']); - }); -}); +import { + ALL_CHECKS, + LAYOUT_PROBE_LIMITS, + type LayoutProbeLimits, + type LayoutSamples, + PHONE_VIEWPORT, +} from './layout-samples'; +import { samples } from './layout-test-fixtures'; describe('layout warnings', () => { it('shows each finding and note as a layout warning', () => { @@ -163,9 +27,9 @@ describe('layout warnings', () => { limit: 390, }; - expect(layoutWarnings({ issues: [issue], notes: ['Stopped after 5,000 elements.'] })).toEqual([ + expect(layoutWarnings({ issues: [issue], notes: ['Not the whole page.'] })).toEqual([ { source: LAYOUT_WARNING_SOURCE, message: 'The page scrolls sideways.' }, - { source: LAYOUT_WARNING_SOURCE, message: 'Stopped after 5,000 elements.' }, + { source: LAYOUT_WARNING_SOURCE, message: 'Not the whole page.' }, ]); expect(layoutWarnings({ issues: [], notes: [] })).toEqual([]); }); @@ -182,6 +46,8 @@ describe('layout warnings', () => { interface FakePage { page: Page; viewports: Array<{ width: number; height: number }>; + /** The limits the page-side function was called with, one per measurement. */ + calls: LayoutProbeLimits[]; } /** A page whose measurements come from a script, one per call of the collector. */ @@ -190,24 +56,28 @@ function fakePage( measurements: Array, ): FakePage { const viewports: Array<{ width: number; height: number }> = []; + const calls: LayoutProbeLimits[] = []; const queue = [...measurements]; const page = { viewport: () => original, setViewport: async (viewport: { width: number; height: number }) => { viewports.push(viewport); }, - evaluate: async (fn: unknown) => { + evaluate: async (fn: unknown, limits: LayoutProbeLimits) => { if (fn !== collectLayoutSamples) return undefined; + calls.push(limits); const next = queue.shift(); if (next === undefined) throw new Error('Unexpected extra measurement'); if (next instanceof Error) throw next; return next; }, }; - return { page: page as unknown as Page, viewports }; + return { page: page as unknown as Page, viewports, calls }; } const desktop = { width: 1280, height: 800 }; +const atDesktop = (overrides: Partial = {}) => + samples({ viewportWidth: 1280, documentWidth: 1280, ...overrides }); const overflowAtPhone = samples({ documentWidth: 700, overflowing: [{ selector: 'table#prices', right: 700 }], @@ -215,10 +85,7 @@ const overflowAtPhone = samples({ describe('inspectLayout', () => { it('measures at the current width and at phone width, then restores the viewport', async () => { - const { page, viewports } = fakePage(desktop, [ - samples({ viewportWidth: 1280, documentWidth: 1280 }), - overflowAtPhone, - ]); + const { page, viewports } = fakePage(desktop, [atDesktop(), overflowAtPhone]); const inspection = await inspectLayout(page); @@ -229,8 +96,20 @@ describe('inspectLayout', () => { expect(viewports).toEqual([{ ...desktop, ...PHONE_VIEWPORT }, desktop]); }); - it('does not resize a page that is already phone width', async () => { - const { page, viewports } = fakePage({ width: PHONE_VIEWPORT.width, height: 700 }, [ + it('runs contrast and names at the current width, and tap targets at phone width', async () => { + const { page, calls } = fakePage(desktop, [atDesktop(), samples()]); + + await inspectLayout(page); + + expect(calls.map((call) => call.checks)).toEqual([ + { overflow: true, clipped: true, contrast: true, targets: false, names: true }, + { overflow: true, clipped: true, contrast: false, targets: true, names: false }, + ]); + expect(calls.every((call) => call.maxElements === LAYOUT_PROBE_LIMITS.maxElements)).toBe(true); + }); + + it('runs every check in one pass on a page that is already phone width', async () => { + const { page, viewports, calls } = fakePage({ width: PHONE_VIEWPORT.width, height: 700 }, [ overflowAtPhone, ]); @@ -238,11 +117,12 @@ describe('inspectLayout', () => { expect(inspection.issues).toHaveLength(1); expect(viewports).toEqual([]); + expect(calls.map((call) => call.checks)).toEqual([ALL_CHECKS]); }); it('restores the viewport and rejects when the phone measurement fails', async () => { const { page, viewports } = fakePage(desktop, [ - samples({ viewportWidth: 1280, documentWidth: 1280 }), + atDesktop(), new Error('Execution context was destroyed'), ]); @@ -257,57 +137,114 @@ describe('inspectLayout', () => { await expect(inspectLayout(page)).rejects.toThrow('The page has no viewport to measure'); }); - it('keeps the first findings and says so when there are more than the budget', async () => { - const clipped = Array.from({ length: MAX_LAYOUT_ISSUES }, (_, index) => ({ - selector: `p.item-${index}`, - textRight: 400, - boxRight: 300, - textBottom: 20, - boxBottom: 40, - })); - const { page } = fakePage(desktop, [ - samples({ viewportWidth: 1280, documentWidth: 1280, clipped }), - overflowAtPhone, - ]); - - const inspection = await inspectLayout(page); + it('stops when the run is cancelled', async () => { + const controller = new AbortController(); + controller.abort(); + const { page } = fakePage(desktop, [samples(), samples()]); - expect(inspection.issues).toHaveLength(MAX_LAYOUT_ISSUES); - expect(inspection.issues.every((issue) => issue.check === 'clipped-text')).toBe(true); - expect(inspection.notes).toEqual([ - `Layout checks found ${MAX_LAYOUT_ISSUES + 1} issues; only the first ${MAX_LAYOUT_ISSUES} are listed.`, - ]); + await expect(inspectLayout(page, controller.signal)).rejects.toThrow('Preview cancelled'); }); - it('says when the page was only measured in part', async () => { - const { page } = fakePage(desktop, [ - samples({ viewportWidth: 1280, documentWidth: 1280, truncated: true }), - samples(), - ]); + describe('when there is more than the budget', () => { + const clipped = (count: number) => + Array.from({ length: count }, (_, index) => ({ + selector: `p.item-${index}`, + textRight: 400, + boxRight: 300, + textBottom: 20, + boxBottom: 40, + })); + + it('lists the budget, one check at a time, and says how many there were', async () => { + const { page } = fakePage(desktop, [ + atDesktop({ + clipped: clipped(5), + textGroups: Array.from({ length: 30 }, (_, index) => ({ + selector: `p.low-${index}`, + count: 1, + foreground: [200 - index, 200 - index, 200 - index, 1], + layers: [], + large: false, + })), + }), + samples({ unnamedControls: [{ selector: 'button.menu', kind: 'button' }] }), + ]); + + const inspection = await inspectLayout(page); + + expect(inspection.issues).toHaveLength(MAX_LAYOUT_ISSUES); + expect(new Set(inspection.issues.map((issue) => issue.check))).toEqual( + new Set(['clipped-text', 'low-contrast', 'missing-name']), + ); + expect(inspection.notes).toEqual([ + `Layout checks found 36 issues; ${MAX_LAYOUT_ISSUES} are listed, taken from each check in turn.`, + ]); + }); - const inspection = await inspectLayout(page); + it('says how many findings went past the per-check limits', async () => { + const { page } = fakePage(desktop, [atDesktop({ omitted: 3 }), samples({ omitted: 1 })]); - expect(inspection.issues).toEqual([]); - expect(inspection.notes).toEqual([ - `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, - ]); + const inspection = await inspectLayout(page); + + expect(inspection.notes).toEqual([ + '4 more findings past the per-check limits were not listed.', + ]); + const single = await inspectLayout( + fakePage(desktop, [atDesktop({ omitted: 1 }), samples()]).page, + ); + expect(single.notes).toEqual(['1 more finding past the per-check limits was not listed.']); + }); }); - it('stops when the run is cancelled', async () => { - const controller = new AbortController(); - controller.abort(); - const { page } = fakePage(desktop, [samples(), samples()]); + describe('notes about the measurement', () => { + it('says when the page was only measured in part', async () => { + const { page } = fakePage(desktop, [atDesktop({ truncated: true }), samples()]); - await expect(inspectLayout(page, controller.signal)).rejects.toThrow('Preview cancelled'); + const inspection = await inspectLayout(page); + + expect(inspection.issues).toEqual([]); + expect(inspection.notes).toEqual([ + 'Layout checks did not cover the whole page (more than 5,000 elements or 400 controls); the rest was not measured.', + ]); + }); + + it('says how many text elements were not measured for contrast, and why', async () => { + const many = fakePage(desktop, [atDesktop({ unmeasuredText: 12 }), samples()]); + const one = fakePage(desktop, [atDesktop({ unmeasuredText: 1 }), samples()]); + + expect((await inspectLayout(many.page)).notes).toEqual([ + 'Contrast was not checked for 12 text elements on a background image or gradient, or partly transparent.', + ]); + expect((await inspectLayout(one.page)).notes).toEqual([ + 'Contrast was not checked for 1 text element on a background image or gradient, or partly transparent.', + ]); + }); + + it('says how many colour combinations were past the limit', async () => { + const { page } = fakePage(desktop, [atDesktop({ droppedTextGroups: 3 }), samples()]); + + expect((await inspectLayout(page)).notes).toEqual([ + '3 colour combinations beyond the first 200 were not checked for contrast.', + ]); + }); }); it('passes the documented limits to the page', () => { - expect(LAYOUT_PROBE_LIMITS).toEqual({ + expect(LAYOUT_PROBE_LIMITS).toMatchObject({ maxElements: 5000, overflowTolerance: 1, clipTolerance: 2, maxOverflowElements: 3, maxClippedTexts: 5, + largeTextPx: 24, + largeBoldTextPx: 18.66, + boldWeight: 700, + maxTextGroups: 200, + minMeasuredOpacity: 0.99, + minTargetSizePx: 24, + maxTargets: 400, + maxSmallTargets: 5, + maxUnnamedControls: 5, }); }); }); diff --git a/apps/desktop/src/main/layout-probe.ts b/apps/desktop/src/main/layout-probe.ts index e64bcc831..76d1a5371 100644 --- a/apps/desktop/src/main/layout-probe.ts +++ b/apps/desktop/src/main/layout-probe.ts @@ -1,8 +1,9 @@ /** - * Deterministic layout checks for the pages `preview` and `done` already open - * in Chrome. The page-side function only collects raw measurements; the - * decisions (what counts as overflow, how many findings to keep) are made here - * on plain data so they can be tested without a browser. + * Deterministic layout and accessibility checks for the pages `preview` and + * `done` already open in Chrome. The page-side function only collects raw + * measurements; the decisions (what counts as overflow, low contrast, a small + * target) are made on that data in `layout-analysis.ts`, so they can be tested + * without a browser. * * Findings are advice. They never change `ok` or the `done` status, because a * heuristic can be wrong and a false positive must not cost a repair round. @@ -15,6 +16,18 @@ import { MAX_LAYOUT_ISSUES, } from '@open-codesign/core'; import type { Page } from 'puppeteer-core'; +import { analyzeLayoutSamples, selectIssues } from './layout-analysis'; +import { + ALL_CHECKS, + LAYOUT_BUDGET_MS, + LAYOUT_PROBE_LIMITS, + type LayoutProbeChecks, + type LayoutProbeLimits, + type LayoutSamples, + PHONE_VIEWPORT, + type Rgba, + type TextGroup, +} from './layout-samples'; import { boundedPreview } from './preview-interactions'; // collectLayoutSamples is serialized into the page. Node's tsconfig has no DOM @@ -22,11 +35,18 @@ import { boundedPreview } from './preview-interactions'; interface PageStyle { display: string; position: string; + visibility: string; + opacity: string; overflowX: string; overflowY: string; textOverflow: string; webkitLineClamp?: string; animationName: string; + color: string; + backgroundColor: string; + backgroundImage: string; + fontSize: string; + fontWeight: string; } interface PageRect { left: number; @@ -47,80 +67,45 @@ interface PageElement extends PageNode { parentElement: PageElement | null; children: ArrayLike; childNodes: ArrayLike; + disabled?: boolean; + value?: string; + alt?: string; + labels?: ArrayLike; getBoundingClientRect(): PageRect; querySelectorAll(selector: string): ArrayLike; + matches(selector: string): boolean; + closest(selector: string): PageElement | null; + getAttribute(name: string): string | null; +} +interface PageCanvasContext { + globalCompositeOperation: string; + fillStyle: string; + fillRect(x: number, y: number, width: number, height: number): void; + getImageData(x: number, y: number, width: number, height: number): { data: ArrayLike }; +} +interface PageCanvas { + width: number; + height: number; + getContext(type: '2d', options: { willReadFrequently: boolean }): PageCanvasContext | null; } declare const document: { documentElement: PageElement & { clientWidth: number; scrollWidth: number }; body: PageElement | null; createRange(): { selectNodeContents(node: PageNode): void; getBoundingClientRect(): PageRect }; + createElement(tag: 'canvas'): PageCanvas; + getElementById(id: string): PageElement | null; }; declare const Node: { TEXT_NODE: number }; declare function getComputedStyle(element: PageElement): PageStyle; declare function requestAnimationFrame(callback: () => void): number; -/** The narrow viewport every page is also measured at. */ -export const PHONE_VIEWPORT = { width: 390, height: 844 } as const; -/** Most elements one measurement visits; a larger page is measured only in part. */ -export const MAX_PROBE_ELEMENTS = 5000; -/** Sub-pixel rounding is not overflow. */ -export const OVERFLOW_TOLERANCE_PX = 1; -/** Text may stick out of a clipping box by this much before it counts as clipped. */ -export const CLIP_TOLERANCE_PX = 2; -/** Elements named per viewport when the page scrolls sideways. */ -export const MAX_OVERFLOW_ELEMENTS = 3; -/** Clipped texts named per viewport. */ -export const MAX_CLIPPED_TEXTS = 5; -/** Time allowed for both measurements together. */ -export const LAYOUT_BUDGET_MS = 5000; - -export interface OverflowingElement { - selector: string; - /** Right edge of the element, in CSS px from the left of the viewport. */ - right: number; -} - -export interface ClippedText { - selector: string; - textRight: number; - boxRight: number; - textBottom: number; - boxBottom: number; -} - -/** What the page-side function returns. Plain data, so it survives serialization. */ -export interface LayoutSamples { - viewportWidth: number; - documentWidth: number; - /** The page cannot scroll sideways, because the root or body hides horizontal overflow. */ - rootClipsX: boolean; - overflowing: OverflowingElement[]; - clipped: ClippedText[]; - /** The element limit was reached, so the rest of the page was not measured. */ - truncated: boolean; -} - -export interface LayoutProbeLimits { - maxElements: number; - overflowTolerance: number; - clipTolerance: number; - maxOverflowElements: number; - maxClippedTexts: number; -} - -export const LAYOUT_PROBE_LIMITS: LayoutProbeLimits = { - maxElements: MAX_PROBE_ELEMENTS, - overflowTolerance: OVERFLOW_TOLERANCE_PX, - clipTolerance: CLIP_TOLERANCE_PX, - maxOverflowElements: MAX_OVERFLOW_ELEMENTS, - maxClippedTexts: MAX_CLIPPED_TEXTS, -}; - /** - * Runs inside the page: plain DOM, `getComputedStyle` and `getBoundingClientRect`. - * It must stay self-contained, because Puppeteer serializes it to source. + * Runs inside the page: plain DOM, `getComputedStyle`, `getBoundingClientRect` + * and a 1 × 1 canvas to turn any CSS colour into sRGB. It must stay + * self-contained, because Puppeteer serializes it to source. */ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { + const { checks } = limits; const root = document.documentElement; const body = document.body; const clipsX = (value: string) => value === 'hidden' || value === 'clip'; @@ -136,6 +121,12 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { rootClipsX, overflowing: [], clipped: [], + textGroups: [], + unmeasuredText: 0, + droppedTextGroups: 0, + omitted: 0, + smallTargets: [], + unnamedControls: [], truncated: false, }; if (!body) return samples; @@ -158,9 +149,46 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { const path = parts.join(' > '); return (node === body || node === null ? path : `… > ${path}`).slice(0, 120); }; + const hasText = (node: PageNode) => (node.textContent ?? '').trim() !== ''; + const ownTextNodes = (element: PageElement) => + Array.from(element.childNodes).filter( + (child) => child.nodeType === Node.TEXT_NODE && hasText(child), + ); + + // Any CSS colour, as sRGB: computed colours can be in other spaces (oklch, color-mix). + const colors = new Map(); + const canvas = checks.contrast ? document.createElement('canvas') : null; + if (canvas) { + canvas.width = 1; + canvas.height = 1; + } + const context = canvas ? canvas.getContext('2d', { willReadFrequently: true }) : null; + if (checks.contrast && !context) throw new Error('A 2D canvas is not available to read colours'); + const toRgba = (value: string): Rgba => { + const known = colors.get(value); + if (known) return known; + if (!context) throw new Error('A 2D canvas is not available to read colours'); + context.globalCompositeOperation = 'copy'; + context.fillStyle = value; + context.fillRect(0, 0, 1, 1); + const data = context.getImageData(0, 0, 1, 1).data; + const color: Rgba = [ + data[0] ?? 0, + data[1] ?? 0, + data[2] ?? 0, + Math.round(((data[3] ?? 0) / 255) * 1000) / 1000, + ]; + colors.set(value, color); + return color; + }; - const sideways = documentWidth - viewportWidth > limits.overflowTolerance && !rootClipsX; + const sideways = + checks.overflow && documentWidth - viewportWidth > limits.overflowTolerance && !rootClipsX; const extending = new Set(); + const textGroups = new Map(); + const targets: Array<{ element: PageElement; box: PageRect }> = []; + const controlSelector = + 'a[href], button, input:not([type="hidden"]), select, textarea, summary, [role="button"], [role="link"], [role="tab"], [role="checkbox"], [role="radio"], [role="switch"], [role="menuitem"], [role="textbox"], [role="combobox"], [role="slider"]'; const elements = body.querySelectorAll('*'); const visited = Math.min(elements.length, limits.maxElements); samples.truncated = elements.length > limits.maxElements; @@ -172,12 +200,50 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { return false; }; + const textOf = (node: PageNode | null) => (node?.textContent ?? '').replace(/\s+/g, ' ').trim(); + const accessibleName = (element: PageElement): string => { + const labelledBy = (element.getAttribute('aria-labelledby') ?? '') + .split(/\s+/) + .filter(Boolean) + .map((id) => textOf(document.getElementById(id))) + .join(' ') + .trim(); + if (labelledBy) return labelledBy; + const label = (element.getAttribute('aria-label') ?? '').trim(); + if (label) return label; + const tag = element.tagName.toLowerCase(); + const type = (element.getAttribute('type') ?? '').toLowerCase(); + if (tag === 'input') { + if (type === 'submit' || type === 'reset') return (element.value ?? '').trim() || type; + if (type === 'button') return (element.value ?? '').trim(); + if (type === 'image') return (element.alt ?? '').trim(); + } + const labelText = Array.from(element.labels ?? []) + .map((node) => textOf(node)) + .join(' ') + .trim(); + if (labelText) return labelText; + if (tag !== 'input' && tag !== 'select' && tag !== 'textarea') { + const text = textOf(element); + if (text) return text; + } + for (const inner of Array.from(element.querySelectorAll('img[alt], svg title, [aria-label]'))) { + const innerName = + (inner.getAttribute('alt') ?? '').trim() || + (inner.getAttribute('aria-label') ?? '').trim() || + (inner.tagName.toLowerCase() === 'title' ? textOf(inner) : ''); + if (innerName) return innerName; + } + return (element.getAttribute('title') ?? '').trim(); + }; + for (let index = 0; index < visited; index += 1) { const element = elements[index]; if (!element) continue; const style = getComputedStyle(element); if (style.display === 'none') continue; const box = element.getBoundingClientRect(); + const shown = style.visibility === 'visible'; if (sideways && style.position !== 'fixed' && box.width > 0) { if (box.right > viewportWidth + limits.overflowTolerance && !clippedByAncestor(element)) { @@ -185,48 +251,122 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { } } - const clipsText = clipsX(style.overflowX) || clipsX(style.overflowY); - const intentional = - style.textOverflow === 'ellipsis' || - (style.webkitLineClamp !== undefined && style.webkitLineClamp !== 'none') || - style.animationName !== 'none'; - if ( - !clipsText || - intentional || - box.width < 1 || - box.height < 1 || - samples.clipped.length >= limits.maxClippedTexts - ) { - continue; + if (checks.clipped) { + const clipsText = clipsX(style.overflowX) || clipsX(style.overflowY); + const intentional = + style.textOverflow === 'ellipsis' || + (style.webkitLineClamp !== undefined && style.webkitLineClamp !== 'none') || + style.animationName !== 'none'; + if (clipsText && !intentional && box.width >= 1 && box.height >= 1) { + for (const child of ownTextNodes(element)) { + const range = document.createRange(); + range.selectNodeContents(child); + const text = range.getBoundingClientRect(); + if (text.width < 1 || text.height < 1) continue; + const tolerance = limits.clipTolerance; + if ( + text.right > box.right + tolerance || + text.bottom > box.bottom + tolerance || + text.left < box.left - tolerance || + text.top < box.top - tolerance + ) { + if (samples.clipped.length < limits.maxClippedTexts) { + samples.clipped.push({ + selector: selectorOf(element), + textRight: Math.round(text.right), + boxRight: Math.round(box.right), + textBottom: Math.round(text.bottom), + boxBottom: Math.round(box.bottom), + }); + } else { + samples.omitted += 1; + } + break; + } + } + } + } + + if (checks.contrast && shown && box.width >= 1 && box.height >= 1) { + if (ownTextNodes(element).length > 0) { + let opacity = 1; + for (let node: PageElement | null = element; node; node = node.parentElement) { + opacity *= Number(getComputedStyle(node).opacity); + } + // The backgrounds behind the text: its own ancestors, up to the first opaque one. + const layers: Rgba[] = []; + let pictured = false; + for (let node: PageElement | null = element; node; node = node.parentElement) { + const nodeStyle = getComputedStyle(node); + if (nodeStyle.backgroundImage !== 'none') { + pictured = true; + break; + } + const layer = toRgba(nodeStyle.backgroundColor); + if (layer[3] > 0) { + layers.push(layer); + if (layer[3] === 1) break; + } + } + if (pictured || opacity < limits.minMeasuredOpacity) { + samples.unmeasuredText += 1; + } else { + const size = Number.parseFloat(style.fontSize); + const large = + size >= limits.largeTextPx || + (size >= limits.largeBoldTextPx && + Number.parseInt(style.fontWeight, 10) >= limits.boldWeight); + const foreground = toRgba(style.color); + const key = `${foreground.join(',')}|${layers.map((layer) => layer.join(',')).join(';')}|${large}`; + const group = textGroups.get(key); + if (group) { + group.count += 1; + } else if (textGroups.size < limits.maxTextGroups) { + textGroups.set(key, { + selector: selectorOf(element), + count: 1, + foreground, + layers, + large, + }); + } else { + samples.droppedTextGroups += 1; + } + } + } } - for (const child of Array.from(element.childNodes)) { - if (child.nodeType !== Node.TEXT_NODE || !(child.textContent ?? '').trim()) continue; - const range = document.createRange(); - range.selectNodeContents(child); - const text = range.getBoundingClientRect(); - if (text.width < 1 || text.height < 1) continue; - const tolerance = limits.clipTolerance; - if ( - text.right > box.right + tolerance || - text.bottom > box.bottom + tolerance || - text.left < box.left - tolerance || - text.top < box.top - tolerance - ) { - samples.clipped.push({ - selector: selectorOf(element), - textRight: Math.round(text.right), - boxRight: Math.round(box.right), - textBottom: Math.round(text.bottom), - boxBottom: Math.round(box.bottom), - }); - break; + + if ((checks.targets || checks.names) && shown && element.matches(controlSelector)) { + if (!element.closest('[aria-hidden="true"]')) { + if (checks.names && accessibleName(element) === '') { + if (samples.unnamedControls.length < limits.maxUnnamedControls) { + const role = element.getAttribute('role'); + samples.unnamedControls.push({ + selector: selectorOf(element), + kind: `${element.tagName.toLowerCase()}${role ? ` role=${role}` : ''}`, + }); + } else { + samples.omitted += 1; + } + } + // A control with no size, or hidden by opacity, is a visually hidden input whose label is the target. + const measurable = box.width >= 2 && box.height >= 2 && style.opacity !== '0'; + if ( + checks.targets && + measurable && + element.disabled !== true && + style.display !== 'inline' + ) { + if (targets.length < limits.maxTargets) targets.push({ element, box }); + else samples.truncated = true; + } } } } // Name the outermost elements that cross the edge: along each chain, the first box // that goes past it is where the overflow starts. - const outermost: OverflowingElement[] = []; + const outermost: Array<{ selector: string; right: number }> = []; for (const element of extending) { if (element.parentElement && extending.has(element.parentElement)) continue; outermost.push({ @@ -236,56 +376,37 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { } outermost.sort((a, b) => b.right - a.right); samples.overflowing = outermost.slice(0, limits.maxOverflowElements); - return samples; -} + samples.omitted += Math.max(0, outermost.length - limits.maxOverflowElements); -const formatPx = (value: number) => `${value.toLocaleString('en-US')} px`; + samples.textGroups = Array.from(textGroups.values()); -/** Turns one measurement into findings. Pure, so the thresholds are testable without Chrome. */ -export function analyzeLayoutSamples( - samples: LayoutSamples, - limits: Pick = LAYOUT_PROBE_LIMITS, -): LayoutIssue[] { - const issues: LayoutIssue[] = []; - const { viewportWidth, documentWidth } = samples; - - if (!samples.rootClipsX && documentWidth - viewportWidth > limits.overflowTolerance) { - if (samples.overflowing.length === 0) { - issues.push({ - check: 'horizontal-overflow', - viewportWidth, - selector: 'html', - message: `The page is ${formatPx(documentWidth)} wide at a ${formatPx(viewportWidth)} viewport, so it scrolls sideways; no single element was identified.`, - measured: documentWidth, - limit: viewportWidth, - }); - } - for (const element of samples.overflowing) { - issues.push({ - check: 'horizontal-overflow', - viewportWidth, - selector: element.selector, - message: `${element.selector} reaches ${formatPx(element.right)}, past the ${formatPx(viewportWidth)} viewport edge, so the page scrolls sideways.`, - measured: element.right, - limit: viewportWidth, + // A small target passes when the square around its centre touches no other target. + const half = limits.minTargetSizePx / 2; + const small: Array<{ selector: string; width: number; height: number }> = []; + for (const { element, box } of targets) { + if (box.width >= limits.minTargetSizePx && box.height >= limits.minTargetSizePx) continue; + const centerX = (box.left + box.right) / 2; + const centerY = (box.top + box.bottom) / 2; + const crowded = targets.some( + (other) => + other.element !== element && + other.box.left < centerX + half && + other.box.right > centerX - half && + other.box.top < centerY + half && + other.box.bottom > centerY - half, + ); + if (crowded) { + small.push({ + selector: selectorOf(element), + width: Math.round(box.width), + height: Math.round(box.height), }); } } - - for (const text of samples.clipped) { - const horizontal = text.textRight > text.boxRight; - issues.push({ - check: 'clipped-text', - viewportWidth, - selector: text.selector, - message: horizontal - ? `${text.selector} hides part of its text: the text reaches ${formatPx(text.textRight)} but the box ends at ${formatPx(text.boxRight)} (viewport ${formatPx(viewportWidth)}).` - : `${text.selector} hides part of its text: the text reaches ${formatPx(text.textBottom)} from the top but the box ends at ${formatPx(text.boxBottom)} (viewport ${formatPx(viewportWidth)}).`, - measured: horizontal ? text.textRight : text.textBottom, - limit: horizontal ? text.boxRight : text.boxBottom, - }); - } - return issues; + small.sort((a, b) => Math.min(a.width, a.height) - Math.min(b.width, b.height)); + samples.smallTargets = small.slice(0, limits.maxSmallTargets); + samples.omitted += Math.max(0, small.length - limits.maxSmallTargets); + return samples; } export interface LayoutInspection { @@ -304,8 +425,8 @@ async function settleLayout(page: Page): Promise { ); } -async function measure(page: Page): Promise { - return page.evaluate(collectLayoutSamples, LAYOUT_PROBE_LIMITS); +async function measure(page: Page, checks: LayoutProbeChecks): Promise { + return page.evaluate(collectLayoutSamples, { ...LAYOUT_PROBE_LIMITS, checks }); } /** @@ -317,40 +438,68 @@ export async function inspectLayout(page: Page, signal?: AbortSignal): Promise { const original = page.viewport(); if (original === null) throw new Error('The page has no viewport to measure'); - const widths = original.width === PHONE_VIEWPORT.width ? [] : [PHONE_VIEWPORT]; + const alreadyPhone = original.width === PHONE_VIEWPORT.width; const issues: LayoutIssue[] = []; - let truncated = false; + const measured: LayoutSamples[] = []; const take = (samples: LayoutSamples) => { + measured.push(samples); issues.push(...analyzeLayoutSamples(samples)); - truncated ||= samples.truncated; }; - take(await measure(page)); - try { - for (const viewport of widths) { - await page.setViewport({ ...original, ...viewport }); + take(await measure(page, alreadyPhone ? ALL_CHECKS : CURRENT_WIDTH_CHECKS)); + if (!alreadyPhone) { + try { + await page.setViewport({ ...original, ...PHONE_VIEWPORT }); await settleLayout(page); - take(await measure(page)); + take(await measure(page, PHONE_WIDTH_CHECKS)); + } finally { + await page.setViewport(original); } - } finally { - if (widths.length > 0) await page.setViewport(original); } const notes: string[] = []; - if (truncated) { + if (measured.some((samples) => samples.truncated)) { + notes.push( + 'Layout checks did not cover the whole page (more than 5,000 elements or 400 controls); the rest was not measured.', + ); + } + const unmeasured = measured.reduce((sum, samples) => sum + samples.unmeasuredText, 0); + if (unmeasured > 0) { + notes.push( + `Contrast was not checked for ${unmeasured} text element${unmeasured === 1 ? '' : 's'} on a background image or gradient, or partly transparent.`, + ); + } + const dropped = measured.reduce((sum, samples) => sum + samples.droppedTextGroups, 0); + if (dropped > 0) { + notes.push( + `${dropped} colour combination${dropped === 1 ? '' : 's'} beyond the first 200 were not checked for contrast.`, + ); + } + const omitted = measured.reduce((sum, samples) => sum + samples.omitted, 0); + if (omitted > 0) { notes.push( - `Layout checks stopped after ${MAX_PROBE_ELEMENTS.toLocaleString('en-US')} elements; the rest of the page was not measured.`, + `${omitted} more finding${omitted === 1 ? '' : 's'} past the per-check limits ${omitted === 1 ? 'was' : 'were'} not listed.`, ); } if (issues.length > MAX_LAYOUT_ISSUES) { notes.push( - `Layout checks found ${issues.length} issues; only the first ${MAX_LAYOUT_ISSUES} are listed.`, + `Layout checks found ${issues.length} issues; ${MAX_LAYOUT_ISSUES} are listed, taken from each check in turn.`, ); } - return { issues: issues.slice(0, MAX_LAYOUT_ISSUES), notes }; + return { issues: selectIssues(issues, MAX_LAYOUT_ISSUES), notes }; } /** What `done` shows for a measurement: each finding and note as a non-blocking warning. */ diff --git a/apps/desktop/src/main/layout-samples.ts b/apps/desktop/src/main/layout-samples.ts new file mode 100644 index 000000000..2fff24ac1 --- /dev/null +++ b/apps/desktop/src/main/layout-samples.ts @@ -0,0 +1,157 @@ +/** + * What the page-side layout function measures, and the named limits that bound + * it. Plain data and constants only: the function runs in the page, the + * decisions are made on this data in `layout-analysis.ts`. + */ + +/** The narrow viewport every page is also measured at. */ +export const PHONE_VIEWPORT = { width: 390, height: 844 } as const; +/** Most elements one measurement visits; a larger page is measured only in part. */ +export const MAX_PROBE_ELEMENTS = 5000; +/** Time allowed for all measurements together. */ +export const LAYOUT_BUDGET_MS = 5000; + +/** Sub-pixel rounding is not overflow. */ +export const OVERFLOW_TOLERANCE_PX = 1; +/** Text may stick out of a clipping box by this much before it counts as clipped. */ +export const CLIP_TOLERANCE_PX = 2; +/** Elements named per viewport when the page scrolls sideways. */ +export const MAX_OVERFLOW_ELEMENTS = 3; +/** Clipped texts named per viewport. */ +export const MAX_CLIPPED_TEXTS = 5; + +/** WCAG 2.x level AA contrast: 4.5:1 for text, 3:1 for large text. */ +export const MIN_CONTRAST_RATIO = 4.5; +export const MIN_LARGE_TEXT_CONTRAST_RATIO = 3; +/** Large text is at least 24 px, or at least 18.66 px and bold (WCAG's 18 pt and 14 pt bold). */ +export const LARGE_TEXT_PX = 24; +export const LARGE_BOLD_TEXT_PX = 18.66; +export const BOLD_WEIGHT = 700; +/** Distinct colour and size combinations kept per measurement; more are counted, not checked. */ +export const MAX_TEXT_GROUPS = 200; +/** Text this transparent, or sitting in something this transparent, is not measured. */ +export const MIN_MEASURED_OPACITY = 0.99; + +/** WCAG 2.2 SC 2.5.8: a pointer target is at least 24 by 24 CSS px, or has room around it. */ +export const MIN_TARGET_SIZE_PX = 24; +/** Targets looked at per measurement; the spacing exception compares each with the others. */ +export const MAX_TARGETS = 400; +/** Small targets named per viewport, smallest first. */ +export const MAX_SMALL_TARGETS = 5; + +/** Controls without a name named per measurement. */ +export const MAX_UNNAMED_CONTROLS = 5; + +/** Red, green and blue as 0 to 255 and alpha as 0 to 1, in sRGB. */ +export type Rgba = [number, number, number, number]; + +export interface OverflowingElement { + selector: string; + /** Right edge of the element, in CSS px from the left of the viewport. */ + right: number; +} + +export interface ClippedText { + selector: string; + textRight: number; + boxRight: number; + textBottom: number; + boxBottom: number; +} + +/** Text elements that share a colour, the backgrounds behind it and a size class. */ +export interface TextGroup { + selector: string; + /** How many text elements share this combination. */ + count: number; + foreground: Rgba; + /** The backgrounds behind the text, innermost first, up to and including the first opaque one. */ + layers: Rgba[]; + large: boolean; +} + +export interface SmallTarget { + selector: string; + width: number; + height: number; +} + +export interface UnnamedControl { + selector: string; + /** The tag, plus the role when there is one, for the message. */ + kind: string; +} + +/** What the page-side function returns. Plain data, so it survives serialization. */ +export interface LayoutSamples { + viewportWidth: number; + documentWidth: number; + /** The page cannot scroll sideways, because the root or body hides horizontal overflow. */ + rootClipsX: boolean; + overflowing: OverflowingElement[]; + clipped: ClippedText[]; + textGroups: TextGroup[]; + /** Text elements on a background image or gradient, or partly transparent, that were not measured. */ + unmeasuredText: number; + /** Colour combinations past `maxTextGroups` that were not measured. */ + droppedTextGroups: number; + /** Findings past a per-check limit (overflowing elements, clipped texts, small targets, unnamed controls). */ + omitted: number; + smallTargets: SmallTarget[]; + unnamedControls: UnnamedControl[]; + /** The element limit was reached, so the rest of the page was not measured. */ + truncated: boolean; +} + +/** Which checks one measurement runs. Contrast and names do not depend on width; tap targets matter at phone width. */ +export interface LayoutProbeChecks { + overflow: boolean; + clipped: boolean; + contrast: boolean; + targets: boolean; + names: boolean; +} + +export interface LayoutProbeLimits { + checks: LayoutProbeChecks; + maxElements: number; + overflowTolerance: number; + clipTolerance: number; + maxOverflowElements: number; + maxClippedTexts: number; + largeTextPx: number; + largeBoldTextPx: number; + boldWeight: number; + maxTextGroups: number; + minMeasuredOpacity: number; + minTargetSizePx: number; + maxTargets: number; + maxSmallTargets: number; + maxUnnamedControls: number; +} + +export const ALL_CHECKS: LayoutProbeChecks = { + overflow: true, + clipped: true, + contrast: true, + targets: true, + names: true, +}; + +export const LAYOUT_PROBE_LIMITS: LayoutProbeLimits = { + checks: ALL_CHECKS, + maxElements: MAX_PROBE_ELEMENTS, + overflowTolerance: OVERFLOW_TOLERANCE_PX, + clipTolerance: CLIP_TOLERANCE_PX, + maxOverflowElements: MAX_OVERFLOW_ELEMENTS, + maxClippedTexts: MAX_CLIPPED_TEXTS, + largeTextPx: LARGE_TEXT_PX, + largeBoldTextPx: LARGE_BOLD_TEXT_PX, + boldWeight: BOLD_WEIGHT, + maxTextGroups: MAX_TEXT_GROUPS, + minMeasuredOpacity: MIN_MEASURED_OPACITY, + minTargetSizePx: MIN_TARGET_SIZE_PX, + maxTargets: MAX_TARGETS, + maxSmallTargets: MAX_SMALL_TARGETS, + maxUnnamedControls: MAX_UNNAMED_CONTROLS, +}; diff --git a/apps/desktop/src/main/layout-test-fixtures.ts b/apps/desktop/src/main/layout-test-fixtures.ts new file mode 100644 index 000000000..3abf009f8 --- /dev/null +++ b/apps/desktop/src/main/layout-test-fixtures.ts @@ -0,0 +1,20 @@ +import type { LayoutSamples } from './layout-samples'; + +/** A measurement of a page with nothing to report, with fields overridden by a test. */ +export function samples(overrides: Partial = {}): LayoutSamples { + return { + viewportWidth: 390, + documentWidth: 390, + rootClipsX: false, + overflowing: [], + clipped: [], + textGroups: [], + unmeasuredText: 0, + droppedTextGroups: 0, + omitted: 0, + smallTargets: [], + unnamedControls: [], + truncated: false, + ...overrides, + }; +} diff --git a/apps/desktop/src/main/layout-wiring.browser.test.ts b/apps/desktop/src/main/layout-wiring.browser.test.ts index 82d3ec0aa..d30f1d851 100644 --- a/apps/desktop/src/main/layout-wiring.browser.test.ts +++ b/apps/desktop/src/main/layout-wiring.browser.test.ts @@ -5,7 +5,7 @@ import { LAYOUT_WARNING_SOURCE, trimPreviewResult } from '@open-codesign/core'; import { findSystemChrome } from '@open-codesign/exporters'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { makeRuntimeVerifier } from './done-verify'; -import { PHONE_VIEWPORT } from './layout-probe'; +import { PHONE_VIEWPORT } from './layout-samples'; import { runPreview } from './preview-runtime'; vi.mock('./logger', () => ({ getLogger: () => ({ warn: vi.fn(), error: vi.fn() }) })); diff --git a/packages/core/src/tools/done.ts b/packages/core/src/tools/done.ts index 3e12f3cc1..c4867b6a7 100644 --- a/packages/core/src/tools/done.ts +++ b/packages/core/src/tools/done.ts @@ -40,7 +40,7 @@ const STOP_LINES = [ /** Layout warnings are advice, so the agent may fix one it agrees with, but not by looping on `done`. */ const LAYOUT_ADVICE_LINES = [ - 'The design is verified. The layout warnings below are advice, not errors.', + 'The design is verified. The layout and accessibility warnings below are advice, not errors.', 'If you agree one is a real defect, fix it with one edit and check it with', '`preview`; do not call `done` again. Otherwise your only remaining action', 'is a short 2–3 sentence natural-language summary of the design decisions —', diff --git a/packages/core/src/tools/preview.ts b/packages/core/src/tools/preview.ts index 316996fb1..5bcc15b09 100644 --- a/packages/core/src/tools/preview.ts +++ b/packages/core/src/tools/preview.ts @@ -123,19 +123,25 @@ export const AssetError = Type.Object({ type: Type.Optional(Type.String()), }); -/** Layout findings are advice: they never change `ok` or the `done` status. */ +/** Layout and accessibility findings are advice: they never change `ok` or the `done` status. */ export const MAX_LAYOUT_ISSUES = 20; export const LayoutIssue = Type.Object({ - check: Type.Union([Type.Literal('horizontal-overflow'), Type.Literal('clipped-text')]), + check: Type.Union([ + Type.Literal('horizontal-overflow'), + Type.Literal('clipped-text'), + Type.Literal('low-contrast'), + Type.Literal('small-target'), + Type.Literal('missing-name'), + ]), /** Width of the viewport the page was measured at. */ viewportWidth: Type.Integer(), /** A short CSS-style path to the element, for the agent to find it in the source. */ selector: Type.String(), message: Type.String(), - /** What was measured, in CSS px, and the limit it was compared with. */ - measured: Type.Number(), - limit: Type.Number(), + /** What was measured and the limit it was compared with (px, or a contrast ratio), when the check is a number. */ + measured: Type.Optional(Type.Number()), + limit: Type.Optional(Type.Number()), }); export type LayoutIssue = Static; From 4ac2dd36b17dd1331acbd43929bb11b4c8e76494 Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 00:06:50 +0800 Subject: [PATCH 4/9] test(desktop): measure contrast, tap targets and names in Chrome A fixture for each rule and each exemption, in system Chrome: grey on white just under 4.5:1 and the 3:1 rule for large text, backgrounds from ancestors and translucent layers, oklch() and color-mix(), pairs reported once with a count, and text on a gradient or partly transparent counted instead of guessed. Small targets pass alone and fail beside another target, inline links are exempt, disabled and visually hidden controls are skipped. Names come from text, aria-label, aria-labelledby, label, title, image alt, SVG title and input defaults; a placeholder is not a name. Alpha in rgb() and rgba() is read exactly: a canvas rounds it to 8 bits and moved a ratio by 0.03. --- ...layout-probe.accessibility.browser.test.ts | 226 ++++++++++++++++++ .../layout-probe.contrast.browser.test.ts | 160 +++++++++++++ apps/desktop/src/main/layout-probe.ts | 11 + 3 files changed, 397 insertions(+) create mode 100644 apps/desktop/src/main/layout-probe.accessibility.browser.test.ts create mode 100644 apps/desktop/src/main/layout-probe.contrast.browser.test.ts diff --git a/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts b/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts new file mode 100644 index 000000000..98e0d6e8f --- /dev/null +++ b/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts @@ -0,0 +1,226 @@ +import { describe, expect, it } from 'vitest'; +import { chrome, DESKTOP, useChromePage } from './layout-browser-kit'; +import { inspectLayout } from './layout-probe'; +import { PHONE_VIEWPORT } from './layout-samples'; + +describe.skipIf(!chrome)('tap targets and control names in system Chrome', () => { + const { page, mount } = useChromePage('layout-accessibility'); + + const inspect = async (markup: string) => { + await mount(markup); + return inspectLayout(page()); + }; + const targetsIn = async (markup: string) => + (await inspect(markup)).issues.filter((issue) => issue.check === 'small-target'); + const namesIn = async (markup: string) => + (await inspect(markup)).issues.filter((issue) => issue.check === 'missing-name'); + + const square = (size: number, extra = '') => + `width:${size}px;height:${size}px;padding:0;border:0;${extra}`; + + describe('tap targets', () => { + // A small target is only a problem when another target is within 24 px of it (WCAG 2.2 SC 2.5.8). + const neighbour = ``; + + it('reports a small button next to another target at phone width, with its size', async () => { + const issues = await targetsIn( + `
${neighbour}
`, + ); + + expect(issues).toEqual([ + { + check: 'small-target', + viewportWidth: PHONE_VIEWPORT.width, + selector: 'button#close', + message: + 'button#close is 16 × 16 px; a tap target should be at least 24 × 24 px, or have that much room around it.', + measured: 16, + limit: 24, + }, + ]); + }); + + it('accepts a target of 24 px, and reports one that is short in only one direction', async () => { + expect( + await targetsIn( + `
${neighbour}
`, + ), + ).toEqual([]); + const issues = await targetsIn( + `
${neighbour}
`, + ); + expect(issues.map((issue) => [issue.selector, issue.measured])).toEqual([ + ['button#wide', 20], + ]); + }); + + it('passes a lone small target, because nothing else is within 24 px of it', async () => { + expect(await targetsIn(``)).toEqual( + [], + ); + }); + + it('does not ask a link inside a sentence to be 24 px', async () => { + expect( + await targetsIn( + `

Read the terms before you start. ${neighbour}

`, + ), + ).toEqual([]); + const issues = await targetsIn( + `
Chip${neighbour}
`, + ); + expect(issues.map((issue) => issue.selector)).toEqual(['a#chip']); + }); + + it('lets a small target pass when no other target is within the 24 px square around it', async () => { + const spaced = `
+ +
`; + const touching = `
+ +
`; + + expect(await targetsIn(spaced)).toEqual([]); + expect((await targetsIn(touching)).map((issue) => issue.selector)).toEqual([ + 'button#a', + 'button#b', + ]); + }); + + it('skips disabled, hidden and visually hidden controls', async () => { + expect( + await targetsIn( + ` + + + + + `, + ), + ).toEqual([]); + }); + + it('names the smallest first, at most five, and says how many more there were', async () => { + const buttons = [17, 12, 15, 10, 16, 14, 13, 11] + .map( + (size) => ``, + ) + .join(''); + + const { issues, notes } = await inspect(`
${buttons}
`); + + expect( + issues.filter((issue) => issue.check === 'small-target').map((issue) => issue.measured), + ).toEqual([10, 11, 12, 13, 14]); + expect(notes).toEqual(['3 more findings past the per-check limits were not listed.']); + }); + + it('says so when the page has more controls than it looks at', async () => { + const buttons = Array.from( + { length: 401 }, + () => ``, + ); + + const { notes } = await inspect(buttons.join('')); + + expect(notes).toEqual([ + 'Layout checks did not cover the whole page (more than 5,000 elements or 400 controls); the rest was not measured.', + ]); + }); + + it('measures at phone width only, so a small target is reported once', async () => { + const { issues } = await inspect( + `
${neighbour}
`, + ); + + expect(issues.map((issue) => issue.viewportWidth)).toEqual([PHONE_VIEWPORT.width]); + }); + }); + + describe('control names', () => { + it('reports an icon button that has no text, with the tag in the message', async () => { + const issues = await namesIn( + '', + ); + + expect(issues).toEqual([ + { + check: 'missing-name', + viewportWidth: DESKTOP.width, + selector: 'button#menu', + message: + 'button#menu (button) has no accessible name: it has no text, aria-label, aria-labelledby, label or title.', + }, + ]); + }); + + it.each([ + ['text', ''], + ['aria-label', ''], + [ + 'aria-labelledby', + 'Search', + ], + ['title', ''], + ['a label for the input', ''], + ['a wrapping label', ''], + ['the alt of an image in a link', 'Home'], + [ + 'the title of an svg in a button', + '', + ], + ['the default of a submit input', ''], + ['the value of a button input', ''], + ['text in a role=button element', '
Open
'], + ])('accepts a control named by %s', async (_label, markup) => { + expect(await namesIn(markup)).toEqual([]); + }); + + it.each([ + ['an empty button', '', 'button'], + ['a link with an empty alt image', '', 'a'], + ['an input with only a placeholder', '', 'input'], + ['an empty link', '', 'a'], + ['a select with no label', '', 'select'], + [ + 'a role=switch element with no text', + '
', + 'div role=switch', + ], + ])('reports %s', async (_label, markup, kind) => { + const issues = await namesIn(markup); + + expect(issues).toHaveLength(1); + expect(issues[0]?.message).toContain(`(${kind})`); + }); + + it('skips controls that are hidden from the user and from assistive technology', async () => { + expect( + await namesIn( + ` + + + `, + ), + ).toEqual([]); + }); + + it('names at most five controls, and says how many more there were', async () => { + const { issues, notes } = await inspect( + Array.from( + { length: 8 }, + (_, index) => ``, + ).join(''), + ); + + expect(issues.filter((issue) => issue.check === 'missing-name')).toHaveLength(5); + expect(notes).toEqual(['3 more findings past the per-check limits were not listed.']); + }); + + it('checks at the current width only, so a finding is not repeated at phone width', async () => { + const { issues } = await inspect(''); + + expect(issues.map((issue) => issue.viewportWidth)).toEqual([DESKTOP.width]); + }); + }); +}); diff --git a/apps/desktop/src/main/layout-probe.contrast.browser.test.ts b/apps/desktop/src/main/layout-probe.contrast.browser.test.ts new file mode 100644 index 000000000..e1463c38f --- /dev/null +++ b/apps/desktop/src/main/layout-probe.contrast.browser.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from 'vitest'; +import { chrome, DESKTOP, useChromePage } from './layout-browser-kit'; +import { inspectLayout } from './layout-probe'; + +describe.skipIf(!chrome)('contrast checks in system Chrome', () => { + const { page, mount } = useChromePage('layout-contrast'); + + const contrastOf = async (markup: string) => { + await mount(markup); + const { issues, notes } = await inspectLayout(page()); + return { issues: issues.filter((issue) => issue.check === 'low-contrast'), notes }; + }; + + describe('text and background', () => { + it('reports grey text on white that is just under 4.5:1, with both colours', async () => { + const { issues, notes } = await contrastOf('

Fine print

'); + + expect(notes).toEqual([]); + expect(issues).toEqual([ + { + check: 'low-contrast', + viewportWidth: DESKTOP.width, + selector: 'p#note', + message: + 'p#note has low text contrast: 4.47:1 for #777777 on #ffffff, but text needs 4.5:1.', + measured: 4.47, + limit: 4.5, + }, + ]); + }); + + it('accepts text at 4.5:1 and above, and the browser default of black on white', async () => { + expect( + (await contrastOf('

Readable

Default colours

')).issues, + ).toEqual([]); + }); + + it('asks only 3:1 of text that is 24 px, or 18.66 px and bold', async () => { + const grey = 'color:#949494'; + + expect((await contrastOf(`

Large

`)).issues).toEqual( + [], + ); + expect( + (await contrastOf(`

Bold

`)).issues, + ).toEqual([]); + expect( + (await contrastOf(`

Not bold

`)).issues, + ).toHaveLength(1); + expect((await contrastOf(`

Small

`)).issues).toHaveLength(1); + }); + + it('takes the background from the nearest ancestor that sets one', async () => { + const { issues } = await contrastOf( + '

Dim on dark

', + ); + + expect(issues).toHaveLength(1); + expect(issues[0]?.message).toContain('p#dim has low text contrast'); + expect(issues[0]?.message).toContain('#444444 on #222222'); + }); + + it('lays a translucent background over what is behind it', async () => { + const { issues } = await contrastOf( + '

New

', + ); + + expect(issues[0]?.message).toBe( + 'p#badge has low text contrast: 3.97:1 for #ffffff on #808080, but text needs 4.5:1.', + ); + }); + + it('lays translucent text over its background', async () => { + const { issues } = await contrastOf( + '

Half-transparent text

', + ); + + expect(issues[0]?.message).toContain('#808080 on #ffffff'); + }); + + it('reads colours in spaces other than sRGB', async () => { + const { issues } = await contrastOf( + `

oklch

+

color-mix

`, + ); + + expect(issues.map((issue) => issue.selector).sort()).toEqual(['p#a', 'p#b']); + expect(issues.find((issue) => issue.selector === 'p#b')?.message).toContain('#999999'); + }); + }); + + describe('what is reported once, and what is not measured', () => { + it('reports one finding for text elements that share a colour pair, and says how many', async () => { + const { issues } = await contrastOf( + Array.from( + { length: 5 }, + (_, index) => `

Hint

`, + ).join(''), + ); + + expect(issues).toHaveLength(1); + expect(issues[0]?.message).toMatch(/5 text elements use this pair\.$/); + }); + + it('lists the worst pair first', async () => { + const { issues } = await contrastOf( + `

Mild

Bad

`, + ); + + expect(issues.map((issue) => issue.selector)).toEqual(['p#bad', 'p#mild']); + }); + + it('does not guess on an image or gradient, and counts what it skipped', async () => { + const { issues, notes } = await contrastOf( + '

Over a gradient

', + ); + + expect(issues).toEqual([]); + expect(notes).toEqual([ + 'Contrast was not checked for 1 text element on a background image or gradient, or partly transparent.', + ]); + }); + + it('does not measure partly transparent elements', async () => { + const { issues, notes } = await contrastOf( + '

Faded

Faded too

', + ); + + expect(issues).toEqual([]); + expect(notes).toEqual([ + 'Contrast was not checked for 2 text elements on a background image or gradient, or partly transparent.', + ]); + }); + + it('ignores text nobody can see', async () => { + const { issues, notes } = await contrastOf( + `

Hidden

+

Not rendered

+

`, + ); + + expect(issues).toEqual([]); + expect(notes).toEqual([]); + }); + + it('measures the element that holds the text, not a container whose children have their own', async () => { + const { issues } = await contrastOf( + '
Black text
', + ); + + expect(issues).toEqual([]); + }); + + it('measures only at the current width, so a finding is not repeated at phone width', async () => { + const { issues } = await contrastOf('

Fine print

'); + + expect(issues.map((issue) => issue.viewportWidth)).toEqual([DESKTOP.width]); + }); + }); +}); diff --git a/apps/desktop/src/main/layout-probe.ts b/apps/desktop/src/main/layout-probe.ts index 76d1a5371..2d5530e41 100644 --- a/apps/desktop/src/main/layout-probe.ts +++ b/apps/desktop/src/main/layout-probe.ts @@ -164,9 +164,20 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { } const context = canvas ? canvas.getContext('2d', { willReadFrequently: true }) : null; if (checks.contrast && !context) throw new Error('A 2D canvas is not available to read colours'); + // Computed sRGB colours are `rgb()` or `rgba()` strings, which are read exactly; a canvas + // rounds alpha to 8 bits, so it is used only for the other spaces. + const legacyColor = + /^rgba?\(\s*([\d.]+)[,\s]+([\d.]+)[,\s]+([\d.]+)(?:\s*[,/]\s*([\d.]+)(%?))?\s*\)$/; const toRgba = (value: string): Rgba => { const known = colors.get(value); if (known) return known; + const parts = legacyColor.exec(value); + if (parts) { + const alpha = parts[4] === undefined ? 1 : Number.parseFloat(parts[4]) / (parts[5] ? 100 : 1); + const exact: Rgba = [Number(parts[1]), Number(parts[2]), Number(parts[3]), alpha]; + colors.set(value, exact); + return exact; + } if (!context) throw new Error('A 2D canvas is not available to read colours'); context.globalCompositeOperation = 'copy'; context.fillStyle = value; From cf9a5659a71de66d52c3b0bf922c469564adbb6c Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 00:12:11 +0800 Subject: [PATCH 5/9] test(desktop): make each exemption matter in the tap target tests A small target passes alone, so a test for an exemption has to put a target beside it; otherwise the check would pass without the exemption. Disabled, hidden, transparent, aria-hidden and visually hidden controls and a link inside a sentence now each sit next to a neighbour, and removing the exemption fails the test. The colour reader only handles the comma form of rgb() and rgba() that Chrome writes for computed colours; every other space goes through the canvas. --- ...layout-probe.accessibility.browser.test.ts | 30 +++++++++++-------- apps/desktop/src/main/layout-probe.ts | 15 ++++++---- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts b/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts index 98e0d6e8f..21b4b5206 100644 --- a/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts +++ b/apps/desktop/src/main/layout-probe.accessibility.browser.test.ts @@ -21,6 +21,8 @@ describe.skipIf(!chrome)('tap targets and control names in system Chrome', () => describe('tap targets', () => { // A small target is only a problem when another target is within 24 px of it (WCAG 2.2 SC 2.5.8). const neighbour = ``; + // Wide enough to sit under a link in a sentence. + const bar = ``; it('reports a small button next to another target at phone width, with its size', async () => { const issues = await targetsIn( @@ -63,7 +65,7 @@ describe.skipIf(!chrome)('tap targets and control names in system Chrome', () => it('does not ask a link inside a sentence to be 24 px', async () => { expect( await targetsIn( - `

Read the terms before you start. ${neighbour}

`, + `

Read the terms before you start.

${bar}`, ), ).toEqual([]); const issues = await targetsIn( @@ -87,17 +89,21 @@ describe.skipIf(!chrome)('tap targets and control names in system Chrome', () => ]); }); - it('skips disabled, hidden and visually hidden controls', async () => { - expect( - await targetsIn( - ` - - - - - `, - ), - ).toEqual([]); + it.each([ + ['disabled', ``], + ['not rendered', ``], + ['invisible', ``], + ['transparent', ``], + [ + 'hidden from assistive technology', + ``, + ], + [ + 'a visually hidden input whose label is the target', + '', + ], + ])('skips a control that is %s, even with a target right next to it', async (_label, markup) => { + expect(await targetsIn(`
${markup}${neighbour}
`)).toEqual([]); }); it('names the smallest first, at most five, and says how many more there were', async () => { diff --git a/apps/desktop/src/main/layout-probe.ts b/apps/desktop/src/main/layout-probe.ts index 2d5530e41..e18e57bc0 100644 --- a/apps/desktop/src/main/layout-probe.ts +++ b/apps/desktop/src/main/layout-probe.ts @@ -164,17 +164,20 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { } const context = canvas ? canvas.getContext('2d', { willReadFrequently: true }) : null; if (checks.contrast && !context) throw new Error('A 2D canvas is not available to read colours'); - // Computed sRGB colours are `rgb()` or `rgba()` strings, which are read exactly; a canvas - // rounds alpha to 8 bits, so it is used only for the other spaces. - const legacyColor = - /^rgba?\(\s*([\d.]+)[,\s]+([\d.]+)[,\s]+([\d.]+)(?:\s*[,/]\s*([\d.]+)(%?))?\s*\)$/; + // Computed sRGB colours are `rgb(r, g, b)` or `rgba(r, g, b, a)`, which are read exactly; + // a canvas rounds alpha to 8 bits, so it is used only for the other spaces. + const legacyColor = /^rgba?\((\d+), (\d+), (\d+)(?:, ([\d.]+))?\)$/; const toRgba = (value: string): Rgba => { const known = colors.get(value); if (known) return known; const parts = legacyColor.exec(value); if (parts) { - const alpha = parts[4] === undefined ? 1 : Number.parseFloat(parts[4]) / (parts[5] ? 100 : 1); - const exact: Rgba = [Number(parts[1]), Number(parts[2]), Number(parts[3]), alpha]; + const exact: Rgba = [ + Number(parts[1]), + Number(parts[2]), + Number(parts[3]), + parts[4] === undefined ? 1 : Number(parts[4]), + ]; colors.set(value, exact); return exact; } From aeffd6f6d6c0fa1186b7a64c8814cd05ab6c0a16 Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 00:15:01 +0800 Subject: [PATCH 6/9] test(core): run the design loop with the production agent and a scripted model Eight scenarios on the real pi Agent, with only the model stream and the browser executors replaced: write, preview, hear a runtime error from done, fix it and finish; a layout warning that does not fail done and changes what the agent is told; fixing the warning and checking it with preview, not done; preview listing layout issues while staying ok; warnings beside runtime errors; five done calls with warnings that never spend a repair round; three error rounds that still end the run with the workspace kept; and a layout measurement that did not run. --- .../layout-checks-in-done-and-preview.md | 2 +- packages/core/src/agent.design-loop.test.ts | 412 ++++++++++++++++++ 2 files changed, 413 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/agent.design-loop.test.ts diff --git a/.changeset/layout-checks-in-done-and-preview.md b/.changeset/layout-checks-in-done-and-preview.md index a49450c1d..5055c796e 100644 --- a/.changeset/layout-checks-in-done-and-preview.md +++ b/.changeset/layout-checks-in-done-and-preview.md @@ -3,4 +3,4 @@ "@open-codesign/desktop": patch --- -Check layout in `preview` and `done`. After a page loads, a deterministic measurement looks for a page that scrolls sideways and for text that its box clips, at the current width and again at a 390 px phone width. `preview` lists up to 20 findings in `layoutIssues`; `done` shows them under "Non-blocking warnings", and when one is present it lets the agent fix it and check it with `preview` instead of telling it to stop. Findings never change `ok` or the `done` status, so a false positive cannot cost a repair round. `done:verify:v1` now returns `warnings` and a `schemaVersion` next to `errors`. +Check layout and accessibility in `preview` and `done`. After a page loads, a deterministic measurement looks for a page that scrolls sideways, text that its box clips, text with too little contrast (WCAG AA, 4.5:1, or 3:1 for large text), tap targets under 24 by 24 px that have another target next to them, and controls with no accessible name. It measures at the current width and again at a 390 px phone width. `preview` lists up to 20 findings in `layoutIssues`; `done` shows them under "Non-blocking warnings", and when one is present it lets the agent fix it and check it with `preview` instead of telling it to stop. Findings never change `ok` or the `done` status, so a false positive cannot cost a repair round. `done:verify:v1` now returns `warnings` and a `schemaVersion` next to `errors`. diff --git a/packages/core/src/agent.design-loop.test.ts b/packages/core/src/agent.design-loop.test.ts new file mode 100644 index 000000000..c8cdb0c0a --- /dev/null +++ b/packages/core/src/agent.design-loop.test.ts @@ -0,0 +1,412 @@ +import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { Agent, AgentMessage, AgentOptions, StreamFn } from '@mariozechner/pi-agent-core'; +import { type AssistantMessage, createAssistantMessageEventStream } from '@mariozechner/pi-ai'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { generateViaAgent } from './agent'; +import type { GenerateInput } from './index'; +import type { DoneDetails, DoneRuntimeResult, DoneRuntimeVerifier } from './tools/done'; +import type { LayoutIssue, PreviewResult, RunPreviewFn } from './tools/preview'; +import type { TextEditorFsCallbacks } from './tools/text-editor'; + +const native = vi.hoisted(() => ({ stream: vi.fn(), agents: [] as Agent[] })); + +// Substitute only pi's streaming boundary; Agent scheduling, tools and recovery are real. +vi.mock('@mariozechner/pi-agent-core', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + Agent: class extends actual.Agent { + constructor(options: AgentOptions) { + super({ ...options, streamFn: native.stream }); + native.agents.push(this); + } + }, + }; +}); + +const DESIGN = `--- +version: alpha +name: Test design +colors: + primary: "#111111" +typography: + body: + fontFamily: Inter + fontSize: 16px + fontWeight: 400 +rounded: + sm: 4px +spacing: + sm: 8px +--- + +## Overview + +An isolated integration fixture. +`; +const BROKEN = `function App() { return

{price}

; } +ReactDOM.createRoot(document.getElementById('root')).render();`; +const FIXED = BROKEN.replace('{price}', 'Pricing'); +const input: GenerateInput = { + prompt: 'Build a pricing page', + history: [], + model: { provider: 'openai', modelId: 'gpt-5' }, + apiKey: 'synthetic-only', + systemPrompt: 'Use the design tools, then summarize.', +}; + +type Call = Extract; +const call = (name: string, args: Record, id = name): Call => ({ + type: 'toolCall', + name, + arguments: args, + id, +}); +const todos = () => call('set_todos', { items: [{ text: 'Complete design', checked: false }] }); +const create = (path: string, text: string) => + call( + 'str_replace_based_edit_tool', + { command: 'create', path, file_text: text }, + `create-${path}`, + ); +const preview = (id = 'preview') => call('preview', { path: 'App.jsx' }, id); +const done = (id = 'done') => call('done', { path: 'App.jsx' }, id); +const fix = (oldStr: string, newStr: string, id = 'fix') => + call( + 'str_replace_based_edit_tool', + { command: 'str_replace', path: 'App.jsx', old_str: oldStr, new_str: newStr }, + id, + ); + +function response(model: Parameters[0], content: AssistantMessage['content']) { + const stopReason: 'toolUse' | 'stop' = content.some((part) => part.type === 'toolCall') + ? 'toolUse' + : 'stop'; + const message: AssistantMessage = { + role: 'assistant', + api: model.api, + provider: model.provider, + model: model.id, + content, + timestamp: Date.now(), + stopReason, + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + }; + const stream = createAssistantMessageEventStream(); + stream.push({ type: 'done', reason: stopReason, message }); + return stream; +} + +/** What the model was shown before each of its turns, so a test can read the tool results it saw. */ +let requests: AgentMessage[][]; +function script(batches: Array) { + requests = []; + native.stream.mockImplementation((model, context) => { + requests.push([...context.messages]); + const batch = batches.shift(); + if (batch === undefined) throw new Error('Unexpected extra provider call'); + return response(model, typeof batch === 'string' ? [{ type: 'text', text: batch }] : batch); + }); +} + +/** The text of the tool results with this id that the model had seen by its n-th turn (0-based). */ +function toolResultText(turn: number, toolCallId: string): string { + const message = requests[turn]?.find( + (item) => item.role === 'toolResult' && item.toolCallId === toolCallId, + ); + if (!message || message.role !== 'toolResult') + throw new Error(`No ${toolCallId} result at turn ${turn}`); + return message.content.map((part) => (part.type === 'text' ? part.text : '')).join(''); +} + +const overflow: LayoutIssue = { + check: 'horizontal-overflow', + viewportWidth: 390, + selector: 'table#prices', + message: + 'table#prices reaches 800 px, past the 390 px viewport edge, so the page scrolls sideways.', + measured: 800, + limit: 390, +}; +const layoutWarning = { source: 'layout', message: overflow.message }; +const runtimeError = { + source: 'console.error', + message: 'ReferenceError: price is not defined', + lineno: 1, +}; +const clean: DoneRuntimeResult = { errors: [], warnings: [] }; + +function previewResult(overrides: Partial = {}): PreviewResult { + return { + ok: true, + consoleErrors: [], + assetErrors: [], + metrics: { nodes: 4, width: 1280, height: 800, loadMs: 1 }, + ...overrides, + }; +} + +let root: string; +let fs: TextEditorFsCallbacks; +let writes: string[]; +beforeEach(() => { + native.stream.mockReset(); + native.agents.length = 0; + root = mkdtempSync(join(tmpdir(), 'codesign-design-loop-')); + writes = []; + fs = { + view(path) { + if (!readdirSync(root).includes(path)) return null; + const content = readFileSync(join(root, path), 'utf8'); + return { content, numLines: content.split('\n').length }; + }, + listDir: () => readdirSync(root), + create(path, content) { + writeFileSync(join(root, path), content); + writes.push(path); + return { path }; + }, + strReplace(path, oldStr, newStr) { + const current = readFileSync(join(root, path), 'utf8'); + if (!current.includes(oldStr)) throw new Error('Missing exact edit target'); + writeFileSync(join(root, path), current.replace(oldStr, newStr)); + writes.push(path); + return { path }; + }, + insert() { + throw new Error('Unexpected insert'); + }, + }; +}); +afterEach(() => rmSync(root, { recursive: true, force: true })); + +/** Runs the production agent with stubbed browser executors, recording every `done` result. */ +function run(options: { verify: DoneRuntimeVerifier; runPreview?: RunPreviewFn }) { + const checks: DoneDetails[] = []; + const result = generateViaAgent( + { ...input, runPreview: options.runPreview ?? (async () => previewResult()) }, + { + fs, + runtimeVerify: options.verify, + onEvent: (event) => { + if (event.type === 'tool_execution_end' && event.toolName === 'done') { + checks.push(event.result.details as DoneDetails); + } + }, + }, + ); + return { result, checks }; +} + +describe('the design loop with the production agent and a scripted model', () => { + it('writes, previews, hears a runtime error from done, fixes it and finishes', async () => { + const verify = vi + .fn() + .mockResolvedValueOnce({ errors: [runtimeError], warnings: [] }) + .mockResolvedValueOnce(clean); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', BROKEN)], + [preview()], + [done('done-1')], + [fix('{price}', 'Pricing')], + [preview('preview-2')], + [done('done-2')], + 'Fixed the heading.', + ]); + + const { result, checks } = run({ verify }); + const finished = await result; + + expect(checks.map((check) => check.status)).toEqual(['has_errors', 'ok']); + expect(verify.mock.calls.map(([source]) => source.includes('Pricing'))).toEqual([false, true]); + expect(toolResultText(4, 'done-1')).toMatch( + /^has_errors\n- console\.error: ReferenceError: price is not defined \(line 1\)/, + ); + expect(readFileSync(join(root, 'App.jsx'), 'utf8')).toBe(FIXED); + expect(finished.resourceState?.lastDone?.status).toBe('ok'); + expect(finished.artifacts).toHaveLength(1); + }); + + describe('layout and accessibility advice', () => { + it('lists a layout warning without failing done, and tells the agent it may fix one without calling done again', async () => { + const verify = vi + .fn() + .mockResolvedValue({ errors: [], warnings: [layoutWarning] }); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', FIXED)], + [preview()], + [done()], + 'Done, with one layout note.', + ]); + + const { result, checks } = run({ verify }); + const finished = await result; + + expect(checks).toHaveLength(1); + expect(checks[0]).toMatchObject({ status: 'ok', errors: [], warnings: [layoutWarning] }); + const text = toolResultText(4, 'done'); + expect(text).toContain('Non-blocking warnings:'); + expect(text).toContain(`- layout: ${layoutWarning.message}`); + expect(text).toContain('advice, not errors'); + expect(text).not.toContain('STOP.'); + expect(finished.resourceState?.lastDone?.status).toBe('ok'); + }); + + it('lets the agent fix the warning and check it with preview, then stop without a second done', async () => { + const verify = vi + .fn() + .mockResolvedValue({ errors: [], warnings: [layoutWarning] }); + const runPreview = vi + .fn() + .mockResolvedValueOnce(previewResult({ layoutIssues: [overflow] })) + .mockResolvedValueOnce(previewResult()); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', FIXED)], + [preview('preview-1')], + [done()], + [fix('Pricing', 'Plans')], + [preview('preview-2')], + 'Fixed the layout note.', + ]); + + const { result, checks } = run({ verify, runPreview }); + await result; + + expect(checks).toHaveLength(1); + expect(verify).toHaveBeenCalledTimes(1); + expect(runPreview).toHaveBeenCalledTimes(2); + expect(readFileSync(join(root, 'App.jsx'), 'utf8')).toContain('Plans'); + }); + + it('shows the model the layout issues of a preview, which stays ok', async () => { + const runPreview = vi + .fn() + .mockResolvedValue(previewResult({ layoutIssues: [overflow] })); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', FIXED)], + [preview()], + [done()], + 'Done.', + ]); + + const { result } = run({ verify: async () => clean, runPreview }); + await result; + + const text = toolResultText(3, 'preview'); + expect(text).toContain( + 'preview ok: 4 nodes, 0 console errors, 0 asset errors, 1 layout issue', + ); + expect(text).toContain('"layoutIssues"'); + expect(text).toContain('table#prices'); + }); + + it('reports layout warnings next to the runtime errors that fail done', async () => { + const verify = vi + .fn() + .mockResolvedValueOnce({ errors: [runtimeError], warnings: [layoutWarning] }) + .mockResolvedValueOnce(clean); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', BROKEN)], + [preview()], + [done('done-1')], + [fix('{price}', 'Pricing')], + [done('done-2')], + 'Fixed.', + ]); + + const { result, checks } = run({ verify }); + await result; + + expect(checks.map((check) => check.status)).toEqual(['has_errors', 'ok']); + expect(checks[0]?.warnings).toEqual([layoutWarning]); + const text = toolResultText(4, 'done-1'); + expect(text.startsWith('has_errors')).toBe(true); + expect(text).toContain('Non-blocking warnings:'); + }); + + it('does not count warnings as repair rounds, however often done is called', async () => { + const verify = vi + .fn() + .mockResolvedValue({ errors: [], warnings: [layoutWarning] }); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', FIXED)], + [preview()], + [done('done-1')], + [done('done-2')], + [done('done-3')], + [done('done-4')], + [done('done-5')], + 'Done.', + ]); + + const { result, checks } = run({ verify }); + const finished = await result; + + expect(checks.map((check) => check.status)).toEqual(['ok', 'ok', 'ok', 'ok', 'ok']); + expect(finished.resourceState?.lastDone?.status).toBe('ok'); + }); + + it('still stops after three done error rounds, whatever the warnings say', async () => { + const verify = vi + .fn() + .mockResolvedValue({ errors: [runtimeError], warnings: [layoutWarning] }); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', BROKEN)], + [preview()], + [done('done-1')], + [done('done-2')], + [done('done-3')], + ]); + + const { result, checks } = run({ verify }); + + await expect(result).rejects.toMatchObject({ code: 'GENERATION_INCOMPLETE' }); + expect(checks.map((check) => check.status)).toEqual([ + 'has_errors', + 'has_errors', + 'has_errors', + ]); + expect(readFileSync(join(root, 'App.jsx'), 'utf8')).toBe(BROKEN); + }); + + it('treats a layout measurement that did not run as a warning, not a failure', async () => { + const notRun = { + source: 'layout', + message: 'Layout checks did not run: Execution context was destroyed', + }; + const verify = vi + .fn() + .mockResolvedValue({ errors: [], warnings: [notRun] }); + script([ + [todos()], + [create('DESIGN.md', DESIGN), create('App.jsx', FIXED)], + [preview()], + [done()], + 'Done.', + ]); + + const { result, checks } = run({ verify }); + const finished = await result; + + expect(checks[0]?.status).toBe('ok'); + expect(toolResultText(4, 'done')).toContain(`- layout: ${notRun.message}`); + expect(finished.resourceState?.lastDone?.status).toBe('ok'); + }); + }); +}); From b098de85adc1b1af1a8f57c04189d4dfaa77763a Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 00:32:13 +0800 Subject: [PATCH 7/9] test(desktop): give the fake browser a complete layout measurement The harness document test replaces puppeteer and answered the layout function with an early, partial shape. The analysis now reads every field, so it failed and the verifier reported "Layout checks did not run". It answers with the shared fixture instead. --- .../desktop/src/main/harness-document.test.ts | 82 +++++++++---------- 1 file changed, 39 insertions(+), 43 deletions(-) diff --git a/apps/desktop/src/main/harness-document.test.ts b/apps/desktop/src/main/harness-document.test.ts index 5bf4afb62..aa9d9f932 100644 --- a/apps/desktop/src/main/harness-document.test.ts +++ b/apps/desktop/src/main/harness-document.test.ts @@ -21,50 +21,46 @@ const browser = vi.hoisted(() => ({ vi.mock('@open-codesign/exporters', () => ({ findSystemChrome: async () => 'synthetic-chrome' })); vi.mock('./logger', () => ({ getLogger: () => ({ warn: vi.fn(), error: vi.fn() }) })); -vi.mock('puppeteer-core', () => ({ - default: { - launch: async () => { - const handlers = new Map void>(); - const page = { - viewport: () => ({ width: 1280, height: 800 }), - setViewport: async () => {}, - setRequestInterception: async () => {}, - evaluateOnNewDocument: async () => {}, - on: (event: string, handler: (arg: unknown) => void) => handlers.set(event, handler), - goto: async (url: string) => { - const request: FakeRequest = { - url: () => url, - isNavigationRequest: () => true, - resourceType: () => 'document', - respond: vi.fn(async () => {}), - continue: vi.fn(async () => {}), - abort: vi.fn(async () => {}), - }; - browser.navigations.push({ - url, - existedOnDisk: existsSync(fileURLToPath(url)), - request, - }); - handlers.get('request')?.(request); - await vi.waitFor(() => expect(request.respond).toHaveBeenCalled()); - }, - evaluate: async (fn: { name: string }) => - fn.name === 'collectLayoutSamples' - ? { - viewportWidth: 1280, - documentWidth: 1280, - rootClipsX: false, - overflowing: [], - clipped: [], - truncated: false, - } - : { nodes: 1, width: 1280, height: 800 }, - close: async () => {}, - }; - return { newPage: async () => page, close: async () => {} }; +vi.mock('puppeteer-core', async () => { + const { samples } = await import('./layout-test-fixtures'); + return { + default: { + launch: async () => { + const handlers = new Map void>(); + const page = { + viewport: () => ({ width: 1280, height: 800 }), + setViewport: async () => {}, + setRequestInterception: async () => {}, + evaluateOnNewDocument: async () => {}, + on: (event: string, handler: (arg: unknown) => void) => handlers.set(event, handler), + goto: async (url: string) => { + const request: FakeRequest = { + url: () => url, + isNavigationRequest: () => true, + resourceType: () => 'document', + respond: vi.fn(async () => {}), + continue: vi.fn(async () => {}), + abort: vi.fn(async () => {}), + }; + browser.navigations.push({ + url, + existedOnDisk: existsSync(fileURLToPath(url)), + request, + }); + handlers.get('request')?.(request); + await vi.waitFor(() => expect(request.respond).toHaveBeenCalled()); + }, + evaluate: async (fn: { name: string }) => + fn.name === 'collectLayoutSamples' + ? samples({ viewportWidth: 1280, documentWidth: 1280 }) + : { nodes: 1, width: 1280, height: 800 }, + close: async () => {}, + }; + return { newPage: async () => page, close: async () => {} }; + }, }, - }, -})); + }; +}); const workspace = mkdtempSync(join(tmpdir(), 'codesign-harness-doc-')); From 36727de1c0db322f39193ceeaef081f6de32495b Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 13:50:20 +0800 Subject: [PATCH 8/9] fix(core): cut preview warnings to the five the result declares PreviewResult.warnings declares at most five entries, but trimPreviewResult cut only the console errors, asset errors and layout issues. The layout notes can already fill the list, and a late browser shutdown adds one more, so a result could carry six. It is cut like the other arrays now. Refs #493 --- packages/core/src/tools/preview.test.ts | 14 ++++++++++++++ packages/core/src/tools/preview.ts | 8 +++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tools/preview.test.ts b/packages/core/src/tools/preview.test.ts index 6c5a66a1e..20df600eb 100644 --- a/packages/core/src/tools/preview.test.ts +++ b/packages/core/src/tools/preview.test.ts @@ -12,6 +12,7 @@ import { MAX_CONSOLE_ENTRIES, MAX_LAYOUT_ISSUES, MAX_PREVIEW_STEPS, + MAX_PREVIEW_WARNINGS, makePreviewTool, type PreviewInput, type PreviewResult, @@ -399,6 +400,19 @@ describe('makePreviewTool', () => { expect(result.details.layoutIssues).toEqual(issues.slice(0, MAX_LAYOUT_ISSUES)); }); + it('caps warnings to the documented budget', async () => { + const warnings = Array.from( + { length: MAX_PREVIEW_WARNINGS + 1 }, + (_, index) => `Note ${index}`, + ); + const runPreview = vi.fn().mockResolvedValue(cannedResult({ warnings })); + + const result = await makePreviewTool(runPreview).execute('call', { path: 'App.jsx' }); + + expect(MAX_PREVIEW_WARNINGS).toBe(5); + expect(result.details.warnings).toEqual(warnings.slice(0, MAX_PREVIEW_WARNINGS)); + }); + it('leaves the field out when the host did not measure the layout', async () => { const runPreview = vi.fn().mockResolvedValue(cannedResult()); diff --git a/packages/core/src/tools/preview.ts b/packages/core/src/tools/preview.ts index 5bcc15b09..44fba6724 100644 --- a/packages/core/src/tools/preview.ts +++ b/packages/core/src/tools/preview.ts @@ -123,6 +123,9 @@ export const AssetError = Type.Object({ type: Type.Optional(Type.String()), }); +/** Warnings a preview result carries; the ones past it are cut. */ +export const MAX_PREVIEW_WARNINGS = 5; + /** Layout and accessibility findings are advice: they never change `ok` or the `done` status. */ export const MAX_LAYOUT_ISSUES = 20; @@ -160,7 +163,7 @@ export const PreviewResult = Type.Object({ loadMs: Type.Number(), }), reason: Type.Optional(Type.String()), - warnings: Type.Optional(Type.Array(Type.String(), { maxItems: 5 })), + warnings: Type.Optional(Type.Array(Type.String(), { maxItems: MAX_PREVIEW_WARNINGS })), layoutIssues: Type.Optional(Type.Array(LayoutIssue, { maxItems: MAX_LAYOUT_ISSUES })), visibleText: Type.Optional(Type.String()), steps: Type.Optional( @@ -186,6 +189,9 @@ export function trimPreviewResult(result: PreviewResult): PreviewResult { ...result, consoleErrors: result.consoleErrors.slice(0, MAX_CONSOLE_ENTRIES), assetErrors: result.assetErrors.slice(0, MAX_ASSET_ERRORS), + ...(result.warnings !== undefined + ? { warnings: result.warnings.slice(0, MAX_PREVIEW_WARNINGS) } + : {}), ...(result.layoutIssues !== undefined ? { layoutIssues: result.layoutIssues.slice(0, MAX_LAYOUT_ISSUES) } : {}), From ea14719624a9b3e23338cf96f2f126666a2a614e Mon Sep 17 00:00:00 2001 From: lyy <3483820747@qq.com> Date: Fri, 16 Oct 2026 13:50:21 +0800 Subject: [PATCH 9/9] fix(desktop): name the side that clipped text sticks out of The probe flagged text that sticks out of a clipping box on the left or top, but a clipped-text sample held only the right and bottom edges, and the analysis chose the bottom message whenever the text did not reach past the right edge. Text pushed off the left or top by a negative text-indent, a negative margin or a flex alignment was reported as "reaches N from the top but the box ends at M" with N below M, which names the wrong side. A sample now records the side that sticks out the most, with the text's edge and the box's edge on that side, and the message describes that side. The right edge no longer wins by default either: a 1 px overhang on the right used to choose the right-edge message even when the bottom was cut by 30 px. Refs #493 --- apps/desktop/src/main/layout-analysis.test.ts | 28 +++++++++-- apps/desktop/src/main/layout-analysis.ts | 37 +++++++++------ .../src/main/layout-probe.browser.test.ts | 46 +++++++++++++++++++ apps/desktop/src/main/layout-probe.test.ts | 7 ++- apps/desktop/src/main/layout-probe.ts | 27 ++++++----- apps/desktop/src/main/layout-samples.ts | 12 +++-- 6 files changed, 120 insertions(+), 37 deletions(-) diff --git a/apps/desktop/src/main/layout-analysis.test.ts b/apps/desktop/src/main/layout-analysis.test.ts index a8fe009b3..fa23624e1 100644 --- a/apps/desktop/src/main/layout-analysis.test.ts +++ b/apps/desktop/src/main/layout-analysis.test.ts @@ -158,8 +158,10 @@ describe('analyzeLayoutSamples: overflow and clipped text', () => { viewportWidth: 1280, documentWidth: 1280, clipped: [ - { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, - { selector: 'p.teaser', textRight: 200, boxRight: 300, textBottom: 190, boxBottom: 120 }, + { selector: 'div#card', side: 'right', textEdge: 340, boxEdge: 300 }, + { selector: 'p.teaser', side: 'bottom', textEdge: 190, boxEdge: 120 }, + { selector: 'span.badge', side: 'left', textEdge: 20, boxEdge: 40 }, + { selector: 'h2.title', side: 'top', textEdge: 8, boxEdge: 30 }, ], }), ); @@ -183,6 +185,24 @@ describe('analyzeLayoutSamples: overflow and clipped text', () => { measured: 190, limit: 120, }, + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'span.badge', + message: + 'span.badge hides part of its text: the text starts at 20 px but the box starts at 40 px (viewport 1,280 px).', + measured: 20, + limit: 40, + }, + { + check: 'clipped-text', + viewportWidth: 1280, + selector: 'h2.title', + message: + 'h2.title hides part of its text: the text starts at 8 px from the top but the box starts at 30 px (viewport 1,280 px).', + measured: 8, + limit: 30, + }, ]); }); }); @@ -312,9 +332,7 @@ describe('analyzeLayoutSamples: small targets and names', () => { samples({ documentWidth: 600, overflowing: [{ selector: 'div.wide', right: 600 }], - clipped: [ - { selector: 'div#card', textRight: 340, boxRight: 300, textBottom: 60, boxBottom: 80 }, - ], + clipped: [{ selector: 'div#card', side: 'right', textEdge: 340, boxEdge: 300 }], textGroups: [group()], smallTargets: [{ selector: 'button.close', width: 16, height: 16 }], unnamedControls: [{ selector: 'button.menu', kind: 'button' }], diff --git a/apps/desktop/src/main/layout-analysis.ts b/apps/desktop/src/main/layout-analysis.ts index 901db0c52..6c6346715 100644 --- a/apps/desktop/src/main/layout-analysis.ts +++ b/apps/desktop/src/main/layout-analysis.ts @@ -5,6 +5,7 @@ import type { LayoutIssue } from '@open-codesign/core'; import { + type ClippedText, LAYOUT_PROBE_LIMITS, type LayoutSamples, MIN_CONTRAST_RATIO, @@ -95,21 +96,31 @@ function overflowIssues(samples: LayoutSamples, tolerance: number): LayoutIssue[ })); } +function clippedEdges({ side, textEdge, boxEdge }: ClippedText): string { + const text = formatPx(textEdge); + const box = formatPx(boxEdge); + switch (side) { + case 'right': + return `the text reaches ${text} but the box ends at ${box}`; + case 'bottom': + return `the text reaches ${text} from the top but the box ends at ${box}`; + case 'left': + return `the text starts at ${text} but the box starts at ${box}`; + case 'top': + return `the text starts at ${text} from the top but the box starts at ${box}`; + } +} + function clippedIssues(samples: LayoutSamples): LayoutIssue[] { const { viewportWidth } = samples; - return samples.clipped.map((text) => { - const horizontal = text.textRight > text.boxRight; - return { - check: 'clipped-text', - viewportWidth, - selector: text.selector, - message: horizontal - ? `${text.selector} hides part of its text: the text reaches ${formatPx(text.textRight)} but the box ends at ${formatPx(text.boxRight)} (viewport ${formatPx(viewportWidth)}).` - : `${text.selector} hides part of its text: the text reaches ${formatPx(text.textBottom)} from the top but the box ends at ${formatPx(text.boxBottom)} (viewport ${formatPx(viewportWidth)}).`, - measured: horizontal ? text.textRight : text.textBottom, - limit: horizontal ? text.boxRight : text.boxBottom, - }; - }); + return samples.clipped.map((text) => ({ + check: 'clipped-text', + viewportWidth, + selector: text.selector, + message: `${text.selector} hides part of its text: ${clippedEdges(text)} (viewport ${formatPx(viewportWidth)}).`, + measured: text.textEdge, + limit: text.boxEdge, + })); } function contrastIssues(samples: LayoutSamples): LayoutIssue[] { diff --git a/apps/desktop/src/main/layout-probe.browser.test.ts b/apps/desktop/src/main/layout-probe.browser.test.ts index 867ca5c1c..3e533b48a 100644 --- a/apps/desktop/src/main/layout-probe.browser.test.ts +++ b/apps/desktop/src/main/layout-probe.browser.test.ts @@ -117,6 +117,44 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { expect(issues[0]?.measured).toBeGreaterThan(issues[0]?.limit ?? Number.POSITIVE_INFINITY); }); + it.each([ + [ + 'right', + `
${LONG_TEXT}
`, + 'the text reaches', + 1, + ], + [ + 'bottom', + `
${LONG_TEXT}
`, + 'from the top but the box ends at', + 1, + ], + [ + 'left', + '
Indented off the left edge
', + 'the text starts at', + -1, + ], + [ + 'left, when the text also sticks out of the right by less', + `
${'x'.repeat(50)}
`, + 'the text starts at', + -1, + ], + [ + 'top', + '
Pushed above the top edge
', + 'from the top but the box starts at', + -1, + ], + ])('names the %s side when text sticks out of it', async (_side, markup, wording, direction) => { + const [issue] = await clippedAt(markup); + + expect(issue?.message).toContain(wording); + expect(Math.sign((issue?.measured ?? 0) - (issue?.limit ?? 0))).toBe(direction); + }); + it('ends the path at an element with an id and otherwise keeps three levels', async () => { const issues = await clippedAt( `

${LONG_TEXT}

`, @@ -129,6 +167,14 @@ describe.skipIf(!chrome)('layout checks in system Chrome', () => { expect(unnamed[0]?.selector).toBe('… > div > div > p'); }); + it('reports text that sticks out by more than 2 px and not by 2 px', async () => { + const indented = (px: number) => + `
Indented
`; + + expect(await clippedAt(indented(2))).toEqual([]); + expect(await clippedAt(indented(3))).toHaveLength(2); + }); + it('leaves text alone that fits its box', async () => { expect( await clippedAt('
Short
'), diff --git a/apps/desktop/src/main/layout-probe.test.ts b/apps/desktop/src/main/layout-probe.test.ts index 3c6c21a23..60383a390 100644 --- a/apps/desktop/src/main/layout-probe.test.ts +++ b/apps/desktop/src/main/layout-probe.test.ts @@ -149,10 +149,9 @@ describe('inspectLayout', () => { const clipped = (count: number) => Array.from({ length: count }, (_, index) => ({ selector: `p.item-${index}`, - textRight: 400, - boxRight: 300, - textBottom: 20, - boxBottom: 40, + side: 'right' as const, + textEdge: 400, + boxEdge: 300, })); it('lists the budget, one check at a time, and says how many there were', async () => { diff --git a/apps/desktop/src/main/layout-probe.ts b/apps/desktop/src/main/layout-probe.ts index e18e57bc0..20506e5f2 100644 --- a/apps/desktop/src/main/layout-probe.ts +++ b/apps/desktop/src/main/layout-probe.ts @@ -277,20 +277,25 @@ export function collectLayoutSamples(limits: LayoutProbeLimits): LayoutSamples { range.selectNodeContents(child); const text = range.getBoundingClientRect(); if (text.width < 1 || text.height < 1) continue; - const tolerance = limits.clipTolerance; - if ( - text.right > box.right + tolerance || - text.bottom > box.bottom + tolerance || - text.left < box.left - tolerance || - text.top < box.top - tolerance - ) { + const sides = [ + { side: 'right', beyond: text.right - box.right, text: text.right, box: box.right }, + { + side: 'bottom', + beyond: text.bottom - box.bottom, + text: text.bottom, + box: box.bottom, + }, + { side: 'left', beyond: box.left - text.left, text: text.left, box: box.left }, + { side: 'top', beyond: box.top - text.top, text: text.top, box: box.top }, + ] as const; + const worst = sides.reduce((most, next) => (next.beyond > most.beyond ? next : most)); + if (worst.beyond > limits.clipTolerance) { if (samples.clipped.length < limits.maxClippedTexts) { samples.clipped.push({ selector: selectorOf(element), - textRight: Math.round(text.right), - boxRight: Math.round(box.right), - textBottom: Math.round(text.bottom), - boxBottom: Math.round(box.bottom), + side: worst.side, + textEdge: Math.round(worst.text), + boxEdge: Math.round(worst.box), }); } else { samples.omitted += 1; diff --git a/apps/desktop/src/main/layout-samples.ts b/apps/desktop/src/main/layout-samples.ts index 2fff24ac1..b8ed801e3 100644 --- a/apps/desktop/src/main/layout-samples.ts +++ b/apps/desktop/src/main/layout-samples.ts @@ -51,12 +51,16 @@ export interface OverflowingElement { right: number; } +/** The side of a clipping box that its text sticks out of the most. */ +export type ClippedSide = 'right' | 'bottom' | 'left' | 'top'; + export interface ClippedText { selector: string; - textRight: number; - boxRight: number; - textBottom: number; - boxBottom: number; + side: ClippedSide; + /** The text's edge on that side, in CSS px from the top left of the viewport. */ + textEdge: number; + /** The box's edge on that side, in the same px. */ + boxEdge: number; } /** Text elements that share a colour, the backgrounds behind it and a size class. */