Improve Castos player accessibility (#811)#894
Conversation
Adds ARIA attributes for screen reader support: aria-pressed on mute toggle, dynamic aria-label on speed button, aria-expanded on panel triggers, Escape to close panels, aria-selected on playlist items, and aria-live announcement for copy actions. Close buttons changed from div to button elements. Co-Authored-By: Claude via AIContext
📝 WalkthroughWalkthroughThis pull request enhances the Castos player's accessibility by introducing localized screen reader text, adding ARIA roles and attributes to controls and regions, creating utility helpers for screen reader announcements and panel management, and extending keyboard handling with Escape-key support for closing panels. ChangesPlayer Accessibility Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.51)Composer install failed: this project depends on private packages that require authentication (e.g. GitLab/GitHub, Laravel Nova, etc.). Instead, run PHPStan in a CI/CD pipeline where you can use custom packages — our pipeline remediation tool can use the PHPStan output from your CI/CD pipeline. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
templates/players/castos-player.php (1)
116-124: 💤 Low valueConsider adding
aria-labelto subscribe links for consistency with share links.The share links (Facebook, Twitter, Download) at lines 149-158 include
aria-labelattributes, but the subscribe links in this loop only havetitle. For consistent accessibility, consider addingaria-labelhere as well.♿ Optional: Add aria-label to subscribe links
<a href="<?php echo esc_url( $subscribe_link['url'] ) ?>" target="_blank" rel="noopener noreferrer" class="<?php echo esc_attr( $subscribe_link['class'] ) ?>" - title="Subscribe on <?php echo esc_attr( $subscribe_link['label'] ) ?>"> + title="Subscribe on <?php echo esc_attr( $subscribe_link['label'] ) ?>" + aria-label="<?php printf( esc_attr__( 'Subscribe on %s', 'seriously-simple-podcasting' ), esc_attr( $subscribe_link['label'] ) ); ?>">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/players/castos-player.php` around lines 116 - 124, The subscribe link anchors in the foreach over $subscribe_links (using $subscribe_link) only set a title but lack an aria-label for accessibility consistency with the share links; update the anchor in templates/players/castos-player.php to add an aria-label attribute (e.g. aria-label="<?php echo esc_attr( 'Subscribe on ' . $subscribe_link['label'] ) ?>") alongside the existing title, ensuring you use esc_attr() for escaping the label text just as is done for title and class.assets/js/castos-player.js (1)
84-96: 💤 Low valueReconsider
aria-pressedusage on play/pause buttons.The
aria-pressedattribute is typically used for toggle buttons. Here, the play and pause buttons are separate elements where one is always hidden. Settingaria-pressedon a hidden button doesn't provide value to assistive technology users.Consider either:
- Using a single toggle button with
aria-pressedand dynamicaria-label("Play"/"Pause")- Removing
aria-pressedfrom these buttons entirely since they're not togglesThe current approach where the hidden pause button has
aria-pressed="true"when audio is paused doesn't align with standard ARIA patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/castos-player.js` around lines 84 - 96, syncPlayButton currently toggles aria-pressed on two separate elements (playBtn and pauseBtn), which is not appropriate for separate show/hide buttons; remove all aria-pressed manipulation from syncPlayButton and instead update the accessible label of the visible control (use playBtn.setAttribute('aria-label', 'Play') when audio.paused and pauseBtn.setAttribute('aria-label', 'Pause') when playing, or simpler: clear aria-label on the hidden button and set the correct aria-label on the visible one), ensuring the hidden element does not present conflicting ARIA state; reference the syncPlayButton function and the playBtn, pauseBtn and audio variables when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assets/js/castos-player.js`:
- Around line 84-96: syncPlayButton currently toggles aria-pressed on two
separate elements (playBtn and pauseBtn), which is not appropriate for separate
show/hide buttons; remove all aria-pressed manipulation from syncPlayButton and
instead update the accessible label of the visible control (use
playBtn.setAttribute('aria-label', 'Play') when audio.paused and
pauseBtn.setAttribute('aria-label', 'Pause') when playing, or simpler: clear
aria-label on the hidden button and set the correct aria-label on the visible
one), ensuring the hidden element does not present conflicting ARIA state;
reference the syncPlayButton function and the playBtn, pauseBtn and audio
variables when making the change.
In `@templates/players/castos-player.php`:
- Around line 116-124: The subscribe link anchors in the foreach over
$subscribe_links (using $subscribe_link) only set a title but lack an aria-label
for accessibility consistency with the share links; update the anchor in
templates/players/castos-player.php to add an aria-label attribute (e.g.
aria-label="<?php echo esc_attr( 'Subscribe on ' . $subscribe_link['label'] )
?>") alongside the existing title, ensuring you use esc_attr() for escaping the
label text just as is done for title and class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ff8c6bc-28ba-48f7-887c-eaef1c94b9f7
📒 Files selected for processing (4)
assets/js/castos-player.jsphp/classes/controllers/class-players-controller.phptemplates/players/castos-player.phptests/WPUnit/PlayersControllerTest.php
1 participant
Summary
aria-pressedtoggle; speed button: dynamicaria-labelwith current speedaria-expandedon trigger buttons, Escape key to closerole="listbox"/role="option"witharia-selectedon active itemaria-live="polite"region announces "Copied" to screen readers<div>to semantic<button>witharia-labelwp_localize_scriptfor i18n supportTest plan
aria-expandedtoggles, Escape closes and returns focuscodecept run WPUnit PlayersControllerTest)Summary by CodeRabbit
New Features
Enhancements