Problem/Motivation

WYSIWYG in core.

Proposed resolution

tl;dr: This issue is about adding an easily configurable, sensibly preconfigured, enabled-by-default CKEditor into Drupal 8.

What's already done:

This issue is about:

  • implementing the editor.module API so that CKEditor will become available on any form element that is using processed (filtered) text
  • giving the user the ability to configure CKEditor with a drag-and-drop interface

Note that this was developed at http://drupal.org/project/wysiwyg_ckeditor by quicksketch, he did the bulk of the work on that module, and thus the bulk of this patch!

Remaining tasks

Before commit:
All done!

Follow-ups:

User interface changes

  • A new module: CKEditor — enabled by default in the Standard profile.
  • A WYSIWYG editor enabled by default for the "Filtered HTML" and "Full HTML" text formats!
    Here you can see it in action both for form-based WYSIWYG editing and in-place ("true") WYSIWYG editing:
    CKEditor through Editor.module both on regular form and for true WYSIWYG.png

API changes

  • A new API for CKEditor integration: modules can register additional CKEditor plugins and override things.
  • An API modification to the Editor module to allow for syncing text editor configuration changes (unidirectionally) to filter settings.

Commit message

To ensure all those who've contributed significantly get credit, because they are not all visible in this issue!

Issue #1824500 by quicksketch, Wim Leers, jessebeach: WYSIWYG: Add CKEditor module to core

Original report by webchick

Note the original report was at #1878344: Add CKEditor JS library to core!

Per http://buytaert.net/from-aloha-to-ckeditor

This is postponed on #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration.

Work is happening in http://drupal.org/project/wysiwyg_ckeditor.

CommentFileSizeAuthor
#141 1890502-changelog.patch711 bytesWim Leers
FAILED: [[SimpleTest]]: [MySQL] 49,437 pass(es), 2 fail(s), and 3 exception(s). View
#136 Screen Shot 2013-02-07 at 9.20.42 PM.png26.81 KByannisc
#136 Screen Shot 2013-02-07 at 9.15.48 PM.png13.28 KByannisc
#114 ckeditor_module-1890502-114.patch127.72 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 49,312 pass(es). View
#114 interdiff.txt22.46 KBWim Leers
#106 ckeditor-toolbar.png35.04 KBsun
#105 ckeditor.patch128.25 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 4 fail(s), and 0 exception(s). View
#105 ckeditor.interdiff.txt9.33 KBsun
#99 interdiff.txt2.22 KBnod_
#99 core-ckeditor-1890502-99.patch128.79 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 49,223 pass(es). View
#98 core-ckeditor-1890502-98.patch128.75 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 49,227 pass(es). View
#98 interdiff.txt9.73 KBeffulgentsia
#95 ckeditor-tooltips.png46.12 KBjessebeach
#90 core-ckeditor-1890502-89.patch127.62 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 49,055 pass(es), 14 fail(s), and 0 exception(s). View
#90 interdiff.txt3.64 KBnod_
#88 ckeditor.patch127.72 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,897 pass(es), 16 fail(s), and 0 exception(s). View
#88 ckeditor.interdiff.txt10.03 KBsun
#84 core-ckeditor-1890502-84.patch127.5 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 48,818 pass(es). View
#84 interdiff.txt21.96 KBnod_
#78 ckeditor_module-1890502-78.patch129.85 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 48,871 pass(es), 1 fail(s), and 0 exception(s). View
#78 interdiff.txt26.31 KBWim Leers
#70 ckeditor_module-1890502-70.patch120.26 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 48,851 pass(es). View
#70 interdiff.txt29.94 KBWim Leers
#70 ckeditor_toolbar_builder_ui_with_styles.png32.85 KBWim Leers
#66 ckeditor_module-1890502-66.patch110.11 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 48,807 pass(es). View
#66 interdiff.txt21.45 KBWim Leers
#62 ckeditor_module-1890502-62.patch111.32 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 48,805 pass(es). View
#62 interdiff.txt6.03 KBWim Leers
#57 ckeditor_module-1890502-57.patch111.33 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,079 pass(es), 19 fail(s), and 13 exception(s). View
#57 interdiff.txt17.19 KBWim Leers
#57 interdiff_editor_form_restructuring.txt20.04 KBWim Leers
#57 ckeditor_toolbar_builder_ui.png27.92 KBWim Leers
#53 ckeditor_module-1890502-53.patch100.01 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,371 pass(es), 18 fail(s), and 13 exception(s). View
#53 interdiff.txt92.01 KBWim Leers
#44 Screen Shot 2013-01-26 at 16.45.16.png45.44 KBWim Leers
#44 Screen Shot 2013-01-26 at 16.21.17.png46.38 KBWim Leers
#47 Screen Shot 2013-01-28 at 22.12.23.png47.01 KBWim Leers
#41 ckeditor_module-1890502-41.patch45.83 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 49,305 pass(es). View
#41 interdiff.txt13.32 KBWim Leers
#22 ckeditor_module-1890502-21.patch42.96 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 50,731 pass(es). View
#22 interdiff.txt29.23 KBquicksketch
#14 ckeditor_module-1890502-14.patch46.83 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 50,704 pass(es). View
#14 ckeditor_module-1890502-images-14-do-not-test.patch50.02 KBWim Leers
#13 ckeditor_module-1890502-13.patch97.01 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,744 pass(es), 1 fail(s), and 0 exception(s). View
#12 ckeditor_module-1890502-12.patch114.98 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,764 pass(es), 1 fail(s), and 0 exception(s). View
#12 interdiff.txt48.23 KBWim Leers
#3 CKEditor configuration to filter settings syncing.png14.19 KBWim Leers
#3 CKEditor configuration to filter settings syncing.png14.19 KBWim Leers
#2 ckeditor_module-1890502-2.patch160.87 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 50,941 pass(es). View
#2 interdiff.txt12.41 KBWim Leers
#1 ckeditor_module-1890502-1.patch151.26 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 51,001 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
151.26 KB
PASSED: [[SimpleTest]]: [MySQL] 51,001 pass(es). View

First, I want to stress this again, which is already mentioned in the issue summary:

Note that this was developed at http://drupal.org/project/wysiwyg_ckeditor by quicksketch, he did the bulk of the work on that module, and thus the bulk of this patch!

Let that be clear.


This patch is identical to http://drupalcode.org/project/wysiwyg_ckeditor.git/tree/abb254b1cd646b5d..., with the following changes:

quicksketch agreed with me posting this patch, because it will hopefully get more testing and feedback this way, but — as he said — "it's a little less polished than his typical patch". Keep that in mind.

Also note that he has originally developed his CKEditor 4 plugins against 4.0-dev, whereas #1878344: Add CKEditor JS library to core is 4.0.1, so these plugins may be broken. For example, it seems the drupalcaption plugin is currently broken. The plugins should not be the center of attention anyway, the overall structure should be.

Wim Leers’s picture

Issue summary: View changes

More review info, more info about provided CKEditor plugins.

Wim Leers’s picture

FileSize
12.41 KB
160.87 KB
PASSED: [[SimpleTest]]: [MySQL] 50,941 pass(es). View

This reroll adds my initial, very rough attempt at syncing the CKEditor toolbar configuration to the filter settings. The addition of this feature means this patch can now be considered to contain all aspects of the minimally viable implementation. (Or at least so think Bojhan and I right now.)

Notes about "editor config <-> filter settings syncing":

  • This was already discussed to some extent in the Editor.module issue, see #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, comments 40, 41, 42, 43, 45 and 47.
  • The syncing only needs to be unidirectional: FROM editor configuration TO filter settings. quicksketch believes so, and I agree.
  • Ideally, we'd have a nicely abstracted API that allows text editors to indicate that a feature requires A) certain HTML tags (and attributes) and/or B) a certain filter to function at all.
  • We could have a nice and simple JS API, but this also needs to work when JS is disabled. So AFAICT the calculations should happen on the server side, through Drupal form #ajax handling, so that it can easily also work when JS is disabled.
  • Editors are free to build whatever kind of configuration for they need, hence we have no standardized way of knowing what is allowed when, hence it's necessary to extend \Drupal\editor\Plugin\EditorInterface to extract the relevant data from the current form state.
  • Similarly, filters have free-form settings forms and settings data structures, so either we'll have to add metadata or an enforced structure to filter settings, or editor.module will have to extend filter.module's callbacks with new ones to work. I took the latter approach for now.
Wim Leers’s picture

I forgot to include a screenshot with #2, here we go:

CKEditor configuration to filter settings syncing.png

Wim Leers’s picture

Issue summary: View changes

d.o fail

Wim Leers’s picture

Issue summary: View changes

Add screenshots.

Wim Leers’s picture

Issue summary: View changes

Minor clarification.

quicksketch’s picture

Thanks Wim. Are these changes being made to the wysiwyg_ckeditor project too, or are they exclusive to this patch?

I had imagined the adding/removal of tags to be less intrusive. Right now it seems as though the user has to save the form twice?

+  // If the user hasn't been warned yet about this specific list of tags to be
+  // allowed, then do so, before allowing this form to be saved.
+  if (!empty($prohibited_required_tags) && (!isset($form_state['prohibited_required_tags']) || $form_state['prohibited_required_tags'] != $prohibited_required_tags )) {
+    drupal_set_message(t('Your text editor configuration will cause the following HTML tags to be allowed: @tag-list.', array('@tag-list' => '<' . implode('>, <', $prohibited_required_tags) . '>')), 'warning');
+    $form_state['prohibited_required_tags'] = $prohibited_required_tags;
+    $form_state['rebuild'] = TRUE;
+  }

Instead I would like to see this handled by Filter module providing information to *all* filters about the tags that are required, via both a JS and PHP API. That way any setting (not just the toolbar) can require additional tags, and likewise, any filter (such as WYSIWYG Filter in contrib, if it continues to exist) can respond to this tag requirement. The HTML tag filter we have in core of course would respond to these callbacks out of the box.

In a truly ideal world, the list of allowed tags would be passed through each enabled filter *in order*, so if a particular tag was not allowed at the time it needed to be, modules would either A) be able to alert the user to rearrange the filters or B) add the tag to the list of allowed tags. At the end of the loop (if no validation errors were thrown) the new list of tags would be saved by any modules providing a tag filtering list.

That's a lot of talking I know without any code to back it up. Maybe we should give this task its own issue and discuss it there? Although reconciling text editors with text formats is the *entire point* of Editor module, I don't know if it's a hard requirement for the initial addition of CKEditor module.

sun’s picture

Can we split out all of the CKEditor skin/theme files into a separate subdirectory, please? I.e.:

- core/modules/ckeditor/images/buttons/link.png
+ core/modules/ckeditor/skin/drupal/images/buttons/link.png

That said, could we add the skin/theme in a follow-up issue? All of those files are rather distracting here... :-/

Likewise, can we move the JS files that are unrelated to CKEditor plugins but regular Drupal integration JS files into the module's top-level directory? I.e.:

- core/modules/ckeditor/js/ckeditor.admin.js
- core/modules/ckeditor/js/ckeditor.js
+ core/modules/ckeditor/ckeditor.admin.js
+ core/modules/ckeditor/ckeditor.js

That said, not entirely sure on the removal of /js. I just have troubles to make sense of the individual parts, integrations, plugins, and what belongs to what, etc. The code base on its own should be cleanly and clearly indicate that already.

Same for the CSS files. That said, it's entirely not clear to me which files in /css belong to what. E.g., ckeditor-caption.css seems to belong to the caption plugin...? So why isn't it in /js/plugins/drupalcaption/ ?

Speaking of, is it possible to use a properly namespaced 'drupal.caption' as plugin identifier and name everywhere, instead of 'drupalcaption'?

Lastly, the 'webkitdrag' plugin doesn't seem to be ours? If that's true, then it shouldn't be contained in ckeditor.module, but should be moved into /misc/ckeditor instead.

I'd also prefer if we could move those custom plugins into a follow-up issue. Equally distracting here.

Essentially, I'd like to focus on the pure + plain ckeditor integration here. No custom UI, no custom plugins, and whatnot. Let's discuss and tackle that first and get it in separately. It looks like I'll have a ton of comments on just that part, and I wouldn't want that discussion to get derailed by UI + UX + usability aspects of the plugins and the custom skin and whatnot.

What do you think?

quicksketch’s picture

That said, could we add the skin/theme in a follow-up issue? All of those files are rather distracting here... :-/

We don't have a skin/theme in this module. The buttons are only used in the admin area for arranging the toolbar, but they happen to be the same buttons used by the default skin of CKEditor (moono). They're not used on the front-end for any purpose.

E.g., ckeditor-caption.css seems to belong to the caption plugin...? So why isn't it in /js/plugins/drupalcaption/ ?

It's probably because this CSS file is needed on the front-end, not just when the plugin is loaded. I'm not sure loading CSS files from CKEditor plugin directories really makes a lot of sense. You could think of it as the CSS being the important part and the plugin makes them accessible. It may make sense to bundle that file into ckeditor.css with the rest of the generic styles.

Lastly, the 'webkitdrag' plugin doesn't seem to be ours? If that's true, then it shouldn't be contained in ckeditor.module, but should be moved into /misc/ckeditor instead.

It's a temporary measure until I can convince the original author to add it to ckeditor.com. It's pulled from this Github sandbox: https://github.com/sstur/ck-webkitdrag (I submitted a couple pull requests to that project to get the resulting version we're using currently). Ideally of course it would be included in CKEditor itself (like tinyMCE and Aloha have done), but its existence in the CKEditor module is a stand-in until that happens.

No custom UI, no custom plugins, and whatnot. Let's discuss and tackle that first and get it in separately.

I'm probably okay with this, though I fear we'll end up with drawn-out discussions about each of the custom pieces (which may be warranted, I just think it will slow things down disproportionately). Some things clearly need individual work: the teaser break plugin should probably be tied together with removing the teaser splitter button for example, the drupaldialog box could be tied together with actually using Drupal's modals, the drupalcaption plugin will probably be rewritten to use "widgets" once they're included in CKEditor 4.1. Pretty much *everything* in this project is not in a finished state. So it's a matter of whether we want to solicit feedback on the whole experience and iterate upon it, or put on one piece at a time.

My experience has been that single pieces is a slower process overall, but at least it avoids half-doneness (instead nothing gets in at all). But like @sun implies, there are probably 20 issues (or more) packed into this single patch. It may be better to simply postpone it all and work in a dedicated queue (http://drupal.org/project/wysiwyg_ckeditor) until we can offer a module that won't result in a 300 comment issue.

webchick’s picture

I'm really not a fan of that plan, personally. History has shown that until features are committed to core and in peoples' faces, a) the full range of problems with them aren't fleshed out/known, and b) no one else but the people driving those patches helps at all (or even looks at them at all in most cases), so it's a tremendous burden on the shoulders of a very small team, who risks missing major things. We have about a month left to go before "for real" feature freeze, and I think committing this the day before hand or whatever is a very risky proposition. :\ I'd much rather get it in by, say, the end of this month so there's still a couple of weeks to get in any follow-up features we might need, and/or detail a plan for those which simply can't be done by Feb 18 for some reason.

I think a better plan is to try and articulate what we think the full range of problems are, ensure there are follow-ups for all of them so that they're documented, and then decide which of those are commit blockers, if any, and which can follow on after commit.

sun’s picture

My point is that, for now, I primarily and only want to care for the PHP code aspects that relate to integrating ckeditor.module with editor.module and edit.module. I have no use for UI/UX discussions at this point, because I want to discuss and focus on much deeper and very technical aspects of this.

The actual changes I'm referring to probably make up <~20% of this patch only. Essentially, the Wysiwyg API aspects (because this patch factually moves another big amount of Wysiwyg module code into core, even though it's labeled as "ckeditor.module").

It doesn't make sense to me to mix that overly technical discussion with any kind of discussion on the custom skin, custom plugins, and all the other customizations and their user testing, cross-browser testing, theme testing, etc. — which is guaranteed to happen (and must happen) as soon as the first end-user and UX fan hits this issue, but these are entirely different topics of its own.

webchick’s picture

I'd be more comfortable in that case keeping the "mega patch" together so it could be easily tested/reviewed as a whole (that whole UX bikeshedding discussion that we know needs to occur in order for this to be RTBC has already been postponed for over 2 months because of waiting on Editor module in the first place), but creating a dedicated sub-issue for discussing the UX aspects of it so it doesn't unnecessarily complicate the PHP review.

Basically, we are out of time for waterfalls at this stage in the game, I feel.

Wim Leers’s picture

#4: I didn't make those changes to the wysiwyg_ckeditor module because I know the quality of that aspect is low (it's just a working proof-of-concept to gather feedback to determine the direction to take), so I didn't want to push that into the repo.

Regarding sun's remarks and quicksketch's and webchick's responses: I think it might be possible to find a good middle ground here. I think we could consider the Drupal-specific plugins to be follow-up issues. We do need the images for the toolbar configuration UI. And for clarity: there is no custom UI.
I think these changes would result in the best balance of 1) focus/scope, 2) getting the overall code architecture & quality to "core worthy" before commit, 3) sensible follow-up issues.

Wim Leers’s picture

Issue tags: +CKEditor in core

Tagging; this helps the CKEditor folks track the "CKEditor in core" status on the Drupal side. On the CKEditor side, see http://dev.ckeditor.com/query?keywords=~Drupal.

Wim Leers’s picture

FileSize
48.23 KB
114.98 KB
FAILED: [[SimpleTest]]: [MySQL] 50,764 pass(es), 1 fail(s), and 0 exception(s). View

Reroll as per my suggestions in #10 due to lack of response.

No functional changes; only removing the CKEditor plugins and their related code.

Wim Leers’s picture

FileSize
97.01 KB
FAILED: [[SimpleTest]]: [MySQL] 50,744 pass(es), 1 fail(s), and 0 exception(s). View

And another reroll, this time with all of the images optimized by http://imageoptim.com/ (i.e. optimized by PNGOUT, AdvPNG, Pngcrush, extended OptiPNG). Otherwise identical to #12. No interdiff because whomever can read binary diffs is not a Drupal developer but somebody who writes in assembly.

20 KB less. Less intimidating, thus better.

If people prefer that, I could also include the images as a secondary "images" patch, so that you can really focus on the code itself in the "main" patch.

Wim Leers’s picture

FileSize
50.02 KB
46.83 KB
PASSED: [[SimpleTest]]: [MySQL] 50,704 pass(es). View

Aaaand last but not least: a reroll of #13, now split into two patches:

  1. the main patch, the one we must review and move forward
  2. the patch with just the images, that will most likely never (or almost never) change during the review 'n roll process

Now you've got different styles to choose from. Make your choice!

chx’s picture

Status: Needs review » Needs work

Some quick observations: theme_ckeditor_settings_toolbar has an awful lot of code for a theme function. editor module contains _filter functions?

webchick’s picture

Status: Needs work » Needs review

Leaving needs review to get more eyeballs, but Wim can clean that up on his next re-roll.

Gábor Hojtsy’s picture

Here are some notes based on a quick code-only review:

+++ b/core/modules/ckeditor/ckeditor.admin.incundefined
@@ -0,0 +1,173 @@
+/**
+ * @file
+ * Callbacks and theming for the CKEditor toolbar configuration UI.
+ */

"Callbacks" sounds odd, there does not seem to be any actual callbacks here, only template preprocess and theming hooks.

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -0,0 +1,294 @@
+  $plugins = module_invoke_all('ckeditor_plugins');
+  drupal_alter('ckeditor_plugins', $plugins);

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/editor/editor/CKEditor.phpundefined
@@ -0,0 +1,251 @@
+    drupal_alter('ckeditor_css', $css, $editor, $format);

These three newly added hooks would all need to be documented in a ckeditor.api.php. Unless I missed even more added hooks that also need docs :)

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -0,0 +1,294 @@
+  // Populate image locations, which all match button names.

Well, not all of them since some of them have it specified explicitly. I was pretty puzzled until I got down here and seen the missing ones are filled in later.

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -0,0 +1,294 @@
+      // Because we know the list of strings, drupal_strtolower() is not needed.

I don't understand this comment?! Maybe you meant it is due to the strings all using base ASCII chars?

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -0,0 +1,294 @@
+/**
+ * Retrieves the default theme's CKEditor stylesheets defined in the .info file.
+ *
+ * Themes may specify iFrame-specific CSS files for use with CKEditor by
+ * including a "ckeditor_stylesheets" key in the theme .info file.
+ *
+ * @code
+ * ckeditor_stylesheets[] = css/ckeditor-iframe.css
+ * @endcode
+ */

I'm not sure if I like this approach, would be good to validate with themers. It is pretty custom to ckeditor. Usually themes would implement a (pre)process hook for other things when they need to add extra CSS.

Also iFrame written with this capitalisation looks odd.

+++ b/core/modules/ckeditor/css/ckeditor-iframe.cssundefined
@@ -0,0 +1,18 @@
+/**
+ * CSS added to iFrame-based instances only.
+ */

+++ b/core/modules/ckeditor/css/ckeditor-rtl.cssundefined
@@ -0,0 +1,18 @@
+/**
+ * RTL styles used by CKEditor. Added to front-end theme and iFrame editors.
+ */

iFrame (again) (also at other places)?!

+++ b/core/modules/ckeditor/css/ckeditor-iframe.cssundefined
@@ -0,0 +1,18 @@
+ol,ul,dl {

Should we have spaces here after the commas?

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+/**
+ * @file
+ * Styles for configuration of CKEditor module.
+ */

Minor: colors in this file were uppercased, while elsewhere they are lowercase :)

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+ul.ckeditor-buttons {
+  min-height: 26px;
+  min-width: 26px;
+  list-style: none;
+
+  float: left;
+  clear: left;
+  padding: 0;
+  margin: 0 6px 5px 0;
...
+ul.ckeditor-buttons li:first-child {
+  -moz-border-top-left-radius: 2px;
+  -moz-border-bottom-left-radius: 2px;
+  -webkit-border-top-left-radius: 2px;
+  -webkit-border-bottom-left-radius: 2px;
+  border-top-left-radius: 2px;
+  border-bottom-left-radius: 2px;
+}
+ul.ckeditor-buttons li:last-child {
+  -moz-border-top-right-radius: 2px;
+  -moz-border-bottom-right-radius: 2px;
+  -webkit-border-top-right-radius: 2px;
+  -webkit-border-bottom-right-radius: 2px;
+  border-top-right-radius: 2px;
+  border-bottom-right-radius: 2px;

When things in CSS assume the first child is on the left and the last child is on the right; or put margins different on the left side vs. the right side and do floating, it seems to be the exact candidate for RTL styling the other way around. This applies to other things in this CSS as well. Struck me odd that this CSS does not have an RTL counterpart.

+++ b/core/modules/ckeditor/js/ckeditor.admin.jsundefined
@@ -0,0 +1,111 @@
+  detach: function (context, settings) {
+    // @todo
+  }

I assume this is to be done before commit?

+++ b/core/modules/ckeditor/js/ckeditor.jsundefined
@@ -0,0 +1,32 @@
+    return !!CKEDITOR.replace(element, format.editorSettings);
...
+    return !!editor;

What is the significance of using !! ?!

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/editor/editor/CKEditor.phpundefined
@@ -0,0 +1,251 @@
+ * Definition of \Drupal\ckeditor\Plugin\editor\editor\CKEditor.

The up to date code standards say "Contains ..." I believe.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/EditorBase.phpundefined
@@ -64,4 +64,10 @@ public function settingsFormValidate(array $form, array &$form_state) {
+  public function mapSettingsFormStateToRequiredHTMLTags(array $form_state) {
+    return array();
+  }

How can this be an empty array? According to its docs, it sounds like it is not very useful this way?! @todo?

Wim Leers’s picture

Thanks for the reviews!

#15/@chx: The _filter functions are for the functionality added in #2: "editor config <-> filter settings syncing". Please read #2 for more info.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -0,0 +1,294 @@
+  drupal_add_css(drupal_get_path('module', 'ckeditor') . '/css/ckeditor.css');

Should be a different hook and we need to use library stuff. See this change notice for some nice examples: http://drupal.org/node/1876152

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+  border: 1px solid #B6B6B6;

We lowercase colors

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+  -moz-border-radius: 3px;

see the other border radius comment

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+  display: inline-block;
+  height: 18px;
+  padding: 4px 6px;
+  outline: none;
+  cursor: move;
+  float: left;
+  border: 0;

Should be in aplhabetical order

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+  -moz-border-top-left-radius: 2px;
+  -moz-border-bottom-left-radius: 2px;
+  -webkit-border-top-left-radius: 2px;

See the other radius comment

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -0,0 +1,170 @@
+  -moz-border-top-right-radius: 2px;
+  -moz-border-bottom-right-radius: 2px;
+  -webkit-border-top-right-radius: 2px;

we removed all the moz and webkit border radius stuff for D8 because the standard border radius is supported in both browsers

Wim Leers’s picture

Status: Needs work » Needs review

@aspilicious: your review is solely code style/nitpicks. This is not the right time for that. Please provide feedback on how it works, not on what the code looks like. Especially if you set it to "needs work" for only such a superficial review, while we need structural feedback, that's not very nice.

Back to NR.

quicksketch’s picture

@aspilicious: the code review is greatly appreciated! But what @WimLeers is saying is that functional reviews are *more* appreciated.

I do think all these things need to be cleaned up. I personally didn't know about hook_page_build() being used for every-page CSS, so I'm glad to have that pointed out. Most of the other CSS for admin section is written the way it is because the styling was pulled directly from the Moono theme in CKEditor, to make the admin toolbar appear the same as the front-end theme. CSS properties in alphabetical order was never finalized (the handbook page is still "draft" proposal) and JohnAlbin's latest proposal finally removes that contentious proposal.

I'll go through all the feedback thus far and try to get a patch that balances somewhere between removing the questionable stuff and polishing the necessities.

quicksketch’s picture

FileSize
29.23 KB
42.96 KB
PASSED: [[SimpleTest]]: [MySQL] 50,731 pass(es). View

Okay here's a patch that cuts quite a bit more and incorporates most of the feedback:

  • All the code-style and spelling recommendations have been incorperated.
  • Regarding CSS styles, I eliminated vendor prefixes for border-radius, but kept the existing ones for box-shadow and gradients, since vendor support is not yet widespread.
  • I *removed* all of the changes to Editor and Filter modules (sincerest apologies Wim) because if this issue is focusing on adding CKEditor module, it shouldn't attempt to introduce new functionality into other modules. Considering that's functionality that will need to touch on all three modules at the same time, I think it would be better served by a separate issue.
  • Added documentation for all the new hooks.
  • Switched from hook_init() to hook_page_build() for adding CSS.

Feedback to a couple of points made above:

What is the significance of using !! ?!

I personally don't like this style, but it's not uncommon. If you have a variable that is undefined, a single ! makes it Boolean true, and another ! makes it Boolean false. The same goes for other types with defined values. It's basically a shorthand for converting any variable to a Boolean equivalent. I don't think it's a holdup.

When things in CSS assume the first child is on the left and the last child is on the right; or put margins different on the left side vs. the right side and do floating, it seems to be the exact candidate for RTL styling the other way around. This applies to other things in this CSS as well. Struck me odd that this CSS does not have an RTL counterpart.

I think you're right that RTL styling would be appropriate here. The patch right now should work in RTL, but the toolbar buttons will be on the opposite side of the screen from where they should be. We should add this CSS, as I haven't yet gotten to it in this reroll.

theme_ckeditor_settings_toolbar has an awful lot of code for a theme function.

I agree, but unless we want to decrease the flexibility of the output (say, by pre-building the IMG tags in a preprocess function), then I think all the current code is necessary. It's not pretty that we have to rebuild theme_item_list() in here because it won't print an empty <ul> tag either. Though the code isn't elegant, I don't think an ugly theme function (that no one would seriously override) should be an issue.

+ * @code
+ * ckeditor_stylesheets[] = css/ckeditor-iframe.css
+ * @endcode

I'm not sure if I like this approach, would be good to validate with themers. It is pretty custom to ckeditor. Usually themes would implement a (pre)process hook for other things when they need to add extra CSS.

The reason I did this is because if you're in an admin-theme, the hooks of the front-end theme are not going to be loaded. Reading the info file provides an easy option for themers, and makes the setting readable without loading the theme itself (which was the purpose of .info files to begin with). That said I'm not 100% sure this is the best option, but at least it *is* an option for themers. Without this, the only way to inject CSS into the editor would be through a module or sub-theming seven.

I committed most of these changes to the wysiwyg_ckeditor project too: #1893510: Use hook_page_build() instead of hook_init() to add CSS and #1893560: Implement code review updates (iframe titling, add docs, and code styling)

One thing I observed in this whole process is that continuing to attempt to maintain this patch and the wywisyg_ckeditor module (8.x branch) is tremendously difficult. Now that we've changed course from adding the module as a "drop-in" to "drop-in and then remove a bunch of stuff", maintaining this patch and keeping our current module with the same changes is really time consuming. Hopefully this version is less contentious so we don't have to do dual-updating like this for long.

This patch doesn't include the images, you can apply those from the second patch in #14. Interdiff is from Wim's first patch in #14.

yannisc’s picture

I cannot apply the images patch.

I tried "git apply --check ckeditor_module-1890502-images-14-do-not-test.patch", but I get: fatal: corrupt patch at line 103.

I applied the full patch (code and images), images got in with errors:

ckeditor_module-1890502-13.patch:865: trailing whitespace.
?PNG
ckeditor_module-1890502-13.patch:874: trailing whitespace.
?PNG
ckeditor_module-1890502-13.patch:883: trailing whitespace.
?PNG
ckeditor_module-1890502-13.patch:893: trailing whitespace.
?PNG
ckeditor_module-1890502-13.patch:902: trailing whitespace.
?PNG
warning: squelched 60 whitespace errors
warning: 65 lines add whitespace errors.

Module is working though and images got in their place with this patch.

Wim Leers’s picture

@quicksketch: I'm fine with you removing those changes in the editor and filter modules, but then the question is: is it okay if we do the "text editor configuration to filter setting syncing" in a follow-up? If that's the case, I'd be happy to do it that way, but I was under the impression that that would be a blocker.

@yannisc: You have to apply the images patch with additional parameters so git knows that it shouldn't care about trailing whitespace. These are binary files, so we don't control where whitespace lives. In #1824500-1: In-place editing for Fields there are such instructions IIRC (I'm replying from my phone).

yannisc’s picture

@Wim Lees: even with the --whitespace=nowarn --ignore-whitespace options, I get "fatal: corrupt patch at line 103" for the images only patch.
The #13 patch applied cleanly though with these parameters.

yannisc’s picture

Module seems to work!

I like that the ckeditor in this version formats html nicely, so when you switch to source view, source is readable.

quicksketch’s picture

Title: WYSIWYG: Add CKEditor to core » WYSIWYG: Add CKEditor module to core

Is it okay if we do the "text editor configuration to filter setting syncing" in a follow-up? If that's the case, I'd be happy to do it that way, but I was under the impression that that would be a blocker.

My preference would have been to deliver a "complete package" module right from the beginning (or at least a comprehensive start). Considering we're already removing large portions of CKEditor module's code (the custom plugins) and focusing on the basics of the module, I think a followup is appropriate considering our approach. Despite the stated preferences earlier for putting everything in, that's clearly not the route we've taken. Maintaining this partial-version of CKEditor as a patch and the complete one in contrib already is difficult. At this point it would make sense to just get in the non-controversial parts and then iterate directly in core, as that's the direction we've ended up taking already.

quicksketch’s picture

Issue summary: View changes

Fix typo.

Wim Leers’s picture

Issue summary: View changes

Added #1894644: Unidirectional editor configuration -> filter settings syncing (editor config <-> filter settings unidirectional syncing).

Wim Leers’s picture

Issue summary: View changes

Removing #1874712: Analyze/improve how other modules can provide CKEditor plug-ins from the list of reference issues, because I pulled in that discussion into this issue.

Wim Leers’s picture

Issue summary: View changes

Descoping issue; relevant part of issue summary moved over to #1894644: Unidirectional editor configuration -> filter settings syncing.

Wim Leers’s picture

#22:

For the code that was omitted in this reroll, I created the follow-up issue #1894644: Unidirectional editor configuration -> filter settings syncing and marked it as blocked on this one. Issue summary updated.

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ * - location: Required for all external plugins. String path to the plugin

Nitpick: s/location/path/, no?

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ *   - image: An image for the button to be used in the toolbar.
+ *   - image_rtl: If the image needs to have a right-to-left version, specify
+ *     an alternative file that will be used in RTL editors.
+ *   - image_alternative: If this button does not render as an image, specify
+ *     an HTML string representing the contents of this button. This alternative
+ *     will only be used in the administrative section for assembling the
+ *     toolbar.

Not only the image_alternative entry, but all of these will only be used in the administrative section for assembling the toolbar.
Or what am I missing? Do you have bigger plans for this?

Note to all: the reason why we must include/link to the images separately, is because in any build of CKEditor, the buttons' images are optimized into a sprite.
I think it might be worth updating our CKEditor (at core/misc) to include individual plugin's button images, despite the sprite. Because now, it seems as if it's up to the CKEditor module to keep these updated, whereas it really depends on CKEditor (the JS lib). However, that can be tricky when updating our build of CKEditor, since it's not part of the default build process, so we'd have to set up a custom build process (or do it manually each time).

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ *   - required_tags: If this button requires certain HTML tags to be allowed,

This is in fact not sufficient metadata. E.g.: if the a tag is allowed but the href attribute on that tag is disallowed, then it's still impossible to insert links.

Also in light of more advanced alternatives to core's filter_html filter, we should include more information. This can happen in a follow-up issue though.

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+    'path' => drupal_get_path('module', 'mymodule') . '/js/myplugin',
+    'file' => 'plugin.js',

Why do the path and filename have to be specified separately? Why can't there only be file and that just also contain the path?

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ * Modify the list of available CKEditor plugins.

Nitpick: this allows for more than just modifying the *list*, it allows each CKEditor plugin's metadata to be altered.

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ * @param $css

Nitpick: @param array $css

+++ b/core/modules/ckeditor/ckeditor.api.phpundefined
@@ -0,0 +1,158 @@
+ * Enabled callback for hook_ckeditor_plugins().
+ *
+ * Note: This is not really a hook. The function name is manually specified via
+ * 'enabled callback' in hook_ckeditor_plugins(), with this recommended callback
+ * name pattern. It is called from
+ * \Drupal\ckeditor\Plugin\editor\editor\CKEditor:getJSSettings().
+ *
+ * This callback should determine if a plugin should be enabled for a CKEditor
+ * instance. Plugins may be enabled based off an explicit setting, or enable
+ * themselves based on the configuration of another setting, such as enabling
+ * based on a particular button being present in the toolbar.

I'm not sure I like the entire approach of how modules can provide CKEditor plugins — and this part is the most contentious aspect.

I think it's worth reiterating what was posted over at #1874712: Analyze/improve how other modules can provide CKEditor plug-ins by quicksketch (I closed that issue because it was almost empty and we should have the discussion here anyway; issue summary updated):

What do you think about hook_ckeditor_plugins()? Is that something that could (or should) be converted to D8's plugin system also? It's a bit different because if each CKEditor plugin were required to be a Drupal plugin class, we'd end up with ~20 classes that would all be empty. At first I thought because most *_info() hooks are going away in D8 (and hook_ckeditor_plugins() is still essentially an *_info() hook, even though it doesn't have "info" in its name), that using the plugin system would make sense. However on further thought, I'm not sure it's a good fit. Perhaps if each plugin provided a settings callback too, the plugin system would make sense. Right now the module is simply assuming the plugin will form_alter() the CKEditor configuration form to expose its options.

I think that

  1. The whole CKEditor plugin approach that's being proposed needs to be signed off on.
  2. For me, the strangest part, is this "enabled callback" stuff. I understand the need for it, but can't we think of something more elegant? Do we really need to allow for arbitrary logic to check whether a CKEditor plugin should be enabled?

AFAICT this is the single most important aspect of this issue.

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -5,20 +5,14 @@
+define('CKEDITOR_VERSION', '4.0.1');

I am personally very much fond of defines because it can prevent a lot of subtle problems. However, in this case, I don't see the point. You use it only one location.

system_library_info() for example also doesn't do this:

// jQuery.
  $libraries['jquery'] = array(
    'title' => 'jQuery',
    'website' => 'http://jquery.com',
    'version' => '1.8.2',
    'js' => array(
      'core/misc/jquery.js' => array('group' => JS_LIBRARY, 'weight' => -20),
    ),
  );

So why should we do differently here?

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/editor/editor/CKEditor.phpundefined
@@ -153,7 +125,7 @@ function getJSSettings(Editor $editor) {
-        if ($plugin['enabled callback'] === TRUE || $plugin['enabled callback']($editor) && !empty($plugin['path'])) {
+        if ($plugin['enabled callback'] === TRUE || $plugin['enabled callback']($editor, $plugin_name) && !empty($plugin['path'])) {

A subtle change, the "enabled callback" now also receives $plugin_name. Why is this necessary? When an "enabled callback" is altered?

One thing I observed in this whole process is that continuing to attempt to maintain this patch and the wywisyg_ckeditor module (8.x branch) is tremendously difficult. Now that we've changed course from adding the module as a "drop-in" to "drop-in and then remove a bunch of stuff", maintaining this patch and keeping our current module with the same changes is really time consuming. Hopefully this version is less contentious so we don't have to do dual-updating like this for long.

Because of the changes I introduced in #2 (which are now removed), I figured we'd split our ways at that point and just keep rerolling this patch instead of also maintaining the module.
This is also why I had a shell script in the D8 Edit module branch that helped me with rolling core patches, but there there was a cleaner separation; it's hard to exclude just the plugins from the patch rerolls if you base your patch rerolls on the module. I could try to come up with a similar for CKEditor, but … why can't we just do continuous patch rerolls instead? :)

#23: Reproduced. I have no idea why all of that is happening, but it obviously is.


To prevent us from talking past each other, we must agree on the scope. With the issue summary updated wrt to the two descopings that have happened already (no Drupal-specific CKEditor plugins in this issue, and no filter settings updating), I believe this is the intended scope (copy/pasted from the issue summary):

This issue is about:

  • implementing the editor.module API so that CKEditor will become available on any form element that is using processed (filtered) text
  • giving the user the ability to configure CKEditor with a drag-and-drop interface

Do we all agree?

quicksketch’s picture

I could try to come up with a similar for CKEditor, but … why can't we just do continuous patch rerolls instead? :)

Much to my surprise, over 600 users are already using the contrib version (only for D7), which makes it an ideal place for gathering feedback since it's a lot easier to drop in a module in your existing website than to participate in the core queue. Additionally the contrib version can continue to develop while this issue works through it's less-ambitious version of the module.

Nitpick: s/location/path/, no?

Yeah we can switch this. I'm not sure why I named it location.

Not only the image_alternative entry, but all of these will only be used in the administrative section for assembling the toolbar.
Or what am I missing? Do you have bigger plans for this?

Right now everything is only used on the admin-area, that's true. But if you need to add a button to the toolbar on the front-end, an individual button image will be required since your new plugin won't have its icon included in the default sprite provided by the CKEditor library. However the binding between an icon and its plugin is done entirely through the plugin.js file, so including any integration on the Drupal-side isn't really necessary (other than for the admin section, like it is currently).

Why do the path and filename have to be specified separately? Why can't there only be file and that just also contain the path?

This reflects CKEditor's API for adding a plugin. I agree they could be combined, or we could make the file name "plugin.js" optional, as it is optional for CKEditor: http://docs.cksource.com/ckeditor_api/symbols/CKEDITOR.resourceManager.h...

For me, the strangest part, is this "enabled callback" stuff. I understand the need for it, but can't we think of something more elegant? Do we really need to allow for arbitrary logic to check whether a CKEditor plugin should be enabled?

I think an enabled callback is highly useful. It makes it so that plugins can enable themselves based on indirect settings. For example the 'webkitresize' plugin or the 'drupalimage' plugin enable themselves based on the presence of the Image button. Any module that provides a filter could enable its corresponding CKEditor plugin automatically based on the filter being enabled. The root of the purpose for the enabled callback is to avoid requiring explicit enabling of plugins, instead plugins are mostly enabled by derivative of another setting (or a combination of settings). Of course a module could provide an explicit setting if they desired, but for the most part I feel like plugins should be enabled when needed based on enabling of individual options rather than turning on a plugin. Let's say we make "Paste as plain text" an option. *Enabling* that option would *disable* the pastefromword plugin, since if everything is plain text we don't need the additional option for pastefromword. If that option was not enabled, pastefromword would be enabled by default.

Now we could generate a list of which plugins are needed at the time that the editor configuration is saved, rather than using a callback function, but I feel like the functionality of enabling/disabling plugins derived from indirect settings is an important feature to maintain.

I am personally very much fond of defines because it can prevent a lot of subtle problems. However, in this case, I don't see the point. You use it only one location.

This was caused by rerolling based on the contrib project. I'm happy to not use a constant, but we should update the contrib version too if we want to try maintaining that.

A subtle change, the "enabled callback" now also receives $plugin_name. Why is this necessary?

In case you have a collection of plugins that use the same plugin callback.

Do we all agree?

Yep I think the new scope is spot-on.

Wim Leers’s picture

Yep I think the new scope is spot-on.

Okay. Great.

So, plan of attack AFAICT:

  • You're working on the accessibility of the drag-and-drop CKEditor toolbar configuration UI (as announced at #1872206-18: Improve CKEditor toolbar configuration accessibility).
  • Jesse Beach will review your a11y work — I couldn't do it half as well as she can, because she's got far more a11y experience.
  • I'll work on writing tests. I'll also try to come up with a shell script to simplify patch rerolls :)

#29:

Right now everything is only used on the admin-area, that's true. But if you need to add a button to the toolbar on the front-end, an individual button image will be required since your new plugin won't have its icon included in the default sprite provided by the CKEditor library. However the binding between an icon and its plugin is done entirely through the plugin.js file, so including any integration on the Drupal-side isn't really necessary (other than for the admin section, like it is currently).

Well, all I was saying was:

  • *all* of those things are only used on the admin area, and that will remain true, so why do we call out one of them separately?
  • it would be nice if we didn't have to include those images in the CKEditor module, but if they'd be part of the CKEditor library; because that is where they really belong.

I *know* that this is only necessary because we cannot easily get at the sprite.

I *know* that CKEditor doesn't use any of this metadata, and that's why I think we should explicitly state for each of the things I called out that it's only used in the admin area.

Alternatively, we could go the advanced route and just ask the CKEditor folks to provide a text file with a lookup system so that we know at which coordinates which icon lives, so that we can then extract the icons programmatically (some ImageAPI/ImageCache treatment) and store those "extracted icons" somewhere in the files directory. But yeah … that's complex and most definitely a low-priority follow-up.

This reflects CKEditor's API for adding a plugin. I agree they could be combined […]

Oh! That is sensible. Can we then maybe state in the documentation that we're trying to mirror CKEditor's API there? :)

I think an enabled callback is highly useful.

I totally agree! It's just that I think there should be a more elegant way of doing it. I can't think of it, but it all still feels kinda weird to me (probably in the same way as many D8 things just feel weird to you; it just takes getting used to).
Part of my concern here is that we're introducing a new (or at least somewhat different) pattern here.

Now we could generate a list of which plugins are needed at the time that the editor configuration is saved, rather than using a callback function, but I feel like the functionality of enabling/disabling plugins derived from indirect settings is an important feature to maintain.

That would indeed answer my "elegance"/"pattern" concerns. But at the same time, it's clearly less flexible.

If I'm the only one with concerns about the "enabled callback" approach, I'd be happy to leave it as is. I just think that's one of the few "special" things in this patch, so it's worth debating to check whether everybody is on the same page with it.

sun’s picture

I'll also try to come up with a shell script to simplify patch rerolls

Use https://github.com/sun/git-subtree - that's why I strongly recommended to have a core sandbox for this entire work:

git add remote wysiwyg git.drupal.org:sandbox/123456.git
git checkout -b ckeditor origin/8.x
git push -u wysiwyg ckeditor
git subtree add --prefix=core/modules/ckeditor git.drupal.org:project/wysiwyg_ckeditor.git 8.x-1.x
git push wysiwyg ckeditor

That's how #1668292: Move simplified Profile2 module into core and other major core modules have been developed for D8. You can perform changes (e.g., remove files + code) from the subtree in your core sandbox — the pulled in subtree behaves like any other code, and upstream merges are performed in the way you expect them, regardless of whether you git merge 8.x or whether you git subtree pull --prefix=core/modules/ckeditor


Sorry, I did not have time to look at the latest patch yet. Will try to do so ASAP. IMHO, it's too early for looking into usability/accessibility issues, but ok...

Wim Leers’s picture

#31: Sure, but will that also work well when you need *a subset* of a module? I.e. we need the wysiwyg_ckeditor module, minus the plugin JS files (easy), but also with some parts of the .module removed… *That* is a lot trickier.

Wim Leers’s picture

Issue summary: View changes

Update issue summary to reflect an issue descoping that happened at the request of sun in #12.

Wim Leers’s picture

Based on the scope agreed upon in #29, I added another important follow-up task to the issue summary:

Enable the CKEditor module by default in the standard install profile, enable it as the default Text Editor for the Filtered HTML and Full HTML text formats, provide default configurations of CKEditor for both of those text formats, and potentially modify the filters/filter settings of those text formats.

jessebeach’s picture

When I try to apply the images patch in #14, it errors out as being fatally corrupted on line 103. Maybe it should be created with git-format-patch if diff was used before?

Wim Leers’s picture

#34: yeah I'm not sure why the images patch from #14 is failing. I'll have to reroll it with --binary IIRC. For now, the easiest way to get those images, is to git clone --recursive --branch 8.x-1.x http://git.drupal.org/project/wysiwyg_ckeditor.git; cp -R wysiwyg_ckeditor/images $YOUR_DRUPAL_8_GIT/core/modules/ckeditor/. Will reroll, but can't do that right now.

effulgentsia’s picture

You can also apply #13 and the interdiff from #22 on top. That gets you the latest images and code that have been submitted to this issue.

effulgentsia’s picture

From Wim:

For me, the strangest part, is this "enabled callback" stuff. I understand the need for it, but can't we think of something more elegant? Do we really need to allow for arbitrary logic to check whether a CKEditor plugin should be enabled?

From quicksketch:

I think an enabled callback is highly useful. It makes it so that plugins can enable themselves based on indirect settings. For example the 'webkitresize' plugin or the 'drupalimage' plugin enable themselves based on the presence of the Image button.

My recommendation is to do one of three things:
1. Either remove the 'enabled callback' feature from this patch and work it out in a follow up,
2. Or, if wanting to keep it in this patch, then also add a couple plugins, like the ones mentioned above, to this patch,
3. Or, if the only real plugins that need the feature would require their own lengthy review process to get into core, then to add a couple test case plugins that represent the essentials of these real ones, so that the architectural needs of 'enabled callback' are made clear via these test cases.

What I'm currently not clear on is whether:
a. We foresee other PHP callbacks/methods that CKEditor plugins could benefit from (e.g., settingsForm())
b. Whether there's a strong need for module X to define a plugin, module Y to override the 'enabled callback' of that plugin, and module Z to override some other callback of that plugin.

If there's not a clear use case for b., then generally, all callbacks should be methods on the plugin's class. If there is a use case for b., then it potentially indicates a need to do something fancier, like how we separate the different "controller"/"form controller"/"render controller"/"list controller"/etc. classes that act on Entity type plugins.

if each CKEditor plugin were required to be a Drupal plugin class, we'd end up with ~20 classes that would all be empty

Nope. We can create a base class, and make multiple plugins all use it directly. We can accomplish that with custom code on a CKEditorPluginManager class or can add generic support for that directly into Drupal\Core\Plugin\Discovery. I'll open an issue for the latter, but can also help on the former if the other issue doesn't get traction quickly enough.

quicksketch’s picture

a. We foresee other PHP callbacks/methods that CKEditor plugins could benefit from (e.g., settingsForm())

I think there's a good chance a settingsForm would be useful. Presently you're required to form_alter() the filter settings form in order to add an option, which is far from ideal. Combined with keeping the 'enabled callback', it seems like that's two arguments for using Plugins.

However, my personal aversion to plugins (annotations, 6 nested directories, cryptic naming conventions) makes me want to defer on this action. If someone would convert hook_ckeditor_plugins to use Drupal plugins, that'd be great. I would prefer to avoid the high level of risk to both my keyboard and face.

quicksketch’s picture

+    'js' => array(
+      '/core/misc/ckeditor/ckeditor.js' => array(),
+    ),

I just found this pretty serious bug. CKEditor won't load if you've installed Drupal in a sub directory. Should be DRUPAL_ROOT . 'core/misc/ckeditor/ckeditor.js'.

nod_’s picture

I'm pretty sure it's core/misc/ckeditor/ckeditor.js in other places where that happens, see system.module.

Wim Leers’s picture

FileSize
13.32 KB
45.83 KB
PASSED: [[SimpleTest]]: [MySQL] 49,305 pass(es). View

A reroll … where no more images patch is required! We just reuse CKEditor's CSS! Any arguments regarding "but what if a user wants to use skin FOO instead of the default" can be redirected to /dev/null, for several reasons:

  • the current drag-and-drop toolbar UI is specifically tailored to the Moono skin already
  • a user wanting to use a different skin must use some contrib module, because modifying core/misc/ckeditor equals to hacking core, which we never support
  • if a user wants to use a non-optimized build of CKEditor (which would break this), then that user would have to use some contrib module already anyway; and such a contrib module could easily use hook_ckeditor_plugins_alter() to make everything work just fine in that case
  • I checked all of the above with the CKEditor guys, and they confirm that this is fine, also in the long term, as long as the assumption "users only use the Drupal-provided build of CKEditor and Drupal will always include an optimized build of CKEditor".

This solves the image woes.

I also fixed #39, removed the CKEDITOR_VERSION define and made the ckeditor.api.php changes I suggested, as well as documenting the newly added image_alternative_rtl button property.

This patch does not yet address #37 and #38.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

#37/#38: I'm currently working on this, but I figured I'd let followers know and assign the issue to myself to prevent duplicate work. In the mean time, some feedback already.

However, my personal aversion to plugins (annotations, 6 nested directories, cryptic naming conventions) makes me want to defer on this action. If someone would convert hook_ckeditor_plugins to use Drupal plugins, that'd be great. I would prefer to avoid the high level of risk to both my keyboard and face.

:) No worries — I'm explored this for us in right now. I hope that what you will get to see will make sense to you.

My recommendation is to do one of three things:

I went with option 3, test plugins to demonstrate the needs.

What I'm currently not clear on is whether:
a. […]
b. Whether there's a strong need for module X to define a plugin, module Y to override the 'enabled callback' of that plugin, and module Z to override some other callback of that plugin.

If there's not a clear use case for b., then generally, all callbacks should be methods on the plugin's class. If there is a use case for b., then it potentially indicates a need to do something fancier

I personally don't see this being a common case. However, we can still accomodate for it by having the result of the plugin's "enabled callback" being alterable. Then we can retain a simple plugin structure/architecture.

Nope. We can create a base class, and make multiple plugins all use it directly. We can accomplish that with custom code on a CKEditorPluginManager class or can add generic support for that directly into Drupal\Core\Plugin\Discovery. I'll open an issue for the latter, but can also help on the former if the other issue doesn't get traction quickly enough.

This, I have no idea how to achieve, nor what that code would look like. It sounds very magical. Can you maybe clarify with pseudocode?

Wim Leers’s picture

Issue summary: View changes

Important follow-up task: enable CKEditor by default; adjust filters/filter settings accordingly.

Bojhan’s picture

Took a first look at this, its really taking shape :) Thanks for all the iterations so far!

1) Lets remove all the fieldsets and wrap all of this in one fieldset called "Text editor" within that we also put the initial select list. That way there is one grouping, not three :)

2) Lets have a "different" kind of styling for the available toolbar buttons, currently you have two gray bars - there is no clear visual distinction that signals one is the available, and the other one is the current. Perhaps we can do something visual, e.g. changing the background gradient to be white and some more spacing.

3) The "Style" option is very confusing, this seems like a nice extra option - but really used by specific use cases with people who know what they are doing. . But I dont really see a reason why we would include this in core. This would make a nice contrib module.

4) The "Format" listing is also confusing, and this is because essentially the word "Format" triggers ones perception that you are talking about all the allowed HTML tags but thats actually further down the page. Wim offered the idea of just removing this, and making "allowed tags" just fill that in - that could work, the only reservation I would have is that its hard to know which values are allowed/picked up in the "Format" select list. We should find a good solution to this, just relabing will probably not be enough it creates quite a visual impact in this fieldset.

5) Lets relabel "Format" to "Normal" in the toolbar, that way it matches what will be on the front-end WYSIWYG 99% of the time.

6) Wim mentioned we might have plugins that allow configuration (e.g. media youtube/image module - define what account/site it is connected to). I dont yet know how this will look (maybe a table?), but write me down to design it once its needed.

Wim Leers’s picture

I sat together with Bojhan today (we did a mini sprint of sorts) and we came up with the above feedback for the "CKEditor toolbar builder" (which is the term we agreed upon), because that is clearly the single biggest aspect of this module that needs usability feedback (CKEditor itself is already mature instead of usability).

I agree that the "Style list" settings is not core material; it's contrib material. It's for the "stylescombo" plugin that is part of the default (and also our) build of CKEditor. I think we should simply remove that plugin from our build and defer it to contrib.

I also created HTML prototypes of the two most significant changes:

  1. BEFORE:
    Screen Shot 2013-01-28 at 22.12.23.png
  2. A single outer "Text editor" fieldset, with the "Format list" and "Style list" settings removed and the "Format" button renamed to "Normal" because that is how it will look to the end user (Bojhan's points 1, 3, 4, 5). End result:
    Screen Shot 2013-01-26 at 16.45.16.png
  3. And, another prototype, this one with the improved styling for the available toolbar buttons (i.e. all of Bojhan's points except point 6):
    Screen Shot 2013-01-26 at 16.21.17.png

(Please ignore the visual bugs, that's because I had applied another patch.)

EDIT: added "before" screenshot. This was initially posted on January 26, 2013 at 5:14pm, CET.

wwalc’s picture

My $0.02:

Ad 5) "Format" might be a bit more clear for beginners? "Format" is displayed before the editor is focused for the first time. I guess users that already used CKEditor will have no issues with both labels ("Normal" and "Format"). Also in discussions people usually will be referring to "Format" combo not to "Normal" combo.

Ad 4) "Format list" - I'd say KISS. "Allowed HTML tags" is causing some issues here:
- one will have to make sure that "Format list" matches the allowed HTML tags list (which BTW expects a different format than "p, h1, h2, h3, h4, h5, h6").
- the default value is confusing because it does not match "Allowed HTML tags" (h1, h2, h3 are no allowed)

If one allows a set of tags, he's probably fine with seeing them in CKEditor "Paragraph Format" dropdown.

Ad 3) "Style list" - assuming that the "Format list" will be gone, this will be the only option that has left. The problem is that it might be very hard to use it because of:

A list of classes that will be provided in the "Styles" dropdown, each on a separate line. These styles should be available in your theme's editor.css as well as in your theme's main CSS file.

It means that theme developer must have done already a lot of work in order to make the classes "available" for the administrator to be used in "Style list". Why not let him specify this last "third" thing simply through API?

The situation could be a bit different in Full HTML format, where inline styles are not removed and one could be able to specify styles without even touching the theme. However it looks like this field would not accept JavaScript object like this anyway:

	{ name: 'Marker: Yellow',	element: 'span', styles: { 'background-color': 'Yellow' } },
	{ name: 'Marker: Green',	element: 'span', styles: { 'background-color': 'Lime' } },

	{ name: 'Big',				element: 'big' },
	{ name: 'Small',			element: 'small' },
	{ name: 'Typewriter',		element: 'tt' }

for the very same reason like Font / Size combos are not available.

So maybe let's get rid of this configuration option and leave it to be specified using some API?


I'm wondering whether it would not make sense to make some "useful things" like Color buttons / Size / Font combos available in D&D configuration in Full HTML (in formats where "Limit allowed HTML tags" is not enabled). After all inline styles are not removed there.

Alternatively, we might see soon an extra module that adds all these things.

quicksketch’s picture

- one will have to make sure that "Format list" matches the allowed HTML tags list (which BTW expects a different format than "p, h1, h2, h3, h4, h5, h6").

The primary purpose of putting the WYSIWYG configuration on the text format form is to make it so adjustments are made automatically to your filter configuration based on your editor configuration. This part of the UI isn't complete yet, but it's being worked on in #1894644: Unidirectional editor configuration -> filter settings syncing. So we're hoping to already have configuration changes go from the editor settings to the filter settings. I don't think we can/should automatically derive the list of tags from the filter configuration, since the list provided in the dropdown is usually significantly shorter than the list of tags, especially as some tags are given dedicated buttons (i.e. blockquote).

As for the style list, I specify a style list on at least half of the WYSIWYGs I configure, but I agree that it's highly tied into the CSS file that is provided, so switching this to a code-provided option and putting the responsibility onto a custom (or contrib) module would make sense, but seems like it would be beyond most users to configure properly. If that's the case, maybe we should remove the style dropdown entirely as a button from the available buttons, since if you can't configure it there's no point in being able to enable it.

I'm wondering whether it would not make sense to make some "useful things" like Color buttons / Size / Font combos available in D&D configuration in Full HTML (in formats where "Limit allowed HTML tags" is not enabled). After all inline styles are not removed there.

I'm not particularly a big fan of this recommendation, since inline styles are so heavily frowned upon by designers and developers alike. End-users don't give a hoot though, so maybe you're correct that we should make this buttons available. We don't have the ability yet to hide/show buttons based on the configuration of filters, so I think either way this should wait for separate issue since it has particular requirements around needing the HTML tag filter to be disabled.

Wim Leers’s picture

(Uploading a "before" screenshot for use in #44.)

effulgentsia’s picture

This, I have no idea how to achieve, nor what that code would look like. It sounds very magical. Can you maybe clarify with pseudocode?

It's actually much less magical than you think. You can create a class, e.g., Drupal\ckeditor\Plugin\ckeditor\ckeditor\CKEditorPluginBase that implements a isEnabled() method (I don't know if that's the best method name, or whether shouldEnable() or something like that would be better). Then, your CKEditorPluginManager class, instead of setting up a $this->discovery object in its constructor, could implement its own getDefinition() and getDefinitions() methods. For now, getDefinitions() could do exactly what ckeditor_plugins() does in the current patch, maybe with the addition of adding some defaults, like 'class' => 'Drupal\ckeditor\Plugin\ckeditor\ckeditor\CKEditorPluginBase'.

In other words, we bypass the typical discovery process of 1 annotated class = 1 plugin, and instead implement a custom discovery process of invoking info()/alter() hooks. This way, nothing prevents multiple info entries referencing the same class. But, we still stick to the plugin API of having a plugin manager that code can call getDefinitions() and createInstance() on. And we drop the concept of callbacks in favor of object methods.

However, we can still accomodate for it by having the result of the plugin's "enabled callback" being alterable.

No. The 'class' key would be alterable, not any individual callback. So, a module could still override, but two different modules can't simultaneously and independently override two different methods. That's the main bit of flexibility that's lost when moving from a bunch of disparate callbacks to a class. In most cases, it's not much of a loss. In cases where it is, you can architect is as multiple classes assisting a single master class, as we did with the various 'controller' classes that work on entities.

We can accomplish that with custom code on a CKEditorPluginManager class or can add generic support for that directly into Drupal\Core\Plugin\Discovery. I'll open an issue for the latter

Yeah, eventually, we'll probably want to take the custom approach I suggest above and make it a pattern that other similar plugin types could also use (essentially, any plugin type where the metadata is primary and the class methods are secondary, so that multiple plugins using the same class is a common situation). But I'd rather defer doing that abstraction until we have a couple use cases nailed down, like this one. There's some interesting stuff happening in #1845546: Implement validation for the TypedData API that also touches on some enhancements to the typical annotation-based discovery.

Wim Leers’s picture

#48:

In other words, we bypass the typical discovery process of 1 annotated class = 1 plugin, and instead implement a custom discovery process of invoking info()/alter() hooks.

For now, I solved it in a much simpler way: I just have a Internal plugin, which pretends to be a single plugin, but in fact it contains the metadata for all plugins that are bundled with our CKEditor build.
If the need arises, we can still add support for the more advanced set up you described.

No. The 'class' key would be alterable, not any individual callback.

I realized the same :)

tkoleary’s picture

Re #44,

Just wanted to make sure the comment I made in usability IRC was captured here. The separation of the buttons goes a long way to solving the problem but IMHO the order is still a usability anti-pattern. The user should first choose a button *then* drag it to the toolbar, so the logical order is to put the available buttons above the bar, not below.

Wim Leers’s picture

#50: I disagree. Order conveys importance. The thing you're working on and the end result is at the top, and IMHO this makes more sense. Then again, I'm no designer nor usability expert :)

yoroy’s picture

I think this is mostly just a choice to make where none is better than the other.

Wim Leers’s picture

FileSize
92.01 KB
100.01 KB
FAILED: [[SimpleTest]]: [MySQL] 50,371 pass(es), 18 fail(s), and 13 exception(s). View

In this (huge) reroll:

  • pluginification
  • tests
  • lots of small improvements/fixes, as well as refactoring to improve testability

NOT: #43/#44/#45, i.e. simplification/clean-up/usability improvements of the toolbar builder UI. (That's coming up next.)

Pluginification

My approach for the plugins is to use four different interfaces.

  1. CKEditorPluginInterface (methods: isInternal(), getFile() and getConfig(Editor)), i.e. for the bare minimum. A plugin is enabled (and thus loaded) if either the button is present or if some programmatically defined contextual condition is true. So for your plugin to load, you have to implement one of the two, and you can implement both if you want.
  2. CKEditorPluginContextualInterface (methods: isEnabled()) is for contextually loaded plugins.
  3. CKEditorPluginButtonsInterface (methods: getButtons()) is for plugins with buttons that must be configurable in the toolbar builder, and if one of these buttons is present, the plugin will automatically be loaded.
  4. And finally: CKEditorPluginConfigurableInterface, for an optional settingsForm().

So: yes, many interfaces, but you just pick the ones you want to use. It also helps clearly differentiating different plugins (some plugins have no buttons).

(I've already discussed this with quicksketch and overall he's +1 on this.)

Note: The Internal CKEditor plugin covers all functionality/plugins provided by the bundled build of CKEditor. Since I wanted to demonstrate what a CKEditorPluginContextualInterface-implementing CKEditor plugin would look like, I've included a setting to allow the user to choose between either CTRL+L or CTRL+K as the "create link" shortcut. Each plugin's settings end up in vertical tabs. Once I've implemented #43/#44/#45 (easy), then I'll post a screenshot as well. Note that this setting does not have to stay, I just wanted *a* setting to demonstrate it with. I would prefer this setting to be committed; it can be removed once we have other CKEditor plugins with settings in Drupal core. It's helpful to demonstrate the full range of functionality.

Tests

  • CKEditorTest.php (DUTB): tests the CKEditor class (implementation of a "Text Editor plugin")
  • CKEditorPluginManagerTest (DUTB): tests all aspects of the the CKEditorPluginManager class (which contains several convenience methods related to plugin management that better belong on that class rather than in the CKEditor class).
  • The above two test classes test all granular things. The next two test classes test the visible end results, and can thus test in a more coarse manner.
  • CKEditorLoadingTest (DWTB): tests loading of CKEditor under various configurations.
  • CKEditorAdminTest (DWTB): tests configuring of each of CKEditor's configurable aspects.
  • Note that I've included 4 CKEditor plugins for testing purposes, all llama-themed :)

