Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,10 @@ function createEventHandler(element, events) {
return event.defaultPrevented || event.returnValue === false;
};

forEach(events[type || event.type], function(fn) {
// Copy event handlers in case event handlers array is modified during execution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var eventHandlersCopy = shallowCopy(events[type || event.type] || []);

forEach(eventHandlersCopy, function(fn) {
fn.call(element, event);
});

Expand Down
22 changes: 22 additions & 0 deletions test/jqLiteSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,28 @@ describe('jqLite', function() {
});


it('should deregister specific listener within the listener and call subsequent listeners', function() {
var aElem = jqLite(a),
clickOnceSpy = jasmine.createSpy('clickOnce'),
clickSpy = jasmine.createSpy('click'),
clickOnceListener = function () {
aElem.off('click', clickOnceListener);
return clickOnceSpy();
};

aElem.on('click', clickOnceListener);
aElem.on('click', clickSpy);

browserTrigger(a, 'click');
expect(clickOnceSpy).toHaveBeenCalledOnce();
expect(clickSpy).toHaveBeenCalledOnce();

browserTrigger(a, 'click');
expect(clickOnceSpy).toHaveBeenCalledOnce();
expect(clickSpy.callCount).toBe(2);
});


it('should deregister specific listener for multiple types separated by spaces', function() {
var aElem = jqLite(a),
masterSpy = jasmine.createSpy('master'),
Expand Down