🎨 Palette: Improve accessibility of coverage filters and search#201
🎨 Palette: Improve accessibility of coverage filters and search#201EffortlessSteven wants to merge 1 commit intomainfrom
Conversation
- Add `aria-label` to coverage search input. - Add `aria-pressed` and `aria-controls` to coverage filter buttons. - Update embedded JavaScript to toggle `aria-pressed` state on click. - Fix `maud` escaping issue by wrapping inline scripts and styles in `PreEscaped`. - Add integration test `test_coverage_page_accessibility` to verify attributes. Accessibility improvements: - Screen readers can now identify the purpose of the search box. - Screen readers can now determine which filter is currently active. Bug fix: - Inline JavaScript `&&` operator was previously escaped to `&&`, breaking the filter logic. This is now fixed using `PreEscaped`.
|
👋 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. |
Summary by CodeRabbit
WalkthroughAdded documentation about Maud HTML escaping behavior, introduced accessibility tests for coverage page UI elements, and updated UI rendering code to wrap dynamic HTML/CSS/JavaScript content with 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 user experience on the AC Coverage page by addressing critical accessibility concerns and fixing a rendering bug. By integrating ARIA attributes, interactive elements are now more usable for all users, including those relying on assistive technologies. The fix for 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 effectively improves the accessibility of the coverage page by adding appropriate ARIA attributes and fixes an HTML escaping issue with inline scripts and styles in the maud templates. The changes are well-supported by a new accessibility test, and a suggestion has been made to make the new test more comprehensive.
| // Verify 'All' button is pressed by default | ||
| let all_btn_selector = Selector::parse("button#filter-all").unwrap(); | ||
| let all_btn = document.select(&all_btn_selector).next().unwrap(); | ||
| assert_eq!(all_btn.value().attr("aria-pressed"), Some("true")); | ||
|
|
||
| // Verify 'Passing' button is not pressed | ||
| let passing_btn_selector = Selector::parse("button#filter-passing").unwrap(); | ||
| let passing_btn = document.select(&passing_btn_selector).next().unwrap(); | ||
| assert_eq!(passing_btn.value().attr("aria-pressed"), Some("false")); | ||
| } |
There was a problem hiding this comment.
The test currently only verifies the initial aria-pressed state for the 'All' and 'Passing' filter buttons. To make the test more robust and comprehensive, it would be better to check the initial state of all filter buttons ('failing' and 'unknown' as well).
Refactoring this into a loop would also make the test more maintainable if more filters are added in the future.
// Verify initial aria-pressed states
for (filter_id, expected_state) in [
("all", "true"),
("passing", "false"),
("failing", "false"),
("unknown", "false"),
] {
let selector_str = format!("button#filter-{}", filter_id);
let selector = Selector::parse(&selector_str).unwrap();
let button = document.select(&selector).next().unwrap_or_else(|| {
panic!("Filter button with selector '{}' not found", selector_str)
});
assert_eq!(
button.value().attr("aria-pressed"),
Some(expected_state),
"Button '{}' should have aria-pressed='{}'",
filter_id,
expected_state
);
}
}There was a problem hiding this comment.
Pull request overview
Improves accessibility and restores correct client-side behavior on the AC Coverage UI by adding ARIA attributes to filter/search controls and preventing maud from HTML-escaping inline CSS/JS content (which breaks rawtext/script contexts).
Changes:
- Wrap inline
<style>/<script>content inPreEscaped(...)to avoid entity escaping that breaks CSS/JS. - Add
aria-labelto the coverage search input andaria-pressed/aria-controlsto filter buttons; update JS to togglearia-pressed. - Add a DOM contract test verifying the new accessibility attributes on
/ui/coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/http-platform/src/ui.rs | Adds ARIA attributes to coverage controls and uses PreEscaped for inline CSS/JS (and Mermaid output). |
| crates/app-http/tests/ui_contract_dom.rs | Adds an accessibility-focused DOM test for the coverage page. |
| .Jules/palette.md | Documents the PreEscaped requirement for inline CSS/JS and Mermaid content in maud. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| .mermaid data-uiid="graph.diagram" { | ||
| (mermaid_diagram) | ||
| (PreEscaped(mermaid_diagram)) |
There was a problem hiding this comment.
PreEscaped(mermaid_diagram) renders the Mermaid source as raw HTML. Since graph.to_mermaid() incorporates labels from specs and mermaid_label() only escapes quotes (not <, >, &), a crafted title/label containing <...> could be interpreted as HTML and become an XSS/injection vector (or simply break the page markup). Prefer emitting the diagram as normal escaped text (no PreEscaped) if Mermaid can parse via textContent, or otherwise ensure the diagram is inserted in a way that cannot be HTML-parsed (e.g., via textContent) / is properly sanitized before PreEscaped.
| (PreEscaped(mermaid_diagram)) | |
| (mermaid_diagram) |
|
|
||
| ## 2026-02-15 - Maud Escaping Inline Scripts/Styles | ||
| **Learning:** `maud` automatically escapes string content, which breaks inline JavaScript (e.g., `&&` becomes `&&`) and CSS (e.g., `>` becomes `>`). | ||
| **Action:** Always wrap inline CSS, JavaScript, and pre-formatted content (like Mermaid diagrams) in `PreEscaped(...)` when using `maud` templates. |
There was a problem hiding this comment.
The guidance to "Always wrap ... pre-formatted content (like Mermaid diagrams) in PreEscaped(...)" is overly broad. PreEscaped disables HTML escaping and is only safe for trusted/static content; for dynamic content derived from specs/user input it can introduce XSS or markup-breaking injection. Consider updating this note to distinguish inline <style>/<script> (where escaping breaks functionality) from dynamic HTML/text content (where PreEscaped should be avoided or combined with sanitization / textContent insertion).
| **Action:** Always wrap inline CSS, JavaScript, and pre-formatted content (like Mermaid diagrams) in `PreEscaped(...)` when using `maud` templates. | |
| **Action:** For trusted/static inline CSS or JavaScript where escaping breaks functionality, wrap the block in `PreEscaped(...)`. Do not use `PreEscaped` for dynamic or user-controlled content (including pre-formatted text like Mermaid diagrams); instead, let `maud` escape it, sanitize it first, or inject it via safe mechanisms (e.g., `textContent` in JS). |
This PR improves the accessibility of the AC Coverage page by adding ARIA attributes to interactive elements. It also fixes a bug where inline JavaScript and CSS were being incorrectly escaped by the
maudtemplate engine, breaking the filtering functionality.Changes:
crates/http-platform/src/ui.rs:aria-labelto search input.aria-pressedandaria-controlsto filter buttons.PreEscapedto prevent HTML entity escaping.aria-pressed.crates/app-http/tests/ui_contract_dom.rs:test_coverage_page_accessibilityto verify the presence and correctness of new attributes.Verification:
cargo test -p app-http(new test passed).PR created automatically by Jules for task 7337919575116934754 started by @EffortlessSteven