🎨 Palette: Improve Staff Access Modal UX & Accessibility#38
🎨 Palette: Improve Staff Access Modal UX & Accessibility#38
Conversation
- Add aria-haspopup="dialog" and role="button" to Staff Access link - Add aria-label to private-pass-input - Implement auto-focus and Enter-key submission in js/main.js Co-authored-by: LVT-ENG <214667862+LVT-ENG@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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 and user experience of the 'Staff Access' modal by implementing ARIA roles, focus management, and keyboard support. The review identifies a critical security vulnerability involving client-side password validation and provides suggestions for improving HTML semantics by using button elements and refining JavaScript event listener management.
| input.addEventListener('keydown', (e) => { | ||
| if (e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| this.verifyPrivatePass(); |
There was a problem hiding this comment.
This function call leads to verifyPrivatePass, which performs password validation on the client-side with a hardcoded password ("SAC_MUSEUM_2026"). This is a critical security vulnerability. Anyone can view the page's JavaScript source to find the password and bypass this protection.
All authentication and authorization checks must be performed on the server-side. The client should send the password to a secure backend endpoint for verification.
| <li><a href="#try-on">Búnker</a></li> | ||
| <li><a href="#consultation">P.A.U.</a></li> | ||
| <li><a href="javascript:void(0)" onclick="requestPrivatePass()">Staff Access</a></li> | ||
| <li><a href="javascript:void(0)" onclick="requestPrivatePass()" aria-haspopup="dialog" role="button">Staff Access</a></li> |
There was a problem hiding this comment.
For better accessibility and semantics, it's recommended to use a <button> element for actions instead of an <a> tag with role="button". A native <button> is keyboard-accessible by default (handling both Enter and Space keys) and is the correct semantic element for triggering an action like opening a modal.
You will need to apply styling to the button to make it visually consistent with the other navigation links. Using a class like nav-button can help with this.
| <li><a href="javascript:void(0)" onclick="requestPrivatePass()" aria-haspopup="dialog" role="button">Staff Access</a></li> | |
| <li><button type="button" class="nav-button" onclick="requestPrivatePass()" aria-haspopup="dialog">Staff Access</button></li> |
| if (!this._passListener) { | ||
| input.addEventListener('keydown', (e) => { | ||
| if (e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| this.verifyPrivatePass(); | ||
| } | ||
| }); | ||
| this._passListener = true; | ||
| } |
There was a problem hiding this comment.
To improve code clarity, it's better to use a more descriptive name for the flag that tracks if the event listener has been added. _passListener is ambiguous; it could be a boolean or the listener function itself. A name like _isPassKeyListenerAttached would be more explicit.
For better maintainability and to prevent potential memory leaks, it's a best practice to add event listeners when they are needed and remove them when they are not (e.g., in closePrivatePass()). While the current implementation works, attaching an event listener that is never removed is not ideal in long-running applications.
| if (!this._passListener) { | |
| input.addEventListener('keydown', (e) => { | |
| if (e.key === 'Enter') { | |
| e.preventDefault(); | |
| this.verifyPrivatePass(); | |
| } | |
| }); | |
| this._passListener = true; | |
| } | |
| if (!this._isPassKeyListenerAttached) { | |
| input.addEventListener('keydown', (e) => { | |
| if (e.key === 'Enter') { | |
| e.preventDefault(); | |
| this.verifyPrivatePass(); | |
| } | |
| }); | |
| this._isPassKeyListenerAttached = true; | |
| } |
1 participant
💡 What: Added accessibility attributes (
aria-haspopup,role="button",aria-label), implemented auto-focus, and added "Enter" key support for password submission in the Staff Access modal.🎯 Why: To make the staff login flow more intuitive, accessible for screen readers, and faster for keyboard-centric users.
♿ Accessibility: Improved screen reader identification of the modal trigger and input field.
📸 Before/After: Visual confirmation via Playwright shows the modal opening with the input automatically focused and ready for keyboard input.
PR created automatically by Jules for task 7288896619108026548 started by @LVT-ENG