Improper node matching #4

Closed
eligrey opened this Issue Jul 3, 2011 · 6 comments

Projects

None yet

2 participants

@eligrey

You should match only HTML elements, as you currently match non-HTML <progress> elements and don't match HTML <progress> elements in an XML context. Here's a helper function that should make it easy:

function match_HTML_element(elt, name) {
    if (elt.nodeType === elt.ELEMENT_NODE) {
        return false;
    }
    var elt_name = elt.localName;
    if (!elt.ownerDocument.xmlVersion) {
        elt_name = elt_name.toLowerCase();
    }
    return elt.namespaceURI === "http://www.w3.org/1999/xhtml" && elt_name === name;
}

You'd want to replace node.nodeName === 'PROGRESS' with match_HTML_element(node, "progress") and label.nodeName !== 'LABEL' with !match_HTML_element(label, "label") in your code.

@LeaVerou
Owner

You're right regarding matching HTML<progress> elements in an XML context (will fix, thanks), but why would someone use this in a document with non-HTML <progress> elements? That one looks too much of an edge case to me.

@eligrey

Well it's more the latter of elements in an XML context that's the issue, and I might as well fix the detection of non-HTML elements as well while I was at it. Please note that my helper function had a typo if you intend on using it elt.localName === name was supposed to be elt_name === name (just fixed it).

@eligrey eligrey closed this Jul 3, 2011
@LeaVerou LeaVerou reopened this Jul 3, 2011
@eligrey

Oops, sorry didn't mean to hit close.

@LeaVerou LeaVerou pushed a commit that referenced this issue Jul 3, 2011
Lea Verou Partial fix for issue #4 0d06173
@LeaVerou
Owner

Do you think my commit is sufficient for fixing the HTML elements in XML context issue?

As for the other, adding 10 extra lines of code for that much of an edge case seems a bit of an overkill to me tbh.

@eligrey

It fixes it, but it'll also match in an XML context, though you are right that almost nobody would ever do that and include this script, so yeah it's fine.

@LeaVerou
Owner

And if they do, it's probably a mistake, so they would want the script to be applied. Closing issue, thanks for everything :)

@LeaVerou LeaVerou closed this Jul 3, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment