Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove call to CKEDITOR.removeAllListeners() #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

closesimple-mf
Copy link

@closesimple-mf closesimple-mf commented Oct 24, 2022

Hello,

I believe we found a bug in this library a while back that was introduced in 5ada62c

We have a custom ckeditor plugin that replaces references like ${thisOne} with a fancy looking 'placeholder' you can click on to open a model. Pressing backspace deletes the entire placeholder at once.

After upgrading this library we noticed you could no longer delete the placeholder, and the cursor would jump to the bottom when pressing backspace:

aa

As well as console errors:
aaa

I tracked it down to the call to CKEDITOR.removeAllListeners(). What I believe was happening was that if a ckeditor instance got destroyed on the page, it would remove all listeners off of all ckeditor instances, and screwed up ck's plugin system somehow.

I debugged in to ckeditor's source and saw the call this library does to this.instance.destroy(); should be sufficient for the cleanup. destroy() calls this.removeAllListeners(), removing its own listeners and not other ckeditor instance listeners:

aaaa

We've been running with this change in production for a while now.

Thanks!

This screws up widget placeholder deletion in subsequent editors that are created -
possibly because the new editor gets created first, then this gets called, as this
is run async in a zone.

        this.instance.destroy(); is good enough.  I looked in to it in ckeditor and it
calls this.removeAllListeners(), removing its own listeners and not other ckeditor
instance listeners
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant