Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#4797 closed Bug (fixed)

Opera: using enter key in dialogs triggers javascript error

Reported by: wwalc Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0.1
Keywords: Confrmed Opera Cc: alfonsoml

Description (last modified by wwalc)

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 m.nguyen 6 years ago.
4797_2.patch (1.1 KB) - added by garry.yao 6 years ago.
4797_3.patch (1.7 KB) - added by garry.yao 6 years ago.
4797_4.patch (6.1 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by wwalc

  • Description modified (diff)
  • Summary changed from Opera: using enter key in dialogs causing various issues to Opera: using enter key in dialogs triggers javascript error

Changed 6 years ago by m.nguyen

comment:2 Changed 6 years ago by m.nguyen

  • Keywords Review? added
  • Owner set to m.nguyen
  • Status changed from new to assigned

comment:3 Changed 6 years ago by garry.yao

Could you explain what's happening here?

comment:4 Changed 6 years ago by m.nguyen

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

comment:5 Changed 6 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 6 years ago by garry.yao

comment:6 follow-up: Changed 6 years ago by garry.yao

  • Cc alfonsoml added
  • Keywords Discussion added

We need hear some voices here.

comment:7 Changed 6 years ago by alfonsoml

  • 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 6 years ago by fredck

  • 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 6 years ago by m.nguyen

  • Owner changed from m.nguyen to new
  • Status changed from assigned to new

Changed 6 years ago by garry.yao

comment:10 Changed 6 years ago by garry.yao

  • Keywords Review? added; Review- removed
  • Owner changed from new to garry.yao
  • Status changed from new to assigned

comment:11 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:12 Changed 6 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [5412].

comment:13 follow-up: Changed 6 years ago by garry.yao

  • Keywords Review+ removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 6 years ago by garry.yao

comment:14 Changed 6 years ago by garry.yao

  • Keywords Review? added

The patch providing the ability for caching dialog covers.

comment:15 Changed 6 years ago by fredck

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 6 years ago by garry.yao

  • Keywords Review? removed
  • Resolution set to fixed
  • Status changed from reopened to closed

#5618 is opened for it.

comment:17 Changed 6 years ago by alfonsoml

This has caused #5688

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