🎨 Palette: Add accessible close buttons to modals#517
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 standardizes modal close interactions by adding aria-label attributes and focus-visible ring utilities to close buttons across several components, while also updating the project's accessibility documentation. Feedback focuses on ensuring WCAG compliance, specifically regarding the contrast ratio of focus indicators on light backgrounds and increasing hit areas for touch targets. Additionally, reviewers noted the need for a focus trap in PreviewModal and an Escape key listener in TabbedHelpModal to maintain consistent accessibility patterns.
| <button | ||
| onClick={onClose} | ||
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors" | ||
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" |
There was a problem hiding this comment.
The focus-visible:ring-accent utility uses the brand accent color (#00d47e), which has a contrast ratio of approximately 1.5:1 against the white background. This fails the WCAG 2.1 success criterion 1.4.11 (Non-text Contrast), which requires a minimum ratio of 3:1 for user interface components like focus indicators. Consider using a darker color for the focus ring on light backgrounds to ensure it is clearly visible to users with low vision.
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" | |
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ink focus-visible:ring-offset-2" |
| <button | ||
| onClick={onClose} | ||
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors" | ||
| className="p-2 text-gray-500 hover:text-gray-700 hover:bg-gray-100 rounded-lg transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" |
There was a problem hiding this comment.
While improving the accessibility of the close button, please note that this modal currently lacks a focus trap. According to the repository's general rules, all modals must implement a focus trap to ensure keyboard focus remains within the modal while it is open. This is critical for users navigating via keyboard or screen readers.
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.
| <button | ||
| onClick={onClose} | ||
| className="text-gray-400 hover:text-gray-600 transition-colors" | ||
| className="text-gray-400 hover:text-gray-600 transition-colors rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" |
There was a problem hiding this comment.
The focus-visible:ring-accent utility fails WCAG 1.4.11 contrast requirements on light backgrounds. Additionally, the button lacks padding, resulting in a small hit area (24x24px) and a tight focus ring. Adding padding improves both the touch target size (WCAG 2.5.5/2.5.8) and the visual clarity of the focus indicator.
| className="text-gray-400 hover:text-gray-600 transition-colors rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" | |
| className="p-1 text-gray-400 hover:text-gray-600 transition-colors rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ink focus-visible:ring-offset-2" |
| onClick={onClose} | ||
| className="text-gray-400 hover:text-gray-600 transition-colors" | ||
| className="text-gray-400 hover:text-gray-600 transition-colors rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2" | ||
| aria-label="Close modal" |
There was a problem hiding this comment.
This modal is missing a keyboard event listener for the Escape key, which is inconsistent with PreviewModal and standard accessibility patterns for modals. Since this PR aims to standardize modal close interactions, please ensure all modals support closing via the Escape key.
| <button | ||
| onClick={handleCloseModal} | ||
| className="text-white/80 hover:text-white transition-colors" | ||
| className="text-white/80 hover:text-white transition-colors rounded-md focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 focus-visible:ring-offset-ink" |
There was a problem hiding this comment.
The close button lacks padding, resulting in a small interactive target. Adding p-1 improves the hit area for touch users. Also, using rounded-lg would be more consistent with the other modals modified in this PR.
| className="text-white/80 hover:text-white transition-colors rounded-md focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 focus-visible:ring-offset-ink" | |
| className="p-1 text-white/80 hover:text-white transition-colors rounded-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-2 focus-visible:ring-offset-ink" |
|
Closing duplicate — the most recent version of this change is PR #548. |
1 participant
What: Added explicit
aria-label="Close modal"attributes and visible keyboard focus states (using Tailwindfocus-visible:ring-2utilities) to the icon-only close buttons inPreviewModal,UploadResumeModal, andTabbedHelpModal.Why: Improves keyboard navigation by ensuring focused elements are visibly distinct, and enhances screen reader accessibility by providing an explicit accessible name for icon-only buttons.
Before/After: Before, the close buttons had no visible focus ring when navigated via keyboard, and some lacked accessible names. After, they have clear focus rings and
aria-labels.Accessibility: Addressed WCAG 2.4.7 (Focus Visible) and WCAG 4.1.2 (Name, Role, Value) by ensuring keyboard focus is perceivable and controls have proper names.
PR created automatically by Jules for task 3536034801201971057 started by @aafre