@heff added an attributes argument to createEl(). closes #2589

fixes #2176

CHANGELOG.md

@@ -131,6 +131,7 @@ CHANGELOG
 * @heff moved scss vars to be private ([view](https://github.com/videojs/video.js/pull/2584))
 * @heff added a fancy loading spinner ([view](https://github.com/videojs/video.js/pull/2582))
 * @gkatsev added a mouse-hover time display to the progress bar ([view](https://github.com/videojs/video.js/pull/2569))
+* @heff added an attributes argument to createEl() ([view](https://github.com/videojs/video.js/pull/2589))
 
 --------------------
 

package.json

@@ -27,6 +27,7 @@
     "lodash-compat": "^3.9.3",
     "object.assign": "^2.0.1",
     "safe-json-parse": "^4.0.0",
+    "tsml": "1.0.1",
     "videojs-font": "1.3.0",
     "videojs-ie8": "1.1.0",
     "videojs-swf": "5.0.0-rc1",

src/js/button.js

@@ -37,17 +37,20 @@ class Button extends Component {
    * @return {Element}
    * @method createEl
    */
-  createEl(tag='button', props={}) {
-    // Add standard Aria and Tabindex info
+  createEl(tag='button', props={}, attributes={}) {
     props = assign({
       className: this.buildCSSClass(),
-      'role': 'button',
-      'type': 'button', // Necessary since the default button type is "submit"
-      'aria-live': 'polite', // let the screen reader user know that the text of the button may change
       tabIndex: 0
     }, props);
 
-    let el = super.createEl(tag, props);
+    // Add standard Aria info
+    attributes = assign({
+      role: 'button',
+      type: 'button', // Necessary since the default button type is "submit"
+      'aria-live': 'polite' // let the screen reader user know that the text of the button may change
+    }, attributes);
+
+    let el = super.createEl(tag, props, attributes);
 
     this.controlTextEl_ = Dom.createEl('span', {
       className: 'vjs-control-text'

src/js/component.js

@@ -211,12 +211,13 @@ class Component {
    * Create the component's DOM element
    *
    * @param  {String=} tagName  Element's node type. e.g. 'div'
-   * @param  {Object=} attributes An object of element attributes that should be set on the element
+   * @param  {Object=} properties An object of properties that should be set
+   * @param  {Object=} attributes An object of attributes that should be set
    * @return {Element}
    * @method createEl
    */
-  createEl(tagName, attributes) {
-    return Dom.createEl(tagName, attributes);
+  createEl(tagName, properties, attributes) {
+    return Dom.createEl(tagName, properties, attributes);
   }
 
   localize(string) {

src/js/control-bar/live-display.js

@@ -33,7 +33,8 @@ class LiveDisplay extends Component {
 
     this.contentEl_ = Dom.createEl('div', {
       className: 'vjs-live-display',
-      innerHTML: `<span class="vjs-control-text">${this.localize('Stream Type')}</span>${this.localize('LIVE')}`,
+      innerHTML: `<span class="vjs-control-text">${this.localize('Stream Type')}</span>${this.localize('LIVE')}`
+    }, {
       'aria-live': 'off'
     });
 

src/js/control-bar/progress-control/seek-bar.js

@@ -33,7 +33,8 @@ class SeekBar extends Slider {
    */
   createEl() {
     return super.createEl('div', {
-      className: 'vjs-progress-holder',
+      className: 'vjs-progress-holder'
+    }, {
       'aria-label': 'video progress bar'
     });
   }

src/js/control-bar/spacer-controls/spacer.js

@@ -25,11 +25,10 @@ class Spacer extends Component {
   /**
    * Create the component's DOM element
    *
-   * @param {Object} props An object of properties  
    * @return {Element}
    * @method createEl
    */
-  createEl(props) {
+  createEl() {
     return super.createEl('div', {
       className: this.buildCSSClass()
     });

src/js/control-bar/time-controls/current-time-display.js

@@ -34,8 +34,11 @@ class CurrentTimeDisplay extends Component {
 
     this.contentEl_ = Dom.createEl('div', {
       className: 'vjs-current-time-display',
-      innerHTML: '<span class="vjs-control-text">Current Time </span>' + '0:00', // label the current time for screen reader users
-      'aria-live': 'off' // tell screen readers not to automatically read the time as it changes
+      // label the current time for screen reader users
+      innerHTML: '<span class="vjs-control-text">Current Time </span>' + '0:00',
+    }, {
+      // tell screen readers not to automatically read the time as it changes
+      'aria-live': 'off'
     });
 
     el.appendChild(this.contentEl_);

src/js/control-bar/time-controls/duration-display.js

@@ -40,16 +40,19 @@ class DurationDisplay extends Component {
 
     this.contentEl_ = Dom.createEl('div', {
       className: 'vjs-duration-display',
-      innerHTML: `<span class="vjs-control-text">${this.localize('Duration Time')}</span> 0:00`, // label the duration time for screen reader users
-      'aria-live': 'off' // tell screen readers not to automatically read the time as it changes
+      // label the duration time for screen reader users
+      innerHTML: `<span class="vjs-control-text">${this.localize('Duration Time')}</span> 0:00`
+    }, {
+      // tell screen readers not to automatically read the time as it changes
+      'aria-live': 'off'
     });
 
     el.appendChild(this.contentEl_);
     return el;
   }
 
   /**
-   * Update duration time display   
+   * Update duration time display
    *
    * @method updateContent
    */

src/js/control-bar/time-controls/remaining-time-display.js

@@ -34,8 +34,11 @@ class RemainingTimeDisplay extends Component {
 
     this.contentEl_ = Dom.createEl('div', {
       className: 'vjs-remaining-time-display',
-      innerHTML: `<span class="vjs-control-text">${this.localize('Remaining Time')}</span> -0:00`, // label the remaining time for screen reader users
-      'aria-live': 'off' // tell screen readers not to automatically read the time as it changes
+      // label the remaining time for screen reader users
+      innerHTML: `<span class="vjs-control-text">${this.localize('Remaining Time')}</span> -0:00`,
+    }, {
+      // tell screen readers not to automatically read the time as it changes
+      'aria-live': 'off'
     });
 
     el.appendChild(this.contentEl_);

src/js/control-bar/volume-control/volume-bar.js

@@ -32,7 +32,8 @@ class VolumeBar extends Slider {
    */
   createEl() {
     return super.createEl('div', {
-      className: 'vjs-volume-bar',
+      className: 'vjs-volume-bar'
+    }, {
       'aria-label': 'volume level'
     });
   }

src/js/menu/menu-item.js

@@ -24,15 +24,15 @@ class MenuItem extends Button {
    * Create the component's DOM element
    *
    * @param {String=} type Desc
-   * @param {Object=} props Desc 
+   * @param {Object=} props Desc
    * @return {Element}
    * @method createEl
    */
-  createEl(type, props) {
+  createEl(type, props, attrs) {
     return super.createEl('li', assign({
       className: 'vjs-menu-item',
       innerHTML: this.localize(this.options_['label'])
-    }, props));
+    }, props), attrs);
   }
 
   /**

src/js/slider/slider.js

@@ -43,18 +43,22 @@ class Slider extends Component {
    * @return {Element}
    * @method createEl
    */
-  createEl(type, props={}) {
+  createEl(type, props={}, attributes={}) {
     // Add the slider element class to all sub classes
     props.className = props.className + ' vjs-slider';
     props = assign({
+      tabIndex: 0
+    }, props);
+
+    attributes = assign({
       'role': 'slider',
       'aria-valuenow': 0,
       'aria-valuemin': 0,
       'aria-valuemax': 100,
       tabIndex: 0
-    }, props);
+    }, attributes);
 
-    return super.createEl(type, props);
+    return super.createEl(type, props, attributes);
   }
 
   /**

src/js/utils/dom.js

@@ -4,6 +4,8 @@
 import document from 'global/document';
 import window from 'global/window';
 import  * as Guid from './guid.js';
+import log from './log.js';
+import tsml from 'tsml';
 
 /**
  * Shorthand for document.getElementById()
@@ -29,25 +31,28 @@ export function getEl(id){
  * @return {Element}
  * @function createEl
  */
-export function createEl(tagName='div', properties={}){
+export function createEl(tagName='div', properties={}, attributes={}){
   let el = document.createElement(tagName);
 
   Object.getOwnPropertyNames(properties).forEach(function(propName){
-      let val = 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' || propName === 'type') {
-       el.setAttribute(propName, val);
-      } else {
-       el[propName] = val;
-      }
+    let val = properties[propName];
+
+    // See #2176
+    // We originally were accepting both properties and attributes in the
+    // same object, but that doesn't work so well.
+    if (propName.indexOf('aria-') !== -1 || propName === 'role' || propName === 'type') {
+      log.warn(tsml(`Setting attributes in the second argument of createEl()
+                has been deprecated. Use the third argument instead.
+                createEl(type, properties, attributes). Attempting to set ${propName} to ${val}.`));
+      el.setAttribute(propName, val);
+    } else {
+      el[propName] = val;
+    }
+  });
+
+  Object.getOwnPropertyNames(attributes).forEach(function(attrName){
+    let val = attributes[attrName];
+    el.setAttribute(attrName, attributes[attrName]);
   });
 
   return el;

test/unit/utils/dom.test.js

@@ -20,11 +20,16 @@ test('should return the element with the ID', function(){
 });
 
 test('should create an element', function(){
-  var div = Dom.createEl();
-  var span = Dom.createEl('span', { 'data-test': 'asdf', innerHTML:'fdsa' });
+  let div = Dom.createEl();
+  let span = Dom.createEl('span', {
+    innerHTML: 'fdsa'
+  }, {
+    'data-test': 'asdf'
+  });
+
   ok(div.nodeName === 'DIV');
   ok(span.nodeName === 'SPAN');
-  ok(span['data-test'] === 'asdf');
+  ok(span.getAttribute('data-test') === 'asdf');
   ok(span.innerHTML === 'fdsa');
 });