Skip to content

Commit

Permalink
fix: make media bindings more robust
Browse files Browse the repository at this point in the history
The media bindings where fragile because the "prevent rerun if update in progress"-logic was flawed: It didn't (re)set the `updating` flag correctly, because it assumed an event and a render effect would always directly follow each other, with one always being first - but that's not true.
It's much more robust to instead compare the value with what's currently present in the DOM. For the very fast-firing current-time-binding a variable is used to not invoke the DOM getter as much, for the others this is not necessary.
Lastly, the playback-rate-binding contained another bug where the listener was setup inside the effect and therefore reinitialized on each binding change - it needs to move into a different effect.
  • Loading branch information
dummdidumm committed Jun 27, 2024
1 parent 8904e0f commit 7b5efdd
Show file tree
Hide file tree
Showing 9 changed files with 135 additions and 34 deletions.
5 changes: 5 additions & 0 deletions .changeset/popular-feet-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: make media bindings more robust
58 changes: 24 additions & 34 deletions packages/svelte/src/internal/client/dom/elements/bindings/media.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ function time_ranges_to_array(ranges) {
export function bind_current_time(media, get_value, update) {
/** @type {number} */
var raf_id;
var updating = false;
/** @type {number} */
var value;

// Ideally, listening to timeupdate would be enough, but it fires too infrequently for the currentTime
// binding, which is why we use a raf loop, too. We additionally still listen to timeupdate because
Expand All @@ -34,22 +35,21 @@ export function bind_current_time(media, get_value, update) {
raf_id = requestAnimationFrame(callback);
}

updating = true;
update(media.currentTime);
var next_value = media.currentTime;
if (value !== next_value) {
update((value = next_value));
}
};

raf_id = requestAnimationFrame(callback);
media.addEventListener('timeupdate', callback);

render_effect(() => {
var value = get_value();
var next_value = Number(get_value());

// through isNaN we also allow number strings, which is more robust
if (!updating && !isNaN(/** @type {any} */ (value))) {
media.currentTime = /** @type {number} */ (value);
if (value !== next_value && !isNaN(/** @type {any} */ (next_value))) {
media.currentTime = value = next_value;
}

updating = false;
});

teardown(() => cancelAnimationFrame(raf_id));
Expand Down Expand Up @@ -113,22 +113,21 @@ export function bind_ready_state(media, update) {
* @param {(playback_rate: number) => void} update
*/
export function bind_playback_rate(media, get_value, update) {
var updating = false;

// Needs to happen after the element is inserted into the dom, else playback will be set back to 1 by the browser.
// For hydration we could do it immediately but the additional code is not worth the lost microtask.
// Needs to happen after element is inserted into the dom (which is guaranteed by using effect),
// else playback will be set back to 1 by the browser
effect(() => {
var value = get_value();
var value = Number(get_value());

// through isNaN we also allow number strings, which is more robust
if (!isNaN(/** @type {any} */ (value)) && value !== media.playbackRate) {
updating = true;
media.playbackRate = /** @type {number} */ (value);
if (value !== media.playbackRate && !isNaN(value)) {
media.playbackRate = value;
}
});

// Start listening to ratechange events after the element is inserted into the dom,
// else playback will be set to 1 by the browser
effect(() => {
listen(media, ['ratechange'], () => {
if (!updating) update(media.playbackRate);
updating = false;
update(media.playbackRate);
});
});
}
Expand Down Expand Up @@ -200,9 +199,7 @@ export function bind_paused(media, get_value, update) {
* @param {(volume: number) => void} update
*/
export function bind_volume(media, get_value, update) {
var updating = false;
var callback = () => {
updating = true;
update(media.volume);
};

Expand All @@ -213,14 +210,11 @@ export function bind_volume(media, get_value, update) {
listen(media, ['volumechange'], callback, false);

render_effect(() => {
var value = get_value();
var value = Number(get_value());

// through isNaN we also allow number strings, which is more robust
if (!updating && !isNaN(/** @type {any} */ (value))) {
media.volume = /** @type {number} */ (value);
if (value !== media.volume && !isNaN(value)) {
media.volume = value;
}

updating = false;
});
}

Expand All @@ -230,10 +224,7 @@ export function bind_volume(media, get_value, update) {
* @param {(muted: boolean) => void} update
*/
export function bind_muted(media, get_value, update) {
var updating = false;

var callback = () => {
updating = true;
update(media.muted);
};

Expand All @@ -244,9 +235,8 @@ export function bind_muted(media, get_value, update) {
listen(media, ['volumechange'], callback, false);

render_effect(() => {
var value = get_value();
var value = !!get_value();

if (!updating) media.muted = !!value;
updating = false;
if (media.muted !== value) media.muted = value;
});
}
1 change: 1 addition & 0 deletions packages/svelte/tests/runtime-browser/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export function equal(a, b, message) {
/**
* @param {any} condition
* @param {string} [message]
* @returns {asserts condition}
*/
export function ok(condition, message) {
if (!condition) throw new Error(message || `Expected ${condition} to be truthy`);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { test, ok } from '../../assert';

export default test({
mode: ['client'],
async test({ assert, target }) {
const audio = target.querySelector('audio');
const button = target.querySelector('button');
ok(audio);

assert.equal(audio.muted, false);

audio.muted = true;
audio.dispatchEvent(new CustomEvent('volumechange'));
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.muted, true, 'event');

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.muted, false, 'click 1');

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.muted, true, 'click 2');

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.muted, false, 'click 3');
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let muted = $state(false);
</script>

<audio bind:muted></audio>
<button onclick={() => (muted = !muted)}>toggle</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { test, ok } from '../../assert';

export default test({
mode: ['client'],
async test({ assert, target }) {
const audio = target.querySelector('audio');
const button = target.querySelector('button');
ok(audio);

assert.equal(audio.playbackRate, 0.5);

audio.playbackRate = 1.0;
audio.dispatchEvent(new CustomEvent('ratechange'));
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.playbackRate, 1.0);

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.playbackRate, 2);

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.playbackRate, 3);

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.playbackRate, 4);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let playbackRate = $state(0.5);
</script>

<audio bind:playbackRate></audio>
<button onclick={() => (playbackRate += 1)}>increment</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { test, ok } from '../../assert';

export default test({
mode: ['client'],
async test({ assert, target }) {
const audio = target.querySelector('audio');
const button = target.querySelector('button');
ok(audio);

assert.equal(audio.volume, 0.1);

audio.volume = 0.2;
audio.dispatchEvent(new CustomEvent('volumechange'));
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.volume, 0.2);

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.volume, 0.2 + 0.1); // JavaScript can't add floating point numbers correctly

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.volume, 0.2 + 0.1 + 0.1);

button?.click();
await new Promise((r) => setTimeout(r, 100));
assert.equal(audio.volume, 0.2 + 0.1 + 0.1 + 0.1);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let volume = $state(0.1);
</script>

<audio bind:volume></audio>
<button onclick={() => (volume += 0.1)}>increment</button>

0 comments on commit 7b5efdd

Please sign in to comment.