Opened 8 years ago

Closed 8 years ago

#3717 closed Bug (fixed)

config.dialog_backgroundCoverColor does not work in IE7 or IE8

Reported by: Mike Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.1
Component: UI : Dialogs Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc: mrceolla@…

Description

This is my first bug report for this project. Please let me know if I am doing anything incorrectly.

I have set a custom dialog_backgroundCoverColor in the editor host page before calling CKEDITOR.replace. It works correctly in FF3 but it does not work on IE7 or IE8 with compatibility mode on and off. Here is my script block inside an ASP page:

<script type="text/javascript">

CKEDITOR.config.toolbarLocation = 'Out:xToolbar'; CKEDITOR.config.height = '800px'; CKEDITOR.config.dialog_backgroundCoverColor = 'rgb(255, 0, 0)'; CKEDITOR.config.contentsCss = '../<%=folder%>style.css'; CKEDITOR.config.skin = 'office2003'; CKEDITOR.config.toolbar = [

['Source','-','Save','NewPage','Preview','-','Templates'], ['Cut','Copy','Paste','PasteText','PasteFromWord','-','Print', 'SpellChecker','Scayt'], ['Undo','Redo','-','Find','Replace','-','SelectAll','RemoveFormat'],

['Form', 'Checkbox', 'Radio', 'TextField', 'Textarea', 'Select', 'Button', 'ImageButton', 'HiddenField'],

'/', ['Bold','Italic','Underline','Strike','-','Subscript','Superscript'], ['NumberedList','BulletedList','-','Outdent','Indent','Blockquote'], ['JustifyLeft','JustifyCenter','JustifyRight','JustifyBlock'], ['Link','Unlink','Anchor'], ['Image','Flash','Table','HorizontalRule','Smiley','SpecialChar','PageBreak'], '/', ['Styles','Format','Font','FontSize'], ['TextColor','BGColor'], ['Maximize','ShowBlocks','-','About']

];

CKEDITOR.replace( 'editleftcol', {

filebrowser_browse_url : '/editor/ckfinder/ckfinder.html?action=js&func=SetUrl&thumbFunc=SetUrl', filebrowser_image_browse_url : '/editor/ckfinder/ckfinder.html?action=js&func=SetUrl&thumbFunc=SetUrl&Type=Images', filebrowser_flash_browse_url : '/editor/ckfinder/ckfinder.html?action=js&func=SetUrl&thumbFunc=SetUrl&Type=Flash', filebrowser_upload_url : '/editor/ckfinder/core/connector/php/connector.php?command=QuickUpload&type=Files', filebrowser_image_upload_url : '/editor/ckfinder/core/connector/php/connector.php?command=QuickUpload&type=Images', filebrowser_flash_upload_url : '/editor/ckfinder/core/connector/php/connector.php?command=QuickUpload&type=Flash'

});

CKEDITOR.replace( 'editrightcol');

</script>

Attachments (3)

3717.patch (30.9 KB) - added by Artur Formella 8 years ago.
3717_2.patch (2.5 KB) - added by Artur Formella 8 years ago.
3717_3.patch (2.2 KB) - added by Frederico Caldeira Knabben 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Mike

Cc: mrceolla@… added

comment:2 Changed 8 years ago by Artur Formella

Keywords: Confirmed added
Owner: set to Artur Formella
Status: newassigned

Hmm. There is a small difference between config set in config.js and in plugins. The last method cannot be overwritten before replace() because we have

CKEDITOR.config.dialog_backgroundCoverColor = 'white';

So we need to replace it with

CKEDITOR.config.dialog_backgroundCoverColor = CKEDITOR.config.dialog_backgroundCoverColor || 'white';

(just like it is done in WSC plugin)

Changed 8 years ago by Artur Formella

Attachment: 3717.patch added

comment:3 Changed 8 years ago by Artur Formella

Keywords: Review? added

comment:4 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

Are you sure this is the correct way to go? I think instead we should instead suggest user to always dedicate configurations to editor instance instead.

Changed 8 years ago by Artur Formella

Attachment: 3717_2.patch added

comment:5 Changed 8 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.0CKEditor 3.1

I don't see how this patch can help with this ticket. Actually, using CKEDITOR.tools.extend may be a good option for us to fix this ticket. I mean, having all plugin related settings being defined with it.

In any case, we don't need to work on it right now. The packaged distribution contains all plugins into the ckeditor.js file, so it would be definitely possible to override this setting for all instances with CKEDITOR.config.dialog_backgroundCoverColor.

Other than that, it's always possible to define these settings by instance, avoiding any issues, so we can delay it for the 3.1.

comment:7 Changed 8 years ago by Artur Formella

Owner: Artur Formella deleted
Status: assignednew

comment:8 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

My suggestion here is to instead not having the default color being initialized in the plugin file. We could instead simply have this setting documented, and use it in the code only if defined, failing down into its default value.

comment:9 Changed 8 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: newassigned

Changed 8 years ago by Frederico Caldeira Knabben

Attachment: 3717_3.patch added

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

This patch introduces a pattern we should consider using for our settings. There is no effective need to have them initialized in the CKEDITOR.config object. In this way we are able to set the CKEDITOR.config object in-page with the default settings we would like to have.

I'm proposing these changes for the dialog setting only to simply match this ticket needs, but we should consider adopting the same approach for all plugin settings.

comment:11 Changed 8 years ago by Garry Yao

Keywords: Review+ added; Review? removed

It's a good move, while it's noticed that cover background-color is not working on IE6, I'll open a new ticket for it.

comment:12 Changed 8 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [4364].

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