Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
24 changes: 23 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,25 @@ ${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 findings carry every element that failed the rule. List them all so the
// issue reflects the full scope of the violation, not just one example node.
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
7 changes: 7 additions & 0 deletions .github/actions/file/src/updateFilingsWithNewFindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ function getFilingKey(filing: ResolvedFiling | RepeatedFiling): string {
}

function getFindingKey(finding: Finding): string {
// Axe reports every element failing a rule under a single rule-level finding,
// so the rule itself is the stable identity. Keying on one element's exact
// markup was fragile: any DOM shift produced a new key and re-filed an issue
// that was already tracked. Axe findings therefore key on the rule, not HTML.
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')
})
})
63 changes: 63 additions & 0 deletions .github/actions/file/tests/updateFilingsWithNewFindings.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
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', () => {
// The same rule fails on the same page, but a layout change altered the
// element's surrounding markup. The finding should still map to issue #1
// rather than being treated as a brand new violation.
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')
})
})
7 changes: 7 additions & 0 deletions .github/actions/find/src/findForUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,17 @@ async function runAxeScan({

if (rawFindings) {
for (const violation of rawFindings.violations) {
// Axe groups every element that fails a rule into one violation. Capture
// all of them so a single issue can report the rule's full scope on the
// page, and so matching keys on the rule rather than one element's markup.
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