Skip to content

Commit

Permalink
Maded some revisions to #1369
Browse files Browse the repository at this point in the history 
closes #1369
  • Loading branch information
@heff
heff committed Aug 4, 2014
1 parent b3bbb17 commit 527a33a
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 73 deletions.
46 changes: 23 additions & 23 deletions src/js/lib.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,29 @@ var hasOwnProp = Object.prototype.hasOwnProperty;
* @private
*/
vjs.createEl = function(tagName, properties){
var el, propName;

el = document.createElement(tagName || 'div');

for (propName in properties){
if (hasOwnProp.call(properties, propName)) {
//el[propName] = properties[propName];
// Not remembering why we were checking for dash
// but using setAttribute means you have to use getAttribute

// The check for dash checks for the aria-* attributes, like aria-label, aria-valuemin.
// The additional check for "role" is because the default method for adding attributes does not
// add the attribute "role". My guess is because it's not a valid attribute in some namespaces, although
// browsers handle the attribute just fine. The W3C allows for aria-* attributes to be used in pre-HTML5 docs.
// http://www.w3.org/TR/wai-aria-primer/#ariahtml. Using setAttribute gets around this problem.

if (propName.indexOf('aria-') !== -1 || propName=='role') {
el.setAttribute(propName, properties[propName]);
} else {
el[propName] = properties[propName];
}
var el;

tagName = tagName || 'div';
properties = properties || {};

el = document.createElement(tagName);

vjs.obj.each(properties, function(propName, val){
// Not remembering why we were checking for dash
// but using setAttribute means you have to use getAttribute

// The check for dash checks for the aria-* attributes, like aria-label, aria-valuemin.
// The additional check for "role" is because the default method for adding attributes does not
// add the attribute "role". My guess is because it's not a valid attribute in some namespaces, although
// browsers handle the attribute just fine. The W3C allows for aria-* attributes to be used in pre-HTML5 docs.
// http://www.w3.org/TR/wai-aria-primer/#ariahtml. Using setAttribute gets around this problem.
if (propName.indexOf('aria-') !== -1 || propName == 'role') {

This comment has been minimized.

Sign in to view

Copy link
@dcharbonnier

dcharbonnier Aug 5, 2014

no sure why you do that, the setAttribute is supported even on ie8 and the performances are better than [] adding this huge block of comment with a "no remembering" and a condition seams odd. But not related to the current pull request.
http://jsperf.com/attribute-vs-setattribute/3

This comment has been minimized.

Sign in to view

Copy link
@gkatsev

gkatsev Aug 5, 2014

Member
  • I'm guessing that it used to be el[propName] originally and then changed to add aria and role attributes via setAttributes.
  • I know that some of the things that are set here are things like class which would require elsewhere in the code change what the propName for that field is when used via setAttribute and via the square brackets.
  • That jsperf is inconclusive, if anything, it shows that not using setAttribute would be faster. However, in reality, it probably doesn't make much, if at all, a difference, especially since the amount of items that we traverse here is so small.

However, you are right, we probably should just transition and only use setAttribute.

This comment has been minimized.

Sign in to view

Copy link
@heff

heff Aug 5, 2014

Author Member

Yeah, we use it to streamline setting properties like style, innerHTML, and classname. And actually that's probably still the majority of the way we use this function. We talked about adding some way to set attributes specifically in another issue, and I'm still up for that.

This comment has been minimized.

Sign in to view

Copy link
@dcharbonnier

dcharbonnier Aug 5, 2014

Sorry my jsperf link was cryptic. I think that the performance difference is very small, non even sure what's the 'best', and videojs is not a template engine so the only goal is to clean the code and make it easier to understand.

This comment has been minimized.

Sign in to view

Copy link
@gkatsev

gkatsev Aug 5, 2014

Member

goal is to clean the code and make it easier to understand

Then we're in agreement :)
el.setAttribute(propName, val);
} else {
el[propName] = val;
}
}
});

return el;
};

Expand Down Expand Up @@ -402,7 +402,7 @@ vjs.setElementAttributes = function(el, attributes){
if (attrValue === null || typeof attrValue === 'undefined' || attrValue === false) {
el.removeAttribute(attrName);
} else {
el.setAttribute(attrName,attrValue === true ? '' : attrValue);
el.setAttribute(attrName, (attrValue === true ? '' : attrValue));
}
});
};
Expand Down
4 changes: 2 additions & 2 deletions src/js/media/html5.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ vjs.Html5.prototype.createEl = function(){
el = clone;
player.tag = null;
} else {
el = vjs.createEl('video', {});
el = vjs.createEl('video');
vjs.setElementAttributes(el,
vjs.obj.merge(player.tagAttributes||{}, {
vjs.obj.merge(player.tagAttributes || {}, {
id:player.id() + '_html5_api',
'class':'vjs-tech'
})
Expand Down
7 changes: 4 additions & 3 deletions src/js/player.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ 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++;

// Store the tag attributes used to restore html5 element
this.tagAttributes = tag && vjs.getElementAttributes(tag);

// Set Options
// The options argument overrides options set in the video tag
// which overrides globally set options.
Expand Down Expand Up @@ -195,7 +196,7 @@ vjs.Player.prototype.createEl = function(){

// Copy over all the attributes from the tag, including ID and class
// ID will now reference player box, not the video tag
attrs = vjs.getAttributeValues(tag);
attrs = vjs.getElementAttributes(tag);
vjs.obj.each(attrs, function(attr) {
el.setAttribute(attr, attrs[attr]);
});
Expand Down
19 changes: 8 additions & 11 deletions test/unit/lib.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,20 +152,17 @@ test('should read tag attributes from elements, including HTML5 in all browsers'
equal(trackVals['title'], 'test');
});

test('should set element attributes from object', function(){
var el, vid1Vals;

test('should set tag attributes from object', function(){
el = document.createElement('div');
el.id = 'el1';

var tags = '<video id="vid1" controls data-test="ok"></video>';
vjs.setElementAttributes(el, { controls: true, 'data-test': 'asdf' });

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');
equal(el.getAttribute('id'), 'el1');
equal(el.getAttribute('controls'), '');
equal(el.getAttribute('data-test'), 'asdf');
});

test('should get the right style values for an element', function(){
Expand Down
40 changes: 17 additions & 23 deletions test/unit/player.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,32 +495,26 @@ test('Data attributes on the video element should persist in the new wrapper ele
equal(player.el().getAttribute('data-id'), dataId, 'data-id should be available on the new player element after creation');
});

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;
test('should restore attributes from the original video tag when creating a new element', function(){
var player, html5Mock, el;

var tag = document.getElementById('example_1');
var player = new videojs.Player(tag, {techOrder:['html5']});
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);
player = PlayerTest.makePlayer();
html5Mock = { player_: player };

PlayerTest.htmlEqualWithSort(player.tech.el_.outerHTML, html.replace('example_1','example_1_html5_api'));
});
// simulate attributes stored from the original tag
player.tagAttributes = {
'preload': 'auto',
'controls': true,
'webkit-playsinline': true
};

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;
// set options that should override tag attributes
player.options_['preload'] = 'none';

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);
// create the element
el = vjs.Html5.prototype.createEl.call(html5Mock);

PlayerTest.htmlEqualWithSort(player.tech.el_.outerHTML, html.replace('example_1','example_1_html5_api').replace(' autoplay=""',''));
equal(el.getAttribute('preload'), 'none', 'attribute was successful overridden by an option');
equal(el.getAttribute('controls'), '', 'controls attribute was set properly');
equal(el.getAttribute('webkit-playsinline'), '', 'webkit-playsinline attribute was set properly');
});
11 changes: 0 additions & 11 deletions test/unit/test-helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,5 @@ 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));

}
};

1 comment on commit 527a33a

@dcharbonnier
Copy link

Choose a reason for hiding this comment

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

thank you, looks good to me

Please sign in to comment.