Support drag-and-drop image uploads in CKEditor

+1 to all that.

But IMO this would initially just skip two steps of uploading an inline image as it exists in D8: rather than having to click the image button and then clicking the "upload" button, you'd just drag in the image and have that open the dialog. That's IMO step 1.

No need for this to be postponed.

#9: I'd like to get feedback from @mlewand on this, since this is using a CKEditor API and he's going to be far more knowledgeable about this than me.

#10: I don't see how this is remotely related to #2113931: File Field design update: Upload field.? It sounds like you're thinking/saying/arguing that images uploaded via CKEditor should always also be listed in a @FieldType=file field on the same entity? That's the only way I can make sense of #10. If that's what you mean, I agree that'd be nice, but A) it's not possible to require that, B) making that required would be a huge BC break. This issue is just about making the existing experience nicer. See #4.

because temporarily I'm unable to install D8.

You need to do composer install now — see https://www.drupal.org/node/2648064.

Thanks, @mlewand! Now @thpoul can continue :)

AWESOME WORK!!!

I think there are two big missing things:

1. Before showing the dialog: this is not doing the "instant preview" of the dropped image, unlike http://sdk.ckeditor.com/samples/fileupload.html#uploading-dropped-and-pa... — it looks like #9 actually had some of the code necessary for that?
2. After saving the dialog: there's no <img src=…> being inserted yet!

Once that's done, the behavior/experience is similar to that at http://sdk.ckeditor.com/samples/fileupload.html#uploading-dropped-and-pa..., but with one key UX difference: you're forced to enter an alt tag, because Drupal cares about accessibility. (And a bunch of technical differences, of course.)

To address Inherit the drupalimage and drupalimagecaption functionality: do we even need a separate plugin? I don't think we do? I think this can live in the drupalimage CKEditor plugin. I also don't see how it can work otherwise; how could it otherwise work with drupalimagecaption?

IOW: just remove the new PHP code, and just merge the JS code with the existing JS code in drupalimage/plugin.js

Simulate 3G in your browser. It now takes a long time for your dialog to show up. Then it takes a long time for the image to upload.

Having that preview would make this much more tolerable. It ensures the user sees that things are working.

I now tested #20 myself. With both 3G and 2G simulation.

CKEditor shows a "preview" of the image while the uploading still happens at http://sdk.ckeditor.com/samples/fileupload.html#uploading-dropped-and-pa...

.cke_upload_uploading img { opacity: 0.3; }

I could see that helping things a lot in those "slow network" scenarios.

1. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,47 @@
   +        supportedTypes: /image\/(jpeg|png|gif|bmp)/
   ...
   +        if (CKEDITOR.fileTools.isTypeSupported(file, /image\/(jpeg|png|gif|bmp)/)) {
   

   We should hardcode this to match \Drupal\editor\Form\EditorImageDialog::buildForm()'s hardcoded values:

   'file_validate_extensions' => array('gif png jpg jpeg'),
   

2. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,47 @@
   +          $(window).on('dialogcreate', function (e, dialog, $element, settings) {
   

   This means every created dialog in the future will have this happen. We need it to happen only once.

3. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,47 @@
   +            var uploadButtonId = '#' + form.find('[name=fid_upload_button]').attr('id');
   

   You should not use [name=…], but instead [data-drupal-selector=…].

4. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,47 @@
   +            _.each(Drupal.ajax.instances, function (value, key, list) {
   +              if (value !== null && value.selector === uploadButtonId) {
   +                var myAjaxObject = Drupal.ajax.instances[key];
   

   Rather than doing this difficult-to-understand nesting, let's have a helper function to find the right AJAX object.

#23 is a huge improvement, UX-wise!

I'd love to see something like .cke_upload_uploading img { opacity: 0.3; } to indicate it's an in-progress thing. thpoul told me in IRC that he had difficulty finding the right place to do this: CKEditor's uploadwidget uses CKEDITOR.addCss(), which we cannot use. Plus, that only works for iframe instances, we need it to work for inline and iframe instances, and it must be customizable by themes.

After some searching, I found the solution:

12:25:58 <WimLeers> the "text editor" in-place editor's getAttachments() calls the text editor plugin manager's getAttachments(), which calls each text editor plugin's getLibraries() method
12:26:12 <WimLeers> And the "CKEditor" text editor plugin has this:
12:26:15 <WimLeers> 
  public function getLibraries(Editor $editor) {
    $libraries = array(
      'ckeditor/drupal.ckeditor',
    );

    // Get the required libraries for any enabled plugins.
    $enabled_plugins = array_keys($this->ckeditorPluginManager->getEnabledPluginFiles($editor));
    foreach ($enabled_plugins as $plugin_id) {
      $plugin = $this->ckeditorPluginManager->createInstance($plugin_id);
      $additional_libraries = array_diff($plugin->getLibraries($editor), $libraries);
      $libraries = array_merge($libraries, $additional_libraries);
    }

    return $libraries;
  }
12:26:31 <WimLeers> i.e. it calls the getLibraries() method on every CKE plugin plugin
12:26:58 <WimLeers> So… we could let \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImage::getLibraries return a file that contains this
12:27:08 <WimLeers> So I think the right solution is actually:
12:27:27 <WimLeers> 1. add new "image/ckeditor-drag-and-drop" library
12:27:32 <WimLeers> 2. return that in \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImage::getLibraries()
12:27:56 <WimLeers> 3. Let image.module implement hook_ckeditor_css_alter() and return the same CSS file there

Now reviewing the actual patch.

Hmmm dammit! #24 assumes that the DrupalImage plugin lives in the image module, but it lives in the ckeditor module. So the library would be called something like ckeditor/plugins.drupalimage.drag-and-drop-uploads.

1. beforeSerialize() needs to be better documented. What does it do? Why specifically beforeSerialize() and not beforeSubmit() or one of the others?

2.               options.contentType = false;
                 options.processData = false;
   

   Why these overrides? Needs docs.

3. Now only one dialog is being updated, but it still isn't guaranteed to be the right dialog. It's just doing it for the first dialog that appears. I realize that it's extremely unlikely that another dialog will be opened in the mean time, but it still doesn't feel right.
4. http://sdk.ckeditor.com/samples/fileupload.html supports dropping multiple files. We must limit it to a single file (i.e. ignore other dropped files), because we can't open multiple dialogs at the same time. (And again, we want that dialog to be opened so the content creator is forced to provide alternative text.)
5. I tried to fully grok what https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/uploadwidge... + https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/uploadimage... do. But I failed. We need somebody from the CKEditor team to review this code.
   (Note that I even went back in time to see how they dealt with integrating uploadimage and image2, but they never integrated that. Note that Drupal's drupalimage plugins is just an extension of/a layer on top of image2.)
6. Finally, I looked into the (AFAICT) sole missing feature: when closing the EditorImageDialog, the placeholder image should be removed. The tricky thing there is that the design/assumption is that it's only Editor(Link|Image)Dialog that's creating/inserting HTML. But in this case, we already insert HTML before opening the dialog, so we must remove it afterwards. That makes it more difficult than I hoped it would be.
   I considered making saveCallback be called even upon closing (but this time with its argument not being the values, but undefined). Unfortunately, that would be an API change.
   But the reason that that is designed in that particular way, is because Drupal's AJAX Dialog API is designed to be used solely from the server side, not from the client side. It's literally impossible to specify a close() callback for a specific dialog instance (you could do it for all of them, but that'd be beside the point), despite http://api.jqueryui.com/dialog/#event-close explicitly supporting that. This is also in part to the horrendous complexity/confusingness that jQuery UI (Dialog)'s API is. (Look at core/misc/dialog/dialog.js + core/misc/dialog/dialog.position.js + core/misc/dialog/dialog.ajax.js to get even a vague idea.)
7. So, the best we can do is to provide editor:dialogopen (to solve point 3) and editor:dialogclose events (to solve the previous point), similar to the already-existing editor:dialogsave event. We cannot tie it to this particular dialog instance due to the reasons given in the previous point, but we can make it reliable nonetheless: by adding a certain class to the dialog and then mapping dialog:aftercreate to editor:dialogopen and dialog:afterclose to editor:dialogclose, but only if those events are being triggered for a dialog that has that certain class, we have sufficient guarantees. Because there's guaranteed to only ever be one such modal dialog active.
8. Finally, we need to make sure that "undo" still works correctly. Currently, if you drag-and-drop an image into CKEditor, complete the dialog, then if you undo, you'll actually revert back to the preview image. We don't want that, of course :)

This reroll fixes points 3, 6 and 7. And my changes need further refinement, this is just providing some of the missing things that this issue really needs.

#26 took me >3 hours to figure out how to make those events work (i.e. points 6 + 7). Then once I had them, I fixed point 3 in a few minutes.

I already pinged @mlewand for #26.5 :)

#28's interdiff is looking good! (The failing test is because you need to add the same stuff to the stable theme.)

And to make it easier to see the changes for the CKEditor team people who review this: this is #28, minus the changes to our build of CKEditor. i.e. just the changes that need review.

#26.8 has not yet been addressed — or at least it's not yet working correctly for me.

Finally, we need to make sure that undo still works correctly. Currently, if you drag-and-drop an image into CKEditor, complete the dialog, then if you undo, you'll actually revert back to the preview image. We don’t want that, of course :)

EDIT: well, it works okay in case of canceling (drag + drop image, then close the image dialog). But it does not yet work correctly in case of the dialog being used as intended: alternative text filled out, click "save", and then undo.

Plus, this is still missing JS test coverage.

It'd also be great if the CKEditor team could review this of course, so I pinged them as well.

I'd love to RTBC this, but I can't, not without a \Drupal\FunctionalJavascriptTests\JavascriptTestBase test. I'd prefer to have the CKEditor team's approval of the JS too, but I could live without that since the code is so small, and easily revertible.

The functionality now works great!

IMHO this should qualify for beta target, since it's easily revertible.

1. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,76 @@
   +              // jQuery.ajax() by default sets the contentType as
   +              // “application/x-www-form-urlencoded; charset=UTF-8”. As of
   +              // jQuery 1.6 you can pass false to tell jQuery to not set any
   +              // content type header.
   +              // @see http://api.jquery.com/jquery.ajax/
   

   Why do we not want to send a Content-Type request header?

2. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -267,6 +267,76 @@
   +              // order to send a DOMDocument, or other non-processed data, we
   

   We're not sending a DOMDocument? (That's PHP!) We're sending FormData?

3. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -363,6 +433,25 @@
   +        ajaxInstance = Drupal.ajax.instances[k];
   +        return ajaxInstance;
   

   Don't assign, just return.

4. +++ b/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
   @@ -363,6 +433,25 @@
   +    return ajaxInstance;
   

   Just return null.