Palette: Add accessibility attributes to coverage UI#204
Palette: Add accessibility attributes to coverage UI#204EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
- Added `aria-label` to the coverage search input. - Added `aria-pressed` attributes to filter buttons. - Updated inline JavaScript to toggle `aria-pressed` state. - Fixed `maud` template to use `PreEscaped` for inline scripts and styles to prevent HTML entity escaping. - Added `test_coverage_search_has_label` to verify accessibility compliance.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughThese changes enhance UI accessibility by adding ARIA attributes (aria-pressed, aria-label) to coverage filter controls and search inputs, and refactor HTML rendering to properly handle raw CSS/JS content using PreEscaped from the Maud templating engine. A corresponding test validates the presence of aria-label on the search box element. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
Summary of ChangesHello @EffortlessSteven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the accessibility of the coverage UI by integrating ARIA attributes for better screen reader support. It also addresses a critical rendering issue by correctly handling inline scripts and styles, preventing unintended HTML entity escaping. These changes ensure a more robust and inclusive user interface. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the accessibility of the coverage UI by adding aria-label and aria-pressed attributes. It also fixes a bug with HTML entity escaping in inline scripts and styles by using PreEscaped. The changes are well-implemented and include a new test to verify the accessibility improvements. I have a couple of suggestions to improve the robustness of the new test and the JavaScript code.
| assert!( | ||
| aria_label.is_some(), | ||
| "Search input should have aria-label attribute" | ||
| ); | ||
| assert!( | ||
| !aria_label.unwrap().is_empty(), | ||
| "Search input aria-label should not be empty" | ||
| ); |
There was a problem hiding this comment.
The test for the aria-label on the search input can be made more specific and robust. Instead of just checking for the presence of a non-empty attribute, you could assert that it has the exact expected value. This ensures the accessible name is correct and prevents accidental changes.
Given the PR description specifies the label text, testing for it directly would be a good practice.
| assert!( | |
| aria_label.is_some(), | |
| "Search input should have aria-label attribute" | |
| ); | |
| assert!( | |
| !aria_label.unwrap().is_empty(), | |
| "Search input aria-label should not be empty" | |
| ); | |
| assert_eq!( | |
| aria_label, | |
| Some("Filter acceptance criteria by ID or title"), | |
| "Search input should have the correct aria-label text" | |
| ); |
| const activeBtn = document.getElementById('filter-' + status); | ||
| activeBtn.classList.add('active'); | ||
| activeBtn.setAttribute('aria-pressed', 'true'); |
There was a problem hiding this comment.
To make the JavaScript code more robust, it's a good practice to add a null check for activeBtn. document.getElementById will return null if no element with the given ID is found. While the status values are currently hardcoded and should always find a matching element, this check adds defensiveness against future changes or unexpected states.
| const activeBtn = document.getElementById('filter-' + status); | |
| activeBtn.classList.add('active'); | |
| activeBtn.setAttribute('aria-pressed', 'true'); | |
| const activeBtn = document.getElementById('filter-' + status); | |
| if (activeBtn) { | |
| activeBtn.classList.add('active'); | |
| activeBtn.setAttribute('aria-pressed', 'true'); | |
| } |
There was a problem hiding this comment.
Pull request overview
This pull request improves the accessibility of the coverage UI page by adding ARIA attributes and fixes a bug where inline scripts and styles were being HTML-escaped by Maud. The changes ensure that screen reader users have better context about the search input and filter button states, while the PreEscaped wrapper fix prevents JavaScript and CSS syntax from being broken by HTML entity encoding.
Changes:
- Added
aria-labelto the coverage search input for screen reader accessibility - Added
aria-pressedattributes to filter buttons with JavaScript support for toggling - Wrapped inline styles and scripts in
PreEscaped()to prevent HTML entity escaping - Added test coverage to verify the search input's aria-label attribute exists
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/http-platform/src/ui.rs | Added PreEscaped import; wrapped inline styles/scripts in PreEscaped; added aria-label to search input and aria-pressed to filter buttons; updated JavaScript to toggle aria-pressed state |
| crates/app-http/tests/ui_contract_dom.rs | Added test to verify coverage search input has aria-label attribute |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| script src="https://cdn.jsdelivr.net/npm/mermaid@10/dist/mermaid.min.js" {} | ||
| style { (styles()) } | ||
| style { (PreEscaped(styles())) } | ||
| script { "mermaid.initialize({ startOnLoad: true, theme: 'default' });" } |
There was a problem hiding this comment.
The mermaid initialization script should also be wrapped in PreEscaped for consistency with the fix applied to other inline scripts and styles. While this particular script doesn't currently contain characters that would be HTML-escaped (like &&, <, >), wrapping it in PreEscaped ensures consistency and prevents potential future issues if the script is modified to include such characters.
| script { "mermaid.initialize({ startOnLoad: true, theme: 'default' });" } | |
| script { (PreEscaped("mermaid.initialize({ startOnLoad: true, theme: 'default' });")) } |
| button #filter-all.filter-btn onclick="filterData('all')" aria-pressed="true" { "All" } | ||
| button #filter-passing.filter-btn onclick="filterData('passing')" aria-pressed="false" { "Passing" } | ||
| button #filter-failing.filter-btn onclick="filterData('failing')" aria-pressed="false" { "Failing" } | ||
| button #filter-unknown.filter-btn onclick="filterData('unknown')" aria-pressed="false" { "Unknown" } |
There was a problem hiding this comment.
The filter buttons now have aria-pressed attributes, but there's no test coverage to verify this functionality. Consider adding a test similar to test_coverage_search_has_label that checks whether the filter buttons have the aria-pressed attribute set correctly in the initial HTML. This would ensure the accessibility feature doesn't regress in future changes.
| assert!( | ||
| aria_label.is_some(), | ||
| "Search input should have aria-label attribute" | ||
| ); | ||
| assert!( | ||
| !aria_label.unwrap().is_empty(), | ||
| "Search input aria-label should not be empty" | ||
| ); |
There was a problem hiding this comment.
The test only verifies that the aria-label attribute exists and is non-empty, but doesn't verify its actual content. This is inconsistent with the existing test pattern at lines 263-288 where test_active_nav_link_has_aria_current uses assert_eq! to verify the exact value. Consider using assert_eq! to verify that the aria-label matches the expected value "Filter acceptance criteria by ID or title" to ensure the attribute provides meaningful information to screen reader users.
| assert!( | |
| aria_label.is_some(), | |
| "Search input should have aria-label attribute" | |
| ); | |
| assert!( | |
| !aria_label.unwrap().is_empty(), | |
| "Search input aria-label should not be empty" | |
| ); | |
| assert_eq!( | |
| aria_label, | |
| Some("Filter acceptance criteria by ID or title"), | |
| "Search input should have aria-label describing the filter functionality" | |
| ); |
💡 What: Added
aria-labelto the coverage search input andaria-pressedattributes to the filter buttons. Updated the JavaScript to toggle thearia-pressedstate. Fixed a bug where inline scripts and styles were not wrapped inPreEscaped, causing HTML entity escaping.🎯 Why: To improve accessibility for screen reader users by providing clear labels and state indications. The
PreEscapedfix ensures the UI renders correctly without broken syntax.📸 Before/After:
&&remains&&instead of&&).♿ Accessibility:
aria-label="Filter acceptance criteria by ID or title"to search input.aria-pressedto filter buttons.PR created automatically by Jules for task 8129843187323623886 started by @EffortlessSteven