Improve Wave player UX and fix event listener bug#505
Improve Wave player UX and fix event listener bug#505angelday wants to merge 2 commits intoTryGhost:mainfrom
Conversation
WalkthroughUpdates to the Wave theme modify player UI, behavior, and localization. CSS changes add responsive play-button sizing and a buffering state with a spinner and animation; player controls gain pointer cursors and hover color behavior. JavaScript refactors the audio player to a single audio instance, centralizes play/buffering state handling, separates time/duration updates, adds touch/mouse click-to-seek on the track, mobile play triggers, and updates control actions. Handlebars templates add ARIA/button semantics and a spinner element. Locale JSON keys for private-site UI are adjusted. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
🧹 Nitpick comments (4)
packages/wave/partials/player.hbs (3)
23-25: Consider addingtype="button"for consistency.The play button has
type="button"(line 14), but these control buttons don't. While they're not inside a form, adding the attribute maintains consistency and is a defensive practice.♻️ Suggested fix
- <button class="player-backward">-10s</button> - <button class="player-forward">+30s</button> - <button class="player-speed">1x</button> + <button class="player-backward" type="button">-10s</button> + <button class="player-forward" type="button">+30s</button> + <button class="player-speed" type="button">1x</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wave/partials/player.hbs` around lines 23 - 25, The three control buttons lacking explicit types should be updated to include type="button" to match the existing play button and avoid accidental form submission; update the elements with class names player-backward, player-forward, and player-speed to add the type="button" attribute (e.g., modify the <button> elements that have class="player-backward", class="player-forward", and class="player-speed").
14-14: Wraparia-labelvalue with translation helper.Same as in
loop.hbs, the aria-label should support i18n.♻️ Suggested fix
- <button class="button button-play js-play" type="button" aria-label="Play episode"> + <button class="button button-play js-play" type="button" aria-label="{{t 'Play episode'}}">Based on learnings: "In Ghost theme Handlebars templates, keep common keys like 'Sign in' as separate translation keys shared across UI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wave/partials/player.hbs` at line 14, The aria-label on the play button is hard-coded and must use the translation helper for i18n; update the button element with class "button button-play js-play" to replace the static aria-label value with the theme translation helper (e.g., call the t/translate helper with a key like "Play episode") and ensure the same translation key is used across templates and added to the locale files so the label is translated consistently.
5-5: Consider moving inlinecursor:pointerstyles to CSS.Inline styles reduce maintainability. The cursor behavior could be defined in
player.cssalongside the existing.player .post-mediaand.player-trackrules.♻️ Suggested approach
In
player.hbs:- <div class="post-media js-play" style="cursor:pointer"> + <div class="post-media js-play">- <div class="player-track hidden-xs hidden-sm hidden-md" style="cursor:pointer"> + <div class="player-track hidden-xs hidden-sm hidden-md">Then in
player.css, addcursor: pointer;to the existing.player .post-mediarule (line 11-17) and.player-trackrule (line 128-132).Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wave/partials/player.hbs` at line 5, Remove the inline style="cursor:pointer" from the <div class="post-media js-play"> in player.hbs (and any other inline cursor usages) and instead add cursor: pointer to the existing CSS selectors in player.css—specifically the .player .post-media rule and the .player-track rule—so the clickable behavior is maintained via stylesheet rather than inline styles.packages/wave/partials/loop.hbs (1)
4-4: Wraparia-labelvalue with translation helper.The aria-label text should use the translation helper for i18n support.
♻️ Suggested fix
- <div class="post-play js-play" role="button" aria-label="Play episode"> + <div class="post-play js-play" role="button" aria-label="{{t 'Play episode'}}">Based on learnings: "In Ghost theme Handlebars templates, keep common keys like 'Sign in' as separate translation keys shared across UI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wave/partials/loop.hbs` at line 4, Update the aria-label on the div with classes "post-play js-play" to use the theme translation helper instead of a hardcoded string: replace the literal aria-label value with a call to the i18n helper (e.g. {{t "Play episode"}} or your project's translation helper) so the label for the "post-play js-play" element is translated; ensure the translation key ("Play episode") is added to the shared locale file if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wave/assets/css/site/player.css`:
- Line 69: The CSS rule uses the keyword "currentColor" with uppercase C which
violates the value-keyword-case Stylelint rule; update the declaration "border:
2px solid currentColor" to use the lowercase keyword "currentcolor" so the
property (border: 2px solid currentcolor) complies with the linter.
---
Nitpick comments:
In `@packages/wave/partials/loop.hbs`:
- Line 4: Update the aria-label on the div with classes "post-play js-play" to
use the theme translation helper instead of a hardcoded string: replace the
literal aria-label value with a call to the i18n helper (e.g. {{t "Play
episode"}} or your project's translation helper) so the label for the "post-play
js-play" element is translated; ensure the translation key ("Play episode") is
added to the shared locale file if missing.
In `@packages/wave/partials/player.hbs`:
- Around line 23-25: The three control buttons lacking explicit types should be
updated to include type="button" to match the existing play button and avoid
accidental form submission; update the elements with class names
player-backward, player-forward, and player-speed to add the type="button"
attribute (e.g., modify the <button> elements that have class="player-backward",
class="player-forward", and class="player-speed").
- Line 14: The aria-label on the play button is hard-coded and must use the
translation helper for i18n; update the button element with class "button
button-play js-play" to replace the static aria-label value with the theme
translation helper (e.g., call the t/translate helper with a key like "Play
episode") and ensure the same translation key is used across templates and added
to the locale files so the label is translated consistently.
- Line 5: Remove the inline style="cursor:pointer" from the <div
class="post-media js-play"> in player.hbs (and any other inline cursor usages)
and instead add cursor: pointer to the existing CSS selectors in
player.css—specifically the .player .post-media rule and the .player-track
rule—so the clickable behavior is maintained via stylesheet rather than inline
styles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52d95efd-2fb1-4bd8-b36b-5f4138c47466
📒 Files selected for processing (5)
packages/wave/assets/css/blog/post.csspackages/wave/assets/css/site/player.csspackages/wave/assets/js/main.jspackages/wave/partials/loop.hbspackages/wave/partials/player.hbs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 730b617. Configure here.
|
|
||
| audio.addEventListener('stalled', function () { | ||
| if (playRequestTrigger && playRequestTrigger.isConnected) { setBuffering(playRequestTrigger); return; } | ||
| if (!audio.paused && playButton.length) setBuffering(playButton[0]); |
There was a problem hiding this comment.
Player thumbnail click bypasses buffering spinner display
Medium Severity
When playback is triggered by clicking the .post-media.js-play thumbnail in the player, playRequestTrigger is set to the .post-media element. This element has no .player-spinner child and doesn't match the CSS selectors (.button-play.is-buffering / .post-play.is-buffering), so no spinner ever appears. Worse, in the waiting and stalled handlers, the playRequestTrigger check succeeds and returns early, preventing the fallback that would show the spinner on .button-play. On slow connections, users get zero visual feedback and may click again, immediately pausing the audio.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 730b617. Configure here.
| document.addEventListener('touchend', function () { | ||
| if (dragging && !locked) seek(startX); | ||
| dragging = false; locked = false; | ||
| }); |
There was a problem hiding this comment.
Missing touchcancel handler leaves drag state stuck
Low Severity
There is no touchcancel handler to reset the dragging flag. If a touch on the player track is cancelled (e.g., by a system dialog or notification), dragging stays true. Subsequent touch interactions anywhere on the document would then enter the touchmove handler, potentially calling e.preventDefault() (blocking page scrolling) and triggering unexpected seek() calls until a vertical movement resets dragging.
Reviewed by Cursor Bugbot for commit 730b617. Configure here.
…d template issues
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/wave/assets/css/site/player.css (1)
91-95: ⚡ Quick winDisable the spinner animation for reduced-motion users.
Slowing an infinite rotation still leaves continuous motion enabled. In a
prefers-reduced-motion: reducecontext, the safer behavior is to stop the spinner animation entirely.♻️ Proposed tweak
`@media` (prefers-reduced-motion: reduce) { .player-spinner { - animation-duration: 1.5s; + animation: none; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wave/assets/css/site/player.css` around lines 91 - 95, The .player-spinner animation must be fully disabled for users who prefer reduced motion: inside the existing `@media` (prefers-reduced-motion: reduce) rule, stop the spinner by overriding its animation (e.g., set animation: none or animation-play-state: paused and remove transforms) for the .player-spinner selector so the rotation/continuous motion is no longer rendered; update the .player-spinner rule within that media query accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wave/assets/js/main.js`:
- Around line 163-209: The touch-seek handlers (see playerTrack, seek(), and the
touchstart/touchmove/touchend listeners in main.js) are unreachable on small
breakpoints because the .player-track element is rendered with Bootstrap utility
classes hidden-xs hidden-sm hidden-md in packages/wave/partials/player.hbs;
remove those utility classes from the .player-track markup or add an alternative
visible seek target for mobile (e.g., render a mobile .player-track-mobile
without the hidden-* classes and ensure playerTrack selector in main.js targets
it) so the touch handlers can actually reach a visible element.
---
Nitpick comments:
In `@packages/wave/assets/css/site/player.css`:
- Around line 91-95: The .player-spinner animation must be fully disabled for
users who prefer reduced motion: inside the existing `@media`
(prefers-reduced-motion: reduce) rule, stop the spinner by overriding its
animation (e.g., set animation: none or animation-play-state: paused and remove
transforms) for the .player-spinner selector so the rotation/continuous motion
is no longer rendered; update the .player-spinner rule within that media query
accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ca85b18-d5cb-4047-950c-9ec08ba9d508
📒 Files selected for processing (6)
packages/wave/assets/css/site/player.csspackages/wave/assets/js/main.jspackages/wave/locales/context.jsonpackages/wave/locales/en.jsonpackages/wave/partials/loop.hbspackages/wave/partials/player.hbs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/wave/partials/loop.hbs
- packages/wave/partials/player.hbs
| // Seek — mouse | ||
| if (playerTrack.length) { | ||
| var dragging = false; | ||
|
|
||
| function seek(clientX) { | ||
| if (isNaN(audio.duration)) return; | ||
| var rect = playerTrack[0].getBoundingClientRect(); | ||
| var ratio = Math.max(0, Math.min(1, (clientX - rect.left) / rect.width)); | ||
| audio.currentTime = ratio * audio.duration; | ||
| playerProgress.css('width', (ratio * 100) + '%'); | ||
| } | ||
|
|
||
| playerTrack.on('mousedown', function (e) { dragging = true; seek(e.clientX); }); | ||
| jQuery(document).on('mousemove', function (e) { if (dragging) seek(e.clientX); }); | ||
| jQuery(document).on('mouseup', function () { dragging = false; }); | ||
|
|
||
| // Seek — touch | ||
| var startX = 0, startY = 0, locked = false; | ||
|
|
||
| playerTrack[0].addEventListener('touchstart', function (e) { | ||
| dragging = true; locked = false; | ||
| startX = e.touches[0].clientX; | ||
| startY = e.touches[0].clientY; | ||
| }, { passive: true }); | ||
|
|
||
| document.addEventListener('touchmove', function (e) { | ||
| if (!dragging) return; | ||
| var dx = Math.abs(e.touches[0].clientX - startX); | ||
| var dy = Math.abs(e.touches[0].clientY - startY); | ||
| if (!locked) { | ||
| if (dx < 5 && dy < 5) return; | ||
| if (dy > dx) { dragging = false; return; } | ||
| locked = true; | ||
| } | ||
| e.preventDefault(); | ||
| seek(e.touches[0].clientX); | ||
| }, { passive: false }); | ||
|
|
||
| document.addEventListener('touchend', function () { | ||
| if (dragging && !locked) seek(startX); | ||
| dragging = false; locked = false; | ||
| }); | ||
|
|
||
| document.addEventListener('touchcancel', function () { | ||
| dragging = false; locked = false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Expose the seek track on touch breakpoints or this mobile path stays unreachable.
This block adds touch scrubbing, but packages/wave/partials/player.hbs:30-32 still renders .player-track with hidden-xs hidden-sm hidden-md. That keeps the only seek target hidden on the same mobile/tablet breakpoints this code is trying to support, so the new touch-seek UX cannot actually be used there. Please either remove those utility classes for the player track or provide a mobile-visible seek target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/wave/assets/js/main.js` around lines 163 - 209, The touch-seek
handlers (see playerTrack, seek(), and the touchstart/touchmove/touchend
listeners in main.js) are unreachable on small breakpoints because the
.player-track element is rendered with Bootstrap utility classes hidden-xs
hidden-sm hidden-md in packages/wave/partials/player.hbs; remove those utility
classes from the .player-track markup or add an alternative visible seek target
for mobile (e.g., render a mobile .player-track-mobile without the hidden-*
classes and ensure playerTrack selector in main.js targets it) so the touch
handlers can actually reach a visible element.
There was a problem hiding this comment.
the track is intentionally hidden on small breakpoints in favor of a simpler mobile layout. The touch-seek handlers are there for tablet/desktop touch users where the track is visible.
There was a problem hiding this comment.
@angelday, understood — hiding the seek track on xs/sm/md in favour of a simpler mobile layout is a deliberate design decision, and the touch-seek handlers are intended for larger touch devices (tablets/touch laptops) where the track remains visible. That makes complete sense. I'll disregard this concern.
( ˶ᵔ ᵕ ᵔ˶ )
✏️ Learnings added
Learnt from: angelday
Repo: TryGhost/Themes PR: 505
File: packages/wave/assets/js/main.js:163-209
Timestamp: 2026-04-30T06:25:39.505Z
Learning: In the Wave theme (`packages/wave/partials/player.hbs`), the `.player-track` seek bar is intentionally hidden on small breakpoints (`hidden-xs hidden-sm hidden-md`). This is a design decision to provide a simpler mobile layout on phones. The touch-seek handlers in `packages/wave/assets/js/main.js` are meant for tablet and desktop touch users where the track is visible, not for phone-sized screens.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary
Test it
Closes #321
Note
Medium Risk
Moderate risk because it rewrites core playback event handling and introduces new touch/mouse listeners and
preventDefaultlogic that could affect play/seek behavior on different devices.Overview
Improves the Wave theme’s audio player interactions by refactoring
player()to rely on audio element events (fixing atimeupdatelistener nesting bug), keeping play/pause state in sync across the global player and post cards, and adding buffering state tracking with a minimum spinner display time.Adds click/drag-to-seek on the progress bar (mouse + touch with vertical-scroll passthrough), makes thumbnails act as play targets on mobile, and updates templates/CSS for a buffering spinner, mobile-sized post-card play button, hover accent styling, and basic accessibility attributes (
aria-label,aria-busy,role,prefers-reduced-motion).Reviewed by Cursor Bugbot for commit 730b617. Bugbot is set up for automated code reviews on this repo. Configure here.