Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#10219 closed Bug (fixed)

editor.destry() throw an error

Reported by: Charlie Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.1.1
Component: General Version: 4.0.2
Keywords: IBM Cc: monahant@…

Description

<button onclick="javascript:CKEDITOR.instances.editor1.destroy();">Click</button>

I add this code in the inline editing demo page.

it throw an error.

bug condition is do not click edit area before destry().

if the edit area once actived, destry action will not throw error. or we put the destry action in window.setTimeout, it will neither throw error.

Change History (21)

comment:1 Changed 7 years ago by Charlie

I tested version 4.0.1.1, it didn't throw error.

comment:2 Changed 7 years ago by Piotrek Koszuliński

Status: newconfirmed

It's reproducible on 4.0.2 and 4.1; works fine on 4.0.1.

comment:3 Changed 7 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.0.3

comment:4 Changed 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

Issue introduced by git:b862b5f.

comment:5 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/10219.

comment:6 Changed 7 years ago by Piotrek Koszuliński

PS. I forgot to mention TC:

  • Open inlineall sample.
  • Destroy one of inline editors.
  • Click any of the editors.

On mouseup error is thrown.

Last edited 7 years ago by Piotrek Koszuliński (previous) (diff)

comment:7 Changed 7 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:8 Changed 7 years ago by Piotrek Koszuliński

We found that magicline may unsafely execute few functions on every switch to wysiwyg mode. However, only one of these calls caused issue and it is fixed in this ticket. Since we couldn't find a TCs for the rest, but also we don't want to rewrite part of magicline now, I reported #10227.

comment:9 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on git:a7b117f.

comment:10 in reply to:  9 Changed 7 years ago by Teresa Monahan

Cc: monahant@… added
Keywords: IBM added

Replying to Reinmar:

Fixed on git:a7b117f.

I can still reproduce this. The error does not occur if I destroy the editor instance through the browser console. However it does occur if I include this button in the inlineall.html sample and click it:

<button onclick="CKEDITOR.instances.editor1.destroy();">Click</button>

The error is: TypeError: sel is null /ckeditor/plugins/clipboard/plugin.js Line 906

comment:11 Changed 7 years ago by Jakub Ś

Resolution: fixed
Status: closedreopened

I can confirm this is still happening. Opening page with button and clicking it without activating editor results in error.

comment:12 Changed 7 years ago by Jakub Ś

Status: reopenedconfirmed

comment:13 Changed 7 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.0.3CKEditor 4.1.1

We missed a case, when mouseup is fired at exactly the same moment when destroy() is called. We were too focused on reproducing this issue from code.

Unfortunately we won't be able to include a patch for the second part of this issue in 4.0.3, because it's too late.

I'm targeting this ticket to 4.1.1.

Note that this error should not cause any problems because it is thrown inside setTimeout() in native event listener.

comment:14 Changed 7 years ago by Piotrek Koszuliński

Additional TCs can be find here: #9684 and if not already fixed, should be fixed in this ticket.

comment:15 Changed 7 years ago by Olek Nowodziński

Owner: changed from Piotrek Koszuliński to Olek Nowodziński
Status: confirmedassigned

comment:16 Changed 7 years ago by Olek Nowodziński

Status: assignedreview

Created branch t/10219b with a simple solution that cleans remaining deferred callbacks on editor destroy. It seems to solve all the "click&destroy" cases.

comment:17 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:18 Changed 7 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged into master git:86d354eb6f.

comment:19 Changed 6 years ago by Joe

I am getting the error outlined here #9684 in 4.1.2 it is causing IE 10 to display an error dialog. Why not check if the selection returned is null before calling a method on it, or alternately have getSelection() not return null? Right now the code is not handling all the possible return values [specifically null] coming from getSelection().

I am using CKEDITOR.inline() to start the editor and <instance>.destroy() to remove the editor. I only wanted to document this, we don't need a fix we'll work around it.

comment:20 Changed 6 years ago by tepez

This is not fixed, yet. I had to add an additional check in the timeout that the instance is not yet destroyed.

This happens because the timeout cancellation(on destroy) is triggered after the timeout itself when the instance is destroyed during mouse-down (for me, it happens when an instance is being destroyed during dragging).

If you need, I can create a jsfiddle that shows this error.

editable.attachListener( CKEDITOR.env.ie ? editable : editor.document.getDocumentElement(), 'mouseup', function() {
  mouseupTimeout = setTimeout( function() {
    if (editor.state != 'destroyed') {
      setToolbarStates();
    }
  }, 0 );
});		

comment:21 Changed 6 years ago by Jakub Ś

If you need, I can create a jsfiddle that shows this error.

Please do because I wasn't able to reproduce this problem.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy