feat(player): add volume/mute controls - #651

Substantively good — clean attribute/property/event surface, careful clamp, controls UI follows the existing pattern (slider scrub mirrors the scrubber, listeners removed in destroy()). Holding the stamp on one specific finding around the runtime side, plus a few non-blocking notes.

⚠️ Blocker-tier observation: runtime bridge has no set-volume handler

The PR description states that set-volume is "forwarded to the iframe runtime," and the test at hyperframes-player.test.ts:1272 confirms the postMessage is sent. But the iframe runtime doesn't currently act on it.

packages/core/src/runtime/types.ts:9-18 — RuntimeBridgeControlAction union:

export type RuntimeBridgeControlAction =
  | "play" | "pause" | "seek"
  | "set-muted" | "set-media-output-muted"
  | "set-playback-rate"
  | "enable-pick-mode" | "disable-pick-mode"
  | "flash-elements";
  // ← no "set-volume"

packages/core/src/runtime/bridge.ts:39-50 — installRuntimeControlBridge:

if (action === "set-muted") { deps.onSetMuted(...); return; }
if (action === "set-media-output-muted") { deps.onSetMediaOutputMuted(...); return; }
if (action === "set-playback-rate") { deps.onSetPlaybackRate(...); return; }
// ← no "set-volume" branch

Concrete impact:

• Parent path (composition has audio-src attribute, OR runtime emitted media-autoplay-blocked and the host promoted to parent proxies): works fully — m.el.volume = v in attributeChangedCallback.
• Runtime path (default — no audio-src, autoplay not blocked, runtime owns audible playback): the volume slider has no audible effect. Any <video> / <audio> rendered inside the composition keeps its original volume regardless of slider position. Mute still works on this path because set-muted IS handled.

This is exactly the bucket the unchecked manual smoke ([ ] Manual: open a composition with audio, verify ... volume slider adjusts level) would have caught. Two reasonable resolutions:

1. Add the runtime-side handler (preferred — small, mirrors set-muted):

   // types.ts
   export type RuntimeBridgeControlAction =
     | ...
     | "set-volume";
   
   // RuntimeBridgeControlMessage: add `volume?: number;`
   
   // bridge.ts BridgeDeps: + onSetVolume: (volume: number) => void;
   
   // bridge.ts handler:
   if (action === "set-volume") {
     deps.onSetVolume(Number(data.volume ?? 1));
     return;
   }

   And at the runtime install site (wherever installRuntimeControlBridge(deps) is wired), set el.volume on the same media elements that set-muted walks. Plus a corresponding bridge.test.ts case mirroring the existing set-muted test. Should be ~30 lines.

2. Scope the feature explicitly in this PR — update the PR description and add a comment in hyperframes-player.ts:369-376 noting "volume only takes effect for parent-owned audio (audio-src set, or after autoplay-blocked promotion); runtime-owned audio currently doesn't honor set-volume." Land the runtime handler in a follow-up PR.

Either is fine — but as-shipped, a user enabling controls on a typical composition (no audio-src) sees the slider work visually but produce no audible change. That'll generate a "is this broken?" report.

Other observations (non-blocking)

Volume slider doesn't auto-unmute. Every native player I know (HTML5 <video> controls, YouTube, Spotify) treats "user moves the volume slider" as an implicit unmute — keeping mute and volume orthogonal is surprising. Concrete: user clicks mute → silent. User then drags volume slider to 50% → still silent (because muted flag is still on), but the slider visually reads 50% and the speaker icon shows muted-with-X. The behavior is correct per the model (mute and volume are independent), but it's UX-confusing. Suggest: in controls.ts:handleVolumeAt, if isMuted is true and the user scrubs to a non-zero value, also fire callbacks.onMuteToggle() to unmute. Optional; depends on whether you want native-player parity or to keep them independent.

A11y gap on the volume slider. controls.ts:107 — the mute button has aria-label="Mute"/"Unmute" (good), but the volume slider element has no role="slider", no aria-valuemin/max/now, and no keyboard handler (Arrow keys don't adjust volume). Mouse-only volume control. The existing time-scrubber has the same limitation, so this is consistent with the current bar — but worth tracking as a follow-up if a11y is a goal for the OSS player.

Mute icon on volume=0 + unmuted state. getVolumeIcon at controls.ts:120-124 shows the muted icon when volume === 0 even if muted is false. That's fine — visually consistent — but combined with the mute button toggling aria-label, it's possible to have:

• volume=0, muted=false: button shows muted icon, aria-label says "Mute"
• volume=0, muted=true: button shows muted icon, aria-label says "Unmute"

Same icon, different label. Probably fine (screen readers will read the label correctly), but a sighted user can't tell those two states apart visually. Edge case, not worth fixing alone.

Volume change while runtime owns audio AND no audio-src — the parent-media loop is a no-op. hyperframes-player.ts:371 — for (const m of this._parentMedia) m.el.volume = v; is fine but iterates an empty array on the runtime path. Once the runtime handler lands, this becomes the "for both paths" code; today it's parent-only. Just an observation.

Test coverage gaps that would have caught the runtime miss:

• No test asserts that the runtime receives and applies set-volume (it can't, because it doesn't). A test mirroring bridge.test.ts:52 ("dispatches set-muted command") would have surfaced the gap during development.
• No test for the volume slider mouse-scrub interaction in controls.ts (the mousedown → mousemove → mouseup chain). The slider-presence test verifies the DOM exists but not that scrubbing works.
• No test for "volume slider while muted — does it unmute or stay silent?" — depends on which behavior you want, but pinning either is worth doing.

Praise: The _setupParentAudioFromUrl change at hyperframes-player.ts:1271-1272 — applying both el.muted and the new el.volume to fresh parent audio elements — is the right hook. Without it, a player created with volume="0.5" audio-src="..." set in HTML markup would have applied volume to non-existent media at construction time and lost the value before _parentMedia got populated. Test 2 (sets volume on parent media when audio-src is configured) pins this. Easy to miss, well caught.

Summary

Holding stamp pending one of the two runtime-bridge resolutions above. Everything else is non-blocking — would land cleanly on a follow-up if you address the runtime gap in this PR.

— Review by Rames Jusso (pr-review)

Verdict: Request changes — one blocker (iframe-side set-volume is a no-op), several important gaps.

Nice scope discipline (4 files, no drive-bys) and the parent-media plumbing is correct. The reason I'm blocking is that the headline claim — "Volume forwarded to parent-frame audio proxies and iframe runtime via set-volume postMessage control" — only holds for the parent-frame audio proxy path. Inside the iframe runtime, the message is silently dropped. Details below.

Blockers

• blocker — packages/core/src/runtime/bridge.ts & packages/core/src/runtime/types.ts: There is no onSetVolume handler in the runtime bridge. The action "set-volume" is not in RuntimeBridgeControlAction, not in the if (action === ...) chain in installRuntimeControlBridge, and not in HYPERFRAME_CONTROL_ACTIONS in inline-scripts/runtimeContract.ts. Result: every set-volume postMessage from the player is received by the iframe and ignored. Volume control therefore only works for the cross-origin parent-frame audio proxy path (audio-src set). For the normal same-origin iframe playback path — which is the common one — the slider does nothing for in-iframe <audio>/<video>. To fix:

  1. Add "set-volume" to RuntimeBridgeControlAction and the volume?: number field on RuntimeBridgeControlMessage.
  2. Add "set-volume" to HYPERFRAME_CONTROL_ACTIONS.
  3. Add an onSetVolume(volume: number) dep + if (action === "set-volume") deps.onSetVolume(...) branch in installRuntimeControlBridge.
  4. Wire it in runtime/init.ts next to onSetMuted — iterate document.querySelectorAll("video, audio") and set el.volume. Also store on state so newly-mounted media (sub-compositions activating later) inherit the current volume, mirroring how onSetMuted works with state.bridgeMuted.
  5. Add a unit test in packages/core/src/runtime/bridge.test.ts covering set-volume.

  Also: _sendControl(action: string, ...) in hyperframes-player.ts is typed as string, which is why the type system didn't catch this missing handler. Worth tightening to RuntimeBridgeControlAction in a follow-up so the next bridge action can't drift like this.

Important

• important — packages/player/src/hyperframes-player.ts:~232 (the muted attribute case): Setting muted does not dispatch volumechange. Per HTML5 spec, mute changes fire volumechange on HTMLMediaElement. Listeners that bind to volumechange to reflect mute state in surrounding UI will miss mute toggles. Fire dispatchEvent(new Event("volumechange")) from the "muted" branch of attributeChangedCallback for parity with the platform.

• important — packages/player/src/hyperframes-player.ts:~370 (volume case in attributeChangedCallback): volumechange fires on every attribute write, even when the value didn't change. Setting volume = player.volume in user code triggers a redundant event, and the postMessage + parent-media writes also re-execute. Compare against _old/this._volume and short-circuit no-ops. This also avoids gratuitous postMessage chatter when the slider drag emits the same value twice.

• important — packages/player/src/controls.ts:~140-160 (volume slider): No touch support. The existing scrubber correctly handles touchstart/touchmove/touchend; the new volume slider only listens for mousedown/mousemove/mouseup. On touch devices the slider is unscrubbable, and combined with .hfp-volume-wrap:hover .hfp-volume-slider-wrap { width: 64px } (no :focus-within / no tap-to-expand) the entire control is unreachable on phones/tablets. Either use Pointer Events for both scrubbers (preferred — one path for mouse + touch + pen) or duplicate the touch branch onto the volume slider. Add :focus-within, or a sticky tap-to-expand on coarse pointers, to the CSS expand rule.

• important — packages/player/src/controls.ts (volume slider a11y): The slider is a <div> with no role, no tabindex, no aria-valuenow/aria-valuemin/aria-valuemax, no keyboard handler. Screen readers see a meaningless container; keyboard users can't adjust volume at all. Either make it role="slider" with the ARIA attrs and arrow-key handlers (Up/Down/Home/End), or use a native <input type="range"> and style it. The mute button is fine — it has aria-label and is a real <button>.

• important — packages/player/src/controls.ts:~165-167 (volume scrub listeners) and :~233-234 (destroy): The scrub listener registration uses document.addEventListener("mousemove"/"mouseup", ...) and is removed in destroy. On reasonably-orchestrated control teardown this is fine, but the volume slider re-uses the same internal volumeScrubbing flag as the seek scrubber's scrubbing? Actually they're separate (volumeScrubbing vs scrubbing) — so no cross-talk. Nit-level: factor attachScrubber(track, onValue) so seek + volume share one implementation; the duplication is the source of the touch-support omission above.

• important — Mute/volume interaction contract: The PR doesn't define what happens when the user mutes, then drags the slider above 0 — does it auto-unmute (HTML5 <video> does not; mute and volume are independent), and does the slider visually reflect the mute icon while volume is non-zero? Right now getVolumeIcon shows the muted icon iff muted || volume === 0, but moving the slider while muted does NOT toggle isMuted, so the icon stays muted while the underlying value changes — confusing for users. Decide and document: either (a) HTML5-style independent (then dragging the slider while muted should still show the muted icon, which matches today), or (b) YouTube-style "unmute on slider drag" (then add the toggle). Either way add a test pinning the contract.

• important — Tests don't cover the cases that matter: Add tests for:

  • volume = NaN / volume = undefined → what does parseFloat(val || "1") produce? parseFloat(null) is NaN, Math.min(1, NaN) is NaN. Try player.setAttribute("volume", "abc") and assert behavior.
  • volumechange does NOT fire on no-op writes (after the fix above).
  • set-muted → does it also fire volumechange? (after the fix above)
  • Newly-mounted parent-media proxies (the dynamic case in _createParentMedia) inherit the current volume — there's a code path for this in the diff but no test asserting it works after a volume change followed by a new audio mount.
  • Iframe-runtime side: a bridge.test.ts case that set-volume actually applies to media elements.

Nits

• nit — controls.ts: let currentVolume = 1 is initialized to 1 but updateVolume(this._volume) is called immediately in _setupControls, so the player-side initial value wins. Fine, but consider taking initial volume as a ControlsOptions value to avoid the implicit two-step initialization.
• nit — styles.ts: The volume slider uses .hfp-volume-fill with position: absolute but .hfp-volume-slider doesn't declare position: relative — it works because the existing scrubber CSS sets it for .hfp-scrubber, but .hfp-volume-slider is a different class. Verify visually; if the fill ever escapes its container, this is why.
• nit — Magic number: 64px slider expand width and 0.5 low/high threshold are bare numerics. Not worth abstracting now, but if the design changes you'll want CSS variables.

Praise

• Volume is correctly applied to newly-created parent-media proxies in _createParentMedia — easy to forget for the late-mount sub-composition case. Good catch.
• Clamping in both the property setter and the attribute callback so setAttribute from outside HTML can't bypass it. Correct defensive layering.
• Icon swap based on (muted, volume) rather than just one — matches user expectations.
• Test coverage for the parent-media path (mockAudio.volume === 0.5) is the right shape.

— Vai

Re-verdict: Approve.

The blocker from my prior review is cleanly resolved at 5e2d8574. The runtime bridge now handles set-volume end-to-end with the composition logic explicit:

• set-volume added to RuntimeBridgeControlAction union, volume?: number on RuntimeBridgeControlMessage, handler in bridge.ts with [0,1] clamping
• bridgeVolume stored in RuntimeState, applied to media elements
• syncRuntimeMedia composes userVolume × clipAuthorVolume so per-clip author intent is preserved when the user adjusts the player slider — that's the right semantic, not just "user wins"
• Two new media tests pin the composition (userVolume × clip volume) so a future refactor can't silently drop one of the factors

Three of the six important items from my prior review are also addressed:

• HTML5 volumechange spec compliance — muted toggle now dispatches volumechange ✅
• Mobile — touch support added to the slider (touchstart/move/end) ✅
• Mute+slider interaction contract — defined: scrubbing above 0 while muted auto-unmutes (matches native player UX, also resolves Rames's identical finding) ✅

Three items still open — flagging as follow-ups, not blocking this PR:

• ARIA + keyboard control on the slider. Real accessibility miss for a player surface; should land in a follow-up commit/PR before this gets wide distribution. Standard pattern: role="slider", aria-valuemin/max/now, aria-label="Volume", arrow-key handling on the slider element.
• Volume=0 + unmuted vs muted state visual collision (per Rames's finding) — same icon for both; ARIA labels can disambiguate but sighted users still can't tell. Cheap fix: distinct icon for (volume=0, muted=false).
• No debounce on no-op volumechange events. Minor; only matters if downstream consumers do expensive work in the handler.

postMessage origin check on set-volume is also worth filing as a hardening follow-up, but it's not a regression introduced by this PR.

Nice work on the iframe-side wiring — the userVolume × clipVolume composition test in particular is the right shape for catching future regressions.

— Vai

LGTM at 5e2d8574 — blocker cleanly resolved, all four implementation paths I traced are sound.

Round-1 concerns: resolved

• ✅ Blocker (runtime bridge gap) — set-volume wired end-to-end: action union (types.ts:14), BridgeDeps.onSetVolume (bridge.ts:8) with [0,1] clamping at the boundary (bridge.ts:44), state.bridgeVolume (state.ts:13,82), init.ts:onSetVolume walks media elements composing clipVolume × userVolume, and syncRuntimeMedia carries userVolume through every per-tick re-sync (media.ts:151). The fix matches the set-muted precedent exactly. Test coverage at bridge.test.ts (dispatch, clamp, default) + media.test.ts (composition: 0.8 × 0.5 = 0.4, default × user = user) — both paths pinned.

• ✅ Muted dispatches volumechange — hyperframes-player.ts:368. HTML5 spec parity ✓.

• ✅ Auto-unmute on slider scrub — controls.ts:168: if (isMuted && fraction > 0) callbacks.onMuteToggle();. Synchronous attribute round-trip means the icon read on the next line sees the updated state correctly.

• ✅ Touch support — controls.ts:187-204. Mirrors mouse pattern, listeners cleaned up in destroy().

One small concern, non-blocking — composition divergence on data-volume="0"

The two paths that compose user volume × per-clip author volume use different fallback semantics:

media.ts:151 (per-tick sync):

el.volume = (clip.volume ?? 1) * userVol;  // ?? — preserves 0

init.ts:1561 (bridge handler, walks DOM directly):

const clipVolume = parseFloat(el.dataset.volume ?? "") || 1;  // || — coerces 0 to 1
el.volume = clipVolume * volume;

For <audio data-volume="0"> (intentionally-silenced clip):

• On per-tick sync → el.volume = 0 * userVol = 0 ✓ stays silent
• On set-volume bridge message → parseFloat("0") = 0, 0 || 1 = 1, el.volume = 1 * userVol = userVol ✗ now plays at user volume

So a data-volume="0" clip is silent during normal playback but starts audibly playing the moment the user touches the volume slider. That's strictly-worse-than-master for that specific case.

Edge case (composition authors rarely write data-volume="0" — they'd just omit the audio element), but the inconsistency between two paths in the same PR that should compose identically is the actual code smell. Suggested follow-up:

// init.ts:1561 — match media.ts semantics
const parsed = parseFloat(el.dataset.volume ?? "");
const clipVolume = Number.isFinite(parsed) ? parsed : 1;
el.volume = clipVolume * volume;

Not blocking — this PR is a strict improvement over master for every realistic case. Worth a follow-up commit / one-line cleanup.

Other follow-ups (consistent with what Vai flagged)

• ARIA / keyboard control on the slider — real a11y miss for an OSS player widget
• volume=0 + unmuted vs muted icon collision (my round-1 finding) — same icon, different label
• postMessage origin check on the bridge handler — hardening, not a regression

All deferrable. None of them block this PR.

Praise

• The userVolume × clipAuthorVolume composition is the right semantic model — gives composition authors per-track volume control while still respecting the user's player-level slider. Clean separation of concerns.
• Test at media.test.ts pinning 0.8 × 0.5 = 0.4 is exactly the right shape — catches drift in the composition formula without coupling to internals.
• The installRuntimeControlBridge clamp at Math.max(0, Math.min(1, Number(data.volume ?? 1))) is defense-in-depth on top of the player-side clamp. Right place to harden against malformed bridge messages from any source.

Ship it.

— Review by Rames Jusso (pr-review)

Re-verdict: Approve.

All remaining items from my prior re-review are cleanly resolved at 9ef99ff9:

• ARIA + keyboard control — slider has role="slider", aria-label="Volume", aria-valuemin/max/now, tabindex=0, arrow-key support (5% steps), auto-unmutes on key-up from muted. That's the full keyboard-accessibility shape, not a partial. New test pins the ARIA attribute presence.
• Icon collision — (volume=0, muted=false) now shows the low-volume icon; (muted=true) shows the muted icon. Visually distinguishable, pinned by test. Resolves Rames's finding.
• clipVolume parity — init.ts uses Number.isFinite(parsed) ? parsed : 1 matching media.ts. data-volume="0" is now preserved at both call sites instead of silently coerced to 1 in the bridge path. Cleanly resolves Rames's || vs ?? catch.

Three commits, three rounds of clean responses to feedback — 9977ed89 → 5e2d8574 → 9ef99ff9. Each commit was tightly scoped to the items raised.

Only item left from the original review is the postMessage origin check on set-volume, which I flagged as hardening / not-a-regression and would belong in a follow-up. Not gating this PR.

Shipping.

— Vai

Re-approving at 9ef99ff9 — all three remaining follow-ups addressed cleanly.

Verified:

• ✅ clipVolume parity (init.ts:1564-1565): now uses Number.isFinite(parsed) ? parsed : 1, matching media.ts:151 (clip.volume ?? 1) semantics. data-volume="0" is now preserved across both paths — the divergence I flagged is gone. Implementation matches my suggested snippet verbatim.

• ✅ ARIA + keyboard (controls.ts:121-126, 216-234): role="slider", aria-label="Volume", aria-valuemin/max/now, tabindex=0 on the slider. Keyboard handler covers Arrow Right/Up/Left/Down with 5% steps, clamps to [0,1], preventDefault to block page scroll, auto-unmutes on key-up from muted (consistent with mouse-scrub behavior). aria-valuenow is updated in all three mutation paths (mouse/touch scrub at :175, keyboard at :231, updateVolume API at :354) — no drift. ARIA test pins the initial state.

• ✅ Icon collision (controls.ts:151-153): getVolumeIcon now distinguishes muted=true (always MUTED_ICON) from volume=0 unmuted (LOW_ICON). Test at line 1339 explicitly asserts zeroVolumeHtml !== mutedHtml. Reasonable design call — unmuted-but-zero shows LOW so users can tell "I dragged the slider to 0" from "I clicked mute."

Two micro-gaps in coverage (not blocking, deferrable):

• The keyboard handler's actual behavior (arrow adjusts volume, auto-unmutes from muted) isn't pinned by a test — only the ARIA attributes are. A simulate keydown ArrowRight, expect volume increase by 5% test would prevent silent regression.
• aria-valuenow initial value is asserted, but not the update path — no test for "scrub to 0.5, expect aria-valuenow='50'." Easy to add.

Both small enough to call follow-ups. Implementation is sound and matches the contracts pinned by the existing tests.

Ship it. Solid iteration loop — Magi addressed every reviewer item across two fix-up commits without scope creep.

— Review by Rames Jusso (pr-review)