Improve the UX of Quick Editing single-valued image fields

I tried to find the corresponding module, but can't find it. Can you find it, Lewis? Or do you perhaps know the creator of that video?

Amazing module!! it would become so easy to change images if it published :)

This is really nice. Would be good to get a similar function in for edit forms - for multivalue uploads especially drag and drop is a massive timesaver. https://www.drupal.org/project/dragndrop_upload is pretty good on D7

I have pushed the code to the sandbox project https://www.drupal.org/sandbox/avitslv/2413017 :)
And I have created a PA issue here https://www.drupal.org/node/2451563

For now it works with D7 and using only image formatter.

Would be nice to see this in a prototype.

#8++

I had no idea this issue existed, but I pushed up an experimental module to Github recently that has a similar feature set for Drupal 8/Quickedit: https://github.com/mortenson/quickedit_image

Here's how the basic flow works.

That looks great! Would love to get that into core :)

@wim-leers Thanks! I should clean up the documentation and add tests before pulling any patches - but would this be appropriate as its own module (i.e. as it is now), or should it live in Quickedit?

It should live in image.module. It'd just be another InPlaceEditor plugin. See the structure:

1. default: =\Drupal\quickedit\Plugin\InPlaceEditor\FormEditor
2. editor.module provides \Drupal\editor\Plugin\InPlaceEditor\Editor
3. THIS ISSUE: image.module provides \Drupal\image\Plugin\InPlaceEditor\Image

i.e. it's for image fields, whose Field Widget is provided by \Drupal\image\Plugin\Field\FieldWidget\ImageWidget, so it's also up to that module to provide a specialized in-place editor.

Per #12, this obviously already has design! Would be so great to have this in 8.2 :)

Embedded the GIF in #12 in the IS. The IS will need to be updated further though, to reflect the new direction.

And #12 already exists (at https://github.com/mortenson/quickedit_image), but this would definitely still need tests.

Fix to the correct media tag.

Working on a patch without tests to get the issue started, since it seems like there's a lot of interest.

Awesome! :)

Here's a first-shot patch, I can work on tests too but it seems like other people were interested in taking a crack at that. If no one claims that task by tomorrow I can start writing something.

Yay!!

<3 samuel.mortenson <3

I'll review tomorrow.

This will definitely need review from a JS maintainer like @nod_.

In the mean time, here's my high-level feedback. There is room for making things more robust, and complying with all the nitpicky things that core complies with, but overall, this already looks great!

1. +++ b/core/modules/image/image.routing.yml
   @@ -71,3 +71,29 @@ image.effect_edit_form:
   +image.upload:
   +  path: '/quickedit/image/upload/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
   ...
   +image.info:
   +  path: '/quickedit/image/info/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
   

   I think it'd make sense to merge these in a single /quickedit/image/{entity_type}/… route, with GET used for reading the info, and POST for changing it.

2. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,351 @@
   +      // the Quickedit Field Form.
   

   s/Quickedit/Quick Edit/

3. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,351 @@
   +          var $dropzone = this.renderDropzone('upload', Drupal.t('Drag file here or click to upload'));
   

   Where is this element (and its event handlers) being removed?

4. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,351 @@
   +            var $input = $('<input type="file">').trigger('click');
   ...
   +      var $toolbar = $('.quickedit-image-field-info');
   

   These global selectors make me nervous. We should do scoped searching, i.e. within the this.$el. In other words, this should use this.$el.find(SELECTOR) instead. That'd remove all risk of potentially changing other elements (outside of a Quick Edit context) that happen to use the same classes.

5. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,351 @@
   +    ajax: function (type, url, data, callback) {
   

   Why provide a new utility instead of using Drupal.ajax? I'm sure you have a good reason, but let's document that in the docblock for this function then.

6. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,351 @@
   +      var $dropzone = this.$el.find('.quickedit-image-dropzone');
   

   So, like this :)

7. +++ b/core/modules/image/src/Controller/QuickeditImageController.php
   @@ -0,0 +1,208 @@
   +   *   The Ajax response.
   

   s/Ajax/JSON/

8. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -22,6 +22,9 @@
   + *   quickedit = {
   + *     "editor" = "image"
   
   +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
   @@ -23,6 +23,9 @@
   + *   quickedit = {
   + *     "editor" = "image"
   

   Since this is modifying metadata, you'll need to provide an empty update hook in image.install, to clear caches, to ensure that existing sites start using this new default in-place editor :)

9. +++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
   @@ -0,0 +1,42 @@
   +    if ($field_definition->getFieldStorageDefinition()->getCardinality() == 1
   +      && $field_definition->getType() == 'image') {
   

   Let's use strict equality here.

10. +++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
    @@ -0,0 +1,42 @@
    +      return TRUE;
    +    }
    +    return FALSE;
    

    Also, you can just return this condition. So rather than if (CONDITION==TRUE) { return TRUE} return FALSE, you can just do return CONDITION

#26.1 My only hesitation with this is that you would be GET'ing field settings, and POST'ing the new file upload, so from an API perspective it's a bit confusing. If you'd like, I could try to come up with a generic object that could be passed both ways, containing the file and its metadata (alt/title/etc).

#26.2 Done, I make this mistake all the time.

#26.3 I believe that Drupal.quickedit.EditorView.save() replaces the contents of the entire View with the new field, which would remove the dropzone element. That said, event handlers are not explicitly removed. Should I be doing this before Drupal.quickedit.EditorView.save() is called?

#26.4 Done.

#26.5 Drupal.ajax is, for the most part, intended to be used in correlation with the Form API. Since this editor doesn't use the Form API and the routes return JSON objects that don't include things like Drupal.AjaxCommands, Drupal.ajax isn't the best fit. The reason I architected the editor this way is so that, at some point, the routes could be made more generic and used by non-Drupal/Quickedit specific components. An example of this is CKEditor, which supports drag+drop file uploads. Drupal 8 doesn't allow this yet in its WYSIWYG implementation, but it could easily be added in a follow up issue, given the logic being worked on here. Tying these routes to the Form API and Drupal's AJAX system would make that integration more difficult.

#26.6 Noted!

#26.7 Done.

#26.8 Done, I only added an update to image.install since responsive_image is dependent on image, and wouldn't need its own cache flush.

#26.9/#26.10 Done.

#26.1: having looked at the code closer, you're absolutely right. Never mind me :)

#26.3: I'd have to dive deeper into the code to make a firm recommendation. But I think it'd be at least great to document where these are being removed.

#26.5: Great! Can you move that explanation into the code as a comment, so that future readers/maintainers of the code know this too? :)

#26.8: Yep, perfect!

Added clearer documentation per #29.

Ran into case insensitivity issues with Git, re-created #30 with the correct file name.

Added library override in the stable theme, which should pass tests.

Edit: Some permission changes got thrown into the patch by accident, but as we still need to write tests for this I'm not too worried. Please ignore this when reviewing the patch. Sorry!

Looking great, hope we can get this in!

Looking great, hope we can get this in!

Does this mean we already have usability sign-off?

I guess I could have stated it more official :P. Yes!

Alright, in excitement, here's another review, this time with lots of nitpicks!

The key thing that we still need, is tests. You could quite easily test the two new routes using classic tests. But we should also test the JS in at least a superficial way (requiring perfect test coverage for that is unreasonable, since Quick Edit itself has zero JS test coverage, because JS tests didn't exist yet back then). You can find a bunch of examples in core by searching for JavascriptTestBase.

1. --- /dev/null
   +++ b/core/modules/image/css/editors/image.css
   

   This needs to be split in functional styles and theme styles.

   And then the theme styles should be moved into Classy perhaps? Not sure about that, but I'm certain about the splitting.

2. +++ b/core/modules/image/css/editors/image.css
   @@ -0,0 +1,115 @@
   +  display: flex;
   

   Flexbox! First time we are getting that into core!

3. +++ b/core/modules/image/css/editors/image.css
   @@ -0,0 +1,115 @@
   +.quickedit-image-dropzone.hover {
   

   Why a hover class? Why not the pseudoclass?

4. +++ b/core/modules/image/css/editors/image.css
   @@ -0,0 +1,115 @@
   +.quickedit-image-dropzone * {
   

   Woah. That's a scary selector. Wouldn't be using > * be better?

5. +++ b/core/modules/image/image.install
   @@ -61,3 +61,10 @@ function image_requirements($phase) {
   +function image_update_8001() {
   

   8201, since this is for 8.2.x

6. +++ b/core/modules/image/images/error.svg
   @@ -0,0 +1,4 @@
   \ No newline at end of file
   

   Let's add a newline to satisfy git

7. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,362 @@
   +  Drupal.quickedit.editors.image = Drupal.quickedit.EditorView.extend(/** @lends Drupal.quickedit.editors.image# */{
   

   The @lends comment is wrong

8. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,362 @@
   +     *   Options for the plain text editor.
   

   c/p remnant

9. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,362 @@
   +      // the Quick Edit Field Form.
   ...
   +          // the document (usually opens them in the same tab).
   

   The "the" fits on the previous line

10. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +    template_errors: _.template(
    +      '<div class="quickedit-image-errors">' +
    +      '  <%= errors %>' +
    +      '</div>'
    +    ),
    +
    +    /**
    +     * Template to display the dropzone area.
    +     */
    +    template_dropzone: _.template(
    +      '<div class="quickedit-image-dropzone <%- state %>">' +
    +      '  <i class="quickedit-image-icon"></i>' +
    +      '  <span class="quickedit-image-text"><%- text %></span>' +
    +      '</div>'
    +    ),
    +
    +    /**
    +     * Template to display the toolbar.
    +     */
    +    template_toolbar: _.template(
    +      '<form class="quickedit-image-field-info">' +
    +      '<% if (alt_field) { %>' +
    +      '  <label for="alt" class="<% if (alt_field_required) { %>form-required<%} %>">Alt</label>' +
    +      '  <input type="text" placeholder="<%- alt %>" value="<%- alt %>" name="alt" <% if (alt_field_required) { %>required<%} %>/>' +
    +      '<% } %>' +
    +      '<% if (title_field) { %>' +
    +      '  <label for="title" class="<% if (alt_field_required) { %>form-required<%} %>">Title</label>' +
    +      '  <input type="text" placeholder="<%- title %>" value="<%- title %>" name="title" <% if (title_field_required) { %>required<%} %>/>' +
    +      '<% } %>' +
    +      '</form>'
    +    ),
    

    I think these must use Drupal.theme(), so that themes can override these JS templates. With Backbone/Underscore templates, that's not possible.

    Unless you can justify this, of course.

11. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +            if(e.originalEvent.dataTransfer && e.originalEvent.dataTransfer.files.length){
    

    Violates Drupal core's eslint.

12. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +          // Before we submit, validate the alt/title text fields.
    +          this.save(options);
    

    Calling this.save() for validation looks strange

13. +++ b/core/modules/image/js/editors/image.js
    @@ -0,0 +1,362 @@
    +     * Drupal.ajax is not called here as the Form API is unused by this editor,
    +     * and our JSON requests/responses try to be editor-agnostic. Ideally
    

    s/editor/in-place editor/

14. +++ b/core/themes/stable/stable.info.yml
    @@ -98,6 +98,10 @@ libraries-override:
    diff --git a/sites/README.txt b/sites/README.txt
    
    diff --git a/sites/README.txt b/sites/README.txt
    old mode 100644
    new mode 100755
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    
    diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
    old mode 100644
    new mode 100755
    diff --git a/sites/development.services.yml b/sites/development.services.yml
    
    diff --git a/sites/development.services.yml b/sites/development.services.yml
    old mode 100644
    new mode 100755
    diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
    
    diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
    old mode 100644
    new mode 100755
    diff --git a/sites/example.sites.php b/sites/example.sites.php
    
    diff --git a/sites/example.sites.php b/sites/example.sites.php
    old mode 100644
    new mode 100755
    

    These mode changes need to be removed :)

#38.1: Done. Both stylesheets are duplicated in stable, which should trickle down to most themes.

#38.2: I don't dare attempt vertical+horizontal centering without it. ;)

#38.3: The "hover" class is unique to ":hover" in that it only is added if a user is hovering a file over the dropzone area. This allows us to indicate that an area is "droppable" in a unique way to just hovering over the dropzone with a mouse.

#38.4: It is scary, but it is necessary to avoid a host of issues with drag+drop. Without a fix like this, if a user drops a file onto the icon or text inside the dropzone, the *drop events will be triggered on those elements and not the dropzone, which is what we have events bound to. This usually results in the browser redirecting to the file, which is not good. See http://stackoverflow.com/questions/10867506/dragleave-of-parent-element-... for more info.

#38.5/#38.6: Done!

#38.7: I'm not sure what's wrong with it exactly, but I'm copying what Quickedit is doing for all of its Backbone Views and Models. See http://cgit.drupalcode.org/drupal/tree/core/modules/quickedit/js/views/A... for what I based this off of.

#38.8: Done.

#38.9: If I put "the" on the previous line, it will be 81 characters long.

#38.10: Why not both? I went ahead and moved all the templates to the Drupal.theme namespace, but kept the Underscore template style. Users can choose to override these with raw functions if they wish.

#38.11: Done, and I fixed a few other ESLint errors.

#38.12: The comment was outdated, the save method is not overridden.

#38.13: Done!

#38.14: Sorry about that, I did mention it in my comment but knew it would come up.

Sadly I'm going offline for the weekend so I won't be able to write tests just yet, but I'm dedicated to doing this next week. If anyone wants to take the reigns on this, I won't mind.

Awesome :) I'm hoping @nod_ will have time to also review this. Ideally, I should be able to provide a more thorough review of the JS, but I haven't touched Quick Edit's JS in well over a year (if not two). So I'm personally actually already okay with this code, it's sufficiently maintainable already. The only thing that is an absolute must-have still is test coverage.

Added tests!

+++ b/core/modules/image/src/Tests/QuickEditImageTest.php
@@ -0,0 +1,138 @@
+class QuickEditImageTest extends ImageFieldTestBase {

Given that this is a JS based feature we should also have a JS based testing, see #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

Agreed. The most relevant example to look at is probably \Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest.

I agree, but with Quick Edit already having no Javascript test coverage I'm wondering if that should be a follow up issue.

Well, we are talking about a feature request, aren't we :)

I hear you. But the rule is: "nothing new that uses JS gets added without tests". Otherwise the "lack of tests" problem just becomes bigger.

You don't need to test every possible edge case. Just test the common case. It'd be unreasonable to require you to have test coverage for every Quick Edit edge case, that'd indeed be out of scope.

Got it, I'll start re-writing the test.

Edit: I actually like the test I already have, it makes sure the controller works without taking the editor into account. I'll write a new Javascript test for the most basic drag+drop upload behavior.

Yes! Keep the current test — it tests the endpoints, which is great. Just add one to test the JS code too.

Here's a basic Javascript test which covers the standard upload flow that this feature provides. To accomplish this I had to move some code around and create new traits, as well as researching a way to mock the drag and drop behavior since we're using a headless browser and not something like Selenium. Hopefully the Quick Edit bits inspire a base class and larger test coverage for Quick Edit in the future!

Looks like the pesky GastonJS bug I found while working on this popped up again. Here's a small change that might fix it (locally the test passes for me with or without this).

So after some troubleshooting and screenshot-ing, I found that the inconsistent part of the tests was Quick Edit itself. Debugging the HTML output of the session right before failure showed either "Your changes to Test Node could not be saved" or "Could not load the form for ..." messages coming from Quick Edit after the "Save" button was pressed. Everything up to that point worked consistently without errors.

Now the test as it stands now will cover everything up to the point of saving, but will not actually save the Node. This shows that the Javascript and Controller works, but of course doesn't prove that the new Image is really set on the Node. Unless someone knows a path forward, I would suggest troubleshooting the Quick Edit weirdness when we actually add Javascript test coverage for that module.

To accomplish this I had to move some code around and create new traits, as well as researching a way to mock the drag and drop behavior since we're using a headless browser and not something like Selenium.

Sounds tricky but great!

Hopefully the Quick Edit bits inspire a base class and larger test coverage for Quick Edit in the future!

+1 :) I'm certain this will help!

#53: +1 for that scope of testing. This tests this in-place editor, nothing more. That's fine.

However, what I'd still like to see this testing, is entering a value for alt text, and then verifying that it's there. Clicking the image again, to modify its alt text, then verify that the modified alt text is present. Then I think this is testing everything of this in-place editor.

1. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,144 @@
   +    if ($this->profile != 'standard') {
   +      $this->drupalCreateContentType(['type' => 'article', 'name' => 'Article']);
   +    }
   

   This will always be true. Let's move this $this->drupalCreateContentType() call into a setUp() method.

2. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,144 @@
   +    $admin = $this->drupalCreateUser(['access contextual links', 'access in-place editing', 'access content', 'access administration pages', 'administer site configuration', 'administer content types', 'administer node fields', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content', 'administer node display']);
   +    $this->drupalLogin($admin);
   

   These can also be moved into a setUp() method.

3. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,144 @@
   +    // Initiate Quick Editing.
   +    $this->triggerClick($entity_selector . ' [data-contextual-id] > button');
   +    $this->click($entity_selector . ' [data-contextual-id] .quickedit > a');
   +    $this->click($field_selector);
   

   Elegant :)

4. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,144 @@
   +    // Our headless browser can't drag+drop files, but we can mock the event.
   +    // Append a hidden upload element to the DOM.
   +    $script = 'jQuery("<input id=\"quickedit-image-test-input\" type=\"file\" />").appendTo("body")';
   +    $this->getSession()->executeScript($script);
   +
   +    // Find the element, and set its value to our new image.
   +    $input = $this->assertSession()->elementExists('css', '#quickedit-image-test-input');
   +    $filepath = $this->container->get('file_system')->realpath($valid_images[1]->uri);
   +    $input->attachFile($filepath);
   +
   +    // Trigger the upload logic with a mock "drop" event.
   +    $script = 'var e = jQuery.Event("drop");'
   +      . 'e.originalEvent = {dataTransfer: {files: jQuery("#quickedit-image-test-input").get(0).files}};'
   +      . 'e.preventDefault = e.stopPropagation = function() {};'
   +      . 'jQuery(".quickedit-image-dropzone").trigger(e);';
   +    $this->getSession()->executeScript($script);
   

   Woah, clever!

5. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,144 @@
   +    $this->assertSession()->elementNotExists('css', 'img[src*="' . $valid_images[0]->filename . '"]');
   +    $this->assertSession()->elementExists('css', 'img[src*="' . $valid_images[1]->filename . '"]');
   

   I think these selectors include the Quick Edit entity + field selectors, to ensure these have appeared in the expected place.

I also just manually tested this and MAN THIS IS AWESOME!!!!!!!!!!

Really the only remark I have on the UX is that the label says Alt instead of Alternative text. The image widget says Alternative text. I think this should too. For consistency, but also understandability: Alt sounds too tech-y, too programmer-y for a content author.

#55 is a review of just the JS test coverage.

Here's a review of the entire patch. Basically only nits. This patch looks very close to ready to me :) I think this is looking very likely to still land in 8.2!

1. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +   * Theme function for validation errors of the Image module's in-place
   +   * editor.
   ...
   +   * Theme function for the dropzone element of the Image module's in-place
   +   * editor.
   

   Must fit in 80 cols.

   I'd omit the word "module's". Just "the Image in-place editor" is fine, and actually is even more accurate.

2. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +          var $dropzone = this.renderDropzone('upload', Drupal.t('Drag file here or click to upload'));
   

   Shouldn't Drag be Drop here?

3. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +            if(e.originalEvent.dataTransfer && e.originalEvent.dataTransfer.files.length) {
   

   A single small eslint error here :)

4. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +            var $input = $('<input type="file">').trigger('click');
   

   This is creating an <input type=file> without putting it in the DOM, right? Perhaps a brief comment here would be valuable.

5. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +      this.renderDropzone('upload loading', Drupal.t('Uploading <i>@file</i>...', {'@file': file.name}));
   

   Nit: these are 3 points, should be an ellipsis: …, not ....

6. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +        // Indicate that the field has changed - this enables the
   +        // "Save" button.
   ...
   +        // Replace our innerHTML with the new image. If we replaced
   +        // our entire element with data.html, we would have to
   +        // implement complicated logic like what's in
   +        // Drupal.quickedit.AppView.renderUpdatedField.
   

   This can be reformatted a bit to put more on the same line.

7. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,397 @@
   +     * in-place editor, and our JSON requests/responses try to be editor
   +     * agnostic. Ideally similar logic and routes could be used by modules
   

   Super nit: s/editor agnostic/editor-agnostic/

8. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,222 @@
   +   * Returns a JSON object representing the new file upload, or validation
   +   * errors.
   

   Must fit in 80 cols.

   Suggestion: s/a JSON object/JSON/

9. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,222 @@
   +      return new JsonResponse(['errors' => $this->renderer->render($messages), 'main_error' => $this->t('The requested image failed validation.')]);
   

   This message looks a bit weird to me. It's not really a requested image, is it? It's an uploaded image. It's really just an image. So what about The image failed validation.?

@Wim I also never understood why its "Alternative". Its the only text :D, but anyway guess we should just follow the spec.

Heh :) We can open a separate issue to rename Alternative text to Text in a separate issue for all places in drupal core where it is used if you want :)

All review comments addressed, and tests fixed (with the Quick Edit save added back in!) locally.

1. +++ b/core/modules/image/css/editors/image.theme.css
   @@ -0,0 +1,88 @@
   +  -webkit-user-select: none;
   

   Don't we also need the moz and ms ones? And perhaps also the unprefixed (yet unofficial) one?

2. +++ b/core/modules/image/image.routing.yml
   @@ -71,3 +71,29 @@ image.effect_edit_form:
   +    _permission: 'access in-place editing'
   +    _access_quickedit_entity_field: 'TRUE'
   ...
   +    _permission: 'access in-place editing'
   +    _access_quickedit_entity_field: 'TRUE'
   

   Great! But there's no test coverage for this in QuickEditImageControllerTest. A simple check with an anonymous user to verify that they get 403 responses is sufficient.

3. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,221 @@
   +   * Stores the Quick Edit tempstore.
   

   Nit: s/Stores the/The/

4. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,221 @@
   +   *   The Ajax response.
   

   s/Ajax/JSON/

5. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,221 @@
   +    return new JsonResponse($info);
   

   This should be \Drupal\Core\Cache\CacheableJsonResponse, and needs the appropriate cache tags.

6. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,221 @@
   +   * Returns a JSON object representing the current state of the field.
   

   Wrong comment.

7. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,221 @@
   +   * @param string $langcode
   +   *   The language code of the field that is being rendered.
   +   * @return \Drupal\image\Plugin\Field\FieldType\ImageItem
   

   Nit: We need an empty line between the last @param and the @return.

8. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,177 @@
   +    // Login as an admin user who can use Quick Edit and edit Articles.
   

   Nit: s/Login/Log in/

9. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,177 @@
   +    $this->adminUser = $this->drupalCreateUser(['access contextual links', 'access in-place editing', 'access content', 'access administration pages', 'administer site configuration', 'administer content types', 'administer node fields', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content', 'administer node display']);
   

   Let's put these all below each other.

   Also, let's rename this from $this->adminUser to $this->contentAuthorUser or something like that, and remove all the administrative permissions?

All notes addressed, again. :-)

#62.9 Was really helpful since it uncovered that QuickEditImageControllerTest didn't need to extend ImageFieldTestBase, as it wasn't testing any Drupal forms. Now the setUp method of both tests are in sync.

For the test failure - I can't replicate it locally, but it could be that the wait time isn't long enough for the upload. I bumped it up, but if this fails for the same reason I'll probably need to debug the TestBot.

I have a new theory - in Drupal.quickedit.editors.image.uploadImage, an AJAX POST is made, then when it finishes successfully the dropzone element is removed. So really, we need to do two waits - one for AJAX, one for the dropzone element to be removed.

Well that theory was wrong - let's remove what's failing now and go back to a normal AJAX wait. From there we can debug the next failure.

This is a patch triggered by my daily dealing with APCU

Reproduced locally (with APC enabled or disabled, doesn't matter):

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 35.4 seconds, Memory: 7.25Mb

There was 1 failure:

1) Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest::testUpload
Javascript condition met:
jQuery('[data-quickedit-field-id="node/1/twoxvvug/en/full"] .quickedit-image-dropzone').length == 0
Failed asserting that false is true.

/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:102
/Users/wim.leers/Work/drupal-tres/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php:153

FAILURES!
Tests: 1, Assertions: 11, Failures: 1.
wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ apcu off
Disabling APCu…
wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 33.03 seconds, Memory: 7.25Mb

There was 1 failure:

1) Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest::testUpload
Javascript condition met:
jQuery('[data-quickedit-field-id="node/1/ygzg3zlu/en/full"] .quickedit-image-dropzone').length == 0
Failed asserting that false is true.

/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/wim.leers/Work/drupal-tres/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php:102
/Users/wim.leers/Work/drupal-tres/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php:153

FAILURES!
Tests: 1, Assertions: 11, Failures: 1.

Trying to figure out a fix.

I'm pretty certain that I've found the reason it is not working for testbot nor me, but is for @samuel.mortenson. Phantomjs.

By including #2702661: Make it simple to take screenshots whilst using JavascriptTestBase's patch and taking a screenshot, you'll see that the input[type=file] continues to say "Choose file". (This patch/test uses that just to simulate a dragged-and-dropped file.)

So I started searching, and found http://stackoverflow.com/questions/29916779/file-upload-with-behat-mink-..., which points to https://github.com/ariya/phantomjs/issues/12506. It was fixed in phantomjs 2.1 (https://github.com/ariya/phantomjs/milestone/14?closed=1), released on Feb 3, 2016.

On my machine, I have this version:

wim.leers at wimleers-acquia in ~/Work/drupal-unus on 8.1.x*
$ brew info phantomjs
phantomjs: stable 2.1.1 (bottled), HEAD
Headless WebKit scriptable with a JavaScript API
http://phantomjs.org/
/usr/local/Cellar/phantomjs/2.0.0 (58 files, 50M) *
  Poured from bottle on 2015-02-02 at 14:43:39
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/phantomjs.rb

… which says "stable 2.1.1", but then later it says "2.0.0", but really it's been compiled in February 2015, and the fix only landed in December 2015. (Oh, I guess "stable 2.1.1" means that that is the current stable version.)

After updating (to 2.1.1), this is what happens:

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.2.x*
$ sudo -u _www php ./vendor/bin/phpunit -c core --group image --testsuite functional-javascript
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

.

Time: 11.68 seconds, Memory: 7.25Mb

OK (1 test, 11 assertions)

So, testbot must be running Phantomjs 2.0.x, instead of Phantomjs 2.1.x.

EDIT: samuel.mortenson confirmed: he's running PhantomJS 2.1.1

Talked to testbot maintainer @Mixologic. He confirms testbot is currently on PhantomJS 2.0. So that's definitely the explanation then. He says updating PhantomJS may be possible in the next few days.

See #2773637: Update PhantomJS to 2.1.1.

The only possible work-around is to create a File object ourselves. See https://www.w3.org/TR/file-upload/#dfn-filelist. Concrete example at the bottom of http://thinkmoult.com/using-sahi-mink-behat-test-html5-drag-drop-file-up.... This works in Chrome for example:

new File([new Blob()], "name.png");

But in PhantomJS 2.0, and 2.1, and HEAD, this is what you get:

TypeError: FileConstructor is not a constructor (evaluating 'new File([new Blob()], "name.png")')

PhantomJS issue: https://github.com/ariya/phantomjs/issues/14247.

So, alas, there's no work-around possible.

Filed #2773791: Clicking elements with children in Javascript tests throws a GastonJS exception to add my triggerClick() fix to core.

I know the TestBot hasn't been updated yet, but I wanted to re-post my patch from #64 since the last three patches were just debugging the TestBot.

Fixing tests

@droplet Awesome work! Your debugging on the triggerClick() problem has been great, and seeing the hack removed here with tests passing is very validating.

Now I'm just waiting for more review. :-)

Yep. I see you see that the testbot is now updated. Carry on.

WOOT! GREEN!

This feature is perfectly isolated: it's a new Quick Edit in-place editor, plus supporting files. It can therefore be easily reverted during beta or RC if problems are uncovered.

IMO:

1. the risk is contained
2. the benefit is huge (just see the animated GIF in the issue summary)
3. it was ready in time, but was blocked on infrastructure
4. therefore, still worthy of committing it to 8.2.x

Let's vote for REALTIME debugging.
#2775653: JavascriptTests with webDriver

and make Drupal Tests compatible with Psysh, then we able to do live code & testing Tests.

(Just a note, I didn't review any of this patch. I'm fixing the tests only.)

For what my opinion is worth, which is probably nothing...+1 to what Wim said in #84. This is a valuable, low-risk feature and would be a big win for 8.2.

+1 for RTBC. Great improvement and isolated enough to be a save thing to add.

Great work @Samuel.mortenson!

+++ b/core/modules/image/src/Tests/ImageFieldCreationTrait.php
@@ -0,0 +1,68 @@
+trait ImageFieldCreationTrait {

+++ b/core/modules/simpletest/src/TestFilesTrait.php
@@ -0,0 +1,117 @@
+trait TestFilesTrait {

This can occur in a blocking patch. At 60k this patch is big enough and these changes distract from the context.

IN #26 @Wim Leers asks for a review from a JS maintianer like @_nod that still does not appear to have occurred. @droplet notes in #85 that they have not reviewed any of the patch,

It would also be great to get a CSS maintainer's eyes on the issue - maybe @Cottser?

Wim is on honeymoon for the next 3 weeks, so if someone else is able to re-roll per @alexpott's comments, that'd be awesome.

Here's a shot at the refactor suggested in #88. I filed #2782309: Refactor File and Image related image field creation logic into a new trait for the new traits, and re-rolled the patch from #81 to not include any of the trait changes.

Keeping the issue in Needs Work as it will fail tests until the new issue is committed.

@samuel.mortenson, only committers should add the beta target tag. (We have some disagreement about whether this issue can be one.) Thanks!

Triggering testbot.

@samuel.mortenson, one strategy we can use is to combine "-do-not-test" patches containing only the changes for this issue with full patches that include the blockers, to prove tests pass. Otherwise we should postpone this issue on the other.

@xjm Here's the full patch from #81 and a do-not-test patch without the trait refactoring.

Sorry for adding the "beta target" tag prematurely, I inferred from @wim-leers and @webchick's feedback that there was consensus among core committers. Won't happen again!

Reviewing

Lost my comment mid way so a quick version and i'll finish later.

1. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +      this.model.set('originalValue', $.trim(this.$el.html()));
   

   there is a native function for this. If it's trimmed, it's not really the original value no?

2. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +          var stopEvent = function (e) {
   +            e.preventDefault();
   +            e.stopPropagation();
   +          };
   

   I'd rather keep things explicit and have preventDefault and stopPropagation everywhere.

3. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +          $dropzone.on('dragleave', function (e) {
   +            stopEvent(e);
   

   It's better do to the preventDefault and stopPropagation after any handling code. In case the code breaks.

4. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +            $input.change(function () {
   

   Always use .on(), not the jquery event aliases

5. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +      var data = new FormData();
   

   This is not available on IE9/10, I'm fine with it but that might be a problem.

6. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +        $el.html(content);
   

   Might need a Drupal.attachBehaviors no?

7. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +    ajax: function (type, url, data, callback) {
   

   Why not make it like the jquery ajax function and let it accept an object. that would prevent the need to pass in null later on.

   $.extends() can be use to merge the default and provided values.

8. +++ b/core/modules/image/js/editors/image.js
   @@ -0,0 +1,402 @@
   +        $dropzone = $(Drupal.theme.quickeditImageDropzone({
   

   We're supposed to use it like Drupal.theme('quickeditImageDropzone', {}) but it's not a big problem

Yep, does look much better.

Addressed all my issues, Need an descision from core comitters on the IE10 not supporting FormData.

underscore templates are not necessary, it doesn't take much more code to do it in JS and we don't introduce a depencency on underscore templates. If js templating (as in using dedicated templating language) happens in D8 it need to be a dedicated decision, not sneaked in in an unrelated patch.

Any word from core maintainers about whether or not this is going to be targeted for the 8.2.x beta cycle?

@samuel.mortenson: the last beta was released for Drupal 8.2, so no chance there. RC1 is coming out next week, see https://groups.drupal.org/node/513529. Improvements are already committed to 8.3, eg. the views listing page is already significantly different in 8.3 from 8.2, so this has a good chance to land early in 8.3 and get released next April.

Thanks! Updating the issue to reflect that.

Sad to see this didn't make it in 8.2. Of course, we need to stop somewhere. Glad to see this is extremely likely to be in 8.3!

Need an descision from core comitters on the IE10 not supporting FormData.

Actually, IE10 does support FormData. It's just buggy, and the bugs are only being fixed in Edge (not even in IE11). IE10 currently has 0.57% global market share — even less than IE9! And it's unsupported since January 12, 2016.

I'll be bold.

Fixed minor patch conflict.

Thanks for the reroll!

Nice, glad to see this RTBC again.

Looks like @alexpott asked for @Cottser's feedback on the CSS, so assigning.

There is some movement in other issues around doing similar File/Image test refactors as this issue - so we should try to incorporate #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile when eventually committing this issue.

If this issue would just land, then that issue can just update the tests added by this patch. :)

Thanks for the bumps, hoping to get to this soon.

As of #2235977-68: JS Client-side file validation is broken (because ajaxPageState is broken?), that major bug's patch is now also depending on the JS test trait that this issue adds.

Also, this issue is a very big usability win.

For both of those reasons, bumping this to major.

Re-rolled the patch assuming that #2782309: Refactor File and Image related image field creation logic into a new trait is going to get committed. This patch is smaller than #110 as #2738567: Add test trait for drupalGetTestFiles and drupalCompareFile was already committed.

Well, actually, based on #120, this is now blocked on #2782309: Refactor File and Image related image field creation logic into a new trait.

Unpostponing it after #2782309: Refactor File and Image related image field creation logic into a new trait

Back to RTBC per #121

As to not confuse reviewers, I've re-named the no-traits patch and re-uploaded it.

Perfect :)

One thing I'm wondering is: is it okay to add a route which requires another module to be installed.

1. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,225 @@
   +      if (in_array($view_mode_id, $entity_view_mode_ids)) {
   

   Nitpick: its always better to use string mode when dealing with strings.

2. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
   @@ -0,0 +1,225 @@
   +    $field_list = $entity->getTranslation($langcode)->$field_name;
   ...
   +      $field = $entity->getTranslation($langcode)->$field_name->get(0);
   

   Just a personal style: IMHO its just way nicer to use ->get($field_name). Note: get(0) is the same as first()

#127: RE: route requiring another module. That's a very good point actually. Perhaps we should conditionally add those routes, only if the quickedit module is installed?

#127.1 s/string/strict/ :D

Sorry for the delayed review, folks. I'm not sure if the routing point brought up in #127 needs addressing before commit or not.

+++ b/core/modules/image/css/editors/image.theme.css
@@ -0,0 +1,90 @@
+  margin-right: 10px;
...
+  margin-right: 0;

I think these need /* LTR */ comments and relevant RTL code added (see screenshots below). https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

LTR:

RTL:

+++ b/core/modules/image/image.routing.yml
@@ -71,3 +71,29 @@ image.effect_edit_form:
+    _access_quickedit_entity_field: 'TRUE'
...
+    _access_quickedit_entity_field: 'TRUE'

This isn't necessarily a security issue, since anyone hitting this route should get a ServiceNotFoundException. But yes, these should be added dynamically.

NW for #127.1 (it's already safe as #130 explains, but it's clearer) + #129.

I based my route definition on the editor module's in-place editor route, which is not added dynamically:

editor.field_untransformed_text:
  path: '/editor/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
  defaults:
    _controller: '\Drupal\editor\EditorController::getUntransformedText'
  options:
    parameters:
      entity:
        type: entity:{entity_type}
  requirements:
    _permission: 'access in-place editing'
    _access_quickedit_entity_field: 'TRUE'

I'll re-roll today and address #129 and #127, but if we need to address the route issue we should do it for editor as well.

but if we need to address the route issue we should do it for editor as well.

Excellent point. Let's defer that to a follow-up then.

This patch addresses the notes in #129 and #127.

Thanks!

Thanks! Only minor things left I think. :)

CSS

One more minor CSS point:

+++ b/core/modules/image/css/editors/image.theme.css
@@ -0,0 +1,99 @@
+  -webkit-user-select: none;
+  -moz-user-select: none;
+  -ms-user-select: none;

We should also include the non-prefixed version. Chrome 54 and higher even support it :)

http://caniuse.com/#search=user-select

PHP

1. +++ b/core/modules/image/src/Tests/QuickEditImageControllerTest.php
   @@ -0,0 +1,186 @@
   +   * @param integer $nid
   

   Minor: PHPCS wants this to be int, not integer.

   FILE: ...ev/core/modules/image/src/Tests/QuickEditImageControllerTest.php
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    157 | ERROR | [x] Expected "int" but found "integer" for parameter
        |       |     type

2. +++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   @@ -0,0 +1,171 @@
   +    $this->assertSession()->elementExists('css', $entity_selector . ' ' . $field_selector . ' ' . $new_image_selector);
   +  }
   +}
   

   Minor: Missing blank line before the class' closing brace per PHPCS.

   FILE: ...ules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    171 | ERROR | [x] The closing brace for the class must have an empty
        |       |     line before it

JavaScript

There are also some eslint errors. The 'use strict' ones seem wrong, maybe something is wrong on my end or we need to update our estlintrc again? The rest seem legitimate though.

/Users/Scott/Sites/d8commit.dev/core/modules/image/js/editors/image.js
  6:1  error  Use the global form of 'use strict'  strict
  8:3  error  Use the global form of 'use strict'  strict

✖ 2 problems (2 errors, 0 warnings)


/Users/Scott/Sites/d8commit.dev/core/modules/image/js/theme.js
   6:1    error  Use the global form of 'use strict'          strict
   8:3    error  Use the global form of 'use strict'          strict
  21:47   error  Missing space before function parentheses    space-before-function-paren
  42:63   error  Infix operators must be spaced               space-infix-ops
  78:118  error  There should be no spaces inside this paren  space-in-parens
  81:14   error  Multiple spaces found before ''</form>''     no-multi-spaces

✖ 6 problems (6 errors, 0 warnings)

Addressed the notes in #136, except the "use strict" warning, which would also apply to many other Drupal JS files. Guessing that's just a quirk.

I also updated the stable theme's copy of image.theme.css to reflect the changes I made in #134, I missed that last time.

The strict thing is indeed a known problem, being fixed at #2821113: eslint 'use strict' rule wrong.

Is it really working?

Could anyone teach me how to use it? I remember that's no config when I tested and patched in #81. Now, I can't drag-n-drop.

Tested with simplytest.me also.

@droplet Yes, the patch still works. To test it, you can install the standard profile, create a new Article with only* the Title and Image fields filled in, then Quick Edit that Article from the front-end. Once Quick Edit is initiated, clicking on the Article's image should prompt you to either click to upload or drag and drop.

* The Bartik theme floats Article images the left, which puts them below the body field, which has 100% width. As a result you cannot Quick Edit Article images in Bartik if the body field is filled in.

Ahh. I found the problem.

It doesn't work if I ALSO upload another an image via IMAGE ICON in CKEditor.

EDITED: even only TEXT in body field, it doesn't work

This patch does not add any functionality to CKEditor, it only adds a new inline editor to single-value Image fields. You should not be using the body field for testing.

#142: for that, there is #2560457: Support drag-and-drop image uploads in CKEditor. This issue applies only to Quick Edit. You're referring to CKEditor.

Updated the issue title to make that as explicit as possible.

Umm.... any issues for overlay conflicts between 2 fields?

There's a hidden bug in CORE (or this patch)

1. applied patch & update.php & clear caches.
2. GIT reset --hard, clear caches.
3.
ERROR 500
drupal8x/quickedit/attachments?_wrapper_format=drupal_ajax

Looks like doesn't clear the router caching correctly

Umm.... any issues for overlay conflicts between 2 fields?

Quick Edit has problems when fields are :

1. Overlapped other fields, which may cause contextual links to stack.
2. Positioned with a z-index below another element (and as a result are not click-able)
3. Positioned absolutely

Themes can do lots of things to make Quick Edit inaccessible, and while we could patch Bartik to not use floats for image fields, that is out of scope for this issue and may cause problems for users with Bartik sub-themes.

I haven't seen the bug you pointed out, can this be replicated consistently by installing Drupal, applying the patch, then running update.php?

I don't think I'll have time to re-review this myself in the near future, but another committer can pick it up. I was happy with it overall last I looked :) thanks all!

+++ b/core/modules/image/image.install
--- a/core/modules/image/image.libraries.yml
+++ b/core/modules/image/image.libraries.yml

+++ b/core/modules/image/image.libraries.yml
@@ -3,3 +3,19 @@ admin:
+quickedit.inPlaceEditor.image:
...
+    - quickedit/quickedit

Are we OK with image module providing a library that depends on quickedit? The alternative would be to add this library in hook_library_info_build() (or what it's called) conditionally if quickedit module is installed. Not sure ...

#148: that quickedit.inPlaceEditor.image asset library will only be attached by Quick Edit the module. Just like it's okay to define a service that will only be conditionally picked up by some module, this is a library that will only be conditionally picked up by some module.

So even if we want to move this to hook_library_info_build(), that can absolutely be done in a follow-up.

This patch has been RTBC for >2 months, since #112. Everything since than has been nitpicks. We can keep nitpicking it to death, or we can land it and then just keep improving the Quick Edit module. At this rate, nobody will ever again want to contribute new features to Quick Edit. :(

We can keep nitpicking it to death, or we can land it and then just keep improving the Quick Edit module. At this rate, nobody will ever again want to contribute new features to Quick Edit. :(

I completely agree, I really wanted to see this on Drupal 8.2

+++ b/core/modules/image/src/Plugin/InPlaceEditor/Image.php
@@ -0,0 +1,39 @@
+namespace Drupal\image\Plugin\InPlaceEditor;
...
+ * @InPlaceEditor(
+ *   id = "image"
+ * )
+ */
+class Image extends InPlaceEditorBase {
...
+        'image/quickedit.inPlaceEditor.image',

Ahh I missed, that the library is conceptually attached to the plugin.

In that case, there's no reason for any additional logic, in my opinion.

Thanks for setting me straight!

np, and thanks for looking out :) I totally do appreciate the constructive criticism!

Reviewed this on UX meeting today, since Cottser handed it over for another committer to look at.

While testing, we ran into the same issue as @droplet, where if you upload an image via CKEditor, as well as an image via a regular image field, the Quick Edit target will fail to allow you to target the Image field (only Body). However, this seems to be a pre-existing condition, according to comments by Wim and others.

We walked through the patch together and looks good to us. Nice test coverage (including JS tests! :)). Code all seems to be conforming to coding standards (or at least https://github.com/alexpott/d8githooks wasn't triggering anything), and error handling seems to be working well, so we should be good to go here.

Committed and pushed to 8.3.x. Thanks!

Yay!

This is so awesome, just tested it, thank you for this!

Happy that this has landed, thus I won't reopen this, but create some follow up issues:

#2827997: Quick Editing single images is impossible when they are floated
#2827996: Drag and drop UX for Quick Editing small images is difficult

Nice, at last this issue is committed!!!

#157: thanks!

• Closed #2827997: Quick Editing single images is impossible when they are floated as a duplicate of #2637500: Fields floated via CSS can break Quick Edit — also happens for Article nodes in Bartik.
• #2827996: Drag and drop UX for Quick Editing small images is difficult is a genuine need for improvement.

I definitely played a part in delaying this (#149). Sorry to be a bottleneck, but happy to see this in :)

Just for reference, another two follow ups:

#2829790: Unexpected behavior when switching from one changed Quick Edit field to another
#2829792: Improve the "Image" InPlaceEditor plugin's "alt" attribute UI

#161: Thanks! Updated/clarified both of those issues. Thanks so much for digging deep and finding edge cases :)

Also updated https://www.drupal.org/docs/8/core/modules/quick-edit/overview to mention this now!