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:
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
- #1844220: Make in-place editing keyboard and aurally accessible — also see the a11y subissues
- #1678002: Edit should provide a usable entity-level toolbar for saving fields
Major
- #1825474: Integrate the Edit module page actions into the D8 Toolbar (#1137920) (patch that does this is ready to go)
- Integrate with WYSIWYG - #1809702: WYSIWYG: Add Aloha Editor to core
- #1747916: Edit mode should be activated when clicking the Edit toolbar tab (was: "(selected tab) indicator does not change on View/Edit")
Normal
- Clean up
EditEntityFieldAccessCheck::accessEditEntityField()
once #1862750: Implement entity access API for nodes lands (#1696660: Add an entity access API for single entity access was just the framework, no entity type implements it yet).
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:
direct
property editor widget:
form
property editor widget:
direct-with-wysiwyg
property editor widget:
API changes
- There is a new type of plugin (to let any WYSIWYG editor become "the"
direct-with-wysiwyg
editor): seeProcessedTextEditorBase
. - Formatters must now indicate the editability of textual fields explicitly (because only those can be edited with the
direct
ordirect-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.
Comment | File | Size | Author |
---|---|---|---|
#129 | in_place_editing_for_fields-129.patch | 169.01 KB | effulgentsia |
#129 | interdiff.txt | 1.7 KB | effulgentsia |
#124 | in_place_editing_for_fields-124.patch | 169.01 KB | effulgentsia |
#123 | in_place_editing_for_fields-123.patch | 171.83 KB | Wim Leers |
#123 | in_place_editing_for_fields-aloha-integration-123-do-not-test.patch | 10.69 KB | Wim Leers |
Comments
Comment #1
Wim LeersApply 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.
Comment #2
aspilicious CreditAttribution: aspilicious commentedThats a lot of JS... is this alrdy reviewed? o_O
As I'm a js noob I'm just reviewing the php part.
On my custom forms I want to be able to configure this. Just like we have "hook_admin_paths"
globals? Can't we move this to the dic or some other new and fancy system. We are trying to remove these globals.
This belongs to the wysiwyg patch?
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.
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
Don't forget to sepearate @param and @return with a newline.
Mai summary should fit on 80 chars. A more detailed one can be written underneath the short summary.
Same but this is going to be a plugin soonish
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.
Comment #3
Wim Leers#2: Thanks for the review! Unfortunately almost everything (that is not related to doc style) you say is already answered by the issue summary:
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)".
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"".
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".
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? :)
Thanks, all fixed.
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.
Comment #4
Wim LeersNote 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.
Comment #5
mgiffordI 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?
Comment #6
Wim Leers#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:
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.
Comment #7
Wim Leersd.o fail.
Comment #8
Wim Leers#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):
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:
(First: *everything* in Create.js is a "widget", because every component is in fact a jQuery UI widget.)
'form'
(drupalFormWidget
) for form-based editing of structured, non-textual data (this can work for *any* Drupal field)'direct'
(drupalContentEditableWidget
) for plain contentEditable editing, subclasseseditWidget
'direct-with-wysiwyg'
(drupalAlohaWidget
) for WYSIWYG editing, subclassesalohaWidget
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:
The main directory contains
edit.js
(with Drupal behaviors and app initialization code),app.js
(the "Edit app" logic),theme.js
(with overridableDrupal.theme.something
functions),util.js
(a few utility functions), andbackbone.drupalform.js
(an implementation of Backbone.sync on top of Drupal's Form API andDrupal.ajax
0.Comment #9
Wim LeersPatch 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:
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.Comment #10
webchickSo 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.
Comment #11
seutje CreditAttribution: seutje commentedI only did a little visual review, scrolling through the patch
this seems a bit duplicate, could we unify this or something? only the urlFormat seems different.
here element is a regular DOM element.
but here it's a jQuery collection?
and here it's a regular DOM element again?
is there a reason these are soft-checks with type coercion?
should probably cache that length instead of re-evaluating every loop, might squeeze out a ms
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
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.
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
Comment #12
Wim LeersThanks so much for the review!
Code issues
element
name for different variables in SparkEditService.js: 1) in all cases, it's a jQuery-wrapped DOM element: a jQuery collection; 2) this part of the code needs further attention, it's been written by @bergie; 3) this implements code for VIE, and thus follows VIE coding standards, so no dollar sign to indicate this is a jQuery-wrapped DOM element, whereas it really is; 4) also in the Create.js codebase, you'll often seeelement
(e.g.editor.element
), in all those cases it's a jQuery collection. This is in fact the coding style jQuery itself uses for its jQuery UI Widget factory.js/createjs
directory. Why not alias to$
? Because all of them are jQuery UI widget subclasses.$.widget
doesn't read quite as well asjQuery.widget
. Why have them at all then? To declare dependencies..find('.edit-toolbar:not(:has(.edit-toolgroup.wysiwyg))')
-evilness: this is utter crap, and evil crap at that. It was necessary in Edit's early days due to race conditions. Now since the move to Create.js, this is no longer necessary. Gone :) http://drupalcode.org/project/edit.git/commit/a07431cStructural questions
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?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:
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).Comment #13
swentel CreditAttribution: swentel commentedGave 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.
This file doesn't exist anymore.
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.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedLooks 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.
Comment #15
yched CreditAttribution: yched commentedWow, lots of code :-)
I can't really say I reviewed this, but I read through the field API related code.
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()
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.
Should be named $field
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 ?
Why not entity_load() ?
Isn't field_view_value() more like what you want ?
I think this needs to be updated after #1812866: Rebuild the server side AJAX API
$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 ?
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedYay for all the recent progress, especially related to Create.js integration. Here's a review of the PHP code:
Should be package = Core
Is it really fields *and* entities? Or is it more like "functionality for entity fields" or even just "functionality for fields".
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.
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.
We have content negotiation in D8 now, so this is no longer needed (and ajax_deliver() no longer exists).
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?
In D8, all 3 of these examples will likely be language-dependent. Why is whether or not we display the #title conditional on that?
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.
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.
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'].
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.
Comment #16.0
Wim LeersUpdate. Create.js refactoring is done. Toolbar integration is ready. a11y plan exists.
Comment #16.1
Wim LeersDrop Drupal.ajax todo, it's not essential, it's just a nice to have.
Comment #16.2
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #16.3
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #17
Wim LeersThanks 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:
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:
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" andTextPlainFormatter
, which will always use "direct"), but it does not work with "advanced" text formatters that trim the text or show a summary (TextSummaryOrTrimmedFormatter
andTextTrimmedFormatter
).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.
I have no idea. Fixed — thanks :)
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-applycheck_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 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.See above.
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.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/27b118fHeh. Yay.
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.
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 otherModuleNameController
, 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?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.
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 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 (seeentity_form_submit_build_entity()
andEntityFormController::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.
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?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.
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 inviewElement
, 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.
Comment #18
Wim LeersComment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedHaven'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:
This is *not* compatible with the GPL. You will need replacement images and remove the current images from drupal.org repositories.
Comment #19.0
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdated issue summary.
Comment #20
Wim Leers#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.
Comment #21
yched CreditAttribution: yched commentedre: 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 ?
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...
Comment #22
Wim LeersRE: 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:
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?
Comment #23
sunI didn't have time to review the whole thing. For now, just these trivial things:
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)
Any references to "Spark" should be removed from the core implementation.
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?Comment #24
yched CreditAttribution: yched commentedSure, 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 ?
Comment #25
Wim Leers#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:
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.
Comment #26
Wim LeersUpdate.
#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 morehook_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 frommissing-api.inc
toedit.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:
Comment #27
Wim LeersVisual changes introduced by the patch in #26:
- D8 toolbar integration: before — after (source: #1825474-28: Integrate the Edit module page actions into the D8 Toolbar (#1137920))
- Close icon: before — after
- Throbber: before — after
Comment #27.0
Wim LeersClarify that the D8 toolbar integration patch is ready.
Comment #27.1
Wim LeersUpdate to reflect current state of Entity Access API; rename "type" to "editor".
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedShould toolbar also be added as a dependency? Or is the idea to allow contrib modules to integrate Edit module some other way?
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.
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.
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?
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.
Can these 3 lines be replaced by field_view_field()?
That's the default, so why have it here?
Comment #30
nod_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.
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.
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).
Edit/Our stuff:
1. formwidget.js for things like body/taxonomy terms
2. "simple" contenteditable
3. Aloha
There are a few ajax commands that are created.
(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:
Some remarks:
formUpdated
event is crap, it gets in the way.I might have missed a few things, hopefully that helps out for the other people wanting to review this JS :)
Actionable feedback.
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.Comment #31
Wim LeersI'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:
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.
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).
Comment #31.0
Wim LeersHigh-level JS explanation.
Comment #31.1
Wim LeersLast 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).
Comment #31.2
Wim LeersMinor improvements.
Comment #32
Wim LeersReroll. Changes:
DrupalUnitTestBase
. Initially, they were running on top ofWebTestbase
and they took 36–37 seconds. After switching to usingDrupalUnitTestBase
, 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.includes/(form|pages).inc
toedit.(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:
Comment #34
Wim LeersOops. Some test files were missing. Sorry about that. These should pass.
Comment #36
Wim Leers#34: in_place_editing_for_fields-34.patch queued for re-testing.
Comment #37
Wim Leers.
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedJust 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.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedDoes 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)
Comment #40
webchickWell, 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.
Comment #41
webchick#1849526: Add VIE and Create.js libraries to core (upstream fixes) just got committed, so this will need a re-roll.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedIf 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.
Comment #43
effulgentsia CreditAttribution: effulgentsia commentedThis is just #34 with #1849526: Add VIE and Create.js libraries to core (upstream fixes) reverse applied.
Comment #44
yched CreditAttribution: yched commentedhm, 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.
Comment #45
jessebeach CreditAttribution: jessebeach commentedWell, 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.
Comment #46
jessebeach CreditAttribution: jessebeach commentedThis 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.
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedAs 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.
Comment #48
Bojhan CreditAttribution: Bojhan commentedMy 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?
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.
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:
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 :)
Comment #49
henribergius CreditAttribution: henribergius commentedThis 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...
Comment #50
Bojhan CreditAttribution: Bojhan commentedWe 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).
Comment #51
Wim Leers#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.
Comment #52
Bojhan CreditAttribution: Bojhan commentedThanks! 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.
Comment #53
eigentor CreditAttribution: eigentor commentedHaving 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.
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedAlrighty. 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).
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedCome 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.
Comment #56
yched CreditAttribution: yched commentedNew 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)Makes sense, but I'm a bit surprised that there is no specific "use inline editing" permission ?
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedI 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?
Comment #58
Bojhan CreditAttribution: Bojhan commented@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).
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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?
Comment #60
Bojhan CreditAttribution: Bojhan commented@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).
Comment #60.0
Bojhan CreditAttribution: Bojhan commentedLink 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.
Comment #60.1
jessebeach CreditAttribution: jessebeach commentedadded 1747916
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedThis implements #56 plus a typo fix.
I wouldn't be so quick to dismiss inline editing entirely for sites where content is reused. I came across this quote recently:
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?
Comment #63
Bojhan CreditAttribution: Bojhan commented@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.
Comment #64
Wim LeersThe 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:
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?Implements \Drupal\…
, notImplements Drupal\…
(even though , because you changed this in several locations. I also changed in the spots you missed.EditorSelectorInterface
, along with an overridableEditorSelector
, to allow contrib to add more editors. Excellent :)Form
directory to define its form in classes? (Except forEntity
, 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?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.EditFieldForm::simplify()
a bit more. And some other small improvements.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… :)
Comment #65
effulgentsia CreditAttribution: effulgentsia commentedThere'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.
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.
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.
Comment #66
Wim LeersThanks 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?
Comment #67
effulgentsia CreditAttribution: effulgentsia commentedThanks for the link in #66. I hadn't read that section before. Here's some bullet points from there that make exceeding 80 ok.
But here's one that some of my cleanup might have violated:
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 :)
Comment #68
jessebeach CreditAttribution: jessebeach commentedI 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
Comment #69
Wim Leers#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 :)
Comment #70
effulgentsia CreditAttribution: effulgentsia commentedFor 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.
Comment #71
David_Rothstein CreditAttribution: David_Rothstein commentedThat 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:
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.
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).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.)
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.)
Comment #72
jessebeach CreditAttribution: jessebeach commentedDavid_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.
Comment #73
Bojhan CreditAttribution: Bojhan commented@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.
Comment #74
Wim Leers#71: Jesse has addressed points 3 and 4 in #72. Bojhan has addressed your second point in #73. I"ll address the first point.
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:
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. :)
Comment #75
Wim LeersDoh. I forgot to pull, so #74 excludes #68 :(. Rerolled. The interdiff for #74 -> #75 is identical to the one for #68. Stupid stupid stupid!
Comment #76
klonosRelated 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.
Comment #77
jessebeach CreditAttribution: jessebeach commentedJust 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
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedI'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.
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).
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?
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedOne 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.
Comment #80
moshe weitzman CreditAttribution: moshe weitzman commentedFWIW, 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.
Comment #81
sunI'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.)
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedWell, 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 invokesetNewRevision()
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.Comment #83
Bojhan CreditAttribution: Bojhan commentedIt 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.
Comment #84
webchickI 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.
Comment #85
webchickAnd 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.
Comment #86
Bojhan CreditAttribution: Bojhan commented@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.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?)
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.
Comment #88
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch is based off #78 and fixes the overlay problem via the method I mentioned above.
Comment #89
Wim LeersSo 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.
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 :)
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.
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()
.Note that
prepareEntity()
lives inEntityFormController
(well, there it is a no-op) and is fully implemented inNodeFormController
. However, Edit doesn't useEntityFormController
at all; it builds the forms it needs throughfield_attach_form()
, because all it really cares about is the field's widget. Hence it is (AFAICT) impossible to useprepareEntity()
.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 byprepareEntity()
), 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 thenViewFormControllerBase::prepareEntity()
appears to somewhat abuse it to also deny access in certain cases (in theory they should overrideEntityFormControllerInterface::build()
, but then they'd have to re-implementEntityFormController::build()
andEntityFormController::init()
, hence it's not really abuse).In conclusion, the only things we could possibly want from
NodeFormController::prepareEntity()
are these: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.
Comment #90
Wim LeersI 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.
Comment #90.0
Wim LeersThe two known bugs have been fixed :)
Comment #91
Wim LeersI already created a follow-issue for the labeling: #1862784: Improve Edit module's labeling of "editable things", also listed in #1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedIf 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.
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.
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)?
That's silly. This patch fixes that.
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'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.
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.
Comment #93
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #94
catchThis 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.
Comment #95
Wim LeersAfter discussing potential solutions to #94 with catch, this is what we landed on:
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.
Comment #96
yched CreditAttribution: yched commentedI'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...
Comment #97
catchYes 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..
Comment #98
sunIs 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?Comment #99
David_Rothstein CreditAttribution: David_Rothstein commentedFirst 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).
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 :)
Comment #100
catch@sun:
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.
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.
Comment #101
Wim LeersI 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.ajax
ified 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 :)
Comment #102
yched CreditAttribution: yched commented@Wim Leers : thanks !
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 :-)
Comment #104
effulgentsia CreditAttribution: effulgentsia commented#101: in_place_editing_for_fields-101.patch queued for re-testing.
Comment #105
effulgentsia CreditAttribution: effulgentsia commented#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
anddata-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?
Comment #106
Wim LeersI'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.
Comment #107
yched CreditAttribution: yched commented@Wim Leers: sounds cool. Thanks !
Comment #108
David_Rothstein CreditAttribution: David_Rothstein commentedFor 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
Comment #109
Wim LeersI 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'.
Comment #110
eaton CreditAttribution: eaton commentedJust 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.
Comment #111
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #113
Dries CreditAttribution: Dries commentedI'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.
Comment #114
effulgentsia CreditAttribution: effulgentsia commented#111: in_place_editing_for_fields-111.patch queued for re-testing.
Comment #115
effulgentsia CreditAttribution: effulgentsia commentedThis brings back the images, so this is the same as #109, just updated to reference text.module in its new location.
Comment #116
effulgentsia CreditAttribution: effulgentsia commentedSince 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.
You can declare
Request $request
in the signature, and then replace $_POST['fields'] with$request->request->get('fields')
.The controller is ContainerAware, so you can replace
drupal_container()
with$this->container
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?
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.
Comment #117
eaton CreditAttribution: eaton commentedJust 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.
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.
Comment #118
webchickHave 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.
Comment #119
eaton CreditAttribution: eaton commentedI'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.
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 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.
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.
Comment #120
eaton CreditAttribution: eaton commented(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.)
Comment #121
Bojhan CreditAttribution: Bojhan commentedIt'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.
Comment #122
Wim Leers#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:
#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:
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.
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.)
Comment #123
Wim Leers#116:
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!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.
OMG! Of course you're absolutely right! Fixed, now returns only
{ access: false }
when the user does not have access to edit a field.Comment #124
effulgentsia CreditAttribution: effulgentsia commentedThanks. 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'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.
Comment #125
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #125.0
jessebeach CreditAttribution: jessebeach commentedLink to new entity access improvement blocking issue; fix JS architecture link.
Comment #126
catchWhat'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?
Comment #127
Wim Leers#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
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 haveabstract 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 extendPluginBase
, so that any implementation class will also inheritPluginBase
(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.
Comment #128
catchThat 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.
Comment #129
effulgentsia CreditAttribution: effulgentsia commentedThey'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.
Comment #130
Dries CreditAttribution: Dries commentedThanks Wim Leers, tkoleary, frega, jessebeach, henribergius, effulgentsia, nod_, yched. This is some legendary work. Committed to 8.x! :) :)
Comment #131
tim.plunkettQuick follow-up: #1872260: Edit and JSON-LD module are missing version strings
Comment #132
tim.plunkettThis was awesome, really nice stuff.
First bug report!
#1872274: Editing breaks the toolbar
Comment #133
Wim Leers#132: I'm not sure if that should make me happy or sad :) In any case: I replied already some time ago.
Comment #134
rudiedirkx CreditAttribution: rudiedirkx commentedThis definitely does not belong in core. Core should be perfect for extending (Features etc), not for optional stuff like this.
Comment #135
pwaterz CreditAttribution: pwaterz commentedThis looks awesome!
Comment #136
David_Rothstein CreditAttribution: David_Rothstein commentedWe 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
Comment #137
quicksketchI 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
Comment #138
Wim LeersMore follow-up issues:
Comment #139
pwaterz CreditAttribution: pwaterz commented@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 :).
Comment #140
eaton CreditAttribution: eaton commentedJust 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.
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:
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.
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
Comment #142
effulgentsia CreditAttribution: effulgentsia commentedThis 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?
Comment #143
Wim Leers#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.
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.
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!
Comment #144
benjifisherThis 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.
Comment #146
Wim Leers.
Comment #146.0
Wim LeersProvide commit message.
Comment #147
xjmWe 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.
Comment #148
xjmComment #149
webchickHi 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.