🎨 Palette: Add keyboard focus indicators to confirmation dialogs#563
🎨 Palette: Add keyboard focus indicators to confirmation dialogs#563aafre wants to merge 1 commit into
Conversation
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 button accessibility in the ResponsiveConfirmDialog component by adding explicit button types and focus-visible Tailwind styles, and documents this learning in .jules/palette.md. The review feedback points out a critical accessibility issue: the modal currently lacks a focus trap, allowing keyboard users to tab out of the dialog. It is recommended to refactor the component using Headless UI's Dialog or to implement a custom focus trap.
| <button | ||
| type="button" | ||
| onClick={onClose} | ||
| 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 | ||
| transition-colors disabled:opacity-50 disabled:cursor-not-allowed | ||
| focus:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 | ||
| min-h-[48px] lg:min-h-[44px]" |
There was a problem hiding this comment.
While adding focus-visible styles to the buttons is a great improvement for keyboard navigation, this modal currently lacks a focus trap. Without a focus trap, keyboard users can tab out of the modal into the background page, which violates accessibility standards (WCAG 2.1) and our general rules.
Since @headlessui/react is already a dependency in this project (used in TabbedHelpModal.tsx), we highly recommend refactoring this component to use Headless UI's Dialog (or FocusTrap), which automatically handles focus trapping, accessibility attributes, and backdrop behavior out of the box.
Alternatively, you can implement a custom focus trap using a useEffect hook and a useRef on the dialog container to intercept the Tab key and wrap focus between the first and last focusable elements.
References
- For accessibility, all modals must implement a focus trap to ensure keyboard focus remains within the modal's focusable elements while it is open.
1 participant
💡 What: Added proper
focus-visiblestates to action buttons inResponsiveConfirmDialog. Also added the explicittype="button"attribute.🎯 Why: To improve keyboard accessibility. Custom styled Tailwind buttons often lose their native browser focus outline, making it difficult or impossible for keyboard users to navigate modal actions.
♿ Accessibility:
focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2to all action and close buttons.focus-visible:ring-red-500and normal actions withfocus-visible:ring-accent.type="button"to ensure screen readers properly announce the elements and to prevent unintended form submissions.PR created automatically by Jules for task 1778306127933000535 started by @aafre