diff --git a/.github/actions/file/src/generateIssueBody.ts b/.github/actions/file/src/generateIssueBody.ts index c67024d..ad703d5 100644 --- a/.github/actions/file/src/generateIssueBody.ts +++ b/.github/actions/file/src/generateIssueBody.ts @@ -50,7 +50,7 @@ ${standardsLine} - [ ] This PR MUST NOT introduce any new accessibility issues or regressions.` const body = `${categoryNotice}## What -An accessibility scan ${finding.html ? `flagged the element \`${finding.html}\`` : `found an issue on ${finding.url}`} because ${finding.problemShort}. Learn more about why this was flagged by visiting ${finding.problemUrl}. +${describeFinding(finding)} ${screenshotSection ?? ''} To fix this, ${finding.solutionShort}. @@ -61,3 +61,24 @@ ${acceptanceCriteria} return body } + +function describeFinding(finding: Finding): string { + const reason = `because ${finding.problemShort}. Learn more about why this was flagged by visiting ${finding.problemUrl}.` + + // Axe carries every failing element; list them all, not just the first. + if (finding.nodes && finding.nodes.length > 0) { + const count = finding.nodes.length + const subject = count === 1 ? 'an element' : `${count} elements` + const elementList = finding.nodes + .map(node => `- \`${node.html}\`${node.target ? ` (selector: \`${node.target}\`)` : ''}`) + .join('\n') + const heading = count === 1 ? 'The following element needs' : 'The following elements need' + return `An accessibility scan flagged ${subject} on ${finding.url} ${reason}\n\n${heading} attention:\n\n${elementList}` + } + + if (finding.html) { + return `An accessibility scan flagged the element \`${finding.html}\` ${reason}` + } + + return `An accessibility scan found an issue on ${finding.url} ${reason}` +} diff --git a/.github/actions/file/src/types.d.ts b/.github/actions/file/src/types.d.ts index ba36ecc..08d8573 100644 --- a/.github/actions/file/src/types.d.ts +++ b/.github/actions/file/src/types.d.ts @@ -1,3 +1,8 @@ +export type FindingNode = { + html: string + target?: string +} + export type FindingCategory = 'wcag' | 'best-practice' | 'experimental' export type Finding = { @@ -6,6 +11,7 @@ export type Finding = { ruleId?: string url: string html?: string + nodes?: FindingNode[] problemShort: string problemUrl: string solutionShort: string diff --git a/.github/actions/file/src/updateFilingsWithNewFindings.ts b/.github/actions/file/src/updateFilingsWithNewFindings.ts index 9c00be5..e322f4c 100644 --- a/.github/actions/file/src/updateFilingsWithNewFindings.ts +++ b/.github/actions/file/src/updateFilingsWithNewFindings.ts @@ -17,6 +17,11 @@ function getFindingKey(finding: Finding, groupBy: GroupBy): string { return `${finding.url};${rule}` case 'finding': default: + // Axe groups every failing element under one rule, so key on the rule, not the + // element's HTML, which shifts with DOM changes and re-files tracked issues. + if (finding.scannerType === 'axe' && finding.ruleId) { + return `${finding.url};axe;${finding.ruleId}` + } if (finding.ruleId && finding.html) { return `${finding.url};${finding.ruleId};${finding.html}` } diff --git a/.github/actions/file/tests/generateIssueBody.test.ts b/.github/actions/file/tests/generateIssueBody.test.ts index 8341438..60a06db 100644 --- a/.github/actions/file/tests/generateIssueBody.test.ts +++ b/.github/actions/file/tests/generateIssueBody.test.ts @@ -78,6 +78,25 @@ describe('generateIssueBody', () => { expect(body).not.toContain('flagged the element') }) + it('lists every node when the finding carries multiple elements', () => { + const body = generateIssueBody( + { + ...baseFinding, + html: 'first', + nodes: [ + {html: 'first', target: 'span.first'}, + {html: 'link', target: 'a.link'}, + ], + }, + 'github/accessibility-scanner', + ) + + expect(body).toContain('flagged 2 elements') + expect(body).toContain('- `first` (selector: `span.first`)') + expect(body).toContain('- `link` (selector: `a.link`)') + expect(body).not.toContain('flagged the element') + }) + it('omits the Occurrences section for a single finding', () => { const body = generateIssueBody(baseFinding, 'github/accessibility-scanner') diff --git a/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts b/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts index 008e186..e5c2aef 100644 --- a/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts +++ b/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts @@ -1,5 +1,64 @@ import {describe, it, expect} from 'vitest' import {updateFilingsWithNewFindings} from '../src/updateFilingsWithNewFindings.ts' +import type {Finding, RepeatedFiling} from '../src/types.d.ts' + +const cachedFinding: Finding = { + scannerType: 'axe', + ruleId: 'color-contrast', + url: 'https://example.com/', + html: 'old markup', + nodes: [{html: 'old markup', target: 'span.post-meta'}], + problemShort: 'elements must meet minimum color contrast ratio thresholds', + problemUrl: 'https://dequeuniversity.com/rules/axe/4.10/color-contrast?application=playwright', + solutionShort: 'ensure the contrast meets WCAG thresholds', +} + +const cachedFiling: RepeatedFiling = { + issue: { + id: 1, + nodeId: 'node-1', + url: 'https://github.com/org/repo/issues/1', + title: 'Accessibility issue: color contrast on /', + }, + findings: [cachedFinding], +} + +describe('updateFilingsWithNewFindings', () => { + it('re-matches an axe finding to its existing issue after the element HTML shifts', () => { + // Same rule and page, but the element's markup shifted; should still map to issue #1. + const shiftedFinding: Finding = { + ...cachedFinding, + html: 'old markup wrapped in a new container', + nodes: [ + {html: 'old markup wrapped in a new container', target: 'div > span.post-meta'}, + ], + } + + const result = updateFilingsWithNewFindings([cachedFiling], [shiftedFinding]) + + expect(result).toHaveLength(1) + const filing = result[0] as RepeatedFiling + expect(filing.issue.url).toBe('https://github.com/org/repo/issues/1') + expect(filing.findings).toHaveLength(1) + expect(filing.findings[0].html).toContain('new container') + }) + + it('files a new issue when a different rule fails on the same page', () => { + const differentRule: Finding = { + ...cachedFinding, + ruleId: 'image-alt', + html: '', + nodes: [{html: '', target: 'img'}], + } + + const result = updateFilingsWithNewFindings([cachedFiling], [differentRule]) + + expect(result).toHaveLength(2) + const newFilings = result.filter(filing => filing.issue === undefined) + expect(newFilings).toHaveLength(1) + expect(newFilings[0].findings[0].ruleId).toBe('image-alt') + }) +}) const colorContrastFinding = (url: string, html: string) => ({ scannerType: 'axe', @@ -18,10 +77,12 @@ describe('updateFilingsWithNewFindings — group_by', () => { colorContrastFinding('https://example.com/b', '3'), ] - it("defaults to 'finding': one filing per individual violation", () => { + it("defaults to 'finding': axe findings collapse by rule and URL", () => { const result = updateFilingsWithNewFindings([], findings) - expect(result).toHaveLength(3) - for (const filing of result) expect(filing.findings).toHaveLength(1) + // /a color-contrast (x2) share one filing; /b color-contrast is its own. + expect(result).toHaveLength(2) + const counts = result.map(f => f.findings.length).sort() + expect(counts).toEqual([1, 2]) }) it("'rule': collapses all occurrences of a rule into a single filing", () => { @@ -80,7 +141,7 @@ describe('updateFilingsWithNewFindings — group_by', () => { expect(result).toHaveLength(2) }) - it("'finding' (default) preserves the original 1:1 behavior with cached filings", () => { + it("'finding' (default) re-matches axe findings to a cached filing by rule and URL", () => { const cached = [ { issue: { @@ -93,9 +154,9 @@ describe('updateFilingsWithNewFindings — group_by', () => { }, ] const result = updateFilingsWithNewFindings(cached, findings) - // One repeated filing (issues/1) plus two brand-new filings. - expect(result).toHaveLength(3) + // Both /a color-contrast findings attach to issues/1; /b opens one new filing. + expect(result).toHaveLength(2) const repeated = result.find(f => f.issue?.url === 'https://github.com/org/repo/issues/1') - expect(repeated?.findings).toHaveLength(1) + expect(repeated?.findings).toHaveLength(2) }) }) diff --git a/.github/actions/find/src/findForUrl.ts b/.github/actions/find/src/findForUrl.ts index ea4f1e1..a93cd6b 100644 --- a/.github/actions/find/src/findForUrl.ts +++ b/.github/actions/find/src/findForUrl.ts @@ -85,11 +85,16 @@ async function runAxeScan({ if (rawFindings) { for (const violation of rawFindings.violations) { + // Capture every failing element, not just the first, so one issue covers the rule. await addFinding({ scannerType: 'axe', category: categorizeAxeViolation(violation.tags), url, html: violation.nodes[0].html.replace(/'/g, '''), + nodes: violation.nodes.map(node => ({ + html: node.html.replace(/'/g, '''), + target: node.target.map(part => (Array.isArray(part) ? part.join(' ') : part)).join(' '), + })), problemShort: violation.help.toLowerCase().replace(/'/g, '''), problemUrl: violation.helpUrl.replace(/'/g, '''), ruleId: violation.id, diff --git a/.github/actions/find/src/types.d.ts b/.github/actions/find/src/types.d.ts index a2f4a53..4102c32 100644 --- a/.github/actions/find/src/types.d.ts +++ b/.github/actions/find/src/types.d.ts @@ -1,3 +1,8 @@ +export type FindingNode = { + html: string + target?: string +} + export type FindingCategory = 'wcag' | 'best-practice' | 'experimental' export type Finding = { @@ -5,6 +10,7 @@ export type Finding = { category?: FindingCategory url: string html?: string + nodes?: FindingNode[] problemShort: string problemUrl: string solutionShort: string diff --git a/.github/actions/find/tests/findForUrl.test.ts b/.github/actions/find/tests/findForUrl.test.ts index 5424488..31a00b3 100644 --- a/.github/actions/find/tests/findForUrl.test.ts +++ b/.github/actions/find/tests/findForUrl.test.ts @@ -118,6 +118,35 @@ describe('findForUrl', () => { }) }) + it('captures every failing element of an axe violation as nodes', async () => { + actionInput = '' + clearAll() + + const violation = { + id: 'color-contrast', + help: 'Elements must meet minimum color contrast ratio thresholds', + helpUrl: 'https://dequeuniversity.com/rules/axe/4.10/color-contrast', + description: 'Ensure contrast meets WCAG thresholds', + tags: ['wcag2aa', 'wcag143'], + nodes: [ + {html: 'one', target: ['span.one'], failureSummary: 'Fix any of the following:'}, + {html: 'two', target: ['div', 'span.two'], failureSummary: 'Fix any of the following:'}, + ], + } + vi.mocked(AxeBuilder.prototype.analyze).mockResolvedValueOnce({ + violations: [violation], + } as unknown as axe.AxeResults) + + const findings = await findForUrl('test.com') + + expect(findings).toHaveLength(1) + expect(findings[0].html).toBe('one') + expect(findings[0].nodes).toEqual([ + {html: 'one', target: 'span.one'}, + {html: 'two', target: 'div span.two'}, + ]) + }) + describe('axe finding categorization', () => { function axeViolation(tags: string[]) { return { @@ -126,7 +155,7 @@ describe('findForUrl', () => { helpUrl: 'https://example.com', description: 'Description', tags, - nodes: [{html: '
', failureSummary: 'summary'}], + nodes: [{html: '
', target: ['div'], failureSummary: 'summary'}], } } diff --git a/tests/site-with-errors.test.ts b/tests/site-with-errors.test.ts index cf21688..4a6b00e 100644 --- a/tests/site-with-errors.test.ts +++ b/tests/site-with-errors.test.ts @@ -39,13 +39,16 @@ describe('site-with-errors', () => { it('cache has expected results', () => { const actual = results.map(({issue: {url: issueUrl}, findings}) => { - const {problemUrl, solutionLong, screenshotId, ...finding} = findings[0] + const {problemUrl, solutionLong, screenshotId, nodes, ...finding} = findings[0] // Check volatile fields for existence only expect(issueUrl).toBeDefined() expect(problemUrl).toBeDefined() // Axe-specific assertions if (finding.scannerType === 'axe') { expect(solutionLong).toBeDefined() + expect(nodes).toBeDefined() + expect(nodes!.length).toBeGreaterThan(0) + expect(nodes![0].html).toBe(finding.html) expect(problemUrl.startsWith('https://dequeuniversity.com/rules/axe/')).toBe(true) expect(problemUrl.endsWith(`/${finding.ruleId}?application=playwright`)).toBe(true) } diff --git a/tests/types.d.ts b/tests/types.d.ts index b12077a..ea72c54 100644 --- a/tests/types.d.ts +++ b/tests/types.d.ts @@ -1,8 +1,14 @@ +export type FindingNode = { + html: string + target?: string +} + export type Finding = { scannerType: string ruleId?: string url: string html?: string + nodes?: FindingNode[] problemShort: string problemUrl: string solutionShort: string