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.

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

use setAttribute when adding type property. Fixes #2480 #2487

Closed
wants to merge 1 commit into from

Conversation

mmcc
Copy link
Member

@mmcc mmcc commented Aug 17, 2015

No description provided.

@@ -43,7 +43,7 @@ export function createEl(tagName='div', properties={}){
// 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') {
if (propName.indexOf('aria-') !== -1 || propName === 'role' || propName === 'type') {
Copy link
Member

Choose a reason for hiding this comment

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

Fine for now, but if we get any more properties we'd want to case, we should switch to something like:

let properties = {
  'role': 1,
  'type': 1,
  'foo': 1,
  'bar'
};
if (propName in properties) {
  el.setAttribute(propName, val);
}

@gkatsev
Copy link
Member

gkatsev commented Aug 17, 2015

LGTM

@pam
Copy link

pam commented Aug 17, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: bb5c97a
Build details: https://travis-ci.org/pam/video.js/builds/75980624

(Please note that this is a fully automated comment.)

@mmcc mmcc closed this in 863f315 Aug 17, 2015
@mmcc mmcc mentioned this pull request Aug 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants