It seems to have a bug when element' ancestor has a matched node.

[bencode]

Example

<div class="item">
  <ul class="menu">
    <li>1</li>
    <li>2</li>
    <li class="item">3</li>
    <li class="item">4</li>
  </ul>
</div>

var menu = document.querySelector('.menu');
delegate(menu, '.item', 'click', function() {
  console.log('here')
});

Maybe Solution

function closest (element, selector, stop) {  // <----- add stop element
  while (element && element !== stop && element.nodeType !== DOCUMENT_NODE_TYPE) {
    if (typeof element.matches === 'function' &&
        element.matches(selector)) {
      return element;
    }
    element = element.parentNode;
  }
}

function listener(element, selector, type, callback) {
  return function(e) {
    e.delegateTarget = closest(e.target, selector, element);  // <------- +

    if (e.delegateTarget) {
        callback.call(element, e);
    }
  }
}

[fregante]

That's just how .closest is meant to work. If you expect li.item, your selector should be li.item

[JJWesterkamp]

I think this is a fine suggestion.

That's just how .closest is meant to work.

Although that might be true, the correct implementation of delegate requires something slightly different from how exactly closest is supposed to work - or something in addition to that.

If in the example the menu reference is passed as 'base element' for the event delegation, the given event callback must never be called in the context of an element that is not a descendant of menu. The current implementation does exactly that because at delegate.js:70 element references are obtained that should not be: elements that are not part of menu its child tree.

---

Yes, e.target is definitely a descendant of menu, otherwise e would never hit menu. But the event itself remains a possibly false match for the specified event-delegation parameters.

[fregante]

The closest implementation should not be altered because that’s just a polyfill, most browsers don’t use that code at all.

At most, if I’m understanding the problem correctly, we should add an additional condition to make sure that the found element is actually inside the parent with the actual event.

Something like if !e.currentTarget.contains(closestElement), return

[JJWesterkamp]

Something like that would work, but would be way less performant than breaking early in the while loop, which might otherwise recurse many times more than is necessary, especially when currentTarget is a deeply nested element.

So maybe change the implementation of closest as suggested nonetheless, and maybe call it closestUntil instead to prevent confusion?

[fregante]

There’s no loop. Closest only gets called once and that code goes after return function () {

[JJWesterkamp]

I don't really understand what you're trying to say.

As far as I can see there's a loop that's not run only once, but each time a function returned from listener is called, which is whenever an event of matching type occurs. This is because that containing function is the function that eventually ends up being the actual event-listener callback.

Also, closest is not implemented as a polyfill, but rather as a regular function that is being exported, imported and used unconditionally.

Please explain if I'm misunderstanding anything.

[fregante]

Also, closest is not implemented as a polyfill, but rather as a regular function that is being exported, imported and used unconditionally.

You're right. Fix coming in #29

As far as I can see there's a loop that's not run only once, but each time a function returned from listener is called, which is whenever an event of matching type occurs.

The loop is in closest but I'm talking about placing the condition in delegate itself, after closest. Look: 1ed2752