Lots and lots of small fixes

  • I simplified the plugin "path + file" separation into simply getFile(). After discussing this with the CKEditor developers, it turned out that this is not really a problem at all, since they explicitly support this in their API as well (see the third example for addExternal()).
  • While working on this, I also discovered a problem with editor.module, fixed here: #1898844: No test coverage for hook_editor_js_settings_alter()
  • Removed the per-button "required_tags" flag, because: 1) it's deferred to #1894644: Unidirectional editor configuration -> filter settings syncing, hence we shouldn't include the metadata here, 2) I've talked to the CKEditor developers about this and they proposed what's described in #1894644-2: Unidirectional editor configuration -> filter settings syncing, which would make the need for this (by definition inaccurate or at least incomplete) metadata obsolete.
  • #type = fieldset -> details, as per #1892968: Fieldsets vs. Details: When to use what and why?
  • The ability for CKEditor plugins for Drupal to define CSS files to be loaded has been removed. After discussing that with the CKEditor developers, it turns out that not a single CKEditor plugin does; if a CKEditor plugin needs some additional CSS, it will do so by adding a very small number of CSS rules.
  • CKEditor::getJSSettings() used to do *a lot* of things, which impeded testability. It still exists (of course, because that's necessary to implement \Drupal\editor\Plugin\EditorInterface), but the logic to determine which plugins are enabled has now moved to CKEditorPluginManager::getEnabledPlugins(), and the logic that builds significant parts of CKEDITOR.config is moved to helper functions (that are separately unit tested).
  • CSS files for use in CKEditor were referenced by doing base_path() . $css_path, with the latter being a Drupal root-relative path. This is very, very bad (it prevents users from overriding this file and to serve these files from a CDN). Similar things happened in the loading of "external" CKEditor plugins (i.e. those plugins that are not part of the Drupal-bundled build of CKEditor). This is now using file_create_url().
  • We were setting the non-standard externalPlugins CKEditor setting. We never passed this to CKEditor, we used this in our JS to tell CKEditor to dynamically load these files. To make it crystal clear that this is a custom setting, I renamed it to drupalExternalPlugins.
aspilicious’s picture

Status: Needs work » Needs review

Did some testing. This is makes it SOOO easy to have a wysiwyg editor in core. Seriously!

Couple of things that maybe are discussed in other issues:
1) We need to to enable ckeditor by default in the standard module. MUST ;) (followup probably)
2) You can't throw away seperators in the drag and drop UI. You can create multiple but there is no way to get rid of them. (yes you can drag them back in the bottom bar but thats not the point).
3) Not sure if this is supported but is there a way to control the buttons + layout in an installation profile? I can imagine every agency would have his custom configuration.
4) All the html widgets added in the editor should be allowed. I forgot to add the h2 tag but I added it in the editor. Confused me for a second.

Again: *awesome*

Status: Needs review » Needs work

The last submitted patch, ckeditor_module-1890502-53.patch, failed testing.

Wim Leers’s picture

#54: Glad you liked it! :)
1) Yes. It's listed as the first follow-up.
2) Good point. I'm not sure if that's a limitation of jQuery Draggables or not.
3) Of course. This just uses the Text Editor module, which includes the Drupal\editor\Plugin\Core\Entity\Editor ("Editor") config entity. Hence this is config system-compatible already.
4) That's listed as the second follow-up ;) See #1894644: Unidirectional editor configuration -> filter settings syncing.

Three out of your four points were already covered by the (meticulously updated) issue summary. Please read that next time :)

Wim Leers’s picture

FileSize
27.92 KB
20.04 KB
17.19 KB
111.33 KB
FAILED: [[SimpleTest]]: [MySQL] 50,079 pass(es), 19 fail(s), and 13 exception(s). View

This reroll addresses #43/#44/#45/#46.

The only thing remaining to be done for this patch to be complete is the a11y of the toolbar builder: #1872206: Improve CKEditor toolbar configuration accessibility (plus whatever problems are raised in reviews for #53/#57, of course).

End result:
ckeditor_toolbar_builder_ui.png


#43:

1)

Lets remove all the fieldsets and wrap all of this in one fieldset called "Text editor" within that we also put the initial select list. That way there is one grouping, not three :)

This required changes to editor.module. It required changing the form structure: previously we had the "editor" form item (for selecting the text editor) and the "editor_settings" container (for the editor-specific settings), but now both have to live in the same fieldset. I did this last, and you can see the changes for this specifically in interdiff_editor_form_restructuring.txt. (Yes, I specifically chose a fieldset, because this is not about collapsing nor … details, but about grouping related form items.)
2) Done.
3) Done (removed from list of buttons + setting removed).
4) Done (setting removed; the setting is now generated automatically; Internal::generateFormatTagsSetting() has a list of possible tags, and blackbox testing with check_markup() to figure out which of those possible tags is also allowed by the text format). The format_tags setting is now also only added if the corresponding "Format" button is actually present :) Comes with updated tests, of course.
5)

Lets relabel "Format" to "Normal" in the toolbar, that way it matches what will be on the front-end WYSIWYG 99% of the time.

Not yet done, as per the feedback in #45. I'd consider this a minor problem at most.


#45:

So maybe let's get rid of this configuration option and leave it to be specified using some API?

Because Drupal core only aims to solve the core problem; this is merely tangential. Drupal core can't do everything.

I'm wondering whether it would not make sense to make some "useful things" like Color buttons / Size / Font combos available in D&D configuration in Full HTML (in formats where "Limit allowed HTML tags" is not enabled). After all inline styles are not removed there.

Drupal encourages structured content; inline styles goes against that. If people really want this, they can install a contrib module that provides this, but we don't want to provide this in Drupal core.

Alternatively, we might see soon an extra module that adds all these things.

Yep :)


#46:

I don't think we can/should automatically derive the list of tags from the filter configuration, since the list provided in the dropdown is usually significantly shorter than the list of tags, especially as some tags are given dedicated buttons (i.e. blockquote).

In this patch, it's derived automatically, but only from a small list of *possible* format tags: p, h(1-6), pre (I excluded address and div because neither of those should be encouraged when writing text). See Internal::generateFormatTagsSetting().


Also:

  • moved the subscript and superscript buttons to right after the strikethrough button, as per @wwalc's feedback.
  • moved around a few other buttons to make a bit more sense (note that this only affects the order of the buttons in the list of available buttons).
  • added comments to groups of buttons to indicate which CKEditor plugin they live in.

Another follow-up to this issue should be: A new CKEditor build should be created & included with Drupal without the following plugins:

  • placeholder (kinda useless for Drupal core
  • stylescombo (also useless for Drupal core, see #44)
jessebeach’s picture

Alright, let's get the remaining a11y issues addressed...

Status: Needs review » Needs work

The last submitted patch, ckeditor_module-1890502-57.patch, failed testing.

effulgentsia’s picture

I only did a code review of the PHP. I skipped over the JS and CSS, and only briefly skimmed the tests. And I didn't do any manual testing. Overall, I think the architecture, code, and docs are all in excellent shape. Really nice to see this this close to ready! What's below is pretty much just nits.

+++ b/core/modules/ckeditor/ckeditor.admin.inc
@@ -0,0 +1,173 @@
+  global $language;
+  $variables['language_direction'] = isset($language->direction) && $language->direction === LANGUAGE_RTL ? 'rtl' : 'ltr';

There's no global $language in D8. What you're probably wanting here is language(LANGUAGE_TYPE_INTERFACE).

+++ b/core/modules/ckeditor/ckeditor.admin.inc
@@ -0,0 +1,173 @@
+  // Assemble items to be added to active button rows.
+  foreach ($variables['active_buttons'] as $row_number => $row_buttons) {
...
+  }
+  // Assemble list of disabled buttons (which are always a single row).
+  foreach ($variables['disabled_buttons'] as $button_name => $button) {
...
+  }
+  // Assemble list of multiple buttons that may be added multiple times.
+  foreach ($variables['multiple_buttons'] as $button_name => $button) {
...
+  }

Seems like there's some duplication here (in the "..." parts) that could be cleaned up with a helper function or some other technique.

+++ b/core/modules/ckeditor/ckeditor.api.php
@@ -0,0 +1,63 @@
+ * Note that because this hook is only called for modules and the active theme,
+ * front-end themes will not be able to use this hook to add their own CSS files
+ * if a different admin theme is active. Instead, front-end themes and base
+ * themes may specify CSS files to be used in iframe instances of CKEditor
+ * through an entry in their .info file:

- Firstly, drupal_alter() does invoke the alter hook implementations of base themes of the active theme, so the part about base themes seems wrong.
- Secondly, what are the situations in which the front-end theme would want to add CSS files to an editor appearing on an admin page that is themed by the admin theme? There may well be good use-cases for that, but if there are, let's add one here as an example.

+++ b/core/modules/ckeditor/ckeditor.module
@@ -0,0 +1,114 @@
+function ckeditor_page_build(&$page) {
+  // Add our CSS file that adds common needed classes, such as align-left,
+  // align-right, underline, indent, etc.
+  $page['#attached']['library'][] = array('ckeditor', 'drupal.ckeditor.css');
+}

Do we have a followup issue to clean this up? Seems odd and not ideal to be adding CSS to every page, just because CKEditor can be used for editing some content. Furthermore, if this is needed because CKEditor inserts markup that we don't transform to something compatible with Drupal core styling, then does that mean if someone disables CKEditor module, then suddenly content that had been edited with it in the past becomes poorly styled?

+++ b/core/modules/ckeditor/ckeditor.module
@@ -0,0 +1,114 @@
+      $css = array_merge($css, _ckeditor_theme_css($info['base theme']));

Reverse the arguments to array_merge(), so that theme css overrides base theme css.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginContextualInterface.php
@@ -0,0 +1,39 @@
+ * Contextually enabled CKEditor plugins can be enabled off an explicit setting,

s/off/via|based on/

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php
@@ -0,0 +1,155 @@
+   * @param bool $exclude_internal_plugins
+   *   Defaults to TRUE. When set to FALSE, plugins whose isInternal() method
+   *   returns TRUE will also be included.

I don't know if we have a standard on positive vs. negative flags, but I think $include_internal_plugins defaulting to FALSE would make more sense here.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php
@@ -0,0 +1,155 @@
+   * @return array
+   *   A list of the enabled CKEditor plugins, with the plugin IDs as keys and
+   *   the Drupal root-relative plugin files as values.
+   *   For internal plugins, the value is NULL.
+   */
+  public function getEnabledPlugins(Editor $editor, $exclude_internal_plugins = TRUE) {

If what we're returning is just the files, can we name this function getEnabledPluginFiles()?

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php
@@ -0,0 +1,155 @@
+   *   A list of the CKEditor plugins that implement buttons, with the plugin
+   *   IDs as keys and lists of button metadata (as implemented by getButtons())
+   *   as values.

This doc doesn't match the implementation, in which button metadata is actually returned in a 'buttons' subkey.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.php
@@ -0,0 +1,155 @@
+  public function getButtonsPlugins(Editor $editor) {
+    $plugins = array_keys($this->getDefinitions());
+    $configurable_plugins = array();

s/$configurable_plugins/$buttons_plugins/?

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/ckeditor/plugin/Internal.php
@@ -0,0 +1,321 @@
+    // Defaults.
+    $config = array('link_shortcut' => 'CTRL+L');
+    if (isset($editor->settings['plugins']['internal'])) {
+      $config = $editor->settings['plugins']['internal'];
+    }

Can this be managed during the plugin instantiation process (i.e., have the caller of createInstance() call createInstance($plugin_id, $editor->settings['plugins'][$plugin_id]))? The constructor can then apply defaults in $this->configuration.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/editor/editor/CKEditor.php
@@ -0,0 +1,200 @@
+  function getDefaultSettings() {

For all functions in this class, please explicitly scope as public or protected.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/editor/editor/CKEditor.php
@@ -0,0 +1,200 @@
+    global $language;

See earlier comment about no global $language in D8.

quicksketch’s picture

Thanks for the great review @effulgentsia.

- Secondly, what are the situations in which the front-end theme would want to add CSS files to an editor appearing on an admin page that is themed by the admin theme? There may well be good use-cases for that, but if there are, let's add one here as an example.

Often times the front-end theme uses a significantly different font and colors (say a dark theme instead of a light one). So the front-end theme may want to provide CSS that is used in the iframe of the editor, so that the appearance lines up more closely to the front-end.

Do we have a followup issue to clean this up? Seems odd and not ideal to be adding CSS to every page, just because CKEditor can be used for editing some content. Furthermore, if this is needed because CKEditor inserts markup that we don't transform to something compatible with Drupal core styling, then does that mean if someone disables CKEditor module, then suddenly content that had been edited with it in the past becomes poorly styled?

I thought about this one a bit, and I'm still not sure that putting the CSS file in CKEditor module was the right location. All of the classes in that CSS file *could* be generic across Drupal, including things like "align-right" and "underline". There's nothing that is theme-specific, it's purely functional CSS that is used to avoid all use of inline styles that WYSIWYGs have a bad habit of using. The only reason why I included it in CKEditor module was because there's no guarantee that all editors will need to use all of these functional styles (not all editors have align button for example). However I agree that if you turned of CKEditor module, it would result in a very bad consequence of causing your styles to be lost. I hadn't thought about that. Worse, other editors might provide a CSS file identical to this one that would just override each other with the same styles.

So I think a good question is, where *is* a good place for generic, functional content styles like this? I do feel like these styles are all common enough that they should be included on every page of the site. They're also so functional that I don't think they're likely to be overridden by a theme, *maybe* grouping them into system.base.css might make sense.

I'm still crazy busy this week. Wim said he'll be doing a reroll based on your feedback.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6.03 KB
111.32 KB
PASSED: [[SimpleTest]]: [MySQL] 48,805 pass(es). View

Solely test fixing in this reroll.

Next: addressing #60.


Test breakage is seemingly entirely caused by our test framework delivering different results depending on the modules enabled in the actual site. The order of plugins found is different when the CKEditor module is enabled in the actual site or not! (If ever there were a place to use multiple exclamation marks, so here they are: !!!!!!!!)

Note that all tests were green locally, but once I disabled the CKEditor module, then reran tests, I got failures in CKEditorPluginManagerTest.

After a very fruitful chat with EclipseGc, amateescu and tim.plunkett, I now also know why the other tests failed: I'm using CKEditorBundle.php, but I should be using CkeditorBundle.php, because of ContainerBuilder::camelize() (http://drupalcode.org/project/drupal.git/blob?f=core/lib/Drupal/Core/Dru...). The bundle was discovered fine on my system because it uses a case-insensitive file system, but the PIFR servers use a case-sensitive file system, hence they don't find the file they're looking for, hence test failures.

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability, -wysiwyg, -accessibility, -needs accessibility review, -Spark, -CKEditor in core

The last submitted patch, ckeditor_module-1890502-62.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +wysiwyg, +accessibility, +needs accessibility review, +Spark, +CKEditor in core

#62: ckeditor_module-1890502-62.patch queued for re-testing.

Wim Leers’s picture

Either HEAD is broken, or I somehow broke BlockUpgradePathTest, or testbot is acting up again. All the CKEditor tests passed though, so #62 is a success :)

Wim Leers’s picture

FileSize
21.45 KB
110.11 KB
PASSED: [[SimpleTest]]: [MySQL] 48,807 pass(es). View

This patch addresses effulgentsia's feedback in #60.


#60:

If I didn't reply to something, I fixed it.

- Firstly, drupal_alter() does invoke the alter hook implementations of base themes of the active theme, so the part about base themes seems wrong.
- Secondly, what are the situations in which the front-end theme would want to add CSS files to an editor appearing on an admin page that is themed by the admin theme? There may well be good use-cases for that, but if there are, let's add one here as an example.

quicksketch already addressed the second point in #61. The first point is fixed. I've only changed the docblock to now read like this:

 * Front-end themes (and base themes) can easily specify CSS files to be used in
 * iframe instances of CKEditor through an entry in their .info file:
Do we have a followup issue to clean this up? Seems odd and not ideal to be adding CSS to every page, just because CKEditor can be used for editing some content. Furthermore, if this is needed because CKEditor inserts markup that we don't transform to something compatible with Drupal core styling, then does that mean if someone disables CKEditor module, then suddenly content that had been edited with it in the past becomes poorly styled?

quicksketch also addressed this in #61 (it's essentially necessary to avoid the EVIL of inline styles), but as he says, it's very hard to come up with a good solution for this. He suggested including it in system.base.css, which I think is a fair trade-off (at least if we remove the caption-related classes). A lot of content (w|c)ould want to use these classes, and they're definitely not CKEditor-specific.
Can you think of a better solution?
Should we defer this to a (critical) follow-up issue?

If what we're returning is just the files, can we name this function getEnabledPluginFiles()?

I'd rather not. This function can be used in two different ways: to retrieve the CKEditor plugin JS files, or to get a list of all enabled plugins (by just looking at the array keys). Both types of information are essential, and I feel getEnabledPlugins() is a better name because of that than getEnabledPluginFiles(). I'm open to suggestions, of course.

Can this be managed during the plugin instantiation process (i.e., have the caller of createInstance() call createInstance($plugin_id, $editor->settings['plugins'][$plugin_id]))? The constructor can then apply defaults in $this->configuration.

But the defaults can be dependent on $editor (and by extension, $format — the text format). So I'm not sure that's a good idea. If we really want this, then IMHO we must add a getDefaultSettings() method to CKEditorPluginInterface. I'm not sure that additional architectural weight is worth it though — right now, we have the absolute bare minimal interfaces (i.e. bare minimal number of methods for the CKEditor plugin module developer to implement); this would break that.

For all functions in this class, please explicitly scope as public or protected.

Done; you may be surprised to see two helper functions be declared public. The reason: it simplifies testability. Otherwise I have to test this humongous end result, now I can test the edge cases of these smaller pieces of functionality much better.


More reviews, please? :)

sun, quicksketch? :)

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability, -wysiwyg, -accessibility, -needs accessibility review, -Spark, -CKEditor in core

The last submitted patch, ckeditor_module-1890502-66.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +wysiwyg, +accessibility, +needs accessibility review, +Spark, +CKEditor in core

#66: ckeditor_module-1890502-66.patch queued for re-testing.

eojthebrave’s picture

Status: Needs review » Needs work

Seems like this is coming along really well. I haven't installed and played around with the latest version of the patch yet but here's a couple of things I noticed in the code.

Also, Llama. :)

+++ b/core/modules/ckeditor/js/ckeditor.admin.jsundefined
@@ -0,0 +1,101 @@
+      /**
+       * jQuery Sortable stop event. Save updated toolbar positions to the textarea.

Comment should wrap

+++ b/core/modules/ckeditor/js/ckeditor.admin.jsundefined
@@ -0,0 +1,101 @@
+  detach: function (context, settings) {
+    // @todo
+  }

Does this need to be addressed before this is RTBC?

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginInterface.phpundefined
@@ -0,0 +1,69 @@
+ * This is the most basic CKEditor plugin interface; it provides the bare
+ * minimal information. Solely implementing this interface is not sufficient to

I think this should read "bare minimum" instead of "bare minimal"

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorAdminTest.phpundefined
@@ -0,0 +1,147 @@
+    $this->assertTrue(((string) $options[0]) === 'None', 'Option 1 in the he Text Editor select is "None".');
+    $this->assertTrue(((string) $options[1]) === 'CKEditor', 'Option 2 in the he Text Editor select is "CKEditor".');
+    $this->assertTrue(((string) $options[0]['selected']) === 'selected', 'Option 1 ("None") is selected.');

"in the he" looks like a typo.

Keep up the awesome work Wim!

Powered by Dreditor.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
32.85 KB
29.94 KB
120.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,851 pass(es). View

I discussed the current state of the CKEditor module with both sun and quicksketch last night. One important aspect they both adamantly agree on, is that the "Styles dropdown" (of the stylescombo plugin for CKEditor) functionality is a >90% use case for Drupal sites. I disagree, and Bojhan does too, but then again, we have less site building experience (or at least I have).
Later, I asked nod_ what he thought. He very strongly agreed with quicksketch and sun. So many smart people can't all be wrong. So I'm adding a UI to configure that setting — it will only appear if you drag the "Styles" dropdown/button into the active toolbar.
I also brought this up with Bojhan and he's fine with proceeding that the assumption is that this is a >=80% use case.


As per the above, in this reroll:

  • Nitpicks of #69 addressed.
  • Got rid of the (very silly) '"Create link" keyboard shortcut' setting (that now defaults to CTRL+K all the time, instead of CKEditor's default CTRL+L — CTRL+K is used by Microsoft Word and Google Docs, so we probably want to do the same).
  • Added a StylesCombo plugin to the CKEditor module, which is
  • Added drupal.ckeditor.stylescombo.admin.js, which implements two Drupal behaviors: one to ensure the "Styles" settings are only shown when the Styles dropdown/button is actually present in the toolbar; the other to provide a summary for the vertical tab.
  • CKEditor module's drag-and-drop toolbar builder now triggers an event whenever a button is added or removed.
  • public/protected is now specified also for the Internal and StylesCombo classes (i.e. CKEditor plugin classes).
  • Renamed the vertical tabs from "CKEditor plugin settings" to "Optional settings".
  • I forgot tests for *one* thing: the one that covers CKEDITOR.config.format_tags — done now.

And this is what it looks like:

ckeditor_toolbar_builder_ui_with_styles.png

Status: Needs review » Needs work
Issue tags: -Needs usability review, -Usability, -wysiwyg, -accessibility, -needs accessibility review, -Spark, -CKEditor in core

The last submitted patch, ckeditor_module-1890502-70.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +Usability, +wysiwyg, +accessibility, +needs accessibility review, +Spark, +CKEditor in core

#70: ckeditor_module-1890502-70.patch queued for re-testing.

plach’s picture

I'm wondering whether #1874562: Upgrade path broken and yet tests pass could have a role in this mess.

Status: Needs review » Needs work

The last submitted patch, ckeditor_module-1890502-70.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review

#70: ckeditor_module-1890502-70.patch queued for re-testing.

Bojhan’s picture

Issue tags: -Needs usability review

I have looked at this several times over the course of its progress, and I like the direction it has taken. Although there are probably plenty of further tweaks (e.g. I don't like VT's inside fieldsets) - for an initial commit this is ready to go from my pov.

Wim Leers’s picture

FileSize
26.31 KB
129.85 KB
FAILED: [[SimpleTest]]: [MySQL] 48,871 pass(es), 1 fail(s), and 0 exception(s). View

a11y feedback on the toolbar builder was happening over at #1872206: Improve CKEditor toolbar configuration accessibility. It's now been deemed ready by mgifford, so I'm now merging jessebeach's a11y work into this patch! :)

RTBC is in sight!

Status: Needs review » Needs work
Issue tags: -Usability, -wysiwyg, -accessibility, -needs accessibility review, -Spark, -CKEditor in core

The last submitted patch, ckeditor_module-1890502-78.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

#78: ckeditor_module-1890502-78.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +wysiwyg, +accessibility, +needs accessibility review, +Spark, +CKEditor in core

The last submitted patch, ckeditor_module-1890502-78.patch, failed testing.

webchick’s picture

FFS, these random upgrade path failures are going to make me kill something.

webchick’s picture

Status: Needs work » Needs review

#78: ckeditor_module-1890502-78.patch queued for re-testing.

nod_’s picture

FileSize
21.96 KB
127.5 KB
PASSED: [[SimpleTest]]: [MySQL] 48,818 pass(es). View

JS review. Made a few changes, not sure I haven't broken anything since I can't test anything somehow.

Removed an uneeded level of closure, fixed JSLint warnings, moved function so that they are declared before being used. Fixed libraries dependencies. and a couple of code style changes.

nod_’s picture

Issue summary: View changes

Moved filter settings syncing screenshot + explanation to #1894644.

mgifford’s picture

Removing "needs accessibility review" - there are some minor known issues, but those are now listed elsewhere.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

I filed follow ups for what I didn't feel was adequately addressed in #60, and for a new bug I discovered in manual testing: #1905022: CKEditor fails to render customized toolbar.

As usual, I feel our process works better with handling loose ends like that in follow ups, rather than continually rerolling a big patch. Given we're green (after many bot hiccups) and several people here have also reviewed without identifying showstoppers, RTBC!! Great work, everyone!

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Issue summary: View changes

Added proposed commit message

sun’s picture

I have quite significant problems with some of the code.

With that, I mean points like:

- The concept of that ckeditor.css does not feel right and will have problematic consequences. It should not be loaded, and probably should not exist.

- The administration UI doesn't work at all for me in manual testing. I'm also not able to make sense of what some of the buttons are meaning and there are no titles/tooltips. There's also some unnecessary fieldset/container/whatnot wrapping going on. And clicking on buttons makes the page jump to the top...

- The default toolbar contains buttons it should not contain.

- The toolbar buttons have too much horizontal spacing to each other within a button group.

Those are just the UI issues; I haven't looked at the code yet. I think this needs quite a bit more work.

sun’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
10.03 KB
127.72 KB
FAILED: [[SimpleTest]]: [MySQL] 48,897 pass(es), 16 fail(s), and 0 exception(s). View

At minimum, we need this.

It looks like @nod_'s last-minute JS changes broke the administrative toolbar builder. I stopped debugging and added a @todo instead.

Dries’s picture

This is a huge step forward for Drupal. :-)

sun's first bullet point in #87 talks about the code - I agree that the styling shouldn't live in ckeditor.css as that would get removed if you disbable CKEditor or switch to another editor at a future point in time.

sun's other three bullet points are about the UI and default configuration. I just tried the patch and I agree that there are unnecessary fieldsets (e.g. in the text format and editors configuration screens), that tooltips would be handy, etc. However, I don't understand what is wrong with the default buttons (maybe the alignment buttons could be removed?) and I don't have issues with the spacing.

So I agree with sun's remarks in #87, although I wouldn't say they are "quite significant problems" (except for the first bullet).

nod_’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
127.62 KB
FAILED: [[SimpleTest]]: [MySQL] 49,055 pass(es), 14 fail(s), and 0 exception(s). View

Sorry about that, fixed the todo.

hass’s picture

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/ckeditor/plugin/Internal.php
@@ -72,7 +72,7 @@ public function getConfig(Editor $editor) {
   public function getButtons() {
     $button = function($name, $direction = 'ltr') {
-      return '<a href="#" class="cke-icon-only cke_' . $direction . '" role="button" aria-label="' . $name . '"><span class="cke_button_icon cke_button__' . str_replace(' ', '', $name) . '_icon">' . $name . '</span></a>';
+      return '<a href="#" class="cke-icon-only cke_' . $direction . '" role="button" title="' . $name . '" aria-label="' . $name . '"><span class="cke_button_icon cke_button__' . str_replace(' ', '', $name) . '_icon">' . $name . '</span></a>';
     };

Any reason for not using l() or other themable function?

Status: Needs review » Needs work

The last submitted patch, core-ckeditor-1890502-89.patch, failed testing.

Wim Leers’s picture

#87/#89:

Can you point me to which fieldsets/containers are unnecessary? Note that the vertical tabs exist because it needs to accommodate for many plugins being installed.

RE: ckeditor.css: see #61, point 2. effulgentsia already created a follow-up for it to maybe move it to system.base.css. Why a follow-up? Because it is likely to result in bikeshedding.

Tooltips: that can easily be a follow-up. But of course I agree; for less obvious icons this would require guessing. For screenreader users, it's already pronounced though.

Default buttons: that will be addressed in the first follow-up, where we will enable it by default in the standard profile, and where we likely need to adjust the text format configuration as well.

Too much horizontal spacing: no. That's an opinion, not a fact. Bojhan and tkoleary, strongly visual people, did not raise this. It looks identical to the "real" CKE. In any case, this is minor at most.

#91: because the intent is not to link, but to generate markup that CKEditor's CSS will apply to.

jessebeach’s picture

Status: Needs work » Needs review

#90: core-ckeditor-1890502-89.patch queued for re-testing.

jessebeach’s picture

FileSize
46.12 KB

#87-2

The administration UI doesn't work at all for me in manual testing. I'm also not able to make sense of what some of the buttons are meaning and there are no titles/tooltips. There's also some unnecessary fieldset/container/whatnot wrapping going on. And clicking on buttons makes the page jump to the top...

I see tooltips when hovering over buttons.

Screenshot of the ckeditor configuration form. A button is hovered and a tooltip describing the button is present.

Status: Needs review » Needs work

The last submitted patch, core-ckeditor-1890502-89.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review

#87-2

There's also some unnecessary fieldset/container/whatnot wrapping going on.

At least for the "Button configuration" fieldset, I found it necessary to wrap the configuration in order to have some threshold to breach in order for the screen reader to announce the description of the form elements. Here's a video of the description being read. The sound is terrible, sorry for that.

http://screencast.com/t/EEWj8RzVy33Q

effulgentsia’s picture

FileSize
9.73 KB
128.75 KB
PASSED: [[SimpleTest]]: [MySQL] 49,227 pass(es). View

This one:
- Adds the fix from #1905022-4: CKEditor fails to render customized toolbar.
- Restores ckeditor_page_build(), but adds a @todo linking to #1904976: Move CSS required by CKEditor-edited text to an appropriate location.
- Updates tests to work with the new default buttons of #88.

Additionally, per #95, looks like #90 fixed the missing tooltips and "page jump to the top" problems.

There's also some unnecessary fieldset/container/whatnot wrapping going on.

If I understand this right, it's referring to the "Text Editor" fieldset looking ugly on its own as it cuts the visual flow between the "Roles" checkboxes above it and the "Enabled filters" checkboxes below it. And it's made even uglier with a "Button Configuration" fieldset nested within it. Unless someone comes up with a brilliant quick fix to that that everyone can agree on, can we please punt that cleanup to a followup?

Any reason for not using l() or other themable function?

#93 answered why it's not l(). Whether it should be more easily themable though is a good question. On the one hand, I don't know how much themes could get away with customizing CKEditor toolbar markup without breaking functionality. On the other hand, some themer somewhere is likely to want to customize it to whatever extent CKEditor allows. Let's open a followup to discuss this further.

nod_’s picture

FileSize
128.79 KB
PASSED: [[SimpleTest]]: [MySQL] 49,223 pass(es). View
2.22 KB

Ok I double checked manually all the replacements I made in the JS with regards to events, a few more replacements to make sure the element selected is the same as before.

Grayside’s picture

Going off the screenshot in #70, should there really be default configuration that indicates using an h1 tag in the wysiwyg? I thought the theme and node title was relied on for that.

Status: Needs review » Needs work
Issue tags: -Usability, -wysiwyg, -accessibility, -Spark, -CKEditor in core

The last submitted patch, core-ckeditor-1890502-99.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +wysiwyg, +accessibility, +Spark, +CKEditor in core

#99: core-ckeditor-1890502-99.patch queued for re-testing.

quicksketch’s picture

Going off the screenshot in #70, should there really be default configuration that indicates using an h1 tag in the wysiwyg? I thought the theme and node title was relied on for that.

In the near future (or the present already in the world of Webkit/Mozilla/IE10+), H1s are the semantically correct tag inside of each section of the page. So you always start at H1 within your particular section (or article) such as a node post. See the W3C Spec on Sections and H1 - H6 titles. Considering A) it's logical to start at H1 if you haven't been told otherwise (i.e. 99% of the population) and B) starting at H1 is the recommended approach for HTML5, we should definitely leave H1 in the default list.

Wim Leers’s picture

#98:

If I understand this right, it's referring to the "Text Editor" fieldset looking ugly on its own as it cuts the visual flow between the "Roles" checkboxes above it and the "Enabled filters" checkboxes below it. And it's made even uglier with a "Button Configuration" fieldset nested within it. Unless someone comes up with a brilliant quick fix to that that everyone can agree on, can we please punt that cleanup to a followup?

This was done as per Bojhan's feedback — see #43 & #44.

So we have two fieldsets:
The first one is the "Text Editor" one. This one is essential to indicate which part of the form relate to the text editor. i.e. it ensures it's visually clear which form items are for the text editor vs. the text format.
The second one is the "Button Configuration" one. This one is essential for accessibility — see jessebeach's explanation at #97.

#100: that's not default configuration; by default it's *empty*, because it's impossible for us to guess what styles a theme provides. I had just typed that there for screenshotting purposes. That being said, #103 makes good points too as to why that would in fact be valid :)

sun’s picture

FileSize
9.33 KB
128.25 KB
FAILED: [[SimpleTest]]: [MySQL] 49,276 pass(es), 4 fail(s), and 0 exception(s). View
  1. On ckeditor.css:
    I do not think we want to support that in core. That is, because it has long-term consequences on maintenance and updates. The CSS classes are specific to CKEditor. I (rather strongly) believe that any kind of text editor should only produce proper HTML markup (i.e., elements). It is left to [custom] themes to display that markup in the way they want. The Styles plugin is exactly in line with that: Nothing by default, hence no permanent styles to maintain in core — if you use it, then your theme(s) need to implement the styles in the way they were intended, and if they ever cease to work, then it's guaranteed to be your own fault, not core's.

    Therefore, ckeditor.css should be left out of this patch. If absolutely needed, then we should create a dedicated follow-up issue, in order to discuss whether it can be added, what can be in there, and how we're going to handle its future consequences.

  2. Also tested the UI once more with the latest patch — works a bit better now, but:

    The "Optional settings" label/container appears permanently, but no matter what I do, it is always empty. I tried enabling, disabling, clicking, double-clicking buttons. Oh. Now I tried saving the entire format and re-editing it, and suddenly there are optional settings for the Styles plugin. Looks like something in the toolbar builder JS is still not working correctly.

    This is not fixed in attached patch.

  3. I also agree with all others that the "Available buttons" should appear before the "Active toolbar". For one, I've seen many toolbar builders in other apps and this has always been the order presentation. Secondly, there is a direct connection between the active toolbar and the (optional) settings right below. The visual/vertical flow should simply be: Disabled » Enabled » Settings (of Enabled).

  4. Also cleaned up interface texts and removed one wrapping fieldset.

    The amount of custom markup generation in theme_ckeditor_settings_toolbar() still doesn't sit right though. Most of that should use standard render elements and standard theme components instead. We can and should also change theme_item_list() to optionally allow to output an empty list. That doesn't have to be cleaned up prior to commit, but there should be a follow-up issue for that.

sun’s picture

FileSize
35.04 KB

Sorry, forgot screenshot:

ckeditor-toolbar.png

Status: Needs review » Needs work

The last submitted patch, ckeditor.patch, failed testing.

quicksketch’s picture

On ckeditor.css:
I do not think we want to support that in core. That is, because it has long-term consequences on maintenance and updates.

Hm, this means that we would have no ability to use any of the following buttons: left/right/center align, indent/dedent, or underline. All of these require either a very basic class to perform their functionality or inline styles. By default, CKEditor produces inline styles and we've made it use these classes. Users expect to be able to have this functionality out-of-the-box.

This latest patch has broken indent/dedent and underlign by removing its classes, but we still have these lines that specify classes to CKEditor:

+      'indentClasses' => array('indent1', 'indent2', 'indent3'),
+      'coreStyles_underline' => array('element' => 'span', 'attributes' => array('class' => 'underline')),

The result is that when you click these buttons, nothing happens.

I'm not certain ckeditor.css is the right place for the CSS, but the CSS must be included somewhere. We can make moving it a followup task, make a new file owned by editor or system, or (IMO the best location) move it to system.base.css, since it already includes our other functional styles like clearfix, nowrap, container-inline, and element-hidden.

sun’s picture

Hm, this means that we would have no ability to use any of the following buttons

Yes. And this gets us back to a larger discussion of what the target audience of Drupal is. Based on my experience, the vast majority of site builders do not want to hand such formatting/layout tools to their users, since they're guaranteed to break the layout/design/theme, as users will use them in flabbergasting ways. Instead, you provide a very limited but at the same time very powerful set of formats/styles, which result in dedicated + custom styling, for which your entire site + theme + layout + CSS is prepared.

This also circles into responsive design and mobile, because, alas, the .indentationX styles in ckeditor.css are defined with absolute pixel values. Speaking of these in particular, .indentation1, .indentation2, .indentation3 is a rather weird construct that one would normally solve with wrapping containers and a single .indentation class instead. So that's not the kind of markup that core should produce by default. Also, we'll notice that comment.css implements an .indentation class already, with slightly different markup semantics, etc.pp.

Anyway, back to the point: I wholeheartedly disagree with the statement of "the CSS must be included somewhere", and instead, I'm saying that core should lead by example. It should not "enable" users to produce markup/content that shouldn't exist on a well formatted/styled site in the first place. Exactly for the same reasons for why the "Font color" and "Background color" buttons are not included either.

We can discuss this topic further in a dedicated follow-up issue, but I do not think that ckeditor.css should be part of this patch.

quicksketch’s picture

We can discuss this topic further in a dedicated follow-up issue, but I do not think that ckeditor.css should be part of this patch.

This CSS is required for underline, align, and indent buttons to work. If we do not include it, all the buttons I listed above won't do anything; meaning they'd all need to be removed (which I do not agree with). We should provide the buttons and if site-builders choose not to use them then that's up to them.

+      '#description' => t('A list of classes that will be provided in the "Styles" dropdown. Enter one class on each line in the format: Label|element.class. Example: Title|h1.title.<br />These styles should be available in your theme\'s CSS file.'),

We should flip the order of these properties. In Options module (and Webform module) we specify key|value pairs with the machine name on the left and the human-readable label on the right.

nod_’s picture

@sun, thanks to you css files are trivial to override from the theme. Why not include it and document the fact that a theme can override it easily? Like you said, some will use pixels, ems or others responsive styling.

quicksketch’s picture

An additional thing that is missing in the current patches: The RTL styling for CKEditor admin forms was lost. The keyboard and accessibility porting from #1872206: Improve CKEditor toolbar configuration accessibility seems like it must have accidentally dropped it.

aspilicious’s picture

Yeah a wysiwyg in core without "left" "center" or "right" buttons will be a huge WTF. So I don't agree with removing that styling.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.46 KB
127.72 KB
PASSED: [[SimpleTest]]: [MySQL] 49,312 pass(es). View

The biggest obstacle that was raised since #86 (i.e. when this patch was first RTBC) is this one: ckeditor.css.

The arguments in #105.1, #108, #109 and #110 are excellent — thanks :)

I tried to take all these arguments, as well as feedback from Bojhan and from the CKEditor developers into account while coming to a solution that is hopefully acceptable to all.

It boils down to this: alignment is layout control. Underline is presentational. Both layout & presentation should be controlled (decided) by the theme. Alignment even has the potential to break/uglify things in responsive themes. Generally: if something makes sense to be fully controlled by the front-end theme, it should be impossible to easily add a button for it, because it must depend on the front-end theme. Let's call this "The Theme Test".

I felt that indentation (i.e. indent/dedent) is an essential piece of functionality though (i.e. nested lists are always feasible/needed/allowed), so I wanted to take sun's comments on that to heart (i.e. get rid of the indent1, indent2 and indent3 classes) and just have CKEditor add nothing special to nested lists; and just do that: nest them. Have the CSS take care of the indentation (e.g. use the ul ul selector).
Then I discovered the (to me) weird and unknown fact that CKEditor in fact does not set these indentation levels on nested lists, but on top-level lists only, as well as p, h(1,2,3,4,5,6), pre and div tags! (Insert more exclamation marks here.) Clearly, that's very much undesirable for Drupal.
Maybe I was the only person not having realized this. In any case, I should have noticed this — my apologies.


Here is my full proposal (already reflected in attached patch). Take into account the different classes provided by ckeditor.css:

1. .align-(left|right|center|justify) {…}
2. img.align-(left|right|center) {…}
3. img.full-width {…}
4. .underline {…}
5. .indent(1|2|3) {…}
6. .caption-(left|right|center) {…}
  1. Alignment of text (selector 1 above): Theme Test Result: belongs in theme. FredCK (from CKEditor) agrees by the way:
  2. My personal recomendation is removing alignment features by default. Alignment is a bad thing, generally.

    Action: get rid of the CSS, disable it by removing the "justify" plugin entirely, thus also getting rid of the "align left/right/center/block" buttons.

  3. Alignment of images (selector 2 above): Theme Test Result: belongs in theme. Action: get rid of the CSS, disable it once http://dev.ckeditor.com/ticket/9990 is fixed in CKEditor, by disallowing the style and align attributes on image tags.
  4. Underline (selector 4 above): Theme Test Result: belongs in theme. Action: get rid of the CSS, don't ever allow users to add the Underline button via the CKEditor toolbar builder UI. Advanced users can add it by manually setting the CKEditor toolbar configuration.
  5. Indentation (selector 5 above): Theme Test Result: belongs in theme. Action: get rid of the CSS, configure CKEditor's "indent" plugin to allow only lists to be indented, without ever adding any attributes, i.e. the indent/dedent actions should just cause a nested <ul> or <ol> to be inserted (I don't think anybody wants to be able to indent <h2>s, for example). That is blocked on http://dev.ckeditor.com/ticket/10027.
  6. Selectors 3 and 6 are for additional CKEditor plugins that are not part of this issue anymore as of #12. Hence they should in fact have been removed already back then.

I do know that at least quicksketch believes that the inability to use the text alignment buttons and the underline button is a problem. However, we all agree with the fact that the other functionality introduced by this patch is useful. So let's discuss the reintroduction of those in a follow-up issue: #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default).


Other feedback:

#105.2: I cannot reproduce your problems. The behavior is simple: by default, plugins that have settings forms -> these settings forms are always visible. If your plugin depends on e.g. a specific button being enabled, then you can write JS to only show the settings form if that button is enabled. The StylesCombo plugin/Styles button does this. You don't have to save anything; just enabling the Styles button should be enough. This works reliably for me.

#105.3: I disagree with your generalization; but I do agree with your visual/vertical flow argument. Thanks for implementing that!

#105.4: I disagree with your removal of the "Text Editor" wrapping fieldset. The text format administration form already was a visual mess, but with all the text editor stuff just being injected there without a wrapping fieldset just makes it worse. I'm okay committing this with the changes you made, but the text format administration form should definitely be improved in follow-up issues. Fortunately, there's already #1834682: Consolidate filter options in the UI when configuring a format that addresses most of that, so we can continue this part of the problem space there.

#110:

We should flip the order of these properties. In Options module (and Webform module) we specify key|value pairs with the machine name on the left and the human-readable label on the right.

Done.

#112:

An additional thing that is missing in the current patches: The RTL styling for CKEditor admin forms was lost.

I can't find a ckeditor.admin-rtl.css file. It's not in the first patch you posted at #1872206-20: Improve CKEditor toolbar configuration accessibility, nor the next one. It's also not in the wysiwyg_ckeditor git repo. AFAIK it never existed.
If you're referring to the /* RTL */ annotation, that was changed to /* LTR */ as per http://drupal.org/node/132442#language-rtl.
So I just went ahead and created ckeditor.admin-rtl.css, based on the /* LTR */ annotations in ckeditor.admin.css.

Status: Needs review » Needs work
Issue tags: -Usability, -wysiwyg, -accessibility, -Spark, -CKEditor in core

The last submitted patch, ckeditor_module-1890502-114.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +wysiwyg, +accessibility, +Spark, +CKEditor in core

#114: ckeditor_module-1890502-114.patch queued for re-testing.

quicksketch’s picture

Status: Needs review » Needs work

So I just went ahead and created ckeditor.admin-rtl.css, based on the /* LTR */ annotations in ckeditor.admin.css.

Thanks, maybe what happened is my original patch lacked it to begin with. At this point we would have needed to recreate it anyway.

I'm surprised at the conclusions you reached in #114 @WimLeers. Regardless of the "presentation vs content" debate, users have a very strong expectation of alignment, indentation, and underline functionality. All of the following services provide all of these features:

  • Wordpress (all buttons)
  • Joomla (all buttons)
  • Google Sites and Google Docs (all buttons)
  • Hackpad (all buttons)
  • Squarespace (no underline or indent, just alignment)
  • Weebly (no indent)

I couldn't even think of a service that *doesn't* have alignment. Almost all had Underline, and a few lacked indentation. Maybe we want to do our own thing and enforce strict content creation that is only content and has no semblance of layout, but there are many users who are going to be turned off by Drupal's intentional efforts to hinder them.

I strongly believe that any battle about content vs. layout should be moved to a per-site discussion. If you're running a blog, you probably don't care about content vs. layout. If you're running a newspaper, it's probably very important. Even if you're anywhere on that spectrum, a good theme will be able to adjust the applied CSS through media queries as it makes sense to do so, for example making image or caption align-left and align-right classes become full-width at a certain viewport size.

On image alignment, I feel that is equally important as text alignment. Doubly-so for image + caption alignment, considering that's the thing that is the biggest pain to accomplish but the most frequently desired.

Wim Leers’s picture

Status: Needs work » Needs review

I strongly believe that any battle about content vs. layout should be moved to a per-site discussion.

Agreed. That's why I created #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default).

Overall, I agree that it's a very, very thorny issue. That's why I created #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default), so that we don't get stuck in a very long bikeshed. Clearly, you and sun have diametrically opposed viewpoints. I'm somewhere in between, though with the future of the web in mind (more form factors & content reuse), I'm definitely more on sun's side.
(I honestly hadn't questioned these buttons precisely because they're so common. I've been so focused on the innards that I forgot to think about the outside.)

Ironically, what you are arguing for is that the proclaimed necessity for alignment, underlining and indentation is — AFAICT — opposed to what your colleague eaton argued against :) See #1824500-117: In-place editing for Fields and http://www.lullabot.com/articles/inline-editing-and-cost-leaky-abstractions.


  • Google Sites, Google Docs, Hackpad, Squarespace and Weebly are all page-centric. They're about building pages of content. Hence, it makes sense for them, since this is pretty much the only layout tool you get there!
    Furthermore, they either don't allow for content reuse at all, or at the very least not to the same "structured content" degree that Drupal can.
    So, frankly, I consider these invalid comparisons.
  • Joomla and Wordpress: true. But both of them allow for crappy mark-up (style attributes; everything goes). By the same line of reasoning, we should also allow for font colors to be set (like Wordpress). Wordpress is specifically targeted at bloggers; that's not the target audience for Drupal..

It's not because Drupal is following the lead of other CMSes by finally also adopting a WYSIWYG editor, that we should do exactly what they do. Drupal should encourage structured content as much as possible. Good, reusable HTML content is part of that.

If necessary, I'd much rather add a "Help" button that educates the user about why the buttons that users expect but are missing, than including them by default.


@quicksketch:
1. I don't think you're really arguing for <p style="margin-left: 80px;">This type of indentation</p>, right?
2. If you mark this as NW, can you please call out what you think exactly needs work? Thanks :) In the mean time, I think we can use more reviews here.
Wim Leers’s picture

Note that the patch in #114 can easily be tested by going to http://simplytest.me/project/spark/8.x-1.x, clicking "Launch sandbox" and then going to admin/config/content/formats/filtered_html (for the toolbar builder) and node/2/edit (for seeing CKEditor in action).

quicksketch’s picture

1. I don't think you're really arguing for <p style="margin-left: 80px;">This type of indentation</p>, right?

No, of course not. I already went to quite a bit of effort to remove all inline styles from the existing implementation prior to the plugin-gutting. Now without the drupalimage plugin, we're back to float left/right. The existing indentation code still uses classes, as does alignment and underline.

Drupal is meant for a lot of different kinds of sites. If we don't want to include alignment buttons by default out-of-box, I'm fine with that (though I still think it would be a big mistake).

Drupal should encourage structured content as much as possible. Good, reusable HTML content is part of that.

If necessary, I'd much rather add a "Help" button that educates the user about why the buttons that users expect but are missing, than including them by default.

There should be no need to have a help button to explain why a feature is missing. If there's not a substitute or an obvious way to accomplish the task without the buttons, the buttons shouldn't be removed. In the current form, this "WYSIWYG" is little more than a "see your own bold tags". I hate to say it, but if this is where we're heading then we should abandon WYSIWYG and use markdown, THAT would "encourage" a clean content paradigm. That said, I DON'T think this is where we should be heading. Users want EASY and Drupal should make that at least an option. As a USER of Drupal, I want these buttons (and classes) to be present. If there's no ability to format images of any kind, there's no point in inserting image. If there's no point in inserting an image, there's no point in WYSIWYG.

effulgentsia’s picture

I agree with quicksketch and aspilicious that not allowing left/center/right/justify buttons without installing a contrib module is totally lame. I don't see what the harm is in Drupal core mapping these buttons to align-left, align-right, align-center, and align-justify classes, and adding a couple lines of CSS for each of these in system.base.css. However, sounds like maybe with mobile devices, etc., it's not quite that simple, in which case, I'm also fine punting on it to #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default).

Are there any other blockers for this patch? If not, I'm tempted to RTBC it again.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all agree that alignment buttons should be part of the default editor core provides (I think so too). It also seems that it's not necessarily trivial. Why not commit this as is and continue in #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default) which is already set as a major as well. The other option is to quickly slap the alignment classes on things in this patch and fine-tune in the other one.

I'd suggest commit this as is and figure things out in #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default). I feel weird setting this RTBC when I haven't been involved in this too much, but this seems more of a process issue than a feature discussion. So, for the sake of progress: RTBC :-)

quicksketch’s picture

Works for me. We know that one way or another this has to get in, and that it's going to require work either way. For serious though, we can't possibly build a legitimate Drupal-WYSIWYG integration without some kind of class manipulation. I feel bad that now almost all the actual integration with the CKEditor library has been gutted from this patch. The integration is a shell of what it was in the original patch (though the admin configuration has gotten quite a bit better). Let's get this in so we can do some real integration, now that every possible problem has been punted.

Owen Barton’s picture

Can someone clarify precisely what the issue is with alignment and mobile?

I understand the conceptual desire to avoid layout oriented markup in content, although I agree with others that in the case of align most users and site builders probably expect it. With Indent measured in px I understand the issue with mobile (although that seems fairly easily solved). I don't get the issue with alignment and mobile, however.

webchick’s picture

Hm. I'm not planning to mark this down from RTBC, but I agree 300% with quicksketch, yoroy, and others. This is a user-facing feature. The onus is therefore on developing a solution that matches user expectations; e.g. How WYSIWYG editors work in every other application and in every other website that users have interacted with. If we do not ship with this, as the default configuration, then IMO we have eliminated at least half of the benefits of including a pre-configured WYSIWYG editor out of the box.

Now, for the site builder, we offer a number of flexible options if they're developing the kind of sites that need strict content/theme separation: configurable buttons, easily overridable CSS, and even the option to shut it off altogether do the site has no WYSIWYG editor. Let's not punish both site builders eho need WYSIWYG, and their users, by making using WYSIWYG a pain in the ass on Drupal: the entire point of this and other authoring experience improvements is to help those who can't help themselves, and to help claw back years of list ground on Drupal's UX.

