search

ruhley / angular-color-picker

Vanilla AngularJS Color Picker Directive with no requirement on jQuery
http://ruhley.github.io/angular-color-picker/
MIT License
165 stars 78 forks source link

Correctly remove mouse-event listeners on destroy #109

Closed cahva closed 8 years ago

cahva commented 8 years ago

$document.off and $document.on functions need to have the exact same function so it can remove it correctly. In this case, .bind(this) was used in starting the listener and that became different function in the eyes of event handler and could not be removed on $destroy.

I created one variable holding the mouse-event listener functions that $document.off and $document.on now refers to.

cahva commented 8 years ago

Oh forgot to say that I found out about this issue because our app was using 20+ color-pickers per page and each color-picker registers its own listeners. When navigating our SPA (ui-router), I noticed that it got more slower every time I navigated back and forth. Then I checked devtools eventlisteners and there was lot of color-picker related listeners and I was on page which did not need color-picker.

So every back & forth navigation fired up new mouseup, mousedown etc. events every time and keeped the old ones.

ruhley commented 8 years ago

Hi @cahva,

Thanks for the PR. Before I merge is there a different way that is considered best practice to handle document events? If someone had a page with 100 color pickers on it, then every mouse action (down, up, click, move) would trigger an event for each color picker. I would like to make this library as lean and efficient as possible.

ashconnell commented 8 years ago

@ruhley you could use a single event with some magic checks on the event.target element but this shouldn't be needed. You'd see this kind of implementation on lists with thousands of items etc.

As long as the events are being removed (which this PR is doing) then it should suffice. It's unlikely that people have hundreds of color-pickers all visible at once.

cahva commented 8 years ago

I thought the same yesterday that there must be a better way to listen to events but sadly I have to say I'm not an expert. One thing that came to my mind that when you add the component it should start to listen to the other events(eg. mousemove which I think is the one that eats performance) only when you open the colorpicker. But ofcourse that wont help if you have them inline(=open) in the beginning.

BTW, I upgraded from old version(~1.0.2) and there is clearly a performance hit from that version to the current version. It's almost a rewrite so I don't know how much you have changed the event handling from that old version. Ofcourse the performance hit could be lurking somewhere else, I don't know yet as I haven't dug deeper in the latest code.

ruhley commented 8 years ago

Released in v2.4.6