🎨 Palette: Enhance DuplicateResumeModal accessibility and keyboard support#500
🎨 Palette: Enhance DuplicateResumeModal accessibility and keyboard support#500aafre 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 DuplicateResumeModal by implementing ARIA roles, keyboard support for the Escape key, and improved focus styles for action buttons. Feedback includes a suggestion to optimize the Escape key event listener by conditionally attaching it only when the modal is open, and a recommendation to implement a focus trap to prevent keyboard users from navigating outside the modal while it is active.
| useEffect(() => { | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape' && isOpen) { | ||
| onCancel(); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('keydown', handleEscape); | ||
| return () => window.removeEventListener('keydown', handleEscape); | ||
| }, [isOpen, onCancel]); |
There was a problem hiding this comment.
The keydown event listener is currently attached to the window as long as the component is mounted, even if the modal is not visible. It's more efficient to conditionally add the listener only when isOpen is true. This also simplifies the logic within the handler by removing the need to check isOpen inside the event listener. Additionally, ensure that the onCancel callback is triggered to keep the parent state in sync.
| useEffect(() => { | |
| const handleEscape = (e: KeyboardEvent) => { | |
| if (e.key === 'Escape' && isOpen) { | |
| onCancel(); | |
| } | |
| }; | |
| window.addEventListener('keydown', handleEscape); | |
| return () => window.removeEventListener('keydown', handleEscape); | |
| }, [isOpen, onCancel]); | |
| useEffect(() => { | |
| if (!isOpen) return; | |
| const handleEscape = (e: KeyboardEvent) => { | |
| if (e.key === 'Escape') { | |
| onCancel(); | |
| } | |
| }; | |
| window.addEventListener('keydown', handleEscape); | |
| return () => window.removeEventListener('keydown', handleEscape); | |
| }, [isOpen, onCancel]); |
References
- When refactoring a component to use a simpler API, ensure backward compatibility for legacy controlled patterns by triggering all necessary callbacks (e.g., onStartEdit, onCancel) to keep parent component state in sync.
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="duplicate-modal-title" |
There was a problem hiding this comment.
While ARIA roles and keyboard shortcuts have been added, the modal is still missing a focus trap. According to the general rules, all modals must implement a focus trap to ensure keyboard focus remains within the modal's focusable elements while it is open. This is crucial for accessibility to prevent users from tabbing out of the modal 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.
|
Closing duplicate — the most recent version of this change is PR #533. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
What: Added ARIA roles (
role="dialog",aria-modal),aria-labelledby,Escapekey close functionality, andfocus-visiblestyling to the DuplicateResumeModal component.Why: To ensure the modal is fully accessible to screen readers, can be easily dismissed via keyboard navigation, and provides clear visual focus indicators for keyboard users.
Before/After: The modal previously lacked proper semantic ARIA attributes and keyboard shortcuts. It now functions as a standard accessible dialog.
Accessibility: Screen readers will now correctly identify the modal and announce its title. Keyboard users can dismiss it intuitively with
Escapeand clearly see which button is focused.PR created automatically by Jules for task 8584831921930382650 started by @aafre