webchick’s picture

Sorry for all the typos; i'm on my phone in Sydney and surrounded by sharks. ;)

tkoleary’s picture

Add ing to what webchick said, I think it's important to give the broadest possible set of options to users or we hamper ourselves compared to other systems. IMHO this is really a default configuration issue:

  • Large scale publishers will want to tightly couple content authors to the theme
  • Mom and pop site owners will want to do what they want to do

Our job is to mak it a one click operation to do this "Use theme CSS"/"Allow user CSS" or something along those lines.

sun’s picture

  1. The discussion on these buttons and their behavior has much larger consequences, which have not been addressed. Namely:

    • History. Updates. Upgrades. — There is absolutely no precedent for these styles in core. If they would have been introduced in Drupal 4.7.5, we would still have to have them in D8 core today, exactly in the way they would've been defined back in Drupal 4.7.5, because they affect all user content, on hundreds of thousands of sites.
    • More targeted/qualified theme CSS styles — which will override these styles, no matter what. And due to how CSS works, this cannot be reversed.
    • Generic CSS class name clashes. — There's no sign of a research, with regard to whether the chosen CSS class names are following best/common practices and whether they might clash (or druplicate?) with generic class names being used elsewhere.
  2. We're exaggerating on a subset of functionality that isn't used in the first place.

    Yes, these buttons are exposed and cluttering the UI everywhere, but where is the proof that anyone is actually using them?

    I've built many sites ranging from very very small scale to very large scale, and all with "WYSIWYG" editor support, but there has never been a single user that asked for that formatting functionality. Even in the other applications that have been mentioned, I've yet to see a published document or post that would actually use them (in a proper way that doesn't look completely distorted, so let's please skip over the jokes).

    The most sense that one is able to make of the alignment features are table cells — but alas, alignment happens on a different layer there. RTL is also achieved slightly differently. Every other intended usage circles back into layout & design, which is guaranteed to break, and thus NOT WYSIWYG.

    Likewise, the indentation features are completely off, too. Indentation applies to lists, and only to lists, in order to create sub-lists, and that's it. Indenting random HTML elements circles back into layout & design, which is guaranteed to break, and thus NOT WYSIWYG.

    To achieve a true "What You See Is What You Get" experience (as much as that is still possible today), you need to stick to the rules of semantically marked up + classified content, which is translated and styled into a presentation that works for a particular screen, device, and resulting layout.

    Was there, perhaps, a very good reason for dropping all of these attributes & elements from HTML5?

  3. With regard to the left/right-floating and centered/full-width containers (for images/captions/whatever-embedded), we will have to have a good and intense discussion, in order to properly address the consequences mentioned above. Classes are definitely the way to go there, but we'll equally run into the big media/screen and not-so-WYSIWYG-as-we-thought problem space.

    And of course, also the much larger problem space of CSS that is bound to an individual plugin of a text editor, but the CSS of the plugin needs to be always loaded on all pages and retained forever — regardless of whether the editor is loaded (or even enabled), and even if the particular plugin gets disabled.

  4. I still did not have time to look at the actual PHP code and architecture that's being introduced here. For now, I mainly trust @effulgentsia's sign-off. However, in my brief look at the code, a few class/interface names could be improved, but we can do that in a follow-up.

    Also, I still have the impression that pure extension plugins are not really handled yet. At least, I did not see a code path or option in the text editor/toolbar settings/UI that would allow to enable/disable/configure extensions (not buttons). E.g., a very common case for that is the Paste From Word plugin/extension, which actually supports a global setting to "take over" and process all paste operations that may happen — so instead of providing a separate button for pasting from word, you normally enable the extension with that global setting being enabled, and thus, it doesn't even matter whether you have any "Paste" button(s) in the toolbar or not - every paste is processed by the plugin, even if you press CTRL+V on your keyboard. — I want to stress on the fact that this is just one of many examples. There are actually many more extensions (with and without own buttons) that provide very useful, button-less API/interaction functionality.

    In short, the general architectural design should follow a very simple and vertical Extension » Button(s) hierarchy, whereas the built-in/internal buttons may be provided by an "internal" extension/plugin. I've seen references to "internal" in my brief look at the code, but unless I'm very mistaken, all of them were related to buttons only.

    EDIT: In case this particular aspect and example of the Paste From Word plugin no longer applies to CKEditor 4+, then replace it with any other extension, please. The focus is on the architectural design, not the example.

Gábor Hojtsy’s picture

@sun: if we care so much for upgrades, class name clashes and theme based styling accidentally overriding the default styles (and don't care so much for themes being able to override them at all), then we can just as well add inline styles with the elements and be done with that. We need to figure out a tradeoff and this is such basic functionality, that it would not be no support for the most basic things that people want. Either we go with classes and then need to support that (and allow for themes to override things) or we go with inline styles, and have less of an upgrade burden butor provide more flexibility and cleaner markup.

Reality is there is this world of ideal markup and styling separation but most people using WYSIWYG editors will not adhere to that as strict as you want, because content modelling ideals matter less than getting things done in most businesses. Drupal is not only built for academics building backend data stores for semantic web tools.

Wim Leers’s picture

Also, I still have the impression that pure extension plugins are not really handled yet. At least, I did not see a code path or option in the text editor/toolbar settings/UI that would allow to enable/disable/configure extensions (not buttons).

It is supported. To do precisely what you described, implement CKEditorPluginInterface CKEditorPluginContextualInterface and CKEditorPluginConfigurableInterface.

I've even explicitly documented this:

+/**
+ * Defines an interface for contextually enabled CKEditor plugins.
+ *
+ * Contextually enabled CKEditor plugins can be enabled via an explicit setting,
+ * or enable themselves based on the configuration of another setting, such as
+ * enabling based on a particular button being present in the toolbar.
+ *
+ * If a contextually enabled CKEditor plugin must also be configurable (e.g. in
+ * the case where it must be enabled based on an explicit setting), then one
+ * must also implement the CKEditorPluginConfigurableInterface interface.
+ *
+ * @see CKEditorPluginConfigurableInterface
+ */
+interface CKEditorPluginContextualInterface extends CKEditorPluginInterface {

See LlamaContextual.php for an example.

effulgentsia’s picture

I think #128 makes some good points in 1, 2, and 3. Paraphrasing/summarizing/adding:

  • Many people here, including me, have been claiming that CMS users commonly want/use left/right/center/justify buttons for aligning free-floating text (not table cells, images, or image captions, which we need to handle separately anyway). But, how do we know this to be true? Perhaps more research is in order, and one good reason to punt on it to #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default), so that the research can be discussed there.
  • If we can show that in fact it is a common need, then we need to make sure that the markup we add to achieve it is something we expect to be able to support for a very long time, not just 1 or 2 major versions, because our general Drupal core policy is that we always support content upgrades. Without wysiwyg in core, we've never (AFAIK) had to confront this issue before of Drupal core making decisions of what to add to people's content, and then needing to support that forever. Contrib modules have been doing this though, and dealing (or not dealing) with the consequences, so it would also be nice to gather research on what these contrib modules have learned in the process. Another reason to punt to #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default).
  • Image floating, sizing, and caption alignment is a different problem-space than the above. Unlike the above, I don't think anyone is disputing that this is a very common need. But it shares the same issue that we need to choose carefully the markup we add to people's content to support it. At one time, I thought there was discussion to use data- attributes for it and have a text filter convert it to the needed markup on output only. That has the benefit of allowing us to change the output markup whenever new best practices emerge. Perhaps the same approach could be used for text alignment? I don't know if we want to tackle the image/caption discussion/implementation in #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default) or a separate follow up.

So, given these reasons to handle these issues in follow ups, I think we're still RTBC here on this initial patch. Even though much has been stripped from it, getting it in will allow us to stop carrying 100K of stuff around in rerolls, and allows us to work out all these follow ups in targeted issues with small, easy to review patches.

I still did not have time to look at the actual PHP code and architecture that's being introduced here. For now, I mainly trust @effulgentsia's sign-off. However, in my brief look at the code, a few class/interface names could be improved, but we can do that in a follow-up.

I'm certainly not claiming that all the code is bug free and perfect. But I think whatever problems are discovered can be addressed as they come up. I do think the general architecture, code cleanliness, and docs are a very solid foundation on which to do further polishing as needed.

Bojhan’s picture

This sounds like a solid strategy, I agree with @sun that we should continue the discussion about layout related buttons. Although many say its expected, we should question how often its used in the Drupal context and whether it is future-proof (mobile), my first inclination was too to remove this as layout control is quite tricky.

yoroy’s picture

Assigned: Wim Leers » Dries

Asigning to the Dries.

quicksketch’s picture

Many people here, including me, have been claiming that CMS users commonly want/use left/right/center/justify buttons for aligning free-floating text (not table cells, images, or image captions, which we need to handle separately anyway). But, how do we know this to be true?

I'd like to note that the alignment buttons serve multiple purposes in that they can (and should) align all of the mentioned items: text, table cells, images, and image captions. I'm really more concerned about images and captions, but no matter what is being aligned, we ultimately will commit to a class name to do the work of alignment on these things.

I don't see needing to support these classes long-term to be a problem. If we would have added these classes in Drupal 4.7, yes we would still have them today, and at the same time, that means that we'd have 6 years where users could format their content the way they wanted in a consistent manner. Now what we've got is hundreds of thousands of sites that all have different markup for handling inline images individually. It's our lack of ability to commit to any particular markup styling that has ensured either a poor editorial experience or the use of Full HTML formatting, since that's the only way the WYSIWYGs we have today work with alignment.

Regarding data- attributes vs. class names, I think using data- attributes for captioning may be a good approach, though it has drawbacks (handling nested HTML tags inside of a data- attribute gets pretty nasty), but may be the right decision because of the changing HTML markup around figures/images. Even if using data-attributes though, this will end up producing a class on output via filtering that will need to be placed somewhere. In the cases where our markup is nothing but an added class, using the class directly on the tag makes a lot of sense. i.e. <p class="align-right"> instead of <p data-align="right">. Using a data attribute on these simple situations just means we'll have to add more filtering that does the conversion to a class later.

Gábor Hojtsy’s picture

All right. I personally use alignment all the time in both my d.o posts (for text, images and inline boxes) and my blog posts for example, but I might be a minority. I'm fine punting that for now as long as we commit to trying to figure it out elsewhere. (in #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default)).

yannisc’s picture

Gmail email composer and WordPress both have alignment options enabled as default. I attach screenshots.

I personally don't use them often, but I think it's an essential feature for editors that are comfortable with html.

yoroy’s picture

quicksketch’s picture

At this point I'd much rather commit what we have and do the alignment issue separately just so we can get past this bikeshed. I think with a dedicated issue we'll easily be able to unleash the hordes of end-users to drive home the necessity of alignment. Then we can work on other postponed issues:

#1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms
#1873500: CKEditor + Edit
#1872226: Ability to configure CKEditor’s pasting to match the currently active text format
#1894644: Unidirectional editor configuration -> filter settings syncing

And others in the CKEditor in core tag queue.

Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Fixed

I read up on all the discussion between quicksketch, sun, and others and agree that we should move forward with this patch, and talk more about the alignment/layout issues in a separate issue. It also agree that it is an important topic.

I've personally only used these alignment features in tables and for image captions. That are valid use cases, meaning we can't remove the functionality. That aside, if we removed these features, we're simply ignoring the problem instead of fixing it. Our users will use a mix of different, inconsistent solutions in contrib, making it worse. So like quicksketch, I like the idea of core committing to a particular markup/styling. In fact, once we picked a solution for core, I'm going to go back and update the content on my sites (e.g. buytaert.net) to make it nice and consistent. It actually makes me feel better. :)

I decided to go ahead and commit this patch to 8.x. For the first time in history, Drupal will ship with WYSIWYG! This patch has been in the works for *months*, so thanks everyone for the hard work and rigor. We're obviously not done yet so onto the next WYSIWYG related patches! Thanks. :-)

cweagans’s picture

Title: WYSIWYG: Add CKEditor module to core » Change notification for: WYSIWYG: Add CKEditor module to core
Priority: Major » Critical
Status: Fixed » Active

This needs a change notice, and I think it probably deserves a CHANGELOG.txt entry too :)

Wim Leers’s picture

Title: Change notification for: WYSIWYG: Add CKEditor module to core » WYSIWYG: Add CKEditor module to core
Status: Active » Needs review
FileSize
711 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,437 pass(es), 2 fail(s), and 3 exception(s). View

Follow-up issues that have been ready for a while and can be moved to RTBC state very quickly:


Patch attached that adds a CHANGELOG.txt entry. I don't know what's appropriate there or not, but it seemed sensible to put this relatively high in the changelog, since it's user-facing, and the farther down the list you go, the more technical it seems to get.

Change notice: http://drupal.org/node/1911646.

Related: change notice for editor.module: http://drupal.org/node/1911614.

Gábor Hojtsy’s picture

Category: feature » task
Priority: Critical » Minor
Status: Needs review » Reviewed & tested by the community

The changelog looks good. The change notice seems appropriately placed. Recategorizing as minor task for the changelog entry.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1890502-changelog.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
Gábor Hojtsy’s picture

Right, it is impossible for #141 to fail so whatever the testbot says, it should be committed :)

Wim Leers’s picture

And finally, a very, very important issue: for enabling CKEditor by default in the Standard install profile. Hint: it's not as simple as it seems.

Enjoy: #1911884: Enable CKEditor in the Standard install profile.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I would've used the word 'awesome' a few thousand more times in there, but this'll do. :D

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Component: editor.module » ckeditor.module
effulgentsia’s picture

Category: task » feature
Priority: Minor » Critical

Back to original category and priority for d.o. statistics.

Status: Fixed » Closed (fixed)

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

hass’s picture

I feel like we need an upgrade path from D7 and created #1927424: WYSIWYG: CKEditor upgrade path and namespace collision with contrib?.

hass’s picture

Issue summary: View changes

Reflect that all remaining tasks have been performed.

Elijah Lynn’s picture

I just opened a new issue at #2492339: How to debug CKEditor in core? for how to get the unminified source into pages for Drupal 8.

Elijah Lynn’s picture