Opened 14 years ago
Closed 14 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 )
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)
Change History (12)
comment:1 Changed 14 years ago by
Keywords: | Confirmed added |
---|---|
Owner: | set to Garry Yao |
Status: | new → assigned |
comment:2 Changed 14 years ago by
Description: | modified (diff) |
---|
Changed 14 years ago by
Attachment: | 2927.patch added |
---|
comment:3 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
Keywords: | Review- added; Review? removed |
---|
Changed 14 years ago by
Attachment: | 2927_2.patch added |
---|
comment:6 Changed 14 years ago by
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 14 years ago by
Keywords: | Review+ added; Review? removed |
---|
Reduced reason was found, update description accordingly.