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

TODO: always return results in document-order #63

Closed
rvagg opened this issue Dec 12, 2011 · 6 comments
Closed

TODO: always return results in document-order #63

rvagg opened this issue Dec 12, 2011 · 6 comments

Comments

@rvagg
Copy link
Collaborator

rvagg commented Dec 12, 2011

I'm just going to put this out there as something for someone to do at some point in the future if they feel so inclined. I don't think it'd be a particularly easy job, especially while maintaining current performance.

Native querySelectorAll() works, conceptually at least, by scanning all nodes from top to bottom and checking them against the selector, each match gets added to the return list and then we end up with a list of elements in proper document order. Qwery does this except in the case of grouped selectors because we split the selector up into component parts with selector.split(',') and process them separately with _qwery(). This split is also done for all browsers in the case of element-rooted queries with a relationship selector first (e.g. Q('>.yee, .haa', element)).

Take a fragment: <p><a/><b/><i/><em/></p> and query it for 'b,a'. qSA will give you [<a/>,<b/>] because this is document-order while Qwery will give you [<b/>,<a/>] because we first process the 'b' and then the 'a' and append the results together. Sizzle and NW manage to do this properly. There are some cases where it matters but it mainly matters because that's the de-facto standard that's emerged and been written into the actual standard.

Here's a snippet that you can run in the console of a modern browser to demonstrate the issue and could form the basis of a simple unit test:

var e = document.createElement('p');
e.innerHTML = '<a/><i/><b/>';
e.querySelectorAll = null; // comment this out to run with the native qSA
var r = qwery('b,a', e); // insert other selector engine here if you want to see if it works properly
if (r.length !== 2 || !/[aA]/.test(r[0].tagName)) console.log("Bzzt! Wrong answer", r);
else console.log('correct:',r);

This mainly matters for IE <= 7, it matters in IE8 when you're using CSS3+ selectors (otherwise it uses the in-built CSS2-compliant qSA) and I think it'll matter for all browsers with element-rooted queries with relationship selectors first (see above). IE6&7 will hopefully not matter soon so fixing it just for the CSS3 qSA case might be a bit simpler.

@ded
Copy link
Owner

ded commented Dec 12, 2011

yeah, i've been fully aware of this one for a while, but actually don't care much for it since i've never ran into a case, ever, where it mattered.

@rvagg
Copy link
Collaborator Author

rvagg commented Dec 12, 2011

Well I just ran into a case where it does matter! I have a new Ender module that's nearly ready for release that depends on having results in document order. But thankfully I have a workaround for these special cases. Will explain when I have the code in GitHub.

@rvagg
Copy link
Collaborator Author

rvagg commented Dec 13, 2011

So here's where it matters. In Traversty I use the selector engine to find child elements matching the given selector and then return the element at the index requested.

So, $('ul').down('span,li', 2) (both args are optional but the problem only occurs where you provide both) is intended to return the 3rd child element in document order matching the selector 'span,li'. But since Qwery will do the 'span' first followed by the 'li' we'd likely get them in the wrong order and return the wrong element.

This is an uncommon use-case though, you're more likely to give very simple selectors ($('ul').down('li', 2)), it's also only a problem on IE6&7 (and 8 if you use a CSS3 selector) but it's a use-case none the less! And the workaround is fairly simple, if the selector has a comma then I get all child elements (byTag(*)) and run an is() on each of them: https://github.com/rvagg/traversty/blob/3eb6f257e641abeead2f31eb258a05e53639a45d/src/traversty.js#L60-74

@ded
Copy link
Owner

ded commented Dec 15, 2011

yeah seems like it should only be done as a special forked case and wouldn't want to hurt the core engine with not-so-useful-most-of-the-time overhead

@dperini
Copy link

dperini commented Feb 9, 2012

Well, it seems there are many more simple cases requiring elements in document order.

Never needed to extract a TOC from a document ?

document.querySelectorAll('h1,h2,h3,h4,h5,h6');

never used ?

@ded
Copy link
Owner

ded commented Feb 13, 2014

this should no longer be an issue with native qsa

@ded ded closed this as completed Feb 13, 2014
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

No branches or pull requests

3 participants