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

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 1 commit
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
Prev Previous commit
Next Next commit
helper to set attributes on an element from a map of values
  • Loading branch information
@dcharbonnier
dcharbonnier committed Jul 25, 2014
commit 0e78c9e53c7710c693d1758d8f3692d85053cda0
19 changes: 19 additions & 0 deletions src/js/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,25 @@ vjs.createEl = function(tagName, properties){
return el;
};

/**
* 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){
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

changed to getElementAttributes

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

var keys = Object.keys(attributes);
Copy link
Member

Choose a reason for hiding this comment

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

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

for(var i = 0,l = keys.length;i<l;i++) {
var attrName = keys[i];
var attrValue = attributes[attrName];
if (attrValue === null || typeof attrValue === 'undefined' || attrValue === false) {
el.removeAttribute(attrName);
} else {
el.setAttribute(attrName,attrValue === true ? '' : attrValue);
}
}
};

/**
* Uppercase the first letter of a string
* @param {String} string String to be uppercased
Expand Down
16 changes: 16 additions & 0 deletions test/unit/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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.getAttributeValues(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