src() called as a getter should return the video src #968
Conversation
|
Here's my understanding: videoElement.src returns the value of the src attribute on the video tag. This may be different from videoElement.currentSrc which returns the selected source, whether provided by the attribute or source tags. i.e. if only source tags are used, videoElement.src returns nothing. It never really made sense to provide a src() getter because only source tags have been used with video.js (the src attribute on the video tag should work, but doesn't yet). Which means src() would always return nothing if following the spec. So currentSrc() has been the appropriate getter. Can you go deeper into the use case where currentSrc is failing? It sounds like something is setting src(), and then something different would be immediately getting currentSrc(), and while it should be fast, the async nature of src -> currentSrc is causing an issue. techGet('src') works well in your change because we're relying on the src attribute in the underlying video element. If we decided to pass through the source tags to the underlying video element for some reason, this method would break. That may never happen, but it's worth noting. |
| @@ -1069,7 +1073,7 @@ vjs.Player.prototype.src = function(source){ | |||
| } | |||
| } | |||
| } | |||
| return this; | |||
| return source; | |||
There was a problem hiding this comment.
This return will always happen after a setter now that you've added code for a getter. All other video.js API setters return this for chaining. Is there a reason this needs to return the source value?
There was a problem hiding this comment.
I updated this return to match the behavior of the src property on the video element. As long as the getter behavior is available, keeping the original behavior of the setter for consistency would be ok with me.
There was a problem hiding this comment.
Ok cool, let's keep it returning this here then.
Any more details on this? |
|
I was working on the HLS plugin and wanted it to conditionally activate if the video source looked like an m3u8. Here's a simplified example. I didn't notice the lack of |
|
The source attribute on the video tag isn't really supported anyway (something we should fix). In this modified example I just switched the embed code to use source tags, and the example works. Would that cover the issue you were having, or no? |
|
Nvm, reread your last comment that said using source child elements would break the strategy. I'm interested to know the use case where you'd set the src attribute and not call the src() API method. Either way it sounds like making the src attribute work would be the best first step here? |
| src; | ||
| src = player.src(); | ||
|
|
||
| notEqual(src, player, 'the player is not returned'); |
There was a problem hiding this comment.
@dmlap just a heads up, this doesn't actually test anything except that the stub is working. :) Actually Flash would break for this test if it wasn't mocked, because flash.src doesn't return anything. Fixing it now and pulling this in.
Made the src method return the player when setting
It can be handy that src() returns the player object when it is invoked but it does not match the behavior of the corresponding property on the video element. Ignoring the spec however, while the video element is running the resource selection algorithm, currentSrc may be undefined. If the video source has been specified through an attribute on the video element, src() is the natural way to expose that URL programmatically. Without this change, it's necessary to bypass the player and interact with the tech directly to determine the value of the src attribute.