Merge QA: working volume on browser players, and mute is no longer collateral

# Conflicts:
#	server/player/sw.js
#	server/test/player-sw-scope.test.js

server/player/index.html

@@ -1676,11 +1676,12 @@
           if (BS && BS.reboot()) console.log('[bs] reboot requested via host');
           else console.log('reboot: not supported on this player');
         }
-        // Media volume, 0-100 from the dashboard. Applies to whatever is playing now and is
-        // remembered for items mounted later.
+        // Media volume. Applies to whatever is playing now and is remembered for items mounted
+        // later (see setMediaVolume). The wire parsing is volumeLevelFromCommand().
         if (data.type === 'set_volume') {
-          const pct = Number(data.payload?.value ?? data.value);
-          if (isFinite(pct)) setMediaVolume(Math.max(0, Math.min(100, pct)) / 100);
+          const v = volumeLevelFromCommand(data);
+          if (v === null) console.warn('[volume] set_volume with no usable level/value:', JSON.stringify(data));
+          else setMediaVolume(v);
         }
       });
 
@@ -4032,6 +4033,42 @@
       try { return BS.displayPower(on); } catch (e) { return false; }
     }
 
+    /*
+     * The 0..1 volume a set_volume command is asking for, or null when it does not carry one.
+     *
+     * The wire form is `payload.level`, a FRACTION. That is what the dashboard sends
+     * (device-detail.js: `{ level: slider / 100 }`), what a group command relays unchanged, and what
+     * the Android player reads (`optDouble("level")`) — so the fraction is canonical and this player
+     * now matches it instead of inventing a third convention.
+     *
+     * What was here read `payload.value` and treated it as a percentage, which matched nothing the
+     * product sends: the slider was a no-op on every browser panel. Correcting only the KEY would
+     * have been worse than leaving it broken — 0.5 clamped as a percentage is 0.5%, inaudible, and
+     * it would have looked fixed to anyone who checked only that the handler ran.
+     *
+     * `value` is still accepted as a 0..100 percentage for any caller written against the old
+     * handler. The scale is chosen by WHICH KEY ARRIVED, never by the magnitude of the number: 1 is
+     * legal in both scales (1% and full volume), so a magnitude test is guaranteed to be wrong for
+     * somebody, silently, in whichever direction hurts more.
+     *
+     * Extracted from the socket handler so the parsing can be asserted without a socket — the bug
+     * it replaces was invisible precisely because nothing ever ran it against a real payload.
+     */
+    function volumeLevelFromCommand(data) {
+      if (!data || typeof data !== 'object') return null;
+      const p = (data.payload && typeof data.payload === 'object') ? data.payload : {};
+      const pick = (k) => {
+        const raw = (p[k] !== undefined && p[k] !== null) ? p[k] : data[k];
+        if (raw === undefined || raw === null || raw === '' || typeof raw === 'boolean') return null;
+        const n = Number(raw);
+        return isFinite(n) ? n : null;
+      };
+      const level = pick('level');                                   // canonical: 0..1
+      const pct = level === null ? pick('value') : null;             // legacy: 0..100
+      const v = level !== null ? level : (pct !== null ? pct / 100 : null);
+      return v === null ? null : Math.max(0, Math.min(1, v));
+    }
+
     // Volume that survives the next item. Media elements are created per item, so remembering the
     // level is what makes a volume command stick rather than lasting until the playlist advances.
     let mediaVolume = null;
@@ -4040,8 +4077,17 @@
       // Wall followers stay silent by design; don't override that.
       try { if (typeof isWallFollower === 'function' && isWallFollower()) return; } catch (e) { /* not a wall */ }
       document.querySelectorAll('video, audio').forEach((el) => {
-        try { el.volume = v; el.muted = v === 0; } catch (e) { /* element torn down mid-call */ }
+        try { el.volume = v; } catch (e) { /* element torn down mid-call */ }
       });
+      // Volume is a LEVEL. Mute is a separate decision with four inputs and a fixed order
+      // (lib/media-mute.js), already resolved when the element was mounted, and this function does
+      // not get to overrule it. It used to write `el.muted = (v === 0)`, which un-muted whatever was
+      // playing on any non-zero volume: an item an operator had deliberately silenced started making
+      // noise the moment somebody touched the volume slider — reproduced live (item flagged muted,
+      // one set_volume, muted went false). The same write contradicts the resolver's autoplay rule,
+      // where unmuting without a user gesture costs the VIDEO rather than winning the audio.
+      //
+      // Nothing is lost by leaving mute alone: volume 0 is silence on every media element we run on.
     }
 
     // Media elements are created per item across several code paths — fullscreen, zones, the
@@ -4059,7 +4105,10 @@
       }
       if (mediaVolume == null) return;
       try { if (typeof isWallFollower === 'function' && isWallFollower()) return; } catch (err) { /* not a wall */ }
-      try { el.volume = mediaVolume; el.muted = mediaVolume === 0; } catch (err) { /* gone */ }
+      // Level only — see setMediaVolume: the mount path has already resolved this element's mute
+      // from media-mute.js, and re-deciding it here from the volume number alone would undo a
+      // per-item mute (and, without a user gesture, the playback itself).
+      try { el.volume = mediaVolume; } catch (err) { /* gone */ }
     }, true);
 
     // Screen-off state, tracked because the playlist keeps advancing while the screen is "off"

server/player/sw.js

@@ -1,6 +1,6 @@
 // v24: an empty playlist payload no longer prunes. `assignments: []` is what the server sends for a
-// device between playlists AND for a snapshot that failed to parse, and treating it as "keep nothing"
-// wiped the panel's entire offline library on a message that means nothing of the sort.
+// device between playlists AND for a snapshot that failed to parse, and treating it as "keep
+// nothing" wiped the panel's entire offline library on a message that means nothing of the sort.
 // v23: offline.cache is claimed only when a worker is actually IN CONTROL — a real BrightSign
 // widget exposes navigator.serviceWorker, refuses to register one, and was advertising the
 // capability to the fleet regardless.
