🎨 Palette: Make file headers keyboard accessible#19
Conversation
Co-authored-by: praxstack <73683289+praxstack@users.noreply.github.com>
|
👋 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. |
📝 WalkthroughWalkthroughThis PR updates accessibility guidance documentation and implements keyboard support with ARIA attributes for interactive file headers in a staged-files UI, adding focus-visible styling, semantic roles, and keyboard event handlers for improved assistive technology compatibility. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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 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 |
![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)
public/index.html (1)
1216-1249:⚠️ Potential issue | 🟠 MajorRestructure the file header to avoid nested interactive controls.
.file-headerhasrole="button"but contains a checkbox input and a button element. This violates accessibility best practices by nesting interactive controls, creates a confusing accessible name, and makes screen reader output unnecessarily verbose. Refactor the expander as its own button-like element (or a semantic<button>), keeping the checkbox and comment controls as separate, non-nested interactive targets outside the expander's interactive region.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 1216 - 1249, The .file-header currently has role="button" and contains nested interactive elements (the checkbox with id "select-${fileId}" and the button with class "comment-btn"); split the expander into its own interactive element so nested controls are not inside it: replace the clickable region that invokes toggleFile('${file}', '${fileId}')/onkeydown handler with a dedicated button or element (e.g., an actual <button> acting as the expander, keeping id "header-${fileId}" or a new expander id) and move the checkbox (select-${fileId}) and comment button (comment-btn / showCommentModal) outside that expander element so their onclick/onchange handlers use event.stopPropagation() to avoid triggering toggleFile; preserve the same handler functions (toggleFile, toggleFileSelection, showCommentModal), aria-expanded toggling for the expander, and keyboard accessibility logic but ensure no interactive controls are nested inside the expander.
🧹 Nitpick comments (1)
.Jules/palette.md (1)
5-7: Prefer native disclosure controls in the guidance.This reads like
div[role="button"]is the standard solution, but that pattern is much easier to get wrong than a native<button>/<details>and it led directly to the nested interactive-control problem inpublic/index.html. Please make this guidance say “prefer native semantics first; only fall back to ARIA-upgraded<div>elements when the markup truly cannot change.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.Jules/palette.md around lines 5 - 7, Update the guidance to prefer native semantic controls before recommending ARIA-enhanced divs: change the recommendation text so it advises using native <button> or <details> elements first (e.g., replace or precede the current “div[role='button']” guidance), and only recommend adding role="button", tabindex="0", aria-expanded, :focus-visible styling and onkeydown handlers for .file-header or other custom interactive <div> elements when the DOM cannot be changed (reference the problematic public/index.html nested interactive-control case as an example to avoid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/index.html`:
- Around line 1221-1222: Inline JS handlers are breaking for filenames with
quotes; stop interpolating raw file paths into onclick/onkeydown/onchange
attributes (where toggleFile, toggleFileSelection, and showCommentModal are
referenced) and instead add data-* attributes (e.g., data-file, data-file-id) to
the elements and attach event listeners in your script using addEventListener to
read element.dataset.file and dataset.fileId and call toggleFile(...),
toggleFileSelection(...), or showCommentModal(...)—remove the inline handler
strings so special characters in file names no longer break the generated JS.
---
Outside diff comments:
In `@public/index.html`:
- Around line 1216-1249: The .file-header currently has role="button" and
contains nested interactive elements (the checkbox with id "select-${fileId}"
and the button with class "comment-btn"); split the expander into its own
interactive element so nested controls are not inside it: replace the clickable
region that invokes toggleFile('${file}', '${fileId}')/onkeydown handler with a
dedicated button or element (e.g., an actual <button> acting as the expander,
keeping id "header-${fileId}" or a new expander id) and move the checkbox
(select-${fileId}) and comment button (comment-btn / showCommentModal) outside
that expander element so their onclick/onchange handlers use
event.stopPropagation() to avoid triggering toggleFile; preserve the same
handler functions (toggleFile, toggleFileSelection, showCommentModal),
aria-expanded toggling for the expander, and keyboard accessibility logic but
ensure no interactive controls are nested inside the expander.
---
Nitpick comments:
In @.Jules/palette.md:
- Around line 5-7: Update the guidance to prefer native semantic controls before
recommending ARIA-enhanced divs: change the recommendation text so it advises
using native <button> or <details> elements first (e.g., replace or precede the
current “div[role='button']” guidance), and only recommend adding role="button",
tabindex="0", aria-expanded, :focus-visible styling and onkeydown handlers for
.file-header or other custom interactive <div> elements when the DOM cannot be
changed (reference the problematic public/index.html nested interactive-control
case as an example to avoid).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7617aae-bf10-47c0-8e04-5094f7605afa
📒 Files selected for processing (2)
.Jules/palette.mdpublic/index.html
| onclick="toggleFile('${file}', '${fileId}')" | ||
| onkeydown="if(event.key === 'Enter' || event.key === ' ') { event.preventDefault(); toggleFile('${file}', '${fileId}'); }"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me look at the specific lines in context
head -n 1230 public/index.html | tail -n 20Repository: praxstack/ai-visual-code-review
Length of output: 1346
🏁 Script executed:
# Also, let's search for where this code is generated/how 'file' is populated
rg -n "toggleFile" public/index.html -B 10 -A 2 | head -100Repository: praxstack/ai-visual-code-review
Length of output: 3184
🏁 Script executed:
# Search for how the 'file' variable is created and if any escaping is applied
rg -n "onclick.*toggleFile" public/index.html -B 5 -A 2Repository: praxstack/ai-visual-code-review
Length of output: 629
🏁 Script executed:
# Look at the code that populates selectedFiles and file list
rg -n "selectedFiles|file.*=" public/index.html -B 5 | head -150Repository: praxstack/ai-visual-code-review
Length of output: 7324
🏁 Script executed:
# Search for any escaping functions or sanitization of file paths
rg -n "escape|sanitize|encodeURI|replace" public/index.htmlRepository: praxstack/ai-visual-code-review
Length of output: 794
🏁 Script executed:
# Look at the beginning of the function to understand where file comes from
sed -n '1200,1215p' public/index.htmlRepository: praxstack/ai-visual-code-review
Length of output: 690
🏁 Script executed:
# Look at the escapeHtml function
sed -n '1658,1670p' public/index.htmlRepository: praxstack/ai-visual-code-review
Length of output: 644
🏁 Script executed:
# Check if there are any other instances of inline handlers with file interpolation
rg -n "onclick.*file\|onchange.*file\|onkeydown.*file" public/index.htmlRepository: praxstack/ai-visual-code-review
Length of output: 57
🏁 Script executed:
# Verify if this file comes from user input or external API and could contain quotes
rg -n "fetch.*file.*diff\|/api/" public/index.html | head -20Repository: praxstack/ai-visual-code-review
Length of output: 57
Inline handlers break with file names containing quotes.
The code interpolates raw file paths directly into JavaScript strings within HTML attributes without escaping. A file named docs/John's-note.md will generate invalid JavaScript: onclick="toggleFile('docs/John's-note.md', ...)" causing syntax errors and breaking both click and keyboard toggle functionality.
Affected lines:
- Line 1221-1222:
toggleFile()in onclick and onkeydown - Line 1235:
toggleFileSelection()in onchange - Line 1243:
showCommentModal()in onclick
Replace inline handlers with addEventListener() or use data-* attributes to store the file path and retrieve it via JavaScript instead of interpolating into the handler string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/index.html` around lines 1221 - 1222, Inline JS handlers are breaking
for filenames with quotes; stop interpolating raw file paths into
onclick/onkeydown/onchange attributes (where toggleFile, toggleFileSelection,
and showCommentModal are referenced) and instead add data-* attributes (e.g.,
data-file, data-file-id) to the elements and attach event listeners in your
script using addEventListener to read element.dataset.file and dataset.fileId
and call toggleFile(...), toggleFileSelection(...), or
showCommentModal(...)—remove the inline handler strings so special characters in
file names no longer break the generated JS.
1 participant
💡 What: Made the custom
.file-headeraccordion toggles fully accessible via keyboard by adding semantic ARIA attributes and key handlers.🎯 Why: Keyboard users and screen readers couldn't expand or collapse file diffs because the
<div>elements lacked a button role and focusability.📸 Before/After: Screenshots were taken in the local verification folder; a clear focus ring is now present around the header when tabbed to, and the UI responds to Enter/Space.
♿ Accessibility:
role="button"andtabindex="0".aria-expandedlogic to reflect the expand/collapse state.aria-hidden="true"to decorative icons.onkeydownhandler for Enter/Space keys to toggle files.:focus-visiblestyling for visual focus indicators..Jules/palette.md.PR created automatically by Jules for task 9097906485083077054 started by @praxstack
Summary by CodeRabbit
New Features
Documentation