Skip to content
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

Add more bindings for media element properties #3650

Merged
merged 4 commits into from
Nov 14, 2019

Conversation

MattiasBuelens
Copy link
Contributor

This adds read-only bindings for seeking and ended to <audio> and <video> elements.

These can be used to build a custom video UI, similar to the media elements example. For example:

  • The ended property can be used to show a replay button instead of a regular play/pause button:
<button on:click={() => paused = !paused}>
	{#if ended}
		Replay
	{:else if paused}
		Play
	{:else}
		Pause
	{/if}
</button>
  • The seeking property can be used to show a loading spinner while the media element is seeking.

I also noticed that the current compiler would generate code for bind:volume like this:

m(target, anchor) {
	insert(target, audio, anchor);

	audio.volume = ctx.volume;
}

However, if ctx.volume is still undefined, then this would throw a TypeError because it's not a number between 0 and 1 (spec).

To fix this, I've added an !isNaN(ctx.volume) check, so we don't set an invalid value when mounting. I've also refactored the code rendering for bindings a bit to make it easier to add such "mount conditions", similar to how we add "update conditions". 🙂

@MattiasBuelens
Copy link
Contributor Author

I also wanted to support videoWidth and videoHeight on <video> elements, but I ran into an issue with the events. A <video> element dispatches a resize event whenever these properties change, but this conflicts with Svelte's own resize event.

So my question: what should the following do?

<video on:resize={handleResize}/>

Should handleResize be called when videoWidth and/or videoHeight change? Or when the video element's actual width and/or height change? 😕

@MattiasBuelens MattiasBuelens force-pushed the more-media-bindings branch 2 times, most recently from 0d057dc to ee30d65 Compare October 27, 2019 22:46
@Rich-Harris Rich-Harris merged commit 004faf6 into sveltejs:master Nov 14, 2019
@Rich-Harris
Copy link
Member

Apologies for the delay — this is excellent, thank you.

I'm not sure I understand about the resize event — do you mean that it conflicts with the add_resize_listener stuff (which adds a listener to an <object> inside an element)? It should be possible for us to support both. Listening for the video's own resize event should definitely be equivalent to video.onresize = handlervideoWidth/videoHeight bindings would be pretty cool. We should probably have a separate issue for that.

@MattiasBuelens MattiasBuelens deleted the more-media-bindings branch November 14, 2019 23:27
@MattiasBuelens
Copy link
Contributor Author

Yes, I thought it could conflict with the resize event used for the dimension bindings. But now that I've looked at it more closely, I realized that this resize event is not accessible for Svelte components: the add_resize_listener helper will never be used for on: event handlers, since those will always uses @listen.

So the resize event for the dimension bindings is completely internal to add_bindings. It's only used to define the binding and then handled it. The event name doesn't matter, as long as it doesn't conflict with any other events used by other binding definitions.

I think the simplest fix would be:

  • Change the event name for the dimension bindings from resize to e.g. dimensionchange or internal:resize. The name doesn't matter, since it never shows up in the generated code.
  • Add a binding for videoWidth and videoHeight that is triggered on a resize event, which can now no longer be confused with the internal event for dimension bindings.

For example:

<video bind:videoWidth bind:offsetWidth on:resize="fn">

should:

  • use listen(video, 'resize', ...) for the videoWidth binding
  • use add_resize_listener(...) for the offsetWidth binding
  • use listen(video, 'resize', ...) again for the on:resize event handler

I'll whip up a PR. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants