Zepto.find() behaves differently from jQuery.find() #730

Closed
sergiolopes opened this Issue Mar 23, 2013 · 14 comments

Projects

None yet

7 participants

@sergiolopes

Test cases:

(I tested in Chrome only but it should be similar in all browsers)

Since Zepto uses querySelectorAll, it matches children elements differently that jQuery. element.querySelectorAll evaluates the selector relatively to the page context, not scoped to the element as jQuery does:

http://www.w3.org/TR/selectors-api/#examples0

Don't know what to do. Should Zepto not use querySelectorAll for find() and mimic jQuery's behavior? Or should Zepto be different than jQuery? In that case, people must know this difference since it can cause serious incompatibility problems for people migrating from jQuery to Zepto.

@mislav
Collaborator

I didn't know about this distinction. Thanks for bringing it to our attention.

It's unfortunate that the behavior is different, but while thinking about it, I realized I don't care about this getting "fixed". As far as I care, the behavior in Zepto is OK.

The collection.find(selector) method is supposed to find elements that match a selector that are descendants of elements in the collection. Your Zepto example shows that this is true, since paragraphs in "#subelement" match the "body p" selector.

@mislav mislav closed this Mar 24, 2013
@shalecraig

To start a pull request for this, we could probably filter the results returned by contains. Would be slower, but we would match their api.

@mislav

@mislav
Collaborator

@shalecraig And technically how do you intend to solve this? jQuery has a custom CSS selector engine.

Don't start on a pull request just yet, because we might not accept it (per my comment above). But I'm interested in how would you solve this practically.

@shalecraig

This is an oversimplification, but I don't think this would be too hard:

find: function(selector){
      var result, $this = this
      if (typeof selector == 'object')
        result = $(selector).filter(function(){
          var node = this
          return emptyArray.some.call($this, function(parent){
            return $.contains(parent, node)
          })
        })
      else if (this.length == 1)
          result = $(zepto.qsa(this[0], selector)).filter(function(){
          var node = this
          return emptyArray.some.call($this, function(parent){
            return $.contains(parent, node)
          })
        })
      else result = this.map(function(){ return zepto.qsa(this, selector) })
      return result
    },

We can de-duplicate a bit to look like:

find: function(selector){
      var result, $this = this
      if (typeof selector === 'object' || this.length === 1) {
        var res;
        if (selector === 'object') {
          res = $(selector)
        } else {
          res = $(zepto.qsa(this[0], selector))
        }
        return res.filter(function(){
          var node = this
          return emptyArray.some.call($this, function(parent){
            return $.contains(parent, node)
          })
      } else result = this.map(function(){ return zepto.qsa(this, selector) })
      return result
    },
@mislav
Collaborator

@shalecraig Your code now fails to find a simple paragraph nested in the div:

Zepto("#subelement").find("p").addClass("matched")
@philipwalton

Eric Bidelman recently blogged about the querySelector issue on html5rocks:
http://updates.html5rocks.com/2013/03/What-s-the-CSS-scope-pseudo-class-for

The TL;DR is that in the future we'll be able to pase the :scoped pseudo-class to fix this:
element.querySelectorAll(":scope p") only searches for paragraphs that are children of the calling element.

@savetheclocktower

Here's a clearer example of why this behavior is troublesome:

It's lamentable that this is querySelectorAll's spec bug, but the result is that there are real and surprising differences in behavior between the different versions of find.

In Prototype, we briefly explored assigning the given element a temporary, unique ID, then prepending that ID to the selector string. But that's a lot of DOM work and might be prohibitively costly. (Prototype solved it by moving to Sizzle.)

@shalecraig: I might be misunderstanding, but filtering through $.contains won't do anything. All the results returned by querySelectorAll are already guaranteed to be descendants of the given node. The problem is that some of the descendant nodes it returns may be outside of the expected result set because the semantics of the selector are different than they are in jQuery.

@mislav
Collaborator

Hi @savetheclocktower! Long time :)

We already do the unique ID tag in selector.js module to support queries such as find("> foo"). I'd like to think I would be open to doing it on every query ever in Zepto, but I'm reluctant to give the green light on that. It just smells as a potential disaster of edge cases. Thoughts?

@savetheclocktower

What sorts of edge cases are you worried about? I'm having a hard time envisioning them.

Also, you wouldn't need to do it on every query ever. This only produces surprising results in selectors with combinators. $node.find('div'), for example, wouldn't need to be transformed.

(You would need to sniff out comma-separated selectors — div, div span, p would need to be changed to div, .Zepto1364478728175 div span, p. So you'd need to split on commas, strip leading/trailing whitespace, analyze each selector individually, transform the ones that contain combinators, then join them up again. Easy! 😃 )

@mislav
Collaborator

So you'd need to split on commas, strip leading/trailing whitespace, analyze each selector individually, transform the ones that contain combinators, then join them up again. Easy!

Famous last words :P

So there you have an example of an edge case hard to tackle. To reliably split selectors on commas, we would need to have a selector parser.

@savetheclocktower

Yeah, I was trying to encode the sarcasm in my initial comment.

As for splitting selectors, @tdd (if I recall correctly) wrote Prototype's Selector#split method, which just threw a regex at the problem. To my knowledge, this Just Worked.

@mislav
Collaborator

OK SMARTASSES I reopened this now, any takers? I don't care about this enough to actually write code for this.

@mislav mislav reopened this Mar 28, 2013
@tdd

Oh man, @savetheclocktower, the entire legacy Selector engine… 😄 I still have very vivid memories of the evenings (well, Europe/Paris time) we spent working on that, Andrew. And despite the tons of regex + XPath we had to cook up, it's a very fond memory. We were basically the first to put that kind of feature set out! Good times.

(although I'll grant that this was not such a happy cranking out of code…)

@madrobby
Owner

Closing this because there was no response to our questions in a while (see #734). Feel free to reopen with the requested information! :)

@madrobby madrobby closed this Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment