Make WYSIWYG editors available for in-place editing

Note that on D7 this approach and js works well. It's nice to work with. Will do a more proper review if I get some time this week.

I posted or read these comments in other issues, so I'll repost them here (mostly around terminology):

- The name "drupalwysiwygwidget.js" seems exceedingly odd for Druapal file name. We never smash everything all together like this. Is this some precedent set by Create.js?

- I've said this in other places, but "attachTrueWywisyg" and "supports_true_wysiwyg" terminology I still find poorly named. Pretty much every time you say it, then it's immediately followed by an explanation of supporting in-place editing. In the code itself, always puts the phrase in quotes, "true WYSIWYG". So if we always explain it or it needs quotes around it, why don't we just use the explanation directly. Although "in-place editing is more accurate, "inline editing" is a common phrase, used by CKEditor and Aloha (and other projects). And it avoids awkward camel casing (inPlaceEditing, inplaceEditing?) Could we name these "attachInlineEditor" and "supports_inline_editor"? (The naming of things in a broader sense is also being discussed in #1874640: Rename edit module to quickedit).

+++ b/core/modules/editor/editor.module
@@ -78,6 +78,20 @@ function editor_library_info() {
+  $libraries['edit.editor.editor'] = array(
...
+      $path . '/js/createjs/drupalwysiwygwidget.js' => array(

Any chance we can get back to sane filenames that are delimited by dots or so, and even more importantly, within the module namespace? :)

I hope it's not a CreateJS limitation or something?

The filename should start with "editor." (the namespace of the module that supplies it), and followed by whatever is in there; multiple terms can be separated by dots or hyphens.

2) I also don't quite get the library name... ;)

+++ b/core/modules/editor/js/createjs/drupalwysiwygwidget.js
@@ -0,0 +1,158 @@
+    /**
+     * Removes validation errors' markup changes, if any.
+     *
+     * Note: this only needs to happen for type=direct, because for type=direct,
+     * the property DOM element itself is modified; this is not the case for
+     * type=form.
+     */
+    _removeValidationErrors: function() {
+      this.element
+        .removeClass('edit-validation-error')
+        .next('.edit-validation-errors').remove();
+    },

The following applies not only to this code snippet, but in general to the code:

1) The "Note:" prefix is superfluous in almost all of the doc block descriptions.

2) The description refers to a "type", but there is no type involved in this function anywhere. Thus, anyone who's looking at this is left baffled. :)

3) In general, the code appears to be documented well, but when actually reading the docs, one has to realize that single aspects are just documented in a "wordy" way, and some aspects are about very detailed technical issues that are lacking context for someone else to make sense of. ;)

I wanted to provide a replacement example for this particular one, but I'm actually not familiar enough with the code base and APIs to know what this was trying to communicate for realz.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+ * Definition of \Drupal\editor\Plugin\edit\editor\Editor.

1) Should be "Contains".

2) The separation between edit and editor is a bit weird... Did we already consider to merge the two modules?

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+ *   alternativeTo = {"direct"},

The alternativeTo declaration looks a bit odd to me. It sounds like it's trying to categorize Edit\editor plugins in a way that they're exclusive to each other when appearing in configuration. However, that kind of business logic shouldn't be part of the meta data.

Can we change the property into just "category" or "categories"?

