Problem/Motivation

When you want to fix a typo, or restructure your paragraphs, or quickly add another tag to your blog post, you always have to go to the node/%/edit page. Once you're there, you have to navigate through a potentially enormous form to find the particular thing you want to edit.

Even if #1510532: [META] Implement the new create content page design gets done (which will make the above less painful), it is still much slower to have to navigate to another page to be able to edit the thing that you're currently viewing (i.e. you see exactly where the mistake is, you know exactly where you want to edit content).

Imagine you were able to edit those things right there, in-place?

Sound like science fiction? This is becoming standard fare in most other CMS/CMF/… software. Drupal 8 needs to have this feature out of the box in order to remain competitive.

Proposed resolution

Add in-place editing to Drupal 8 core:

Screenshot of 'Edit' tab & tray in toolbar.
Screenshot of editing the node title in-place
Screenshot of editing the node author in-place
Screenshot of editing the node body in-place

On the PHP side, we implement hook_preprocess_field() to annotate fields with classes and data attributes so the JS side knows what to do. _edit_wysiwyg_get_field_editor() figures whether each field should be editable and which Create.js editor widget it should use.
Edit.module is <400 LoC, the form logic is <100 LoC, AJAX page callbacks are <200 LoC.

On the JS side, we build on top of the http://createjs.org framework (for the editing portion) and http://viejs.org (for the parsing and syncing of content portion), and we use Backbone Views to ensure everything is nicely decoupled. For details on the JS architecture, see #1824500-8: In-place editing for Fields.

Remaining tasks

Pre-feature freeze
All remaining tasks done!

However, we're:
- working with Create.js and VIE.js maintainers on fixing the two only known bugs in Edit (can be fixed post-commit)
- getting as much of the accessibility plan done as possible (can be completed post-commit)

Post-feature freeze

Critical

Major

Normal

User interface changes

No UI changes on the back-end. Whenever there is at least one field that the current user is allowed to edit, the "Edit" tab in the D8 toolbar shows up. When clicked, its tray shows up, which contains a "View/Quick edit" toggle; it is set to "View" by default. If the user toggles it, all content will fade away except for the fields that are editable. You can then click on any of them to edit them in-place.

  • The "Edit" tab & tray in the D8 toolbar:
    Screenshot of 'Edit' tab & tray in toolbar.
  • direct property editor widget:
    Screenshot of editing the node title in-place
  • form property editor widget:
    Screenshot of editing the node author in-place
  • direct-with-wysiwyg property editor widget:
    Screenshot of editing the node body in-place

API changes

  • There is a new type of plugin (to let any WYSIWYG editor become "the" direct-with-wysiwyg editor): see ProcessedTextEditorBase.
  • Formatters must now indicate the editability of textual fields explicitly (because only those can be edited with the direct or direct-with-wysiwyg editors): see the changes to text.module’s formatters, plus http://drupalcode.org/project/edit.git/blob/d3a6777e2953a72612fc2e98b93c... for docs. I’m not sure where and how this should be documented?

Commit message

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

Issue #1824500 by Wim Leers, tkoleary, frega, jessebeach, henribergius, effulgentsia, nod_, yched: In-place editing for Fields.
CommentFileSizeAuthor
#129 in_place_editing_for_fields-129.patch169.01 KBeffulgentsia
#129 interdiff.txt1.7 KBeffulgentsia
#124 in_place_editing_for_fields-124.patch169.01 KBeffulgentsia
#123 in_place_editing_for_fields-123.patch171.83 KBWim Leers
#123 in_place_editing_for_fields-aloha-integration-123-do-not-test.patch10.69 KBWim Leers
#123 interdiff.txt4.32 KBWim Leers
#119 inline-confusion-points.jpeg533.54 KBeaton
#119 inline-moving-text.jpeg364.98 KBeaton
#115 in_place_editing_for_fields-115.patch168.32 KBeffulgentsia
#111 in_place_editing_for_fields-111.patch164.55 KBeffulgentsia
#109 in_place_editing_for_fields-109.patch171.77 KBWim Leers
#109 in_place_editing_for_fields-aloha-integration-109-do-not-test.patch10.69 KBWim Leers
#109 interdiff.txt20.91 KBWim Leers
#101 in_place_editing_for_fields-101.patch169.47 KBWim Leers
#101 in_place_editing_for_fields-aloha-integration-101-do-not-test.patch10.69 KBWim Leers
#101 interdiff.txt11.56 KBWim Leers
#92 in_place_editing_for_fields-92.patch163.52 KBeffulgentsia
#92 interdiff.txt6 KBeffulgentsia
#89 in_place_editing_for_fields-89.patch166.45 KBWim Leers
#89 in_place_editing_for_fields-aloha-integration-89-do-not-test.patch10.69 KBWim Leers
#89 interdiff-75-89.txt1014 bytesWim Leers
#88 in_place_editing_for_fields-88.patch162.19 KBDavid_Rothstein
#88 interdiff-78-88.txt415 bytesDavid_Rothstein
#78 in_place_editing_for_fields-78.patch162.06 KBeffulgentsia
#78 interdiff-68-78.txt1.91 KBeffulgentsia
#75 in_place_editing_for_fields-75.patch166.4 KBWim Leers
#75 in_place_editing_for_fields-aloha-integration-75-do-not-test.patch10.69 KBWim Leers
#74 before.png4.94 KBWim Leers
#74 after.png13.16 KBWim Leers
#74 interdiff.txt2.31 KBWim Leers
#74 in_place_editing_for_fields-74.patch166.28 KBWim Leers
#74 in_place_editing_for_fields-aloha-integration-74-do-not-test.patch10.69 KBWim Leers
#68 in_place_editing_for_fields-68.patch169.1 KBjessebeach
#68 interdiff.txt547 bytesjessebeach
#64 in_place_editing_for_fields-64.patch164.89 KBWim Leers
#64 in_place_editing_for_fields-aloha-integration-64-do-not-test.patch10.69 KBWim Leers
#64 interdiff_pre_effulgentsia.txt29.59 KBWim Leers
#64 interdiff_effulgentsia.txt11.78 KBWim Leers
#64 interdiff_post_effulgentsia.txt1.96 KBWim Leers
#61 edit-1824500-61.patch154.37 KBeffulgentsia
#61 interdiff.txt4.35 KBeffulgentsia
#54 edit-1824500-54.patch154.47 KBeffulgentsia
#54 interdiff.txt54.06 KBeffulgentsia
#48 quick-edit.png5.63 KBBojhan
#48 clashing-shadows.png78.59 KBBojhan
#48 roundedcorners-darkgraydivider.png60.49 KBBojhan
#47 edit-oop-wip.txt38.79 KBeffulgentsia
#46 edit-1824500-46.patch151.65 KBjessebeach
#46 interdiff.txt14.57 KBjessebeach
#43 edit-1824500-43.patch136.99 KBeffulgentsia
#34 in_place_editing_for_fields-34.patch327.04 KBWim Leers
#34 in_place_editing_for_fields-aloha-integration-34-do-not-test.patch10.69 KBWim Leers
#32 in_place_editing_for_fields-32.patch325.01 KBWim Leers
#32 in_place_editing_for_fields-aloha-integration-32-do-not-test.patch10.69 KBWim Leers
#32 interdiff.txt15.75 KBWim Leers
#31 in_place_editing_for_fields-31.patch316.39 KBWim Leers
#31 in_place_editing_for_fields-aloha-integration-31-do-not-test.patch10.6 KBWim Leers
#31 interdiff.txt65.73 KBWim Leers
#27 before.png2.7 KBWim Leers
#27 after.png1.88 KBWim Leers
#26 in_place_editing_for_fields-26.patch314.58 KBWim Leers
#26 in_place_editing_for_fields-aloha-integration-26-do-not-test.patch3.99 KBWim Leers
#26 interdiff.txt452.35 KBWim Leers
#17 in_place_editing_for_fields-16.patch331.56 KBWim Leers
#17 in_place_editing_for_fields-aloha-integration-16-do-not-test.patch3.99 KBWim Leers
#17 interdiff.txt25.8 KBWim Leers
#17 ajax-commands-do-not-test.patch6.63 KBWim Leers
#12 in_place_editing_for_fields-12.patch331.3 KBWim Leers
#12 in_place_editing_for_fields-aloha-integration-12-do-not-test.patch3.99 KBWim Leers
#12 interdiff.txt8.43 KBWim Leers
#9 in_place_editing_for_fields-9.patch331.64 KBWim Leers
#9 in_place_editing_for_fields-aloha-integration-9-do-not-test.patch4.05 KBWim Leers
#8 createjs-structure.png39.82 KBWim Leers
#3 interdiff.txt2.67 KBWim Leers
#3 in_place_editing_for_fields-3.patch103.07 KBWim Leers
#3 in_place_editing_for_fields-aloha-integration-3-do-not-test.patch5.19 KBWim Leers
#1 in_place_editing_for_fields-1.patch102.99 KBWim Leers
#1 in_place_editing_for_fields-aloha-integration-1.patch10.5 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
10.5 KB
102.99 KB

Apply the main patch with git apply --whitespace=nowarn --ignore-whitespace <patchfile>, otherwise the images will be broken (at least on my system).

The second patch is to provide Edit + Aloha Editor integration (for that, install either the Aloha Editor module for Drupal 8, or apply #1809702: WYSIWYG: Add Aloha Editor to core.

aspilicious’s picture

Thats a lot of JS... is this alrdy reviewed? o_O
As I'm a js noob I'm just reviewing the php part.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+  // Don't provide a view/edit toggle on admin pages, node/add and node/%/edit pages.
+  if (arg(0) == 'admin' || (arg(0) == 'node' && arg(1) == 'add') || (arg(0) == 'node' && arg(2) == 'edit')) {
+    return;
+  }

On my custom forms I want to be able to configure this. Just like we have "hook_admin_paths"

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+  global $editbar;

globals? Can't we move this to the dic or some other new and fancy system. We are trying to remove these globals.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+  // Only add dependencies on the WYSIWYG editor when it's actually available.
+  if (count(module_implements('edit_wysiwyg_info'))) {
+    $libraries['edit']['dependencies'][] = _edit_get_wysiwyg_info('javascript library');

This belongs to the wysiwyg patch?

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ * Helper function for edit_preprocess_field().

We don't use these "Helper function" thingies anymore. The underscore in the function name alrdy tells me that. In stead this should start with a 3th person verb.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ *   @see edit_preprocess_field()

Hurts my eyes. This should be a small summary. The @see should be move to the end of the docblock. See the documentation on node/1354.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ * Given a field, its instance info and the formatter being used, determine its
+ * editability.

The main summary should fit in 80 chars

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ * @param string $formatter_type
+ *   The field's formatter type name.
+ * @return string
+ *   The editability: 'disabled', 'form', 'direct' or 'direct-with-wysiwyg'.

Don't forget to sepearate @param and @return with a newline.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ * Given a text field (with cardinality 1) that defaults to 'direct' editability
+ * and has text processing enabled, check whether the text format allows it to
+ * use WYSIWYG-powered direct editing or whether 'form' based editing needs to

Mai summary should fit on 80 chars. A more detailed one can be written underneath the short summary.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,365 @@
+ * Retrieves a list of all available WYSIWYG integration for Edit. Only the
+ * first is actually used.
+ *
+ * @todo Convert to the plug-in system!

Same but this is going to be a plugin soonish

+++ b/core/modules/edit/js/theme.jsundefined
@@ -0,0 +1,154 @@
+ * @param settings
+ *   An object with the following keys:
+ *   - None.
+ * @return
+ *   The corresponding HTML.

Can we always put a newline between @param and @return statements? Thnx

Final note: feature requests aren't critical. If it's critical it's a task and SHOULD be done. Else it is major.

Wim Leers’s picture

#2: Thanks for the review! Unfortunately almost everything (that is not related to doc style) you say is already answered by the issue summary:

Thats a lot of JS... is this alrdy reviewed? o_O

It was already partially reviewed, everybody agrees this needs to be refactored.

See "We're looking for reviews on everything except for JS for now (please wait with reviewing the JS until #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8) is in)." and "Critical: #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8) (will be worked on by @frega, @bergie and myself during BADCamp)".

On my custom forms I want to be able to configure this. Just like we have "hook_admin_paths"

Thanks, this is super helpful! :) I had no idea this existed :O I was even more surprised to see that it already exists since D7! Thanks a lot, glad to get rid of that ugly line of code :)
Note that the goal is to also get rid of the hook_page_alter(), but that's "Critical: #1823894: Improve Edit's "Editbar"".

globals? Can't we move this to the dic or some other new and fancy system. We are trying to remove these globals.

I know this is crappy and evil. It's also the simplest thing that could possibly work. This is "Critical: #1823894: Improve Edit's "Editbar"" again. Explicitly noted at #1823894-1: Improve Edit's "Editbar".

This belongs to the wysiwyg patch?

No. Edit's integration with a WYSIWYG editor must be pluggable. This is what provides that, but it'll be improved as we move to a plugin-based implementation. This is "Critical: #1823898: Get rid of the _edit_get_wysiwyg_info() + hook_edit_wysiwyg_info() travesty, convert to the plugin system.". I think the name of that issue also shows how much I detest the current implementation? :)

We don't use these "Helper function" thingies anymore.

Hurts my eyes. This should be a small summary. The @see should be move to the end of the docblock. See the documentation on node/1354.

The main summary should fit in 80 chars

Mai summary should fit on 80 chars. A more detailed one can be written underneath the short summary.

Thanks, all fixed.

Can we always put a newline between @param and @return statements? Thnx

I asked to not yet review the JS, but in any case: core's JS also does it this way (vertical-tabs.js).


I understand it's a lot of code to review overall (PHP+JS+CSS), but I'm wondering if you also looked at the PHP code itself, rather than just the documentation? I'd love to get feedback on that :)

The Aloha integration patch was not updated (and is from now on also marked to not be tested). I'm continuing to attach it every time to prevent confusion. Let me me know if I should do that differently.

Wim Leers’s picture

Note that you can test it right here:

http://sc1c414b09eaec05.s2.simplytest.me/node/2, user/pass: admin/admin

Or install it for yourself over at http://simplytest.me/project/spark.

mgifford’s picture

Issue tags: +a11ySprint

I think there's going to be a bit of a AE focus for the sprint. @Wim, can you update your sandbox with the latest code for us to evaluate on the weekend?

Wim Leers’s picture

Issue tags: -a11ySprint

#5:

AE focus? AE=Aloha Editor? This is not the right issue for Aloha Editor accessibility stuff.

With regard to in-place editing being made accessible:

And last but most definitely not least: how to deal with accessibility? Does it even make sense to support in-place editing for people who use screen readers? Would it be more efficient for them to continue to go to the node/%/edit page and use the form there?

If you're going to review Edit's accessibility: today I'm going to post an updated core patch, which has all of Edit's JavaScript refactored on top of Create.js. It's much better :) I will make sure to also provide a sandbox.

Wim Leers’s picture

Issue tags: +a11ySprint

d.o fail.

Wim Leers’s picture

FileSize
39.82 KB

#1741122: Validation support: validation errors currently result in JS alert()s with JSON barf has been done shortly after posting this initial patch, but more notably, we (@frega, @bergie and myself) have finished #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8). Now Edit's JS is about 20 times more awesome :) I'd like to thank @frega in particular for his vast contributions — he kept the Create.js port of Edit moving forward after DrupalCon Munich, and has been instrumental for the current JS architecture!
I will keep this comment just about JS architecture, the next one will contain the patch.

There are still a few bugs, but overall, it is working much better, is now very reliable (thanks to a much better architecture and thus less edge cases to worry about), is much more documented (still not completely though), but most importantly: it's at least 10 times more maintainable!

Overall, the JS code weight has increased (all sizes are minified + gzipped):

  • it used to be about 5 KB plus jQuery
  • now it is about 6.5 KB, plus 14 KB for Create.js and VIE.js, plus 9.6 KB for Underscore and Backbone, plus jQuery.

Having had the privilege of making "the old Edit" near-bugfree, I can't stress enough how that extra code weight is what makes this more maintainable. It's almost a pleasure to debug this now :) (insofar as debugging can be considered a pleasure)

JS architecture

We now leverage Create.js' architecture. We're only using a subset of it though. Let's go through the diagram:
Create.js structure

(First: *everything* in Create.js is a "widget", because every component is in fact a jQuery UI widget.)

  • "Create" widget is the UI, the controller, the thing that ties everything together. We don't use Create's UI, we use our own, so we don't actually use any of this. The "Toolbar" widget is part of this UI, so we also don't use that.
  • "Editable" widget: the full name is "EditableEntity" widget, and this — as the name says — maps to "semantic web entities". Thus it cleanly maps to Drupal entities.
  • "Editing" widget: the full name is "PropertyEditor" widget, and this — as the name says — maps to "semantic web properties of entities". Thus it cleanly maps to Drupal fields of entities. Currently we have three:
    1. 'form' (drupalFormWidget) for form-based editing of structured, non-textual data (this can work for *any* Drupal field)
    2. 'direct' (drupalContentEditableWidget) for plain contentEditable editing, subclasses editWidget
    3. 'direct-with-wysiwyg' (drupalAlohaWidget) for WYSIWYG editing, subclasses alohaWidget
  • "Workflows" widget: not using this yet, but we want to for #1678002: Edit should provide a usable entity-level toolbar for saving fields.
  • "Storage" widget: we are using this widget, it abstracts the storage layer away so that you can replace this with a different storage layer. This is what enables Create.js to seamlessly switch from "server storage" to localStorage (e.g. for saving drafts). Note that we're not yet using localStorage — this could only work for textual things; things you don't edit through a form (because a form may modify itself dynamically, thus how could we ever reliably save the values of a form to localStorage and reliably restore them? — this is impossible). Until we have JSON-LD in Drupal 8, we're using Drupal forms as a transport mechanism; we will be able to cleanly swap this for a JSON-LD based storage layer once that is in Drupal core (though this would only apply to textual things; things you don't edit through a form).
  • "Notifications" widget: we are not using this yet, but we might.

Create.js carries each PropertyEditor widget through a series of states: inactive, candidate, highlighted, activating, active, changed, saving, saved, invalid. The "Edit application" receives these state change requests and can decide whether they're acceptable or not. So, the core of how it all works boils down to a simple state machine.

JS code structure

Inside the "js" directory, you'll find these subdirectories:

  • createjs: all Create.js overrides
    • editingWidgets: all Create.js "editing widgets" (PropertyEditor widgets): two have been overridden, one is custom to Drupal (the form-based one).
  • lib: all 3rd party libs: Create.js & VIE.js
  • models: all Backbone models, which is just one: for the "Edit application" state
  • routers: all Backbone routers, which is again just one: so that one can provide a link to an end-user that puts the end-user directly in edit mode
  • viejs: a custom VIE DOM parsing service (based on data- attributes), this will go away once we can make the switch to RDFa
  • views: all Backbone Views: menu (for the view/edit toggle menu), overlay, modal, fielddecorator (which decorates the in-place editable fields) and toolbar (which provides per-field toolbars).

