Skip to content

🎨 Palette: Improve DuplicateResumeModal accessibility and interaction#533

Open
aafre wants to merge 1 commit into
mainfrom
palette/duplicate-modal-a11y-13457695637123231205
Open

🎨 Palette: Improve DuplicateResumeModal accessibility and interaction#533
aafre wants to merge 1 commit into
mainfrom
palette/duplicate-modal-a11y-13457695637123231205

Conversation

@aafre
Copy link
Copy Markdown
Owner

@aafre aafre commented May 15, 2026

💡 What:
Added accessibility and interaction improvements to the DuplicateResumeModal. This includes backdrop click to close, Escape key support, ARIA roles, and proper focus ring visibility for buttons.

🎯 Why:
The modal previously required users to explicitly click the "Cancel" button to close it, which was not intuitive. It also lacked necessary ARIA attributes for screen readers and did not clearly indicate focus state for keyboard navigation.

📸 Before/After:
Visually similar, but buttons now show a distinct outline when focused via keyboard, and the modal can be closed by clicking the backdrop or pressing the Escape key.

♿ Accessibility:

  • Added role="dialog", aria-modal="true", and aria-labelledby to the modal container.
  • Linked the modal title h2 with the aria-labelledby ID.
  • Added aria-hidden="true" to the decorative SVG icon.
  • Added focus-visible:ring-2 focus-visible:ring-offset-2 classes to the "Duplicate" and "Cancel" buttons.
  • Added Escape key event listener for quick closing.

PR created automatically by Jules for task 13457695637123231205 started by @aafre

Added backdrop click handling, Escape key support, ARIA attributes, and explicit keyboard focus outlines to DuplicateResumeModal for improved accessibility.

Co-authored-by: aafre <8656674+aafre@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves the accessibility of the DuplicateResumeModal by implementing Escape key support, backdrop click-to-close functionality, and standard ARIA attributes. It also updates the .Jules/palette.md documentation to include these accessibility practices. Feedback focuses on optimizing the event listener attachment to trigger only when the modal is open, implementing a mandatory focus trap for the modal as per repository rules, and ensuring the documentation reflects this focus trap requirement.

Comment on lines +52 to +58
<div
className="bg-white rounded-lg shadow-xl max-w-md w-full mx-4"
onClick={(e) => e.stopPropagation()}
role="dialog"
aria-modal="true"
aria-labelledby="duplicate-modal-title"
>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While the modal now includes several accessibility improvements, it is missing 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's focusable elements while it is open. This prevents users from accidentally tabbing out of the modal into the background content.

References
  1. For accessibility, all modals must implement a focus trap to ensure keyboard focus remains within the modal's focusable elements while it is open.

Comment on lines +27 to +36
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape' && isOpen && !isDuplicating) {
onCancel();
}
};

window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
}, [isOpen, isDuplicating, onCancel]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The keydown event listener is currently attached to the window as soon as the component is mounted, even if the modal is not open. It's more efficient to only attach the listener when isOpen is true. This also allows removing the redundant isOpen check inside the handler.

Suggested change
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape' && isOpen && !isDuplicating) {
onCancel();
}
};
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
}, [isOpen, isDuplicating, onCancel]);
useEffect(() => {
if (!isOpen) return;
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape' && !isDuplicating) {
onCancel();
}
};
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
}, [isOpen, isDuplicating, onCancel]);

Comment thread .Jules/palette.md
**Action:** Always use `ResponsiveConfirmDialog` for destructive confirmation prompts (such as `DeleteResumeModal`) to ensure a consistent, accessible, and mobile-friendly UX that prevents accidental data loss.
## 2024-05-15 - DuplicateResumeModal Accessibility Enhancement
**Learning:** Custom modals like DuplicateResumeModal often lack basic interactive backdrop close handling, keyboard handlers, ARIA roles, and keyboard navigation outline visibility, which impairs accessibility.
**Action:** When creating or modifying custom modals, ensure `onClick` handlers for the backdrop correctly ignore inner modal clicks (via `stopPropagation`), handle the `Escape` key, assign `role="dialog"`, use `aria-modal="true"`, link titles via `aria-labelledby`, add `aria-hidden` to decorative items, and add explicit `focus-visible` outline styles to buttons.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The action item for custom modals should include the requirement for a focus trap, as specified in the repository's general rules.

Suggested change
**Action:** When creating or modifying custom modals, ensure `onClick` handlers for the backdrop correctly ignore inner modal clicks (via `stopPropagation`), handle the `Escape` key, assign `role="dialog"`, use `aria-modal="true"`, link titles via `aria-labelledby`, add `aria-hidden` to decorative items, and add explicit `focus-visible` outline styles to buttons.
Action: When creating or modifying custom modals, ensure they implement a focus trap, onClick handlers for the backdrop correctly ignore inner modal clicks (via stopPropagation), handle the Escape key, assign role="dialog", use aria-modal="true", link titles via aria-labelledby, add aria-hidden to decorative items, and add explicit focus-visible outline styles to buttons.
References
  1. For accessibility, all modals must implement a focus trap to ensure keyboard focus remains within the modal's focusable elements while it is open.
  2. Ensure documentation, such as testing plans, is updated to reflect UI and implementation changes made in the same pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant