@dhoko opened this Issue on August 25th 2015
find: function (selector)
{
    // we use querySelectorAll only on document, not on nodes because of its unexpected behavior. See for
    // instance http://stackoverflow.com/questions/11503534/jquery-vs-document-queryselectorall and
    // http://jsfiddle.net/QdMc5/ and http://ejohn.org/blog/thoughts-on-queryselectorall
    if (!document.querySelectorAll || !selector) {
        return []; // we do not support all browsers
    }

    var foundNodes = document.querySelectorAll(selector);

    return this.htmlCollectionToArray(foundNodes);
},
findMultiple: function (selectors)
{
    if (!selectors || !selectors.length) {
        return [];
    }

    var index, foundNodes;
    var nodes = [];
    for (index = 0; index < selectors.length; index++) {
        foundNodes = this.find(selectors[index]);
        nodes = nodes.concat(foundNodes);
    }

    nodes = this.makeNodesUnique(nodes);

    return nodes;
},
findNodesByTagName: function (node, tagName)
{
    if (!node || !tagName || !node.getElementsByTagName) {
        return [];
    }

    var foundNodes = node.getElementsByTagName(tagName);

    return this.htmlCollectionToArray(foundNodes);
},
makeNodesUnique: function (nodes)
{
    var copy = [].concat(nodes);
    nodes.sort(function(n1, n2){
        if (n1 === n2) {
            return 0;
        }

        var index1 = indexOfArray(copy, n1);
        var index2 = indexOfArray(copy, n2);

        if (index1 === index2) {
            return 0;
        }

        return index1 > index2 ? -1 : 1;
    });

    if (nodes.length <= 1) {
        return nodes;
    }

    var index = 0;
    var numDuplicates = 0;
    var duplicates = [];
    var node;

    node = nodes[index++];

    while (node) {
        if (node === nodes[index]) {
            numDuplicates = duplicates.push(index);
        }

        node = nodes[index++] || null;
    }

    while (numDuplicates--) {
        nodes.splice(duplicates[numDuplicates], 1);
    }

    return nodes;
},
findFirstNodeHavingClass: function (node, className)
            {
                if (!node || !className) {
                    return;
                }

                if (this.hasNodeCssClass(node, className)) {
                    return node;
                }

                var nodes = this.findNodesHavingCssClass(node, className);

                if (nodes && nodes.length) {
                    return nodes[0];
                }
            },
findNodesHavingAttribute: function (nodeToSearch, attributeName, nodes)
            {
                if (!nodes) {
                    nodes = [];
                }

                if (!nodeToSearch || !attributeName) {
                    return nodes;
                }

                var children = getChildrenFromNode(nodeToSearch);

                if (!children || !children.length) {
                    return nodes;
                }

                var index, child;
                for (index = 0; index < children.length; index++) {
                    child = children[index];
                    if (this.hasNodeAttribute(child, attributeName)) {
                        nodes.push(child);
                    }

                    nodes = this.findNodesHavingAttribute(child, attributeName, nodes);
                }

                return nodes;
            }

Hi,

I think you can remove a lot of code from here by using document.querySelectorAll or document.querySelectorAll (Compat table). If you read one of the link from the find method:

document.querySelectorAll() has several inconsistencies across browsers and is not supported in older browsersThis probably won't cause any trouble anymore nowadays

So with it you won't need makeNodesUnique, and you can use findMultiple without doing a lot of queries.

PS: I think you need to support IE8 but without its support we can remove the code from findFirstNodeHavingAttribute too

@sgiehl commented on August 25th 2015 Member

For piwik.js we need to try to support as much browsers as possible, as those browsers wouldn't be tracked correctly otherwise. So imho we should also support IE < 8.

@tsteur commented on August 26th 2015 Owner

With piwik.js we still support IE6 etc. Here are also some thoughts on https://github.com/piwik/piwik/blob/2.15.0-b2/misc/internal-docs/content-tracking.md#thoughts-on-piwikjs selectors and querySelectorAll(). We can't use it anytime soon, unfortunately.

@dhoko commented on August 26th 2015

Yup I understand why you need to support these browsers, but you can use a modern code, then a polyfill for olders one, isn't it ?

@tsteur :

As we don't need many features we could implement it ourselves but probably needs a lot of cross-browser testing which I wanted to avoid. We'd only start with querySelectorAll() maybe. Brings also incredible performance benefits (2-10 faster than jQuery) but there might be problems see

It's faster and consistant accross browsers. It You can find some issues with it cf but if you know them it's not a problem anymore.

I think a polyfill is a good solution.

@tsteur commented on August 26th 2015 Owner

Yup I understand why you need to support these browsers, but you can use a modern code, then a polyfill for olders one, isn't it ?

I know, we're just going this way to keep testing etc simple. By using one code path for all it's easier for us to test etc. Eg our query code would only run on old browsers and we would most likely not notice if it is not working correctly etc. It might be possible to do if we had automated testing on older browsers but this is not the case currently

@tsteur commented on August 26th 2015 Owner

Is there currently a problem eg re performance etc? As mentioned, ideally it would be the case like you said but currently not really doable for us.

@dhoko commented on August 26th 2015

I don't think it's a problem with performance, piwik has to perform as well as it can. Dude Piwik parse the DOM (with a lot of lol :confounded:) ! It has to work on mobile device etc.

AMHA If you can use a modern and fast API you have to.

This Issue was closed on August 26th 2015
Powered by GitHub Issue Mirror