destroy() method for Quill instances

[leoasis]

This PR tackles #138.

This is the core destroy implementation, it still doesn't clean up modules, but as you can see it will call the destroy method on them if they define it.
I've reached a point where we need to define a couple of things.
First, we need to decide what the state of the container must be like after destroying. Should we empty it? Should we reset it to the original html? Should we leave the html as it was at the moment of destroying?
Second, you will notice that I still have a commented test that verifies that we're removing the "focus" event listener on the iframe container in the Editor. Since there is no off method implemented in the dom util, we need to have something for this. I have two ideas:

1. Implement off by tracking the real listener and the wrapped listener in the Wrapper class, and calling removeEventListener with the same function that addEventListener was called with. For this to work we need to use the same instance of the Wrapper class, so things like dom(elem).on('focus', something); dom(elem).off('focus', something) won't work.
2. Change the on method to return a function that will remove the listener, instead of returning the Wrapper instance. This is very helpful since you don't need to store all the listeners in a separate variable in order to reference the same function when calling off. Here, off will be called by you with the correct listener when you call the returning function from on. Also, I think there are no cases of on being chained with other methods, so this shouldn't break existing usage.

Let me know what you think! Open to more ideas or things I've missed!

[jhchen]

It's fine to use the lodash bind but can you keep the this.focus? I prefer to refer to functions with this and variables with ampersand

[jhchen]

Cool thanks for the PR. Sorry for the style changes but the codebase should maintain consistent code conventions. I'll also add a style guide soon so this can be easier in the future.

One option is upon destruction set the iframe container to the html contents of the editor inside the iframe. This would solve most of the event listener problems since all the listening dom nodes would get destroyed. The exception is the focus on the iframe container you have a test for but that could perhaps be changed to be attached to the iframe itself instead.

[leoasis]

Hey, no worries on enforcing styles! I agree on making those changes, so no problem :D

Regarding moving the focus listener to the iframe, I'll check if that makes the tests pass and test it a little bit manually to see if that works.

[leoasis]

Seems moving the focus event listener to the iframe is something IE does not like

[jhchen]

Sorry for the delay. I was debating the best solution to the focus listener issue and think it might be that actually we can just pass in this.focus to the focus listener and thus that can just be passed into removeEventListener. To stay consistent though we should create a dom function off that is right now just a fallthrough to removeEventListener. Can you add this to a separate PR with a simple test?

[leoasis]

Since we're passing _.bind(this.focus, this), as the listener, we need to keep a reference to that function so that we can call off on that. Anyway that's not the problem really, the real problem is that the function that actually gets added as a listener is created inside the dom.on method, which wraps the function you pass to it. If we call off which just falls through to removeEventListener without that into account, it won't remove anything.

Perhaps I didn't understand correctly what you meant! So let me know if that's the case!

[jhchen]

I think the _.bind() call is unnecessary so if it's changed to dom(@iframeContainer).on('focus', this.focus) then later reference this.focus in the event removal step.

[leoasis]

We DO need the bind call, since the focus function is using this internally. Anyway even if we didn't, that wouldn't work since dom.on internally attaches another function, which we don't have a reference to remove later on: https://github.com/quilljs/quill/blob/develop/src/lib/dom.coffee#L125-L132

[jhchen]

Hmm okay forget dom.on attaches an anonymous function. What's the issue with IE? I'm not seeing any obvious errors..

[leoasis]

You're right, it seems the editor is not losing focus on IE when the input is being focused. It doesn't seem to be related to this though, at least not in an obvious way.

[jhchen]

I'm not sure what you mean.. what input are you talking about?

[leoasis]

The one in the failing test https://travis-ci.org/quilljs/quill/jobs/37244733#L305

[jhchen]

Sorry the latest release added a lot of changes and conflicts to this PR. There's no more focus listener so that issue has taken care of itself. The failing focus test might also have fixed itself as there were some focus issues that have been fixed in the latest release.

If you can resolve the merge conflicts we can give this another try.

[leoasis]

Awesome! I'll resolve the conflicts and make this work

On Friday, November 7, 2014, Jason Chen [email protected] wrote:

Sorry the latest release added a lot of changes and conflicts to this PR.
There's no more focus listener so that issue has taken care of itself. The
failing focus test might also have fixed itself as there were some focus
issues that have been fixed in the latest release.

If you can resolve the merge conflicts we can give this another try.

—
Reply to this email directly or view it on GitHub
#223 (comment).

[leoasis]

@jhchen check it now, resolved the conflicts and made this work again

[jhchen]

Awesome thanks!