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

Nodes incorrectly matched when selector starts above passed in context #92

Open
tomocchino opened this issue Oct 4, 2013 · 5 comments

Comments

@tomocchino
Copy link

The context node is ignored if a match happens that surrounds it. This is hard to explain so I wrote some code here, and a fiddle: http://jsfiddle.net/dvYmQ/

<div id="one">
  <div id="two">
    <div id="three"></div>
  </div>
</div>

the following matches even though the entirety of the selector isn't contained inside div two

var two = document.getElementById('two');
qwery('#one #three', two); // shouldn't match, but it returns [three]

The only reason I noticed this at all is because I attempted to replace Facebook's selector engine with qwery tonight at a hackathon, and we have an explicit test case for this. I know you haven't worked on qwery in a while, but it's still tiny, and seems good, so I figured I'd try to cut us over.

I also attempted to fix so I could send you a patch, but I've been awake too long and am largely useless right now.

@rvagg
Copy link
Collaborator

rvagg commented Oct 4, 2013

I'll have a look at this when I'm home next week.

@ryanve
Copy link

ryanve commented Oct 4, 2013

This may relate to the nature of QSA. It effectively queries the whole document and then filters for descendants.

1 === document.documentElement.querySelectorAll('html body').length

whereas

0 === jQuery('html').find('html body').length

Based on jsfiddle.../1 qwery matches like QSA even with useNativeQSA:false. I imagine emulating QSA is the desired behavior. The latter is harder to implement.

@rvagg
Copy link
Collaborator

rvagg commented Oct 14, 2013

Thanks for the info @ryanve, that zepto discussion is quite interesting; sounds like there's a general agreement that qSA has a spec bug on this issue. The link to the article about :scoped is particularly interesting.

It actually wouldn't be hard to change to match the jQuery behaviour on this as we already have element id rewriting in place for relationship-first selectors (e.g. qwery('> .foo', document.getElementById('bar'))). See here. It would just be a matter of invoking this for every query where a context is provided. Probably needs some benchmarking work to see the impact of this.

The element id rewriting works like this, if required:

  1. create a new unique id for the context/root element of the query if it doesn't have one
  2. rewrite the selector to have a prefix of '[id=newid]', you can imagine the effect when you provide a selector like '> .foo'.
  3. perform the query on the element's parent (if it has a parent)
  4. remove the id if we created a new one for the element

If we did this for all queries that involved a custom context/root then we'd be automatically restricting queries to children of that element, no matter what the query because it's already scoped.

@ryanve
Copy link

ryanve commented Oct 15, 2013

The temp [id] is a clever solution. qSA's behavior is not intuitive. But when aware of it, it doesn't seem too problematic either. Chained finds .find('ul').find('li') can ensure the other behavior. I'm on the fence.

@rvagg
Copy link
Collaborator

rvagg commented Oct 16, 2013

I'm happy to standardise on the Sizzle behaviour here; it's just a matter of ensuring that doing so doesn't impose large performance costs.

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