Ticket #7588 (closed Bug: fixed)

Opened 3 years ago

Last modified 3 years ago

dialog semi-transparent background div created twice

Reported by: kkez Owned by: fredck
Priority: Normal Milestone: CKEditor 3.5.4
Component: UI : Dialogs Version: 3.5.1
Keywords: Cc:

Description

This is happening since 3.5.1. 3.5 didn't have this problem and neither did 3.4.x.

How to reproduce:

1) include ckeditor.js: <script type="text/javascript" src="ckeditor/ckeditor.js"></script>

2)use ckeditor php class to create the editor, or include ckeditor.js again and then create the editor via javascript.

3) click on a toolbar button that opens a dialog, eg. "link" (note: the bug happens now, you can see that two background div are created)

4) close the dialog

5) since there are two background div, the second one remains and covers everything.

Attachments

7588.patch (867 bytes) - added by alfonsoml 3 years ago.
Proposed patch
7588_2.patch (5.6 KB) - added by alfonsoml 3 years ago.
Revised patch
7588_3.patch (6.4 KB) - added by alfonsoml 3 years ago.
Updated patch
7588_4.patch (706 bytes) - added by fredck 3 years ago.

Change History

comment:1 Changed 3 years ago by kkez

I know that I shouldn't include ckeditor.js twice, but i need to create the editor in php and sometimes in javascript on the same page, and i can't detect in php whether ckeditor.js was already included.

comment:2 Changed 3 years ago by alfonsoml

  • Status changed from new to review
  • Owner set to alfonsoml
  • Version set to 3.5.1

People have been reporting this kind of problems in the forums since 3.5.1 but most of the reports are too vague.

The key is to add twice the ckeditor.js file, but use the release version, in the development version it works fine.

The fix is easy enough that I think that it should be targeted for 3.5.4

Changed 3 years ago by alfonsoml

Proposed patch

comment:3 Changed 3 years ago by wwalc

  • Status changed from review to review_failed

When using the compressed version of CKEditor I get the following error:

k.dialog is undefined
[Break On This Error] (131 out of range 14)
ckeditor.js (line 131)
uncaught exception: [CKEDITOR.resourceManager.load] Resource name "panel" was not found at "http://127.0.0.1/ckeditor_trunk/_dev/releaser/release/release/plugins/panel/plugin.js?t=B3J855O".

tested on replacebyclass sample with:

<script type="text/javascript" src="../ckeditor.js"></script>
<script type="text/javascript" src="../ckeditor.js"></script>

Changed 3 years ago by alfonsoml

Revised patch

comment:4 Changed 3 years ago by alfonsoml

  • Status changed from review_failed to review

I don't know why it seemed to work in my previous check, I guess that I might have tested something else.

The new patch looks scary, but it's just putting the CKEDITOR.ui creation in an anonymous function and exit if it has already been initalized.

comment:5 Changed 3 years ago by wwalc

  • Status changed from review to review_failed

I got the following error in Firefox when running the compressed version (replacebyclass with two ckeditor.js), after opening the Link dialog:

this.$ is undefined

Changed 3 years ago by alfonsoml

Updated patch

comment:6 Changed 3 years ago by alfonsoml

  • Status changed from review_failed to review

This one was trickier because the error depended on the execution order, but the problem was due to the recreation of CKEDITOR.tools and the addFunction that used a different array the second time, so this was a big problem that might have been the reason for some strange bugs in the past (currently people couldn't use CKEditor if they included ckeditor.js twice due to this ticket)

Changed 3 years ago by fredck

comment:7 Changed 3 years ago by fredck

  • Owner changed from alfonsoml to fredck

I'm proposing a simpler approach to this.

Considering that the editor script is included inside a (function(){...})() when packaging, I'm just using the packager features to check whether the editor is loaded, avoiding processing ckeditor.js entirely, if is has already been loaded.

This will fix the release version. For the source version this is not needed because the loader already checks whether a script has been loaded or not.

comment:8 Changed 3 years ago by wwalc

  • Status changed from review to review_passed
  • Milestone set to CKEditor 3.5.4

comment:9 Changed 3 years ago by fredck

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

Fixed with [6815].

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