This patch is against the 6--2 branch. Any comments are welcome.
I'll check the HEAD branch later to see if I can help somehow there.

Below is a summary of changes included in attached patch:

  • Corrected CKEditor name (CKeditor -> CKEditor).
    (Thank you in advance for correcting the component name from "Editor - CKeditor" to "Editor - CKEditor").
  • Corrected download link.
  • Added ckeditor.css for better compatibility with IE, just launch it in IE to see what I mean.
    Because table.cke_editor selector is used it will not affect anything but CKEditor.
  • Added default value for resize_minWidth for better compatibility with all themes.
    Two/three column layouts running on a screen with resolution 800x600 or even 1024x768 have problems when CKEditor is resized.
    Until #313497: Allow configuration of advanced editor settings is fixed, this should be a reasonable default value.
  • Used config.format_tags to properly pass "CSS -> Block" formats settings from Wysiwg module
    to CKEditor ("HTML block format" (Format) button)
  • Grouped buttons in same the way as they are grouped in the default "Full" toolbar in CKEditor.
    This way CKEditor toolbar will look better by separating visually different kind of buttons.

    Right now everything is passed to CKEditor as one big array. For example if someone selects
    'Styles', 'Format', 'Font', 'FontSize', 'TextColor', 'BGColor'
    then it is sent as:
    [['Styles', 'Format', 'Font', 'FontSize', 'TextColor', 'BGColor']]
    with this patch, it will be corrected to:
    [['Styles', 'Format', 'Font', 'FontSize'], ['TextColor', 'BGColor']]
    Buttons that are not defined in $toolbar_groups array will just belong to the nearest group (they will not disappear).
    It is a reasonable compromise until #277954: Allow to sort editor buttons is fixed.

  • Reordered buttons in the same way as they are ordered in the default CKEditor toolbar.
    This should be useful because:
    a) people using FCKeditor/CKEditor got used to the order of buttons provided by this editor,
    b) it should be easier to locate correct button in the "Buttons and plugins" section by looking at the default "Full" toolbar that is enabled by default when installing Wysiwyg module.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wwalc’s picture

Uploaded patch file.

wwalc’s picture

There is one more thing that I forgot to mention: CreateDiv button was added in CKEditor 3.1.
This patch enables it in the "Buttons and plugins" section for versions 3.1.0.4885 and above.

wwalc’s picture

Adding improved patch, I forgot to remove the baseHref property from ckeditor.inc.
It is not needed there, besides it is breaking CKEditor in IE (CKEditor doesn't load theme styles properly for example).

TwoD’s picture

Thanks for taking the time to create this patch! I'm currently swamped with other things, and sun is too, so all help is appreciated!
I've not tested the patch yet so I don't know these fixes turn out yet but I think I have a pretty good idea from just reading the code.

Could you please integrate the config.format_tags solution from #636814: CKeditor: load $config['block_formats'] into CKeditor settings?
There are not config.format_## for all the things that settings box says is allowed.

Sun and I have previously turned down patches including button grouping for just one editor, so I'll have to discuss that with him. (There's no doubt it looks better with the buttons grouped but we don't like having to add compromises.)
I don't have a problem with the reordering tho.

What is the problem with baseHref in IE? I ran a grep across the code to see where it was used before including it, just to make sure only Preview was affected so it wouldn't cause problems in normal editing mode if set wrong. I have no info on this issue so any details you can provide would be helpful.

wwalc’s picture

Thanks TwoD for a prompt reply and your helpful comments.

1) baseHref
Indeed baseHref is actually useful in Image preview window, Preview button and so on. So let's keep it, but fix the problem that I noticed.
When baseHref is defined, CKEditor adds <base href="' + config.baseHref + '"/> to it's iframe (to wysiwyg area) and to a preview window.
The problem is that right now, it is set to something like '/drupal/' (this is what base_path() is returning on my Drupal installation located in http://example.com/drupal/) and IE expects there an absolute URL (http://example.com/drupal/).
See attached image to see what's the problem (Fresh Drupal installation + Wysiwyg module + CKEditor profile saved once to be sure that "Use theme CSS" is used).

2) config.format_tags
Nice catch by marcvangend. I have improved it a little bit by checking for tags that are already defined in CKEditor, there is no need to pass extra code for headers etc.
Unfortunately, this will not work as expected with CKEditor 3.1 due to http://dev.fckeditor.net/ticket/5038 and http://dev.fckeditor.net/ticket/1032.
In any case, we can add that code at this stage, because it's valid.

