🎨 Palette: [UX improvement] Enhance Coverage UI Accessibility#310
🎨 Palette: [UX improvement] Enhance Coverage UI Accessibility#310EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
* Updated search inputs in `app-http` and `http-platform` to use `type="search"` and added an `aria-label`. * Handled `aria-pressed` states dynamically in filter button JavaScript logic. * Set initial active and `aria-pressed` states on the "All" button on the server-side via `maud` to eliminate Flash of Unstyled Content (FOUC). * Added active navigation link logic to `app-http` using `aria-current="page"` and styling rules.
|
👋 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. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Summary by CodeRabbitRelease Notes
WalkthroughThese changes implement server-side ARIA attribute initialization for accessibility state management, moving Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results283 tests 245 ✅ 11m 15s ⏱️ Results for commit 18ea598. ♻️ This comment has been updated with latest results. |
* Cleanly bumped `astral-tokio-tar` and `rustls-webpki` using `cargo update`. * Removed junk file `=0.26.4` and reverted out-of-scope `adapters-db-sqlx/Cargo.toml` change that caused the CI to fail previously.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/http-platform/src/ui.rs (1)
821-834:⚠️ Potential issue | 🔴 CriticalXSS vulnerability: User data interpolated into innerHTML without escaping.
The
renderTablefunction directly interpolatesac.id,ac.title,ac.story,ac.requirement, and scenario items intoinnerHTMLwithout HTML escaping. This differs fromcrates/app-http/src/platform/ui.rswhich defines and uses anescapeHtml()function.If any coverage data contains HTML or JavaScript, it will execute in the user's browser.
🔒 Proposed fix: Add escapeHtml function and use it
Add the
escapeHtmlfunction beforerenderTable:+ function escapeHtml(text) { + if (!text) return ''; + const map = { + '&': '&', + '<': '<', + '>': '>', + '"': '"', + "'": ''' + }; + return text.toString().replace(/[&<>"']/g, function(m) { return map[m]; }); + } + function renderTable(data) {Update
renderTableto use escaping:const scenarios = ac.scenarios.length > 0 ? '<ul class="scenario-list">' + - ac.scenarios.map(s => '<li>' + s + '</li>').join('') + + ac.scenarios.map(s => '<li>' + escapeHtml(s) + '</li>').join('') + '</ul>' : '<em style="color: `#999`;">No scenarios</em>'; row.innerHTML = ` - <td><code>${ac.id}</code></td> - <td>${ac.title}</td> + <td><code>${escapeHtml(ac.id)}</code></td> + <td>${escapeHtml(ac.title)}</td> <td><span class="status-badge ${badgeClass}">${statusBadge}</span></td> - <td><code>${ac.story}</code></td> - <td><code>${ac.requirement}</code></td> + <td><code>${escapeHtml(ac.story)}</code></td> + <td><code>${escapeHtml(ac.requirement)}</code></td> <td>${scenarios}</td> `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/http-platform/src/ui.rs` around lines 821 - 834, The renderTable code is inserting unescaped user data into row.innerHTML (ac.id, ac.title, ac.story, ac.requirement and each scenario string), causing an XSS risk; add an escapeHtml(s: string) utility (or reuse the one from crates/app-http/src/platform/ui.rs) and use it for every interpolated value when building the HTML string (i.e., escape ac.id, ac.title, ac.story, ac.requirement and each scenario item before concatenation), or alternatively build the row DOM using createElement and set textContent for those fields instead of using innerHTML; ensure the scenarios array is mapped through the escaper (escapeHtml(s)) before joining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.Jules/palette.md:
- Around line 4-7: The markdown linter flagged MD022 (missing blank line before
heading); add a single blank line immediately before the "## 2024-05-23 -
Initial ARIA states" heading in .Jules/palette.md so the heading is separated
from the preceding content; confirm the blank line precedes that exact heading
text to satisfy the linter.
---
Outside diff comments:
In `@crates/http-platform/src/ui.rs`:
- Around line 821-834: The renderTable code is inserting unescaped user data
into row.innerHTML (ac.id, ac.title, ac.story, ac.requirement and each scenario
string), causing an XSS risk; add an escapeHtml(s: string) utility (or reuse the
one from crates/app-http/src/platform/ui.rs) and use it for every interpolated
value when building the HTML string (i.e., escape ac.id, ac.title, ac.story,
ac.requirement and each scenario item before concatenation), or alternatively
build the row DOM using createElement and set textContent for those fields
instead of using innerHTML; ensure the scenarios array is mapped through the
escaper (escapeHtml(s)) before joining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48d04f7e-2e3a-44cc-8852-0afb71067ff1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.Jules/palette.mdcrates/adapters-db-sqlx/Cargo.tomlcrates/app-http/src/platform/ui.rscrates/http-platform/src/ui.rs
|
|
||
| ## 2024-05-23 - Initial ARIA states | ||
| **Learning:** Initializing ARIA attributes like `aria-pressed` or `aria-current` via client-side JavaScript (`DOMContentLoaded`) causes a Flash of Unstyled Content (FOUC) and can lead screen readers to announce incorrect initial states before the script executes. | ||
| **Action:** Always render initial ARIA states directly on the server (e.g., using `maud` server-side rendering) to ensure immediate accessibility and correct visual state upon page load. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor markdown formatting issue.
The static analysis tool flagged a missing blank line before the heading on line 5 (MD022). This is a minor formatting nit.
📝 Proposed fix
**Action:** Use `attr=[condition.then(|| "value")]` for all conditional ARIA attributes.
+
## 2024-05-23 - Initial ARIA states
**Learning:** Initializing ARIA attributes like `aria-pressed` or `aria-current` via client-side JavaScript (`DOMContentLoaded`) causes a Flash of Unstyled Content (FOUC) and can lead screen readers to announce incorrect initial states before the script executes.
**Action:** Always render initial ARIA states directly on the server (e.g., using `maud` server-side rendering) to ensure immediate accessibility and correct visual state upon page load.🧰 Tools
🪛 LanguageTool
[typographical] ~5-~5: To join two clauses or introduce examples, consider using an em dash.
Context: ...ditional ARIA attributes. ## 2024-05-23 - Initial ARIA states Learning: Initia...
(DASH_RULE)
🪛 markdownlint-cli2 (0.21.0)
[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.Jules/palette.md around lines 4 - 7, The markdown linter flagged MD022
(missing blank line before heading); add a single blank line immediately before
the "## 2024-05-23 - Initial ARIA states" heading in .Jules/palette.md so the
heading is separated from the preceding content; confirm the blank line precedes
that exact heading text to satisfy the linter.
1 participant
💡 What:
searchand adding anaria-label.aria-pressedattributes that dynamically toggle alongside the.activeclass.DOMContentLoadedstate initialization with server-side rendering for the default active filter button.aria-current="page") to theapp-httpcrate.🎯 Why:
To improve accessibility for screen readers navigating the AC Coverage UI. Initializing ARIA attributes via client-side JavaScript can cause a Flash of Unstyled Content (FOUC) and lead screen readers to initially announce incorrect states before the script executes. Server-side rendering ensures immediate correctness. Using standard
type="search"inputs andaria-labels increases predictability and navigability for users of assistive technology.📸 Before/After:
(Visuals recorded via verification script)
textinput. Filter buttons lackedaria-pressedstate and initial active class was applied after page load. Navigation link inapp-httpdid not indicate current page.♿ Accessibility:
Added
aria-labelto search inputs,aria-pressedto interactive toggle buttons, andaria-current="page"to the active navigation link. Eliminated FOUC for initial focus states.PR created automatically by Jules for task 11105527675851552943 started by @EffortlessSteven