From 752ce26cf1c44cd05d83cf17dd40203a70820980 Mon Sep 17 00:00:00 2001 From: Kevin Heis Date: Wed, 5 Nov 2025 11:37:59 -0800 Subject: [PATCH] Auto-generate aria-labels for octicons and remove linter rule (#58349) --- .../contributing/content-linter-rules.md | 1 - src/article-api/tests/article-body.ts | 69 ++++++++ src/content-linter/lib/linting-rules/index.ts | 3 +- .../lib/linting-rules/octicon-aria-labels.ts | 58 ------- src/content-linter/style/github-docs.ts | 7 +- .../tests/unit/octicon-aria-labels.ts | 155 ------------------ src/content-render/liquid/octicon.ts | 12 +- src/content-render/tests/octicon.ts | 30 ++++ .../start-your-journey/hello-world.md | 14 ++ 9 files changed, 126 insertions(+), 223 deletions(-) create mode 100644 src/article-api/tests/article-body.ts delete mode 100644 src/content-linter/lib/linting-rules/octicon-aria-labels.ts delete mode 100644 src/content-linter/tests/unit/octicon-aria-labels.ts diff --git a/data/reusables/contributing/content-linter-rules.md b/data/reusables/contributing/content-linter-rules.md index a2555eb777..243d60f22a 100644 --- a/data/reusables/contributing/content-linter-rules.md +++ b/data/reusables/contributing/content-linter-rules.md @@ -52,7 +52,6 @@ | GHD041 | third-party-action-pinning | Code examples that use third-party actions must always pin to a full length commit SHA | error | feature, actions | | GHD042 | liquid-tag-whitespace | Liquid tags should start and end with one whitespace. Liquid tag arguments should be separated by only one whitespace. | error | liquid, format | | GHD043 | link-quotation | Internal link titles must not be surrounded by quotations | error | links, url | -| GHD044 | octicon-aria-labels | Octicons should always have an aria-label attribute even if aria-hidden. | warning | accessibility, octicons | | GHD045 | code-annotation-comment-spacing | Code comments in annotation blocks must have exactly one space after the comment character(s) | warning | code, comments, annotate, spacing | | GHD046 | outdated-release-phase-terminology | Outdated release phase terminology should be replaced with current GitHub terminology | warning | terminology, consistency, release-phases | | GHD047 | table-column-integrity | Tables must have consistent column counts across all rows | warning | tables, accessibility, formatting | diff --git a/src/article-api/tests/article-body.ts b/src/article-api/tests/article-body.ts new file mode 100644 index 0000000000..dadb514f61 --- /dev/null +++ b/src/article-api/tests/article-body.ts @@ -0,0 +1,69 @@ +import { beforeAll, describe, expect, test } from 'vitest' + +import { get } from '@/tests/helpers/e2etest' + +const makeURL = (pathname: string): string => + `/api/article/body?${new URLSearchParams({ pathname })}` + +describe('article body api', () => { + beforeAll(() => { + // If you didn't set the `ROOT` variable, the tests will fail rather + // cryptically. So as a warning for engineers running these tests, + // alert in case it was accidentally forgotten. + if (!process.env.ROOT) { + console.warn( + 'WARNING: The article body tests require the ROOT environment variable to be set to the fixture root', + ) + } + }) + + test('happy path', async () => { + const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) + expect(res.statusCode).toBe(200) + expect(res.body).toContain('## Introduction') + expect(res.body).toContain('This is just a test.') + expect(res.headers['content-type']).toContain('text/markdown') + }) + + test('octicons auto-generate aria-labels', async () => { + const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) + expect(res.statusCode).toBe(200) + + // Check that octicons without aria-label get auto-generated ones + expect(res.body).toContain('aria-label="check icon"') + expect(res.body).toContain('aria-label="git branch icon"') + }) + + test('octicons with custom aria-labels use the custom value', async () => { + const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) + expect(res.statusCode).toBe(200) + + // Check that custom aria-labels are preserved + expect(res.body).toContain('aria-label="Supported"') + expect(res.body).toContain('aria-label="Not supported"') + }) + + test('octicons with other attributes still get auto-generated aria-labels', async () => { + const res = await get(makeURL('/en/get-started/start-your-journey/hello-world')) + expect(res.statusCode).toBe(200) + + // Check that octicons with width attribute still get aria-labels + expect(res.body).toContain('aria-label="rocket icon"') + expect(res.body).toContain('width="32"') + }) + + test('a pathname that does not exist', async () => { + const res = await get(makeURL('/en/never/heard/of')) + expect(res.statusCode).toBe(404) + const { error } = JSON.parse(res.body) + expect(error).toBe("No page found for '/en/never/heard/of'") + }) + + test('non-article pages return error', async () => { + // Index pages are not articles and should not be renderable + const res = await get(makeURL('/en/get-started')) + expect(res.statusCode).toBe(403) + const { error } = JSON.parse(res.body) + expect(error).toContain("isn't yet available in markdown") + }) +}) diff --git a/src/content-linter/lib/linting-rules/index.ts b/src/content-linter/lib/linting-rules/index.ts index 233de02596..3afc9fa151 100644 --- a/src/content-linter/lib/linting-rules/index.ts +++ b/src/content-linter/lib/linting-rules/index.ts @@ -42,7 +42,6 @@ import { tableColumnIntegrity } from '@/content-linter/lib/linting-rules/table-c import { thirdPartyActionPinning } from '@/content-linter/lib/linting-rules/third-party-action-pinning' import { liquidTagWhitespace } from '@/content-linter/lib/linting-rules/liquid-tag-whitespace' import { linkQuotation } from '@/content-linter/lib/linting-rules/link-quotation' -import { octiconAriaLabels } from '@/content-linter/lib/linting-rules/octicon-aria-labels' import { liquidIfversionVersions } from '@/content-linter/lib/linting-rules/liquid-ifversion-versions' import { outdatedReleasePhaseTerminology } from '@/content-linter/lib/linting-rules/outdated-release-phase-terminology' import { frontmatterVersionsWhitespace } from '@/content-linter/lib/linting-rules/frontmatter-versions-whitespace' @@ -105,7 +104,7 @@ export const gitHubDocsMarkdownlint = { thirdPartyActionPinning, // GHD041 liquidTagWhitespace, // GHD042 linkQuotation, // GHD043 - octiconAriaLabels, // GHD044 + // GHD044 removed - octicon aria-labels are now auto-generated codeAnnotationCommentSpacing, // GHD045 outdatedReleasePhaseTerminology, // GHD046 tableColumnIntegrity, // GHD047 diff --git a/src/content-linter/lib/linting-rules/octicon-aria-labels.ts b/src/content-linter/lib/linting-rules/octicon-aria-labels.ts deleted file mode 100644 index 376c67185f..0000000000 --- a/src/content-linter/lib/linting-rules/octicon-aria-labels.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { TokenKind } from 'liquidjs' -import { getLiquidTokens, getPositionData } from '../helpers/liquid-utils' -import { addFixErrorDetail } from '../helpers/utils' -import type { RuleParams, RuleErrorCallback, Rule } from '../../types' -/* -Octicons should always have an aria-label attribute even if aria hidden. For example: - - DO use aria-label - {% octicon "alert" aria-label="alert" %} - {% octicon "alert" aria-label="alert" aria-hidden="true" %} - {% octicon "alert" aria-label="alert" aria-hidden="true" class="foo" %} - - This is necessary for copilot to be able to recognize the svgs correctly when using our API. - -*/ - -export const octiconAriaLabels: Rule = { - names: ['GHD044', 'octicon-aria-labels'], - description: 'Octicons should always have an aria-label attribute even if aria-hidden.', - tags: ['accessibility', 'octicons'], - parser: 'markdownit', - function: (params: RuleParams, onError: RuleErrorCallback) => { - const content = params.lines.join('\n') - // Using 'any' type for tokens as getLiquidTokens returns tokens from liquid-utils.ts which lacks type definitions - const tokens = getLiquidTokens(content) - .filter((token: any) => token.kind === TokenKind.Tag) - .filter((token: any) => token.name === 'octicon') - - for (const token of tokens) { - const { lineNumber, column, length } = getPositionData(token, params.lines) - - const hasAriaLabel = token.args.includes('aria-label=') - - if (!hasAriaLabel) { - const range = [column, length] - - const octiconNameMatch = token.args.match(/["']([^"']+)["']/) - const octiconName = octiconNameMatch ? octiconNameMatch[1] : 'icon' - const originalContent = token.content - const fixedContent = `${originalContent} aria-label="${octiconName}"` - - addFixErrorDetail( - onError, - lineNumber, - `octicon should have an aria-label even if aria hidden. Try using 'aria-label=${octiconName}'`, - token.content, - range, - { - lineNumber, - editColumn: column, - deleteCount: length, - insertText: `{% ${fixedContent} %}`, - }, - ) - } - } - }, -} diff --git a/src/content-linter/style/github-docs.ts b/src/content-linter/style/github-docs.ts index aa914dc108..fd9312af5a 100644 --- a/src/content-linter/style/github-docs.ts +++ b/src/content-linter/style/github-docs.ts @@ -162,12 +162,7 @@ const githubDocsConfig = { 'partial-markdown-files': true, 'yml-files': true, }, - 'octicon-aria-labels': { - // GHD044 - severity: 'warning', - 'partial-markdown-files': true, - 'yml-files': true, - }, + // GHD044 removed - octicon aria-labels are now auto-generated 'code-annotation-comment-spacing': { // GHD045 severity: 'warning', diff --git a/src/content-linter/tests/unit/octicon-aria-labels.ts b/src/content-linter/tests/unit/octicon-aria-labels.ts deleted file mode 100644 index aa4f03239a..0000000000 --- a/src/content-linter/tests/unit/octicon-aria-labels.ts +++ /dev/null @@ -1,155 +0,0 @@ -import { describe, expect, test } from 'vitest' -import { octiconAriaLabels } from '../../lib/linting-rules/octicon-aria-labels' - -interface ErrorInfo { - lineNumber: number - detail?: string - context?: string - range?: [number, number] - fixInfo?: any // Matches RuleErrorCallback signature - fixInfo structure varies by rule -} - -describe('octicon-aria-labels', () => { - const rule = octiconAriaLabels - - // Helper to create onError callback that captures errors - function createErrorCollector() { - const errors: ErrorInfo[] = [] - // Using any because the actual rule implementation calls onError with an object, - // not individual parameters as defined in RuleErrorCallback - const onError = (errorInfo: any) => { - errors.push(errorInfo) - } - return { errors, onError } - } - - test('detects octicon without aria-label', () => { - const { errors, onError } = createErrorCollector() - - const content = ['This is a test with an octicon:', '{% octicon "alert" %}', 'Some more text.'] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(1) - expect(errors[0].lineNumber).toBe(2) - expect(errors[0].detail).toContain('aria-label=alert') - expect(errors[0].fixInfo.insertText).toContain('aria-label="alert"') - }) - - test('ignores octicons with aria-label', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with a proper octicon:', - '{% octicon "alert" aria-label="alert" %}', - 'Some more text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(0) - }) - - test('detects multiple octicons without aria-label', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with multiple octicons:', - '{% octicon "alert" %}', - 'Some text in between.', - '{% octicon "check" %}', - 'More text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(2) - expect(errors[0].lineNumber).toBe(2) - expect(errors[0].detail).toContain('aria-label=alert') - expect(errors[1].lineNumber).toBe(4) - expect(errors[1].detail).toContain('aria-label=check') - }) - - test('ignores non-octicon liquid tags', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with non-octicon tags:', - '{% data foo.bar %}', - '{% ifversion fpt %}', - 'Some text.', - '{% endif %}', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(0) - }) - - test('suggests correct fix for octicon with other attributes', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with an octicon with other attributes:', - '{% octicon "plus" aria-hidden="true" class="foo" %}', - 'Some more text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(1) - expect(errors[0].lineNumber).toBe(2) - expect(errors[0].fixInfo.insertText).toContain('aria-label="plus"') - expect(errors[0].fixInfo.insertText).toContain('aria-hidden="true"') - expect(errors[0].fixInfo.insertText).toContain('class="foo"') - }) - - test('handles octicons with unusual spacing', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with unusual spacing:', - '{% octicon "x" %}', - 'Some more text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(1) - expect(errors[0].lineNumber).toBe(2) - expect(errors[0].detail).toContain('aria-label=x') - }) - - test('handles octicons split across multiple lines', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with a multi-line octicon:', - '{% octicon "chevron-down"', - ' class="dropdown-menu-icon"', - '%}', - 'Some more text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(1) - expect(errors[0].detail).toContain('aria-label=chevron-down') - }) - - test('falls back to "icon" when octicon name cannot be determined', () => { - const { errors, onError } = createErrorCollector() - - const content = [ - 'This is a test with a malformed octicon:', - '{% octicon variable %}', - 'Some more text.', - ] - - rule.function({ name: 'test.md', lines: content, frontMatterLines: [] }, onError) - - expect(errors.length).toBe(1) - expect(errors[0].detail).toContain('aria-label=icon') - expect(errors[0].fixInfo.insertText).toContain('aria-label="icon"') - }) -}) diff --git a/src/content-render/liquid/octicon.ts b/src/content-render/liquid/octicon.ts index c8ceaa3a44..47d5a0ee86 100644 --- a/src/content-render/liquid/octicon.ts +++ b/src/content-render/liquid/octicon.ts @@ -24,7 +24,10 @@ const SyntaxHelp = 'Syntax Error in tag \'octicon\' - Valid syntax: octicon " * {% octicon "check" width="64" aria-label="Example label" %} */ const Octicon: LiquidTag = { @@ -70,6 +73,13 @@ const Octicon: LiquidTag = { throw new Error(`Octicon ${this.icon} does not exist`) } + // Auto-generate aria-label if not provided + // Replace non-alphanumeric characters with spaces and append " icon" + if (!this.options['aria-label']) { + const defaultLabel = `${this.icon.toLowerCase().replace(/[^a-z0-9]+/gi, ' ')} icon` + this.options['aria-label'] = defaultLabel + } + const result: string = octicons[this.icon].toSVG(this.options) return result }, diff --git a/src/content-render/tests/octicon.ts b/src/content-render/tests/octicon.ts index e17b1088df..7c6387c46a 100644 --- a/src/content-render/tests/octicon.ts +++ b/src/content-render/tests/octicon.ts @@ -42,4 +42,34 @@ describe('octicon tag', () => { 'Octicon pizza-patrol does not exist', ) }) + + test('auto-generates aria-label when not provided', async () => { + const actual = await renderContent('{% octicon "check" %}') + expect(actual).toContain(' { + const actual = await renderContent('{% octicon "git-branch" %}') + expect(actual).toContain(' { + const actual = await renderContent('{% octicon "check" aria-label="Supported" %}') + expect(actual).toContain(' { + const actual = await renderContent('{% octicon "filter" width="32" %}') + expect(actual).toContain('