fix(typeahead): dangling event listeners #4636

oliversalzburg · 2015-10-15T17:22:58Z

Foxandxss · 2015-10-15T18:03:03Z

Appreciated, but I would like to see tests.

icfantv · 2015-10-15T18:15:10Z

@Foxandxss, before adding that comment, I looked and we don't even have tests to check to see that any event listeners are added which is why I didn't ask @oliversalzburg to add them. The onus shouldn't be on him to create tests for checking the adding and removing of all the event listeners. If you're ok with him just adding checks to see that they are removed, I'm ok with that as well. But note that we don't even have a framework in place yet for checking them at all. How do you want to proceed?

Foxandxss · 2015-10-15T18:24:22Z

The responsibility to add tests is for the person who creates a PR.

In this case, creating a typeahead with the append to body, check that there are some events registered ( I guess that is possible) and then kill it and see if those events are gone.

icfantv · 2015-10-15T18:28:50Z

@oliversalzburg, please add a test to check that those two event handlers are removed once the typeahead is destroyed. Thanks. It should be self-explanatory, but let us know if you need help figuring out how to do that.

oliversalzburg · 2015-10-15T18:49:07Z

Sure thing. I'll work on those tests. It might take me a while, since the weekend is approaching.

I just wanted to put the PR out there to be able to discuss it :)

Thanks for the feedback.

oliversalzburg · 2015-10-19T10:47:56Z

I'd appreciate some help with these tests. There are confusing things going on here :P

Right now, both tests fail for me:

Chrome 45.0.2454 (Windows 8.1 0.0.0) typeahead tests event listeners should register event listeners when attached to body FAILED
        Expected spy addEventListener to have been called with [ 'resize', <jasmine.any(Function)>, false ] but it was never called.
            at Object.<anonymous> (d:/bootstrap/src/typeahead/test/typeahead.spec.js:1023:39)
        Expected spy addEventListener to have been called with [ 'scroll', <jasmine.any(Function)>, false ] but it was never called.
            at Object.<anonymous> (d:/bootstrap/src/typeahead/test/typeahead.spec.js:1024:46)
Chrome 45.0.2454 (Windows 8.1 0.0.0) typeahead tests event listeners should remove event listeners when attached to body FAILED
        Expected spy removeEventListener to have been called with [ 'resize', <jasmine.any(Function)> ] but it was never called.
            at Object.<anonymous> (d:/bootstrap/src/typeahead/test/typeahead.spec.js:1033:42)
        Expected spy removeEventListener to have been called with [ 'scroll', <jasmine.any(Function)> ] but it was never called.
            at Object.<anonymous> (d:/bootstrap/src/typeahead/test/typeahead.spec.js:1034:49)
Chrome 45.0.2454 (Windows 8.1 0.0.0): Executed 1014 of 1014 (2 FAILED) (25.43 secs / 25.22 secs)

When I remove the append to body tests (typeahead.spec.js:872), the first test will succeed. I don't understand how that test is affecting mine.

Secondly, the test for removeEventListener always fails. It appears that $destroy is never triggered in the test.

Also, sorry for the whitespace changes, I'll correct those once I have working tests. WebStorm will just mess this up again while I'm working on the files.

icfantv · 2015-10-19T16:12:45Z

src/typeahead/test/typeahead.spec.js

@@ -1049,15 +1071,15 @@ describe('typeahead deprecation', function() {
    expect($log.warn.calls.argsFor(1)).toEqual(['typeahead is now deprecated. Use uib-typeahead instead.']);
    expect($log.warn.calls.argsFor(2)).toEqual(['typeahead-popup is now deprecated. Use uib-typeahead-popup instead.']);
  }));
-  
+


please watch your formatting and only touch lines relevant to your code changes.

Also, sorry for the whitespace changes, I'll correct those once I have working tests. WebStorm will just mess this up again while I'm working on the files.

This is not quite what I had in mind when I was asking for help with the tests ;)

I know. I admit my karma/jasmine is a bit weak and would require the same investigation as you do to ensure the event listeners are being added/removed appropriately.

Well, then I'm going to update the PR with the whitespace fixes and hope that someone can shed some light on this issue later.

icfantv · 2015-10-19T16:28:01Z

@oliversalzburg, why not test to see if a flag in your event handler has been toggled (or something) when the event has been fired and not toggled when it's been removed and the event has been fired? Will that work?

wesleycho · 2015-10-19T16:29:25Z

src/typeahead/typeahead.js

@@ -359,6 +359,10 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position'])
      if (appendToBody || appendToElementId) {
        $popup.remove();
      }
+      if (appendToBody) {


Add empty line above this line

oliversalzburg · 2015-10-19T16:31:43Z

@icfantv As I said in my previous post, the spies work right when the other "append to body" tests aren't run. So I'm pretty confident in the approach I just don't understand how the other test is affecting it.

And the second test fails because the handler for $destroy is never triggered, so the listeners actually are not removed and the test fails as it should.

wesleycho · 2015-10-19T16:33:50Z

Not sure why the tests would fail, probably would need to look into it

icfantv · 2015-10-19T16:37:18Z

@oliversalzburg, did you mean line 872 for the append to body test?

icfantv · 2015-10-19T16:40:11Z

At first glance, I don't see how/why that test would cause yours to fail - each describe() is a separate unit and there shouldn't, in theory, be any spillover.

oliversalzburg · 2015-10-19T17:07:16Z

The code style and whitespace issues should be fixed now.

@icfantv Yes, the tests starting on line 872 are what I was referring to (there was a typo in my other post).

oliversalzburg · 2015-10-19T17:22:27Z

I guess the answer is actually pretty simple.

The previous tests cause the event listeners to be registered. jQuery remembers those listeners.
Because $destroy is never invoked, the listeners are not removed.
When my test runs, jQuery will not invoke addEventListener, because the listeners are already attached.

So it all boils down to, why is there no $destroy?

oliversalzburg · 2015-10-19T17:44:19Z

angularjs: why isn't $destroy triggered when I call element.remove? sounds pretty relevant…

wesleycho · 2015-10-19T17:52:28Z

Perhaps instead of element.remove, you should take the $scope and execute $scope.$destroy()

oliversalzburg · 2015-10-19T18:20:15Z

@wesleycho But the other tests aren't causing the scope to be destroyed (or the $destroy event being emitted from it). So, calling $scope.$destroy() in my tests won't have the desired effect.

When I change originalScope.$on('$destroy', fn) to element.on('$destroy', fn), the handler is invoked and all tests are still passing. Well, except my test that checks for removeEventListener being called. I'm not quite sure why that is, but at least the surrounding code then works as expected.

Foxandxss · 2015-10-19T19:11:41Z

I fixed and merged it.

oliversalzburg · 2015-10-19T19:42:48Z

@Foxandxss Thanks a lot for taking care of this!

However, clearing the listeners manually in the tests doesn't seem like the right approach IMHO.

In general, it seems like $destroy is not broadcast from the scope when the DOM element is removed. You can observe this in http://plnkr.co/edit/XBWPaITD4v9RY29FwR0U?p=preview In fact, due to this behavior, when using jQuery's .remove(), my originally reported issue is still present, as the event listeners are not removed.

As mentioned in angularjs: why isn't $destroy triggered when I call element.remove?, the $destroy event is being emitted from the DOM nodes themselves in this case. Because the typeahead directive does not account for this case, it can still leak event listeners.

The docs even note:

Note that, in AngularJS, there is also a $destroy jQuery event, which can be used to clean up DOM bindings before an element is removed from the DOM.

I would propose that the event should be respected and the listeners should be cleaned up.

Foxandxss · 2015-10-19T19:48:12Z

I am sorry, but I am bit confused.

When you test something, those objects will be created and disappear after each test, but if you bound some of the to the body, they will remain in there for all the eternity.

In the describe for append to body, it is always better to make a cleanup after each test (Perhaps I would even need to delete the element entirely).

For you concrete tests, first one works after you clean the previous ones (as you said, jquery was not adding new listeners because they were there yet). Second one works as soon as your start clean and you trigger $destroy.

I don't see any issue with this approach, but, if you think there is a better way, I am open for it.

oliversalzburg · 2015-10-19T19:59:10Z

Well, the tests aren't accurately testing the directive. The directive is supposed to clean up after itself. But it doesn't. You're cleaning up the event listeners yourself in the test.

That being said, there is still a real issue in the code. If the DOM element (or any of its parents) is removed via jQuery (as it is done in the tests), the event listeners will not be cleaned up, because the $destroy event is not handled. In fact, none of the cleanup code is run in that case.

Foxandxss · 2015-10-19T20:04:20Z

About the first point, yes, the directive is cleaning itself correctly.

  describe('event listeners', function() {
    it('should register event listeners when attached to body', function() {
      spyOn(window, 'addEventListener');
      spyOn(document.body, 'addEventListener');

      var element = prepareInputEl('<div><input ng-model="result" uib-typeahead="item for item in source | filter:$viewValue" typeahead-append-to-body="true"></div>');

      expect(window.addEventListener).toHaveBeenCalledWith('resize', jasmine.any(Function), false);
      expect(document.body.addEventListener).toHaveBeenCalledWith('scroll', jasmine.any(Function), false);
      $scope.$destroy();
    });

    it('should remove event listeners when attached to body', function() {
      spyOn(window, 'removeEventListener');
      spyOn(document.body, 'removeEventListener');

      var element = prepareInputEl('<div><input ng-model="result" uib-typeahead="item for item in source | filter:$viewValue" typeahead-append-to-body="true"></div>');
      $scope.$destroy();

      expect(window.removeEventListener).toHaveBeenCalledWith('resize', jasmine.any(Function), false);
      expect(document.body.removeEventListener).toHaveBeenCalledWith('scroll', jasmine.any(Function), false);
    });
  });

If you change your tests like that. No manual cleanup it works. If you comment out the $destroy on the first test, the second one won't work (because we have "stale" code in the dom).

About the second point, not sure if I follow. If a $destroy is triggered in the parent, all the children will die too.

oliversalzburg · 2015-10-19T20:09:56Z

I'm really not sure how to make this clearer than I already have with the provided explanation, links to the Stack Overflow question, the AngularJS documentation and the Plunker.

So, in the interest of saving everyone's time, I'll just move on.

icfantv added component: typeahead needs: review labels Oct 15, 2015

icfantv added the needs: PR test(s) label Oct 15, 2015

wesleycho added this to the 0.14.3 milestone Oct 19, 2015

icfantv reviewed Oct 19, 2015
View reviewed changes

wesleycho reviewed Oct 19, 2015
View reviewed changes

wesleycho added needs: investigation needs: work and removed needs: PR test(s) needs: review labels Oct 19, 2015

fix(typeahead): dangling event listeners

68619c7

Foxandxss closed this in 94fb282 Oct 19, 2015

oliversalzburg deleted the fix/typeahead-binds branch October 20, 2015 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typeahead): dangling event listeners #4636

