Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4797 closed Bug (fixed)

Opera: using enter key in dialogs triggers javascript error

Reported by: Wiktor Walc Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0.1
Keywords: Confrmed Opera Cc: Alfonso Martínez de Lizarrondo

Description (last modified by Wiktor Walc)

When pressing enter in the dialog, an error occurs

Steps to reproduce (1)

  • select some text
  • open link dialog
  • type some url and press enter (without moving with a tab to the "ok" button)
  • result:
    Event thread: change
    Error:
    name: TypeError
    message: Statement on line 2255: Cannot convert undefined or null to Object
    stacktrace:   Line 2255 of linked script http://127.0.0.1/ckeditor%203.0.x/_source/plugins/dialog/plugin.js
        			return this.getInputElement().getValue();
      Line 72 of linked script http://127.0.0.1/ckeditor%203.0.x/_source/plugins/dialogui/plugin.js
        function(){ this.fire( 'change', { value : this.getValue() } ); }
      ...  Line 150 of linked script http://127.0.0.1/ckeditor%203.0.x/_source/core/event.js
        						listenerFunction.call( scopeObj, ev );
      ...  Line 245 of linked script http://127.0.0.1/ckeditor%203.0.x/_source/core/event.js
        								var retData = listeners[i].call( this, editor, data, stopEvent, cancelEvent );
      Line 48 of linked script http://127.0.0.1/ckeditor%203.0.x/_source/core/dom/domobject.js
        				domObject.fire( eventName, new CKEDITOR.dom.event( domEvent ) );
      ... 
    

Attachments (4)

4797.patch (635 bytes) - added by Minh Nguyen 8 years ago.
4797_2.patch (1.1 KB) - added by Garry Yao 8 years ago.
4797_3.patch (1.7 KB) - added by Garry Yao 8 years ago.
4797_4.patch (6.1 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by Wiktor Walc

Description: modified (diff)
Summary: Opera: using enter key in dialogs causing various issuesOpera: using enter key in dialogs triggers javascript error

Changed 8 years ago by Minh Nguyen

Attachment: 4797.patch added

comment:2 Changed 8 years ago by Minh Nguyen

Keywords: Review? added
Owner: set to Minh Nguyen
Status: newassigned

comment:3 Changed 8 years ago by Garry Yao

Could you explain what's happening here?

comment:4 Changed 8 years ago by Minh Nguyen

In this bug, Opera try to access url field when popup dialog has hidden.

comment:5 Changed 8 years ago by Garry Yao

We've seen similar bugs before, those are #3794, #4101, etc...
Minh's patch definitely works to eliminate this bug, while I'm smelling something wrong from the dialog hiding logic, [ http://dev.fckeditor.net/browser/CKEditor/trunk/_source/plugins/dialog/plugin.js#L807

where] the element will be detached from DOM tree, this's weird actually, never seen [any http://www.extjs.com/deploy/dev/examples/window/hello.html] other dialog implementation doing that, simply toggle the dialog element gain performance benefit as well.

Changed 8 years ago by Garry Yao

Attachment: 4797_2.patch added

comment:6 Changed 8 years ago by Garry Yao

Cc: Alfonso Martínez de Lizarrondo added
Keywords: Discussion added

We need hear some voices here.

comment:7 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

If you test with the ajax sample you'll see that now every dialog that has been launched will remain hidden in the page body even after the editor is destroyed.

I (mistakenly) thought that the current behavior was to hide the dialogs instead of destroying them, it just makes sense to avoid creating the dom again. I don't know why it's done this way.

comment:8 in reply to:  6 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed

Replying to garry.yao:

We need hear some voices here.

I'm ok to have the dialogs hidden only. I think the original intention was to keep the DOM clean, but we should not have such a drastic impact with it.

Alfonso's notes must be considered though.

comment:9 Changed 8 years ago by Minh Nguyen

Owner: changed from Minh Nguyen to new
Status: assignednew

Changed 8 years ago by Garry Yao

Attachment: 4797_3.patch added

comment:10 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed
Owner: changed from new to Garry Yao
Status: newassigned

comment:11 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:12 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5412].

comment:13 Changed 8 years ago by Garry Yao

Keywords: Review+ removed
Resolution: fixed
Status: closedreopened

The following regression is seen after the fix:

Reproducing Procedures

  1. Load the 'Ajax' sample page and click 'Create Editor' button;
  2. Open any of the the dialog;
  3. Execute the following lines of JavaScript in global scope:
    removeEditor();
    
  4. Click once again 'Create Editor' and open the same dialog;
    • Actual Result: Dialog opened with no viewport cover.

Changed 8 years ago by Garry Yao

Attachment: 4797_4.patch added

comment:14 Changed 8 years ago by Garry Yao

Keywords: Review? added

The patch providing the ability for caching dialog covers.

comment:15 Changed 8 years ago by Frederico Caldeira Knabben

Please DO NOT reuse tickets for regressions, opening new tickets for them, possibly pointing to the changesets that introduced them.

comment:16 in reply to:  13 Changed 8 years ago by Garry Yao

Keywords: Review? removed
Resolution: fixed
Status: reopenedclosed

#5618 is opened for it.

comment:17 Changed 8 years ago by Alfonso Martínez de Lizarrondo

This has caused #5688

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