[Regression] Quick editing a body field on Standard does not work

:(

This happened despite it having been manually tested in that issue.

I hope somebody with deep knowledge of that issue will take this on.

Was just testing. Noticed that the functionality still works on the home page view which renders the full node as a teaser. I think the quick edit has some JS that might be relying on the structure of divs. Maybe it just needs a tweak.

I confirmed it is the body field. Other fields on the node work. Here is the markup. What am I missing?

So you would not release Drupal with that bug?

Well, feature-wise quickedit is somewhat useless if you can't edit the body text.

Confirming that it is also broken in Stark.

Got it. Hiding the label breaks it. Quick edit, rightly or wrongly, is relying on the structure of the field__item wrapper div inside the main field div. With no label there is no need for that extra div so it was removed. If you look at the field template you can see the ifs.

So I thought perhaps we have to have the field__item div inside the main one for this to work, but maybe not. I hid the field for an integer field and the quickedit still works. So it looks like it is smart enough to know if there is no markup inside the field, but not when it is text with summary of has the paragraph tags or something.

+++ b/core/modules/editor/js/editor.formattedTextEditor.js
@@ -63,7 +63,13 @@
-      this.$textElement = this.$el.find('.field__item').eq(0);
+      var $fieldItems = this.$el.find('.field__item');
+      if ($fieldItems.length) {
+        this.$textElement = this.$textElement = $fieldItems.eq(0);
+      }
+      else {
+        this.$textElement = this.$textElement = this.$el;
+      }

Ah ok the generic editor. So previously it only looks to see if field__item exist and grabs the first element. This makes it check to see if it exists and if not use what might be considered the parent.

Why the double equals?

this.$el

Shouldn't this also have eq()? In both cases you are grabbing whatever is inside.

Ok, #12 removed those equals.

Had something alike - but also changed the class to use .quickedit-field instead of field__item - I think that will be more robust.
Then themers can eventually even completely remove the field__item class as .quickedit-field is added by quickedit.js

- edit - crosspost all the way :)

I approve of moving it to a quickedit class. That would have to maybe be refactored since you are now moving it to the parent div. It might try to edit that div inside as well.

+++ b/core/modules/editor/js/editor.formattedTextEditor.js
@@ -63,7 +63,13 @@
+      var $fieldItems = this.$el.find('.quickedit-field');
+      if ($fieldItems.length) {
+        this.$textElement = this.$textElement = $fieldItems.eq(0);

This looks like it would try to edit the label since it comes before the field__item div.

+++ b/core/modules/editor/js/editor.formattedTextEditor.js
@@ -63,7 +63,13 @@
+        this.$textElement = this.$textElement = $fieldItems.eq(0);
...
+        this.$textElement = this.$textElement = this.$el;

I seem to be drunken ... everything is double ;) this.$textElement = this.$textElement

@dawwehner, yes alex fixed that in 12. I think swentel just copied what was in 10.

+++ b/core/modules/editor/js/editor.formattedTextEditor.js
@@ -63,7 +63,13 @@
+        this.$textElement = this.$el;

Yeah, I checked. This grabs the label along with it.

edit: With the change to the quickedit class. I didn't grab that line in dreditor.

I agree that's fix the critical issue quick and then edit the code for further improvements in #2559955: Ensure that Quick Edit relies less on the structure of the HTML

Did some manual testing ...

This does not work with multi-value fields. I assume that was actually broken by the field markup changes as well. It looks to be broken in head.

+++ b/core/modules/editor/js/editor.formattedTextEditor.js
@@ -63,7 +63,13 @@
-      this.$textElement = this.$el.find('.field__item').eq(0);

This won't fix it, because there are multiple divs inside el that have .field__item. They are not differentiated.

If this wasn't considered critical before, it definitely is now because the current problem destroys data. If you try to edit a multivalue field it puts the value of the first one into all the other ones, and you won't know until you hit save.

Here is a gif. http://g.recordit.co/jWijCilb9s.gif

I checked the code from before the field markup issue was committed, this would still be a problem. Even with the field-items wrapper it doesn't bother to look for it and just grabs the first item. This may have been broken for a while then. Does anyone recollect the last time they know for certain it worked?

+1

I have been following this issue since this morning (IST). I just tested this with current codebase without applying patch and quickedit seems to be working with body field. i tested with single valued field.

http://www.gfycat.com/PlainImmaterialEidolonhelvum

EDIT
At morning it was not working, with patch it was working.

Also working with multivalued body field http://www.gfycat.com/NervousWeepyGreatwhiteshark Tested with new content type.

P.S. gif quality is a bit poor

What generates that popup? When I edit a field a don't get the editor as a popup window, it is set in the page.

I checked with a fresh install with and without the patch. That popup happens with a "Text (formatted, long, with summary)" field, which the body field is, but it doesnt happen for me on the body field. It only happens on a field I add. Also, I don't think we are suppose to get that popup anyway. I think the edit in page with the wysiwyg toolbar in the hovered box is the desired result.

Looking at the markup being produced something isn't right in the logic for multiples and with or without label. Without a label the inner div ends up being produced but mocks all the properties of the parent, which doesn't seem right. I'm not sure though why the body field behaves differently.

David:

I can't duplicate the behavior you showed in your gif. I tried create multiple values, both with Formatted Long Text With Summary and Plain Text. I also tried with the label on and with the label off. Quick edit worked fine with all of those.

I tested the patches in #20 and #22. Both seemed to work fine.

Could you please list the exact you used to create the fields that exhibited this problem? The field settings as well as the field display settings?

If there is an existing problem from before this patch that deals with some type of multiple-value fields, then let's handle that in a separate issue.

So it was happening consistently for me with the body field of both of the default content types, Article and Basic, even after re-installing a couple times. I just did a git pull and re-installed again, and I can't reproduce it. I was only a couple of commits behind, and see nothing that would have had an affect, so maybe I'm taking crazy pills.

I'm intentionally trying to break the patch in #22, but nothing directly related to this so far. I did discover that if you turn off ckeditor quickedit seems broken.

...every once in a while it just does something weird. Something here is fragile.

Committed and pushed to 8.0.x, thanks! Although it's frustrating that this keeps happening due to unrelated patches. :( Something to include in our pre-RC checklist for sure, since we can't automated test for it.