fix(typeahead): dangling event listeners #4636

oliversalzburg commented Oct 15, 2015

Foxandxss commented Oct 15, 2015

icfantv commented Oct 15, 2015

Foxandxss commented Oct 15, 2015

icfantv commented Oct 15, 2015

oliversalzburg commented Oct 15, 2015

oliversalzburg commented Oct 19, 2015

icfantv Oct 19, 2015

oliversalzburg Oct 19, 2015

icfantv Oct 19, 2015

oliversalzburg Oct 19, 2015

icfantv commented Oct 19, 2015

wesleycho Oct 19, 2015

oliversalzburg commented Oct 19, 2015

wesleycho commented Oct 19, 2015

icfantv commented Oct 19, 2015

icfantv commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

wesleycho commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

fix(typeahead): dangling event listeners #4636

fix(typeahead): dangling event listeners #4636

Conversation

oliversalzburg commented Oct 15, 2015

Foxandxss commented Oct 15, 2015

icfantv commented Oct 15, 2015

Foxandxss commented Oct 15, 2015

icfantv commented Oct 15, 2015

oliversalzburg commented Oct 15, 2015

oliversalzburg commented Oct 19, 2015

icfantv Oct 19, 2015

Choose a reason for hiding this comment

oliversalzburg Oct 19, 2015

Choose a reason for hiding this comment

icfantv Oct 19, 2015

Choose a reason for hiding this comment

oliversalzburg Oct 19, 2015

Choose a reason for hiding this comment

icfantv commented Oct 19, 2015

wesleycho Oct 19, 2015

Choose a reason for hiding this comment

oliversalzburg commented Oct 19, 2015

wesleycho commented Oct 19, 2015

icfantv commented Oct 19, 2015

icfantv commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

wesleycho commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015

Foxandxss commented Oct 19, 2015

oliversalzburg commented Oct 19, 2015