🎨 Palette: Improve keyboard focus visibility in ResponsiveConfirmDialog#452
🎨 Palette: Improve keyboard focus visibility in ResponsiveConfirmDialog#452aafre wants to merge 1 commit into
Conversation
…tons 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 introduces explicit focus indicators to the ResponsiveConfirmDialog component and updates the .Jules/palette.md documentation to reflect this accessibility standard. The review feedback points out that the implementation uses gray-400 for focus rings, which lacks sufficient contrast and deviates from the newly documented preference for the accent color. Suggestions were made to use focus-visible:ring-accent and focus-visible:ring-offset-2 to ensure consistency and better visibility.
| <button | ||
| onClick={onClose} | ||
| className="flex-shrink-0 text-gray-400 hover:text-gray-600 transition-colors p-1 -mr-1" | ||
| className="flex-shrink-0 text-gray-400 hover:text-gray-600 transition-colors p-1 -mr-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-gray-400 rounded" |
There was a problem hiding this comment.
The focus ring color gray-400 provides insufficient contrast (approx. 2.5:1) against the white background, which fails to meet the WCAG 2.1 recommended 3:1 ratio for non-text elements. Additionally, this implementation is inconsistent with the new guideline added in .Jules/palette.md which specifies focus-visible:ring-accent and focus-visible:ring-offset-2. For icon buttons, rounded-full is also preferred to ensure a clean circular focus indicator.
| className="flex-shrink-0 text-gray-400 hover:text-gray-600 transition-colors p-1 -mr-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-gray-400 rounded" | |
| className="flex-shrink-0 text-gray-400 hover:text-gray-600 transition-colors p-1 -mr-1 focus:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 rounded-full" |
References
- Ensure focus indicators follow the .Jules/palette.md guideline using focus-visible:ring-accent and focus-visible:ring-offset-2. (link)
| disabled={isLoading} | ||
| className="w-full lg:w-auto px-6 py-3 border border-gray-300 rounded-lg font-medium text-gray-700 | ||
| hover:bg-gray-50 active:bg-gray-100 | ||
| hover:bg-gray-50 active:bg-gray-100 focus:outline-none focus-visible:ring-2 focus-visible:ring-gray-400 focus-visible:ring-offset-2 |
There was a problem hiding this comment.
Using focus-visible:ring-gray-400 here is inconsistent with the focus-visible:ring-accent standard established in the .Jules/palette.md update. Moreover, the low contrast of gray-400 on white makes the focus state difficult to perceive. Aligning this with the accent color would improve both consistency and accessibility.
| hover:bg-gray-50 active:bg-gray-100 focus:outline-none focus-visible:ring-2 focus-visible:ring-gray-400 focus-visible:ring-offset-2 | |
| hover:bg-gray-50 active:bg-gray-100 focus:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 |
References
- Ensure focus indicators follow the .Jules/palette.md guideline using focus-visible:ring-accent and focus-visible:ring-offset-2. (link)
1 participant
💡 What: Added explicit focus indicators (
focus:outline-none,focus-visible:ring-2,focus-visible:ring-gray-400,focus-visible:ring-offset-2) to the interactive buttons (Close, Cancel, Confirm) withinResponsiveConfirmDialog.🎯 Why: When generic custom dialog components lack explicit
focus-visiblestyling on primary interactable elements, users navigating via keyboard cannot visually discern which element is currently focused. This addresses an accessibility concern to improve keyboard usability for the entire app across instances utilizing this modal.♿ Accessibility: Added explicit focus-visible Tailwind classes mapping to user agent focus behaviors, establishing a stronger visual boundary around active buttons.
PR created automatically by Jules for task 11361179987278779790 started by @aafre