Enhance accessibility features across the application#27
Enhance accessibility features across the application#27fk0u wants to merge 1 commit intoOurCreativity:mainfrom
Conversation
- Added skip link to main content in App component for better navigation - Implemented focus trap in ContributorModal and CreationStudio components to manage keyboard navigation - Added aria-labels and titles to various elements for improved screen reader support - Updated Footer and Navbar components with appropriate aria-labels for social media links and menu buttons - Improved Info page with aria-expanded attribute for FAQ buttons - Introduced .hintrc file for accessibility linting configuration
|
@fk0u is attempting to deploy a commit to the drthought's projects Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @fk0u, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! Pull request ini bertujuan untuk secara signifikan meningkatkan aksesibilitas aplikasi, menjadikannya lebih mudah digunakan bagi individu dengan disabilitas, terutama mereka yang mengandalkan pembaca layar dan navigasi keyboard. Perubahan ini memastikan kepatuhan terhadap standar WCAG 2.1 Level AA, yang mengarah pada pengalaman pengguna yang lebih inklusif dan lancar di seluruh platform. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
Secara keseluruhan, PR ini melakukan pekerjaan yang sangat baik dalam meningkatkan aksesibilitas aplikasi, sejalan dengan deskripsi. Penambahan skip link, focus trap, dan atribut ARIA yang relevan adalah langkah-langkah penting.
Saya menemukan beberapa area yang memerlukan perbaikan:
- Terdapat bug kritis pada implementasi focus trap di
ContributorModalyang menyebabkannya tidak berfungsi. - Logika focus trap diduplikasi di dua komponen, yang dapat diekstraksi menjadi custom hook untuk meningkatkan maintainability.
- Ada penggunaan atribut
titleyang tidak efektif pada input file yang tersembunyi.
Setelah perbaikan ini, fitur aksesibilitas akan jauh lebih kuat dan andal. Terima kasih atas kontribusinya!
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); |
There was a problem hiding this comment.
Implementasi focus trap saat ini memiliki bug kritis. Query modalRef.current?.parentElement?.querySelectorAll(...) menargetkan elemen yang salah dan tidak akan menemukan elemen yang dapat difokuskan seperti tombol tutup atau tautan eksternal, karena elemen-elemen tersebut tidak berada di dalam parentElement dari modalRef.current. Akibatnya, focus trap tidak berfungsi seperti yang diharapkan.
Untuk memperbaikinya, Anda perlu menargetkan container modal yang benar, yaitu dua level di atas modalRef.current (parentElement.parentElement).
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | |
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | |
| ); | |
| const focusableElements = modalRef.current?.parentElement?.parentElement?.querySelectorAll( | |
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | |
| ); |
| React.useEffect(() => { | ||
| if (!isOpen) return; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { | ||
| onClose(); | ||
| return; | ||
| } | ||
|
|
||
| if (e.key === 'Tab') { | ||
| const focusableElements = modalRef.current?.querySelectorAll( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); | ||
|
|
||
| if (!focusableElements || focusableElements.length === 0) return; | ||
|
|
||
| const firstElement = focusableElements[0] as HTMLElement; | ||
| const lastElement = focusableElements[focusableElements.length - 1] as HTMLElement; | ||
|
|
||
| if (e.shiftKey) { | ||
| if (document.activeElement === firstElement) { | ||
| e.preventDefault(); | ||
| lastElement.focus(); | ||
| } | ||
| } else { | ||
| if (document.activeElement === lastElement) { | ||
| e.preventDefault(); | ||
| firstElement.focus(); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown); | ||
| // Focus the close button initially | ||
| setTimeout(() => closeButtonRef.current?.focus(), 100); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| }, [isOpen, onClose]); |
There was a problem hiding this comment.
Logika untuk focus trap di dalam useEffect ini diduplikasi di dua komponen: CreationStudio.tsx dan ContributorModal.tsx. Ini membuat kode lebih sulit untuk dipelihara.
Saya sarankan untuk mengekstrak logika ini ke dalam custom hook yang dapat digunakan kembali, misalnya useFocusTrap. Ini akan membuat kode lebih bersih, mengurangi duplikasi, dan mempermudah pemeliharaan di masa mendatang.
Contoh struktur hook:
function useFocusTrap(containerRef, initialFocusRef, isOpen, onClose) {
useEffect(() => {
if (!isOpen) return;
// ... all the logic from handleKeyDown ...
document.addEventListener('keydown', handleKeyDown);
setTimeout(() => initialFocusRef.current?.focus(), 100);
return () => {
document.removeEventListener('keydown', handleKeyDown);
};
}, [isOpen, onClose, containerRef, initialFocusRef]);
}| onChange={handleImageUpload} | ||
| accept="image/*" | ||
| className="hidden" | ||
| title="Upload Image" |
There was a problem hiding this comment.
Atribut title pada elemen <input> yang tersembunyi (className="hidden") tidak memberikan manfaat aksesibilitas karena elemen ini tidak dapat difokuskan atau diakses oleh screen reader.
Untuk meningkatkan aksesibilitas, area yang dapat diklik (div yang membungkus input ini) harus dibuat dapat diakses. Anda dapat melakukannya dengan menambahkan atribut berikut ke div tersebut:
role="button"tabIndex="0"aria-label="Upload Image"- Handler
onKeyDownuntuk memicu klik saat tombolEnteratauSpasiditekan.
Karena title ini tidak efektif, saya sarankan untuk menghapusnya. Hal yang sama berlaku untuk input unggah video di baris 352.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements accessibility improvements to meet WCAG 2.1 Level AA standards, including skip links, focus trap modals, and ARIA attributes for better screen reader and keyboard navigation support.
Key Changes:
- Added skip link functionality to bypass navigation and jump directly to main content
- Implemented focus trap logic in modals (CreationStudio and ContributorModal) to contain keyboard navigation
- Enhanced ARIA attributes across interactive elements (buttons, accordions, links) for screen reader accessibility
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| App.tsx | Added skip link with sr-only utility and assigned id="main-content" to main element |
| components/Navbar.tsx | Added aria-label to logo link and mobile menu toggle button |
| components/Footer.tsx | Added aria-label attributes to social media icon links |
| pages/Info.tsx | Added aria-expanded to FAQ accordion buttons; refactored inline SVG style to Tailwind class |
| components/CreationStudio/index.tsx | Implemented focus trap with refs, added aria-labels to icon buttons, associated form labels with inputs via htmlFor/id |
| components/ContributorModal.tsx | Implemented focus trap, added aria-label to close button and GitHub link, refactored inline style to Tailwind class |
| .hintrc | Added webhint configuration with aria-valid-attr-value check disabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Instagram size={20} /> | ||
| </a> | ||
| <a href="https://www.tiktok.com/@ourcreativity.ofc" target="_blank" rel="noopener noreferrer" className="text-gray-500 hover:text-white transition-colors" title="TikTok"> | ||
| <a href="https://www.tiktok.com/@ourcreativity.ofc" target="_blank" rel="noopener noreferrer" className="text-gray-500 hover:text-white transition-colors" title="TikTok" aria-label="TikTok"> |
There was a problem hiding this comment.
Adding both title and aria-label with identical values is redundant. Screen readers will announce the aria-label, and sighted users will see the title as a tooltip. For icon-only links, aria-label alone is sufficient for screen reader users. The title attribute can be removed to reduce duplication, or kept if you specifically want a visual tooltip for sighted users (though the redundancy is acceptable in this case).
| setTimeout(() => closeButtonRef.current?.focus(), 100); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); |
There was a problem hiding this comment.
The setTimeout call is not cleaned up if the component unmounts or isOpen changes before the timeout executes. This could cause a focus attempt on an unmounted component, resulting in errors or memory leaks. Store the timeout ID and clear it in the cleanup function.
| setTimeout(() => closeButtonRef.current?.focus(), 100); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| const timeoutId = window.setTimeout(() => closeButtonRef.current?.focus(), 100); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| clearTimeout(timeoutId); |
| setTimeout(() => closeButtonRef.current?.focus(), 100); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); |
There was a problem hiding this comment.
The setTimeout call is not cleaned up if the component unmounts or isOpen changes before the timeout executes. This could cause a focus attempt on an unmounted component, resulting in errors or memory leaks. Store the timeout ID and clear it in the cleanup function.
| setTimeout(() => closeButtonRef.current?.focus(), 100); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| const timeoutId = window.setTimeout(() => closeButtonRef.current?.focus(), 100); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| clearTimeout(timeoutId); |
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="p-3 rounded-full bg-white/5 text-white hover:bg-white hover:text-black transition-all duration-300" | ||
| title="Lihat profil GitHub" |
There was a problem hiding this comment.
Adding both title and aria-label with identical values is redundant. Screen readers will announce the aria-label, and sighted users will see the title as a tooltip. For icon-only links, aria-label alone is sufficient for screen reader users. The title attribute can be removed to reduce duplication, or kept if you specifically want a visual tooltip for sighted users (though the redundancy is acceptable in this case).
| title="Lihat profil GitHub" |
| } | ||
|
|
||
| if (e.key === 'Tab') { | ||
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( |
There was a problem hiding this comment.
The focus trap query selector is using modalRef.current?.parentElement?.querySelectorAll() which searches beyond the modal content. Looking at the DOM structure, modalRef is assigned to the inner scrollable content div, and parentElement would be the motion.div containing the entire modal window. However, this inconsistency with CreationStudio (which uses modalRef.current directly) could lead to including elements outside the intended modal scope. For proper focus containment, the query should be scoped to the actual modal container, not its parent element.
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | |
| const focusableElements = modalRef.current?.querySelectorAll( |
| onChange={handleImageUpload} | ||
| accept="image/*" | ||
| className="hidden" | ||
| title="Upload Image" |
There was a problem hiding this comment.
The title attribute on hidden file inputs is not meaningful for accessibility. Screen readers won't announce these hidden inputs, and sighted users won't see them. The title is typically used for tooltips on visible, interactive elements. Consider removing these title attributes or ensuring they serve a clear purpose in your accessibility strategy.
| title="Upload Image" |
| onChange={handleImageUpload} | ||
| accept="video/*" | ||
| className="hidden" | ||
| title="Upload Video" |
There was a problem hiding this comment.
The title attribute on hidden file inputs is not meaningful for accessibility. Screen readers won't announce these hidden inputs, and sighted users won't see them. The title is typically used for tooltips on visible, interactive elements. Consider removing these title attributes or ensuring they serve a clear purpose in your accessibility strategy.
| title="Upload Video" |
| value={formData.division} | ||
| onChange={(e) => setFormData(prev => ({ ...prev, division: e.target.value as any }))} | ||
| className="w-full bg-[#111] border border-white/10 rounded-xl px-4 py-3 text-white focus:outline-none" | ||
| title="Select Division" |
There was a problem hiding this comment.
Adding a title attribute to a select element that already has an associated label (via htmlFor/id) is redundant. The label provides the accessible name, and the title would only create a tooltip on hover. For accessibility, the label association is sufficient and preferred. Consider removing the title attribute to avoid duplication.
| title="Select Division" |
| "default", | ||
| { | ||
| "aria-valid-attr-value": "off" | ||
| } |
There was a problem hiding this comment.
Disabling the 'aria-valid-attr-value' rule in the hint configuration removes an important accessibility check that validates ARIA attribute values. This rule helps catch invalid ARIA attribute values that could break screen reader functionality. Unless there's a specific false positive you're addressing, consider removing this override to maintain accessibility validation. If there is a specific issue, document it and use more targeted exclusions.
| "default", | |
| { | |
| "aria-valid-attr-value": "off" | |
| } | |
| "default" |
|
|
||
| <div className="flex items-center gap-8"> | ||
| <a href="https://www.instagram.com/ourcreativity.ofc/" target="_blank" rel="noopener noreferrer" className="text-gray-500 hover:text-white transition-colors" title="Instagram"> | ||
| <a href="https://www.instagram.com/ourcreativity.ofc/" target="_blank" rel="noopener noreferrer" className="text-gray-500 hover:text-white transition-colors" title="Instagram" aria-label="Instagram"> |
There was a problem hiding this comment.
Adding both title and aria-label with identical values is redundant. Screen readers will announce the aria-label, and sighted users will see the title as a tooltip. For icon-only links, aria-label alone is sufficient for screen reader users. The title attribute can be removed to reduce duplication, or kept if you specifically want a visual tooltip for sighted users (though the redundancy is acceptable in this case).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); |
There was a problem hiding this comment.
🟡 Focus trap in ContributorModal excludes close button due to incorrect DOM traversal
The focus trap implementation in ContributorModal queries focusable elements using modalRef.current?.parentElement?.querySelectorAll(...) (line 52). However, modalRef is attached to an inner content div (line 145), making parentElement the scrollable container div (line 144). The close button (lines 134-141) is a sibling of this scrollable container, not a child of it.
DOM structure:
<motion.div> (modal window - line 129)
<button ref={closeButtonRef}> (close button - line 134) ← NOT included in query
<div> (scrollable container - line 144) ← This is modalRef.current.parentElement
<div ref={modalRef}> (content - line 145)
When Tab is pressed, the focus trap calculates firstElement and lastElement from elements inside the scrollable container only. The close button is excluded from this list. This means:
- If the user tabs to the close button and then tabs again, the focus trap logic won't work correctly because
document.activeElement(the close button) won't matchlastElement(which is inside the scrollable container) - The focus trap doesn't properly contain focus within all interactive elements of the modal
This undermines the accessibility goal of the focus trap implementation.
Recommendation: Query from the modal window container instead. Either move modalRef to the modal window div (line 129's motion.div), or use a separate ref for the focus trap that encompasses both the close button and the scrollable content. For example: const focusableElements = modalRef.current?.parentElement?.parentElement?.querySelectorAll(...) to get the modal window, or better yet, add a new ref to the motion.div at line 129.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -54,6 +54,51 @@ export const CreationStudio: React.FC<Props> = ({ isOpen, onClose, onPublish }) | |||
|
|
|||
There was a problem hiding this comment.
(Refers to lines 52-53)
🚩 consoleOutput state is accumulated but never displayed
The consoleOutput state variable at components/CreationStudio/index.tsx:52 is set via setConsoleOutput callbacks from PyodideSandbox (lines 368-369), but the accumulated output is never rendered or displayed anywhere in the component. This appears to be intentional infrastructure for future Python console output display, but currently the output is lost. The state keeps accumulating with prev + out which could lead to memory growth if many Python scripts are run in a session.
Was this helpful? React with 👍 or 👎 to provide feedback.
📋 Deskripsi
PR ini mengimplementasikan perbaikan aksesibilitas (Accessibility/a11y) untuk memenuhi standar WCAG 2.1 Level AA, serta meningkatkan pengalaman pengguna screen reader dan keyboard.
Perubahan mencakup:
aria-label,aria-expanded, dan atribut aksesibilitas lainnya pada elemen interaktif (tombol icon, accordion).🔗 Issue Terkait
Fixes #6 (Accessibility Improvements)
🔄 Tipe Perubahan
📸 Screenshot
Tabpertama kali di halaman.✅ Checklist
📝 Catatan untuk Reviewer
Mohon perhatikan implementasi
useEffectpadaContributorModaldanCreationStudiountuk logic focus trap. Pastikan tidak ada memory leak pada event listenerkeydown.