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
48 changes: 44 additions & 4 deletions .github/actions/file/src/generateIssueBody.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,34 @@
import type {Finding} from './types.d.js'

/**
* Determines if a finding is a WCAG violation or a best practice recommendation.
*/
Comment on lines +3 to +5
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc for getRuleTypeLabel says it determines WCAG vs best-practice, but the function also handles experimental. Consider updating the comment to reflect all supported rule types (wcag / best-practice / experimental) to avoid confusion.

This issue also appears in the following locations of the same file:

  • line 6
  • line 49

Copilot uses AI. Check for mistakes.
function getRuleTypeLabel(ruleType?: string): { heading: string; badge: string; isWcag: boolean } {
if (ruleType === 'best-practice') {
return {
heading: 'Best Practice Recommendation',
badge: '\U0001F4A1 Best Practice',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea of adding an emoji, however it doesn't seem to render within the > in the Markdown.

Screen shot of the issue body with the emoji written as unicode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also remove "Type: WCAG Violation/Best practice/experimental" line and just have "This is a X failure."

isWcag: false,
}
}
if (ruleType === 'experimental') {
return {
heading: 'Experimental Rule',
badge: '\U0001F9EA Experimental',
isWcag: false,
}
}
// Default to WCAG violation
return {
heading: 'WCAG Violation',
badge: '\U0001F6A8 WCAG Violation',
isWcag: true,
}
}

