Opened 4 years ago

Closed 3 years ago

#13937 closed Bug (expired)

Clearing "mouseup" timeouts on destroy in clipboard plugin

Reported by: Dariusz Plawecki Owned by:
Priority: Normal Milestone:
Component: General Version:
Keywords: Cc:

Description

We found an issue with "mouseup" callback in ckeditor clipboard plugin which is executed after editor had been destroyed. There was already a bug for this issue (#10219), but actually the solution was not a full fix, although the bug has been closed.

The current solution (4.5.x/plugins/clipboard/plugin.js, lines 642-658):

var mouseupTimeout;

// Use editor.document instead of editable in non-IEs for observing mouseup
// since editable won't fire the event if selection process started within
// iframe and ended out of the editor (#9851).
editable.attachListener( CKEDITOR.env.ie ? editable : editor.document.getDocumentElement(), 'mouseup', function() {
	mouseupTimeout = setTimeout( function() {
		setToolbarStates();
	}, 0 );
} );

// Make sure that deferred mouseup callback isn't executed after editor instance
// had been destroyed. This may happen when editor.destroy() is called in parallel
// with mouseup event (i.e. a button with onclick callback) (#10219).
editor.on( 'destroy', function() {
	clearTimeout( mouseupTimeout );
} );

The full solution for this issue should be like this:

var mouseupTimeouts = [];

...
editable.attachListener( CKEDITOR.env.ie ? editable : editor.document.getDocumentElement(), 'mouseup', function() {
	mouseupTimeouts.push(setTimeout( function() {
		setToolbarStates();
	}, 0 ));
} );

...
editor.on( 'destroy', function() {
	for (var i = 0; i < mouseupTimeouts.length; i++) {
		clearTimeout( mouseupTimeouts[i] );
	}
} );

Timeouts are set many times on each "mouseup" event, so they all should be cleared on destroy (not only the last one which is assigned to variable).

Please validate our suggested solution as soon as possible.

Change History (2)

comment:1 Changed 3 years ago by Jakub Ś

Status: newpending
Version: 4.5.4

First of all I'm sorry for late reply.

Now, while this solution makes a lot of sense, we also need a test case which is causing the problem and we haven't been able to reproduce this problem in any way.

Could you perhaps provide such test case? Perhaps a simplified HTML file with JS where mouse up is registered more than once?

comment:2 Changed 3 years ago by Jakub Ś

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