Skip to content

fix: make media bindings more robust #12206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a rAF loop is right here because media should play when off tab, but the rAF will not run at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fine because once you switch back to the tab it continues to loop, and while you're off tab whatever the binding update causes wouldn't be of interest anyway

}

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>
Loading