🎨 Palette: Improve keyboard focus visibility and accessibility for inline controls#512
🎨 Palette: Improve keyboard focus visibility and accessibility for inline controls#512aafre wants to merge 1 commit into
Conversation
…line controls Co-authored-by: aafre <8656674+aafre@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. |
![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 accessibility and keyboard navigation by adding focus-visible rings, type="button", and aria-label attributes to icon buttons across several components. It also updates the project's learning documentation to include these accessibility standards. Reviewer feedback focused on maintaining color consistency for focus rings and utilizing SVG components from lucide-react instead of raw text characters for better visual quality.
| aria-label="Cancel Editing Title" | ||
| onClick={onCancel} | ||
| className="text-red-600 hover:text-red-800 transition-colors" | ||
| className="text-red-600 hover:text-red-800 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-500 focus-visible:ring-offset-1 rounded" |
There was a problem hiding this comment.
The focus ring color (red-500) is inconsistent with the button's text color (red-600) and the 'Save' button's ring color (green-600). Using red-600 provides better visual consistency and slightly better contrast. Additionally, explicitly set type='button' on this button to prevent unintended form submissions.
| className="text-red-600 hover:text-red-800 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-500 focus-visible:ring-offset-1 rounded" | |
| type="button" className="text-red-600 hover:text-red-800 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-600 focus-visible:ring-offset-1 rounded" |
References
- Explicitly set type='button' on elements that are not intended to submit a form to prevent unintended form submissions.
| type="button" | ||
| onClick={() => handleDescRemove(descIndex)} | ||
| className="text-red-600 hover:text-red-800 p-2 hover:bg-red-50 rounded-lg transition-colors flex-shrink-0 mt-2" | ||
| className="text-red-600 hover:text-red-800 p-2 hover:bg-red-50 rounded-lg transition-colors flex-shrink-0 mt-2 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-500 focus-visible:ring-offset-1" |
There was a problem hiding this comment.
The focus ring color (red-500) is inconsistent with the button's text color (red-600). Using red-600 ensures visual consistency across the UI and matches the color weight of the action. Also, ensure the button has type='button' to avoid accidental form submission.
| className="text-red-600 hover:text-red-800 p-2 hover:bg-red-50 rounded-lg transition-colors flex-shrink-0 mt-2 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-500 focus-visible:ring-offset-1" | |
| type="button" className="text-red-600 hover:text-red-800 p-2 hover:bg-red-50 rounded-lg transition-colors flex-shrink-0 mt-2 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-red-600 focus-visible:ring-offset-1" |
References
- Explicitly set type='button' on elements that are not intended to submit a form to prevent unintended form submissions.
| className="ml-2 hover:opacity-75 text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-1 focus-visible:ring-offset-accent rounded" | ||
| aria-label="Dismiss reminder" | ||
| > | ||
| ✕ |
There was a problem hiding this comment.
For consistency with the mobile dismiss button in this file (line 231) and to ensure a high-quality SVG rendering across all platforms, consider using the X component from lucide-react instead of the raw ✕ character.
| ✕ | |
| <X className="w-4 h-4" /> |
|
Closing duplicate — the most recent version of this change is PR #524. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
💡 What
Added explicit keyboard focus outlines (
focus-visible) and missingtype="button"/aria-labelattributes to various inline icon-only controls (e.g., remove description points, title editing actions, dismiss tooltips).🎯 Why
Custom Tailwind padding and background colors can obscure default browser focus outlines. This made it difficult or impossible for keyboard-only users to see which inline action was currently focused.
📸 Before/After
Before: Tabbing to the "✕" or "✅" buttons provided no visual feedback.
After: Tabbing to these buttons displays a clear, accessible focus ring.
♿ Accessibility
focus-visible:ring-2) on inline controls.type="button"is present on all icon buttons.aria-labels.PR created automatically by Jules for task 10293947439441721319 started by @aafre