feat(accessibility): Implement WCAG 2.1 AA Accessibility Features#90
Merged
OlufunbiIK merged 2 commits intoOlufunbiIK:mainfrom Feb 26, 2026
Merged
Conversation
- Add eslint-plugin-jsx-a11y and @axe-core/react for automated a11y testing - Create accessibility utilities (focus management, screen reader, keyboard shortcuts) - Add useReducedMotion, useFocusTrap, useKeyboardShortcuts, useAnnouncer hooks - Create a11y components: SkipLink, LiveRegion, VisuallyHidden, KeyboardShortcutHelp - Update Button component with loading states and proper ARIA attributes - Update Modal component with focus trap and keyboard navigation - Update MusicPlayer with full keyboard accessibility and screen reader announcements - Update ProgressBar and VolumeControl with keyboard interaction - Add visible focus indicators and reduced motion support in CSS - Create comprehensive accessibility documentation Closes OlufunbiIK#77
📝 WalkthroughWalkthroughThis PR implements comprehensive WCAG 2.1 AA accessibility features across the TipTune frontend, adding new accessibility components and utilities, enhancing existing components with ARIA attributes and keyboard navigation, integrating the jsx-a11y ESLint plugin, and adding global accessibility CSS and documentation. Changes
Sequence DiagramsequenceDiagram
actor User
participant App
participant MusicPlayer
participant useKeyboardShortcuts
participant useAnnouncer
participant LiveRegionProvider
participant ScreenReader
User->>useKeyboardShortcuts: Press spacebar (play/pause shortcut)
useKeyboardShortcuts->>MusicPlayer: Trigger handlePlayPause action
MusicPlayer->>MusicPlayer: Update playback state
MusicPlayer->>useAnnouncer: Call announcePolite("Now playing...")
useAnnouncer->>useAnnouncer: Schedule announcement with debounce
useAnnouncer->>LiveRegionProvider: Call announce(message, priority)
LiveRegionProvider->>LiveRegionProvider: Update live region textContent with timeout
LiveRegionProvider->>ScreenReader: Polite region updates (aria-live="polite")
ScreenReader->>User: Announces "Now playing Song Title"
User->>MusicPlayer: Click next button
MusicPlayer->>useAnnouncer: Call announcePolite("Playing next track...")
useAnnouncer->>LiveRegionProvider: announce(message)
LiveRegionProvider->>ScreenReader: Polite region updates
ScreenReader->>User: Announces playback change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
frontend/src/components/common/Modal.tsx (2)
73-77:onClickon thearia-hiddenbackdrop is unreachable.This
<div>(the translucent overlay) is covered by thefixed inset-0container on line 79, so itsonClick={handleBackdropClick}will never fire — the click is intercepted by the overlying element. The functional backdrop-click handling is on line 82. Remove the dead handler here to avoid confusion.Proposed fix
<div className="fixed inset-0 bg-black/75 transition-opacity" aria-hidden="true" - onClick={handleBackdropClick} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/common/Modal.tsx` around lines 73 - 77, The translucent overlay DIV with className "fixed inset-0 bg-black/75 transition-opacity" currently has an unreachable onClick handler; remove the dead onClick={handleBackdropClick} from that overlay element in Modal.tsx so only the topmost clickable container (the other fixed inset-0 element that actually receives clicks) uses handleBackdropClick, leaving the overlay purely presentational.
85-92: Consider removingrole="document"from the inner panel.
role="document"inside arole="dialog"witharia-modal="true"is a legacy pattern. Modern WAI-ARIA Authoring Practices no longer recommend it, and it can cause some screen readers to exit the modal's focus context, undermining the focus trap. Removing it lets the dialog semantics handle content navigation.Proposed fix
<div ref={modalRef} className={`relative transform overflow-hidden rounded-lg bg-navy-900 border border-navy-700 text-left shadow-xl sm:my-8 sm:w-full sm:max-w-lg ${ prefersReducedMotion ? '' : 'transition-all' }`} onClick={(e) => e.stopPropagation()} - role="document" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/common/Modal.tsx` around lines 85 - 92, The inner panel div that uses modalRef currently sets role="document", which is a legacy pattern and can break screen-reader focus within the dialog; remove the role="document" attribute from that div (the element with ref={modalRef} in Modal.tsx) so the outer dialog's semantics (role="dialog" with aria-modal="true") govern focus and navigation—simply delete the role prop on the modalRef container and keep the existing focus-trap logic intact.frontend/src/components/a11y/LiveRegion.tsx (1)
3-3: DuplicateAriaLivetype — import fromutils/accessibility.tsinstead.The
AriaLivetype is already exported fromfrontend/src/utils/accessibility.ts. Importing it would eliminate duplication and maintain consistency across the codebase.Proposed fix
-type AriaLive = 'polite' | 'assertive' | 'off'; +import { AriaLive } from '@/utils/accessibility';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/a11y/LiveRegion.tsx` at line 3, Remove the duplicated local AriaLive type declaration in LiveRegion.tsx and instead import the existing AriaLive type from the shared accessibility utility that already exports it; update the top of the file to import AriaLive and delete the local "type AriaLive = 'polite' | 'assertive' | 'off';" declaration so the component uses the canonical AriaLive type (refer to the AriaLive symbol and the LiveRegion component/type usages to locate where to change imports and remove the duplicate).frontend/eslint.config.js (1)
35-55: All jsx-a11y rules are downgraded to'warn'; critical rules should be'error'
jsxA11y.configs.recommended.rules(line 28) sets these rules to'error'by default. Lines 35–55 then override every single one to'warn', meaning a11y violations produce lint warnings but won't fail the build or block CI — directly contradicting the PR's acceptance criterion of zero critical violations.Consider promoting the structural/semantic rules that signal broken a11y at the code level to
'error':♻️ Suggested rule level changes
- 'jsx-a11y/aria-props': 'warn', + 'jsx-a11y/aria-props': 'error', - 'jsx-a11y/aria-proptypes': 'warn', + 'jsx-a11y/aria-proptypes': 'error', - 'jsx-a11y/aria-role': 'warn', + 'jsx-a11y/aria-role': 'error', - 'jsx-a11y/aria-unsupported-elements': 'warn', + 'jsx-a11y/aria-unsupported-elements': 'error', - 'jsx-a11y/interactive-supports-focus': 'warn', + 'jsx-a11y/interactive-supports-focus': 'error', - 'jsx-a11y/click-events-have-key-events': 'warn', + 'jsx-a11y/click-events-have-key-events': 'error', - 'jsx-a11y/role-has-required-aria-props': 'warn', + 'jsx-a11y/role-has-required-aria-props': 'error', - 'jsx-a11y/role-supports-aria-props': 'warn', + 'jsx-a11y/role-supports-aria-props': 'error',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/eslint.config.js` around lines 35 - 55, The eslint config is downgrading all jsx-a11y rules to 'warn', contradicting jsxA11y.configs.recommended.rules and allowing critical accessibility violations to pass CI; update the override block that lists rules like 'jsx-a11y/anchor-is-valid', 'jsx-a11y/label-has-associated-control', 'jsx-a11y/no-autofocus' (and other entries in the same list) to set structural/semantic rules that indicate broken accessibility to 'error' (leave truly noisy rules as 'warn' if needed), so the file's override aligns with jsxA11y.configs.recommended.rules and enforces zero critical violations.frontend/src/hooks/useFocusTrap.ts (1)
16-44:handleKeyDownreimplements the focus-trap logic already intrapFocusThe Tab/Shift+Tab wrapping logic in lines 29–41 is a near-verbatim copy of
trapFocusinfrontend/src/utils/accessibility.ts(lines 60–81). The hook should delegate to that utility to eliminate the duplication.♻️ Proposed refactor
Update the import:
-import { getFocusableElements } from '@/utils/accessibility'; +import { getFocusableElements, trapFocus } from '@/utils/accessibility';Replace the callback body:
const handleKeyDown = useCallback( (event: KeyboardEvent) => { if (!enabled || !containerRef.current) return; - - const container = containerRef.current; - const focusableElements = getFocusableElements(container); - - if (focusableElements.length === 0) return; - - const firstFocusable = focusableElements[0]; - const lastFocusable = focusableElements[focusableElements.length - 1]; - const activeElement = document.activeElement as HTMLElement; - - if (event.key === 'Tab') { - if (event.shiftKey) { - if (activeElement === firstFocusable) { - event.preventDefault(); - lastFocusable.focus(); - } - } else { - if (activeElement === lastFocusable) { - event.preventDefault(); - firstFocusable.focus(); - } - } - } + trapFocus(containerRef.current, event); }, [enabled, containerRef] );
getFocusableElementsis still needed for the initial-focus logic in theuseEffect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useFocusTrap.ts` around lines 16 - 44, handleKeyDown duplicates the Tab/Shift+Tab wrapping logic already implemented in trapFocus; instead of reimplementing it inside the useCallback, import trapFocus from frontend/src/utils/accessibility.ts and call trapFocus(event, containerRef.current) (or the trapFocus signature used) from within handleKeyDown, keeping early guards for enabled and containerRef and retaining getFocusableElements for the initial-focus logic in the existing useEffect; update imports to include trapFocus and remove the duplicated wrap-around code block inside handleKeyDown.frontend/src/components/player/VolumeControl.tsx (1)
66-90: Redundantsr-onlyspan alongsidearia-labelon the mute button.The
aria-labelalready provides the accessible name and takes precedence over child text in the accessibility tree. The<span className="sr-only">with identical text is redundant. Consider removing it to reduce duplication.Suggested cleanup
<button onClick={onToggleMute} aria-label={isMuted || volume === 0 ? "Unmute" : "Mute"} aria-pressed={isMuted} data-testid="mute-button" type="button" className="p-1 rounded-md focus:outline-none focus-visible:ring-2 focus-visible:ring-primary-blue" > - <span className="sr-only"> - {isMuted || volume === 0 ? "Unmute" : "Mute"} - </span> {isMuted || volume === 0 ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/player/VolumeControl.tsx` around lines 66 - 90, Remove the redundant visually-hidden label inside the mute button since the button already has an accessible name via aria-label; locate the button with onToggleMute, aria-label={isMuted || volume === 0 ? "Unmute" : "Mute"}, data-testid="mute-button" and delete the <span className="sr-only">... </span> while keeping the aria-label logic and the icon rendering (VolumeX/Volume2) intact.frontend/src/hooks/useKeyboardShortcuts.ts (1)
14-17:shortcutsinUseKeyboardShortcutsOptionsis confusing since shortcuts are also the first argument.The
shortcutsfield is marked required in the options interface, yet the hook's first positional argument is alsoshortcuts. On line 37,...optionscan silently overwrite the positional argument. Since the options type is only used to extractenabledon line 34, consider removingshortcutsfrom the interface:Suggested simplification
interface UseKeyboardShortcutsOptions { enabled?: boolean; - shortcuts: KeyboardShortcut[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useKeyboardShortcuts.ts` around lines 14 - 17, The options interface currently declares a required shortcuts field that conflicts with the hook's first positional argument and may be silently overwritten when spreading ...options; remove the shortcuts property from UseKeyboardShortcutsOptions so it only contains enabled?: boolean, update the useKeyboardShortcuts signature/type usage to accept (shortcuts: KeyboardShortcut[], options?: UseKeyboardShortcutsOptions) and ensure the implementation (especially where ...options is spread and enabled is read) only relies on options.enabled; search for uses of UseKeyboardShortcutsOptions and adjust any callers/overloads to stop passing shortcuts in the options object.frontend/src/components/a11y/VisuallyHidden.tsx (1)
3-17: Consider accepting and spreading additional HTML attributes for flexibility.Currently, consumers can't pass through
id,role, or other HTML attributes. A small enhancement would make the component more reusable.Suggested enhancement
-interface VisuallyHiddenProps { - children: ReactNode; - as?: keyof JSX.IntrinsicElements; -} +interface VisuallyHiddenProps extends React.HTMLAttributes<HTMLElement> { + children: ReactNode; + as?: keyof JSX.IntrinsicElements; +} export const VisuallyHidden: React.FC<VisuallyHiddenProps> = ({ children, as: Component = 'span', + className, + ...props }) => { return ( - <Component className="sr-only"> + <Component className={`sr-only ${className || ''}`} {...props}> {children} </Component> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/a11y/VisuallyHidden.tsx` around lines 3 - 17, Update VisuallyHidden to accept and forward arbitrary HTML attributes: change VisuallyHiddenProps to extend appropriate HTML attribute types (e.g., React.HTMLAttributes<HTMLElement> or use a generic mapped type for JSX.IntrinsicElements) and add a rest props parameter (e.g., ...props) to the VisuallyHidden component signature; then spread those props into the rendered element (the Component returned from the as: Component = 'span' prop). Keep children and the default as behavior but ensure attributes like id, role, className, etc. are forwarded when rendering in VisuallyHidden.frontend/src/components/player/MusicPlayer.tsx (1)
82-83: Floating-point drift in volume keyboard shortcuts.
volume + 0.1/volume - 0.1accumulates IEEE 754 rounding errors (e.g. after three ArrowUp presses from 0:0.30000000000000004).Math.roundinhandleVolumeChangemasks the announcement, but the stored volume value drifts. A simple fix is to round to one decimal place before clamping.♻️ Proposed fix
- { key: 'ArrowUp', action: () => handleVolumeChange(Math.min(volume + 0.1, 1)), description: 'Increase volume' }, - { key: 'ArrowDown', action: () => handleVolumeChange(Math.max(volume - 0.1, 0)), description: 'Decrease volume' }, + { key: 'ArrowUp', action: () => handleVolumeChange(Math.min(Math.round((volume + 0.1) * 10) / 10, 1)), description: 'Increase volume' }, + { key: 'ArrowDown', action: () => handleVolumeChange(Math.max(Math.round((volume - 0.1) * 10) / 10, 0)), description: 'Decrease volume' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/player/MusicPlayer.tsx` around lines 82 - 83, The ArrowUp/ArrowDown keyboard actions mutate the volume with raw floating math which causes IEEE-754 drift; update the actions in the shortcut entries (the objects with key: 'ArrowUp' and key: 'ArrowDown') to compute the new volume by rounding to one decimal place before clamping and passing to handleVolumeChange (e.g. use Math.round((volume + 0.1) * 10) / 10 and Math.round((volume - 0.1) * 10) / 10 respectively, then clamp with Math.min/Math.max), keeping handleVolumeChange(volume) as the single place that applies the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 22-24: The '?' shortcut never fires because isShortcutMatch
currently requires event.shiftKey to be false when the shortcut omits shiftKey;
update the matcher (isShortcutMatch in useKeyboardShortcuts.ts) to treat
unspecified modifier fields as "don't care" (i.e., only enforce modifier match
when the shortcut explicitly sets shiftKey/ctrlKey/altKey/metaKey), or as a
minimal immediate fix add shiftKey: true to the shortcut definition passed to
useKeyboardShortcuts in App.tsx (the entry { key: "?", action: openShortcuts,
description: "Open keyboard shortcuts" }). Ensure isShortcutMatch logic checks
for explicit equality only when the shortcut property is defined.
In `@frontend/src/components/a11y/LiveRegion.tsx`:
- Around line 28-35: The announce function currently treats any non-'assertive'
priority (including AriaLive 'off') as polite; update announce to explicitly
handle 'off' by returning early (no-op) when priority === 'off', leaving
setAssertiveMessage and setPoliteMessage unchanged for 'assertive' and 'polite'
respectively; reference the announce function and the state setters
setAssertiveMessage and setPoliteMessage so the logic short-circuits for 'off'
and only triggers the existing timed updates for 'assertive' and 'polite'.
In `@frontend/src/components/a11y/SkipLink.tsx`:
- Around line 12-19: The click handler handleClick currently always calls
target.scrollIntoView({ behavior: 'smooth' }) which can ignore users'
prefers-reduced-motion setting; import and use the existing useReducedMotion
hook inside the SkipLink component, read its boolean (e.g., reducedMotion), and
when focusing/scrolling the target call scrollIntoView with behavior:
reducedMotion ? 'auto' : 'smooth' (or omit the options when reducedMotion is
true) so reduced-motion users get instant scrolling; update any TypeScript types
if needed and ensure useReducedMotion is referenced near handleClick.
In `@frontend/src/components/common/Button.tsx`:
- Around line 46-48: The component sets aria-busy={loading} but then spreads
{...props} after it, allowing callers to override the component-controlled
loading state; fix by ensuring the component-controlled aria-busy wins — either
destructure aria-busy from the incoming props (like disabled and aria-label are)
or move aria-busy after the {...props} spread in the Button component so
aria-busy={loading} always takes precedence (refer to the aria-busy prop, the
loading prop, and the {...props} spread in Button.tsx).
In `@frontend/src/components/common/Modal.tsx`:
- Around line 66-72: The Modal component currently uses hardcoded
id="modal-title" and id="modal-description" which can collide for multiple
instances; replace those static ids by generating unique ids via React's useId()
inside the Modal function (e.g., const id = useId(); const titleId =
`${id}-title`; const descId = `${id}-description`), then apply titleId to the
title element's id and descId to the description element's id and update the
container's aria-labelledby={titleId} and aria-describedby={description ? descId
: undefined}; update the other occurrences that set
id="modal-title"/"modal-description" and aria attributes (the title/description
elements and the dialog wrapper referenced in the diff) to use these generated
ids.
- Around line 33-37: The focus restoration currently only runs in the useEffect
that watches isOpen, so if the Modal unmounts while open the
previousActiveElement.focus() is never called; move or duplicate the focus
restore logic into the cleanup function of the body-scroll effect (the effect
that toggles document.body.style.overflow and stores previousActiveElement) so
that the cleanup restores focus when the component unmounts as well as when
isOpen becomes false—specifically update the effect that sets
document.body.style.overflow and previousActiveElement to call
previousActiveElement.current?.focus() in its return cleanup, and remove or keep
the existing isOpen-only restore to avoid double-focus but ensure only one code
path is responsible for restoration.
In `@frontend/src/components/player/MusicPlayer.tsx`:
- Around line 95-99: Remove the redundant explicit aria-live="polite" from the
status container in the MusicPlayer component: the <div> that sets role="status"
(the status/notification wrapper in MusicPlayer.tsx) already implies
aria-live="polite" and aria-atomic="true", so delete the aria-live attribute and
keep role="status" and other attributes unchanged.
- Around line 53-65: handleNext and handlePrevious currently announce the
pre-navigation currentTrack (stale) — change them to announce only the
navigation direction (e.g., "Next track" / "Previous track") and rely on the
existing useEffect in useAnnouncer to announce the authoritative "Now playing:
…" once currentTrack.id updates; update the callbacks (handleNext,
handlePrevious) to call announcePolite("Next track") / announcePolite("Previous
track") and remove currentTrack from their dependency arrays so they don't
capture stale track data, keeping next, previous and announcePolite as
dependencies.
In `@frontend/src/components/player/PlayButton.tsx`:
- Around line 17-18: The aria usage in PlayButton.tsx is duplicating state for
screen readers (aria-label changes and aria-pressed both communicate state);
pick one approach and update the button accordingly: either (preferred) keep
aria-pressed={isPlaying} and change aria-label to a static string like
"Play/Pause" (adjust any visible tooltip or title if present), or remove
aria-pressed and keep the dynamic aria-label={isPlaying ? "Pause" : "Play"} so
only the label communicates state; update the button element in the PlayButton
component (where aria-label and aria-pressed are set) to implement the chosen
approach consistently.
In `@frontend/src/hooks/useAnnouncer.ts`:
- Around line 9-41: The debounce timeout created in useAnnouncer's announce
remains active after unmount, causing stale announcements; add a useEffect
cleanup that clears timeoutRef.current (using clearTimeout) when the component
unmounts to cancel any pending setTimeouts so announceToScreenReader is not
called after unmount; update the hook (useAnnouncer) to reference timeoutRef and
perform clearTimeout(timeoutRef.current) and set timeoutRef.current = null in
the cleanup effect.
In `@frontend/src/hooks/useFocusTrap.ts`:
- Around line 62-67: The cleanup in useFocusTrap currently calls
previousActiveElement.focus() only if typeof previousActiveElement.focus ===
'function' — change this to additionally verify the element is still in the
document (e.g., using document.contains(previousActiveElement) or
previousActiveElement.isConnected) before calling focus; update the return
cleanup (which removes handleKeyDown) to check restoreFocus &&
previousActiveElement && element-is-attached && typeof
previousActiveElement.focus === 'function' and only then call
previousActiveElement.focus().
In `@frontend/src/hooks/useKeyboardShortcuts.ts`:
- Around line 46-56: The input-field guard in useKeyboardShortcuts (the
isInputField check used before calling isShortcutMatch on shortcutsRef.current)
misses <select> elements; update the guard to treat SELECT elements as input
fields as well (e.g., include target.tagName === 'SELECT' alongside 'INPUT' and
'TEXTAREA') so keyboard shortcuts are suppressed when a <select> is focused
while preserving existing contentEditable handling and modifier-key exceptions.
- Around line 19-30: isShortcutMatch currently treats an undefined modifier in
KeyboardShortcut as "must not be pressed", breaking shifted printable
characters; update isShortcutMatch so modifier checks treat undefined as "don't
care" (i.e., pass if shortcut.shiftKey is undefined), and for shift in
particular make it lenient by comparing: if shortcut.shiftKey is undefined
return true else require event.shiftKey === shortcut.shiftKey; keep
ctrl/alt/meta either strict (require equality when defined) or follow the same
"undefined = don't care" pattern depending on desired UX, but ensure comparisons
reference event.key and shortcut.key case-insensitively as before and use the
function name isShortcutMatch and type KeyboardShortcut to locate the change.
In `@frontend/src/styles/index.css`:
- Around line 42-64: Replace the deprecated clip declarations in the .sr-only
rule and the .sr-only:focus,.sr-only:active rule with modern clip-path
equivalents: in the .sr-only selector swap clip: rect(0, 0, 0, 0); for
clip-path: inset(0 0 0 0); and in the :focus/:active selector change clip: auto;
to clip-path: none; so the visually-hidden pattern uses clip-path (maintain
existing position/overflow behavior and keep other properties unchanged).
In `@frontend/src/utils/accessibility.ts`:
- Around line 10-24: announceToScreenReader currently treats any AriaLive value
other than 'assertive' as polite, causing 'off' to still announce; update
announceToScreenReader to early-return when priority === 'off' so it performs no
DOM updates, otherwise select the live region by using priority === 'assertive'
? 'a11y-assertive-region' : 'a11y-polite-region' and keep the existing
clearing/setTimeout behavior; reference the announceToScreenReader function and
the live region element ids ('a11y-assertive-region' and 'a11y-polite-region')
when making the change.
- Around line 91-104: formatTimeForScreenReader currently propagates NaN and
returns "NaN seconds"; add an initial guard in formatTimeForScreenReader to
handle non-finite inputs (use Number.isFinite(seconds) or isNaN check) and
return a sensible default like "0 seconds" (also clamp negative values to 0 if
desired) before computing minutes and remainingSeconds so the following logic
using minutes and remainingSeconds never sees NaN.
- Around line 26-50: focusFirstFocusable and focusLastFocusable currently use a
selector that can match disabled controls and thus may focus disabled elements;
update these functions (focusFirstFocusable, focusLastFocusable) to use the
shared getFocusableElements utility (the function defined later in this file) to
obtain the filtered list of elements (or alternatively add :not([disabled]) to
the selector) and then focus the first/last element from that returned list,
ensuring consistent behavior with getFocusableElements and avoiding focusing
disabled controls.
---
Nitpick comments:
In `@frontend/eslint.config.js`:
- Around line 35-55: The eslint config is downgrading all jsx-a11y rules to
'warn', contradicting jsxA11y.configs.recommended.rules and allowing critical
accessibility violations to pass CI; update the override block that lists rules
like 'jsx-a11y/anchor-is-valid', 'jsx-a11y/label-has-associated-control',
'jsx-a11y/no-autofocus' (and other entries in the same list) to set
structural/semantic rules that indicate broken accessibility to 'error' (leave
truly noisy rules as 'warn' if needed), so the file's override aligns with
jsxA11y.configs.recommended.rules and enforces zero critical violations.
In `@frontend/src/components/a11y/LiveRegion.tsx`:
- Line 3: Remove the duplicated local AriaLive type declaration in
LiveRegion.tsx and instead import the existing AriaLive type from the shared
accessibility utility that already exports it; update the top of the file to
import AriaLive and delete the local "type AriaLive = 'polite' | 'assertive' |
'off';" declaration so the component uses the canonical AriaLive type (refer to
the AriaLive symbol and the LiveRegion component/type usages to locate where to
change imports and remove the duplicate).
In `@frontend/src/components/a11y/VisuallyHidden.tsx`:
- Around line 3-17: Update VisuallyHidden to accept and forward arbitrary HTML
attributes: change VisuallyHiddenProps to extend appropriate HTML attribute
types (e.g., React.HTMLAttributes<HTMLElement> or use a generic mapped type for
JSX.IntrinsicElements) and add a rest props parameter (e.g., ...props) to the
VisuallyHidden component signature; then spread those props into the rendered
element (the Component returned from the as: Component = 'span' prop). Keep
children and the default as behavior but ensure attributes like id, role,
className, etc. are forwarded when rendering in VisuallyHidden.
In `@frontend/src/components/common/Modal.tsx`:
- Around line 73-77: The translucent overlay DIV with className "fixed inset-0
bg-black/75 transition-opacity" currently has an unreachable onClick handler;
remove the dead onClick={handleBackdropClick} from that overlay element in
Modal.tsx so only the topmost clickable container (the other fixed inset-0
element that actually receives clicks) uses handleBackdropClick, leaving the
overlay purely presentational.
- Around line 85-92: The inner panel div that uses modalRef currently sets
role="document", which is a legacy pattern and can break screen-reader focus
within the dialog; remove the role="document" attribute from that div (the
element with ref={modalRef} in Modal.tsx) so the outer dialog's semantics
(role="dialog" with aria-modal="true") govern focus and navigation—simply delete
the role prop on the modalRef container and keep the existing focus-trap logic
intact.
In `@frontend/src/components/player/MusicPlayer.tsx`:
- Around line 82-83: The ArrowUp/ArrowDown keyboard actions mutate the volume
with raw floating math which causes IEEE-754 drift; update the actions in the
shortcut entries (the objects with key: 'ArrowUp' and key: 'ArrowDown') to
compute the new volume by rounding to one decimal place before clamping and
passing to handleVolumeChange (e.g. use Math.round((volume + 0.1) * 10) / 10 and
Math.round((volume - 0.1) * 10) / 10 respectively, then clamp with
Math.min/Math.max), keeping handleVolumeChange(volume) as the single place that
applies the value.
In `@frontend/src/components/player/VolumeControl.tsx`:
- Around line 66-90: Remove the redundant visually-hidden label inside the mute
button since the button already has an accessible name via aria-label; locate
the button with onToggleMute, aria-label={isMuted || volume === 0 ? "Unmute" :
"Mute"}, data-testid="mute-button" and delete the <span className="sr-only">...
</span> while keeping the aria-label logic and the icon rendering
(VolumeX/Volume2) intact.
In `@frontend/src/hooks/useFocusTrap.ts`:
- Around line 16-44: handleKeyDown duplicates the Tab/Shift+Tab wrapping logic
already implemented in trapFocus; instead of reimplementing it inside the
useCallback, import trapFocus from frontend/src/utils/accessibility.ts and call
trapFocus(event, containerRef.current) (or the trapFocus signature used) from
within handleKeyDown, keeping early guards for enabled and containerRef and
retaining getFocusableElements for the initial-focus logic in the existing
useEffect; update imports to include trapFocus and remove the duplicated
wrap-around code block inside handleKeyDown.
In `@frontend/src/hooks/useKeyboardShortcuts.ts`:
- Around line 14-17: The options interface currently declares a required
shortcuts field that conflicts with the hook's first positional argument and may
be silently overwritten when spreading ...options; remove the shortcuts property
from UseKeyboardShortcutsOptions so it only contains enabled?: boolean, update
the useKeyboardShortcuts signature/type usage to accept (shortcuts:
KeyboardShortcut[], options?: UseKeyboardShortcutsOptions) and ensure the
implementation (especially where ...options is spread and enabled is read) only
relies on options.enabled; search for uses of UseKeyboardShortcutsOptions and
adjust any callers/overloads to stop passing shortcuts in the options object.
Comment on lines
+22
to
+24
| useKeyboardShortcuts([ | ||
| { key: "?", action: openShortcuts, description: "Open keyboard shortcuts" }, | ||
| ]); |
There was a problem hiding this comment.
? shortcut will never fire due to shift-key matching in isShortcutMatch.
Typing ? on most keyboard layouts requires holding Shift, so event.shiftKey will be true. However, since this shortcut definition omits shiftKey, the matcher in useKeyboardShortcuts.ts enforces !event.shiftKey, which evaluates to false — the shortcut silently never triggers.
Either add shiftKey: true here, or (preferably) fix the matcher to be lenient about modifier keys that aren't explicitly specified. See the related comment on useKeyboardShortcuts.ts for the root-cause fix.
Quick fix at the call site
useKeyboardShortcuts([
- { key: "?", action: openShortcuts, description: "Open keyboard shortcuts" },
+ { key: "?", shiftKey: true, action: openShortcuts, description: "Open keyboard shortcuts" },
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useKeyboardShortcuts([ | |
| { key: "?", action: openShortcuts, description: "Open keyboard shortcuts" }, | |
| ]); | |
| useKeyboardShortcuts([ | |
| { key: "?", shiftKey: true, action: openShortcuts, description: "Open keyboard shortcuts" }, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/App.tsx` around lines 22 - 24, The '?' shortcut never fires
because isShortcutMatch currently requires event.shiftKey to be false when the
shortcut omits shiftKey; update the matcher (isShortcutMatch in
useKeyboardShortcuts.ts) to treat unspecified modifier fields as "don't care"
(i.e., only enforce modifier match when the shortcut explicitly sets
shiftKey/ctrlKey/altKey/metaKey), or as a minimal immediate fix add shiftKey:
true to the shortcut definition passed to useKeyboardShortcuts in App.tsx (the
entry { key: "?", action: openShortcuts, description: "Open keyboard shortcuts"
}). Ensure isShortcutMatch logic checks for explicit equality only when the
shortcut property is defined.
Comment on lines
+28
to
+35
| const announce = useCallback((message: string, priority: AriaLive = 'polite') => { | ||
| if (priority === 'assertive') { | ||
| setAssertiveMessage(''); | ||
| setTimeout(() => setAssertiveMessage(message), 50); | ||
| } else { | ||
| setPoliteMessage(''); | ||
| setTimeout(() => setPoliteMessage(message), 50); | ||
| } |
There was a problem hiding this comment.
Passing priority='off' silently announces as polite instead of being a no-op.
The AriaLive type includes 'off', but the announce function only branches on 'assertive' — everything else (including 'off') falls through to the polite path. If 'off' means "don't announce," it should be handled explicitly.
Proposed fix
const announce = useCallback((message: string, priority: AriaLive = 'polite') => {
+ if (priority === 'off') return;
if (priority === 'assertive') {
setAssertiveMessage('');
setTimeout(() => setAssertiveMessage(message), 50);
} else {
setPoliteMessage('');
setTimeout(() => setPoliteMessage(message), 50);
}
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const announce = useCallback((message: string, priority: AriaLive = 'polite') => { | |
| if (priority === 'assertive') { | |
| setAssertiveMessage(''); | |
| setTimeout(() => setAssertiveMessage(message), 50); | |
| } else { | |
| setPoliteMessage(''); | |
| setTimeout(() => setPoliteMessage(message), 50); | |
| } | |
| const announce = useCallback((message: string, priority: AriaLive = 'polite') => { | |
| if (priority === 'off') return; | |
| if (priority === 'assertive') { | |
| setAssertiveMessage(''); | |
| setTimeout(() => setAssertiveMessage(message), 50); | |
| } else { | |
| setPoliteMessage(''); | |
| setTimeout(() => setPoliteMessage(message), 50); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/a11y/LiveRegion.tsx` around lines 28 - 35, The
announce function currently treats any non-'assertive' priority (including
AriaLive 'off') as polite; update announce to explicitly handle 'off' by
returning early (no-op) when priority === 'off', leaving setAssertiveMessage and
setPoliteMessage unchanged for 'assertive' and 'polite' respectively; reference
the announce function and the state setters setAssertiveMessage and
setPoliteMessage so the logic short-circuits for 'off' and only triggers the
existing timed updates for 'assertive' and 'polite'.
Comment on lines
+12
to
+19
| const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => { | ||
| e.preventDefault(); | ||
| const target = document.getElementById(targetId); | ||
| if (target) { | ||
| target.tabIndex = -1; | ||
| target.focus(); | ||
| target.scrollIntoView({ behavior: 'smooth' }); | ||
| } |
There was a problem hiding this comment.
scrollIntoView({ behavior: 'smooth' }) doesn't consistently respect prefers-reduced-motion.
The JS API scrollIntoView with behavior: 'smooth' does not reliably honor the user's reduced-motion preference across browsers, unlike CSS scroll-behavior. Since this PR is specifically targeting WCAG 2.1 AA and already has a useReducedMotion hook available, consider using it here:
Suggested fix
-import React from 'react';
+import React from 'react';
+import { useReducedMotion } from '@/hooks/useReducedMotion';
export const SkipLink: React.FC<SkipLinkProps> = ({
targetId,
label = 'Skip to main content',
}) => {
+ const prefersReducedMotion = useReducedMotion();
+
const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => {
e.preventDefault();
const target = document.getElementById(targetId);
if (target) {
target.tabIndex = -1;
target.focus();
- target.scrollIntoView({ behavior: 'smooth' });
+ target.scrollIntoView({ behavior: prefersReducedMotion ? 'auto' : 'smooth' });
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/a11y/SkipLink.tsx` around lines 12 - 19, The click
handler handleClick currently always calls target.scrollIntoView({ behavior:
'smooth' }) which can ignore users' prefers-reduced-motion setting; import and
use the existing useReducedMotion hook inside the SkipLink component, read its
boolean (e.g., reducedMotion), and when focusing/scrolling the target call
scrollIntoView with behavior: reducedMotion ? 'auto' : 'smooth' (or omit the
options when reducedMotion is true) so reduced-motion users get instant
scrolling; update any TypeScript types if needed and ensure useReducedMotion is
referenced near handleClick.
Comment on lines
+46
to
+48
| aria-label={ariaLabel} | ||
| aria-busy={loading} | ||
| {...props} |
There was a problem hiding this comment.
aria-busy can be overridden by {...props} spread
aria-busy={loading} is set on line 47, but {...props} on line 48 comes after it. Because aria-busy is not destructured out of props (unlike disabled and aria-label on lines 20–21), any caller that passes aria-busy as a prop will silently override the component-managed loading state, potentially telling screen readers the button is not busy while loading is true.
Move aria-busy after the spread so it always wins:
🔒 Proposed fix
+ {...props}
aria-label={ariaLabel}
- aria-busy={loading}
- {...props}
+ aria-busy={loading}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| aria-label={ariaLabel} | |
| aria-busy={loading} | |
| {...props} | |
| {...props} | |
| aria-label={ariaLabel} | |
| aria-busy={loading} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/Button.tsx` around lines 46 - 48, The
component sets aria-busy={loading} but then spreads {...props} after it,
allowing callers to override the component-controlled loading state; fix by
ensuring the component-controlled aria-busy wins — either destructure aria-busy
from the incoming props (like disabled and aria-label are) or move aria-busy
after the {...props} spread in the Button component so aria-busy={loading}
always takes precedence (refer to the aria-busy prop, the loading prop, and the
{...props} spread in Button.tsx).
Comment on lines
+33
to
+37
| useEffect(() => { | ||
| if (!isOpen && previousActiveElement.current) { | ||
| previousActiveElement.current.focus(); | ||
| } | ||
| }, [isOpen]); |
There was a problem hiding this comment.
Focus restoration won't fire if the modal unmounts while open.
If a parent conditionally removes the Modal component while isOpen is still true, the !isOpen check here never triggers, so focus is never returned to previousActiveElement. Consider consolidating focus restoration into the cleanup of the body-scroll effect (lines 22–31) so it runs on unmount as well.
Proposed fix — restore focus on unmount
useEffect(() => {
+ const prevElement = previousActiveElement.current;
if (isOpen) {
previousActiveElement.current = document.activeElement as HTMLElement;
document.body.style.overflow = 'hidden';
}
return () => {
document.body.style.overflow = 'unset';
+ if (prevElement && prevElement !== document.activeElement) {
+ prevElement.focus();
+ }
};
}, [isOpen]);
-
- useEffect(() => {
- if (!isOpen && previousActiveElement.current) {
- previousActiveElement.current.focus();
- }
- }, [isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/Modal.tsx` around lines 33 - 37, The focus
restoration currently only runs in the useEffect that watches isOpen, so if the
Modal unmounts while open the previousActiveElement.focus() is never called;
move or duplicate the focus restore logic into the cleanup function of the
body-scroll effect (the effect that toggles document.body.style.overflow and
stores previousActiveElement) so that the cleanup restores focus when the
component unmounts as well as when isOpen becomes false—specifically update the
effect that sets document.body.style.overflow and previousActiveElement to call
previousActiveElement.current?.focus() in its return cleanup, and remove or keep
the existing isOpen-only restore to avoid double-focus but ensure only one code
path is responsible for restoration.
Comment on lines
+46
to
+56
| const target = event.target as HTMLElement; | ||
| const isInputField = | ||
| target.tagName === 'INPUT' || | ||
| target.tagName === 'TEXTAREA' || | ||
| target.isContentEditable; | ||
|
|
||
| for (const shortcut of shortcutsRef.current) { | ||
| if (isShortcutMatch(event, shortcut)) { | ||
| if (isInputField && !shortcut.ctrlKey && !shortcut.metaKey) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Input-field guard doesn't cover <select> elements.
<select> elements also use keyboard input (letter keys for option jumping, arrow keys). Firing shortcuts while a <select> is focused could interfere with native behavior.
Suggested fix
const isInputField =
target.tagName === 'INPUT' ||
target.tagName === 'TEXTAREA' ||
+ target.tagName === 'SELECT' ||
target.isContentEditable;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const target = event.target as HTMLElement; | |
| const isInputField = | |
| target.tagName === 'INPUT' || | |
| target.tagName === 'TEXTAREA' || | |
| target.isContentEditable; | |
| for (const shortcut of shortcutsRef.current) { | |
| if (isShortcutMatch(event, shortcut)) { | |
| if (isInputField && !shortcut.ctrlKey && !shortcut.metaKey) { | |
| continue; | |
| } | |
| const target = event.target as HTMLElement; | |
| const isInputField = | |
| target.tagName === 'INPUT' || | |
| target.tagName === 'TEXTAREA' || | |
| target.tagName === 'SELECT' || | |
| target.isContentEditable; | |
| for (const shortcut of shortcutsRef.current) { | |
| if (isShortcutMatch(event, shortcut)) { | |
| if (isInputField && !shortcut.ctrlKey && !shortcut.metaKey) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useKeyboardShortcuts.ts` around lines 46 - 56, The
input-field guard in useKeyboardShortcuts (the isInputField check used before
calling isShortcutMatch on shortcutsRef.current) misses <select> elements;
update the guard to treat SELECT elements as input fields as well (e.g., include
target.tagName === 'SELECT' alongside 'INPUT' and 'TEXTAREA') so keyboard
shortcuts are suppressed when a <select> is focused while preserving existing
contentEditable handling and modifier-key exceptions.
Comment on lines
+42
to
+64
| .sr-only { | ||
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); | ||
| white-space: nowrap; | ||
| border-width: 0; | ||
| } | ||
|
|
||
| .sr-only:focus, | ||
| .sr-only:active { | ||
| position: static; | ||
| width: auto; | ||
| height: auto; | ||
| padding: inherit; | ||
| margin: inherit; | ||
| overflow: visible; | ||
| clip: auto; | ||
| white-space: normal; | ||
| } |
There was a problem hiding this comment.
Deprecated clip property causes Stylelint build errors on lines 49 and 62
Stylelint reports clip as property-no-deprecated (error level) at lines 49 and 62. Replace with the modern clip-path equivalent:
🐛 Proposed fix
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
- clip: rect(0, 0, 0, 0);
+ clip-path: inset(50%);
white-space: nowrap;
border-width: 0;
}
.sr-only:focus,
.sr-only:active {
position: static;
width: auto;
height: auto;
padding: inherit;
margin: inherit;
overflow: visible;
- clip: auto;
+ clip-path: none;
white-space: normal;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .sr-only { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip: rect(0, 0, 0, 0); | |
| white-space: nowrap; | |
| border-width: 0; | |
| } | |
| .sr-only:focus, | |
| .sr-only:active { | |
| position: static; | |
| width: auto; | |
| height: auto; | |
| padding: inherit; | |
| margin: inherit; | |
| overflow: visible; | |
| clip: auto; | |
| white-space: normal; | |
| } | |
| .sr-only { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip-path: inset(50%); | |
| white-space: nowrap; | |
| border-width: 0; | |
| } | |
| .sr-only:focus, | |
| .sr-only:active { | |
| position: static; | |
| width: auto; | |
| height: auto; | |
| padding: inherit; | |
| margin: inherit; | |
| overflow: visible; | |
| clip-path: none; | |
| white-space: normal; | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 49-49: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
[error] 62-62: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/styles/index.css` around lines 42 - 64, Replace the deprecated
clip declarations in the .sr-only rule and the .sr-only:focus,.sr-only:active
rule with modern clip-path equivalents: in the .sr-only selector swap clip:
rect(0, 0, 0, 0); for clip-path: inset(0 0 0 0); and in the :focus/:active
selector change clip: auto; to clip-path: none; so the visually-hidden pattern
uses clip-path (maintain existing position/overflow behavior and keep other
properties unchanged).
Comment on lines
+10
to
+24
| export const announceToScreenReader = ( | ||
| message: string, | ||
| priority: AriaLive = 'polite' | ||
| ): void => { | ||
| const liveRegion = document.getElementById( | ||
| priority === 'assertive' ? 'a11y-assertive-region' : 'a11y-polite-region' | ||
| ); | ||
|
|
||
| if (liveRegion) { | ||
| liveRegion.textContent = ''; | ||
| setTimeout(() => { | ||
| liveRegion.textContent = message; | ||
| }, 50); | ||
| } | ||
| }; |
There was a problem hiding this comment.
announceToScreenReader announces to the polite region when priority === 'off'
AriaLive includes 'off', but the ternary on line 15 only branches on 'assertive'; every other value — including 'off' — resolves to 'a11y-polite-region' and triggers an announcement. A caller passing 'off' expects silence.
🐛 Proposed fix
export const announceToScreenReader = (
message: string,
priority: AriaLive = 'polite'
): void => {
+ if (priority === 'off') return;
const liveRegion = document.getElementById(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/accessibility.ts` around lines 10 - 24,
announceToScreenReader currently treats any AriaLive value other than
'assertive' as polite, causing 'off' to still announce; update
announceToScreenReader to early-return when priority === 'off' so it performs no
DOM updates, otherwise select the live region by using priority === 'assertive'
? 'a11y-assertive-region' : 'a11y-polite-region' and keep the existing
clearing/setTimeout behavior; reference the announceToScreenReader function and
the live region element ids ('a11y-assertive-region' and 'a11y-polite-region')
when making the change.
Comment on lines
+26
to
+50
| export const focusFirstFocusable = (container: HTMLElement): boolean => { | ||
| const focusableElements = container.querySelectorAll<HTMLElement>( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); | ||
|
|
||
| const firstFocusable = focusableElements[0]; | ||
| if (firstFocusable) { | ||
| firstFocusable.focus(); | ||
| return true; | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| export const focusLastFocusable = (container: HTMLElement): boolean => { | ||
| const focusableElements = container.querySelectorAll<HTMLElement>( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); | ||
|
|
||
| const lastFocusable = focusableElements[focusableElements.length - 1]; | ||
| if (lastFocusable) { | ||
| lastFocusable.focus(); | ||
| return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
focusFirstFocusable and focusLastFocusable can programmatically focus disabled elements
The query selector on lines 27–28 and 40–41 uses 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' — without :not([disabled]) guards. This differs from getFocusableElements (line 55) which correctly filters disabled controls. As a result, these two helpers can focus disabled <button>, <input>, <select>, and <textarea> elements, producing inconsistent cross-browser behavior and violating WCAG SC 1.3.1.
The simplest fix is to delegate to getFocusableElements:
🐛 Proposed fix
export const focusFirstFocusable = (container: HTMLElement): boolean => {
- const focusableElements = container.querySelectorAll<HTMLElement>(
- 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'
- );
- const firstFocusable = focusableElements[0];
+ const [firstFocusable] = getFocusableElements(container);
if (firstFocusable) {
firstFocusable.focus();
return true;
}
return false;
};
export const focusLastFocusable = (container: HTMLElement): boolean => {
- const focusableElements = container.querySelectorAll<HTMLElement>(
- 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'
- );
- const lastFocusable = focusableElements[focusableElements.length - 1];
+ const elements = getFocusableElements(container);
+ const lastFocusable = elements[elements.length - 1];
if (lastFocusable) {
lastFocusable.focus();
return true;
}
return false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const focusFirstFocusable = (container: HTMLElement): boolean => { | |
| const focusableElements = container.querySelectorAll<HTMLElement>( | |
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | |
| ); | |
| const firstFocusable = focusableElements[0]; | |
| if (firstFocusable) { | |
| firstFocusable.focus(); | |
| return true; | |
| } | |
| return false; | |
| }; | |
| export const focusLastFocusable = (container: HTMLElement): boolean => { | |
| const focusableElements = container.querySelectorAll<HTMLElement>( | |
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | |
| ); | |
| const lastFocusable = focusableElements[focusableElements.length - 1]; | |
| if (lastFocusable) { | |
| lastFocusable.focus(); | |
| return true; | |
| } | |
| return false; | |
| }; | |
| export const focusFirstFocusable = (container: HTMLElement): boolean => { | |
| const [firstFocusable] = getFocusableElements(container); | |
| if (firstFocusable) { | |
| firstFocusable.focus(); | |
| return true; | |
| } | |
| return false; | |
| }; | |
| export const focusLastFocusable = (container: HTMLElement): boolean => { | |
| const elements = getFocusableElements(container); | |
| const lastFocusable = elements[elements.length - 1]; | |
| if (lastFocusable) { | |
| lastFocusable.focus(); | |
| return true; | |
| } | |
| return false; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/accessibility.ts` around lines 26 - 50,
focusFirstFocusable and focusLastFocusable currently use a selector that can
match disabled controls and thus may focus disabled elements; update these
functions (focusFirstFocusable, focusLastFocusable) to use the shared
getFocusableElements utility (the function defined later in this file) to obtain
the filtered list of elements (or alternatively add :not([disabled]) to the
selector) and then focus the first/last element from that returned list,
ensuring consistent behavior with getFocusableElements and avoiding focusing
disabled controls.
Comment on lines
+91
to
+104
| export const formatTimeForScreenReader = (seconds: number): string => { | ||
| const minutes = Math.floor(seconds / 60); | ||
| const remainingSeconds = Math.floor(seconds % 60); | ||
|
|
||
| if (minutes === 0) { | ||
| return `${remainingSeconds} seconds`; | ||
| } | ||
|
|
||
| if (remainingSeconds === 0) { | ||
| return `${minutes} minute${minutes > 1 ? 's' : ''}`; | ||
| } | ||
|
|
||
| return `${minutes} minute${minutes > 1 ? 's' : ''} and ${remainingSeconds} seconds`; | ||
| }; |
There was a problem hiding this comment.
formatTimeForScreenReader returns "NaN seconds" for non-finite input
When seconds is NaN (typical before audio metadata loads), Math.floor(NaN) === NaN propagates through every branch. The sibling formatTime in ProgressBar.tsx already guards with if (isNaN(seconds)) return "0:00", but this utility does not.
🐛 Proposed fix
export const formatTimeForScreenReader = (seconds: number): string => {
+ if (!isFinite(seconds) || seconds < 0) return '0 seconds';
const minutes = Math.floor(seconds / 60);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const formatTimeForScreenReader = (seconds: number): string => { | |
| const minutes = Math.floor(seconds / 60); | |
| const remainingSeconds = Math.floor(seconds % 60); | |
| if (minutes === 0) { | |
| return `${remainingSeconds} seconds`; | |
| } | |
| if (remainingSeconds === 0) { | |
| return `${minutes} minute${minutes > 1 ? 's' : ''}`; | |
| } | |
| return `${minutes} minute${minutes > 1 ? 's' : ''} and ${remainingSeconds} seconds`; | |
| }; | |
| export const formatTimeForScreenReader = (seconds: number): string => { | |
| if (!isFinite(seconds) || seconds < 0) return '0 seconds'; | |
| const minutes = Math.floor(seconds / 60); | |
| const remainingSeconds = Math.floor(seconds % 60); | |
| if (minutes === 0) { | |
| return `${remainingSeconds} seconds`; | |
| } | |
| if (remainingSeconds === 0) { | |
| return `${minutes} minute${minutes > 1 ? 's' : ''}`; | |
| } | |
| return `${minutes} minute${minutes > 1 ? 's' : ''} and ${remainingSeconds} seconds`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/accessibility.ts` around lines 91 - 104,
formatTimeForScreenReader currently propagates NaN and returns "NaN seconds";
add an initial guard in formatTimeForScreenReader to handle non-finite inputs
(use Number.isFinite(seconds) or isNaN check) and return a sensible default like
"0 seconds" (also clamp negative values to 0 if desired) before computing
minutes and remainingSeconds so the following logic using minutes and
remainingSeconds never sees NaN.
Owner
|
Hi, Please resolve conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements comprehensive accessibility features to ensure TipTune meets WCAG 2.1 Level AA standards, addressing issue #77.
Changes Made
Automated Testing Tools
eslint-plugin-jsx-a11yfor static accessibility linting@axe-core/reactfor runtime accessibility testingAccessibility Hooks
useReducedMotion- Detects user's reduced motion preferenceuseFocusTrap- Traps focus within modals and dialogsuseKeyboardShortcuts- Manages global keyboard shortcutsuseAnnouncer- Announces content to screen readersAccessibility Utilities (
utils/accessibility.ts)A11y Components
SkipLink- Skip to main content linkLiveRegion- ARIA live regions for announcementsVisuallyHidden- Screen reader only contentKeyboardShortcutHelp- Keyboard shortcuts help modalComponent Updates
Button- Added loading states, proper ARIA attributes, forwardRefModal- Added focus trap, keyboard navigation, proper ARIAMusicPlayer- Full keyboard navigation, screen reader announcementsPlayButton- Added aria-pressed, aria-labelProgressBar- Keyboard interaction, proper slider ARIAVolumeControl- Keyboard interaction, proper slider ARIATrackInfo- Semantic HTML, proper alt textAppHeader- ARIA landmarks, current page indicationCSS Improvements
prefers-reduced-motionsupportprefers-contrastsupportDocumentation
frontend/docs/ACCESSIBILITY.mdKeyboard Shortcuts
?Space←/→↑/↓MTesting
Closes #77
Summary by CodeRabbit
Release Notes
New Features
?)Documentation