Opened 11 years ago

Closed 11 years ago

#2927 closed Bug (fixed)

CKEDITOR.event duplicate dom event registration

Reported by: Garry Yao Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Review+ Cc:

Description (last modified by Garry Yao)

Event delegation registration with CKEDITOR.event.implementOnis currently incorrect, when this method is invoked on prototype object of a constructor, then instances of this constructor will share the same private _.events model, which is wrong. This would result in duplicate invocation of event listeners from two different object.

Functonal TestCase

Tested with test-events-duplicate.path

Attachments (4)

test-events-duplicate.patch (1.3 KB) - added by Garry Yao 11 years ago.
Functional testcase
2927.patch (3.0 KB) - added by Garry Yao 11 years ago.
2927_2.patch (3.6 KB) - added by Garry Yao 11 years ago.
test-events-duplicate_2.patch (1.9 KB) - added by Garry Yao 11 years ago.
Updated test case.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by Garry Yao

Keywords: Confirmed added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 11 years ago by Garry Yao

Description: modified (diff)

Reduced reason was found, update description accordingly.

Changed 11 years ago by Garry Yao

Attachment: test-events-duplicate.patch added

Functional testcase

Changed 11 years ago by Garry Yao

Attachment: 2927.patch added

comment:3 Changed 11 years ago by Garry Yao

Keywords: Review? added

Contained in this patch:

  • An update to CKEDITOR.event.implementOn for special logic when imposing on constructor prototype;
  • An update to CKEDITOR.event constructor to preserve existed events( e.g., multiple CKEDITOR.dom.domObject on single naive HTMLElement).

comment:4 Changed 11 years ago by Martin Kou

Very good observation.

There's still one problem with the patch though. Most of the code _source/core/event.js assumes the existence of this._.events. Now that we know putting the _.events objects via prototypes is wrong, so we aren't putting the object into the prototype. A side effect to the fix, however, is that it is now possible for objects extended by CKEDITOR.event.implementOn(prototype, true) to have no _.events at all - which is also wrong.

To demonstrate that, a simple JavaScript snippet for Firebug will show:

function con()
{
 this._ = {};
}
con.prototype = { sayhi: function(){ alert('hi'); } };

CKEDITOR.event.implementOn(con.prototype, true);

var obj = new con();
obj.fire( 'e' );

The last line will raise a JavaScript error, because CKEDITOR.event::fire() assumes the existence of this._.events.

So I'd recommend you add a little override function for all those event functions who depend on the existence of this._.events. So for the first time they're run for an object, they should check for this._.events and create it if it's not there. After the first run, the override code should assign back the ordinary event functions to the object such that the override code would not be unnecessarily executed in subsequent runs.

comment:5 Changed 11 years ago by Martin Kou

Keywords: Review- added; Review? removed

Changed 11 years ago by Garry Yao

Attachment: 2927_2.patch added

Changed 11 years ago by Garry Yao

Updated test case.

comment:6 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Thanks for Martin's use-case,though this's a rare case since the the following codes should always be used together to 'inherit' from CKEDITOR.eventactually:

			function myClass()
			{
				CKEDITOR.event.call( this );
			}
			CKEDITOR.event.implementOn( myClass.prototype, true );

But considering the integrity of API, I also updated the patch to support this use-case.

comment:7 Changed 11 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:8 Changed 11 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3119].

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