From 527a33a252bf657392616b7c393dc2fb0260e81e Mon Sep 17 00:00:00 2001 From: Steve Heffernan Date: Mon, 4 Aug 2014 15:04:39 -0700 Subject: [PATCH] Maded some revisions to videojs/video.js#1369 closes #1369 --- src/js/lib.js | 46 +++++++++++++++++++-------------------- src/js/media/html5.js | 4 ++-- src/js/player.js | 7 +++--- test/unit/lib.js | 19 +++++++--------- test/unit/player.js | 40 +++++++++++++++------------------- test/unit/test-helpers.js | 11 ---------- 6 files changed, 54 insertions(+), 73 deletions(-) diff --git a/src/js/lib.js b/src/js/lib.js index 4567926467..3d035a2abd 100644 --- a/src/js/lib.js +++ b/src/js/lib.js @@ -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') { + el.setAttribute(propName, val); + } else { + el[propName] = val; } - } + }); + return el; }; @@ -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)); } }); }; diff --git a/src/js/media/html5.js b/src/js/media/html5.js index d959d1a96f..eb4b41218d 100644 --- a/src/js/media/html5.js +++ b/src/js/media/html5.js @@ -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' }) diff --git a/src/js/player.js b/src/js/player.js index 6eb9ad64a6..6797d551a9 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -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. @@ -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]); }); diff --git a/test/unit/lib.js b/test/unit/lib.js index a21cc35193..080659c009 100644 --- a/test/unit/lib.js +++ b/test/unit/lib.js @@ -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 = ''; + 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(){ diff --git a/test/unit/player.js b/test/unit/player.js index c69add4c58..5e1335c023 100644 --- a/test/unit/player.js +++ b/test/unit/player.js @@ -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 = ''; - 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 = ''; - 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'); }); diff --git a/test/unit/test-helpers.js b/test/unit/test-helpers.js index ac84970f94..ee91ecb4c7 100644 --- a/test/unit/test-helpers.js +++ b/test/unit/test-helpers.js @@ -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)); - } };