Plural is better, if multiple values need to be supported — although I'm not sure I understand how an edit\editor plugin can apply to multiple in-place editing types.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+      $format_id = $items[0]['format'];
+      if (isset($format_id) && $editor = editor_load($format_id)) {

This looks odd — normally you'd check isset() before accessing an array key, not after.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+      if ($editor && isset($definitions[$editor->editor]) && isset($definitions[$editor->editor]['supports_true_wysiwyg']) && $definitions[$editor->editor]['supports_true_wysiwyg'] === TRUE) {

The "supports_true_wysiwyg" property name appears like a magic flag and doesn't really make sense to me. Can we choose a better and more concise name?

Given this issue title, which contains "true WYSIWYG", "inline editing", and "in-place editing", I've the impression we still didn't find the right terminology...

Btw, since editors can support and not support various things, it would probably make sense to replace that single string property into a "supports" array/hashmap definition; e.g.,:

supports:
  wysiwyg: true

[oh why oh why can't we use YAML in annotations... :P]

+++ b/core/modules/editor/tests/modules/lib/Drupal/editor_test/Plugin/editor/editor/UnicornEditor.php
@@ -18,7 +18,8 @@
+ *   supports_true_wysiwyg = TRUE

Unless I'm mistaken, TRUE must be lowercase in annotations.

#8: editor_true_wysiwyg-1886566-8.patch queued for re-testing.

There are going to be accessibility implications here. Really looking forward to it though as it's going to be a game changer.

Ok, that's a good distinction. Dropping the tag.

#15: editor_true_wysiwyg-1886566-15.patch queued for re-testing.

It makes sense that instead of forcing each module that provides a text editor to implement an in-place editor (\Drupal\edit\Plugin\EditorInterface) plugin, they can simply implement two optional methods, set a flag in their text editor (\Drupal\editor\Plugin\EditorInterface) plugin and be done with it.

I totally agree with this. But I don't think it makes sense for this JavaScript code and the new Editor plugin to live in editor.module. Instead, I think it should live in edit.module, which already has JS editing widgets and PHP plugins for the 'form' and 'direct' editors, so this would be a natural addition of a 3rd editor/widget there.

This suggestion will result in an unfortunately named plugin class of Drupal\edit\Plugin\edit\editor\EditorEditor, but I suggest that we accept that as a temporary measure while we figure out how to adjust our terminology. If I understand it right, the issue is that Create.js uses the terms 'editor' and 'widget' to mean certain things, and meanwhile, Drupal uses both of these terms to mean something else, so when we want to say "a Drupal text editor acting as a Create.js editing widget", we end up with EditorEditor until we resolve the overlap.

Why should edit.module ship code that logically belongs in editor.module

Why do you consider integration between edit.module and editor.module to logically belong in editor.module rather than edit.module? edit.module already has a Direct editor, so it knows about and integrates with Drupal's filter system. editor.module is Drupal core's official way for enhancing Drupal's filter system with a client-side editor. Why shouldn't edit.module make direct use of this? Meanwhile, editor.module currently has no knowledge of field.module, but #15 would add it.

which would introduce a implicit circular dependency

Where would there be a circular dependency?

The Direct editor == contentEditable, and it can only be used when a piece of text does *not* use the filter system!

In that case, would it make sense to move everything related to edit.module's filter system knowledge to editor.module? I'm thinking things like FieldRenderedWithoutTransformationFiltersCommand, the edit_text route and controller, and the Drupal.edit.util.loadRerenderedProcessedText() function. If yes, then that would make sense to me. Otherwise, I'm not clear on the current split between edit.module and editor.module responsibilities in regards to the filter system.

(sorry for being a pain on this)

Per #1873500-22: CKEditor + Edit, yes I do think that issue should be merged into this one. I think they make more sense together than separate, and aren't too big a review burden when combined.

+++ b/core/modules/editor/editor.module
@@ -78,11 +78,48 @@ function editor_library_info() {
+  // Create.js PropertyEditor widget library names begin with "edit.editor".
+  $libraries['edit.editor.wysiwyg'] = array(

Tohrröööö!

Hey, according to this inline comment, "wysiwyg" is going to be our contrib namespace, right? :)

+++ b/core/modules/editor/editor.module
@@ -78,11 +78,48 @@ function editor_library_info() {
 /**
+ * Implements hook_custom_theme().
+ *
+ * @todo Add an event subscriber to the Ajax system to automatically set the
+ *   base page theme for all Ajax requests, and then remove this one off.
+ */
+function editor_custom_theme() {
+  if (substr(current_path(), 0, 7) === 'editor/') {
+    return ajax_base_page_theme();
+  }
+}

If I get this right, then this force-resets the theme back to the front-end theme to prevent the admin theme from kicking in?

In-place editing with a WYSIWYG editor definitely needs to happen in the front-end theme.

So I suspect that this is what happens here? (A comment would be useful.)

That said, the entire front/admin theme drama became so utterly complex in the meantime that I won't stop to call for its complete removal without replacement. Was an inane idea in the first place.

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+ * Depends on Editor.module. Works with any (WYSIWYG) editor that implements the
+ * attachInlineEditor(), detach() and onChange() methods.

Outdated.

Could also use some clarification with regard to on which JS "class" the stated methods are found.

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+      return { padding: true, unifiedToolbar: true, fullWidthToolbar: true };

Can we transform these into multi-line syntax, plz?

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+     * @todo: POSTPONED_ON(Create.js, https://github.com/bergie/create/issues/142)
+     * Get rid of this once that issue is solved.

Weird syntax.

@todo Remove after removal of...
@see http:...

^^

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+      var that = this;

In all cases like in this code, "self" is much more appropriate than "that".

"that" is commonly used when referring to parent objects of nested closures. And, "that" is most often a poor programming pattern to being with; the variable name can be self-descriptive instead. In short, "that" is an artifact of poor coding style and programming habits; it can always be replaced with something better.

"self" is appropriate when referring to class-alike JS objects. +/- in the same way you'd refer to self:: in PHP, which always means the current class.

self is the case here.

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+     * Makes this PropertyEditor widget react to state changes.

"Reacts to state changes."

That said, where are the parameters from and to documented?

If there is an "interface" for this handler, then we should at least clarify so. Either by @see, or by using a terminology similar to "Implements Foo.Bar.Baz."

Looking further through this code, it looks and feels weird that both variables are containing strings... (instead of events) Is that something under our control?

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+        case 'inactive':
+          break;
+        case 'candidate':
...
+          break;
+        case 'highlighted':
+          break;
+        case 'activating':

Like PHP, JS supports to "fall through" from one case to another case in switch control structures.

There should be a blank line between the effective switch cases, so as to quickly and inherently make clear, which case is falling through to another case.

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+            if (from !== 'highlighted') {
+              this.element.attr('contentEditable', 'false');
+              this.textEditor.detach(this.element.get(0), this.textFormat);
+            }

What is happening here, and why? (could use a leading inline comment upfront)

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+        case 'active':
+          this.element.attr('contentEditable', 'true');
+          this.textEditor.attachInlineEditor(

Why do we need to set the contentEditable attribute manually...?

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+    /**
+     * Removes validation errors' markup changes, if any.
+     *
+     * @todo: this should not be necessary, will be obviated by edit.module.
+     */
+    _removeValidationErrors: function () {
+      this.element
+        .removeClass('edit-validation-error')
+        .next('.edit-validation-errors').remove();
+    },
...
+    /**
+     * Cleans up after the widget has been saved.
+     *
+     * @todo: this should not be necessary, will be obviated by edit.module.
+     */
+    _cleanUp: function () {
+      Drupal.edit.util.form.unajaxifySaving(jQuery('#edit_backstage form .edit-form-submit'));
+      jQuery('#edit_backstage form').remove();
+    },

Obsolete?

Alternatively, where is the core follow-up issue for that? Both @todos should be replaced by:

@todo D8: Remove this.
@see http:...

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+     * Loads rerendered processed text for a given property.
...
+    _loadRerenderedProcessedText: function (options) {

One of these two words is too much: "rerendered" or "processed".

Something that is rerendered is processed already. Something that is processed may not be rendered yet.

But both together are a needless stack of adjectives.

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+     * Leverages Drupal.ajax' ability to have scoped (per-instance) command
+     * implementations to be able to call a callback.

I can only remotely imagine what this comment tries to hint at, as I have a relatively good understanding of the APIs and facilities we have in core. 99% of other developers will not be able to make sense of this. Can we clarify what is actually leveraged, and how exactly, and what kind of consequences that has vs. not doing so? :)

+++ b/core/modules/editor/js/editor.createjs.js
@@ -0,0 +1,187 @@
+     * @param options
+     *   An object with the following keys:
+     *    - $editorElement (required): the PredicateEditor DOM element.
+     *    - propertyID (required): the property ID that uniquely identifies the
+     *      property for which this form will be loaded.
+     *    - callback (required: A callback function that will receive the
+     *      rerendered processed text.

If all of these are required, why aren't they separate arguments instead?

+++ b/core/modules/editor/lib/Drupal/editor/Ajax/FieldRenderedWithoutTransformationFiltersCommand.php
@@ -22,7 +23,7 @@ class FieldRenderedWithoutTransformationFiltersCommand extends BaseCommand {
+    parent::__construct('editorFieldRenderedWithoutTransformationFilters', $data);

I seriously hope that I will never have to specific this Ajax command class name anywhere... yes? :)

+++ b/core/modules/editor/lib/Drupal/editor/EditorController.php
@@ -0,0 +1,47 @@
+   * @param string $view_mode
+   *   The view mode the processed text field should be rerendered in.
+   * @return \Drupal\Core\Ajax\AjaxResponse

There should be a blank line between @param and @return directives.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+  function isCompatible(FieldInstance $instance, array $items) {

This validation method does not check whether $instance is a text field.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+    // This editor is incompatible with multivalued fields.
+    if ($field['cardinality'] != 1) {
+      return FALSE;
+    }

I wonder why we still have this limitation...?

Is there an issue to remove this?

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+  protected function textFormatHasTransformationFilters($format_id) {
+    return (bool) count(array_intersect(array(FILTER_TYPE_TRANSFORM_REVERSIBLE, FILTER_TYPE_TRANSFORM_IRREVERSIBLE), filter_get_filter_types_by_format($format_id)));
+  }

Do we have an issue to get rid of this procedural function?

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+  public function getAttachments() {
...
+    // Get the attachments for all text editors that the user might use.
+    $attachments = $manager->getAttachments($formats);

Hm. I did not really expect this on the edit\Editor plugin. It looks more like an operation the plugin.manager.editor should be responsible for. Apparently, that's partially the case already, but only partially.

+++ b/core/modules/editor/lib/Drupal/editor/Plugin/edit/editor/Editor.php
@@ -0,0 +1,100 @@
+    // Filter the current user's text to those that support inline editing.

s/text/formats/

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php
@@ -0,0 +1,200 @@
+    $this->createFieldWithInstance(

Just FYI: The more we introduce of this, the more we'll have to convert/migrate later on. This all-in-one multi-purpose helper wasn't a particularly good idea ;)

@see #1902098: Introduce DUTB helper methods for working with rendered content + FieldPluginUnitTestBase

Patch in #31 applies but results in the following error when viewing a simple article node as admin (fresh D8 install from 2013-03-08, created article node 1, applied patch #31, enabled modules CKEditor, edit and Text Editor, configured CKEditor to be used for text formats):

Error

The website has encountered an error. Please try again later.
Error message
Doctrine\Common\Annotations\AnnotationException: [Semantical Error] The annotation "@Drupal\Core\Annotation\Plugin" in class Drupal\editor\Plugin\edit\editor\Editor does not exist, or could not be auto-loaded. in Doctrine\Common\Annotations\AnnotationException::semanticalError() (line 52 of /Volumes/daten2/projekte/drupal8/code/www/core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/AnnotationException.php).

Besides i realize that Drupal is getting way more complex to debug:
Can anyone reproduce this error?

+  _loadExternalPlugins: function(format) {
+    var externalPlugins = format.editorSettings.drupalExternalPlugins;
+    // Register and load additional CKEditor plugins as necessary.
+    if (externalPlugins) {
+      for (var pluginName in externalPlugins) {
+        if (externalPlugins.hasOwnProperty(pluginName)) {
+          CKEDITOR.plugins.addExternal(pluginName, externalPlugins[pluginName], '');
+        }
+      }
+      delete format.editorSettings.drupalExternalPlugins;
+    }
   }

It's a good idea to split out this logic since we now need it multiple places, but the fact that this function requires format.editorSettings.drupalExternalPlugins to be set, but then deletes that property after its done is some confusing behavior. Although it's more repetition, moving the deletion of this variable outside the function makes the code significantly easier to follow:

Instead of:

this._loadExternalPlugins(format);

Use:

if (format.editorSettings.drupalExternalPlugins) {
  this._loadExternalPlugins(format.editorSettings.drupalExternalPlugins));
  delete format.editorSettings.drupalExternalPlugins;
}

But then all that's left in that function is a loop... this bit of code is difficult to optimize without making it difficult to understand.

-(function (Drupal, CKEDITOR) {
+(function (Drupal, drupalSettings, CKEDITOR, $) {

This isn't necessarily a complant, but drupalSettings is never used in the ckeditor.js file. This will throw a JSLint warning by default. Is it our policy to *always* scope drupalSettings even if it's not used?

+  // Create.js PropertyEditor widget library names begin with "edit.editor".
+  $libraries['edit.editor.editor'] = array(

Oh dear... talk about a namespace mess. I reviewed our general pattern for naming libraries (i.e. $libraries['jquery.ui.draggable'], $libraries['jquery.effects.shake']) and it seems like this is following the correct pattern: edit = name of library, editor = category within parent library, editor = individual implementation within category. Resulting in "edit.editor.editor".

Edit module named these libraries before "editor.module" was created, so the name-spacing isn't entirely its fault. Given the description texts for each of these libraries, I think we might be able to rename the category to avoid name-spacing confusion. Currently the descriptions for each library edit.editor.form, edit.editor.direct and now edit.editor.editor all use "X Create.js PropertyEditor widget". Based on this, I think it may make sense if we renamed these libraries to use "widgets" instead of "editor", resulting in the library names edit.widgets.form, edit.widgets.direct and edit.widgets.editor.

(Additionally thanks for renaming drupalwysiwygwidget.js to editor.createjs.js, that's much better.)

But, alas, we cannot use 'theme callback' => 'ajax_base_page_theme', because this is one of the few modules ahead of the pack: it's using the routing system exclusively instead of the menu system, hence we're forced to have such a work-around.

Yeesh. After researching the new routing system I have to agree that this work-around is the only available solution we have unless we convert the router entries *back* to hook_menu(). As that seems like it would be moving backwards from current trends, we're stuck with the work-around for now.

+      case 'changed':
+        break;
+
+      case 'saving':
+        break;
+
+      case 'saved':
+        break;
+
+      case 'invalid':
+        break;

This seems wasteful that we're checking cases but not actually doing anything with them, but I'm not opposed to keeping them in here. It's just a little strange. If they seem worth keeping then let's leave as-is.

+ * @Plugin(
+ *   id = "editor",
+ *   jsClassName = "editor",
+ *   alternativeTo = {"direct"},
+ *   module = "editor"
+ * )

jsClassName and alternativeTo don't have anything to do with the discovery of these classes does it? Is this issue related enough to fix these definitions and move the non-discovery information into a dedicated method?

Thanks for the progress on this issue Wim. Overall it's looking great. I really appreciate the change from "supports_true_wysiwyg" to "supports_inline_editing". I'll try to give this a functional review followup soon. This issue has probably seen a disproportionate number of code reviews to end-user reviews.

#39: editor_true_wysiwyg-1886566-39.patch queued for re-testing.

AFAICT, all significant feedback has been addressed. Some code style and naming nits haven't been, but whoever cares enough about those can open follow ups for them.

Would be nice for the issue summary to be updated for posterity: either before or after commit.

Excellent!! So nice to see this RTBC, so we can really show off the new features of D8 together! :)

Committed and pushed to 8.x. Thanks!