3) grouping buttons
I hope Sun will allow committing that part of code. It will just make this module a little bit more user friendly and this way possibly extend the user base of developers using CKEditor through Wysiwyg module. As soon as #277954: Allow to sort editor buttons is ready, it can be removed without any harm.

wwalc’s picture

Ok... reviewed my own patch and if someone had Caps Lock turned on when editing block_formats this code would not work as expected (H5 != h5).
Attached new patch with strtolower() call.

Regarding 1) baseHref - I hope you managed to reproduce the problem. The problem may be seen in the wysiwyg area in CKEditor in IE - theme styles are not loaded and browser is using its default styles.

TwoD’s picture

1) The baseHref change makes sense, I should have researched it better.

2) Thanks for the ticket links! I think the implementation looks ok. I still don't know what the coding standars say about "AS" vs "as" though.

3) We'll see. ;)

domineaux’s picture

Category: feature » support
Priority: Normal » Critical

So what is the final patch for the day?

TwoD’s picture

Category: support » feature
Priority: Critical » Normal

Please don't change the tags without a reason.
If you want to help, please test the latest patch and report back here.

EugenMayer’s picture

Just to get things right ( http://drupal.org/node/692756 ) the current stable 6-x-2-0 release does not support the CKeditor?

TwoD’s picture

@EugenMayer, that is correct. The support will be in 2.1, which is very close to release, see #652410-4: Wysiwyg 2.1 for a discussions on which issues are holding it back.

EugenMayer’s picture

TwoD i have seen that issue an iam probably will help you guys getting it done, as iam getting deeper and deeper into WYSIWYG API.

miro_dietiker’s picture

Tested many approaches and also wrote some patch (before) with different approach to make buttons groupable.
However this patch here looks better...

Beside addition of draggable support for buttons wysiwyg should ask an editor not only for buttons but also (default) button groups.
I've started with adding a function that returns default group assignment which sounds generally right to me.

I think we're still better to have this ckeditor specific patch in wysiwyg api till the architecture gets updated and will support draggable.

EDIT: Switched to this patch here and it works perfectly for now.

geerlingguy’s picture

The biggie for me is the minWidth for the editor resize. It bugs the heck out of me that the editor is always extending beyond the content area (a big wtf for end users) when it is resized. My layout only allows 600px for the content area, and setting a smaller minWidth helps with resizing.

wwalc++

Steve Dondley’s picture

patch applied cleanly. seems to work and it has support for template icon. Great!

One suggestion: ckeditor adds a nice background color to each group if icons. The background color is lighter than the rest of the toolbar. It makes the toolbar more modern looking. A minor detail but I think it makes a big difference on the perception on the quality of the toolbar. When I first set up the module with wysiwyg, I thought the icons were actually different that ckeditor's. They look much worse without that lighter background for some reason.

TwoD’s picture

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -276,34 +307,40 @@ function wysiwyg_ckeditor_plugins($edito
+        'Source' => t('Source code'), 'Templates' => t('Templates'),

Would changing the available templates not require modification of the library files? We're trying to avoid that. Currently, we can't create a GUI to properly configure this option. The other editors have this disabled and we've been relying on the wysiwyg_template module to provide a way to use a different file for this configuration. It currently only supports TinyMCE but accepts patches with support for other editors as well.

I really need to discuss this with sun but I can't seem to catch him on IRC...

Powered by Dreditor.

wwalc’s picture

Would changing the available templates not require modification of the library files? We're trying to avoid that.

Yes, but it might be actually a file from another (safe) location, so if someone deletes CKEditor folder and upload a newer version of CKEditor his changes will not be lost.
In order to configure it this way, the following configuration option would have to be adjusted:
http://docs.cksource.com/ckeditor_api/symbols/CKEDITOR.config.html#.temp...
but at this moment there is no easy way to do this (#313497: Allow configuration of advanced editor settings).

The other editors have this disabled

If it is intentionally disabled in all other editors, then I see no problem in disabling this button here as well, just thought someone forgot about it.
On the other side, if someone decides to not select editor buttons at all, then he will have the full toolbar with all buttons, including "Templates" button.

Providing this button through Wysiwyg API template plugin module sounds reasonable to me as well.

TwoD’s picture

wwalc, you nailed the problem with that link to #313497: Allow configuration of advanced editor settings, we can't make that option GUI configurable yet, or even print out where Wysiwyg tells the editor to look for the template file, except for in the README. And to be honest, I don't expect people to scan through the README for editor-specific quirks, I want to keep that file focused on the module itself.

Wysiwyg API template plugin at least provides a proper GUI to locate the template 'registry' file, and is able to influence the editor settings via the plugin API, so it'll have to do for now.

wwalc’s picture

I agree with you (see my last sentence). Considering the way how Wysiwyg module is designed, providing this button through a separate, dedicated module makes much sense.
I wasn't actually aware that such module exists and that "Templates" buttons are removed intentionally.

Regarding #313497: Allow configuration of advanced editor settings - just wanted to point out that there might be some people that actually may want to do some little editor-specific tweaks (people that are familiar with some editor), but this was just my $0.02.

I can provide corrected patch without the Templates button, however ideally before creating another patch with this minor change would like to know if there are any other problems with this patch, if possible.

EugenMayer’s picture

Great to see such a progress here!

mandclu’s picture

subscribe

sun’s picture

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -122,16 +123,19 @@ function wysiwyg_ckeditor_themes($editor
+  global $base_url;
...
-    'baseHref' => base_path(),
+    'baseHref' => $base_url ."/",

(minor) We can use $GLOBALS['base_url'], and also single quotes + spaces before and after the string concatenation operator here.

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -122,16 +123,19 @@ function wysiwyg_ckeditor_themes($editor
+    // For better compatibility with smaller textareas.
+    'resize_minWidth' => 450,

Without knowing about how minWidth works internally, I fear that a hard-coded minWidth of 450 pixels will break a theme's layout in case an editor appears in a smaller area, f.e. a 200px sidebar?

However, if I get this configuration value wrong, then we probably want to clarify its effect/meaning in the inline comment.

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -139,8 +143,17 @@ function wysiwyg_ckeditor_settings($edit
+    $format_tags = explode(',', strtolower($config['block_formats']));
+    $defined_tags = array(
+      'h1' => 1, 'h2' => 1, 'h3' => 1, 'h4' => 1, 'h5' => 1, 'h6' => 1,
+      'p' => 1, 'pre' => 1, 'address' => 1, 'div' => 1
+    );
+    foreach ($format_tags AS $tag) {
+      if (empty($defined_tags[$tag])) {
+        $settings["format_$tag"] = array('element' => $tag);
+      }
+    }

A couple of issues here:

1) strtolower() needs to be drupal_strtolower()

2) "AS" must be lowercase, like any other PHP language statement. It's only uppercase in SQL queries.

3) empty() was probably meant to be !isset()? On a second thought, why don't we simply use array_diff() here? :)

4) If I get this code right, then users cannot remove the default tags, because they are always added by CKEditor? Or are only format_$tag settings defined by default?

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -139,8 +143,17 @@ function wysiwyg_ckeditor_settings($edit
+      'p' => 1, 'pre' => 1, 'address' => 1, 'div' => 1

(minor) Missing trailing comma.

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -276,34 +307,40 @@ function wysiwyg_ckeditor_plugins($edito
       'buttons' => array(
+        'Source' => t('Source code'), 'Templates' => t('Templates'),
+        'Cut' => t('Cut'), 'Copy' => t('Copy'), 'Paste' => t('Paste'),
+        'PasteText' => t('Paste Text'), 'PasteFromWord' => t('Paste from Word'),
+        'SpellChecker' => t('Check spelling'), 'Scayt' => t('Check spelling as you type'),
+        'Undo' => t('Undo'), 'Redo' => t('Redo'),
+        'Find' => t('Search'), 'Replace' => t('Replace'),
+        'SelectAll' => t('Select all'), 'RemoveFormat' => t('Remove format'),
         'Bold' => t('Bold'), 'Italic' => t('Italic'), 'Underline' => t('Underline'),
         'Strike' => t('Strike-through'),

If not absolutely required, we should not change the order of buttons - at least not this way.

Here, the less used buttons come first. The existing order - lacking a feature to customize them - attempts to present the most commonly used buttons first, more advanced buttons later, and putting absolute expert buttons last.

Additionally, still lacking a feature to order buttons, we are trying to keep the order buttons consistent across editors, if possible, so switching from one editor to another is hopefully not too confusing.

Also note that all buttons are presented in the defined order in the administrative editor configuration.

Therefore, I'd be more happy if we could leave the defined order here more or less as-is.

+++ editors/ckeditor.inc	19 Jan 2010 20:18:30 -0000
@@ -276,34 +307,40 @@ function wysiwyg_ckeditor_plugins($edito
+  if (version_compare($editor['installed version'], '3.1.0.4885', '<')) {
+    unset($plugins['default']['buttons']['CreateDiv']);
+  }

Can we add a short comment above this condition, explaining the situation?

+++ editors/css/ckeditor.css	19 Jan 2010 20:07:56 -0000
@@ -0,0 +1,6 @@
+/* CKEditor padding in IE */
+table.cke_editor fieldset { 
+  padding:0 !important;
+}

Hm. I do not understand what this is doing - at least the comment should be clarified.

In addition, shouldn't this selector be prefixed with * html or similar to limit the styles to IE?

(minor) There should be a space between padding: and the value.

Powered by Dreditor.

TwoD’s picture

4) The editor has already defined CKEDITOR.Config.format_p, ...format_h1-6, ...format_div, ...format_address and ...format_pre in the 'format' plugin (they just have 'element' = 'tag' set there as well). Redefining them to the same thing would not be invalid, but redundant. If they are used or not depends on the format_tags, removing a tag there but keeping the format_tagname definition is no problem so the user can always remove the default tags from the string.

picciuto’s picture

subscribe.

sun’s picture

Status: Needs review » Fixed
FileSize
4.11 KB
3.89 KB

Thanks for reporting, reviewing, and testing! Committed attached patch to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Changes I left out:

- Button grouping; we need to join forces on a proper solution that works for all editors. On the plus side, CKEditor seems to have lost the previous complexity of FCKeditor, where not only groups but also "toolbars" existed, IIRC. If that is really the case, then a universal solution should get much easier now. It's exactly that sort in-depth detail we need to analyze and know about all editors to get the ball rolling, and it primarily was FCKeditor's notion of abstract "toolbars" on top of button groupings that led us to defer the implementation of the entire feature.

- The entire CSS file. I was unable to locate any fieldset within the editor. Of course, I'm open to discuss this (and we should do so quickly if required, since 2.1 is about to get out of the doors).

Noteworthy things:

- The CreateDiv button creates DIVs that cannot be removed or altered. I considered to remove this button for that reason, but left it in for now.

- When adding "code" to the list of HTML block formats, then I get the following entry in the list - additionally, the format is applied to the markup, but is not identified later on and it cannot be removed afterwards:

ckeditor-block-format-code.png

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

miro_dietiker’s picture

Component: Editor - CKeditor » Code
FileSize
2.65 KB

Attaching patch that contains the latest button grouping that was missing in the commit.

... just for completeness if someone missing it ...