export function generateIssueBody(finding: Finding, screenshotRepo: string): string {
const ruleTypeLabel = getRuleTypeLabel(finding.ruleType)

const solutionLong = finding.solutionLong
?.split('\n')
.map((line: string) =>
Expand All @@ -18,15 +46,27 @@ export function generateIssueBody(finding: Finding, screenshotRepo: string): str
`
}

const ruleTypeSection = `> **Type:** ${ruleTypeLabel.badge}
>
> ${ruleTypeLabel.isWcag
? 'This is a **WCAG conformance failure**. Fixing this issue helps meet WCAG 2.1 accessibility requirements.'
: 'This is a **best practice recommendation**, not a WCAG conformance failure. Fixing it improves accessibility but is not required for WCAG compliance.'
}`

const acceptanceCriteria = `## Acceptance Criteria
- [ ] The specific violation reported in this issue is no longer reproducible.
- [ ] The fix MUST meet WCAG 2.1 guidelines OR the accessibility standards specified by the repository or organization.
- [ ] A test SHOULD be added to ensure this specific violation does not regress.
- [ ] The specific issue reported in this issue is no longer reproducible.
${ruleTypeLabel.isWcag
? '- [ ] The fix MUST meet WCAG 2.1 guidelines OR the accessibility standards specified by the repository or organization.'
: '- [ ] The fix SHOULD follow recognized accessibility best practices to improve the user experience.'
}
- [ ] A test SHOULD be added to ensure this specific issue does not regress.
- [ ] This PR MUST NOT introduce any new accessibility issues or regressions.`

const body = `## What
const body = `## ${ruleTypeLabel.heading}
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}.

${ruleTypeSection}

${screenshotSection ?? ''}
To fix this, ${finding.solutionShort}.
${solutionLong ? `\nSpecifically:\n\n${solutionLong}` : ''}
Expand Down
11 changes: 11 additions & 0 deletions .github/actions/file/src/openIssue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ export async function openIssue(octokit: Octokit, repoWithOwner: string, finding
const repo = repoWithOwner.split('/')[1]

const labels = [`${finding.scannerType}-scanning-issue`]

// Add rule type label to distinguish WCAG violations from best practices
if (finding.ruleType === 'best-practice') {
labels.push('best-practice')
} else if (finding.ruleType === 'experimental') {
labels.push('experimental')
} else {
// Default to wcag for any WCAG-tagged rule
Comment on lines +26 to +32
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic adds wcag-violation for any finding whose ruleType is not exactly best-practice or experimental (including undefined or unexpected values). Since Finding.ruleType is optional and can be omitted by plugins, this can incorrectly label non-WCAG findings as WCAG violations. Consider only adding wcag-violation when finding.ruleType === 'wcag', and either omit the type label or add an explicit "unknown" label when ruleType is missing.

Suggested change
// Add rule type label to distinguish WCAG violations from best practices
if (finding.ruleType === 'best-practice') {
labels.push('best-practice')
} else if (finding.ruleType === 'experimental') {
labels.push('experimental')
} else {
// Default to wcag for any WCAG-tagged rule
// Add a rule type label only for explicitly recognized rule types
if (finding.ruleType === 'best-practice') {
labels.push('best-practice')
} else if (finding.ruleType === 'experimental') {
labels.push('experimental')
} else if (finding.ruleType === 'wcag') {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this, as a default we don't want to have anything if ruleType is not passed in. One example is our custom reflow scan; this does map to a WCAG violation, however, if we add more built-in scans outside of Axe over time, this won't be relevant. Same with users who create their own custom plugins.

labels.push('wcag-violation')
}

// Only include a ruleId label when it's defined
if (finding.ruleId) {
labels.push(`${finding.scannerType} rule: ${finding.ruleId}`)
Expand Down
2 changes: 2 additions & 0 deletions .github/actions/file/src/types.d.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
export type Finding = {
scannerType: string
ruleId?: string
/** Distinguishes WCAG violations from best practices. One of: "wcag", "best-practice", "experimental" */
ruleType?: string
Comment on lines 1 to +5
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruleType is documented as having only three allowed values, but is typed as a generic string. Consider using a string-literal union (e.g., 'wcag' | 'best-practice' | 'experimental') to make openIssue() / generateIssueBody() logic exhaustive and prevent accidental mislabeling due to typos or unexpected values.

Suggested change
export type Finding = {
scannerType: string
ruleId?: string
/** Distinguishes WCAG violations from best practices. One of: "wcag", "best-practice", "experimental" */
ruleType?: string
export type FindingRuleType = 'wcag' | 'best-practice' | 'experimental'
export type Finding = {
scannerType: string
ruleId?: string
/** Distinguishes WCAG violations from best practices. One of: "wcag", "best-practice", "experimental" */
ruleType?: FindingRuleType

Copilot uses AI. Check for mistakes.
url: string
html?: string
problemShort: string
Expand Down
5 changes: 3 additions & 2 deletions .github/actions/file/tests/generateIssueBody.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ describe('generateIssueBody', () => {
it('includes acceptance criteria and omits the Specifically section when solutionLong is missing', () => {
const body = generateIssueBody(baseFinding, 'github/accessibility-scanner')

expect(body).toContain('## What')
expect(body).toContain('## WCAG Violation')
expect(body).toContain('\U0001F6A8 WCAG Violation')
expect(body).toContain('## Acceptance Criteria')
expect(body).toContain('The specific violation reported in this issue is no longer reproducible.')
expect(body).toContain('The specific issue reported in this issue is no longer reproducible.')
expect(body).not.toContain('Specifically:')
})

Expand Down
26 changes: 25 additions & 1 deletion .github/actions/file/tests/openIssue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,31 @@ describe('openIssue', () => {
expect(octokit.request).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
labels: ['axe-scanning-issue', 'axe rule: color-contrast'],
labels: ['axe-scanning-issue', 'wcag-violation', 'axe rule: color-contrast'],
}),
)
})

it('labels best-practice findings correctly', async () => {
const octokit = mockOctokit()
await openIssue(octokit, 'org/repo', {...baseFinding, ruleType: 'best-practice'})

expect(octokit.request).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
labels: ['axe-scanning-issue', 'best-practice', 'axe rule: color-contrast'],
}),
)
})

it('labels experimental findings correctly', async () => {
const octokit = mockOctokit()
await openIssue(octokit, 'org/repo', {...baseFinding, ruleType: 'experimental'})

expect(octokit.request).toHaveBeenCalledWith(
expect.any(String),
expect.objectContaining({
labels: ['axe-scanning-issue', 'experimental', 'axe rule: color-contrast'],
}),
)
})
Expand Down
12 changes: 12 additions & 0 deletions .github/actions/find/src/findForUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,25 @@ async function runAxeScan({

if (rawFindings) {
for (const violation of rawFindings.violations) {
// Determine rule type from axe-core tags
const tags = violation.tags ?? []
let ruleType: string | undefined
if (tags.some((tag: string) => tag.startsWith('wcag'))) {
Copy link
Copy Markdown
Contributor

@abdulahmad307 abdulahmad307 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor (micro-optimization opportunity); we're iterating over tags in each condition here (I'm not sure how many tags there are in a violation in general - if the list is small, then this is prob not a big deal). We could run 1 loop, find all the info we need (stored in bools), then check against the bools in the conditions instead.

I suppose this can add up (even if the tags list is small) depending on how many pages we're scanning, how many violations are in each page, etc...

ruleType = 'wcag'
} else if (tags.includes('best-practice')) {
ruleType = 'best-practice'
} else if (tags.includes('experimental')) {
ruleType = 'experimental'
}
Comment on lines +82 to +91
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruleType is currently inferred into a string | undefined. Once Finding.ruleType is narrowed to a string-literal union, it would be good to type this local as the same union (or Finding['ruleType']) to avoid accidental values and keep the inference logic aligned with the Finding contract.

Copilot uses AI. Check for mistakes.

await addFinding({
scannerType: 'axe',
url,
html: violation.nodes[0].html.replace(/'/g, '''),
problemShort: violation.help.toLowerCase().replace(/'/g, '''),
problemUrl: violation.helpUrl.replace(/'/g, '''),
ruleId: violation.id,
ruleType,
solutionShort: violation.description.toLowerCase().replace(/'/g, '''),
solutionLong: violation.nodes[0].failureSummary?.replace(/'/g, '''),
})
Expand Down
2 changes: 2 additions & 0 deletions .github/actions/find/src/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export type Finding = {
solutionLong?: string
screenshotId?: string
ruleId?: string
/** Distinguishes WCAG violations from best practices. One of: "wcag", "best-practice", "experimental" */
ruleType?: string
Comment on lines +11 to +12
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruleType is documented as having only three allowed values, but is typed as a generic string. Consider introducing a RuleType = 'wcag' | 'best-practice' | 'experimental' union (and using it for Finding.ruleType) so typos/unhandled values are caught at compile time and downstream logic (labels/body text) can be made exhaustive.

Copilot uses AI. Check for mistakes.
}

export type Cookie = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// - this exist as a test to verify that loading plugins
// via js files still works and there are no regressions

export default async function TestJsFilePluginLoad({ page, addFinding } = {}) {
export default async function TestJsFilePluginLoad() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing!

console.log('testing plugin load using js file')
}

Expand Down
2 changes: 1 addition & 1 deletion eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import tseslint from "typescript-eslint";
import prettierConfig from "eslint-config-prettier";

export default tseslint.config(...tseslint.configs.recommended, prettierConfig, {
files: ["**/*.ts"],
files: ["**/*.ts", "**/*.js"],
rules: {
"@typescript-eslint/no-unused-vars": [
"error",
Expand Down
Loading