Opened 11 years ago

Closed 11 years ago

Last modified 10 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 11 years ago by Charlie

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

comment:2 Changed 11 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 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.0.3

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

Issue introduced by git:b862b5f.

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

Status: assignedreview

Pushed t/10219.

comment:6 Changed 11 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 11 years ago by Piotrek Koszuliński (previous) (diff)

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

Status: reviewreview_passed

comment:8 Changed 11 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 11 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on git:a7b117f.

comment:10 in reply to:  9 Changed 11 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 11 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 11 years ago by Jakub Ś

Status: reopenedconfirmed

comment:13 Changed 11 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 11 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 11 years ago by Olek Nowodziński

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

comment:16 Changed 11 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 11 years ago by Piotrek Koszuliński

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Merged into master git:86d354eb6f.

comment:19 Changed 11 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 10 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 10 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy