Opened 10 years ago

Closed 10 years ago

#3794 closed Bug (fixed)

Error when creating link

Reported by: Frederico Caldeira Knabben Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed 3.0RC Review+ Cc:

Description

  1. Open the link dialog to create or edit a link.
  2. Type any URL.
  3. Hit ENTER to close the dialog and create/change the link.

A JavaScript error is thrown. It doesn't happen when using the mouse, clicking the dialog OK button.

Attachments (3)

3794.patch (705 bytes) - added by Martin Kou 10 years ago.
3794_2.patch (427 bytes) - added by Martin Kou 10 years ago.
3794_2.2.patch (841 bytes) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 10 years ago by Tobiasz Cudnik

Owner: Tobiasz Cudnik deleted
Status: assignednew

comment:3 Changed 10 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

Changed 10 years ago by Martin Kou

Attachment: 3794.patch added

comment:4 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:5 Changed 10 years ago by Garry Yao

Keywords: Review- added; Review? removed

The patch works, but since the onKeyup is at API level, perhaps we should avoid hacks inside, otherwise we get developer confused, can we lift up 'keyUp' handler prior than 'enterkey/esc' logic?

comment:6 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

I've just discussed with Garry about this. An intuitive way to fix this is to hook all event handlers at dialog show and unhook them all at dialog hide - which means changing the registerEvents() function in dialog plugin to the following:

            var registerDomEvent = function( uiElement, dialog, eventName, func )
            {
                dialog.on( 'show', function()
                {
                    uiElement.getInputElement().on( eventName, func, uiElement );
                } );

                dialog.on( 'hide', function()
                {
                    uiElement.getInputElement().removeListener( eventName, func );
                } );
            };

This, however, does not actually work for this case. With the change, what would happen then becomes this:

  1. keyup event fired in the URL textbox.
  2. keyup handler list was copied in L239 of core/event.js
  3. removed the custom listener from keyup handler list
  4. but since the keyup handler list that event.js is looking at is an independent copy, it doesn't care and continues to call the custom keyup handler.
  5. So you still get a JavaScript error.

So if we're to fix this, we'd need to remove the custom keyup listener before the hide event was fired. This has to be done in the enter key handler of text fields in the dialogui plugin. But that would be a very ugly hack since you're using the dialogui plugin to do what should be done by the dialog plugin.

Another issue is that, if you're going to unhook all event handlers at dialog hide, you'd need to rehook them all at dialog show. This would hurt the dialog system's performance.

So I think we should just stick to the original patch.

Changed 10 years ago by Martin Kou

Attachment: 3794_2.patch added

Changed 10 years ago by Garry Yao

Attachment: 3794_2.2.patch added

comment:7 Changed 10 years ago by Martin Kou

Oops, just figured out a better way to fix it. If I have to fix it at the keyup event in the dialogui plugin, instead of removing the event listener, I can cancel the event.

comment:8 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:9 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3737].

Click here for more info about our SVN system.

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