fix(video-player): accessibility & controls-visibility UX for NetflixStylePlayer (#1333)#1374
Merged
rohan-pandeyy merged 5 commits intoJul 19, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesNetflix-style player accessibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Viewer
participant NetflixStylePlayer
participant VideoElement
Viewer->>NetflixStylePlayer: Enter player or press control key
NetflixStylePlayer->>VideoElement: Play, seek, mute, or fullscreen action
NetflixStylePlayer-->>Viewer: Show controls and metadata
NetflixStylePlayer-->>Viewer: Hide controls after timeout
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…tflixStylePlayer (AOSSIE-Org#1333) Follow-up to AOSSIE-Org#1329/AOSSIE-Org#1339 in the same component. Addresses the remaining accessibility and UX gaps: - Render title/description as an overlay that fades with the controls. It is non-interactive (pointer-events-none) so it never steals clicks from the play/pause area beneath it. - Expose the progress bar as an accessible slider: role="slider", aria-valuemin/max/now, aria-valuetext, focusable, seekable via Left/Right (+/-5s) and Home/End. Stops event propagation so the seek-bar keys don't also trigger the global +/-10s shortcut. - Add descriptive, state-aware aria-labels to every control button and the volume slider (play/pause, mute, fullscreen toggles). Forward aria-label through the shared Slider onto the Radix Thumb. - Rework controls-visibility: visible by default while paused (Netflix-style), auto-hide after a delay and on mouse-leave while playing, reveal on touch and keyboard focus. Hidden controls are non-interactive (pointer-events-none). Adds regression tests for the title/description overlay, seek-bar accessibility and keyboard seeking, control aria-labels, and the paused/playing visibility logic.
VanshajPoonia
force-pushed
the
fix/1333-netflix-player-a11y
branch
from
July 11, 2026 06:51
a4d7a66 to
ac752e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx (1)
66-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIgnore global shortcuts while an interactive control is focused.
Pressing Space on a focused control reaches this document listener and also triggers the control’s native activation. For example, Space on Play can toggle playback twice, while Space on Mute unexpectedly toggles playback.
Proposed fix
const handleKeyDown = (e: KeyboardEvent) => { const activeEl = document.activeElement; if ( - activeEl && - (activeEl.tagName === 'INPUT' || - activeEl.tagName === 'TEXTAREA' || - activeEl.getAttribute('contenteditable') === 'true') + activeEl instanceof HTMLElement && + activeEl.closest( + 'button, input, textarea, select, a[href], [contenteditable="true"], [role="slider"]', + ) ) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx` around lines 66 - 104, Update the handleKeyDown document listener in NetflixStylePlayer so it ignores shortcuts whenever the focused element is any interactive control, including BUTTON, A, SELECT, and other elements with interactive roles or tabIndex, in addition to INPUT, TEXTAREA, and contenteditable elements. Return before the switch to prevent native control activation from also triggering togglePlay, toggleMute, or other global shortcuts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/VideoPlayer/__tests__/NetflixStylePlayer.test.tsx`:
- Around line 367-388: Extend the volume accessibility test to cover the slider:
update the Slider mock used by NetflixStylePlayer tests to forward its
aria-label prop, then assert that getByRole('slider', { name: 'Volume' }) is
present in the control-label test alongside the button assertions.
In `@frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx`:
- Around line 39-55: Controls currently hide after the temporary reveal timeout
even when keyboard focus remains inside the player. Add separate focus-within
state, set it on the player’s focus handler and clear it only on blur when focus
moves outside the player; include this state in controlsVisible and update the
relevant focus/blur handlers and control visibility logic so focused controls
remain visible.
---
Outside diff comments:
In `@frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx`:
- Around line 66-104: Update the handleKeyDown document listener in
NetflixStylePlayer so it ignores shortcuts whenever the focused element is any
interactive control, including BUTTON, A, SELECT, and other elements with
interactive roles or tabIndex, in addition to INPUT, TEXTAREA, and
contenteditable elements. Return before the switch to prevent native control
activation from also triggering togglePlay, toggleMute, or other global
shortcuts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e9a59a5-ee8e-4c38-a6d7-33d61b972a44
📒 Files selected for processing (3)
frontend/src/components/VideoPlayer/NetflixStylePlayer.tsxfrontend/src/components/VideoPlayer/__tests__/NetflixStylePlayer.test.tsxfrontend/src/components/ui/Slider.tsx
… guard, slider label test - Keep controls visible while keyboard focus remains inside the player: track focus-within state on the container (cleared only when focus leaves the player) and include it in controlsVisible, so a focused control can no longer go invisible mid-interaction. - Ignore global keyboard shortcuts while any interactive control is focused (button, select, link, slider, etc.), preventing Space on a focused button from both activating it natively and firing the shortcut (e.g. double play-toggle). - Forward aria-label through the test Slider mock and assert the volume slider's accessible name; add a regression test for the focus-within visibility behaviour.
rohan-pandeyy
requested changes
Jul 11, 2026
rohan-pandeyy
left a comment
rohan-pandeyy
left a comment
Member
There was a problem hiding this comment.
Please review and compress all those comments which are going too long.
Maintainer asked to keep comments to 1-2 lines max; trimmed the four that ran 2-4 lines down to single lines without losing the intent.
… resolution unrelated to this PR's files)
Contributor
Author
|
Heads up on the failing Linting check, it's not related to this PR's changes. The check fails on
Since the workflow uses Given this looks environment-side, could you take a look or re-run the check when you get a chance? Happy to dig further if useful, flagging in case it's also blocking other open PRs. |
Member
|
Thanks for your contribution @VanshajPoonia |
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.
Addressed Issues
Fixes #1333 (follow-up to #1329 / #1339 in the same component).
What this does
Builds on the merged #1339 desync fixes to close the remaining accessibility and UX gaps in
NetflixStylePlayer:pointer-events-none, so it never steals clicks from the play/pause area beneath it.role="slider",aria-valuemin/max/now,aria-valuetext, focusable (tabIndex=0), and seekable via ←/→ (±5s) and Home/End. Stops event propagation so the seek-bar keys don't also fire the global ±10s shortcut.aria-labelis forwarded through the sharedSlideronto the RadixThumb.pointer-events-none) rather than merely transparent.Screenshots / Recordings
No layout/styling changes to the existing controls. New user-visible behavior:
Testing
tsc --noEmit,eslint, andvite buildall clean.AI Usage Disclosure
Tool used: Claude Code (Claude Opus 4.8).
Checklist
Summary by CodeRabbit
New Features
Bug Fixes