Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion .github/actions/file/src/generateIssueBody.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function generateIssueBody(finding: Finding, screenshotRepo: string): str
- [ ] This PR MUST NOT introduce any new accessibility issues or regressions.`

const body = `## 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}.
${describeWhat(finding)}

${screenshotSection ?? ''}
To fix this, ${finding.solutionShort}.
Expand All @@ -36,3 +36,24 @@ ${acceptanceCriteria}

return body
}

function describeWhat(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}`
Comment thread
kzhou314 marked this conversation as resolved.
}

return `An accessibility scan found an issue on ${finding.url} ${reason}`
}
6 changes: 6 additions & 0 deletions .github/actions/file/src/types.d.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down
5 changes: 5 additions & 0 deletions .github/actions/file/src/updateFilingsWithNewFindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ function getFilingKey(filing: ResolvedFiling | RepeatedFiling): string {
}

function getFindingKey(finding: Finding): string {
// 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}`
}
Expand Down
19 changes: 19 additions & 0 deletions .github/actions/file/tests/generateIssueBody.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,23 @@ describe('generateIssueBody', () => {
expect(body).toContain(`found an issue on ${findingWithEmptyOptionalFields.url}`)
expect(body).not.toContain('flagged the element')
})

it('lists every node when the finding carries multiple elements', () => {
const body = generateIssueBody(
{
...baseFinding,
html: '<span>first</span>',
nodes: [
{html: '<span>first</span>', target: 'span.first'},
{html: '<a href="x">link</a>', target: 'a.link'},
],
},
'github/accessibility-scanner',
)

expect(body).toContain('flagged 2 elements')
expect(body).toContain('- `<span>first</span>` (selector: `span.first`)')
expect(body).toContain('- `<a href="x">link</a>` (selector: `a.link`)')
expect(body).not.toContain('flagged the element')
})
})
61 changes: 61 additions & 0 deletions .github/actions/file/tests/updateFilingsWithNewFindings.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
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: '<span class="post-meta">old markup</span>',
nodes: [{html: '<span class="post-meta">old markup</span>', 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: '<span class="post-meta">old markup wrapped in a new container</span>',
nodes: [
{html: '<span class="post-meta">old markup wrapped in a new container</span>', 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: '<img src="logo.png">',
nodes: [{html: '<img src="logo.png">', 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')
})
})
5 changes: 5 additions & 0 deletions .github/actions/find/src/findForUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,15 @@ 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',
url,
html: violation.nodes[0].html.replace(/'/g, '&apos;'),
nodes: violation.nodes.map(node => ({
html: node.html.replace(/'/g, '&apos;'),
target: node.target.map(part => (Array.isArray(part) ? part.join(' ') : part)).join(' '),
})),
problemShort: violation.help.toLowerCase().replace(/'/g, '&apos;'),
problemUrl: violation.helpUrl.replace(/'/g, '&apos;'),
ruleId: violation.id,
Expand Down
6 changes: 6 additions & 0 deletions .github/actions/find/src/types.d.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
export type FindingNode = {
html: string
target?: string
}

export type Finding = {
scannerType: string
url: string
html?: string
nodes?: FindingNode[]
problemShort: string
problemUrl: string
solutionShort: string
Expand Down
28 changes: 28 additions & 0 deletions .github/actions/find/tests/findForUrl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,32 @@ describe('findForUrl', () => {
expect(loadedPlugins[1].default).toHaveBeenCalledTimes(0)
})
})

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',
nodes: [
{html: '<span>one</span>', target: ['span.one'], failureSummary: 'Fix any of the following:'},
{html: '<span>two</span>', 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')
Comment thread
kzhou314 marked this conversation as resolved.

expect(findings).toHaveLength(1)
expect(findings[0].html).toBe('<span>one</span>')
expect(findings[0].nodes).toEqual([
{html: '<span>one</span>', target: 'span.one'},
{html: '<span>two</span>', target: 'div span.two'},
])
})
})
5 changes: 4 additions & 1 deletion tests/site-with-errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
6 changes: 6 additions & 0 deletions tests/types.d.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down