nodeName_ does not properly canonicalize element names when using XHTML

[ysulyma]

Angular code assumes that element names will be uppercase. I am using Angular with XHTML5 (with application/xhtml+xml MIME type), where this is not the case. The code in question is the following:

if (msie < 9) {
  nodeName_ = function(element) {
    element = element.nodeName ? element : element[0];
    return (element.scopeName && element.scopeName != 'HTML')
      ? uppercase(element.scopeName + ':' + element.nodeName) : element.nodeName;
  };
} else {
  nodeName_ = function(element) {
    return element.nodeName ? element.nodeName : element[0].nodeName;
  };
}

I was able to remedy this in my situation by simply wrapping the return value of the else block in a call to uppercase(); however, before committing this as a fix I'd like some insight from someone who knows the internals better than me.

Specifically, I'm not sure if/how the msie < 9 block should be changed. I'm guessing the return value should be wrapped again, but does element.scopeName also need to be canonicalized? Is this a moot point since IE doesn't support XHTML anyway?

[ysulyma]

This was causing SCE to throw an error on an interpolated value in ng:src on an img element.

[petebacondarwin]

Here is some background info: http://ejohn.org/blog/nodename-case-sensitivity/

[petebacondarwin]

The concern I have is one of performance. It seems that wasteful to be running uppercase() all the time when the vast majority of cases do not need it.
Perhaps there could be some check of the document type, as well as browser type?

[ysulyma]

Another possible option would be to define variables IMG, OPTION, etc. which are set either to "IMG" or "img" depending on the document/browser type. Checks like nodeName_(node) != "IMG" would then be rewritten to nodeName_(node) != IMG.

[petebacondarwin]

@yuri0 - that solution wouldn't really scale so well as new elements are defined?

[petebacondarwin]

What does jQuery do?

[petebacondarwin]

This doesn't look good: http://jsperf.com/jquery-is-vs-node-name-tolowercase

[petebacondarwin]

Actually, nodeName_ is only used in a small number of places: by the SCE stuff, jqLite.val(), a check that we are not interpolating in the multiple attribute of the select element and most importantly when normalizing a directive name.

Interestingly, this last case is the only one that is not doing a direct comparison to a constant string. And in this case it is called lowercase() on the string anyway. So, if one is going to force a case then it should be lower to prevent an unnecessary lowercase(uppercase(name)) thing going on.

[ysulyma]

@petebacondarwin by my count, these types of checks are done in only 9 places throughout the code, and only on HTML, PRE, IMG, SELECT, TITLE.

[petebacondarwin]

and A

[petebacondarwin]

I think nodeName_ should be changed to return lowercase: https://developer.mozilla.org/en-US/docs/Writing_JavaScript_for_XHTML#Problem.3A_Names_in_XHTML_and_HTML_are_represented_in_different_cases

[petebacondarwin]

Then we wouldn't need to call lowercase() when normalizing directive names and just simple check against lowercase constants. But I am still unsure of the performance impact. Some of this code is very "hot"

[petebacondarwin]

This line: https://github.com/angular/angular.js/blob/master/src/jqLite.js#L502 could be changed to:

if (element.multiple && nodeName_(element) === 'select') {

which would be faster than it is now for the majority of cases.

[petebacondarwin]

These lines: https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L366-L370 could be changed to:

nodeName = nodeName_(this.$$element);

// sanitize a[href] and img[src] values
if ((nodeName === 'a' && key === 'href') ||
  (nodeName === 'img' && key === 'src')) {

$set is not a particular hot function.

[petebacondarwin]

This line https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L619 is made simpler:

directiveNormalize(nodeName_(node)), 'E', maxPriority, ignoreDirective);

[petebacondarwin]

Lines https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1284 and https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1298 would simple change to

(nodeName_(node) != "img" && (attrNormalizedName == "src" ||

and

if (name === "multiple" && nodeName_(node) === "select") {

[petebacondarwin]

There are a few places that also use node.nodeName directly. These ought to be converted to using nodeName_()

[petebacondarwin]

@yuri0 - fancy putting a PR together?

[ysulyma]

Sorry for taking so long (mainly, not making the 1.2 release), I'd never done a PR before and then forgot about it.