Skip to content

Night Shift: fix a11y Space key video toggle #19

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.

Sign up for GitHub

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

Open
EtanHey wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Night Shift: fix a11y Space key video toggle #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/routes/song.$songId.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,12 @@ function SongPageContent({ songId }: SongPageContentProps) {
role="button"
tabIndex={0}
onClick={handleVideoCollapsedToggle}
onKeyDown={(e) => e.key === 'Enter' && handleVideoCollapsedToggle()}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
handleVideoCollapsedToggle();
}
}}
Comment on lines +705 to +710
Copy link

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and get its size
wc -l src/routes/song.$songId.tsx

Repository: EtanHey/songscript

Length of output: 113

🏁 Script executed:

# Read the onKeyDown handler and surrounding context (lines 695-750)
sed -n '695,750p' src/routes/song.$songId.tsx | cat -n

Repository: EtanHey/songscript

Length of output: 125

🏁 Script executed:

# Check for test file for this component
find . -name "*song*test*" -o -name "*test*song*" | head -20

Repository: EtanHey/songscript

Length of output: 102

🏁 Script executed:

# Search for stopPropagation in the tsx files to understand propagation handling
rg "stopPropagation" --type tsx -B2 -A2 | head -100

Repository: EtanHey/songscript

Length of output: 89

🏁 Script executed:

# Check the global Space handler implementation (around lines 669-679)
sed -n '665,685p' src/routes/song.$songId.tsx | cat -n

Repository: EtanHey/songscript

Length of output: 125

🏁 Script executed:

# Find the actual song route file
find . -path "*/routes/*song*" -type f | head -20

Repository: EtanHey/songscript

Length of output: 91

🏁 Script executed:

# List route files to see naming pattern
ls -la src/routes/ | head -20

Repository: EtanHey/songscript

Length of output: 724

🏁 Script executed:

# Read the file with proper escaping - use single quotes to prevent variable expansion
wc -l './src/routes/song.$songId.tsx'

Repository: EtanHey/songscript

Length of output: 95

🏁 Script executed:

# Read the onKeyDown handler and surrounding context (lines 695-750)
sed -n '695,750p' './src/routes/song.$songId.tsx' | cat -n

Repository: EtanHey/songscript

Length of output: 3641

🏁 Script executed:

# Check the global Space handler implementation (around lines 665-685)
sed -n '665,685p' './src/routes/song.$songId.tsx' | cat -n

Repository: EtanHey/songscript

Length of output: 887

🏁 Script executed:

# Search for aria-expanded in the file
rg "aria-expanded" './src/routes/song.$songId.tsx' -n

Repository: EtanHey/songscript

Length of output: 44

🏁 Script executed:

# Check wrapper divs at lines 719-746 and their stopPropagation calls
sed -n '700,746p' './src/routes/song.$songId.tsx' | cat -n

Repository: EtanHey/songscript

Length of output: 3158

🏁 Script executed:

# Check for test file for song.$songId.tsx with different naming patterns
find . -name "*song*test*" -o -name "*test*song*" -o -path "*__tests__*song*" 2>/dev/null

Repository: EtanHey/songscript

Length of output: 102

🏁 Script executed:

# Check if there's a test file matching the component filename exactly
ls -la './src/routes/song.$songId.test.tsx' 2>&1

Repository: EtanHey/songscript

Length of output: 143

🏁 Script executed:

# Search for any onKeyDown stopPropagation in the codebase to see if it's a pattern
rg "onKeyDown" --type tsx -A3 | grep -A3 "stopPropagation"

Repository: EtanHey/songscript

Length of output: 89

🏁 Script executed:

# Verify ToggleGroup and WishlistButton implementations - check if they handle keydown internally
rg "WishlistButton\|ToggleGroup" './src/routes/song.$songId.tsx' -B2 -A5

Repository: EtanHey/songscript

Length of output: 44

🏁 Script executed:

# Check what library provides ToggleGroup to understand keydown behavior
rg "from.*ToggleGroup\|import.*ToggleGroup" --type tsx

Repository: EtanHey/songscript

Length of output: 89

🏁 Script executed:

# Search for ToggleGroup import
grep -n "ToggleGroup" './src/routes/song.$songId.tsx' | head -10

Repository: EtanHey/songscript

Length of output: 928

🏁 Script executed:

# Search for imports at the top of the file
head -50 './src/routes/song.$songId.tsx' | cat -n

Repository: EtanHey/songscript

Length of output: 2532

🏁 Script executed:

# Check if WishlistButton or ToggleGroup stop keydown propagation by searching their source files
find ./src -name "WishlistButton*" -o -name "*ToggleGroup*" -o -name "*wishlist*" | head -20

Repository: EtanHey/songscript

Length of output: 97

🏁 Script executed:

# Look for test files in the codebase to understand the pattern
find ./src -name "*.test.ts" -o -name "*.test.tsx" | head -20

Repository: EtanHey/songscript

Length of output: 457

🏁 Script executed:

# Check the ToggleGroup implementation to see if it stops keydown propagation
cat -n './src/components/ui/toggle-group.tsx'

Repository: EtanHey/songscript

Length of output: 2956

🏁 Script executed:

# Check WishlistButton to see if it handles keydown
cat -n './src/components/WishlistButton.tsx'

Repository: EtanHey/songscript

Length of output: 6003

🏁 Script executed:

# Verify the exact line numbers in the original file
grep -n "onKeyDown" './src/routes/song.$songId.tsx' | head -5

Repository: EtanHey/songscript

Length of output: 101

🏁 Script executed:

# Check if this component file should have a test based on coding guidelines
# Verify the test naming convention used in existing test files
ls -la './src/components/'*.test.tsx | head -10

Repository: EtanHey/songscript

Length of output: 330

🏁 Script executed:

# Verify the behavior of ToggleGroupItem keydown by checking Radix UI behavior
# See if it implements any keydown handling that might stop propagation
rg "ToggleGroupItem\|ToggleGroupPrimitive" './src/components/ui/toggle-group.tsx' -A10 | head -40

Repository: EtanHey/songscript

Length of output: 44

🏁 Script executed:

# Check if there's any keydown stopPropagation in the entire route file
grep -n "stopPropagation" './src/routes/song.$songId.tsx'

Repository: EtanHey/songscript

Length of output: 234

🏁 Script executed:

# Check if the Radix UI ToggleGroupItem handles keydown internally and whether it stops propagation
# Look at how Radix UI typically handles Space/Enter on toggle buttons
web_search "Radix UI ToggleGroup keydown event propagation space enter"

Repository: EtanHey/songscript

Length of output: 112

🏁 Script executed:

# Verify aria-expanded is indeed missing from the role="button" div
grep -n "aria-expanded" './src/routes/song.$songId.tsx'

Repository: EtanHey/songscript

Length of output: 44

🏁 Script executed:

# Confirm coding guideline: check if route page components are typically tested
find ./src/routes -name "*.test.tsx" -o -name "*.test.ts"

Repository: EtanHey/songscript

Length of output: 44

🏁 Script executed:

# Double-check the exact content and structure around the onKeyDown to confirm line numbers in review
sed -n '700,720p' './src/routes/song.$songId.tsx' | cat -n

Repository: EtanHey/songscript

Length of output: 1316

🏁 Script executed:

# Verify if Radix UI ToggleGroupItem has built-in Space/Enter handling by checking if it prevents default
grep -A20 "ToggleGroupPrimitive.Item" './src/components/ui/toggle-group.tsx' | head -25

Repository: EtanHey/songscript

Length of output: 986

Add guard to prevent bubbling from nested interactive elements and missing aria attributes.

The wrapper divs at lines 719 and 725 only call e.stopPropagation() on onClick, not on onKeyDown. When a keyboard user focuses a ToggleGroupItem or WishlistButton and presses Space/Enter, the keydown event bubbles to this handler, which:

  1. Calls handleVideoCollapsedToggle() unintentionally
  2. Calls e.preventDefault() on a bubbled Space event — on some browsers this suppresses the click event that the nested <button> fires on keyup, potentially breaking the nested control

Additionally, the div[role="button"] is missing the aria-expanded attribute to convey its collapsed/expanded state to screen readers.

Add a target === currentTarget guard and add aria-expanded:

🛡️ Proposed fix 
                <div
                  role="button"
                  tabIndex={0}
+                 aria-expanded={!isVideoCollapsed}
                  onClick={handleVideoCollapsedToggle}
                  onKeyDown={(e) => {
-                   if (e.key === 'Enter' || e.key === ' ') {
+                   if ((e.key === 'Enter' || e.key === ' ') && e.target === e.currentTarget) {
                      e.preventDefault();
                      handleVideoCollapsedToggle();
                    }
                  }}

Per coding guidelines, components with logic must have corresponding test files. Add src/routes/song.$songId.test.tsx.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/routes/song`.$songId.tsx around lines 705 - 710, The keyboard handler on
the role="button" wrapper is missing a guard and aria state: update the
onKeyDown handler (the arrow function that calls handleVideoCollapsedToggle) to
first check that e.target === e.currentTarget before calling e.preventDefault()
or handleVideoCollapsedToggle so events from nested interactive elements like
ToggleGroupItem or WishlistButton don't bubble and trigger the toggle; also add
aria-expanded on the role="button" div and bind it to the component's collapsed
state (the same state used by handleVideoCollapsedToggle) so screen readers know
the expanded/collapsed status; finally add a corresponding test file
(song.$songId.test.tsx) to cover keyboard interaction and nesting cases.

className="w-full flex items-center justify-between px-4 py-2 bg-gray-800 border-b border-gray-700 hover:bg-gray-700/50 transition-colors cursor-pointer"
>
<div className="flex items-center gap-3 min-w-0 flex-1">
Expand Down