🎨 Palette: Improve AuthModal accessibility and keyboard navigation#548
🎨 Palette: Improve AuthModal accessibility and keyboard navigation#548aafre 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 enhances the accessibility of the AuthModal component by implementing an Escape key listener, adding standard ARIA attributes (role, aria-modal, aria-labelledby), and improving focus styles for the close button. It also updates the project's accessibility guidelines in palette.md. The review feedback points out that a focus trap is still missing in the modal implementation, which is necessary to prevent keyboard users from navigating outside the dialog while it is active, and suggests updating the documentation to include this requirement.
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { | ||
| onClose(); | ||
| } | ||
| }; | ||
|
|
||
| if (isOpen) { | ||
| window.addEventListener('keydown', handleKeyDown); | ||
| } | ||
|
|
||
| return () => { | ||
| window.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| }, [isOpen, onClose]); |
There was a problem hiding this comment.
While adding the Escape key listener is a significant improvement for accessibility, the repository's general rules require all modals to implement a focus trap. This ensures that keyboard focus remains within the modal's focusable elements while it is open, preventing users from accidentally tabbing out of the dialog into the background content.
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.
|
|
||
| ## 2025-02-14 - Modal Keyboard Accessibility and Escape Key | ||
| **Learning:** Custom modals like `AuthModal.tsx` initially lacked basic modal behavior traits, such as listening for the `Escape` key and standard ARIA attributes (`role="dialog"`, `aria-modal="true"`, `aria-labelledby`). Additionally, the `MdClose` icon buttons in such modals frequently missed keyboard focus styles. | ||
| **Action:** When implementing or updating custom modals, always attach a `keydown` event listener via `useEffect` to handle the `Escape` key for closure. Add `role="dialog"`, `aria-modal="true"`, and link titles using `aria-labelledby`. Ensure all icon-only action buttons use `focus:outline-none focus-visible:ring-2` to provide clear feedback to keyboard users. |
There was a problem hiding this comment.
The action item should be updated to include the requirement for implementing a focus trap in custom modals, ensuring consistency with the repository's general accessibility rules.
| **Action:** When implementing or updating custom modals, always attach a `keydown` event listener via `useEffect` to handle the `Escape` key for closure. Add `role="dialog"`, `aria-modal="true"`, and link titles using `aria-labelledby`. Ensure all icon-only action buttons use `focus:outline-none focus-visible:ring-2` to provide clear feedback to keyboard users. | |
| Action: When implementing or updating custom modals, always attach a keydown event listener via useEffect to handle the Escape key for closure and implement a focus trap. Add role="dialog", aria-modal="true", and link titles using aria-labelledby. Ensure all icon-only action buttons use focus:outline-none focus-visible:ring-2 to provide clear feedback to keyboard users. |
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.
- Ensure documentation, such as testing plans, is updated to reflect UI and implementation changes made in the same pull request.
1 participant
What: Added
Escapekey functionality to close the AuthModal, addedrole="dialog"andaria-modal="true", and improved the focus visibility on the close button.Why: To ensure users relying on keyboards and screen readers can properly interact with and dismiss the authentication modal.
Before/After: The modal now correctly acts as an accessible dialog and closes on
Escape. The close button shows a clear focus ring.Accessibility: Added standard dialog ARIA attributes and a visible focus ring to the icon-only close button.
PR created automatically by Jules for task 16116905777940714228 started by @aafre