ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

.closest() skips origin element #118

Open gmac opened 11 years ago

gmac commented 11 years ago

First off – thanks, and great work on the Ender suite. This is a slick jQuery alternative.

I have noticed something I would consider an issue with Bonzo's implementation of the .closest() command within the Ender bridge. It appears that calling .closest() does not take the origin element into account as a potential selector target; it only appears to climb the tree from the origin. Take the following example...

HTML: <a href="to.html">Click <b>Me</b></a>

JS (using Qwery, Bonzo & Bean):

$("a").on("click", function(evt) {
     evt.preventDefault();
     console.log( $(evt.target).closest("a") );
});

In the above example, clicking on the "Click" portion of the anchor finds nothing, while clicking on the "Me" portion of the anchor successfully finds the parent anchor. This differs from the jQuery implementation, which would find the anchor in both cases. I find the jQuery implementation more intuitive, which includes the origin element as a potential candidate in the closest selector search. If I wanted to omit the origin and just search up the tree, I'd use .parents().

rvagg commented 11 years ago

I agree @gmac, this is a problem that we ought to fix here but if you want better traversal support then you should consider adding traversty to the mix which has a proper closest() and other jQuery-compatible methods. I use it to do exactly what you've provided in your example.

ded commented 11 years ago

if this is how jQuery does it — it seems wrong. i don't understand why the same element would be considered the closest. i suppose this is based on the old saying "nobody knows yourself better than you"

ded commented 11 years ago

the current implementation is here: https://github.com/ded/bonzo/blob/master/src/ender.js#L41

i suppose we can change the src to look like this

var collection = $(selector), j, k, p, r = []
for (j = 0, k = this.length; j < k; j++) {
  p = this[j]
  while (p) {
    if (~indexOf(collection, p)) {
      r.push(p)
      if (closest) break;
    }
    p = p.parentNode
  }
}

this way it gets seeded with the node itself as opposed to starting it off with the parentNode

rvagg commented 11 years ago

Having to $(selector) is so nasty, particularly on a large page with lots of nodes, tho it's tricky if you can't guarantee the existence of an $().is etc. This is why it should just be left to Traversty where all that hard work is taken care of. In fact, I'd vote for these traversal methods being removed from Bonzo to simplify.

I think the problem with the jQuery implementation is purely naming, closest doesn't naturally indicate self-included. But, it's pretty much a must for when you're listening to a DOM event on an element that has child elements so the target may either be the element itself or one of the child elements, hence $(event.target).closest('a') is a sure-way of getting the element you really want, regardless of what was actually clicked.

ded commented 11 years ago

actually, that would be a pretty great 1.4.0 release, and then add Traversty as a dependency.

the only problem then is that this library only ends up working for people who use Ender and would no longer work as a stand-alone.

rvagg commented 11 years ago

would it need to be a dependency? it could just be added to jeesh if we consider traversal functionality core (which I guess it is!)

gmac commented 11 years ago

I agree @ded that the naming of .closest() is odd, especially when considering its functional counterpart is called .parents()... Alas, I think we can all agree that the naming of jQuery methods deferred pragmatism in favor of fluffy names for the less savvy. That aside, the functional cases make a lot of sense: .closest() searches the origin and up, while .parents() searches only upward (personally, I think these would make the most sense as a single library method where includeOrigin is parameterized).

ded commented 11 years ago

let's push for a 2.0 release perhaps that removes a bunch of traversal methods in favor of traversty and include it as a dependency in the jeesh.