Opened 6 years ago

Closed 6 years ago

#7588 closed Bug (fixed)

dialog semi-transparent background div created twice

Reported by: kkez Owned by: Frederico Caldeira Knabben
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 (4)

7588.patch (867 bytes) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Proposed patch
7588_2.patch (5.6 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Revised patch
7588_3.patch (6.4 KB) - added by Alfonso Martínez de Lizarrondo 6 years ago.
Updated patch
7588_4.patch (706 bytes) - added by Frederico Caldeira Knabben 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 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 6 years ago by Alfonso Martínez de Lizarrondo

Owner: set to Alfonso Martínez de Lizarrondo
Status: newreview
Version: 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 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7588.patch added

Proposed patch

comment:3 Changed 6 years ago by Wiktor Walc

Status: reviewreview_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 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7588_2.patch added

Revised patch

comment:4 Changed 6 years ago by Alfonso Martínez de Lizarrondo

Status: review_failedreview

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 6 years ago by Wiktor Walc

Status: reviewreview_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 6 years ago by Alfonso Martínez de Lizarrondo

Attachment: 7588_3.patch added

Updated patch

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

Status: review_failedreview

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 6 years ago by Frederico Caldeira Knabben

Attachment: 7588_4.patch added

comment:7 Changed 6 years ago by Frederico Caldeira Knabben

Owner: changed from Alfonso Martínez de Lizarrondo to Frederico Caldeira Knabben

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 6 years ago by Wiktor Walc

Milestone: CKEditor 3.5.4
Status: reviewreview_passed

comment:9 Changed 6 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [6815].

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