fix(jqLite): Support unbinding event handler self within handler

[chrisirhc]

If an event handler unbinds itself, the next event handler on the same
event and element isn't run.

This works fine in jQuery, and since jqLite doesn't support .one() (to bind handlers that only run once), this
might be a common use case.

I also suggest backporting this to the 1.0.x releases if those are still being maintained.

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

You mean it allows unregistering events because it iterates over a different array than the elements actual event array?

What does jQuery itself do here? If events are fired frequently, it seems potentially bad, especially since it will even clone arrays for things without handlers?

---

It looks like jquery creates a new array of handlers during dispatch, so that's probably okay

... hmmm, looks like IE8 had issues unregistering during dispatch?

[chrisirhc]

@caitp , I forgot that IE8 doesn't have slice. I'll fix that. Edit: Not slice, but another problem, just with the test.

---

Yep, the problem is that off actually modifies the array while the event listeners are dispatched.

Here's what happens:

1. Event handlers array contains [onceClickHandler, clickHandler].
2. Run index 0 of the array, which is onceClickHandler.
3. onceClickHandler unregisters itself.
4. Event handlers array is now [clickHandler].
5. Event dispatcher's forEach increments the index to 1 and finds the length of the array is also 1, so it stops there.

[caitp]

Yeah, I guess I can see how it might be useful --- I still don't necessarily think it's good to clone an empty array and iterate over it, but it's probably not that hurtful

[chrisirhc]

@caitp , I do get your concern. The alternative was to use isArray to check if it was an array before calling slice and otherwise use the object itself. I did that at first, but I also saw that there were places in jqLite where || [] was used when an array was needed. I'm fine with either really as long as this is fixed. This is quite critical, imho.

[chrisirhc]

I'm downloading IE8 vm to debug fix the test.

---

Found it. Was a JScript bug with named function expressions (function referring to itself, highlighted here). Test had to be written another way.

[petebacondarwin]

How about using shallowCopy() instead of slice() here?

Also eventHandlersSlice is not a good name - it is too specific to the implementation.
eventHandlersCopy or something would seem to provide more context

I think a comment explaining why the copy is being made would also be helpful for future maintenance.

[petebacondarwin]

@chrisirhc - can you ensure that you have signed the CLA?

[chrisirhc]

@petebacondarwin , thanks for the review. Updated the request. Didn't realize that shallowCopy creates an array-like object. That's convenient!

Signed the CLA under: Kien Chuan Chua

[petebacondarwin]

Do we actually need to call the event handlers in ascending order? If not then we could simply do:

var eventHandlers = events[type || event.type] || [];
for(var i=eventHandlers.length-1; i>=0; i=i-1) {
  eventHandlers[i].call(element, event);
}

Then if an even removes itself from the list, it won't affect the iteration...

[chrisirhc]

There is no mechanism to prevent handlers from removing other event handlers (or all event handlers). When iterating in any order on the current eventHandlers array, it causes another event handler to be skipped or fired twice, or even throw an error.

Perhaps, I should rename this commit to make this clearer that this prevents errors or unwanted behavior that arise from modifications to the eventHandlers array during execution of the event handlers. It also maintains consistency with jQuery's behavior of on, off and triggerHandlers.

Example:

eventHandlers = [hdl1, hdl2, hdl3]
hdl3 removes hdl2

Execution:
i = 2, eventHandlers = [hdl1, hdl2, hdl3], hdl3 is called. Then, hdl2 is removed.
i = 1, eventHandlers = [hdl1, hdl3], hdl3 is called.
i = 0, eventHandlers = [hdl1, hdl3], hdl1 is called.

hdl3 is called twice. hdl2 doesn't get called on this event at all. While this maybe intended for hdl2, I don't think users should be relying on such behavior. That's perhaps why jQuery is duplicating the array first as well (https://github.com/jquery/jquery/blob/master/src/event.js).

[matsko]

@petebacondarwin @IgorMinar any chance we'l be able to get this PR into 1.2.4?

[vojtajina]

@chrisirhc Thanks for a nice fix!

I refactored your test a tiny bit, see vojtajina@f6ae0db.

Thanks @caitp @petebacondarwin for review. I agree that cloning the array every time a listener is fired is not super efficient, but I wouldn't worry about it right now. If we find it to be a perf/memory bottleneck we can improve it (for instance we can do this: vojtajina@b51049f; I'm however keeping @chrisirhc implementation as it behaves more like real jQuery - if you remove some listener that is has been registered after you, this listener will still get called during the event - in my implementation it would not get called).

Merged as 2f91cfd.

[chrisirhc]

Awesome! And that teaches me to read up on Jasmine's spies 😄 .