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

Restore video tag attributes on tech change #1369

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dcharbonnier

dcharbonnier commented Jul 25, 2014

New attempt to fix this issue #1367 related to the webkit-playsinline attribute missing.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 25, 2014

Member

Thanks for this! Digging in now.

Member

heff commented Jul 25, 2014

Thanks for this! Digging in now.

Show outdated Hide outdated src/js/lib.js
* @private
*/
vjs.setElementAttributes = function(el, attributes){
var keys = Object.keys(attributes);

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

I believe Object.keys will error in ie8. You can use vjs.obj.each instead.

@heff

heff Jul 25, 2014

Member

I believe Object.keys will error in ie8. You can use vjs.obj.each instead.

* @param {Object=} attributes Element attributes to be applied.
* @private
*/
vjs.setElementAttributes = function(el, attributes){

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

The name should match the existing getAttributeValues in the same file for API consistency. I'd be fine with changing that to getElementAttributes since it's not an exported function yet.

@heff

heff Jul 25, 2014

Member

The name should match the existing getAttributeValues in the same file for API consistency. I'd be fine with changing that to getElementAttributes since it's not an exported function yet.

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 25, 2014

changed to getElementAttributes

@dcharbonnier

dcharbonnier Jul 25, 2014

changed to getElementAttributes

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

I don't wanted to do that because the two methods are not symmetrical but I will change that

Ok that's understandable. What are the unsymmetrical pieces?

@heff

heff Jul 25, 2014

Member

I don't wanted to do that because the two methods are not symmetrical but I will change that

Ok that's understandable. What are the unsymmetrical pieces?

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 25, 2014

business logic on getAttributeValues : knownBooleans = ','+'autoplay,controls,loop,muted,default'+',';

setElementAttributes is 'html oriented', you have a dom element you want to update the attributes according to a dict. Nothing related to videojs.
BTW I already changed it but can revert the change and if we keep this change I should put the set and get methods on the same place, now the setElementAttributes is near the createEl

@dcharbonnier

dcharbonnier Jul 25, 2014

business logic on getAttributeValues : knownBooleans = ','+'autoplay,controls,loop,muted,default'+',';

setElementAttributes is 'html oriented', you have a dom element you want to update the attributes according to a dict. Nothing related to videojs.
BTW I already changed it but can revert the change and if we keep this change I should put the set and get methods on the same place, now the setElementAttributes is near the createEl

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

Yeah, that's true. There's some overlap because of the translation from empty string to boolean, so I prefer they stay consistent here. Also I'm wondering if we couldn't remove the knownBooleans and switch every empty string value to a boolean. That might take a little research so not something we need to deal with in this pull request.

@heff

heff Jul 25, 2014

Member

Yeah, that's true. There's some overlap because of the translation from empty string to boolean, so I prefer they stay consistent here. Also I'm wondering if we couldn't remove the knownBooleans and switch every empty string value to a boolean. That might take a little research so not something we need to deal with in this pull request.

Show outdated Hide outdated src/js/media/html5.js
if (player.options_[attr] !== null) {
el[attr] = player.options_[attr];
var attributes = {};
if (typeof player.options_[attr] !== 'undefined') {

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

You're already checking for undefined in setElementAttributes, so could we simplify this?

@heff

heff Jul 25, 2014

Member

You're already checking for undefined in setElementAttributes, so could we simplify this?

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 25, 2014

no, this is for an overwrite of the tag initial values

<video autoplay></video>
videojs('',{autoplay:false});

we don't want to remove the autoplay if there is no autoplay defined on the player options I think. Please confirm.

@dcharbonnier

dcharbonnier Jul 25, 2014

no, this is for an overwrite of the tag initial values

<video autoplay></video>
videojs('',{autoplay:false});

we don't want to remove the autoplay if there is no autoplay defined on the player options I think. Please confirm.

Show outdated Hide outdated src/js/media/html5.js
@@ -95,9 +98,11 @@ vjs.Html5.prototype.createEl = function(){
var attrs = ['autoplay','preload','loop','muted'];
for (var i = attrs.length - 1; i >= 0; i--) {
var attr = attrs[i];
if (player.options_[attr] !== null) {
el[attr] = player.options_[attr];
var attributes = {};

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

We have attrs, attr, and attributes all in the same place now. We should see if that could be simplified or at least get more specific with the var naming.

@heff

heff Jul 25, 2014

Member

We have attrs, attr, and attributes all in the same place now. We should see if that could be simplified or at least get more specific with the var naming.

Show outdated Hide outdated src/js/media/html5.js
el[attr] = player.options_[attr];
var attributes = {};
if (typeof player.options_[attr] !== 'undefined') {
attributes[attr]=player.options_[attr];

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

We need spaces around the equals here for consistent style.

@heff

heff Jul 25, 2014

Member

We need spaces around the equals here for consistent style.

Show outdated Hide outdated src/js/player.js
@@ -32,6 +32,8 @@ vjs.Player = vjs.Component.extend({
init: function(tag, options, ready){
this.tag = tag; // Store the original tag used to set options
this.tagAttributes = (tag) ? vjs.getAttributeValues(tag) : {}; // Store the tag attributes used to restore html5 tech

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

Would it work to store this later in the file where we already run getAttributeValues?
https://github.com/dcharbonnier/video.js/blob/feature/video-attr-restore/src/js/player.js#L134

@heff

heff Jul 25, 2014

Member

Would it work to store this later in the file where we already run getAttributeValues?
https://github.com/dcharbonnier/video.js/blob/feature/video-attr-restore/src/js/player.js#L134

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 26, 2014

Let me try, that can solve two concerns but the code coverage looks quite low on this project so I'm afraid of such changes.

@dcharbonnier

dcharbonnier Jul 26, 2014

Let me try, that can solve two concerns but the code coverage looks quite low on this project so I'm afraid of such changes.

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 26, 2014

getTagSettings is a static method (as much as javascript permit it), accessing an attribute of the instance on this method seams strange to me, even if the reality is that we only use this method on the constructor. I don't think it's wrong but to make it right this imply more work, rename the method, remove the arguments, do you have a convention for "private" methods ?

@dcharbonnier

dcharbonnier Jul 26, 2014

getTagSettings is a static method (as much as javascript permit it), accessing an attribute of the instance on this method seams strange to me, even if the reality is that we only use this method on the constructor. I don't think it's wrong but to make it right this imply more work, rename the method, remove the arguments, do you have a convention for "private" methods ?

@@ -152,6 +152,22 @@ test('should read tag attributes from elements, including HTML5 in all browsers'
equal(trackVals['title'], 'test');
});
test('should set tag attributes from object', function(){

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

Thanks for writing tests!

@heff

heff Jul 25, 2014

Member

Thanks for writing tests!

fixture.innerHTML += html;
var tag = document.getElementById('example_1');
var player = new videojs.Player(tag, {techOrder:['html5']});

This comment has been minimized.

@heff

heff Jul 25, 2014

Member

Have you tried running this in ie8? I think this is going to throw errors. We may need to find a smaller unit of code to test here instead of requiring the full html5 stack to run. I think you could just write a test for vjs.Html5.prototype.createEl.

@heff

heff Jul 25, 2014

Member

Have you tried running this in ie8? I think this is going to throw errors. We may need to find a smaller unit of code to test here instead of requiring the full html5 stack to run. I think you could just write a test for vjs.Html5.prototype.createEl.

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 25, 2014

I prefer to die than using ie8. What make you think it's gone to fail ?
Very hard to write the right test case. This test presume too much of the switching tech method but in the other hand testing only the createElement presume too much of the player and tech constructors...

@dcharbonnier

dcharbonnier Jul 25, 2014

I prefer to die than using ie8. What make you think it's gone to fail ?
Very hard to write the right test case. This test presume too much of the switching tech method but in the other hand testing only the createElement presume too much of the player and tech constructors...

This comment has been minimized.

@heff

heff Jul 26, 2014

Member

Ha, yeah ie8 is no fun, but that just means I end up doing IE8 testing for everyone. :) It could possibly get past IE8 if we have enough checks for missing properties, but I'm still cautious about the long term stability of executing the html5 code in a browser that isn't mean to support it.

In issues like this I try to break it into smaller units to test, and then that makes it easier to stub certain functionality. If you think the switching tech method is an issue, we should make sure we have other tests for that specifically. So one test for vjs.Html5.prototype.createEl and another to make sure switching works as expected.

@heff

heff Jul 26, 2014

Member

Ha, yeah ie8 is no fun, but that just means I end up doing IE8 testing for everyone. :) It could possibly get past IE8 if we have enough checks for missing properties, but I'm still cautious about the long term stability of executing the html5 code in a browser that isn't mean to support it.

In issues like this I try to break it into smaller units to test, and then that makes it easier to stub certain functionality. If you think the switching tech method is an issue, we should make sure we have other tests for that specifically. So one test for vjs.Html5.prototype.createEl and another to make sure switching works as expected.

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 26, 2014

I develop backends or exotic devices, I will tell you if videojs work on chromecast, embed ios, android, xbox one, wii-u, amazon-fire-tv, samsung tv, … and you tell me if it work on ie8 please ? I can't spend hours to setup a ie8… For the createEl I still don't get it, I agree but my knowledge of videojs is too limited for that. We can't test createEl if we don't have a Html5 tech object and we can't create one without the player object, can you drive to the right direction ?

@dcharbonnier

dcharbonnier Jul 26, 2014

I develop backends or exotic devices, I will tell you if videojs work on chromecast, embed ios, android, xbox one, wii-u, amazon-fire-tv, samsung tv, … and you tell me if it work on ie8 please ? I can't spend hours to setup a ie8… For the createEl I still don't get it, I agree but my knowledge of videojs is too limited for that. We can't test createEl if we don't have a Html5 tech object and we can't create one without the player object, can you drive to the right direction ?

This comment has been minimized.

@gkatsev

gkatsev Jul 26, 2014

Member

For better or worse, videojs supports IE8, which means that any code we accept into the project needs to support IE8.
You should be able to test createEl more directly by basically creating a context object and then calling createEl with it, specifically, a player object that has various properties that createEl needs. Something along these lines:

var newEl = vjs.Html5.prototype.createEl.call({
  player: {
    tag: document.createElement('div')
  }
});
@gkatsev

gkatsev Jul 26, 2014

Member

For better or worse, videojs supports IE8, which means that any code we accept into the project needs to support IE8.
You should be able to test createEl more directly by basically creating a context object and then calling createEl with it, specifically, a player object that has various properties that createEl needs. Something along these lines:

var newEl = vjs.Html5.prototype.createEl.call({
  player: {
    tag: document.createElement('div')
  }
});

This comment has been minimized.

@dcharbonnier

dcharbonnier Jul 28, 2014

sadly this will test nothing because the element will not be created if it already exist.

@dcharbonnier

dcharbonnier Jul 28, 2014

sadly this will test nothing because the element will not be created if it already exist.

This comment has been minimized.

@gkatsev

gkatsev Jul 28, 2014

Member

This was just a quick example, didn't mean to imply it is exactly what you need to be able to test just createEl.

@gkatsev

gkatsev Jul 28, 2014

Member

This was just a quick example, didn't mean to imply it is exactly what you need to be able to test just createEl.

@heff heff closed this in 527a33a Aug 4, 2014

heff added a commit that referenced this pull request Aug 4, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Aug 4, 2014

Member

@dcharbonnier I wanted to get this merged in for tomorrow's release, but check out this change log and let me know what you think.
527a33a

Member

heff commented Aug 4, 2014

@dcharbonnier I wanted to get this merged in for tomorrow's release, but check out this change log and let me know what you think.
527a33a

@dcharbonnier

This comment has been minimized.

Show comment
Hide comment
@dcharbonnier

dcharbonnier Aug 5, 2014

good for me, thanks

dcharbonnier commented Aug 5, 2014

good for me, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment