Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets) [#1874936]

Problem/Motivation

Currently, the Edit module has a hardcoded list of Create.js PropertyEditor widgets. Other modules should be able to add new Create.js PropertyEditor widgets.

Proposed resolution

Create a new CreateJsPropertyEditor plug-in type. Interface & other details TBD.

Remaining tasks

All.

User interface changes

None.

API changes

New plug-in type.

Comment	File	Size	Author
#18	edit_editors-1874936-18.patch	63.58 KB	Wim Leers
#18
#18	interdiff.txt	3.68 KB	Wim Leers
#15	edit_editors-1874936-15.patch	61.42 KB	Wim Leers
#15
#15	interdiff.txt	19.52 KB	Wim Leers
#12	edit_editors-1874936-12.patch	59.13 KB	frega
#12
#12	interdiff.txt	7.06 KB	frega
#10	edit_editors-1874936-10.patch	59.71 KB	Wim Leers
#10
#10	interdiff.txt	7.69 KB	Wim Leers
#8	edit_editors-1874936-8.patch	59.79 KB	Wim Leers
#8
#8	interdiff.txt	16.33 KB	Wim Leers
#4	edit_editors-1874936-4.patch	59.59 KB	Wim Leers
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 2 January 2013 at 15:46

Assigned:

Unassigned

» Wim Leers

I'm currently working on this, it's necessary for #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration.

Comment #2

henribergius CreditAttribution: henribergius commented 4 January 2013 at 11:15

The right way to go about this is to leverage VIE's type system. Each attribute (field definition) has a range it accepts as values.

With Create.js you can associate a property editor (and editor configuration) to a range. In case of "form editors", the actual editor implementation is loaded from the server, but having the real, per-field ranges available on client side also allows developers to supply and load their own client-side editor widgets.

Here is a simplistic type definition with a custom range for the "title" attribute: https://github.com/bergie/create/blob/master/examples/example-withtype.h...

A custom editor definition: https://github.com/bergie/create/blob/master/examples/example-withtype.h...

Applying the custom editor to the range: https://github.com/bergie/create/blob/master/examples/example-withtype.h...

Comment #3

henribergius CreditAttribution: henribergius commented 4 January 2013 at 11:22

Note: the VIE SparkEditService already communicates range setups to VIE and Create, but this could be handled in a more sophisticated manner by using semantic ranges instead of straight 1:1 relation between range and property editor widget: http://drupalcode.org/project/edit.git/blob/06e86c82ab5412ef1f78aea343db...

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 4 January 2013 at 23:00

Priority:	Normal	» Major
Status:	Active	» Needs review

File	Size
edit_editors-1874936-4.patch	59.59 KB

In this patch, the following has changed:

Decoupling: there now is an "Edit editor" (\Drupal\edit\Plugin\EditorInterface) plug-in type. Edit module itself uses this for its "direct" (plain contentEditable) editor. An editor can:
- determine dynamically (depending on FieldInstance and the values of the field) whether it is compaible or not
- generate per-field metadata dynamically (depending on FieldInstance and the values of the field), this is then available on the client-side as well (e.g. send the field's text format so that the Create.js PropertyEditor widget can act differently depending on the text format)
- provide "dynamic attachments": depending on the current context, provide different attachments (JS/CSS/JS settings/libraries); this is necessary for e.g. loading WYSIWYG editors that the current user may have access to, i.e. necessary for #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration
There are unit tests that cover all these aspects. Create.js' mapping of field -> PropertyEditor widget is now handled/updated automatically (instead of being hardcoded): modules that provide new "Edit editor"s will work automagically (thanks to the metadata in the plug-in annotations).
Decoupling: The Edit module only assumes a single Editor is always available: the form-based one, which works for every field type that exists. That is where the assumptions end.
Decoupling: The Edit module's ProcessedTextEditor plug-in type ('direct-with-wysiwyg') is gone. It was a necessary evil because there was no text editor abstraction whatsoever, but now there will be (at #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration). It's silly to have to text editor abstractions (with Edit's being extremely limited), especially because that would require modules providing a text editor to implement two separate plug-ins. Instead, the Editor module should simply implement an "Edit editor", which would make any text editor (capable of "true WYSIWYG") available as a Create.js PropertyEditor widget. Code for that will be posted at #1833716-93: WYSIWYG: Introduce "Text editors" as part of filter format configuration.
Decoupling: Edit's JS was hardcoded to deal with the 'form', 'direct' and 'direct-with-wysiwyg' editors, and had hardcoded branching for those. This is now gone. Now you can implement a getEditUiIntegration() method in your Create.js PropertyEditor widget; this will provide Edit's JS with the necessary metadata for optional special handling in a non-hardcoded manner.
Note that this does not prevent us from reusing upstream Create.js PropertyEditor widgets; there are sane defaults, and if you want to leverage this, then you could simply subclass the upstream widget and implement this method in the subclass.
Decoupling: There are zero of the edit-type-(form|direct|direct-with-wysiwyg) class names remaining in the CSS.
Consistency: Edit's JS was already converting the original colon-separated edit ID to a slash-separated edit ID (necessary for VIE compatibility).

Comment #5

5 January 2013 at 01:32

Status:

Needs review

» Needs work

The last submitted patch, edit_editors-1874936-4.patch, failed testing.

Comment #6

yched CreditAttribution: yched commented 5 January 2013 at 14:00

This is somewhat above my head, so I can't really vouch for the design/architecture - it does have a nice feeling to it ;-).
Just a couple remarks after a cursory review.

+class EditorManager extends PluginManagerBase {
+
+  protected $defaults = array(
+    'class' => 'Drupal\edit\Plugin\edit\editor\Form',
+    // 'class' => 'Drupal\edit\Plugin\edit\editor\Direct',
+  );

Is this really needed ? If this plugin type is annotations-based, in which cases would a definition not have a 'class' ?

   function testTextWysiwyg() {
+   // Enable edit_test module so that the 'wysiwyg' Create.js PropertyEditor
+   // widget becomes available.
+  $this->enableModules(array('edit_test'), FALSE);
+

Indent error.

Looking at the mostly empty methods in Drupal\edit\Plugin\edit\editor\Form, it seems there would be a case for an EditorBase base class ?
Also, just "Form", "Wysiwyg" as class names are not specialized enough. According to coding standards, a class name should not need the full namespace to be unambiguous - at least FormEditor, WysiwigEditor ? They are 'editor' plugins, right ?
I didn't check the other classes introduced in the patch.

Comment #7

TwoD

Swedish

Sweden

CreditAttribution: TwoD commented 6 January 2013 at 05:32

Quick note: core/modules/edit/lib/Drupal/edit/Plugins/EditorInterFace.php must be renamed EditorInterface.php (lowercase F) on case sensitive systems or it'll complain about that interface not being found.
It just gave me a WSOD before I flushed the cache tables manually. Possibly what prevented the bot from logging in?

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 January 2013 at 11:14

Status:

Needs work

» Needs review

File	Size
interdiff.txt	16.33 KB
edit_editors-1874936-8.patch	59.79 KB

#6:
1. I thought this was how you indicated that a certain plug-in would always be present… I was very, very wrong. Fixed.
2. Fixed.
3. Base class: done.
4. Not specialized enough class names: from a purely logical POV, I think it's pointless to require "sufficiently specialized class names". It's already embedded in such a specialized (and deep!) directory structure that requiring that is pointless. However, from a practical POV (searching code base, and actually using classes), I agree. Fixed.

#7: Thanks — that was definitely not intentional! It's very well possible that that was indeed the cause for testbot failure. If that's the case, you just saved me countless hours of headdesking :D :) Thank you!

Besides the above, I also fixed some file docblocks: s/"Definition of"/"Contains"/.

Comment #9

7 January 2013 at 11:16

Status:

Needs review

» Needs work

The last submitted patch, edit_editors-1874936-8.patch, failed testing.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 January 2013 at 11:26

Status:

Needs work

» Needs review

File	Size
interdiff.txt	7.69 KB
edit_editors-1874936-10.patch	59.71 KB

D'oh — the thing noticed by TwoD I *did* fix, but git doesn't detect filename case changes. So it's not reflected in the patch :/

I noticed that Edit was putting its interface in an atypical location anyway (inside the Drupal/edit/Plugin directory, instead of in just Drupal/edit), so I moved & renamed it. Now it should be good to go.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 7 January 2013 at 12:35

So, #10 proves that it was the file casing problem that TwoD suggested in #7 that caused the patch to fail. Very good to know, of course :)

Comment #12

frega CreditAttribution: frega commented 7 January 2013 at 18:50

File	Size
interdiff.txt	7.06 KB
edit_editors-1874936-12.patch	59.13 KB

Changes on the PHP-side seem good to me, but i primarily focussed on JS.
Changes in the EditService.js seem all good to me, also the cleanup of DOM-classes etc.

I was only able to test the padding-"aspect", because i currently can't get a widget using either "unifiedToolbar" or "fullwidthToolbar" to work w/ D8.

I adjusted the patch with a few small changes/improvements:
- Fixed small typo/semantics "stage" -> "state" (w/ regard to the different *states* of the create.js editables).
- Fixed indentation in Drupal.edit.views.ToolbarView _unpad/_pad.
- Add _needsPadding-method for consistency between Drupal.edit.views.ToolbarView and Drupal.edit.views.PropertyEditorDecorationView
- Fixed a typo in the defaultConfig in _editUi/getEditUiIntegrationAspect to use the keys "padding", "unifiedToolbar", "fullWidthToolbar" instead of "needsPadding" etc.). Question: should the defaultConfig for the EditUI-Integration maybe come via drupalSettings?

I also did one "largish" refactoring regarding the _editUi-methods on Drupal.edit.views.ToolbarView. Whilst it's possible to have two - one on the prototype and another "static" one on the "object" - it strikes me as a little unusual. I replaced it w/a helper-function in util.js: Drupal.edit.util.getEditUiIntegrationAspect. This seems more "explicit" and clearer to me.

Please note: I called it "getEditUiIntegrationAspect" because your patch uses the terms "EditUi", "integration" and "aspects"; I would favor something like getEditUiIntegrationInfo or even getEditUiIntegrationProperty because "aspects" have a lot of other (technical) connotations, too.

Comment #13

effulgentsia CreditAttribution: effulgentsia commented 7 January 2013 at 23:24

Status:

Needs review

» Needs work

I didn't review the CSS or JS, but I agree with #12 that the PHP is in good shape. Here's some minor nits:

+++ b/core/modules/edit/lib/Drupal/edit/EditorBase.php
@@ -0,0 +1,36 @@
+ * This abstract class provides default implementation for the metadata and
+ * dynamic attachment methods, which many editors likely won't need.

Given that both DirectEditor and FormEditor are known examples where editor-specific metadata and dynamic attachments are not needed, we don't need to speculate on what's "likely". I think this doc can be removed entirely, since the methods return empty arrays, which is clearer than earlier incarnations that had no function body at all.

+++ b/core/modules/edit/lib/Drupal/edit/EditorInterface.php
@@ -0,0 +1,60 @@
+   *   A keyed array with metadata. Each key should be prefixed with the plug-in

Here and elsewhere, s/plug-in/plugin/.

+++ b/core/modules/edit/lib/Drupal/edit/EditorSelector.php
@@ -16,152 +17,110 @@
+    $alternatives = &drupal_static(__FUNCTION__, NULL);

Why do we need a drupal_static() here instead of defining $this->alternatives as a protected property?

+++ b/core/modules/edit/lib/Drupal/edit/EditorSelector.php
@@ -16,152 +17,110 @@
+    $editor_id = isset($formatter_info['edit']['editor']) ? $formatter_info['edit']['editor'] : 'form';

Maybe out of scope for this issue, but do we eventually want to change from formatters specifying an 'editor' (and editors specifying 'alternativeTo') to instead have formatters specify a 'range' (per #2)?

+++ b/core/modules/edit/lib/Drupal/edit/EditorSelector.php
@@ -16,152 +17,110 @@
+    return NestedArray::mergeDeepArray($attachments);

Why do we need to merge $attachments deeply like this, instead of returning them as the array they are? Is it an artifact of edit_toolbar() calling array_merge_recursive()? If so, I don't get why it does that either.

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 8 January 2013 at 00:56

Thanks a lot for the reviews, #12 and #13, I'll get back to you tomorrow :)

Comment #15

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 January 2013 at 20:41

Status:

Needs work

» Needs review

File	Size
interdiff.txt	19.52 KB
edit_editors-1874936-15.patch	61.42 KB

Sorry for the delay — I've been doing massive rerolls of #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration (which is more important than this one right now). It's lovely to see how #12 reviews JS, and #13 reviews PHP! Yay :D :)

#12:

I also did one "largish" refactoring regarding the _editUi-methods on Drupal.edit.views.ToolbarView. Whilst it's possible to have two - one on the prototype and another "static" one on the "object" - it strikes me as a little unusual. I replaced it w/a helper-function in util.js: Drupal.edit.util.getEditUiIntegrationAspect. This seems more "explicit" and clearer to me.

I did this because of the following reasoning:

I wanted all the Backbone-stuff to be self-contained instead of relying on util.js.
However, both the ToolbarView and the DecorationView need to be able to query this metadata.
Hence I went with a static method on ToolbarView (since ToolbarView needs to query the most metadata), and then called that from DecorationView.

I'm okay with your approach too though. Mine indeed felt too convoluted.

"aspects" have a lot of other (technical) connotations

I did s/aspect/setting/. I considered "flag", but that just read strange, I also considered your proposed "property", but the problem with that is that we already have "PropertyEditor widgets" etc, so I'd rather not have "PropertyEditor widgets properties"… :)

#13: All addressed, except for the last two remarks.

1. RE: the range remark:

I'd indeed consider it to be out of scope.
It's intended to store ranges of allowed values, by specifying allowed types (VIE.Types). By specifying PropertyEditor widgets instead, we'd be abusing VIE's attribute range AFAICT.
Most importantly: this is a chicken-and-the-egg situation in that formatters cannot know which editors will all ever be developed. Similarly though, PropertyEditor widgets cannot know the whole universe of types (or formatters) that exists (or that may ever be created). The best we can do (AFAICT), is to indicate that a certain PropertyEditor widget is an alternative to another one, and then have a dynamic way of checking whether it is compatible, so that if the original widget is unable to edit a field (incompatible) for whatever reason, that one of the alternatives may be.
All of this really boils down to mapping Create.js PropertyEditor widgets to fields, much like we have to manually specify in Drupal which Field API widget and which Field API formatter you want to use for a field (and potentially use different ones in each view mode). Create.js does not have a way of solving that automagically; based on RDFa data it can do best effort guesses, but that's it.
Some day we may want to have the ability to configure Create.js PropertyEditor widgets in the Field API UI (and indeed, it would be possible for a contrib module to implement precisely that by overriding Edit's EditorSelector), but for now, that is a bad idea because there are 1) few Create.js PropertyEditor widgets to choose from, 2) we don't have enough flexibility yet because we lack the richer metadata that RDFa annotation will give us, 3) (probably most importantly) we don't want to burden the user with that too.

2. RE: why merge attachments deeply?

Because attachments may contain settings, and settings must be merged deeply (see common.inc/drupal_pre_render_scripts(). I guess the real problem at play here is that #attached contains both JS settings (which must be merged deeply) and libraries/JS/CSS (which can be merged in the more simple manner), so now we have schizophrenic #attached. I believe we should move towards ['#attached'] = array('library' => $libraries, 'setting' => $settings), which makes sense if we get rid of the ability to specify random JS and CSS to be attached.
Anyway, cleaning that up is a whole other issue, and is related to the refactoring of the asset handling in D8.
I think it's fair to say that however it's done here is fine, because we definitely won't be breaking existing code. It will become cleaner once D8's asset handling itself becomes cleaner.

Further:
sun posted this over at #1833716-129: WYSIWYG: Introduce "Text editors" as part of filter format configuration:

Annotations are for meta data only. "Meta data" generally boils down to: Information that is needed _without_ instantiating the plugin.
I.e., pretty much comparable to any other D7-style hook pattern: The info hook declares meta data that's readily available _before_ anything is actually used. And the info hook declares callbacks [methods] to be invoked when a particular thing is used.

Based on that, we should get rid of the library key in the plugin annotations and simply use a getAttachments() method instead, which would subsume getDynamicAttachments(). So I did.

Ready for the next review! :)

Comment #16

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 January 2013 at 16:48

#15 should be rerolled to be in sync with #1875632-31: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), but that's a minor change. I'd rather get feedback on #15 first :)

Comment #17

effulgentsia CreditAttribution: effulgentsia commented 11 January 2013 at 17:40

Status:

Needs review

» Reviewed & tested by the community

Looks good.

Comment #18

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 January 2013 at 18:08

File	Size
interdiff.txt	3.68 KB
edit_editors-1874936-18.patch	63.58 KB

Great :)

While working on other things, I noticed a few comments were now outdated and a few others used inconsistent language. In this reroll, I'm only modifying comments, so retaining RTBC status.

Comment #19

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 January 2013 at 18:10

Title:

Allow modules to define new Create.js PropertyEditor widgets

» Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets)

Better title.

Comment #20

frega CreditAttribution: frega commented 11 January 2013 at 18:31

Title:

Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets)

» Allow modules to define new Create.js PropertyEditor widgets

+1 from me - again focussed on the JS side.

Didn't have time to install a working WYSIWYG but verified that classes/containers are applied correctly when getEditUISettings are "mocked" accordingly. Padding works w/ direct editables. Renaming getEditUIAspects to getEditUISettings makes it cleaner and dropping _needsPadding for a getEditUISetting('padding') in two views also makes a lot of sense.

Comment #21

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 January 2013 at 18:32

Title:

Allow modules to define new Create.js PropertyEditor widgets

» Pluggable in-place editors (allow modules to define new Create.js PropertyEditor widgets)

Thanks for the review, frega!

We crossposted — restoring better title.

Comment #22

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 14 January 2013 at 22:25

Status:

Reviewed & tested by the community

» Fixed

Sorry, meant to commit this on Friday and ran out of time.

Committed and pushed to 8.x. Thanks!

Comment #23

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 15 January 2013 at 09:22

Thanks!

I think we might want to update the in-place editing change notice over at http://drupal.org/node/1872284 ?

However, on second thought, in that change notice, no technical documentation exists. So maybe it's better to write full, proper doc pages for the Edit module once the biggest remaining tasks (see #1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG) are done, and then link to those from the above change notice?

IMO the latter is best because that would be far better for developers/end-users than many disparate, incomplete, less cohesive change records.

Comment #24

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 15 January 2013 at 17:10

Yep, that sounds good. There's space for a "how-to" manual on major APIs over at http://drupal.org/developing/api. I would make the single change notice link to the more extended documentation over there.