The main directory contains edit.js (with Drupal behaviors and app initialization code), app.js (the "Edit app" logic), theme.js (with overridable Drupal.theme.something functions), util.js (a few utility functions), and backbone.drupalform.js (an implementation of Backbone.sync on top of Drupal's Form API and Drupal.ajax0.

Wim Leers’s picture

Patch attached. The JS is in its final shape. Please review the code! While reviewing see the previous comment (#8), it guides you through the JS architecture.

Still apply the patch with git apply --whitespace=nowarn --ignore-whitespace <patchfile> (as per #1).

Known bugs:

  • Clicking 'cancel' doesn't restore the original content for "direct" and "direct-with-wysiwyg" editors. This is blocked on feedback from @bergie, because it's a Create.js thing.
  • Image field uploads: form & upload work, but the new image isn't rendered after "save"; seems to be always "one behind" (saving anew renders the "previously uploaded" file); also interesting: saving an "empty" upload never leads to the field being update (even after multiple saves).
  • Saving non-required field: if you set a non-required option field to "null" (or n/a) it does not get removed from the page (it's gonna on page refresh). This is blocked on feedback from @bergie, because it's a VIE.js thing.

TODO in terms of JS: We're still working with @bergie to improve Create.js in some areas, that will allow our code to be slightly simplified further/made more consistent. We're also going to work with him to make the builds smaller, but that can definitely be a post-feature freeze thing. After talking to @nod_, the current size is definitely small enough already (but since I'm a webperf enthusiast, I won't stop until it's as small as feasible). See the multitude of "@todo" statements, most of which are really just reminders for revisiting/removing code once an upstream Create.js thing has been improved :)

P.S.: the JS size numbers of #8 can be reproduced by running grunt in the Edit module's directory. We include the unminified builds of Create and VIE for now, for easier debugging.

webchick’s picture

So I added a simple text field to my demo content type in Spark and expected this to take on a "direct" editability, however when I clicked it it only showed me the form, not actual "edit in place." Did I do something wrong, or how do I test this?

EDIT: Nevermind! I was testing against the latest code in Edit module repo, not here in the patch.

seutje’s picture

I only did a little visual review, scrolling through the patch

+++ b/core/modules/edit/js/util.jsundefined
@@ -0,0 +1,154 @@
+Drupal.edit.util.calcFormURLForField = function(id) {
+  var parts = id.split('/');
+  var urlFormat = decodeURIComponent(Drupal.settings.edit.fieldFormURL);
+  return Drupal.formatString(urlFormat, {
+    '!entity_type': parts[0],
+    '!id'         : parts[1],
+    '!field_name' : parts[2],
+    '!langcode'   : parts[3],
+    '!view_mode'  : parts[4]
+  });
+
+};
+
+Drupal.edit.util.calcRerenderProcessedTextURL = function(id) {
+  var parts = id.split('/');
+  var urlFormat = decodeURIComponent(Drupal.settings.edit.rerenderProcessedTextURL);
+  return Drupal.formatString(urlFormat, {
+    '!entity_type': parts[0],
+    '!id'         : parts[1],
+    '!field_name' : parts[2],
+    '!langcode'   : parts[3],
+    '!view_mode'  : parts[4]
+  });

this seems a bit duplicate, could we unify this or something? only the urlFormat seems different.

+++ b/core/modules/edit/js/viejs/SparkEditService.jsundefined
@@ -0,0 +1,202 @@
+      var entityElements = jQuery(this.options.subjectSelector, element);

here element is a regular DOM element.

+++ b/core/modules/edit/js/viejs/SparkEditService.jsundefined
@@ -0,0 +1,202 @@
+      var label = element.data('edit-field-label');

but here it's a jQuery collection?

+++ b/core/modules/edit/js/viejs/SparkEditService.jsundefined
@@ -0,0 +1,202 @@
+      if (!element) {

and here it's a regular DOM element again?

+++ b/core/modules/edit/js/views/fielddecorator-view.jsundefined
@@ -0,0 +1,318 @@
+    if ($e === null || $e[0].nodeName == 'HTML') {
...
+    if (c == 'rgba(0, 0, 0, 0)' || c == 'transparent') {
...
+    if (pos == 'auto' || !pos) {
...
+    if (this.$el.css('display') == 'inline') {

is there a reason these are soft-checks with type coercion?

+++ b/core/modules/edit/js/views/fielddecorator-view.jsundefined
@@ -0,0 +1,318 @@
+    for (var i = 0; i < props.length; i++) {

should probably cache that length instead of re-evaluating every loop, might squeeze out a ms

+++ b/core/modules/edit/js/views/fielddecorator-view.jsundefined
@@ -0,0 +1,318 @@
+      r[p] = parseFloat(this._replaceBlankPosition($e.css(p)));
+++ b/core/modules/edit/js/views/toolbar-view.jsundefined
@@ -0,0 +1,445 @@
+        this.$el.css('top', parseFloat(this.$el.css('top')) - 5 + 'px');

Do we really need sub-pixel values here? seems like parseInt would do, and it's faster in some browsers: http://jsperf.com/parseint-vs-parsefloat/7

+++ b/core/modules/edit/js/views/toolbar-view.jsundefined
@@ -0,0 +1,445 @@
+      .find('.edit-toolbar:not(:has(.edit-toolgroup.ops))')
...
+      .find('.edit-toolbar:not(:has(.edit-toolbar-wysiwyg-tabs))')
...
+      .find('.edit-toolbar:not(:has(.edit-toolgroup.wysiwyg))')

This feels a bit awkward and Sizzle-heavy, could we split this up so we can get some qSA performance going?

I'm also a bit unclear about which bits are 3rd party bits and which are ours, so to say.

(function(jQuery) {...})(jQuery)

seems kinda silly, are we expecting something to override the global after this bit was executed? and why not alias it to the dollar-sign while we're at it?

In terms of structure: I don't see any weird things, but I'm definitely no backbone-expert, or general structure-expert, but I can already make a lot more sense of Edit than before

Wim Leers’s picture

Thanks so much for the review!

Code issues

Structural questions

  • I'm also a bit unclear about which bits are 3rd party bits and which are ours, so to say.

    Everything besides the code in js/lib is ours. Which parts were you confused about? I guess things like editable.js, where we need to override some parts of Create.js?

  • In terms of structure: I don't see any weird things, but I'm definitely no backbone-expert, or general structure-expert, but I can already make a lot more sense of Edit than before

    Great!

Updated patch

Contains all of the fixes that address @seutje's concerns; I've also fixed one of the three known bugs.

Still apply the patch with git apply --whitespace=nowarn --ignore-whitespace <patchfile> (as per #1).

Known bugs:

  • Clicking 'cancel' doesn't restore the original content for "direct" and "direct-with-wysiwyg" editors. This is blocked on feedback from @bergie, because it's a Create.js thing.
  • Image field uploads: form & upload work, but the new image isn't rendered after "save"; seems to be always "one behind" (saving anew renders the "previously uploaded" file); also interesting: saving an "empty" upload never leads to the field being update (even after multiple saves).
  • Saving non-required field: if you set a non-required option field to "null" (or n/a) it does not get removed from the page (it's gonna on page refresh). This is blocked on feedback from @bergie, because it's a VIE.js thing.
swentel’s picture

Gave this a test run and oh baby, this is so cool. I only looked at the PHP and I don't have any big problems with it, and most has been mentioned by aspilicious already too. One leftover and one question. Other than that, wow, this would be really cool to have in core.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,403 @@
+module_load_include('inc', 'edit', 'includes/text');

This file doesn't exist anymore.

+++ b/core/modules/edit/edit.moduleundefined
@@ -0,0 +1,403 @@
+    $cache = cache()->get('edit_wysiwyg_info');
+    if ($cache === FALSE) {
+      // Rebuild the cache and save it.
+      $edit_wysiwyg_info = module_invoke_all('edit_wysiwyg_info');
+      drupal_alter('edit_wysiwyg_info', $edit_wysiwyg_info);
+    }

Just wondering if we really gain much here by caching this, but then again, I don't know what kind of info is in there, but if these hooks only return arrays with info, I wouldn't really cache this.

effulgentsia’s picture

I don't know what kind of info is in there, but if these hooks only return arrays with info, I wouldn't really cache this.

Looks like #1823898: Get rid of the _edit_get_wysiwyg_info() + hook_edit_wysiwyg_info() travesty, convert to the plugin system. is listed in the issue summary as something to be incorporated as part of this initial patch. If that is to use AnnotatedClassDiscovery as is becoming our standard for core plugins, we'll likely want to use the CacheDecorator for it, because parsing annotations is slow.

yched’s picture

Wow, lots of code :-)
I can't really say I reviewed this, but I read through the field API related code.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+  $name = $variables['element']['#field_name'];
+  $langcode = $variables['element']['#language'];
+  $view_mode = $variables['element']['#view_mode'];
+  $formatter_type = $variables['element']['#formatter'];
+  $field = $entity->{$name}[$langcode];
+  $instance_info = field_info_instance($entity->entityType(), $name, $entity->bundle());

To be consistent with what the rest of core does :
$name var should be named $field_name,
$field var should be named $items (also, I think this should be $entity->get($name, $langcode) now).
$instance_info should be named $instance

Also applies to the param names in _edit_analyze_field_editability()

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+  $name = $instance_info->definition['field_name'];

FieldInstance is an ArrayAccess right now, so $instance_info['field_name'] should work.
Same for $instance_info['settings']['text_processing'] a couple lines below in the same function.

At some point, ArrayAccess will be removed, and $instance_info->fieldName will be the official syntax, but replacing that throughout all of core will be for a later (large) patch. Right now core does $instance['some_property'] all over the place, like in D7.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+    $info = field_info_field($name);

Should be named $field

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+    // If this field is configured to not use text processing; it is plain text
+    // "regular" direct editing should be used, which is already set.
+    // On the other hand, if it is configured to use text processing; then we
+    // must check whether 'direct-with-wysiwyg' or 'form' editability should be
+    // used.
+    if (!empty($instance_info->definition['settings']['text_processing'])) {
+      $format_id = $field[0]['format'];
+      $editability = _edit_wysiwyg_analyze_field_editability($format_id);
+    }

The function is generic, but this piece of code is really tied to the specific case of text fields.

I wonder if there should be a hook that would let the field type have its say, instead of hardcoding some text-specific logic in here ?

More generally, I'm not sure I get the overall logic in _edit_analyze_field_editability(). Why does 'editability' depend on the formatter that's being used ?

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,191 @@
+  $entities = entity_load_multiple($entity_type, array($entity_id));

Why not entity_load() ?

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,191 @@
+  $field_output = field_view_field($entity_type, $entity, $field_name, 'edit-render-without-transformation-filters');
+  $output = $field_output[0]['#markup'];

Isn't field_view_value() more like what you want ?

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,191 @@
+  $commands[] = array(
+    'command' => 'edit_field_rendered_without_transformation_filters',
+    'id'      => "$entity_type:$entity_id:$field_name:$langcode:$view_mode",
+    'data'    => $output,
+  );
+
+  return array('#type' => 'ajax', '#commands' => $commands);

I think this needs to be updated after #1812866: Rebuild the server side AJAX API

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+    $field = reset($children);

$field should be $field_name.

More generally, that whole edit_text_field_render_without_transformation_filters() / edit_field_attach_view_alter() part seems a bit funky. And very very coupled to the special case of text fields, although I'm having a hard time to follow the code flow and check that it only gets called on text fields.

All in all, I'm a bit worried by the amount of text-field specific code hardcoded in edit.module. That's not ideally decoupled (also, the corresponding bits don't always seem to explicitly check for if ($field['type'] == 'text'), and it also means a contrib field type won't be able to get the same treatment ?

Lastly, we should really strive for zero performance overhead when displaying an entity (or worse a list of entities) with 'edit' toggle off. Not sure edit_preprocess_field() achieves that right now ?

effulgentsia’s picture

Status: Needs review » Needs work

Yay for all the recent progress, especially related to Create.js integration. Here's a review of the PHP code:

+++ b/core/modules/edit/edit.info
@@ -0,0 +1,6 @@
+package = User interface

Should be package = Core

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+ * Provides inline content editing functionality for fields and entities.

Is it really fields *and* entities? Or is it more like "functionality for entity fields" or even just "functionality for fields".

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+ * The Edit module makes content editable inline. Rather than having to visit a
+ * separate page to edit content, it may be edited in-place.

Would be nice to standardize on whether we refer to this as "inline editing" or "in-place editing", but let's pick just one, and use it consistently everywhere. I think I prefer "in-place editing", but I'd like others to offer feedback on the meanings of these terms.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+/**
+ * Implements hook_menu()
+ */
+function edit_menu() {

We have a new router system in D8 now, so let's use it instead of hook_menu(). See #1843084: Convert user/register to Route for an example.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+    'delivery callback'=> 'ajax_deliver',

We have content negotiation in D8 now, so this is no longer needed (and ajax_deliver() no longer exists).

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,403 @@
+function edit_preprocess_field(&$variables) {
+  $entity = $variables['element']['#object'];
+  $name = $variables['element']['#field_name'];
+  $langcode = $variables['element']['#language'];
+  $view_mode = $variables['element']['#view_mode'];
+  $formatter_type = $variables['element']['#formatter'];
+  $field = $entity->{$name}[$langcode];
+  $instance_info = field_info_instance($entity->entityType(), $name, $entity->bundle());
+
+  $entity_access = edit_entity_access('update', $entity->entityType(), $entity);
+  $field_access = field_access('edit', $name, $entity->entityType(), $entity);

As yched points out, this is too much code to run every time any field is viewed. I thought the original plan was for normal "view mode" to not need any special attributes, and for "quick edit mode" to require a page refresh to get all the needed attributes. What happened to that approach?

+++ b/core/modules/edit/includes/form.inc
@@ -0,0 +1,147 @@
+    // Handle pseudo-fields that are language-independent, such as title,
+    // author, and creation date.
+    elseif (empty($form[$element]['#language'])) {
+      $form[$element]['#title_display'] = 'invisible';
+    }

In D8, all 3 of these examples will likely be language-dependent. Why is whether or not we display the #title conditional on that?

+++ b/core/modules/edit/includes/form.inc
@@ -0,0 +1,147 @@
+  // 'submit' in D8 is for "building the entity object", not for actual
+  // submission. It appears though that if there were no validation errors, it
+  // is submitted automatically.
+  field_attach_submit($entity->entityType(), $entity, $form, $form_state, $options);

1. I don't understand the second sentence of this comment. What exactly is happening automatically, and is that good or bad?
2. Please open an issue for us to rename field_attach_submit(), because in both HEAD, and here, it is called from within validation, and that is very confusing. Add a @todo comment here linking to that issue.

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,191 @@
+    $form_state['entity']->save();
+
+    $options = array('field_name' => $field_name);
+    // Reload the entity. This is necessary for some fields; otherwise we'd
+    // render the field without the updated values.
+    $entity = entity_load($entity_type, $entity_id, TRUE);

If what you're saying is that when running this code, $entity ends up different than $form_state['entity'], then congratulations, you discovered a violation of idempotence, which is a bug. Please open an issue with steps to reproduce.

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,191 @@
+  // When working with a hidden form, we don't want any CSS or JS to be loaded.
+  if (isset($_POST['nocssjs']) && $_POST['nocssjs'] === 'true') {
+    drupal_static_reset('drupal_add_css');
+    drupal_static_reset('drupal_add_js');
+  }

Rather than doing this, we should improve ajax_render() to check for that variable and not output the css and js insertion commands. I'd also suggest renaming the variable to something like $_POST['ajax_invisible'].

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextDefaultFormatter.php
@@ -23,6 +23,9 @@
+ *   edit = {
+ *    "editability" = "direct"
  *   }

For several reasons, including some mentioned by yched, I don't like this as an annotation. Instead, how about making the viewElements() method add '#edit_mode' or '#edit_in_place_mode'? And only when not using the default ('form'). By attaching it to the render array, it's easily alterable in all the places where modules might be altering the render array itself.

Wim Leers’s picture

Issue summary: View changes

Update. Create.js refactoring is done. Toolbar integration is ready. a11y plan exists.

Wim Leers’s picture

Issue summary: View changes

Drop Drupal.ajax todo, it's not essential, it's just a nice to have.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Thanks for the reviews, all! Unfortunately, all of them only touch the PHP side, so I'd still very much like reviews of the JS (and CSS) side :)


#13:

This file doesn't exist anymore.

That's right, and I'd already addressed that yesterday, before you said it, while I was working on further clean-up.


#13 + #14: the ugliness in _edit_get_wysiwyg_info() is indeed just a stopgap solution until I had time to work on #1823898: Get rid of the _edit_get_wysiwyg_info() + hook_edit_wysiwyg_info() travesty, convert to the plugin system.. Will get that solved ASAP now.


#15:

  • The variable renaming stuff: all done.
  • The function is generic, but this piece of code is really tied to the specific case of text fields.

    I wonder if there should be a hook that would let the field type have its say, instead of hardcoding some text-specific logic in here ?

    More generally, I'm not sure I get the overall logic in _edit_analyze_field_editability(). Why does 'editability' depend on the formatter that's being used ?

    Only textual fields can have "direct" editability. It's impossible to edit e.g. an image field, a taxonomy field, an options field through something else than a form. So "form" editability" is the default (this you can also see in that function: if no editability is defined for a formatter, then it defaults to "form"). So given that only textual fields can be edited "direct"ly, it may make sense for *some* textual fields to have WYSIWYG editing ("direct-with-wysiwyg"). Fields without text processing (without the filter system being applied to their contents) have any HTML tags stripped. So WYSIWYG editing is not possible for them. So only if $instance['settings']['text_processing'] is set, we should even consider "direct-with-wysiwyg" editability. It is then up to _edit_wysiwyg_analyze_field_editability() to check whether the text format that is currently used is compatible with a WYSIWYG editor.

    Secondly, why does "editability" depend on the formatter? This ties back to the fact that only textual fields can be edited directly. More specifically, the whole goal of in-place editing is that everything looks exactly the same while you're editing as it will look once it's saved. That works fine for "simple" text formatters, that display the full text (TextDefaultFormatter, which might use "direct-with-wysiwyg" and TextPlainFormatter, which will always use "direct"), but it does not work with "advanced" text formatters that trim the text or show a summary (TextSummaryOrTrimmedFormatter and TextTrimmedFormatter).
    We could in theory load the full text via AJAX, inject that instead of the trimmed/summary text, let the user edit it, save it, after it's saved replace replace the full text with the new summary/trimmed text. But clearly, that violates the principle of "everything looks exactly the same while editing as it will look once it's saved".
    Hence it depends on the formatter.

  • Why not entity_load() ?

    I have no idea. Fixed — thanks :)

  • Isn't field_view_value() more like what you want ?

    I wish, but no. I asked @swentel to look at this, because it's hairy — this is a necessity to leverage edit_field_attach_view_alter(); otherwise it won't have an effect at all. edit_field_attach_view_alter() checks if the display mode is a custom display mode (this display mode is only ever set on textual fields), and if that's the case, it's going to re-apply check_markup(), but this time without "filters of the transformation type" (this was introduced in #1782838: WYSIWYG in core: round one — filter types), which means as much as "don't apply filters like Typogrify, link ads, extlink, etc.", IOW: filters that would mess with WYSIWYG editors. Only "security" filters are still run (because we don't allow "direct-with-wysiwyg" on text formats that use a mark-up language such as Markdown anyway, in those cases, we revert to "form" editability). For in-place editing to have "true WYSIWYG" editing, this is necessary.
    I dislike this as much as you dislike it (I'm sure you will dislike this, if not detest this), but I couldn't think of a better way to handle it. If you have other suggestions, I'm all ears :)

  • I think this needs to be updated after #1812866: Rebuild the server side AJAX API

    I looked at that, and got it working, then noticed that after my conversion, File field's "Remove" button stopped working. After debugging, I pinpointed that to the patch that introduced this in the first place. Details: #1812866-54: Rebuild the server side AJAX API. Patch attached that implements this, but not yet included in the main patch because using AjaxResponse breaks this.

  • More generally, that whole edit_text_field_render_without_transformation_filters() / edit_field_attach_view_alter() part seems a bit funky. And very very coupled to the special case of text fields, although I'm having a hard time to follow the code flow and check that it only gets called on text fields.

    See above.

    All in all, I'm a bit worried by the amount of text-field specific code hardcoded in edit.module. That's not ideally decoupled (also, the corresponding bits don't always seem to explicitly check for if ($field['type'] == 'text'), and it also means a contrib field type won't be able to get the same treatment?

    1) Is it reasonable to assume that there are going to be contrib modules providing alternatives to these very basic building blocks? I'm absolutely open to make this better/more abstracted, I'm just honestly asking.
    2) I don't check if a field is provided by the text module, instead I just check if $instance['settings']['text_processing'] is present, which is not really a cleanly abstracted way to do it, but if any contrib module has a textual field with text processing enabled, then it will work as long as it stores its text processing setting in the same location. Not great, but it works.

    Lastly, we should really strive for zero performance overhead when displaying an entity (or worse a list of entities) with 'edit' toggle off. Not sure edit_preprocess_field() achieves that right now ?

    Well, the idea is that you can toggle to edit mode *without* doing a page reload. Part of the point is that it's instantaneous, super fast. If you're going to require a page reload, then part of the benefit is negated: you might as well go to node/%/edit.

    I can think of many strategies to either cache this or do it asynchronously. Furthermore, we don't actually know yet that there is a significant performance impact. Isn't this something we should address post-commit?

Commit for these changes: http://drupalcode.org/project/edit.git/commit/960890c

After the commit, I noticed that I couldn't use EntityInterface::get(), since it actually does *not* handle language, even though its signature indicates otherwise. So: http://drupalcode.org/project/edit.git/commit/27b118f


  • Should be package = Core

    Heh. Yay.

  • Is it really fields *and* entities? Or is it more like "functionality for entity fields" or even just "functionality for fields".

    Initially it also was fields — though it really was just a link to the full entity edit page. We decided that was not very useful, so now it's indeed just fields. Corrected.

  • RE: "inline vs. in-place". Yes, "in-place" everywhere is preferred. I've actually done this *everywhere*, except for in the module docblock. Stupid. Fixed.
  • We have a new router system in D8 now, so let's use it instead of hook_menu(). See #1843084: Convert user/register to Route for an example.

    I wanted to work on this, I looked at #1843084: Convert user/register to Route and #1843844: Convert cron to new routing system. Not a single route has been committed yet it seems, plus these two examples are *extremely* simplistic. There is no documentation on best practices, I have no idea why one is called ModuleNameRouteController and the other ModuleNameController, I have no idea what the top-level key naming scheme in the route yaml files is, etc. #1843084: Convert user/register to Route even introduces changes to the routing system… IOW: I think the current examples are insufficient and that changing to the route system is premature, so can we please do this post-commit?

  • As yched points out, this is too much code to run every time any field is viewed. I thought the original plan was for normal "view mode" to not need any special attributes, and for "quick edit mode" to require a page refresh to get all the needed attributes. What happened to that approach?

    Is it really? See my response above. There is indeed a "view mode" and a "quick edit mode", but it was never intended to require a page refresh.

  • In D8, all 3 of these examples will likely be language-dependent. Why is whether or not we display the #title conditional on that?

    I've gotten rid of it for now, especially because title/author/creation date don't work yet in D8 (because they're not part of the Entity Property API — yet). The reason for this is simply that Edit already provides a label; there is no point in showing the same label again in the form that we load: it looks ugly, stupid and broken.

  • 1. I don't understand the second sentence of this comment. What exactly is happening automatically, and is that good or bad?
    2. Please open an issue for us to rename field_attach_submit(), because in both HEAD, and here, it is called from within validation, and that is very confusing. Add a @todo comment here linking to that issue.

    1. I was just trying to make sense of the crazy way this works in D8 already. In D8, there is no actual submit handler, the EntityFormController stuff (or whichever class it is) takes care of that. IIRC, field_attach_submit() (just maps the form values onto the entity object (see entity_form_submit_build_entity() and EntityFormController::buildEntity()), field_attach_validate() validates the entity object itself, and if the EntityFormController notices there are zero validation errors, then it'll just save the entity object. No submit handler is involved at all. It took me a very long time to figure out this bizarre way of handling validation/saving. I'm sure it's just an artifact of the major changes that have been going on though :)
    2. Done. Issue: #1846648: Rename field_attach_submit(), because it is also called from within validation.

  • If what you're saying is that when running this code, $entity ends up different than $form_state['entity'], then congratulations, you discovered a violation of idempotence, which is a bug. Please open an issue with steps to reproduce.

    I now actually think it was a bug in my code. I first load the entity, then shove it in $form_state['entity'] = $entity, then call $form_state['entity']->save() and then expect $entity to be updated. If I keep the first two steps, but then do $entity = $form_state['entity'] and $entity->save(), it all works fine, no reloading necessary.
    Are you saying that $entity === $form_state['entity'] should remain true at all times?

  • Rather than doing this, we should improve ajax_render() to check for that variable and not output the css and js insertion commands. I'd also suggest renaming the variable to something like $_POST['ajax_invisible'].

    1) Can't we do this post-commit? We're just creatively using existing APIs, it's not really a hack. 2) I think "ajax_invisible" is not very clear. I anticipate a lot of bikeshedding on this functionality, hence I'm asking to do that post-commit.

  • For several reasons, including some mentioned by yched, I don't like this as an annotation. Instead, how about making the viewElements() method add '#edit_mode' or '#edit_in_place_mode'? And only when not using the default ('form'). By attaching it to the render array, it's easily alterable in all the places where modules might be altering the render array itself.

    I don't see where @yched has said that he doesn't like this as an annotation?
    This first lived in a hook_field_formatter_info_alter() implementation. That worked in D8, too. However, since formatter info now lives in the plugin annotations, it made sense to move it there.
    In any case, moving it to viewElements is impossible, because Edit needs to be able to inspect which formatters support "direct" editability. Thus it needs to be some sort of metadata. If it lives in viewElement, it's not inspectable AFAICT.

Commit for these changes: http://drupalcode.org/project/edit.git/commit/8d2503a


One more note: what is currently called "editability" in the PHP code base is now actually called an "editor" on the Create.js/JS side. I think it would help the overall code clarity if we'd rename "editability" to "editor" in the PHP code base, not just for consistency with the JS code base, but also because it's not such a weird term.

Wim Leers’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Haven't looked at the code yet (a quick skimming through it seems to show that there are more TODOs then code right now...), but this caught my eyes:

Creative Commons Attribution-NonCommercial license

This is *not* compatible with the GPL. You will need replacement images and remove the current images from drupal.org repositories.

Damien Tournoud’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

#19:
- Most TODOs actually relate to things that either need to happen upstream (in Create.js), or that need to be fixed in Drupal.
- Wow. We have no idea how this happened. @tkoleary's working on a new "close" icon right now to resolve that.

yched’s picture

re: perf impact:
Well, we learned the hard way in D7 and "body as field", that field rendering is a highly critical path, and worked hard to shave off any possible cycles in there. So having a *very* fast opt out so that edit_field_preprocess() is close to a no-op for most page views seems critical. I mean, this would probably need some benchmarks, but checking node_access() or field_access() for each rendered field on the page sounds like a massive perf drag :-(.

re: 'editability' -> 'editor'
Well, the createJS side is all about inline / client-side editing, so 'editor' is not ambiguous over there.
On our PHP side, though, this lives next to "regular form editing with widgets", so just 'editor' feels like it lacks scope.
Then maybe 'inline_editor' on the PHP side ?

So only if $instance['settings']['text_processing'] is set, we should even consider "direct-with-wysiwyg" editability

The thing is, whatever is in 'settings' is entirely up to the field type, and so far the whole system explicitly avoids assigning general, cross-field-type semantics on settings names and values, like "if you have a setting named 'text_processing' it will mean this and that". Really, we should ask the field type "I have this $field / $instance, what is its 'inline_editor' ?". That would mean a new "field type hook" hook_field_inline_editor() - or, once "field types" are converted to plugins, a new getInlineEditor() method in FieldTypeInterface.

Also, while I get the issue with "trimmed / full / plain" formatters (thanks for re-explaining this), I'm not 100% convinced by the "editor depends on the formatter" approach - but that's a tough one, and I have no better plan right now :-).
But for instance, the fact that, in the current patch, editability depends on :
- the field type + some specific properties in $instance (thus requires some runtime logic)
- the formatter type alone (thus can be determined by a simple entry in the formatter info / annotation)
is not too consistent, and is "just" what happens to work for text fields and formatters as they are in core right now...

This being said, while I do think there is room for decoupling , I don't really see us rearchitecture this before feature freeze, and I'm fine with keeping that for later cleanup patches...

The performance impact, otoh, seems a bit problematic to me :-( Committing a patch with a massive perf impact and tuning in followups means all other patches are evaluated with a skewed notion of performance meanwhile...

Wim Leers’s picture

RE: perf
Simple caching strategies are possible in theory, but in practice they are not possible because Drupal allows every single thing to be overridden, so the caching strategy may not make any assumptions. It is these assumptions that can simplify (read: speed up and have less requirements) the caching strategies…
That's why I doubt it makes sense to address this pre-feature freeze.
It's absolutely true that we'll have to analyze and improve performance. However, we could disable the Edit module by default for now, until we've addressed the performance aspect, so that no other perf evaluations are skewed.

RE: "editability" -> "editor
Thanks, that's helpful feedback!

RE: "text-specificness" and "factors that affect editability"
I realize that it's suboptimal. I, too, would like to see the calculation become much simpler. But then again, this ties back to the fact that Drupal allows every single aspect to be configured and altered.

In summary, to calculate "editability", we need:

  • we must check if the user has access to edit this entity *and* this field — this might be expensive
  • we assume "form" editability by default — this is very cheap
  • a formatter's configured "editability", necessary for textual fields that might use "direct" editability (to distinguish between "subset of textual field" vs. "full textual field") — this needs formatter info, and is relatively cheap (?)
  • a field's cardinality (if cardinality >1, then always fall back to "form" editability) — this needs field info, and is relatively cheap (?)
  • for a field that is still marked to use "direct" editability: a field instance's "text processing" setting, and if it is enabled, the currently active text format (so that we can load the right WYSIWYG editor, if any, otherwise fall back to "form" editability) — this is expensive (but will typically run rarely)

This may seem inconsistent, but all this information is absolutely vital. Edit is simply getting the information where it lives. Field API is designed in a logical, decoupled way, but that same decoupling means Edit has to call many different functions to get the metadata it needs.

I don't see adding a new hook as a solution, that hook would still need to receive all that metadata anyway. Or what am I missing?

sun’s picture

I didn't have time to review the whole thing. For now, just these trivial things:

 core/modules/edit/js/lib/create.js                 | 1643 +++++++++
 core/modules/edit/js/lib/vie.js                    | 3682 ++++++++++++++++++++

At least these two should be moved into /core/misc, since they are generic vendor libraries.

I did not check all other scripts in detail that are being added here — if there are any further that are not technically bound to Edit module but present generic JS libraries that can be re-used in combination with vie or createjs by other/contributed/custom modules to facilitate a Drupal integration, then I think those should be moved into /core/misc, too, so they are available without Edit module being installed. (e.g., I didn't check, but backbone.drupalform.js sounds generic)

 core/modules/edit/js/viejs/SparkEditService.js     |  208 ++

Any references to "Spark" should be removed from the core implementation.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,401 @@
+function edit_page_alter(&$page) {

Hm. To my knowledge, hook_page_alter() is planned for removal in D8 — it looks like the purpose/output of what is happening here is actually a block?

yched’s picture

Field API is designed in a logical, decoupled way, but that same decoupling means Edit has to call many different functions to get the metadata it needs.
I don't see adding a new hook as a solution, that hook would still need to receive all that metadata anyway

Sure, I don't question the 1st sentence. Ask the relevant part for the relevant piece of info in the overall logic.
Ask the access system for the access parts, ask the formatter for the formatter part, ask the field type for the part specific to a field type.
That hook I mention would only handle the part of the decision chain that's specific to a given $instance of a given field type (and a given field value, since the format being used is also part of the logic) - meaning, only the 4th point in your list.

Anyway, as I said, I'm fine with letting it rest for now.

And yeah, I guess letting 'edit' disabled by default for now would let us address the perf overhead in a followup.
But I'm afraid eventually we do need a finer killswitch than just 'edit.module disabled / enabled'...

Possible short term enhancement : entity-level 'edit' access is currently re-checked for every edit_preprocess_field(), that sounds uneeded ?

Wim Leers’s picture

#23:

RE: Create.js & VIE.js: Correct. And note that these are still the debug builds, the minified build is much smaller.
RE: SparkEditService.js: Absolutely.
RE: hook_page_alter() is going away any minute now, we're going to integrate with the new D8 toolbar and that means this is going to go away :)

#24:

Possible short term enhancement : entity-level 'edit' access is currently re-checked for every edit_preprocess_field(), that sounds uneeded ?

Isn't it up to the Entity Access API to provide caching for this? It feels strange to cache this here. If it turns out to be necessary, fine, but for now that'd feel like a premature optimization.

Wim Leers’s picture

Update.

#19: all points addressed.
- each remaining @todo is clearly annotated on which upstream thing (in Create/VIE/Drupal core) it is either blocked or postponed on. @todo's marked as BLOCKED_ON would ideally be fixed before commit, but since time is short, please take the overall stability into account. @todo's marked as POSTPONED_ON indicate things that must be improved, but that really are very minor.
There are only two @todo's remaining that don't depend on upstream things: a very, very minor CSS thing for the Aloha Editor integration (which is not up for review here, it's only here as an example — Edit is not tied to Aloha Editor at all) and one bigger thing, which is the last remaining task: #1823898: Get rid of the _edit_get_wysiwyg_info() + hook_edit_wysiwyg_info() travesty, convert to the plugin system.. I'm working on that tomorrow.
- New 'close' icon with unquestionably clear origins will solve your (most welcome!) licensing concern.

#23: all points addressed.
- The new patch includes the VIE.js "core" build and Create.js "editonly" build. I've indicated this as such in the library definitions, but honestly, this may imply that it's not a perfect fit to have these non-complete builds in core/misc, exactly because they're incomplete. That being said, it should be possible to add complementary builds for both that when combined would be equal to the complete build. That will need some upstream work though.
- All references to Spark removed (the file you pointed to was the last one with such references).
- Our use of hook_page_alter() was only temporary, in anticipation of the D8 toolbar. Since that was committed yesterday, we've already added support for it now. So, no more hook_page_alter()!

Noteworthy changes:
- @todos clearly annotated. (See my reply to #19.)
- New "close" icon to avoid licensing issues. (See my reply to #19.)
- All images have been optimized, they're now significantly smaller.
- VIE.js and Create.js moved into core/misc; i.e. they're no longer part of the Edit module. (See my reply to #23.)
- All remaining references to Spark removed. (See my reply to #23.)
- D8 toolbar integration: #1825474: Integrate the Edit module page actions into the D8 Toolbar (#1137920). There is one flaw currently: if no editable fields exist on the current page, the "Edit" tab in the toolbar will still appear. This is due to a limitation in the toolbar, and is being addressed over at #1847198: Update the structure returned by hook_toolbar(). There is a @todo for this in the code. (See my reply to #23.)
- Add csslint support to our grunt config file. Improved our CSS based on its analysis. (Note that all JS already is conformant with JSHint.)
- I tried to leverage the brand new Entity Access API (#1696660: Add an entity access API for single entity access), but I can't, because that apparently was just the framework; no entity type has implemented it yet. Furthermore, I've been told it may still change completely, this was just the first step. Hence I was unable to remove edit_entity_access(), but I did move it from missing-api.inc to edit.module since it is the only remaining missing API function. Getting rid of that is now blocked on #1839516: Introduce entity operation providers (for lack of a more specific issue).

Next up:

Wim Leers’s picture

FileSize
1.88 KB
2.7 KB

Visual changes introduced by the patch in #26:

- D8 toolbar integration: beforeafter (source: #1825474-28: Integrate the Edit module page actions into the D8 Toolbar (#1137920))
- Close icon: beforeafter
- Throbber: beforeafter

Wim Leers’s picture

Issue summary: View changes

Clarify that the D8 toolbar integration patch is ready.

Wim Leers’s picture

Issue summary: View changes

Update to reflect current state of Entity Access API; rename "type" to "editor".

effulgentsia’s picture

I propose we commit the vendor libraries separately, and hopefully quickly: #1849526: Add VIE and Create.js libraries to core (upstream fixes). Until that's done, it probably makes sense to keep posting complete patches here.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/modules/edit/edit.info
@@ -0,0 +1,6 @@
+dependencies[] = field

Should toolbar also be added as a dependency? Or is the idea to allow contrib modules to integrate Edit module some other way?

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,413 @@
+        '#prefix' => '<h2 class="element-invisible">' . t('In-place edit operations') . '</h2><ul id="edit_view-edit-toggles" class="menu">',
+        '#suffix' => '</ul>',

It's not good to put this kind of markup in #prefix/#suffix, because it's hard for themers to override. user_toolbar() and shortcut_toolbar() manage to have tray contents that don't require this. Are there no #theme functions we can reuse here instead? If that's the case, please add a @todo for us to fix that.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,413 @@
+function edit_preprocess_field(&$variables) {

To mitigate the performance impact of running this for every field, can we at least add a quick return for users without 'access toolbar' permission? However, if we want contrib to use this without a toolbar, we might need to do something a little more flexible than that.

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,413 @@
+  if ($entity_access && $field_access && $editability != 'disabled') {
+    global $edit_toolbar;
+    $edit_toolbar = TRUE;

This is a problem, because it's incompatible with block caching. What if instead of this, we always allow the Edit link/tray to appear in the toolbar, and use JS to hide it if there are no editable elements?

+++ b/core/modules/edit/edit.module
@@ -0,0 +1,413 @@
+function _edit_preprocess_field_wysiwyg(&$variables, $format_id) {

What do you think of removing everything wysiwyg related from this patch, and letting that be a follow up after we've committed a wysiwyg editor itself to core? I think that'll speed up our ability to get the initial patch in. And integrating features (e.g., wysiwyg and Edit) is allowed post feature freeze.

+++ b/core/modules/edit/includes/pages.inc
@@ -0,0 +1,179 @@
+    $options = array('field_name' => $field_name);
+    field_attach_prepare_view($entity->entityType(), array($entity->id() => $entity), $view_mode, $langcode, $options);
+    $output = field_attach_view($entity->entityType(), $entity, $view_mode, $langcode, $options);

Can these 3 lines be replaced by field_view_field()?

+++ b/core/modules/field/modules/text/lib/Drupal/text/Plugin/field/formatter/TextTrimmedFormatter.php
@@ -31,6 +31,9 @@
+ *   edit = {
+ *    "editability" = "form"
  *   }

That's the default, so why have it here?

nod_’s picture

JS review time.

First things first. Comparing non createjs with createjs version. I've reviewed both, no question about it, we want the createjs version. The previous needed workarounds all over the place to make the ajax stuff work with our (acceptedly bad) ajax api and when you're down to using setTimout(, 0) to ensure the order of execution, well, that's never good. I don't care if libraries do it, having it in core means we have to maintain it and I don't want that :)

Now the real fun, createjs. I've yet to talk to Henri about a few details and future-facing stuff, but I figured we could get a lot done beforehand.

I'll go back on the flow and responsibilities since that's the part that wasn't clear to me.

  1. VIE: this is the dirty plumbing. It holds the "entity" (VIE entity != Drupal entity) in a Backbone model and takes care of discussing with the webserver to load and save this "entity". Apparently VIE entity == Drupal entity property. This talks JSON-LD by default. In Drupal a single field that is editable independently is a VIE entity because when a VIE entity is saved, the whole thing is sent to the backend and saved.

    If your content type has 3 fields, "body", "taxonomy" and "related links" there will be 3 VIE entity taking care of each of them independently so that each field can be saved, loaded and rerendered independently.

  2. CreateJS: Handles state changes from the editor (Aloha, a form, whatever) and triggers the saving through VIE/Backbone.Sync. Activate/deactivate stuff. In this patch it takes care of telling VIE which parser it should use to parse the page and get entities out of the markup. It takes care of creating the editing widget based on some conditions that we set. It has some more functionalities that are not used in this patch (hence the custom build).

  3. Edit/Our stuff:

    1. Parser for VIE based on data-* attributes.
    2. Backbone.Sync implementation using our Ajax API.
    3. CreateJS integration, definition of Drupal.createEditable and 3 editing widget
      1. formwidget.js for things like body/taxonomy terms
      2. "simple" contenteditable
      3. Aloha
    4. Backbone view that reacts to CreateJS state changes (that's app.js, most of it is in there)
    5. Then we have a couple of things to do the nice CSS animations/transitions, greying out the background when we're in the edit view, We can simplify quite a number of things in the js/views folder if we don't do animations and all the fancy UI. It's done and working now. No real complains about the code driving this.

    There are a few ajax commands that are created.

    • edit_field_form
    • edit_field_rendered_without_transformation_filters
    • edit_field_form_saved
    • edit_field_form_validation_errors

    (better name? fieldForm and fieldRenderNoFilters or something, in any case should be camel case).

    edit_field_form takes care of displaying the field form on the page. By creating a Drupal.ajax object that reacts to the "edit-internal.edit" event and triggers it immediately (needed to workaround our unflexible ajax API).

When some field is being edited, createJS dispatch the event and our code react to it. First loading the form for the field from the backend. Then displaying either the form or just the textfield (for simple fields). Upon saving our implementation of Backbone.Sync is filling the loaded form (in both cases) and triggering the submit so that it's handled like any other ajaxified form.

Some minor details:

  • should be using drupalSettings instead of Drupal.settings.
  • should be using .on and .off

Some remarks:

  • util.js is there only because our Ajax API is not helpful.
  • our current formUpdated event is crap, it gets in the way.
  • there is a layer of complexity because we have a nice UI for it. If this needs to be simplified we'll want to simplify the interaction first to have any impact on the code size on our end. At this point since it's all working we shouldn't touch it before FF.

I might have missed a few things, hopefully that helps out for the other people wanting to review this JS :)

Actionable feedback.

  1. should be using drupalSettings instead of Drupal.settings.
  2. should be using .on and .off instead of bind/unbind/delegate/undelegate it's not D7 anymore :p
  3. having a list of ajax commands and events triggered could help for reviewing. I know we don't do that for the rest of core, but the rest of core JS isn't that complicated.
  4. Renaming those ajax commands to camelCase.
  5. (important for UX) make our formUpdated event not stupid #1685146: Refactor form.js, some things are happening around #153313: ckeditor input is lost when using the browser's back button which could help.
  6. (follow-up) integrate with the dialog patch to do modal stuff. #1667742: Add abstracted dialog to core (resolves accessibility bug)
  7. (ideally) cleaning up Ajax API so that we don't have that many ugly workarounds to do anything Ajax :p #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation
Wim Leers’s picture

I've completed the conversion of the WYSIWYG aspect to the plugin system, though I'm definitely not satisfied with the current state. This was the last remaining task (#1823898: Get rid of the _edit_get_wysiwyg_info() + hook_edit_wysiwyg_info() travesty, convert to the plugin system.).

@yched has already been reviewing it in that issue, and as a result there have been two follow-up commits already: http://drupalcode.org/project/edit.git/commit/1f5004b & http://drupalcode.org/project/edit.git/commit/244c9f7. Thanks a lot, @yched!


I couldn't convert to the new server-side AJAX commands API due to a bug in there that'd break many AJAXy things, but now I can, because it has been fixed: #1812866-79: Rebuild the server side AJAX API.

So I went ahead and committed the changes to leverage this: http://drupalcode.org/project/edit.git/commit/95cda18, then found admin theme CSS problems caused by removing ajax_base_page_theme(), which apparently is still necessary: http://drupalcode.org/project/edit.git/commit/f94762d.


#29:

Should toolbar also be added as a dependency? Or is the idea to allow contrib modules to integrate Edit module some other way?

To mitigate the performance impact of running this for every field, can we at least add a quick return for users without 'access toolbar' permission?

The idea is to ensure that Edit is not bound to D8 core's toolbar. Preferably, there is a common "toolbar API" (i.e. hook_toolbar()), but I'm not sure if that has really materialized yet. At least for now, you're absolutely right: we should make it depend on the toolbar module.
Done.

RE: "It's not good to put this kind of markup in #prefix/#suffix"
This had been a sore on my eye for months. Now with hook_toolbar(), I was finally able to swap that for clean code.
Fixed.

This is a problem, because it's incompatible with block caching. What if instead of this, we always allow the Edit link/tray to appear in the toolbar, and use JS to hide it if there are no editable elements?

1. Edit is by design/nature incapable of working without JS. Meaning that if a user who doesn't have JS enabled looks at a page, this will imply that the Edit toolbar will *always* be visible.
2. Can you elaborate on how this is incompatible with block caching? I understand why it is incompatible in the D6/D7 architecture, but in the D8 architecture I know of the ESI-approach and all that. Currently, Edit needs to be aware of any pieces of content that will become in-place editable on the page. I.e. it needs "global knowledge", whereas ESI implies no such knowledge.
3. If my understanding of "block caching" (and ESI) in D8 is accurate (and I believe it is), then the only way out of this is: A) hide the "Edit" toolbar tab by default, B) upon JS initialization, scan the page for editable content, C) if there's any editable content, only then show the toolbar tab.

Implemented.

RE: "removing everything wysiwyg related from this patch"
I'm fine with that. But let's first see if that's actually necessary, now that the "WYSIWYG editor plugin stuff" is done.

RE: "field_view_field()".
Done.

RE: "That's the default, so why have it here?"
Explicitness to avoid confusion/wrong assumptions. "direct" is only possible for textual fields. These are all textual fields, but for some it is impossible to use "direct". If you think it should be removed, no problem at all — but for now I've kept it.

Commit: http://drupalcode.org/project/edit.git/commit/b8f0287


#30: Points 1, 2, 4 addressed. Point 3: all commands are now documented in their respective AJAX command plugins, so I believe that's addressed there.

I discussed points 5, 6 and 7 with nod_ and he agrees these can all happen post-commit/post-feature freeze.

Commit: http://drupalcode.org/project/edit.git/commit/49162f7.


The last two things that need to be done: 1) tests (Gábor Hojtsy has been working on this already, and preliminary tests are already in, but excluded from the patch), 2) accessibility: #1844220: Make in-place editing keyboard and aurally accessible (Jesse Beach is working on this).

Wim Leers’s picture

Issue summary: View changes

High-level JS explanation.

Wim Leers’s picture

Issue summary: View changes

Last remaining task done; added the two known bugs as critical post-feature freeze follow-ups (but we'll try to fix them before the commit).

Wim Leers’s picture

Issue summary: View changes

Minor improvements.

Wim Leers’s picture

Reroll. Changes:

  • Now with *unit tests*, powered by DrupalUnitTestBase. Initially, they were running on top of WebTestbase and they took 36–37 seconds. After switching to using DrupalUnitTestBase, that dropped to 6 seconds!
  • hook_library_info() implementation has been cleaned up in light of the changes at #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI), plus all JS is now moved to the footer! This results in better front-end performance.
  • Finally, I've moved includes/(form|pages).inc to edit.(form|pages).inc. (I've excluded this from the interdiff to make the interdiff more useful.)

The issue summary has also been updated. Remaining tasks we're working on pre-feature freeze, but that should not prevent this from being committed:

Status: Needs review » Needs work

The last submitted patch, in_place_editing_for_fields-32.patch, failed testing.

Wim Leers’s picture

Oops. Some test files were missing. Sorry about that. These should pass.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -Needs tests, -wysiwyg, -Accessibility, -Needs accessibility review, -a11ySprint, -in-place editing, -Spark

The last submitted patch, in_place_editing_for_fields-34.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +Needs tests, +wysiwyg, +Accessibility, +Needs accessibility review, +a11ySprint, +in-place editing, +Spark
Wim Leers’s picture

Issue tags: -Needs tests, -a11ySprint

.

effulgentsia’s picture

Just wanted to leave a note here that I started doing another detailed pass on reviewing this, but ran out of steam. Will pick up again tomorrow. In general, it's looking really good though. If someone wants to beat me to RTBC'ing this, I won't object.

effulgentsia’s picture

Does anyone following this issue have ideas on what edit.module should be named? Seems like 'edit' is a bold namespace to claim (it's used in this patch as a CSS class prefix and for other prefixes). Here's some starter ideas, but please suggest others:

- editables.module (Since Editables seems to be an emerging name in VIE, the HTML5 contenteditable attribute, etc.)
- inplace.module
- ipe.module (we generally shy away from acronyms in core module names, though we do have rdf.module)

webchick’s picture

Well, Edit module was named by Dries. :) I'm not sure we want to change the name of it. I'm not horribly opposed, I guess, but what's the rationale?

The module was originally called "ipe" but the Panels people balked at people mistaking it with "Panels IPE" module, so we changed the name. Not sure if it being in core makes this argument any different, but it'll live in contrib for D7 so I'm not sure.

webchick’s picture

Status: Needs review » Needs work

#1849526: Add VIE and Create.js libraries to core (upstream fixes) just got committed, so this will need a re-roll.

effulgentsia’s picture

Well, Edit module was named by Dries. :) I'm not sure we want to change the name of it. I'm not horribly opposed, I guess, but what's the rationale?

If the name is blessed by Dries, I have no objections to it. Form API uses 'edit-' as a prefix for all form element HTML IDs, but we can either change that, or make sure Edit.module doesn't conflict with that. There may be other conflicts, but we can probably find solutions to all of them.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
136.99 KB
yched’s picture

hm, true, the clash with "edit-" FAPI prefixes is a bit unfortunate. And those have been there for years, are present in custom CSS / JS files..., so I'm not sure we want to rename those.

jessebeach’s picture

Well, if FAPI is only adding IDs namespaced to 'edit' and if the Edit module only adds classes (which it should only need to add), then the system might actually end up looking coherent rather than clashing :) It'll all look like it was planned that way.

jessebeach’s picture

FileSize
14.57 KB
151.65 KB

This patch extends the patch from #43 and incorporates code from the following issues:

#1851046: Edit mode should be activated when clicking the Edit toolbar tab
#1851092: When entering edit mode, inform users through the aria-live aural update interface about the state of page
#1851074: Isolate tabbable fields/links to editable fields and Edit module control components when edit mode is enabled
#1851144: Use <button> for elements that are buttons

Please see the accessibility plan for details: #1844220: Make in-place editing keyboard and aurally accessible

The code in this update is a first pass at addressing accessibility for aural UI users. After a round of feedback, we will know what the next round of improvements should contain.

effulgentsia’s picture

FileSize
38.79 KB

As part of reviewing the PHP code, I got inspired to refactor it to D8-style OOP, dependency injection, and new routing system. I'm not done yet, but posting a work in progress patch rebased to #46 just for keeping people in the loop. Mostly, just the EditFieldForm and EditorSelector classes still have procedural functions in them so are totally broken. Am hoping to finish it tomorrow.

While the patch is biggish, it's mostly just a whole lot of moving things around, not changing the underlying logic by much.

Bojhan’s picture

My initial review, I looked at simplytest.me demo that WimLeers linked. Let me start by saying, I have been skeptical of inline editing for a long time - but having seen the progress and its use, I am quite excited - it won't be something for everyone, but sure will be a cool feature for our content creators who constantly switch between editing screens.

Although arguably everything can be post-commit, I am also a little tired of hearing that on every patch I review for core - so this is my review, you can do something with it or not. The goal of this feature is to make a really smooth, quick back and forth of editing - so reviewing with that in mind.

Interaction
1. It feels rather silly to me that you have to press a tab, and then quick-edit. Makes it feel "not so quick". Why don't we have like a toggle? I know we might want a tab for a whole fancy save workflow, but that seems undecided still. What about something like this?

quick-edit.png

2. How to get out of quick-edit always takes me a second, this is because the active trail doesn't show "Quick edit" as activated, so it takes you a second to get out of it by clicking "View". I don't consider this part fully intuitive, you enter a mode - ideally you click that mode to get out of it again. It's a special kind of interaction (toggling), and we use a common interaction pattern for it (menu items) - that don't trigger such expectation - a menu item, takes me somewhere, it doesn't toggle states.

Then onto interaction, I created a video to showcase a few bugs http://screencast.com/t/juLOMoZD1

3. It feels a bit jarring, when the text moves around and the width of the area moves every time you click edit. Ideally its really smooth, you click edit, visually nothing changes but the addition of the top bar, and when saving also nothing happens. The fact that all this width changing happens is probably tricky to fix but to me essential to how smooth this feels in actual interaction.

4. I don't get why we have a diagonally sliding in throbber. That effect looks quite strange, either have it appear - or simply don't show it. The interaction is so fast, 99% of the time I wouldn't need it. I noticed on a slower host you might want it, so lets have it kick in after a few seconds and simply fade in and out?

5. I noticed different styling between your Save and Close button, and that of jesse's install. What is the correct button? Do they validate WCAG AA contrast?

I noticed a lot of alignment issues, in "form" mode - e.g. when editing a teaser. I assume there already where followups for this. Same when it comes to clicking around the UI in "quick edit" mode, in my demo the light gray overlapped the vertical menu.

Visually
We are in the process of working on a style guide, as to describe how we think Seven/Drupal stylistic parts should look - since we introduced loads of different styles the past few months. A few thoughts:

6. Lets remove the clashing shadows.

clashing-shadows.png

7. I'm not very fond of all the blue styling, it is quite a distraction when all you want to use it for is to put a little emphasis on the thing you are editing. Same goes for the very hard corners, this is a style we still need to establish further in core but in general we avoid very hard corners, we don't make them radius 15px, but do make them like a 2/3 px (see buttons, tabs, close icon). In this case it would help greatly with the kind of "strict" feeling it creates currently, an idea how to do that:

roundedcorners-darkgraydivider.png

Critical followups
My personal opinion is that introducing this, with two major missing features is quite a problem and neither of these are easy to solve. But do raise their criticality when this is introduced, as I do not see sites using this in practice without it.

8. Putting this in core without a WYSIWYG, to me sounds like a serious problem. Ideally we would have had a WYSIWYG in core, before we put this in but it looks like that won't happen. Most serious websites use some form of WYSIWYG, going into quick edit mode and being simply unable to effectively edit any text that has been made through a WYSIWYG is a critical problem, because that would mean a majority of websites can't use this feature.

9. The fact that you can't edit the title, would become a critical problem. I see no way, we can ever explain to users a main thing they would want to tweak isn't editable. We can choose to just ignore this problem, but I can quickly see this coming up in real world use.

Keep up the good work! I tried the a11y improvements, and they tab awesome :)

henribergius’s picture

It feels rather silly to me that you have to press a tab, and then quick-edit. Makes it feel "not so quick". Why don't we have like a toggle?

This is a good point. Now switching to quick edit mode is a Backbone route, so you can at least bookmark the link directly to that state.

In the stock UI of Create.js we're using browser's sessionStorage to make the Edit/Browse state essentially a toggle that keeps its state when browsing between pages. This could be an improvement to consider for the Edit module as well.

Here is how this works in the stock Create.js app controller (Drupal's Edit module uses its own instead of this one): https://github.com/bergie/create/blob/master/src/jquery.Midgard.midgardC...

Bojhan’s picture

We can also have "Quick edit" in contextual links or a pen icon, next to the contextual link icon (although Fitts' law might make that tricky).

Wim Leers’s picture

#48.1/2: You're absolutely right. We're planning to get rid of the tray that appears when you click the "Edit" tab, and to make it a toggle button instead. This is also necessary/better for a11y. See #1844220: Make in-place editing keyboard and aurally accessible, point 3.
This we should do pre-commit.

#48.3: First you should know that we're changing the width because otherwise it feels too "cramped": the outline is too close to where you're typing text and it is even possible that you cannot clearly see the cursor anymore if it's at the leftmost position.
Secondly, yes, the goal is absolutely to have *zero* shifting of text. Otherwise, the magic is broken. And you're right, we don't achieve that perfectly just yet. But we're close. Note that even in the screencast video that you've created, the first time you click into it, the text does *not* shift around. However, to make this perfect, we need a lot of very detailed (dare I say "minute"?) investigation of why this is failing in every browser.
This we should do post-commit.

#48.4: Another good point. I've spent some time on figuring out why it's sliding in diagonally, but I've been unable to figure that out so far. It is not intended to slide in diagonally at all. I'd almost say it's a browser bug, but probably it's a matter of timing.
I agree that it's silly to have when it's fast. I've already considered the suggestion you're making, to only show it if it's taking a long time. But imagine that we only insert it after 1 second. 1 second is a long time — it'll look like the website is frozen exactly because there's no throbber. So what we really need to do here, is collect client-side statistics to then heuristically show the throbber (if the load time tends to be larger than x (with e.g. x=0.6) seconds, then show a throbber).
I can disable the throbber altogether for now, and once we've implemented the heuristical approach, we can add it back. Does that sound okay to you?

#48.5: Damien Tournoud noticed earlier in this issue that somehow a CC-licensed image was being used for the close button. That's why that has changed. I guess Jesse has selectively updated her Edit demo, thus causing the old image to remain there. WCAG validation still needs to happen. Currently, we're looking for feedback regarding our a11y from the a11y maintainers.

#48.6: You're saying *no* shadows at all? That is possible. Due to CSS limitations, it's technically impossible to get rid of clashing shadows, unfortunately.

#48.7:
- RE "blueness": this was also part of the a11y plan in #1844220: Make in-place editing keyboard and aurally accessible, point 4 — the distinction between a "candidate" and "the focused editor" is not clear.
- RE "too hard corners, we need rounded corners": this is impossible again, as per CSS limitations.

#48.8: Of course I agree completely.

#48.9: Of course I also agree completely with this one. This cannot be solved by Edit though, we must improve node title/author/date to become proper entity properties/fields, then they will automatically become editable through Edit :)

#49: see my answer to #48.1.

#50: contextual links are terrible on mobile, since they're activated upon hover.

Bojhan’s picture

Thanks! Sorry I am stressing so much on the minute visual/interaction details, but I know most contributors don't care/review that.

#48.1,2 Cool, I would love it not to be the tab, for reasons mentioned around it introducing a different "pattern" from what you expect when you click a tab. But I am contend if this is to be discussed post commit.

#48.3 I see how this is difficult, I am fine with this to be fixed post commit, but lets make sure its a priority this is a detail most contributors won't care about but essential to the finesse of UX.

#48.4 This is probably a larger bug, I have seen it on other places where we do animations - if the item isn't there initially. I'm fine with both approaches, disabling might propel more contributions to fixing it properly.

#48.5 No worries, I can already tell you it doesn't pass WCAG AA - should be an easy fix.

#46.6 Impossible sounds a little harsh, lets investigate this further, perhaps we can find a trick to fix it or to make it less apparent.

#48.7 - I imagine the focused editor, if we have a WYSIWYG already brings more distinction by removing the bottom outline we already have a much nicer look.
- That we can't do rounded corners, again sounds like harsh to respond with impossible - lets investigate this further. I have no solutions to this as I am no front-end expert, but it seems like we shouldn't quickly shoot down such specific stylistic parts that are part of our overall look and feel.

#50 Lets not forget, we can have different solutions per platform. We already do, that's the idea of responsive design appropriate controls per platform. We area also making ways to make contextual links usable on mobile.

eigentor’s picture

Having tried this before when it was less mature, I must say it seems to work quite smoothly now.
A lot of stuff is intertwined with the wysiwyg and is probably not the fault of the edit module.
How I love this:
http://screencast.com/t/akgBdtTPc

I also feel that pressing "Quick edit" way off in the toolbar to edit a text box is a visual and UX disconnect.
But I guess changing the location of the buttons you click to edit something may be the smallest of technical challenges here.

The only not so easy UX problem I foresee is that there is two ways to edit a node: "Quick edit", so in-place-editing and normal editing through the form. This will confuse users, so which one will I choose If I edit a node, is one default and the other one just an extra option. If this is solved, I guess it will be awesome.

effulgentsia’s picture

FileSize
54.06 KB
154.47 KB

Alrighty. I finished what I started in #47. This is a complete reorganization of all the PHP code, but does not touch any of the JS or CSS. I'm uploading the interdiff relative to #46 for completeness, though I don't think it's of much use relative to reading the patch itself.

I'll post another comment here after a dinner break explaining a bit what I did to the PHP and why. In the meantime, it's entirely possible I introduced some regressions relative to #46, and we don't have much automated test coverage in the patch, so if anyone spots any, please post that info.

This does not intentionally change any functionality relative to #46 (i.e., none of the comments since then have been incorporated).

effulgentsia’s picture

I'll post another comment here after a dinner break explaining a bit what I did to the PHP and why.

Come to think of it, I don't think it makes sense to go into detail on that. Anyone who has not yet reviewed the PHP code in earlier patches won't care. For anyone who has, I hope just reading the patch (edit.module plus the classes in core/modules/edit/lib) will be clear. If there's anything not clear, please ask. I left @todo comments everywhere I thought there was still something funky going on, but I think they can all happen in follow ups. As long as Wim oks my changes, I'm willing to RTBC it at this point. Despite JS issues identified in #30, PHP issues identified in the patch's @todo comments, UX issues identified in #48, and not nearly enough automated test coverage, I still think we'll be better served getting this into HEAD and thereby more easily testable by more people. Plus, then all those follow ups can happen in parallel more easily.

Though if anyone wants to make a case for which follow ups are worth delaying broader user testing for, please make it.

yched’s picture

New code organization makes much sense IMO, great work @eff !

Nitpick : EditorAttacher::preprocessField() has two occurrences of $instance->definition['label']. Should be $instance['label'] (see my explanation in the second item of #15 above)

+ function edit_preprocess_field(&$variables) {
+   // This module provides Edit integration only via the Toolbar, so minimize
+   // performance overhead for users without Toolbar access by checking that
+   // first. Modules wishing to integrate some other trigger for in-place
+   // editing can implement their own preprocess function and invoke the
+   // edit.editor.attacher service from there.
+   if (user_access('access toolbar')) {
+     drupal_container()->get('edit.editor.attacher')->preprocessField($variables);
+   }
+ }

Makes sense, but I'm a bit surprised that there is no specific "use inline editing" permission ?

effulgentsia’s picture

I'm a bit surprised that there is no specific "use inline editing" permission

I looked through our set of permissions in core, and the vast majority are for controlling information disclosure or the ability to make some kind of change to the site. 'access contextual links', 'access overlay', and 'access toolbar' are the outliers: as far as I can tell, they do neither, and therefore, I don't know that they make sense as permissions. I didn't want to expand on this (in my opinion) bad pattern of abusing permissions. @Bojhan: what do you think though? Is the ability to access a certain UI piece for doing something you already have permission to do a legitimate use of a permission?

Bojhan’s picture

@effulgentsia I guess if we do it for contextual links, overlay, and toolbar - we should also do it for inline editing. Just to make sure that is consistent. I have no idea whether, its really abuse - you might want to limit or allow certainly functionality based on different audiences (e.g. different "editor" roles).

David_Rothstein’s picture

I just played around with this and of course it's pretty awesome. If you are building a simple brochureware site (with a one-to-one relationship between content and pages) this might be a killer tool.

The problem is that's really not Drupal's bread and butter, and I wonder how this works with more common kinds of Drupal sites. For example:

  1. Revisions. It seems with this patch, in-place editing ignores your node revision settings and always just overwrites the latest revision? That seems like a serious problem.
  2. Content reuse. The design here is of course very much geared towards the "page" as the thing you're editing (which totally makes sense on a brochureware site) but what happens when you get even a small amount more complicated than that? For example, create a view of articles and display it in a block on the front page. This patch will happily let you edit that inline. But how does a user know they are editing the actual article (vs. just editing the block)?

This is becoming standard fare in most other CMS/CMF/… software. Drupal 8 needs to have this feature out of the box in order to remain competitive.

That seems like a loaded statement :) But in any case, if it's true, then surely some other CMS has dealt with the above two problems - can we get any inspiration from them?

I can probably imagine a simple design where (1) a revision fieldset is available in the editing interface, and (2) some kind of messaging which makes it clear that the underlying content itself is being edited - i.e. it could give you a title like "Edit Body on Article test" rather than just "Body"; more like the title you see when you edit something via a normal form?

Bojhan’s picture

@David_Rothstein You are right, see #1678002: Edit should provide a usable entity-level toolbar for saving fields about this. I'm tempted to say for the complexity you describe, inline editing will unlikely to ever offer the amount of UX one would desire - and therefor will likely be disabled. The current design proposed in that issue, is still quite bad, and introduces many controls in a tiny area. Concrete5 has a very nice flow inline editing + revision workflows, but does not really deliver on content reuse (with great indicators upon inline editing, we could solve this).

Bojhan’s picture

Issue summary: View changes

Link to a11y issues, explain toolbar UI changes (and add screenshot), describe new plug-in type. Clarify what we're still doing pre-commit/pre-feature freeze.

jessebeach’s picture

Issue summary: View changes

added 1747916

effulgentsia’s picture

FileSize
4.35 KB
154.37 KB

This implements #56 plus a typo fix.

I'm tempted to say for the complexity you describe, inline editing will unlikely to ever offer the amount of UX one would desire - and therefor will likely be disabled.

I wouldn't be so quick to dismiss inline editing entirely for sites where content is reused. I came across this quote recently:

There’s a good argument that content should be able to flow to many forms and presentations. I agree with that, but I still like writing in at least one of the many possible nice presentations.

We may want to explore what the ideal configuration is. For example, should we provide configuration to enable in-place editing on a per-view-mode basis, and by default enable it on "Full" view mode and no others? And/or explore the field title or other ways of ensuring the person is aware they're editing something that might be getting displayed in multiple places and view modes?

Status: Needs review » Needs work

The last submitted patch, edit-1824500-61.patch, failed testing.

Bojhan’s picture

@effulgentsia I tried to say it very cautiously :(. My intend wasn't to "dismiss it", nor "quickly" I looked at a lot of tools that offer workflow around this. I am just implying it will be quite difficult to solve that hairy problem and that the current solution proposed does not come close, and other systems come closer but still have quite a difficult UX around this.

Wim Leers’s picture

The last two known bugs in Edit have been fixed! :) Major props to @frega and @jbeach for helping me tremendously to fix those. Note that this required upstream fixes; updated VIE + Create.js at #1849526-9: Add VIE and Create.js libraries to core (upstream fixes).

Responses:

  • The following of Bojhan's (#48) UX remarks have been addressed: points 1, 2, 3, 4 and 5 (view/quick edit toggle folded into the Edit tab, teaser form CSS breakage, all loading indicator woes).
  • @effulgentsia's changes of #54 and #56 have been incorporated. Overall: thanks a LOT, and I'm fine with all changes :). It is indeed mostly a lot of moving things around, not changing the underlying logic by much. Remarks while reviewing this:
    • I had used the Plugin/Type directory structure because Views does it this way. Your change suggests that this is not a best-practice? Where do I find these best practices?
    • Apparently the convention is to write Implements \Drupal\…, not Implements Drupal\… (even though , because you changed this in several locations. I also changed in the spots you missed.
    • EditorSelectorInterface, along with an overridable EditorSelector, to allow contrib to add more editors. Excellent :)
    • AFAICT Edit would now be the first module to use the Form directory to define its form in classes? (Except for Entity, but that's a special case.) Does this mean that Edit becomes one of the leading examples of the intended path, or that Edit is deviating from the intended path?
    • Regarding the use of drupal_add_library() to add the JS/CSS for WYSIWYG editors: you're absolutely right, I wish I were able to use #attached, but I can't, precisely because it is too late in the render system for #attached to still work at all :( Thanks for documenting that more explicitly.
    • In a lot of locations, the number of lines of code has been reduced by significantly going beyond the allowed 80 characters per line that the coding standards prescribe. Many go beyond even 120 characters. Is this the new coding style for class-based Drupal? In any case, this is something that can be addressed post-commit.
    • I … eh … simplified EditFieldForm::simplify() a bit more. And some other small improvements.
  • #59: indeed, revisions are a tricky aspect to solve. However, for many sites, there is no complex revisioning/moderation/workflow scenario, so for those sites, Edit in its current state is good to go. That doesn't mean we don't want to address it though, as Bojhan indicated we want to tackle that in #1678002: Edit should provide a usable entity-level toolbar for saving fields, post-commit.
    Secondly, this is indeed very much geared to "edit the current page" as you put it, or rather "edit the content in the context in which it is viewed", as I'd put it. You're absolutely right that in cases where (entity field) content is reused, it might not be clear to users that they're actually editing an entity, e.g. not just "a title", but "the title" of an entity. We've not yet focused on addressing that problem, because we wanted to get the overall in-place editing experience as good and solid as possible first. This is definitely one of the areas where we'd be working on next, but again, post-commit. A very simple potential solution could be to include more information in the label when the view mode is not "full": instead of just "Title" or "Body", we could make that "Product title" or "Blog post body".
    I believe it is reasonable and acceptable to defer the addressing of these concerns post-commit.

We've created #1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG to handle follow-ups after this gets committed. This patch needs #1849526-9: Add VIE and Create.js libraries to core (upstream fixes) to work correctly.

3 interdiffs are attached that should make it easier to review this patch:
- the changes made to Edit before merging in @effulgentsia's work (interdiff_pre_effulgentsia.txt)
- the changes I made to @effulgentsia's work specifically (interdiff_effulgentsia.txt)
- the changes made to Edit after merging in @effulgentsia's work (interdiff_post_effulgentsia.txt)

This one should be close to RTBC… :)

effulgentsia’s picture

I had used the Plugin/Type directory structure because Views does it this way. Where do I find these best practices?

There's the Plugin API handbook, but its plugin manager example uses a Plugin/Manager namespace, which is outdated. I opened #1858360: Move plugin manager classes out of \Type sub-namespace to discuss if/when Type should be used.

Edit would now be the first module to use the Form directory to define its form in classes? (Except for Entity, but that's a special case.) Does this mean that Edit becomes one of the leading examples of the intended path, or that Edit is deviating from the intended path?

There's a little discussion about forms as classes/plugins in #597280: Introduce form registry to fix various problems and increase security. I don't know if we'll want to make every core form a class or not, but I think there's benefits to doing so, like lazy loading via PSR-0. In the case of Edit, I though it ok to do, since Entity does it and the Edit form is basically a mini Entity form.

In a lot of locations, the number of lines of code has been reduced by significantly going beyond the allowed 80 characters per line that the coding standards prescribe.

Where do our coding standards prescribe that? I was aware of them prescribing line breaks in comments to stay within 80 chars, but I'm not aware of constraints on non-comment code lines.

Wim Leers’s picture

Thanks for linking to those issues, following them.

Regarding the 80-char limit: it's listed right at the top of Coding standards — Line length and wrapping :) Or what am I missing?

I hope the unorthodox interdiff strategy made sense?

effulgentsia’s picture

Thanks for the link in #66. I hadn't read that section before. Here's some bullet points from there that make exceeding 80 ok.

  • Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 chars.
  • Control structure conditions may exceed 80 chars, if they are simple to read and understand.

But here's one that some of my cleanup might have violated:

  • Control structure conditions should also NOT attempt to win the Most Compact Condition In Least Lines Of Code Award.

I know where the line for me is between "simple to read and understand" and "attempting to win the Least LOC Award", and I don't think that I crossed it, but it's also subjective, so feel free to fix the places where you think I did cross it :)

jessebeach’s picture

I corrected a small issue with recalculating the height of the Toolbar when inline edit mode is triggered and an open tray is in the horizontal position.

The interdiff

diff --git a/core/modules/edit/js/views/menu-view.js b/core/modules/edit/js/views/menu-view.js
index 2bcdab9..ac7c4e4 100644
--- a/core/modules/edit/js/views/menu-view.js
+++ b/core/modules/edit/js/views/menu-view.js
@@ -59,6 +59,10 @@ Drupal.edit.views.MenuView = Backbone.View.extend({
         .not('#toolbar-tab-edit')
         .add('.tray', '#toolbar-administration')
         .removeClass('active');
+      // Set the height of the toolbar.
+      if ('toolbar' in Drupal) {
+        Drupal.toolbar.setHeight();
+      }
     }
   },
   /**
Wim Leers’s picture

#67: As I said in #64: it's no big deal, we can do that post-commit, if other people besides I consider it necessary :)

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

For me, this is an appropriate level of completion for an initial patch, so RTBC. If someone feels there's something that must be addressed now, and not in a follow up, please set it back to the appropriate status.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
  1. #59: indeed, revisions are a tricky aspect to solve. However, for many sites, there is no complex revisioning/moderation/workflow scenario, so for those sites, Edit in its current state is good to go. That doesn't mean we don't want to address it though, as Bojhan indicated we want to tackle that in #1678002: Move save/cancel to toolbar rather than per-field basis, auto-revisions on edit (with automatic log messages), post-commit.

    That other issue is for a new feature; what I'm talking about here is a critical bug (involving both data loss and access bypass). The simplest way to fix it is to not add anything new to the form, but just make sure the Edit module is respecting the default value of the revision checkbox. (Right now it isn't, which means it clobbers the previous revision whenever you edit, regardless of whether that's the default behavior of the content type or even whether the user who is editing has permission to change the default behavior.)

    I got the impression this bug might have a deeper cause so I looked into it and it seems to be due to this:

    +  public function build(array $form, array &$form_state) {
    +    // Add the field form.
    +    field_attach_form($form_state['entity']->entityType(), $form_state['entity'], $form, $form_state, $form_state['langcode'], array('field_name' =>  $form_state['field_name']));
    

    Why is this code building a form for the field only? Shouldn't it build the form for the entity instead (and use #access to hide everything except the field in question)?

    I think that would solve the bug, but also, it would make this form easy to extend. For example, if someone wanted to write code to expose the revision checkbox and/or log message inside the inline editing form (rather than always having a blank log message), that would be very simple. Right now, I have no idea how you'd do it.

  2. You're absolutely right that in cases where (entity field) content is reused, it might not be clear to users that they're actually editing an entity, e.g. not just "a title", but "the title" of an entity. We've not yet focused on addressing that problem, because we wanted to get the overall in-place editing experience as good and solid as possible first. This is definitely one of the areas where we'd be working on next, but again, post-commit. A very simple potential solution could be to include more information in the label when the view mode is not "full": instead of just "Title" or "Body", we could make that "Product title" or "Blog post body".

    Sounds similar to what I suggested in #59, so I guess everyone's on the same page :) I think something like this should be done now to make sure it works as intended. Tweaking the wording is what could be a followup.

    However, I don't understand why it would be excluded for the "full" view mode? I'm guessing you mean when the node is being displayed as the main item at node/[nid] (i.e., $view_mode == 'full' && node_is_page($node)), but even then, I don't think it should be special-cased. Especially because themers often do crazy things with the node page design (such that a particular field might be displayed in a way that it's not totally obvious it's part of the main node).

  3. +dependencies[] = toolbar
    

    Er, what about Admin Menu users (for starters)?

    I see a comment above that says this dependency is only being added "for now", but I don't see why. If we don't want to add it in the long run, don't add it in the first place :) There is no reason to introduce artificial limitations.

    Related to this, there does seem to be a lot of toolbar-specific code in the patch right now. Is there a reason the "Edit" link couldn't instead be implemented as a regular menu link (with attached CSS/JS in the theme layer)? It could be in the toolbar by default, but a site administrator would have the freedom to put it wherever they want.

    I think this would have been relatively easy with the toolbar that was in core up until recently, but the one that is in D8 right now might make it impossible to put a normal menu item at the top level of the toolbar. (If so, that's the toolbar's problem to solve, not this issue; but I'm concerned it's having a negative effect on the implementation here, especially since the exact toolbar design is still under discussion in various other issues.)

  4. +/**
    + * Implements hook_toolbar().
    + */
    +function edit_toolbar() {
    +  if (path_is_admin(current_path())) {
    +    return;
    +  }
    

    Why? The "Edit" link already hides itself if there isn't any editable content on the page, right? (Many admin pages will tend to fall under that rule anyway, but for ones that aren't I don't see why inline editing should be prevented.)

jessebeach’s picture

David_Rothstein, good point about hook_toolbar. We're looking at how to improve that hook here.

#1847198: Update the structure returned by hook_toolbar()

It's not Edit module's issue to solve. You're right that ultimately, we shouldn't need a dependency on the toolbar for this module.

Bojhan’s picture

@David_Rothstein Regarding labeling, perhaps we should separate that from this issue for now - I gave it some thought, and it introduces quite some visual load. I agree we can make the labels more meaningful, especially when you have different nodes on a page and/or complex layout structures where there is no obvious "main part". But introducing labeling like "[content-type] body" might end up biting us in the ass, if we don't explore further what the impact of that is + visual tweaks we can do - so I would advise against putting that in this patch.

Because in many usecases this will introduce needless clutter, and especially with different view modes we might want to do more handling, e.g. "Article Body: Teaser". We might want to introduce some additional styling, since "Body" is the main thing people want to know (e.g. using different colors for [content-type]). And for mobile we might not want to show this information as it will consume a whole row. So loads more exploring, thinking around this - I think best handled in a separate issue.

Wim Leers’s picture

#71: Jesse has addressed points 3 and 4 in #72. Bojhan has addressed your second point in #73. I"ll address the first point.

  • For most of the life of Edit, Edit has actually had the ability that you describe. If the field you're editing through Edit was on an entity type/bundle that had the "Create new revision" setting enabled by default, then you'd get the same "Create new revision checkbox + log message text area" in the in-place field editing form that you get on the full node edit form.
    The problem with that though is that it completely destroys the smooth, swift UX that is the whole point of in-place editing. Instead of users having to only click on fields they want to change, make the changes, save them, they now also have to 1) look at this revision handling for every single field they edit, 2) think about keeping that checkbox checked or unchecking it, 3) if they keep it checked, also coming up with a good log message.

    Judge for yourself, without and with per-field revision handling:

    before.png

    after.png

  • Note that I said "for most of its life". The funny anecdote here is that nobody ever discovered this feature. Nor did anybody ever say they wanted it to work this way. So recently, I simply removed that. What we need, is for the revision handling to live somewhere else, because it would make the in-place editing itself a non-smooth, ugly, and IMHO even painful experience. (If you'd install the latest Spark alpha for Drupal 7, you'd still be able to use this.)
  • I'm of course not arguing that we shouldn't have revision handling at all. We do need that. But we need it to work in tandem with the rest of the experience, and not work against it. The only way we could see that work, is by not burdening every field that can be edited with revision metadata. I.e. while the user is happily editing in-place, save all changes that are being made by the user either into a new revision (or even create a new "temporary" revision for each changed field individually, then clean up the revisions once the user is ready with editing — this is similar to what Wordpress does), or into the TempStore. Once the user is ready with the editing, we could then ask the user to say whether (s)he wants to A) overwrite the latest published revision, B) save it into a new revision, though we could just not ask that question and default to B if "Create new revision" is enabled.
  • We've been thinking about workflow handling (revisioning is just one aspect of that) for a long time; hence the Spark team has helped land #218755: Support revisions in different states. We even had a prototype built and usability tested for this (#1780984-8: Test prototype for Save/Publish in one place + revision selection). We were waiting to see what would happen with #1777388: Support arbitrary workflow states. The reason we haven't addressed it fully yet, is because we wanted to get the MVP ready, solid and committed. For many use cases, no workflow (and thus revision) support is not a problem. That's what the current core patch achieves.
  • How about a simple compromise for now. This solves addresses your excellent point regarding data loss and access bypass. If the "Create new revision" setting is enabled for an entity type/bundle, then new revisions are created automatically, and they get a Updated the %field-name field through in-place editing. log message. Otherwise, it works like it does today.

As you can see, a very long response, because workflow/revision handling is very important to get right, but also very hard to get right. I still firmly believe we have the MVP in this patch — especially with the small changes added in this reroll of the patch, which addresses your (excellent!) fundamental concerns. :)

Wim Leers’s picture

Doh. I forgot to pull, so #74 excludes #68 :(. Rerolled. The interdiff for #74 -> #75 is identical to the one for #68. Stupid stupid stupid!

klonos’s picture

Related issue: #1860402: Introduce a tempstore layer to in-place editing, similar to Views, that allows for atomic revisions of multiple page edits

This is a crucial feature (task to be done actually) IMO and it deserves a place in the issue summary. I'd add it myself, but I don't know the right section to place it in.

jessebeach’s picture

Just a reminder, we're tracking larger follow-up issues here, most of which are blocked on other issues.

#1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG

effulgentsia’s picture

I'd like to propose an alternate direction than #75: instead of temporarily fixing lack of revision handling with a compromise of automated log messages (which begs for debate on whether such automation is desired), what if we make our temporary compromise simpler: simply disallow in-place editing of node types that auto-revision by default. This patch does so: interdiff relative to #68 attached.

Additionally, I don't see how #72 answers #71.3 and #71.4. This patch addresses those concerns directly. It's true that in the absence of toolbar.module being enabled, there's nothing in the UI that lets you in-place edit, but I didn't find any PHP or JS errors by allowing edit.module to be enabled without toolbar.module. It happily adds some HTML attributes to the field output even though there's nothing taking advantage of them. Meanwhile, it allows some other module to come along and trigger in-place editing without needing toolbar enabled. Similarly, I didn't see why the path_is_admin() check was needed any more: it used to be in earlier versions of this patch, but I think the JS code has become more robust since then, as David points out in 71.4.

Why is this code building a form for the field only? Shouldn't it build the form for the entity instead (and use #access to hide everything except the field in question)?

No, it shouldn't build the form for the entity, because this form is only used as a fallback editor. For text fields, the 'direct' or 'direct-with-wysiwyg' editor can be used to bypass this form entirely in some cases. And potentially, contrib can add other dedicated editors (e.g., an optimized one for images). So any additional workflow enhancements that are needed need to be implemented outside this form, not in it. This form really needs to be just about the one field. (Note: EditController::fieldForm() currently uses this form for handling submission/saving of modified data, even for text fields, but that will likely get improved when we can do those hidden submissions via REST/JSON-LD rather than Drupal's AJAX).

For example, if someone wanted to write code to expose the revision checkbox and/or log message inside the inline editing form (rather than always having a blank log message), that would be very simple. Right now, I have no idea how you'd do it.

Well, if you really wanted to extend this form, you could implement hook_form_edit_field_form_alter(), but per above, revision handling isn't a good use case of that. Extending general (non-field-specific) in-place editing probably requires some knowledge of the Create.js architecture, though maybe in the course of #1678002: Edit should provide a usable entity-level toolbar for saving fields, some Drupal-friendly hooks will get created for it?

effulgentsia’s picture

One thing I noticed when working on #78 is that even #68 has the following bug, which is unchanged by the changes in #78: if you have Overlay enabled, and are on a node view page where the Edit link in the toolbar is available, and then you click on some admin link that opens the overlay, then the toolbar's Edit link from the underlying page is still clickable and clicking it does weird stuff: once it seemed to enable in-place editing of what's below the overlay; another time, it busted me out of the overlay.

moshe weitzman’s picture

FWIW, we have had productive discussions about removing overlay once we have strong in place editing. I mention that just to emphasize that "play nicely with oerlay" should not be a pressing concern.

sun’s picture

I'm confused by the last comments on entity/field revisions. I don't see why that matters — when editing a field value, regardless of in-place/frontend or in the backend, the entity has to go through its regular CRUD update cycle, since that exists for a reason and has consequences on many fronts; i.e., we cannot just change and save the field values. Thus, I can't see why the revisioning flag plays any role; if revisioning is enabled, then a new revision needs to be stored, and if it's disabled, the current revision is overwritten. But that logic is an intrinsic part of the entity's (storage) controller, and nothing that anyone/anything else should care about.

(Sorry, I didn't look at the latest code yet, so my comment is solely based on recent comments.)

effulgentsia’s picture

Thus, I can't see why the revisioning flag plays any role; if revisioning is enabled, then a new revision needs to be stored, and if it's disabled, the current revision is overwritten. But that logic is an intrinsic part of the entity's (storage) controller, and nothing that anyone/anything else should care about.

Well, it's NodeFormController::prepareEntity() that calls $node->setNewRevision(in_array('revision', $node_options));. You're right that the rest is handled by the CRUD cycle. But EditFieldForm does not invoke prepareEntity(). I haven't thought enough about whether it should though: see my comment in #78 for why EditFieldForm shouldn't do all the things that entity forms do, but prepareEntity() might be something it should do. Regardless though, we can certainly have edit.module invoke setNewRevision() from somewhere appropriate, but the reason I didn't in #78 is because we don't have any UI for entering a log message. I'm not yet convinced that creating revisions without log messages (or with auto-generated log messages) is a better interim step than not allowing in-place editing for node types that require new revisions on every change. I think #1678002: Edit should provide a usable entity-level toolbar for saving fields will solve it correctly (or at least with more in-depth community discussion) in either case, so if people here would prefer #78 changed to save revisions with empty log messages or with automated log messages (as in #75), I won't object to that. Just trying to find a way to get this initial patch to a point where we're okay committing it and doing the rest in follow ups.

Bojhan’s picture

It seems like #79 should be addressed, as it will have an impact also on modal dialogs? @Moshe Sorry, but "we" have not had productive meetings about removing the overlay, so I see no valid reason to break an existing UI - the fact that the new toolbar went in broken with the overlay, to me is a serious issue and I don't think we should pursue the same strategy for inline editing.

@effulgentsia I like your idea of removing it when there are automatic revisions, though it probably doesn't really matter :) It's all stopgap solutions, for the real issue #1678002: Edit should provide a usable entity-level toolbar for saving fields. Whatever we do here, we probably shouldn't fuzz too much, because we are going to change it in that issue anyway.

webchick’s picture

I prefer #75 over #78. Whether or not content types are defaulted to revisions on or off is a site builder distinction; you can't expect content editors to understand this when they're navigating around and thinking things are broken because node/1 has inline editing but node/3 doesn't.

webchick’s picture

Status: Needs review » Needs work

And I guess needs work for #79, but that sounds like an edge case to me. If it's taking longer than a couple of hours to figure it out, I'd say let's hold off until post-commit. I agree with Moshe that once inline editing is in place (EDIT: and now that we have proper dialog support in core), Overlay's spot in core needs to be properly re-considered in a follow-up. If the outcome of that discussion becomes "Remove Overlay from core," I certainly don't want to hold up this feature over smoothing out bugs with Overlay. And if Overlay remains, #79 is a "normal" bug, at best.

Bojhan’s picture

@webchick Since we have no place in Core we use modals yet, lol - we don't know if this also occurs on modal dialogs - so I am not sure how contained this issue is to the overlay. Keep in mind though, that the "edit" tab now immediately triggers the "edit mode" - if you have an overlay open, it will close and do weird things, e.g. occasionally it doesn't trigger anything, other times its triggers a white bar on top, sometimes it works. I think clicking a button in the main toolbar of Drupal, causing weird things to happen is a serious issue. Whatever issue queue priority that reflects is not something I am concerned with.

Lets keep all of the drop overlay talk out of this issue - we don't know if and what happens, until then this is still a bug - that hopefully can be solved.

David_Rothstein’s picture

  1. I prefer the behavior in #75 but the code in #78 :) But if the prepareEntity() idea works, that seems even better. In the meantime, #78 would be confusing, but how much more confusing would it be than other things that don't have inline editing on the page (e.g. node titles, block bodies, etc)?

    No, it shouldn't build the form for the entity, because this form is only used as a fallback editor. For text fields, the 'direct' or 'direct-with-wysiwyg' editor can be used to bypass this form entirely in some cases.

    I couldn't figure this out - I got "direct" on the page but it still seemed to use the same form.

    In any case, I spent a while on Friday trying to get this working with a shared entity form, but eventually gave up - the form/entity API don't seem to play nicely enough together for that to work as smoothly as it would need to. It also is tricky, because although most things (e.g., custom submit handlers) probably do make sense to share between the forms, some things (like drupal_set_message() calls) wouldn't. So, I gave up on that. I guess we're going to officially support multiple independent forms for "a person is editing this entity via the user interface", which is a new thing (but probably a good new thing) - though this also means if you want to react to those forms e.g. on submit you'd need two separate hook_form_alter()s to do so. Given that we don't want an infinite number of forms doing the same thing, I still have to wonder if Edit module is really where the second form belongs? (Shouldn't the base form for editing a single field be in the Field module, and Edit module can then extend it, e.g. with hook_forms() or something?)

  2. @David_Rothstein Regarding labeling, perhaps we should separate that from this issue for now - I gave it some thought, and it introduces quite some visual load.

    Understood, but shouldn't we make some effort to solve the problem here? It could definitely be refined later, but the module just seems like it will be confusing on most Drupal sites without some kind of solution that helps people understand the extent of what they're editing, and it's the kind of issue that could easily get forgotten (or left behind because it winds up being really hard) if left for a followup. I guess the patch doesn't actually put the module in the Standard install profile, and "in core but not installed by default" is actually not a bad place for functionality that's very useful but for a minority of sites... however, it also tends to be the place where modules go to die :) It would be great if we can do better.

  3. The overlay bug isn't an edge case (I noticed it too - it happens on basically every page load)... however, it's certainly true that wanting the Edit module to work inside the overlay would be an edge case, so can we solve this just by making the "Edit" link disappear whenever the overlay is open?
  4. I can't figure out why inline editing in core would be a reason to remove the overlay. I guess it replaces some things people often do in the overlay (editing existing fields on existing content), but what about the other 90% or so? And modal dialogs aren't really a reasonable replacement for that either, because you can't navigate around inside a modal dialog - if you could, it would be a replacement, but it would also be no different from the overlay itself :)
David_Rothstein’s picture

This patch is based off #78 and fixes the overlay problem via the method I mentioned above.

Wim Leers’s picture

So I gather that the behavior I proposed #75 is considered somewhat better (by webchick, David_Rothstein and I) than what effulgentsia proposed in #78. Hence I stuck with that, but incorporated effulgentsia's other changes in #78.
I also incorporated David_Rothstein's Overlay fix of #88.


how much more confusing would it be than other things that don't have inline editing on the page (e.g. node titles, block bodies, etc)?

Far less. There's in-place editing for every field that exists. Block bodies aren't fields. Node titles are "pseudo fields"/"extra fields", not actual/proper/real fields.
In-place editing of things other than fields is simply not yet supported. Also see the issue title :)

I still have to wonder if Edit module is really where the second form belongs? (Shouldn't the base form for editing a single field be in the Field module, and Edit module can then extend it, e.g. with hook_forms() or something?)

Well, Edit module doesn't invent/generate magical new forms. All it does, is use the field's widget. So, you can still use hook_field_widget_form_alter() to modify both "in-place editing" and "full editing" forms simultaneously.
I realize that you're talking about the entity aspect of forms, not the field aspect, but then again in-place editing is all (and only!) about editing fields.

It could definitely be refined later, but the module just seems like it will be confusing on most Drupal sites without some kind of solution that helps people understand the extent of what they're editing, and it's the kind of issue that could easily get forgotten (or left behind because it winds up being really hard) if left for a followup.

We're still going to refine Edit post-commit, especially in #1678002: Edit should provide a usable entity-level toolbar for saving fields. The issue of labeling is partially also a design issue, and for that reason, the changes we make in #1678002: Edit should provide a usable entity-level toolbar for saving fields will significantly affect the labeling problem: the "Save" and "Close" buttons that are currently per-field will most likely be moved into the toolbar, which will create additional (breathing) room, and will thus affect the way we can/will do the labeling.
So I feel very strongly that we should not address this here and now.


Lastly, some not-so-well structured thoughts regarding prepareEntity().

But if the prepareEntity() idea works, that seems even better.

Note that prepareEntity() lives in EntityFormController (well, there it is a no-op) and is fully implemented in NodeFormController. However, Edit doesn't use EntityFormController at all; it builds the forms it needs through field_attach_form(), because all it really cares about is the field's widget. Hence it is (AFAICT) impossible to use prepareEntity().
From what I read here and in the code, I started to suspect that it might be essential to call hook_node_prepare() (which is currently called by prepareEntity()), because it could potentially change the values. But then again, "preparing" is something that seems to belong in (Entity|Node)StorageController; I guess the intention here is not "general preparing", but "preparing for use in form".
Most prepareEntity() stuff seem to be about "entity settings" (e.g. the comment setting), "calculating values of pseudo fields" (e.g. the node date & author), but then ViewFormControllerBase::prepareEntity() appears to somewhat abuse it to also deny access in certain cases (in theory they should override EntityFormControllerInterface::build(), but then they'd have to re-implement EntityFormController::build() and EntityFormController::init(), hence it's not really abuse).

In conclusion, the only things we could possibly want from NodeFormController::prepareEntity() are these:

    $node_options = variable_get('node_options_' . $node->type, array('status', 'promote'));
    // Always use the default revision setting.
    $node->setNewRevision(in_array('revision', $node_options));

    node_invoke($node, 'prepare');
    module_invoke_all('node_prepare', $node);

However, it is possible that hook_node_prepare() implementations assume that it's going to be about the full node form. That will not be the case here. In general, it seems this hook is intended to be for "meta modules" (things like comment, metatag, etc.) that add some kind of entity-level setting.
Hence I believe it doesn't make sense to support this for in-place editing of fields.

I could be wrong, though, but in that case the "prepare stuff" needs to be defined more clearly, so let's deal with that outside of this issue.

Wim Leers’s picture

I just noticed #1862750: Implement entity access API for nodes, that will allow us to get rid of the currently hacky access check implementation, because the conversion to Entity Access API is not yet complete. That's a more specific issue than #1839516: Introduce entity operation providers, so I'll now link to that one.

Wim Leers’s picture

Issue summary: View changes

The two known bugs have been fixed :)

Wim Leers’s picture

effulgentsia’s picture

I couldn't figure this out - I got "direct" on the page but it still seemed to use the same form.

If you add a simple Text field (not Long text and summary) to the Page content type, and leave the Text Processing setting at Plain text, then you can get the Direct editor. When you click into the field to start editing it, you can just edit, there's no AJAX request sent, and therefore, no form is returned. When you click Save, you get 2 AJAX requests: one to build the form followed by one to submit the changed values, but per #78, we're hoping to replace this with something RESTful once that's possible (#1826688: REST module: PATCH/update), at which point, there would literally be no form involved at all.

It also is tricky, because although most things (e.g., custom submit handlers) probably do make sense to share between the forms

Why would custom submit handlers make sense to share between a node/NID/edit form and an in-place edit submission? That would be equivalent to saying those submit handlers should also be shared with a RESTful PUT/PATCH submission. No: these are all *different* ways of editing an entity. They need to share validation, but that's a job for #1696648: [META] Untie content entity validation from form validation. They need to share presave()/save() logic, and they already do. But form submit handlers are specific to the form.

Given that we don't want an infinite number of forms doing the same thing, I still have to wonder if Edit module is really where the second form belongs? (Shouldn't the base form for editing a single field be in the Field module, and Edit module can then extend it, e.g. with hook_forms() or something?)

That makes sense, but can we punt that to a follow up, where Field API maintainers can scrutinize it without wading into this entire issue (though yched has been following this issue anyway, but still)?

I guess the patch doesn't actually put the module in the Standard install profile

That's silly. This patch fixes that.

Note that prepareEntity() lives in EntityFormController (well, there it is a no-op) and is fully implemented in NodeFormController.

That's also silly. I opened #1863258: Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API to fix that.

I prefer the behavior in #75 but the code in #78

I'm surprised there was such easy agreement to automated log messages. Great! This patch removes the EditFieldForm::applyDefaultRevisioning() implementation, and reimplements it with a flow more similar to EntityFormController.

Understood, but shouldn't we make some effort to solve the [labeling] problem here? It could definitely be refined later, but the module just seems like it will be confusing on most Drupal sites without some kind of solution that helps people understand the extent of what they're editing, and it's the kind of issue that could easily get forgotten (or left behind because it winds up being really hard) if left for a followup

Given #91, I'm confident it won't be forgotten, though it's prioritized as a "normal" (which I agree with), so Dries could conceivably release Drupal without it being solved. I don't know what makes sense to do in the meantime though, other than committing it (with the Standard profile inclusion) and gathering wider feedback in the other issue.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

I don't think there's anything controversial in #92's interdiff, so I'm tentatively marking RTBC again. @David_Rothstein: if you remain concerned about punting the field label issue to a follow up, or anything else, please kick the status back.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/edit/lib/Drupal/edit/EditorAttacher.phpundefined
@@ -0,0 +1,83 @@
+  /**
+   * Constructs a new EditorAttacher.
+   *
+   * @param \Drupal\edit\Access\EditEntityFieldAccessCheckInterface $access_checker
+   *   An object that checks if a user has access to edit a given field.
+   *
+   * @param \Drupal\edit\EditorSelectorInterface $editor_selector
+   *   An object that determines which editor to attach to a given field.
+   */
+  public function __construct(EditEntityFieldAccessCheckInterface $access_checker, EditorSelectorInterface $editor_selector) {
+    $this->accessChecker = $access_checker;
+    $this->editorSelector = $editor_selector;
+  }
+
+  /**
+   * Implements \Drupal\edit\EditorAttacherInterface::preprocessField().
+   */
+  public function preprocessField(&$variables) {
+    $element = $variables['element'];
+    $entity = $element['#object'];
+    $field_name = $element['#field_name'];
+    if (!$this->accessChecker->accessEditEntityField($entity, $field_name)) {
+      return;
+    }
+
+    $instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle());
+    if ($editor = $this->editorSelector->getEditor($element['#formatter'], $instance, $element['#items'])) {
+      // Attributes needed to make the element editable.
+      $variables['attributes']['data-edit-field-label'] = $instance['label'];
+      $variables['attributes']['data-edit-id'] = $entity->entityType() . ':' . $entity->id() . ':' . $field_name . ':' . $element['#language'] . ':' . $element['#view_mode'];
+      $variables['attributes']['aria-label'] = t('Entity @type @id, field @field', array('@type' => $entity->entityType(), '@id' => $entity->id(), '@field' => $instance['label']));
+      $variables['attributes']['class'][] = 'edit-field';
+      $variables['attributes']['class'][] = 'edit-allowed';
+      $variables['attributes']['class'][] = 'edit-type-' . $editor;
+
+      // Additional attributes for WYSIWYG editor integration.
+      if ($editor == 'direct-with-wysiwyg') {
+        $variables['attributes']['class'][] = 'edit-type-direct';
+        $format_id = $element['#items'][0]['format'];
+        $variables['attributes']['data-edit-text-format'] = $format_id;
+        $variables['attributes']['class'][] = $this->textFormatHasTransformationFilters($format_id) ? 'edit-text-with-transformation-filters' : 'edit-text-without-transformation-filters';
+      }
+    }

This looks like it's going to completely break render caching of entities when edit module is enabled. I discussed this with webchick at BADCamp and she mentioned that since there's a toggle for edit mode on or off, this could potentially just be added with that on.

#914382: Contextual links incompatible with render cache has been open since 2010 for a similar issue with contextual links, I have a plan to un-break contextual links which I don't think is typed up anywhere yet, but it's not good if edit module further breaks any opportunity to cache HTML beyond the page level given all the performance regressions we already have in 8.x.

Wim Leers’s picture

Status: Needs review » Needs work

After discussing potential solutions to #94 with catch, this is what we landed on:

This is the plan.
1) always add the edit metadata in edit_preprocess_field().
2) Through drupalSettings, let the JS know whether the current user has the "in-place editing" permission.
3) If the user has this permission, perform an AJAX request as soon as the onload event fires to check if the current user may edit any of the fields on the current page EXCEPT when the "#edit" fragment is set, then do that AJAX request ASAP.

The only alternative is to have some GET parameter that would indicate that edit mode is turned on, and in that case, disable caching so that we can add our stuff. But requires another page load, which is very bad for UX, especially on high-latency connections. In comparison, the above seems much better, also because it will result in the best server-side cache-hit ratio.

Working on a reroll.

yched’s picture

1) always add the edit metadata in edit_preprocess_field().

I'm still really worried about the perf impact of adding all the logic in edit_preprocess_field() on each field on each rendered entity for all users whenever edit.module is enabled (for the benefit of only a tiny fraction of the users) :-(.
Without a very fast and early killswitch, I don't see how perf impact can be acceptable.

I know we've agreed to discuss that in a followup, but I'm preparing myself to argue for fetching the Create metadata markup in a separate ajax request when the user actually hits the "edit" mode :-/... Less fluid, sure, but IMO we can't trade rendering time for 95% other cases...

catch’s picture

Yes that still concerns me as well, although less than not being able to cache any HTML at all.

I think it's two separate issues though - we'd still want the access checks handled separately too regardless most likely.

I've got absolutely no objection to people having to wait 300ms or so when they click the edit link, compared to adding time for every user whether they click the link or not.

Another option is adding that markup per-role, and splitting the cache on role (which edit module needs to handle, not the entity system at all, but that's less than ideal when all roles either have this on or off..

sun’s picture

Is there any reason we cannot unconditionally add all of those HTML attributes for all users on all displays, regardless of whether the edit mode is enabled?

Can't we perform the access checks and conditionally attach the required JS + settings in a separate step? I.e., essentially having the entire markup ready for front-end inline-editing consumption, but only providing the necessary JS infrastructure when needed/accessible?

The HTML attributes on their own surely don't seem to be a problem. The only other call seems to be the $this->editorSelector->getEditor() - not sure how fast or slow that is, and whether there might be ways to bake that metadata info into some existing to make it available faster?

David_Rothstein’s picture

Why would custom submit handlers make sense to share between a node/NID/edit form and an in-place edit submission?

First example that came to mind would be a module that wants to send e-mail saying "hey, some person just edited this node via the user interface". It doesn't matter which form was used to edit it, just that it was edited by a human.

Looking through an actual codebase, most custom submit handlers on the node form seem to be for revision-related modules (e.g., http://drupal.org/project/ers has a pretty complicated example). I haven't studied this in detail, but presumably the idea is that just like this patch makes the default Node module revisioning fall back on its default behavior when a single field is being edited, other modules that deal with revisioning would need to do something similar with whatever their default behavior is. That's why it makes sense to me to (at least) have the base form in the Field module (since we can much more reasonably and cleanly say "modules need to deal with this extra, separate form" if it lives in a central place like that, rather than in an optional module like Edit).

@David_Rothstein: if you remain concerned about punting the field label issue to a follow up, or anything else, please kick the status back.

I still have some of the concerns I mentioned above, but due to work/family things I probably won't be able to get back to this issue until next week. I trust that the good people of Drupal-land will figure out what to do with it in the meantime :)

catch’s picture

@sun:

Is there any reason we cannot unconditionally add all of those HTML attributes for all users on all displays, regardless of whether the edit mode is enabled?

That's page weight for anonymous users if we do that, for a feature they can't possibly use on 99.999% of sites. It's less of an issue than the server-side caching for me though.

Can't we perform the access checks and conditionally attach the required JS + settings in a separate step? I.e., essentially having the entire markup ready for front-end inline-editing consumption, but only providing the necessary JS infrastructure when needed/accessible?

That's mostly what #95 is, although yes it seems reasonable to not add any of the js for users with no access to this at all.

Wim Leers’s picture

I completed #95. Please review the interdiff.

I'm sure this code could still use some clean-up, but I didn't find any example of a D8 route-based page callback that processes POSTed data. Nevertheless, this proves that #95 works fine.

This includes @effulgentsia's work of #92.


#96:
- after having done #95, there is now barely any logic in preprocessField()
- in unscientific, yet many repeated "performance tests" (subtract two microtime() calls, the variance is great, but doing many tests indicated that the ballpark numbers were right), I noticed that calling the t() function (for the ARIA label) is the biggest cost in preprocessField(). I measured total duration for /node (front page) with 4 nodes (with several fields visible for each) and for the node/1 page. If I excluded the ARIA label, then the total time was around 1.2 ms, both for /node and /node/1. If I included the ARIA label, then the total time was around 2.5 ms for /node and around 1.8 ms for /node/1.
- we could in theory adjust the data available in $variables, this would obviate the need to call $entity->id(), and potentially even the need to call field_info_instance()
- If it turns out to be impossible or even just bad for performance reasons to keep all this metadata setting in preprocessField(): no problem! The only absolutely essential metadata is the "edit-field" class and the "data-edit-id". This would result in around 0.12 ms for /node and <0.1 ms for /node/1. This is surely fast enough! All other metadata would then be loaded through this same AJAX call.

#98: Completely agreed, and that's what this interdiff delivers :)

#99: RE: "submit handlers". What you say, sounds sensible. However, please take into account that this is not intended to continue to be powered by forms. We currently implement saving on top of Drupal.ajaxified forms because we have no other choice, but once we have a full JSON-LD API available for updating fields/entities, then we'll switch to using that. Once that is being used, I'm afraid that any consumer of that API will also not trigger the submit handlers. That's what effulgentsia also tried to say in #92.

#100:
- RE: "page weight for anon users": my last point above would address that. The overhead for anonymous users would drop to " edit-field" being added to the class attribute (11 characters per field, with gzip likely close to 0 bytes difference) and the "edit IDs" for each field (of the form <entity type>:<entity ID>:<field name>:<langcode>:<view mode>). Note that both of these would go away once we start using the RDFa-based annotation instead of this custom stuff. But then again… RDFa probably already adds more page weight anyway.
- RE: "no JS for users that don't have access to this". This is also incorporated :)

yched’s picture

@Wim Leers : thanks !

If it turns out to be impossible or even just bad for performance reasons to keep all this metadata setting in preprocessField(): no problem! The only absolutely essential metadata is the "edit-field" class and the "data-edit-id". This would result in around 0.12 ms for /node and <0.1 ms for /node/1. This is surely fast enough! All other metadata would then be loaded through this same AJAX call.

I'd think that whatever we can put in this "only if user has access to inline edit" ajax call is better off there, but I'm fine with it being a followup :-)

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -wysiwyg, -Accessibility, -Needs accessibility review, -in-place editing, -Spark

The last submitted patch, in_place_editing_for_fields-101.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +wysiwyg, +Accessibility, +Needs accessibility review, +in-place editing, +Spark
effulgentsia’s picture

#101 looks like a nice improvement, but we're not quite there yet.

I agree with #102. The problem isn't node/NID or /node pages: it's a node with 50-300 comments (theme('field') runs on every field of every comment) and Views (which can also run a lot of theme('field') calls).

I could see us choosing one of two approaches:
- Either reinstate the user_access('access in-place editing') kill switch in edit_preprocess_field(), and accept that entity render caching will need to be per-role (or per this boolean).
- Or, strip down EditorAttacher::preprocessField() to only adding edit-field and data-edit-id and making EditController::access() return all the interesting and expensive to compute information (like which editor to use) currently added by EditorAttacher::preprocessField().

Based on #97, seems like catch would prefer the latter, but I'm undecided currently. What do others think?

Wim Leers’s picture

Status: Needs review » Needs work

I'd go with option 2 then: it yields the best render cache hit ratio, lowest per-page processing overhead, most minimal HTML weight overhead possible and is overall the least contentious option (AFAICT). It doesn't matter much whether we just retrieve the access info or more metadata. This should be easy to implement.

I'll reroll.

yched’s picture

@Wim Leers: sounds cool. Thanks !

David_Rothstein’s picture

For what it's worth, I thought this was an interesting "review" of this issue (for those who haven't seen it yet): http://www.lullabot.com/articles/inline-editing-and-cost-leaky-abstractions

Wim Leers’s picture

I completed #106. Please review, catch/yched/effulgentsia.

EDIT: P.S.: while working on this, I discovered #1864944: ajaxPageState does not contain JS files that are in scopes other than 'header'.

eaton’s picture

For what it's worth, I thought this was an interesting "review" of this issue (for those who haven't seen it yet): http://www.lullabot.com/articles/inline-editing-and-cost-leaky-abstractions

Just to clarify, the article wasn't intended as a direct shot at Spark, rather it was inspired by the discussions around it. I think the inline WYSIWYG work going on here is very valuable, but I think we need to be very cautious about how it's actually deployed on shipping sites, and whether it's enabled by default for site-builders. ;-)

All of those questions are separate from the quality of the code, the value of the underlying work of creating a pipeline for client-side editing interfaces.

effulgentsia’s picture

This is just a reroll due to #1862656: Move field type modules out of Field API module, but in the process of rerolling, I lost the images (my text editor was corrupting the binary portion of the patch). If someone can add them back, that would be helpful, or I'll figure out how to do so at some point.

Status: Needs review » Needs work

The last submitted patch, in_place_editing_for_fields-111.patch, failed testing.

Dries’s picture

I'm not sure we need to worry about the re-use use case in this issue. Today, when you see a node and edit it in the overlay, you may not have a clear understanding about where the node gets displayed (a View, homepage, etc.). So Edit in Place may exacerbate that a little, but I think the benefits outweigh the risks.

As for the notification problem; I think we need to differentiate between 'autosave' and (manual) 'save'. Or put differently, we need to be able to differentiate between a programmatic save and an explicit save through a UI action. I think we just need to figure out what hook to add so developers can hook into the proper actions. I believe that can be tackled as a follow-up issue as well.

effulgentsia’s picture

Status: Needs work » Needs review
effulgentsia’s picture

This brings back the images, so this is the same as #109, just updated to reference text.module in its new location.

effulgentsia’s picture

Status: Needs review » Needs work

Since Dries is okay with us addressing David's two remaining concerns in follow ups, and that catch's concerns about entity render caching have been addressed, I think this is really close.

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -0,0 +1,146 @@
+  public function metadata() {
+    if (!isset($_POST['fields'])) {

You can declare Request $request in the signature, and then replace $_POST['fields'] with $request->request->get('fields').

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -0,0 +1,146 @@
+    $metadataGenerator = drupal_container()->get('edit.metadata.generator');

The controller is ContainerAware, so you can replace drupal_container() with $this->container

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -0,0 +1,146 @@
+      // Load the entity.
+      if (!$entity_type || !entity_get_info($entity_type)) {
+        throw new NotFoundHttpException();
+      }
+      $entity = entity_load($entity_type, $entity_id);
+      if (!$entity) {
+        throw new NotFoundHttpException();
+      }
+
+      // Validate the field name and language.
+      if (!$field_name || !($instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle()))) {
+        throw new NotFoundHttpException();
+      }
+      if (!$langcode || (field_valid_language($langcode) !== $langcode)) {
+        throw new NotFoundHttpException();
+      }
+
+      $metadata[$field] = $metadataGenerator->generate($entity, $instance, $langcode, $view_mode);

Do we want to throw exceptions here, or would it make sense to just omit $field from $metadata for invalid fields? Can the JS code handle such omissions? Or, should we add another key in $metadata[$field] for reporting that it wasn't found?

+++ b/core/modules/edit/lib/Drupal/edit/MetadataGenerator.php
@@ -0,0 +1,80 @@
+    $metadata = array(
+      'label' => $label,
+      'access' => $this->accessChecker->accessEditEntityField($entity, $field_name),
+      'editor' => $editor,
+      'aria' => t('Entity @type @id, field @field', array('@type' => $entity->entityType(), '@id' => $entity->id(), '@field' => $label)),
+    );
+    // Additional metadata for WYSIWYG editor integration.
+    if ($editor === 'direct-with-wysiwyg') {
+      $format_id = $items[0]['format'];
+      $metadata['format'] = $format_id;
+      $metadata['formatHasTransformations'] = $this->textFormatHasTransformationFilters($format_id);
+    }

Do we need to return all this metadata when 'access' is FALSE? Seems like unnecessary information disclosure. It's fine if we return it from this function, but we shouldn't send it all the way back to the browser. For performance though, it might be better to not even spend time figuring this stuff out if the user doesn't have access to edit it anyway.

eaton’s picture

I'm not sure we need to worry about the re-use use case in this issue. Today, when you see a node and edit it in the overlay, you may not have a clear understanding about where the node gets displayed (a View, homepage, etc.). So Edit in Place may exacerbate that a little, but I think the benefits outweigh the risks.

Just to clarify, the issue with that specific concern isn't simply that a user can't simultaneously see all of the places a piece of content appears. As you said, that's a problem with almost any editing interface! The problem is that Inline WYSIWYG works by breaking down the distinction between the content and the page itself, presenting the user with a direct manipulation interface. On large sites with lots of content re-use, though, the difference between "direct manipulation" and "a link to a screen where one edits a free-standing content object" is nontrivial. While the former may be flashier it makes unanticipated ripple effects much more likely because it communicates something untrue about the action a user is performing.

Again, this isn't a universal problem with Inline WYSIWYG, just one that appears on larger sites with more content re-use. It's also not the only issue (hiding of invisible metadata, the privileging of the editor's current device design, etc). Given the amount of work that's gone into the Inline WYSIWYG effort, I'm presuming that it will eventually make it in. My purpose isn't to scuttle that effort but to make sure there's a clear and deliberate dialog around the pros and cons, so that site builders understand when it's beneficial and when it will do more harm than good.

As for the notification problem; I think we need to differentiate between 'autosave' and (manual) 'save'. Or put differently, we need to be able to differentiate between a programmatic save and an explicit save through a UI action. I think we just need to figure out what hook to add so developers can hook into the proper actions. I believe that can be tackled as a follow-up issue as well.

N-thing this; those different actions mean different things, and the ability to distinguish between them will have a significant impact on publishing workflow solutions built in D8.

webchick’s picture

Have you tested the patch..? Note that we deliberately do not make inline editing the interface the method to create new content, nor do we do away with the full edit form itself for all the reasons you state (mental model, properties that are not represented visually, etc.). It should be sufficiently clear that inline editing is for "quick" edits to things you can see directly on the page. If it's not, let's get some specific, actionable things we can add/fix, either in this patch before commit or as a follow-up.

eaton’s picture

Have you tested the patch..? Note that we deliberately do not make inline editing the interface the method to create new content, nor do we do away with the full edit form itself for all the reasons you state (mental model, properties that are not represented visually, etc.).

I've absolutely tested the patch! :-) I've been watching it with great interest and working with demos for some time, though the deeper analysis around the concept of inline WYSIWYG editing was triggered in large part be recent presentations and articles from people in the Create.js community who are advocating that inline WYSIWYG be considered the default editing mode for many CMSs. The article I posted over at lullabot.com steered clear of many of the specific implementation details and attempted to focus on the pros and cons of the underlying UX concept.

It should be sufficiently clear that inline editing is for "quick" edits to things you can see directly on the page.

Possibly, but there are no cues separating the two kinds of operations from each other and no guidelines for site builders on which is more appropriate as a primary editing interface for users. That's part of the reason I've been looking into this -- trying to iron our some useful rules of thumb for those kinds of questions, rather than simply weighing in "for" or "against" on the patch itself. It seems pretty clear that Drupal (and possibly several other CMSs) are pushing forward with inline editing, and like FieldAPI vs. Node Hooks in D7, or Dedicated Table vs. Variable Set in D6, guidance will be useful.

If it's not, let's get some specific, actionable things we can add/fix, either in this patch before commit or as a follow-up.

If we're talking about potential points for confusion, there are several specific issues that I believe will result in ongoing user confusion. To be clear, I don't think these indicate any underlying problems with the concept of Inline WYSIWYG editing, they're just problematic issues with the particular UI conventions implemented in this patch.

  1. The interaction that triggers inline editing is ambiguous. All of the other primary toolbar options cause dropdown lists of secondary options to appear. Clicking the 'Edit' link in the toolbar, however, causes the behavior of the current page to shift into "edit mode." There's no indication that the link is different than any other links in the toolbar.
  2. There's nothing to distinguish the Edit link from... well, the Edit link. One alters the behavior of the page I'm viewing, and the other opens a full modal editing form. I'm not sure which link should trigger the inline behavior and which should trigger the traditional popup form, but they should at least be distinguished from each other in some way.
  3. Most inline WYSIWYG implementations make it difficult to edit invisible data, this one actually forces users to use the traditional editing interface to edit data that is visible: the post's author, the post's creation date, and the post's title. The title in particular is frustrating, as the's one of the largest, most obvious, most visible things to make a quick tweak to. I absolutely understand that this is a limitation of using FieldAPI as the key gateway between the inline editing and Drupal's data -- it's just an issue that's bound to create confusion and frustration.

In addition, the specific detail of how large text forms behave when they're edited undercuts the primary use case that you've described above -- spotting an error in the text, and making a quick change to it without having to "get your bearings" on the full edit form. See below:

In the screenshot above, I spotted the word "troo" and realized it should be "true." I entered inline editing mode, clicked the field, and in the popup window that had opened, had to hunt to find the typo anyways. On long articles, there would be a great deal of scrolling and searching to find the error in question. This particular issue means that for long text fields in particular, the benefits of Inline WYSIWYG are negligible.

I'm not sure what the best solutions to those particular issues are; I'd strongly suggest that Edit module remain disabled by default and remain a tool for site-builders to deploy selectively when it's appropriate. Ideally, it would be used on sites where the content types are simple enough that it could be the primary editing interface rather than a parallel one intended for "quick edits." The Plone team in particular implemented and shipped inline editing with the very same "quick tweaks on the front end" use case in mind, only to discover that their users generally wanted to make multiple edits in a go, across several fields.

eaton’s picture

(Apologies for the potentially offensive content in the demo article; it was one of the test nodes I had lying around already. I attempted to take a set of replacement screenshots, but file uploading broke on my d8 install and I haven't sorted out how to get it working again.)

Bojhan’s picture

It's great to see these reviews!

@eaton is largely correct, we do change the mental model here - because if you click "quick edit" on a teaser - you will get the full body text. Therefor your "edit" action does not represent, what you are seeing - breaking "true" WYSIWYG. I think Dries his stance on this is a bit too nuanced, we do break the mental model, it is however decided that this is a reasonable trade-off vs. confusion that you can only edit text in certain view modes. What this does mean, is that you ideally want to signal in some way that you are editing a view mode - and therfor, its no "true" WYSIWYG representation. I already discussed this at length with Wim Leers following David's concerns, and we figured its good followup at #1862784: Improve Edit module's labeling of "editable things".

1. Yup, I noted this in #52 as well. We might want to introduce a toggle, rather than menu button - this all depends on how the overall saving issue turns out.

2. Somehow we keep removing "Quick edit" as the trigger word for inline editing. Lets try and keep that.

3. Yup, I noted this in #48. I think the fact that you can't edit the Title would be a critical follow up, that you can't edit properties is also an issue but I'd say considerably less.

On "full view" the Body text should probally not create such significant visual disonanse. However to my understanding as we move WYSIWYG into core, we wouldn't need the [text formats] part below the body text - so all we have to do is find a place for (Edit Summary), and then we can pretty much keep it at a similair ux level as all the other fields.

I am not very keen on the idea of not enabling this by default. We should figure out ways to resolve the workflow issue, not just avoid it by disabling it by default. Sites with some kind of workflow is part of our core audience, not providing a great UX feature like this for them would be a mistake. It will be a challenge, but lets not upfront decide to disable without even having tried to tackle this problem - we are still in development mode after all.

I don't think any of @eaton his concerns are show stoppers, and it seems he made that very clear - these are issues, that should find good followup activity - but do not keep this from going RTBC.

Wim Leers’s picture

#116: I'll address those tomorrow. Other obligations today.

#119.1: I agree that it's weird that the "Edit" tab in the toolbar behaves very differently than the other tabs. This is an area that we need to improve in the future, based on usability testing.

#119.2: Again agreed; Bojhan's suggestion (to rename it back to "Quick Edit") in #121 could be a solution, but at the same time it's a relatively long string in the toolbar. I think the solution to this depends on what we do for #119.1.

#119.3: You describe three problems:

  1. inability to edit invisible data
  2. inability to edit certain visible data
  3. an annoying form-based editor in some cases

#119.PS: Edit's icons are broken on your site because you didn't apply the patch with the necessary "just apply the damn whitespace already" git flags :) See #1. (git strips whitespace even from binary files, thus causing the PNG files to break.)

Answers:

  1. this may be added in the future, there are several ways we can do this, but it's not part of the MVP.
  2. in the Drupal 7 version, this is supported, but requires extensive and ugly work-arounds and code duplication. In the Drupal 8 version, we specifically decided to not do that, precisely to avoid ugly, hacky and duplicated code. The solution is simply this: the node title, date and author should be converted into proper Entity Properties (currently still called "Fields"), they will then automatically become in-place editable. So, I could bluntly state this as: "it's not Edit module's responsibility". However, if it turns out near the end of the D8 dev cycle to be impossible/unreasonable/whatever to convert node title/date/author into Entity Properties, then we'll have to figure out the best way to make them work.
    tl;dr: Clearly we want these to be in-place editable. However, they are currently still hardcoded into the node form, which forces us to do nasty things. We don't want to do that, hence we wait until they are no longer hardcoded, then it will automatically start working.
  3. You get the ugly form-based editor for processed text fields in two cases: 1) when you don't have a WYSIWYG editor with Edit support installed; 2) when you're editing a teaser. The reasons for the former should be obvious, for the latter we must use the form-based editor because it is possible that it doesn't make sense to edit a summary of a longer piece of text for many potential reasons:
    • for fields that use the "Trimmed" text field formatter, we could potentially load the full text, replace the trimmed representation with that, but now we've forced the page to shift content around in a way that's different from what the end result would look like. Alternatively, we could make just the trimmed text itself editable, but that would imply also updating the full article. We'd have to make this super clear.
    • for fields that use the "Summary or trimmed" text field formatter, the summary may not be based on the full text at all, it may be completely different (when you're using the manually defined summary), so to ensure the user doesn't think he isn't actually editing the full text, we must show the form where this distinction is made obvious. That's what you're seeing.
    • we only have a "Summary or trimmed" text field formatter in core. If we had a "Summary" formatter (i.e. no automatic trimming), then we could make that directly editable (i.e. without the ugly form).
    • even when you're using "Summary or trimmed", we don't show the ability to edit the summary when you're editing the field on a page where it was not a trimmed version/summary. If you're updating the "full length" version of the field, it's possible that you'll also want to update the summary, so this is another area where it should be better. For the sake of simplicity of use, we don't support that right now, but that's of course the other side of this rather complex medallion.

    tl;dr: Automatic text trimming and custom summary handling within one field type and a lack of metadata make editing of "text fields with teasers" pretty complex.

#121: I'm not sure what you meant by "that you can't edit properties is also an issue"? I think you meant to say "invisible properties"?

Finally, I agree that it's good to enable Edit module by default in the Standard install profile, even if only during the development cycle. It'll trigger more people to give feedback, which will lead to better solutions. (I think this is the exact reasoning @effulgentsia has voiced earlier in this issue.)

Wim Leers’s picture

#116:

You can declare Request $request in the signature, and then replace $_POST['fields'] with $request->request->get('fields').

You have no idea how much I wish I'd known that! :) I had spent quite some time to figure out how to access Request $request, but I couldn't figure it out. I had no idea you could just automagically refer to it… Thanks, much cleaner now!

Do we want to throw exceptions here, or would it make sense to just omit $field from $metadata for invalid fields? Can the JS code handle such omissions? Or, should we add another key in $metadata[$field] for reporting that it wasn't found?

That is a most excellent question!
Clearly, not throwing an exception (404) here would make the entire application more robust in case there are invalid field names being requested. On the other hand, when would metadata for such invalid field names ever be requested? That would only be the case in two cases: 1) data corruption/stale render cache, 2) malevolent attackers (not that they would be able to retrieve truly useful information). If data corruption/stale caches occurs, then the real solution is to fix that, and seeing a 404 response in your browser is helpful in pointing in the right direction, plus if the data for one field is corrupted, then that for others is more likely to be corrupted too, so it's better to waste as few resources as possible. If malevolent attacks occur, then we don't need to be obedient and return as much data as possible.

So I believe that we should stick with the current approach here.

Do we need to return all this metadata when 'access' is FALSE? Seems like unnecessary information disclosure.

OMG! Of course you're absolutely right! Fixed, now returns only { access: false } when the user does not have access to edit a field.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +revisit before beta
FileSize
169.01 KB

Thanks. I think all commit blockers for this have been addressed, so back to RTBC. This just re-adds dependencies[] = edit to standard.info which mysteriously got dropped out of #101 and later.

I'd strongly suggest that Edit module remain disabled by default

I'm adding the "revisit before release" tag on this issue so we can evaluate eaton's concerns before shipping.

I'm leaving the "needs accessibility review" tag for now, in case anyone wants to do so before commit. After commit, we may want to move that tag to #1844220: Make in-place editing keyboard and aurally accessible and move that issue to the core queue.

jessebeach’s picture

The accessibility aspect of this module was given a preliminary pass in #1844220: Make in-place editing keyboard and aurally accessible. I fully acknowledge that this was a best-effort attempt and we will need to do follow-up work, as with any UI, to hone the experience. I'm tracking the a11y aspect of Edit in the meta issue for the Drupal authoring experience.

#1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG.

What we have in this patch is a solid base for testing and gathering feedback for improvements.

jessebeach’s picture

Issue summary: View changes

Link to new entity access improvement blocking issue; fix JS architecture link.

catch’s picture

+++ b/core/modules/edit/lib/Drupal/edit/Plugin/ProcessedTextEditorBase.phpundefined
@@ -0,0 +1,29 @@
+/**
+ * Base class for processed text editor plugins.
+ */
+abstract class ProcessedTextEditorBase extends PluginBase implements ProcessedTextEditorInterface {
+
+  /**
+   * Implements \Drupal\edit\Plugin\ProcessedTextEditorInterface::addJsSettings().
+   */
+  public function addJsSettings() {
+  }
+
+  /**
+   * Implements \Drupal\edit\Plugin\ProcessedTextEditorInterface::checkFormatCompatibility().
+   */
+  public function checkFormatCompatibility($format_id) {

What's the point of the base class if these functions are empty? If implementations have to fill them out, why aren't they abstract functions? If they don't, why are they on the interface at all?

Wim Leers’s picture

#126: I don't know, I'm not deeply familiar with conventions used by D8-style OOP. This was introduced by @effulgentsia in #54, whom is far more experienced in that area. A quick search indicates that e.g. Views' QueryPluginBase::query() and others do this as well.

If I understand you correctly, you're arguing that they should become

+  /**
+   * Implements \Drupal\edit\Plugin\ProcessedTextEditorInterface::addJsSettings().
+   */
+  public abstract function addJsSettings();
+
+  /**
+   * Implements \Drupal\edit\Plugin\ProcessedTextEditorInterface::checkFormatCompatibility().
+   */
+  public abstract function checkFormatCompatibility($format_id);

That looks sensible to me. However, note that not a single other class in Drupal 8 contains a public abstract function, and only a handful have abstract function. However, if I change it to either of those two, the following fatal error is triggered: "Can't inherit abstract function", which of course also makes sense (since implementing classes extend this abstract class). The reason this is an abstract class in the first place is because it's a base class that may not be used and that wants to extend PluginBase, so that any implementation class will also inherit PluginBase (so that we can have metadata inspection).
(If there are any errors in the above, I apologize, I'm very much familiar with C++ inheritance, but I'm no expert in PHP inheritance, and certainly not the relatively complex way Drupal is leveraging inheritance.)

It seems the above explanation also applies to Views' QueryPluginBase.

I don't like it either, but it's really a detail in the whole of in-place editing, and we're not the first to do it this way.

catch’s picture

That error's because the inheriting class doesn't override the methods, which goes back to whether they should be on the interface at all.

I still haven't managed to review the patch properly as a whole.

effulgentsia’s picture

They're both on the interface because consuming code (EditorSelector) calls those methods. It makes no sense for a particular text editor plugin to not implement checkFormatCompatibility(), so this removes it from the base class. Additional JS settings are optional though (at least in theory, though in practice, I think any real editor like Aloha or CKEditor will have some), so an empty implementation in the base class is (potentially) helpful. This documents that a bit better. The addJsSettings() method in general is still problematic (it should return settings, not drupal_add_js() them, and it should probably be per-format rather than a global thing run once that needs to add settings for all formats regardless of which are actually used by the page), but I'd like to postpone those cleanups until after #1809702: WYSIWYG: Add Aloha Editor to core or CKEditor lands in core.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks Wim Leers, tkoleary, frega, jessebeach, henribergius, effulgentsia, nod_, yched. This is some legendary work. Committed to 8.x! :) :)

tim.plunkett’s picture

tim.plunkett’s picture

This was awesome, really nice stuff.

First bug report!

#1872274: Editing breaks the toolbar

rudiedirkx’s picture

This definitely does not belong in core. Core should be perfect for extending (Features etc), not for optional stuff like this.

pwaterz’s picture

This looks awesome!

David_Rothstein’s picture

We are currently way way over thresholds, and have been for a while. So was this committed on purpose, or by mistake?

See discussion: #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw

quicksketch’s picture

I personally think this feature belongs in contrib also, but in the likely event the D8 ships with Edit module enabled by default, its UI and functionality should be reconciled with existing Drupal patterns. I've filed a followup issue with suggestions to address some of @eaton's complaints, which may solve several new and old issues with our editorial UI: #1874664: Introduce toolbar level "Edit" mode that shows all contextual links

pwaterz’s picture

@quicksketch I would have to disagree about this being in core. As you know, one of the biggest complaints about Drupal is that it is complicated and difficult to work with. I think having something like this out of the box, makes Drupal more user friendly. The Drupal Pro's know the best combination of modules to achieve their task, but for the beginner it's a daunting task trying to understand what Drupal does and then you add contrib to that. We inadvertently scare people away for the price of code cleanliness. Which is good to a certain point, there needs to be a balance. At some point decisions need to be made to make drupal more friendly out of the box. I think Dries gets that and is making the right decision including this in core. Well thats just my 2 cents :).

eaton’s picture

We inadvertently scare people away for the price of code cleanliness.

Just to clarify, none of the concerns I've mentioned about In-place WYSIWYG editing, or the implementation details referenced earlier in the thread, are about code quality. I'm not close enough to the architectural changes in D8 to pass judgement on the pros or cons of what's going on under the hood, just pointing out that the feature does not "make things easier" -- it makes a particular type of editing workflow possible. That editing workflow is sometimes useful, other times detrimental, based on the type of site and the complexity of the content.

As you know, one of the biggest complaints about Drupal is that it is complicated and difficult to work with.

Trust me, this is something I know all too well. ;-) I've been speaking and writing about that issue for years; training new users and site-builders to work with Drupal since before D5 was released; and building custom editorial interfaces for tiny and gigantic Drupal sites alike, based on extensive interviews with the teams who use them day in and day out. The flashy appeal of Inline/In-Place WYSIWYG has very little overlap with the issues that I see raised every day in those real-world scenarios.

The concerns I've put forward aren't about code quality versus UX. Mine, at least, are threefold:

  1. The feature as it stands complicates real-world editing work by presenting two functionally inconsistent but identically labeled editing modalities, both of which must be learned by users who create and edit content.
  2. The feature (again, as it currently stands) fails at the most basic test: making "click-and-type" editing of large text fields possible.
  3. If all of these problems are resolved, making Inline/In-Place editing the default undercuts Drupal's content modeling and reuse strengths by exposing a page-based, visual-design-centric editing mechanism in an era of increased reliance on diverse publishing channels

Edit module and the architectural changes that go with it are now part of core. As Bojhan pointed out, there are several critical improvements that still need to be made before it lives up to the promise of Inline/In-Place WYSIWYG Editing. The issues linked in Wim Leers' followup, The meta issue about Inline Editing, and the issues pointed out in Bojhan's comment are all important. At the very least, I would encourage the people who believe that Inline/In-Place WYSIWYG should be a default-enabled part of core to help work on those. I believe they are serious UX problems, ones that will turn the feature into a net UX negative unless they can be resolved.

I've weighed in on this a couple of times in this thread, and it's probably no longer the place to voice opinions about a patch that's already landed. I'll probably weigh in on any patch that seeks to enable Edit module by default, but other than that I just wanted to clarify, again, that the issue is not about balancing UX against code purity. All of us want to improve Drupal's UX for the people who use it.

effulgentsia’s picture

I'll probably weigh in on any patch that seeks to enable Edit module by default

The patch that landed, #129, does enable it by default in the Standard install profile. But let's give the follow up UX issues a chance to get resolved before debating whether it should stay that way. This issue is tagged "revisit before release", so we'll still have a chance to evaluate after the follow ups.

effulgentsia’s picture

The flashy appeal of Inline/In-Place WYSIWYG has very little overlap with the issues that I see raised every day in those real-world scenarios.

This isn't the issue to delve into the details of those unrelated real-world scenarios, but is there a link you can share that summarizes what the most important problems that you encounter are?

Wim Leers’s picture

#140: I agree with pretty much everything you say. We are working to address all of that.

#140.1: I agree the UX must be clarified/improved.
#140.2: Do you mean that when you click a field to edit it in-place in D8 HEAD, that you simply get a form and not a WYSIWYG editor? If so, that's in fact our next top priority: get a WYSIWYG editor in core. If not, please clarify!
#140.3: In-place editing is not designed to be the default. It does not even attempt to be the default: you cannot create new content through in-place editing! This is intended for "quick edits", it's not intended to be the main content editing experience.


I have the impression that the majority of your concerns boil down to the fact that users will (ab)use Edit to make the current page ("page-based") look "right" or "perfect" ("visual-design-centric") on the user's current browser/device; that the focus will shift away from Drupal's structured content background/tradition/focus/backbone.

  • RE: "visual-design-centric": If we'd only allow in-place "true WYSIWYG" text editing, I'd strongly agree with your concerns. But that's no the case. We also allow for in-place editing of less "freeform" data: any non-textual field is also in-place editable.
    So is your main fear that people will (attempt to) abuse the WYSIWYG text editor?

    In general, we're not doing "real direct manipulation" for anything: most fields are edited through a form, few are edited through contentEditable (but only typing text) and some are edited through WYSIWYG editors, but we've worked hard to ensure those WYSIWYG editors only allow the same things that the text format allows, so no crazy random HTML can be inserted. I think a large part of the solution here is to educate users better about HTML.

    Finally, it's also possible to have some kind of "show HTML structure" mode. We could decide to ship Drupal's WYSIWYG editor with that enabled by default (I personally would be in favor of that). E.g. Aloha's "Meta View" and CKEditor's "Show Blocks" (demo, click the second button from the bottom right.
  • RE: "page-based": I think there's significant room for improvement here; but I feel this can be mostly or wholly addressed by #1862784: Improve Edit module's labeling of "editable things"? Why would that not be sufficient?

In short: I really want to work with you and learn from you to make in-place editing as good as possible in as many scenarios as possible!

benjifisher’s picture

This patch relies on the new Toolbar API, which is still in flux. It is causing test failures for #1847198: Update the structure returned by hook_toolbar(): see #38 and #50 in that issue.

Since the patch in this issue has already been committed, I will amend the patch in the Toolbar issue. It should be #58 or #59, depending on whether I beat the testbot, which I asked to re-test #38.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Component: field system » edit.module

.

Wim Leers’s picture

Issue summary: View changes

Provide commit message.

xjm’s picture

Issue tags: -revisit before beta +revisit before release

We renamed the tag, but I think this is one we actually do need to look at before release, not necessarily before beta. There have been some UX improvements in the 18 months since it went in, and more are in progress, so we should look at this once those are in.

xjm’s picture

Issue tags: -revisit before release +revisit before release candidate
webchick’s picture

Issue tags: -revisit before release candidate

Hi there. Revisiting this issue before release candidate! :)

Going through each of the points by @eaton's diagram at https://www.drupal.org/files/inline-confusion-points.jpeg:

1) The "Edit" toggle in the toolbar was changed to just a pencil icon. The confusion between it and the Edit tab should therefore no longer be present. (agree that was super confusing before)
2) The Quick Edit toggle is now also physically located far to the right, far away from the elements on the toolbar that open menus, and it is an icon only, rather than an icon+text. Hopefully these visual separations will help users understand the it will exhibit different behaviour than the others.
3) Thanks to numerous improvements to entity/field API since the initial in-place editing patch went in, title, submitted by name/date are all in-place editable now. Even better, custom blocks and possibly even the site name/slogan can be before release, if the issue to convert those into blocks makes it in. So this one should be far less confusing (and definitely more consistent) as well.
4) The issue about long rich text fields and having to hunt/peck for text is still an issue, but only for teasers, as Wim points out in #122.

There are a variety of sub-issues that were spun off to deal with various aspects of the reviews here. All of them except for #1844220: Make in-place editing keyboard and aurally accessible (which I just bumped) are now marked closed (fixed).

Finally, the usability study proposed at https://groups.drupal.org/node/467458 is officially a-go for the end of June, and in-place editing is officially on our list of things to test (both desktop and mobile). That's as "revisit before release candidate" as we could possibly get. Any major and/or release-blocking bugs with this functionality that we find from that will be filed as their own separate bugs/tasks.

Therefore, I feel okay removing the tag on this one. Thanks all, for the detailed feedback.