bonzo icon indicating copy to clipboard operation
bonzo copied to clipboard

.closest() skips origin element

Open gmac opened this issue 13 years ago • 8 comments

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().

gmac avatar Apr 21 '13 16:04 gmac

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.

rvagg avatar Apr 21 '13 22:04 rvagg

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 avatar Jun 01 '13 19:06 ded

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

ded avatar Jun 01 '13 20:06 ded

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.

rvagg avatar Jun 02 '13 01:06 rvagg

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.

ded avatar Jun 02 '13 23:06 ded

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!)

rvagg avatar Jun 03 '13 02:06 rvagg

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).

gmac avatar Jun 03 '13 15:06 gmac

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.

ded avatar Jun 03 '13 22:06 ded