@@ -54,11 +54,26 @@ self.addEventListener('fetch', (event) => {
   const url = new URL(event.request.url);
 
   // Widget renders pinned to a revision: cache-FIRST, because those exact bytes cannot change
-  // without the rev changing. This is what lets a widget keep rendering when the network is gone —
-  // previously the server sent no-store for every render, so widgets were the one thing the
-  // player's offline cache could never hold, and a display that lost its uplink lost them.
-  // ignoreSearch is deliberately NOT used here: the query string carries the rev, and ignoring it
-  // would match a different revision's entry, which is the staleness we are trying to remove.
+  // without the rev changing. ignoreSearch is deliberately NOT used here: the query string carries
+  // the rev, and ignoring it would match a different revision's entry, which is the staleness we
+  // are trying to remove.
+  //
+  // MEASURED LIMIT, do not read more into this branch than it delivers. The player mounts widgets in
+  // an iframe sandboxed to `allow-scripts` with NO allow-same-origin (index.html,
+  // renderWidgetBuffered), so that frame is an OPAQUE-origin client — and a service worker does not
+  // control opaque-origin clients. Its navigation request never reaches this handler. Driven in
+  // Chrome against a live server: a clock widget mounted five times over 25s and the shell cache
+  // held zero widget entries, while a plain fetch() of the identical URL from the controlled page
+  // was intercepted and stored. So this branch serves anything that reaches it — a same-origin
+  // fetch, a future non-sandboxed mount — and today the player's own widgets are not that.
+  //
+  // What actually keeps widgets rendering offline right now is the HTTP cache plus the server's
+  // `max-age=31536000, immutable` on a rev-pinned render (routes/widgets.js). That is sound in a
+  // desktop browser and is NOT a documented-persistent store on BrightSign, which guarantees
+  // survival across reboots for IndexedDB, localStorage and SQLite only — the same gap that made
+  // content caching necessary. Closing it properly means routing the render through a same-origin
+  // fetch and mounting it as srcdoc; granting the frame allow-same-origin instead would hand widget
+  // scripts the player's origin, which is not a trade worth making for an offline nicety.
   if (url.pathname.startsWith('/api/widgets/') && url.pathname.endsWith('/render') && url.searchParams.has('rev')) {
     event.respondWith(
       caches.match(event.request).then(cached => {
@@ -146,14 +161,16 @@ self.addEventListener('message', (event) => {
   // writes a new randomly-named file, so the old copy lives at a different PATH and nothing keyed
   // on the asset path can find it. Without this the cache only grows, and on a panel with a 1GB
   // widget quota a handful of replaced videos is the entire budget.
+  //
   // An EMPTY list is never a prune instruction, and that distinction is the whole guard. "This
-  // display needs nothing" and "the payload did not arrive intact" are the same message on the wire,
-  // and the second is not rare: buildPlaylistPayload() yields `assignments: []` for a device between
-  // playlists, for a playlist never published, AND inside the catch when a published_snapshot fails
-  // to JSON.parse. Honouring it deleted every byte of media the panel held — three cached assets,
-  // one empty payload, cache emptied — which is only survivable while the uplink is up, i.e. exactly
-  // when this cache does not matter. A cache kept too long costs disk the quota reclaims anyway; one
-  // dropped at the wrong moment is a dark screen with no way back.
+  // display needs nothing" and "the payload did not arrive intact" are the same message on the
+  // wire, and the second one is not rare: buildPlaylistPayload() yields `assignments: []` for a
+  // device between playlists, for a playlist that has never been published, AND for a
+  // published_snapshot that fails to JSON.parse. Honouring it deleted every byte of media the panel
+  // held — reproduced here: three cached assets, one empty payload, cache emptied — which is only
+  // survivable while the uplink is up, i.e. exactly when this cache does not matter. A cache that is
+  // kept too long costs disk the quota reclaims anyway; one dropped at the wrong moment is a dark
+  // screen with no way back.
   if (data.prune && data.urls.length > 0) {
     prefetchChain = prefetchChain.then(() => pruneToPlaylist(data.urls)).catch(() => {});
   }

server/test/player-sw-scope.test.js

@@ -70,12 +70,12 @@ test('the worker prunes to the set the player declares', () => {
   // a panel with a 1GB widget quota, a few replaced videos is the whole budget.
   const sw = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8');
   assert.match(sw, /function pruneToPlaylist/);
-  // The guard is `data.prune && data.urls.length > 0`, not a bare `data.prune`: an EMPTY list is
-  // never a prune instruction. `assignments: []` is what the server sends for a device between
-  // playlists and from the catch when a snapshot fails to parse, and honouring it as "keep nothing"
-  // wiped a panel's whole offline library (fixed in 1.9.30).
+  // Driven by the player declaring a complete set — and an EMPTY set is not a declaration. See
+  // test/sw-prune-guard.test.js, which runs the handler: `assignments: []` is what the server sends
+  // for a device between playlists and for a snapshot that failed to parse, and honouring it as
+  // "keep nothing" wiped the panel's whole offline library.
   assert.match(sw, /if \(data\.prune && data\.urls\.length > 0\)/,
-    'the prune must require a NON-EMPTY declared set');
+    'the prune must be driven by the player declaring a NON-EMPTY complete set');
 
   const html = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8');
   assert.match(html, /prune:\s*true/);

server/test/player-volume-command.test.js

@@ -0,0 +1,182 @@
+'use strict';
+
+// The web player's set_volume handler, and the mute state it is not allowed to touch.
+//
+// Two bugs lived here at once, and the second is why fixing the first alone would have been worse
+// than leaving it broken:
+//
+//   1. THE KEY. The dashboard sends `{ level: <0..1> }` (device-detail.js: slider/100) and the
+//      Android player reads exactly that (`optDouble("level")`). The web player read
+//      `payload.value`. Nothing in the product sends `value`, so the slider was a silent no-op on
+//      every browser panel — and a no-op is invisible: the command is delivered, acked, and does
+//      nothing.
+//   2. THE SCALE. The local was named `pct` and divided by 100. Had only the key been corrected,
+//      `level: 0.5` would have become 0.5% — near-silence that LOOKS like the fix worked, because
+//      the handler now runs and the element's volume genuinely changes.
+//
+// And separately: setMediaVolume() wrote `el.muted = (v === 0)`, so any non-zero volume command
+// un-muted whatever was playing — defeating a per-item mute an operator had set on purpose
+// (reproduced in a browser: item flagged muted, one set_volume, muted went false), and defying the
+// autoplay rule in lib/media-mute.js, where unmuting without a gesture costs the video.
+//
+// The real functions are extracted from index.html and run against fake globals, so this asserts
+// what the shipped player does rather than a paraphrase of it.
+
+const { test } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const vm = require('node:vm');
+
+const HTML = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8');
+
+/** Extract a top-level `function name(...) { ... }` from the player by brace matching. */
+function extract(name) {
+  const start = HTML.indexOf(`    function ${name}(`);
+  assert.notEqual(start, -1, `${name}() must exist in the player`);
+  let depth = 0;
+  for (let i = HTML.indexOf('{', start); i < HTML.length; i++) {
+    if (HTML[i] === '{') depth++;
+    else if (HTML[i] === '}' && --depth === 0) return HTML.slice(start, i + 1);
+  }
+  throw new Error(`unterminated ${name}()`);
+}
+
+function loadVolumeParser() {
+  const sandbox = { console: { log() {}, warn() {} } };
+  vm.createContext(sandbox);
+  vm.runInContext(extract('volumeLevelFromCommand'), sandbox);
+  return (data) => vm.runInContext('volumeLevelFromCommand', sandbox)(data);
+}
+
+/**
+ * setMediaVolume() against fake media elements. Returns the elements so the test can see exactly
+ * which properties were written.
+ */
+function loadSetVolume({ wallFollower = false, elements } = {}) {
+  const els = elements || [
+    { tagName: 'VIDEO', volume: 1, muted: false },
+    { tagName: 'VIDEO', volume: 1, muted: true },
+  ];
+  const sandbox = {
+    console: { log() {}, warn() {} },
+    document: { querySelectorAll: () => els },
+    isWallFollower: () => wallFollower,
+    mediaVolume: null,
+  };
+  sandbox.window = sandbox;
+  vm.createContext(sandbox);
+  vm.runInContext('let mediaVolume = null;\n' + extract('setMediaVolume'), sandbox);
+  return { els, call: (v) => vm.runInContext('setMediaVolume', sandbox)(v), sandbox };
+}
+
+// ---------------------------------------------------------------------------------------------
+// THE WIRE FORM
+
+test('THE BUG: the key the dashboard actually sends is honoured', () => {
+  const parse = loadVolumeParser();
+  assert.equal(parse({ type: 'set_volume', payload: { level: 0.5 } }), 0.5);
+});
+
+test('THE OTHER HALF: a fraction is not divided by 100', () => {
+  // The trap. 0.5 -> 0.005 is near-mute and would read as "fixed" to anyone checking that the
+  // handler fires at all.
+  const parse = loadVolumeParser();
+  assert.equal(parse({ type: 'set_volume', payload: { level: 0.5 } }), 0.5);
+  assert.equal(parse({ type: 'set_volume', payload: { level: 1 } }), 1);
+  assert.equal(parse({ type: 'set_volume', payload: { level: 0 } }), 0);
+});
+
+test('the legacy percentage key is still understood, as a percentage', () => {
+  const parse = loadVolumeParser();
+  assert.equal(parse({ type: 'set_volume', payload: { value: 80 } }), 0.8);
+  assert.equal(parse({ type: 'set_volume', value: 25 }), 0.25);   // top-level, as the old code read it
+});
+
+test('the scale comes from the KEY, never from the magnitude', () => {
+  // 1 is legal in both conventions — full volume as a fraction, 1% as a percentage. A magnitude
+  // heuristic has to be wrong for one of them, so there is no heuristic.
+  const parse = loadVolumeParser();
+  assert.equal(parse({ payload: { level: 1 } }), 1, 'level:1 is FULL volume');
+  assert.equal(parse({ payload: { value: 1 } }), 0.01, 'value:1 is one percent');
+});
+
+test('level wins when a caller sends both', () => {
+  const parse = loadVolumeParser();
+  assert.equal(parse({ payload: { level: 0.3, value: 90 } }), 0.3);
+});
+
+test('out-of-range input is clamped, not wrapped', () => {
+  const parse = loadVolumeParser();
+  assert.equal(parse({ payload: { level: 4 } }), 1);
+  assert.equal(parse({ payload: { level: -2 } }), 0);
+  assert.equal(parse({ payload: { value: 5000 } }), 1);
+});
+
+test('a command carrying no level is refused rather than turned into 0', () => {
+  // Returning 0 for "no value" would silence a display on a malformed command — a failure that
+  // looks exactly like someone dragging the slider down.
+  const parse = loadVolumeParser();
+  for (const bad of [{}, { payload: {} }, { payload: { level: null } }, { payload: { level: '' } },
+    { payload: { level: 'loud' } }, { payload: { level: true } }, null, undefined]) {
+    assert.equal(parse(bad), null, `expected null for ${JSON.stringify(bad)}`);
+  }
+});
+
+test('numeric strings are accepted — an integrator posting JSON as text still works', () => {
+  const parse = loadVolumeParser();
+  assert.equal(parse({ payload: { level: '0.4' } }), 0.4);
+  assert.equal(parse({ payload: { value: '70' } }), 0.7);
+});
+
+// ---------------------------------------------------------------------------------------------
+// WHAT IT IS NOT ALLOWED TO TOUCH
+
+test('THE BUG: a volume command does not un-mute a deliberately muted item', () => {
+  const { els, call } = loadSetVolume();
+  els[1].muted = true;                        // an operator silenced this item
+  call(0.6);
+  assert.equal(els[1].volume, 0.6, 'the level still applies');
+  assert.equal(els[1].muted, true, 'a muted element must stay muted');
+  assert.equal(els[0].muted, false, 'and an unmuted one must stay unmuted');
+});
+
+test('volume 0 does not need to set muted — the level alone is silence', () => {
+  const { els, call } = loadSetVolume();
+  call(0);
+  assert.equal(els[0].volume, 0);
+  assert.equal(els[0].muted, false, 'mute is not this function\'s decision to make');
+});
+
+test('a wall follower is left silent — volume never reaches its elements', () => {
+  // One audio source per wall. A follower that honoured the slider would give the room the same
+  // track from every panel, a few milliseconds apart.
+  const { els, call } = loadSetVolume({ wallFollower: true });
+  call(0.9);
+  assert.equal(els[0].volume, 1, 'untouched');
+});
+
+test('the level is remembered so it survives the next item', () => {
+  const { call, sandbox } = loadSetVolume();
+  call(0.35);
+  assert.equal(vm.runInContext('mediaVolume', sandbox), 0.35);
+});
+
+test('an element torn down mid-call cannot break the loop', () => {
+  const boom = { tagName: 'VIDEO', set volume(v) { throw new Error('detached'); }, muted: false };
+  const good = { tagName: 'VIDEO', volume: 1, muted: false };
+  const { call } = loadSetVolume({ elements: [boom, good] });
+  call(0.5);
+  assert.equal(good.volume, 0.5, 'the surviving element still gets the level');
+});
+
+// ---------------------------------------------------------------------------------------------
+// THE HANDLER WIRING — the parser is useless if the socket handler does not call it
+
+test('the set_volume handler routes through the parser and setMediaVolume', () => {
+  const handler = HTML.slice(HTML.indexOf("if (data.type === 'set_volume')"));
+  const block = handler.slice(0, handler.indexOf('\n        }') + 10);
+  assert.match(block, /volumeLevelFromCommand\(data\)/, 'the handler must use the shared parser');
+  assert.match(block, /setMediaVolume\(/, 'and apply it');
+  assert.ok(!/\/\s*100/.test(block), 'the handler must not re-scale — the parser already returns 0..1');
+});

server/test/player-widget-frame-origin.test.js

@@ -0,0 +1,56 @@
+'use strict';
+
+// The widget iframe's origin, and what it costs.
+//
+// The player mounts a widget in an iframe sandboxed to `allow-scripts` with NO allow-same-origin,
+// which gives it an OPAQUE origin: widget scripts cannot read the player's window, its localStorage,
+// or its device token. That is deliberate and worth keeping — widget HTML is operator-authored and
+//, for the webpage widget, third-party.
+//
+// It has a consequence that is easy to forget and was in fact forgotten: a service worker does not
+// control opaque-origin clients, so the widget frame's navigation NEVER reaches sw.js. The
+// cache-first widget branch there is real code that this player's own widgets do not use — measured
+// in Chrome, a clock widget mounted five times over 25 seconds while the shell cache held zero
+// widget entries, and a plain fetch() of the same URL from the controlled page was cached
+// immediately. Widgets survive an outage today on the HTTP cache and the server's immutable
+// Cache-Control, not on the worker.
+//
+// So this test pins the security property, and pins the fact that the offline story for widgets
+// rests on the HTTP header. Someone who "fixes" offline widgets by adding allow-same-origin trades
+// the isolation for a cache — the wrong direction, and the reason this is asserted rather than left
+// as a comment.
+
+const { test } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+
+const HTML = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8');
+const SW = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8');
+const WIDGETS = fs.readFileSync(path.join(__dirname, '..', 'routes', 'widgets.js'), 'utf8');
+
+test('the widget iframe is sandboxed into an opaque origin', () => {
+  const sandboxes = [...HTML.matchAll(/setAttribute\('sandbox',\s*'([^']*)'\)/g)].map((m) => m[1]);
+  assert.ok(sandboxes.length > 0, 'the player must sandbox its widget frames');
+  for (const s of sandboxes) {
+    assert.match(s, /allow-scripts/, 'a widget needs scripts to be a widget');
+    assert.doesNotMatch(s, /allow-same-origin/,
+      'allow-same-origin would give widget scripts the player origin — its storage, its device token');
+  }
+});
+
+test('the offline guarantee for widgets is the HTTP header, and the server still sets it', () => {
+  // If this regresses to no-store, widgets stop surviving an outage everywhere — and the service
+  // worker will NOT quietly cover for it, because it never sees the request.
+  assert.match(WIDGETS, /max-age=31536000, immutable/,
+    'a rev-pinned render must stay hard-cacheable: it is the only thing holding widgets offline');
+  assert.match(WIDGETS, /no-store/, 'a render with no rev must stay uncacheable — nothing distinguishes one from the next');
+});
+
+test('the worker does not claim to be what keeps widgets offline', () => {
+  // The comment above that branch used to say it was. A worker cannot control an opaque-origin
+  // client, so the claim was false in exactly the deployment it was written for.
+  const branch = SW.slice(0, SW.indexOf("url.pathname.startsWith('/api/widgets/')"));
+  assert.match(branch, /opaque-origin|opaque origin/i,
+    'sw.js must record that the widget frame is opaque-origin and bypasses it');
+});