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.

Sign up for GitHub

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

Jump to bottom

Restore video tag attributes on tech change #1369

Closed
wants to merge 13 commits into from
Closed

Restore video tag attributes on tech change #1369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4af8af0
add test for video tag attr conservation
dcharbonnier Jul 24, 2014
c4aa9fa
set attributes of video tag and not only values
dcharbonnier Jul 24, 2014
2537c98
add unsupported attribute to the video tag - test failing
dcharbonnier Jul 25, 2014
0e78c9e
helper to set attributes on an element from a map of values
dcharbonnier Jul 25, 2014
aeefefe
dummy compare of html content with a sort of the attributes
dcharbonnier Jul 25, 2014
dae47c6
ignore html attributes order for comparition
dcharbonnier Jul 25, 2014
998183f
save original tag attributes
dcharbonnier Jul 25, 2014
a443d21
restore original tag attributes n creation and overwrite if required …
dcharbonnier Jul 25, 2014
72bbabd
replace object.keys with vjs.obj.each for ie<9
dcharbonnier Jul 25, 2014
ca8313b
fix spacing
dcharbonnier Jul 25, 2014
b54f5e2
API consistency, getAttributeValues renamed to getElementAttributes
dcharbonnier Jul 25, 2014
1c5f928
clear variable naming
dcharbonnier Jul 25, 2014
0604746
move setElementAttributes close to getElementAttributes
dcharbonnier Jul 26, 2014
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
18 changes: 17 additions & 1 deletion src/js/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,22 @@ vjs.IS_CHROME = (/Chrome/i).test(vjs.USER_AGENT);

vjs.TOUCH_ENABLED = !!(('ontouchstart' in window) || window.DocumentTouch && document instanceof window.DocumentTouch);

/**
* Apply attributes to an HTML element.
* @param {Element} el Target element.
* @param {Object=} attributes Element attributes to be applied.
* @private
*/
vjs.setElementAttributes = function(el, attributes){
vjs.obj.each(attributes, function(attrName, attrValue) {
if (attrValue === null || typeof attrValue === 'undefined' || attrValue === false) {
el.removeAttribute(attrName);
} else {
el.setAttribute(attrName,attrValue === true ? '' : attrValue);
}
});
};

/**
* Get an element's attribute values, as defined on the HTML tag
* Attributs are not the same as properties. They're defined on the tag
Expand All @@ -400,7 +416,7 @@ vjs.TOUCH_ENABLED = !!(('ontouchstart' in window) || window.DocumentTouch && doc
* @return {Object}
* @private
*/
vjs.getAttributeValues = function(tag){
vjs.getElementAttributes = function(tag){
var obj, knownBooleans, attrs, attrName, attrVal;

obj = {};
Expand Down
23 changes: 14 additions & 9 deletions src/js/media/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,13 @@ vjs.Html5.prototype.createEl = function(){
el = clone;
player.tag = null;
} else {
el = vjs.createEl('video', {
id:player.id() + '_html5_api',
className:'vjs-tech'
});
el = vjs.createEl('video', {});
vjs.setElementAttributes(el,
vjs.obj.merge(player.tagAttributes||{}, {
id:player.id() + '_html5_api',
'class':'vjs-tech'
})
);
}
// associate the player with the new tag
el['player'] = player;
Expand All @@ -92,12 +95,14 @@ vjs.Html5.prototype.createEl = function(){
}

// Update specific tag settings, in case they were overridden
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 settingsAttrs = ['autoplay','preload','loop','muted'];
for (var i = settingsAttrs.length - 1; i >= 0; i--) {
var attr = settingsAttrs[i];
var overwriteAttrs = {};
if (typeof player.options_[attr] !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

overwriteAttrs[attr] = player.options_[attr];
}
vjs.setElementAttributes(el, overwriteAttrs);
}

return el;
Expand Down
8 changes: 5 additions & 3 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.getElementAttributes(tag) : {}; // Store the tag attributes used to restore html5 tech

// Make sure tag ID exists
tag.id = tag.id || 'vjs_video_' + vjs.guid++;

Expand Down Expand Up @@ -135,7 +137,7 @@ vjs.Player.prototype.getTagSettings = function(tag){
'tracks': []
};

vjs.obj.merge(options, vjs.getAttributeValues(tag));
vjs.obj.merge(options, vjs.getElementAttributes(tag));

// Get tag children settings
if (tag.hasChildNodes()) {
Expand All @@ -148,9 +150,9 @@ vjs.Player.prototype.getTagSettings = function(tag){
// Change case needed: http://ejohn.org/blog/nodename-case-sensitivity/
childName = child.nodeName.toLowerCase();
if (childName === 'source') {
options['sources'].push(vjs.getAttributeValues(child));
options['sources'].push(vjs.getElementAttributes(child));
} else if (childName === 'track') {
options['tracks'].push(vjs.getAttributeValues(child));
options['tracks'].push(vjs.getElementAttributes(child));
}
}
}
Expand Down
24 changes: 20 additions & 4 deletions test/unit/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ test('should read tag attributes from elements, including HTML5 in all browsers'

document.getElementById('qunit-fixture').innerHTML += tags;

var vid1Vals = vjs.getAttributeValues(document.getElementById('vid1'));
var vid2Vals = vjs.getAttributeValues(document.getElementById('vid2'));
var sourceVals = vjs.getAttributeValues(document.getElementById('source'));
var trackVals = vjs.getAttributeValues(document.getElementById('track'));
var vid1Vals = vjs.getElementAttributes(document.getElementById('vid1'));
var vid2Vals = vjs.getElementAttributes(document.getElementById('vid2'));
var sourceVals = vjs.getElementAttributes(document.getElementById('source'));
var trackVals = vjs.getElementAttributes(document.getElementById('track'));

// was using deepEqual, but ie8 would send all properties as attributes

Expand Down Expand Up @@ -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(){
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for writing tests!


var tags = '<video id="vid1" controls data-test="ok"></video>';

document.getElementById('qunit-fixture').innerHTML += tags;
var el = document.getElementById('vid1');
vjs.setElementAttributes(el, {controls: false,'data-test': 'asdf'});

var vid1Vals = vjs.getElementAttributes(document.getElementById('vid1'));

equal(vid1Vals['controls'], undefined);
equal(vid1Vals['id'], 'vid1');
equal(vid1Vals['data-test'], 'asdf');
});

test('should get the right style values for an element', function(){
var el = document.createElement('div');
var container = document.createElement('div');
Expand Down
31 changes: 30 additions & 1 deletion test/unit/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ test('should accept options from multiple sources and override in correct order'
});

test('should get tag, source, and track settings', function(){
// Partially tested in lib->getAttributeValues
// Partially tested in lib->getElementAttributes

var fixture = document.getElementById('qunit-fixture');

Expand Down Expand Up @@ -482,3 +482,32 @@ test('player should handle different error types', function(){
vjs.log.error.restore();
});

test('should restore all video tags attribute after a tech switch', function(){
var fixture = document.getElementById('qunit-fixture');
var html = '<video id="example_1" class="vjs-tech" preload="" webkit-playsinline="" autoplay=""></video>';
fixture.innerHTML += html;

var tag = document.getElementById('example_1');
var player = new videojs.Player(tag, {techOrder:['html5']});
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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')
  }
});

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

var techOptions = vjs.obj.merge({ 'source': '', 'parentEl': player.el_ }, player.options_['html5']);
vjs.Html5.disposeMediaElement(player.tag);
player.tag = null;
player.tech = new vjs.Html5(player, techOptions);

PlayerTest.htmlEqualWithSort(player.tech.el_.outerHTML, html.replace('example_1','example_1_html5_api'));
});

test('should restore all video tags attribute after a tech switch and keep options', function(){
var fixture = document.getElementById('qunit-fixture');
var html = '<video id="example_1" class="vjs-tech" preload="none" autoplay=""></video>';
fixture.innerHTML += html;

var tag = document.getElementById('example_1');
var player = new videojs.Player(tag, {techOrder:['html5'], autoplay:false});
var techOptions = vjs.obj.merge({ 'source': '', 'parentEl': player.el_ }, player.options_['html5']);
vjs.Html5.disposeMediaElement(player.tag);
player.tag = null;
player.tech = new vjs.Html5(player, techOptions);

PlayerTest.htmlEqualWithSort(player.tech.el_.outerHTML, html.replace('example_1','example_1_html5_api').replace(' autoplay=""',''));
});
11 changes: 11 additions & 0 deletions test/unit/test-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,16 @@ var PlayerTest = {
playerOptions['techOrder'] = ['mediaFaker'];

return player = new videojs.Player(videoTag, playerOptions);
},
htmlEqualWithSort : function(htmlResult,htmlExpected) {
function htmlTransform(str) {
str = str.replace(/[<|>]/g,' ');
str = str.trim();
str = str.replace(/\s{2,}/g, ' ');
return str.split(' ').sort().join(' ');
}
htmlResult= htmlResult.split(' ').sort().join(' ');
equal(htmlTransform(htmlResult),htmlTransform(htmlExpected));

}
};