This project is archived and is in readonly mode.

#954 ✓resolved
Ian Wilson

removeEvent prevents other event from firing

Reported by Ian Wilson | August 1st, 2010 @ 09:17 PM | in 1.3.0 rc2 (closed)

Two listeners are attached to a custom event on an element. The first listener will try to remove itself from the element. When the event is fired the first listener successfully removes itself but while doing so the length of the list of event listeners is decreased by 1. This causes each to end prematurely within fireEvent and the second listener is not reached. Firing the event again will fire the second listener only.

Attached is an example of the bug that was generated using jsfiddle.net. I'm not sure how long the links last but here is a link to it as well: http://jsfiddle.net/hAuKv/2/.

Comments and changes to this ticket

  • Ian Wilson

    Ian Wilson August 1st, 2010 @ 09:23 PM

    Oops also the solution seems to be to clone the list of listeners before calling them. A solution proposed by keeto on irc is to use slice to do the cloning, such as events[type].keys.slice().each(...);.

  • Michael Ficarra

    Michael Ficarra August 3rd, 2010 @ 05:23 PM

    • Tag set to element.event

    Looks like you were almost right. It's not actually the fact that the length was shortened, since that is calculated before looping through the array. The array forEach checks "if (i in this)" through each iteration (http://github.com/mootools/mootools-core/blob/master/Source/Core/Co.... So now, the item indexed by 1 will be indexed by 0, and 1 will not exist in this.

    However, I am not convinced that the events are stored in arrays. If the events are stored in an object, mootools will be using a for...in loop to iterate over the events. In this case (removing a property of an object as it is iterated over), the behaviour is actually undefined.

    After looking at the Element.Events source (http://github.com/mootools/mootools-core/blob/master/Source/Element..., I see that it is, in fact, stored in an array, so the reasoning I mentioned first applies here.

  • Ian Wilson

    Ian Wilson August 4th, 2010 @ 04:10 AM

    • Tag changed from element.event to class.extras, element.event

    You are right that I am wrong with my logic that the loop ending prematurely because of the length was the problem. Cloning should still solve the actual problem though as you described it. Also this problem should exist in both Element.event and the Events mixin in class extras.

    Same behavior with mixin --

    http://jsfiddle.net/hAuKv/6/

  • Michael Ficarra

    Michael Ficarra August 4th, 2010 @ 05:43 AM

    Yes, cloning the array should be a good solution. Do you have a patch?

  • Ian Wilson
  • Michael Ficarra

    Michael Ficarra August 4th, 2010 @ 07:01 AM

    Simple. Just run the slickspec test suite. Visit mootools-core/Specs/index.html in a web browser and click on "1.2public + 1.3public". This will ensure that your patch didn't break any of the current tests (which it doesn't appear that it will). Also, another test should be written that specifically tests the patch so that we can be sure it fixes it and so that there is not a regression some time in the future. Remember to initialize the submodules before attempting this if you haven't already.

  • Sebastian Markbåge

    Sebastian Markbåge August 4th, 2010 @ 11:22 AM

    • State changed from “new” to “open”
  • Ian Wilson

    Ian Wilson August 4th, 2010 @ 06:38 PM

    Well it turns out there is a test in 1.2 for both the Element.Event and Mixin that expects listeners being fired to be able to modify the list of listeners immediately. I can't imagine a use-case for this behavior whereas the bug this ticket addresses seems much more likely to occur in everyday usage. The test manages to avoid triggering the current problem because the event that is removed is the last event in the array. If backwards compatibility is important then the list of events must be allowed to have "holes" in it until the firing is over and then it would need to be consolidated. I can't imagine a way of doing that which isn't fragile. If removeEvent set's slots to null and fireEvent using erase(null) then things might work as long as fireEvent is not fired with the same event during another fireEvent for that event. Anyways that is how I understand it, please correct me if I am wrong. My vote is to remove this test and replace it with a test for this ticket's bug. How should I proceed?

    Here is that test which no longer passes inlined here, its for the Events mixin.

          'should remove an event immediately': function(){
                var object = createObject();
    
                var methods = [];
    
                var three = function(){
                    methods.push(3);
                };
    
                object.addEvent('event', function(){
                    methods.push(1);
                    this.removeEvent('event', three);
                }).addEvent('event', function(){
                    methods.push(2);
                }).addEvent('event', three);
                
                object.fireEvent('event');
                value_of(methods).should_be([1, 2]);
    
                object.fireEvent('event');
                value_of(methods).should_be([1, 2, 1, 2]);
            }
    
  • Ian Wilson

    Ian Wilson August 6th, 2010 @ 04:44 PM

    Here is a test that would confirm the new functionality is working.

    @@@javascript

        'should clone events at start of fireEvent': function(){
            var object = createObject();
    
            var methods = [];
    
            var one = function(){
                object.removeEvent('event', one);
                methods.push(1);
            };
            var two = function(){
                object.removeEvent('event', two);
                methods.push(2);
            };
            var three = function(){
                methods.push(3);
            };
    
            object.addEvent('event', one);
            object.addEvent('event', two);
            object.addEvent('event', three);
    
            object.fireEvent('event');
            value_of(methods).should_be([1, 2, 3]);
    
            object.fireEvent('event');
            value_of(methods).should_be([1, 2, 3, 3]);
        }
    

    @@@@

  • Christoph Pojer

    Christoph Pojer September 4th, 2010 @ 11:33 AM

    • Milestone set to 1.3.0 rc2
    • Assigned user set to “Valerio”
    • Milestone order changed from “786” to “0”
  • Christoph Pojer

    Christoph Pojer September 4th, 2010 @ 05:16 PM

    Fixed in 1.3.

    Thank you for your reports. Please note that in IE6 and IE7 (attachEvent) events on DOM-Elements may fire out of order.

  • Christoph Pojer

    Christoph Pojer September 4th, 2010 @ 05:17 PM

    • State changed from “open” to “resolved”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Pages