-
Notifications
You must be signed in to change notification settings - Fork 1
Enhance accessibility features across the application #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "extends": [ | ||
| "development" | ||
| ], | ||
| "hints": { | ||
| "axe/aria": [ | ||
| "default", | ||
| { | ||
| "aria-valid-attr-value": "off" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||
| import React, { useRef } from 'react'; | ||||||||||||||||||||
| import React, { useRef, useEffect } from 'react'; | ||||||||||||||||||||
| import { motion, AnimatePresence } from 'framer-motion'; | ||||||||||||||||||||
| import { Download, ExternalLink, X, Sparkles, Wand2, Heart, Palette } from 'lucide-react'; | ||||||||||||||||||||
| import html2canvas from 'html2canvas'; | ||||||||||||||||||||
|
|
@@ -36,6 +36,50 @@ export const ContributorModal: React.FC<ContributorDetailProps> = ({ | |||||||||||||||||||
| isOwner, | ||||||||||||||||||||
| }) => { | ||||||||||||||||||||
| const modalRef = useRef<HTMLDivElement>(null); | ||||||||||||||||||||
| const closeButtonRef = useRef<HTMLButtonElement>(null); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Focus Trap | ||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||
| if (!isOpen) return; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const handleKeyDown = (e: KeyboardEvent) => { | ||||||||||||||||||||
| if (e.key === 'Escape') { | ||||||||||||||||||||
| onClose(); | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (e.key === 'Tab') { | ||||||||||||||||||||
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | ||||||||||||||||||||
|
||||||||||||||||||||
| const focusableElements = modalRef.current?.parentElement?.querySelectorAll( | |
| const focusableElements = modalRef.current?.querySelectorAll( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"])' | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π‘ 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.
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,6 +54,51 @@ export const CreationStudio: React.FC<Props> = ({ isOpen, onClose, onPublish }) | |||||||||||||||||||
|
|
||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Refers to lines 52-53) π© consoleOutput state is accumulated but never displayed The Was this helpful? React with π or π to provide feedback. |
||||||||||||||||||||
| // Refs | ||||||||||||||||||||
| const fileInputRef = useRef<HTMLInputElement>(null); | ||||||||||||||||||||
| const modalRef = useRef<HTMLDivElement>(null); | ||||||||||||||||||||
| const closeButtonRef = useRef<HTMLButtonElement>(null); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Focus Trap | ||||||||||||||||||||
| 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); | ||||||||||||||||||||
|
Comment on lines
+96
to
+99
|
||||||||||||||||||||
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,10 @@ export const Footer = () => { | |
| </div> | ||
|
|
||
| <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"> | ||
|
||
| <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"> | ||
|
||
| <Music2 size={20} /> | ||
| </a> | ||
| </div> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.