-
Notifications
You must be signed in to change notification settings - Fork 31
fix: distinguish WCAG violations from best practices in issue output #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
| */ | ||
| function getRuleTypeLabel(ruleType?: string): { heading: string; badge: string; isWcag: boolean } { | ||
| if (ruleType === 'best-practice') { | ||
| return { | ||
| heading: 'Best Practice Recommendation', | ||
| badge: '\U0001F4A1 Best Practice', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => | ||
|
|
@@ -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}` : ''} | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
|
||||||||||||||||||||||||||||
| // 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') { |
There was a problem hiding this comment.
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.
| 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
|
||||||||||||||||||||||||||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'))) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor (micro-optimization opportunity); we're iterating over 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
|
||
|
|
||
| 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, '''), | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| } | ||
|
|
||
| export type Cookie = { | ||
|
|
||
| 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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for fixing! |
||
| console.log('testing plugin load using js file') | ||
| } | ||
|
|
||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc for
getRuleTypeLabelsays it determines WCAG vs best-practice, but the function also handlesexperimental. 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: