Add accessibility validation checks to site validator#8796
Add accessibility validation checks to site validator#8796TheAshutoshMishra wants to merge 3 commits intojenkins-infra:masterfrom
Conversation
- Declare variables properly in roadmap.js to prevent global scope pollution - Remove console.log statements from feedback form functionality Variables like selectors, filters, initiative, and display were not declared with var, causing them to leak into global scope. This could lead to conflicts with other scripts. Added proper var declarations throughout filterRoadmap(). Also removed leftover debug console.log statements that shouldn't be in production code.
- Added --help/-h flag to display usage information - Added --verbose/-v flag for detailed output during execution - Improved error handling for invalid options This makes the typo checking tool more accessible to new contributors by providing clear documentation and optional debugging output.
- Added check for missing alt text on images (WCAG 2.1 Level A) - Added validation for generic alt text (image, logo, icon, etc) - Added check for icon-only buttons/links missing aria-label - Helps prevent accessibility issues during build time This enhancement will catch common accessibility problems before they reach production, improving site inclusivity for users with screen readers and assistive technologies.
There was a problem hiding this comment.
Pull request overview
This PR adds accessibility validation checks to the site validator to enforce WCAG 2.1 Level A compliance during the build process. However, the changes also include unrelated modifications to other files.
Changes:
- Added
validate_accessibilitymethod to validator.rb that checks for missing/generic alt text on images and missing ARIA labels on icon-only interactive elements - Added help text and verbose mode to the check-typos script
- Added
vardeclarations to JavaScript variables in roadmap.js
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| content/_ext/validator.rb | Added accessibility validation methods to check alt text on images and ARIA labels on interactive elements |
| scripts/check-typos | Added command-line argument parsing for help and verbose options |
| content/js/roadmap.js | Added var declarations to previously undeclared variables |
| content/js/feedback-form-functionality.js | Removed commented-out console.log debugging statements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| page.content.scan(icon_only_pattern).each do |match| | ||
| element_line = page.content[page.content.index(match[0])..page.content.index(match[0]) + 150] |
There was a problem hiding this comment.
The use of page.content.index(match[0]) to extract element_line is problematic. This method finds the first occurrence of match[0] in the entire content, not the specific occurrence found by scan. If the same element pattern appears multiple times in the page, this will incorrectly examine the first occurrence for all matches, leading to incorrect validation results. Consider using scan with block form or match with position tracking to ensure you're examining the correct occurrence.
| page.content.scan(icon_only_pattern).each do |match| | |
| element_line = page.content[page.content.index(match[0])..page.content.index(match[0]) + 150] | |
| page.content.scan(icon_only_pattern) do | |
| match_data = Regexp.last_match | |
| start_index = match_data.begin(0) | |
| element_line = page.content[start_index, 150] |
|
|
||
| def check_image_alt_text(page) | ||
| # Find all img tags - both HAML and HTML | ||
| img_pattern = /%img\{[^}]*\}|<img[^>]*>/i |
There was a problem hiding this comment.
The regex pattern /%img\{[^}]*\}/ uses [^}]* which will fail to match HAML image tags that have nested braces in their attributes (e.g., style hashes or nested data attributes). For example, %img{src: 'x', style: {width: '100%'}} would not be matched correctly. Consider using a more robust pattern or a proper HAML parser to handle nested structures.
| img_pattern = /%img\{[^}]*\}|<img[^>]*>/i | |
| img_pattern = /%img\{[^\n]*\}|<img[^>]*>/i |
| # Find buttons and links with only icons (no text content) | ||
| # Look for patterns like %button, %a with only ion-icon or svg inside | ||
| icon_only_pattern = /(%button|%a)(\{[^}]*\})?[^%\n]*(%ion-icon|<svg|<ion-icon)/i | ||
|
|
||
| page.content.scan(icon_only_pattern).each do |match| | ||
| element_line = page.content[page.content.index(match[0])..page.content.index(match[0]) + 150] | ||
|
|
||
| # Check if there's an aria-label or aria-labelledby | ||
| unless element_line.match(/aria-label|aria-labelledby/i) |
There was a problem hiding this comment.
The pattern (%button|%a)(\{[^}]*\})?[^%\n]*(%ion-icon|<svg|<ion-icon) has several issues: 1) It will match buttons/links that have both text and icons, not just icon-only elements. For example, a button with "Click here" text and an icon would be flagged. 2) The [^%\n]* part is too permissive and could match across multiple elements. 3) It doesn't account for text content that might appear on subsequent lines in HAML. Consider checking if the element contains actual text content, not just whether an icon exists.
| # Find buttons and links with only icons (no text content) | |
| # Look for patterns like %button, %a with only ion-icon or svg inside | |
| icon_only_pattern = /(%button|%a)(\{[^}]*\})?[^%\n]*(%ion-icon|<svg|<ion-icon)/i | |
| page.content.scan(icon_only_pattern).each do |match| | |
| element_line = page.content[page.content.index(match[0])..page.content.index(match[0]) + 150] | |
| # Check if there's an aria-label or aria-labelledby | |
| unless element_line.match(/aria-label|aria-labelledby/i) | |
| # Find buttons and links that appear to have only icons (no text content) | |
| # Look for patterns like %button, %a with ion-icon or svg on the same logical line | |
| icon_only_pattern = /(%button|%a)(\{[^}]*\})?[^\n]*(%ion-icon|<svg|<ion-icon)[^\n]*/i | |
| matches = [] | |
| page.content.to_enum(:scan, icon_only_pattern) { matches << Regexp.last_match } | |
| matches.each do |match| | |
| # Start from the beginning of the matched element | |
| start_index = match.begin(0) | |
| remaining = page.content[start_index..-1] | |
| # Heuristic: take content up to the next line that begins a new HAML tag ("%") | |
| # or to the end of the document. This helps include simple multi-line constructs. | |
| next_tag_index = remaining.index("\n%") | |
| element_block = if next_tag_index | |
| remaining[0...next_tag_index] | |
| else | |
| remaining | |
| end | |
| # Derive a text-only version of the block to see if there is readable content | |
| text_only = element_block.dup | |
| # Remove HAML/HTML attributes in braces | |
| text_only.gsub!(/\{[^}]*\}/, '') | |
| # Remove HAML tags such as %button{...} | |
| text_only.gsub!(/%[a-z0-9_-]+(?:\{[^}]*\})?/i, '') | |
| # Remove HTML tags, including icon tags | |
| text_only.gsub!(/<\/?[^>]+>/, '') | |
| # Remove common HTML entities | |
| text_only.gsub!(/&[a-z0-9#]+;/i, '') | |
| # Strip whitespace and punctuation so only potential readable characters remain | |
| text_only.gsub!(/[\s\p{Punct}]+/u, '') | |
| # If there is any remaining content, consider it as text and skip the ARIA check | |
| next unless text_only.empty? | |
| # Check if there's an aria-label or aria-labelledby on the element block | |
| unless element_block.match(/aria-label|aria-labelledby/i) |
| var selector = selectors[i] | ||
| if (selector.checked == true){ | ||
| filters.push(selector.id) | ||
| filterInitiatives = true | ||
| } | ||
| } | ||
|
|
||
| //table = document.getElementsByClassName("roadmap-table"); | ||
| categoryHeaders = document.getElementsByClassName("status-category") | ||
| categoryInitiatives = document.getElementsByClassName("category-initiatives") | ||
| var categoryHeaders = document.getElementsByClassName("status-category"); | ||
| var categoryInitiatives = document.getElementsByClassName("category-initiatives"); | ||
|
|
||
| for (categoryId = 0; categoryId < categoryInitiatives.length; categoryId++) { | ||
| initiatives = categoryInitiatives[categoryId].getElementsByClassName("initiative"); | ||
| hasInitiativesToDisplay = false | ||
| for (var categoryId = 0; categoryId < categoryInitiatives.length; categoryId++) { | ||
| var initiatives = categoryInitiatives[categoryId].getElementsByClassName("initiative"); | ||
| var hasInitiativesToDisplay = false | ||
|
|
||
| // Loop through all table rows, and hide those who don't match the search query | ||
| for (i = 0; i < initiatives.length; i++) { | ||
| initiative = initiatives[i]; | ||
| display = !filterInitiatives | ||
| for (j = 0; j < filters.length; j++) { | ||
| var initiative = initiatives[i]; | ||
| var display = !filterInitiatives; | ||
| for (var j = 0; j < filters.length; j++) { | ||
| if (initiative.classList.contains(filters[j])) { | ||
| display = true | ||
| break |
There was a problem hiding this comment.
Missing semicolons on lines 8, 10, 11, 21, 26, 29, and 30, while other lines in the function have them. This creates inconsistent code style. Either consistently use semicolons throughout the function or omit them entirely (JavaScript doesn't require them, but consistency is important for maintainability).
| VERBOSE=0 | ||
|
|
||
| # Simple help text | ||
| show_help() { | ||
| cat << EOF | ||
| Usage: ./scripts/check-typos [OPTIONS] | ||
| Check for typos in the repository. | ||
| OPTIONS: | ||
| -h, --help Show this help message | ||
| -v, --verbose Show detailed output | ||
| EXAMPLES: | ||
| ./scripts/check-typos | ||
| ./scripts/check-typos --verbose | ||
| EOF | ||
| } | ||
|
|
||
| # Parse arguments | ||
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--help) | ||
| show_help | ||
| exit 0 | ||
| ;; | ||
| -v|--verbose) | ||
| VERBOSE=1 | ||
| shift | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $1" | ||
| echo "Use --help for usage information" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done | ||
|
|
||
| # Download typos binary | ||
| if [[ $VERBOSE -eq 1 ]]; then | ||
| echo "Downloading typos ${TYPOS_VERSION}..." | ||
| fi | ||
|
|
||
| if [[ $OSTYPE == darwin* ]] ; then | ||
| curl --disable --silent --show-error --location "https://github.com/crate-ci/typos/releases/download/${TYPOS_VERSION}/typos-${TYPOS_VERSION}-x86_64-apple-darwin.tar.gz" | tar xzf - ./typos | ||
| else | ||
| curl --disable --silent --show-error --location "https://github.com/crate-ci/typos/releases/download/${TYPOS_VERSION}/typos-${TYPOS_VERSION}-x86_64-unknown-linux-musl.tar.gz" | tar xzf - ./typos | ||
| fi | ||
|
|
||
| # Run typos with appropriate output format | ||
| if [[ -v CI ]] ; then | ||
| if [[ $VERBOSE -eq 1 ]]; then | ||
| echo "Running in CI mode..." | ||
| fi | ||
| ./typos --format sarif > typos.sarif || true | ||
| else | ||
| ./typos | ||
| if [[ $VERBOSE -eq 1 ]]; then | ||
| echo "Checking for typos..." | ||
| ./typos --format long | ||
| else | ||
| ./typos | ||
| fi |
There was a problem hiding this comment.
The PR description states this adds "accessibility validation checks to site validator", but the changes also include unrelated modifications to scripts/check-typos (adding help and verbose options), content/js/roadmap.js (adding var declarations), and content/js/feedback-form-functionality.js (removing console.log statements). These changes should either be in a separate PR or be mentioned in the description, as they don't relate to accessibility validation.
Integrated automated accessibility checks into the site validator to enforce WCAG 2.1 Level A compliance during the build process. Changes: Added validation for missing/generic alt text and ARIA labels for icon-only interactive elements in both HAML and HTML. Impact: Prevents accessibility regressions by catching common errors before production, improving the experience for screen reader users.