Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms

Yep agreed. My preference here would be that editor.module provide a dialog for manipulating both links and images and then CKEditor module can utilize those dialogs. We have already specified a custom drupaldialog plugin for CKEditor which has TODOs for switching to a more official Drupal dialog system. It's unlikely we'll be able to completely eliminate all use of the default CKEditor dialogs though, unless we want to rewrite the dialogs for manipulating some of the more esoteric tags, such as table and table cell properties. However in the event that a user does not require the default CKEditor dialogs, we should be able to disable the CKEditor dialog plugin.

I would agree with replacing (every) CKEditor Dialog with a Drupal one, since they are a bit different to work with for Mac OS X screen reader users.

Since this is a core-specific issue and not backportable to D7, let's move this to the core queue. The 8.x-1.x branch of this module is effectively too forked to be useful (other than for reference).

This is postponed until #1890502: WYSIWYG: Add CKEditor module to core is committed.

#1890502: WYSIWYG: Add CKEditor module to core got committed — unpostponing.

I'll probably use this issue to re-introduce the "drupalimage" plugin and write a "drupaldialog" plugin. The two items that make the most sense to provide dialogs for are links and images. Other things like tables may continue to use the CKEditor dialogs.

Here we have two options:

- Easier (do not confuse with "easy") / limited: Write independent dialogs that are simply opened by the editor but using their own logic. Still not all dialogs will be covered, just the implemented ones.

- Harder / complete: replace the "rendering layer" of the dialog system of CKEditor (dialog plugin) with a custom system, which makes use of the "dialog definitions" we have in CKEditor. In this way, all dialogs will be covered, including plugin contributed ones.

We have never worked on any of the above solutions, so it is still unknown how to make them happen and how easy/hard it may be. I'll certainly be happy to have it investigated. Still after CKEditor 4.1.

If you go ahead with the "Easier" way (looks like the way which quicksketch prefers right now),
the automatic removal of things from dialogs that are not allowed by allowedContent will not be a problem as long as your custom dialogs will do the same :)

For example the CKEditor dialog system is now able to recognize requiredContent property in each single uiElement. Specyfying requiredContent: 'img{float}' in element definition ("align" select field in this example) is enough to tell dialog system to disable this field if required content is not allowed.

If you remove the whole image plugin, remember to specify allowedContent/requiredContent in your custom feature/command definition and provide there something like img[alt,src] (see the source of the image plugin in t/9990 branch for example - a dedicated branch for http://dev.ckeditor.com/ticket/9990).
Otherwise CKEditor will not be aware that this tag is allowed (unless you set the global allowedConent configuration option and provide it there).

Architecturally, I'd also prefer to go with Drupal dialogs, since that inherently removes another hard dependency on CKEditor.

But like CKEditor (module), this was on the plan/roadmap of Wysiwyg module for a very long time, but we never came around to develop a generic implementation and integration...

Opening and closing dialogs is piece of cake. Loading the dialog content is equally trivial. The actual point where it gets a bit hairy are the dialog buttons, and, the actual dialog button actions — i.e., you need to know what Cancel is supposed to do (potentially more than just close), and of course, the primary dialog button is almost guaranteed to need a custom label, as well as some kind of JS callback or similar.

The way we wanted to approach this in Wysiwyg was basically along the lines of a separate JS file/object for each dialog. With the New World Order™ of D8, we might actually be able to leverage Backbone for that, as its architecture is sorta predestined for this kind of task (i.e., a "mini-app" that gets set up, has interaction, and is teared down). That said, most dialog interactions would actually call into server-side endpoints; the client-side dialog futzing would be limited to managing the "state" of the dialog in most cases.

Speaking of, we're definitely entering Wysiwyg API waters here. ;)

Architecturally, I'd also prefer to go with Drupal dialogs, since that inherently removes another hard dependency on CKEditor.

I agree with this statement too. CKEditor will still need some Drupal-specific plugins in our installation, but ultimately it should open up an editor.module-provided dialog.

I've started on implementing this functionality. So far I have only built out the skeletal framework to get drupalimage and drupaldialog plugins loading into the system again. It doesn't load a Drupal-based dialog yet, it just gets the plugins onto the page and replaces the normal CKEditor "image" plugin with the "drupalimage" plugin. Long-term, I think it would be perfectly reasonable for us to remove the normal CKEditor image plugin from the Drupal build.

This plugin requires a non-sprited image, since it's not part of the CKEditor build. This file should be named "image.png" and be placed in core/modules/ckeditor/js/plugins/drupalimage/image.png.

Some open questions for implementation:

- Should we follow CKEditor's coding standards for CKEditor plugins, or Drupal's standards? The plugins right now follow CKEditor standards, but I'm inclined to convert them to Drupal JS standards because it's likely that other Drupal developers are going to be working in this code. We follow our own standards for our jQuery plugins even though the jQuery team uses a different standard (that is similar to CKEditor), so I see this as the same situation.

- The drupalimage plugin has a dependency on the drupaldialog plugin. We don't have a dependency system for CKEditor plugins currently. The patch in #10 provides rudimentary dependency support, but it's not recursive. If your plugin depends on a plugin that depends on a third plugin, both dependencies would need to be explicitly stated. Is it worth building out a more comprehensive auto-detecting dependency system? My initial feeling is "no", because dependencies in CKEditor are usually quite limited. Unlike Drupal modules, 2 or 3 level dependencies are rare.

Should we follow CKEditor's coding standards for CKEditor plugins, or Drupal's standards?

I would definitely recommend going with Drupal standards for anything that is Drupal specific. You should opt for CKEditor standards only if your intention is contributing code back to CKEditor, which I believe is not the case, when it comes to dialogs.

Completely replacing the CKEditor dialog plugin seems unlikely to happen unless we're going to have a custom CKEditor build in Core. Plugin dependencies are hardcoded by name as well as bundled and auto-registered simply by including ckeditor.js.

Here's an updated patch that gets us moving forward a bit. It guts most of the existing drupalimage plugin and replaces the use of the CKEditor dialog with a Drupal dialog. It's not close to usable though: clicking on the image button opens up a dialog that loads from Drupal via an AJAX request, but it doesn't have the ability to save and close the dialog. Currently our dialog API is not sufficient to implement this functionality.

@dawehner pointed me to #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases, which would be very helpful for our purposes. Unfortunately that issue looks stuck in its own conflict dealing with proper abstraction. To keep this issue moving along, we may end up duplicating a lot of the same work that Views UI has done (which apparently has its own JS, CSS, and AJAX actions). This would mean not using the dialog.js file at all and writing our own dialog handling through jQuery UI in editor.js (or a new JS file in editor module).

A few other changes:
- Adds a simple dependency system for CKEditor plugins (we may be able to remove this if it turns out a dialog CKEditor plugin is not needed).
- Adds the ability for a CKEditor plugin to depend on a Drupal library.
- Plugins converted to use Drupal coding standards (still a little incomplete).

Hm. Simplytest.me fails on the patch.

Heh, my last patch apparently was incomplete because it only included half the changes. This one should actually be testable and makes significant progress:

- Actually opens a dialog! Yay!
- Can save the dialog! Yay!
- Inserts or updates an image src/width/height attributes! Yay!

Life got a lot easier on this issue once I stopped trying to use dialog.js. The abstraction layer is drastically over-simplified and not nearly flexible enough for our purposes. This patch loosely uses the same approaches as Views UI and #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases, but the use-case is both simpler and different, so even if that patch was completed in its current form, I don't think it would provide the functionality we need here. Obviously updating the modal framework to actually be useful would be the best option, but considering we don't know the use-cases until we need them, doing this issue first and then later abstracting the API seems like a good approach.

Other changes:
- This version no longer makes a drupaldialog plugin. editor.js provides all the dialog support we need directly.
- A bunch of cruft is removed from the drupalimage plugin.

Responding to @wwalc in #8:

If you remove the whole image plugin, remember to specify allowedContent/requiredContent in your custom feature/command definition and provide there something like img[alt,src] (see the source of the image plugin in t/9990 branch for example - a dedicated branch for http://dev.ckeditor.com/ticket/9990).

I couldn't for the life of me get IMG tags to be allowed by using allowedContent/requiredContent. I'm either doing something wrong or it's not working properly in our current build of CKEditor in core. For now I've had to disable the content filter by setting allowedContent = true in our global configuration. Help figuring out why img tags still aren't allowed would be appreciated.

This patch also includes the latest fix in #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), since adding content via AJAX is currently broken without it:

--- a/core/lib/Drupal/Core/Ajax/AjaxResponse.php
+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -133,7 +133,7 @@ protected function ajaxRender(Request $request) {
     $scripts = drupal_add_js();
     if (!empty($scripts['settings'])) {
       $settings = drupal_merge_js_settings($scripts['settings']['data']);
-      $this->addCommand(new SettingsCommand($settings, TRUE));
+      $this->addCommand(new SettingsCommand($settings, TRUE), TRUE);
     }

We can remove this part of the patch once that issue is fixed.

Patch without the binary image.png file, apparently testbot can't apply patches with binary files in them. This file can still be retrieved from #10, and needs to be placed in core/modules/ckeditor/js/plugins/drupalimage/image.png.

Fixes tests. Fixes display of buttons in the admin interface that use an image instead of the CKEditor default CSS.

Since image alignment isn't going to work if there aren't any alignment buttons, I'll remove that code for a followup. For now it's included in this patch so that I have place to retrieve the working code in the event that I loose track of it locally.

Here's the same patch as #22 with the alignment support removed, and I removed the image.png file for testbot. This should pass all tests and is ready for some basic reviewing. I want to clean up the JS for reading/setting properties to make it more flexible, but for now src/alt/width/height are all saving properly.

Fixing remaining tests. Made attribute detection and sending to Drupal more robust (now all attributes on the img tag are sent to Drupal, allowing for easy contrib extension).

An unfortunate side-effect of letting CKEditor manage its own JS loading for plugins is that locale.module is not parsing the files so calls to Drupal.t() do not actually translate any values. We only had one call to Drupal.t() though, which I circumvented by letting Drupal set the modal dialog title from the PHP side instead of doing it on the JS side. Long-term, we should figure out how Drupal should extend the available translated strings to CKEditor.

Fixed

• Fixed lots of JSHint error, missing closure variables, and use of Drupal.settings instead of drupalSettings.
• Improved docs for Drupal.editor.(de|at)tach().

Feedback

• #26 works only when editing an existing image. It's broken for inserting new images.
• It's very bad for front-end performance that this whole approach implies we're making a round trip to the server for every dialog. Besides the additional load on the server, the latency cost incurred cannot be ignored, especially on mobile devices. Just add a sleep(3); to simulate latency + slow responses of a system under load. Imagine being confronted with that loading screen every time you insert or edit an image… Is using server-side generated forms really a necessity?
• Note that this approach also does not yet address non-Drupal CKEditor plugins: they would still use CKEditor dialogs. It's fine if it that's still a TODO, but I just want to call it out explicitly.
• Double-clicking the image to open the dialog does not work.
• For some reason, it shows an error message for a pre-existing image: "URL cannot be longer than 128 characters but is currently 172 characters long." — but it only shows this *after* clicking "save", and the only if you start editing the same image *again*.
• Modal title: "Insert/Edit Image" should read "Insert image" when inserting and "Add image" when adding.
• Width & height form field labels should probably be inline with the actual form fields. They both should be on the same line, too.

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -20,7 +20,7 @@ function ckeditor_library_info() {
-      $module_path . '/js/ckeditor.js' => array(),
+      $module_path . '/js/ckeditor.js' => array('group' => JS_DEFAULT),

Why set this explicitly?

+++ b/core/modules/ckeditor/js/ckeditor.jsundefined
@@ -15,6 +15,10 @@ Drupal.editors.ckeditor = {
+    // Save settings that are Drupal-specific into the editor config.
+    format.editorSettings.drupal = {
+      format: format.format

Why?

(If a certain plugin wants to know this information, it, it's up to that plugin's getConfig() implementation to pass it to the plugin as a JS setting.)

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginBase.phpundefined
@@ -30,7 +30,7 @@
-abstract class CKEditorPluginBase extends PluginBase implements CKEditorPluginInterface, CKEditorPluginButtonsInterface {

The job of the plugin base class is to simplify the 80% use case.

By removing CKEditorPluginButtonsInterface, that is no longer true.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginBase.phpundefined
@@ -39,4 +39,24 @@ function isInternal() {
+  /**
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginInterface::getConfig().
+   */
+  function getConfig(Editor $editor) {
+    return array();

Why provide a default implementation of this method?

This was omitted intentionally; the plugin implementation should implement this itself, it doesn't make sense to provide a default implementation of this interface method.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginInterface.phpundefined
@@ -41,6 +41,27 @@
+  public function getLibraries(Editor $editor);

IMO the Editor $editor parameter should be removed.

If there is a valid use case to keep it, then it should also exist for getDependencies() (and it should be documented in the docblock).

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.phpundefined
@@ -70,19 +71,29 @@ public function getEnabledPlugins(Editor $editor, $include_internal_plugins = FA
+      // Otherwise enable this plugin if it declares itself as enabled.

"Otherwise" suggests this is an "if … else" construct, which it is not. It is possible for a plugin needing to be enabled if a button one of its buttons is enabled *or* when it enables itself based on the context.

Fixed.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/CKEditorPluginManager.phpundefined
@@ -70,19 +71,29 @@ public function getEnabledPlugins(Editor $editor, $include_internal_plugins = FA
+        // Check if this plugin has dependencies that also need to be enabled.
+        $additional_plugins = array_merge($additional_plugins, array_diff($plugin->getDependencies(), $additional_plugins));
       }
     }
 
+    // Add the list of dependent plugins.
+    foreach ($additional_plugins as $plugin_id) {
+      $plugin = $this->createInstance($plugin_id);
+      $enabled_plugins[$plugin_id] = ($plugin->isInternal()) ? NULL : $plugin->getFile();

"additional plugins" bears no relation to "dependencies", so I suggest renaming it to "depended plugins".

More importantly: this fails to check if a depended plugin is in fact already in the list of enabled. I suspect that might have been intentional though, because the foreach below just updates the list of enabled plugins, so enabling the same plugin twice is idempotent.

Which begs the question: why have the list of additional plugins at all, if you can just update the list of enabled plugins right away?

This also needs test coverage.

+++ b/core/modules/editor/editor.moduleundefined
@@ -79,6 +98,22 @@ function editor_library_info() {
+  // This library ensures that the Drupal AJAX  and jQuery UI libraries are
+  // loaded for dialogs. The JS/CSS for dialogs is included directly in the
+  // drupal.editor library.

Why? Why not have the CSS/JS for dialogs be part of this library?

I couldn't for the life of me get IMG tags to be allowed by using allowedContent/requiredContent. I'm either doing something wrong or it's not working properly in our current build of CKEditor in core. For now I've had to disable the content filter by setting allowedContent = true in our global configuration.

We should have docs ready soon after the release for the data and features activation (allowedContent/requiredContent). In the meantime, let me explain just the most important parts:

allowedContent: 'img[class,id,lang,longdesc,title]',
requiredContent: 'img[alt,src,width,height,class]',

This definition means that a feature (a command in this case) makes it possible to insert the following things:
allowedContent: 'img[class,id,lang,longdesc,title]'
but, at the same time, one line below, the feature expects a CKEditor configuration, where the following content must be allowed at least (in order to "enable itself"):
requiredContent: 'img[alt,src,width,height,class]',

As you can see, there is nothing that would declare the following required attributes: 'alt,src,width,height' in the "allowed content" part.

The following should do the trick instead:

allowedContent: 'img[alt,src,width,height,class,id,lang,longdesc,title]',
requiredContent: 'img[alt,src,width,height,class]',

(allowedContent most of the time should define everything that a feature requires)

In the future version of CKEditor it will be possible to mark a specific attribute as obligatory (http://dev.ckeditor.com/ticket/10006).
So, the proper definition of content generated by a feature (and at the same time, content allowed in CKEditor), would be:

allowedContent: 'img[alt,!src,width,height,class,id,lang,longdesc,title]',

(the "src" attribute is obligatory ("!src") as img without it has no sense at all).
However, the current version of CKEditor included in Drupal does not support it yet.

Btw. It looks like the plugin expects that an image is selected:

plugin.js, line 37
TypeError: imageDOMElement is null
for (var key = 0; key < imageDOMElement.attributes.length; key++) {

(it is impossible to insert a new image)

It's very bad for front-end performance that this whole approach implies we're making a round trip to the server for every dialog. Besides the additional load on the server, the latency cost incurred cannot be ignored, especially on mobile devices.

I think you may be overstating this concern. Every CMS I've seen that truly integrates the image and link handling does so with a server-side request. Try WordPress or Confluence for example. Additionally if we would like to support file uploading, we'll probably do so through the #type = 'managed_file' element. It's also likely that we'd want to do server-side processing if we add any image manipulation abilities. If we want to provide a image browser or link browser via Views, it needs to come from the server side. There are dozens of things that can be done server-side that we can't do in a JS-only dialog. Even with a slight delay, the flexibility is worth the server-side request.

Note that this approach also does not yet address non-Drupal CKEditor plugins: they would still use CKEditor dialogs. It's fine if it that's still a TODO, but I just want to call it out explicitly.

Right, I think I mentioned that in an earlier issue, but I have no intention of entirely replacing CKEditor dialogs. I'm only planing on replacing dialogs for link and image handling. Other plugins that utilize CKEditor dialogs would continue to do so, though for most configurations the dialog plugin would not be necessary.

For some reason, it shows an error message for a pre-existing image: "URL cannot be longer than 128 characters but is currently 172 characters long." — but it only shows this *after* clicking "save", and the only if you start editing the same image *again*.

I think 128 characters is the Drupal default #max_length property. It sounds like validation error messages aren't showing up inside the dialog upon submit.

+ $module_path . '/js/ckeditor.js' => array('group' => JS_DEFAULT),
Why set this explicitly?

I'm not sure why (another core bug perhaps?) but 'group' => JS_DEFAULT places the JS lower in the load order. Without it, ajax.js loads AFTER ckeditor.js, but we need it to load before because ckeditor.js now contains a few AJAX command actions.

+ format: format.format
Why?

The image dialog path includes the text format name. Previously editors did not have any way of telling which text format they were using. Now that they do, CKEditor uses this to open the image dialog at the correct path. The image dialog itself does not yet use the $format object, but it could use it to modify its behavior. A good example (which I think we should include in this patch) would be to throw a validation error if the image SRC is not a local image and the same-domain image filter is enabled.

-abstract class CKEditorPluginBase extends PluginBase implements CKEditorPluginInterface, CKEditorPluginButtonsInterface {
The job of the plugin base class is to simplify the 80% use case.

By removing CKEditorPluginButtonsInterface, that is no longer true.

I changed this while the "drupaldialog" plugin still existed. Because the drupaldialog plugin did not provide a button, I found it couldn't use the base class and had to re-implement all the base class methods (that all returned empty values). This seemed really silly to me that our base class wasn't actually usable for all plugins. Even though the drupaldialog plugin isn't included here any more, my actual attempt at using our existing base classes revealed that the class structure was too presumptuous. We can roll back these changes since their no longer necessary for this patch, but I would prefer to keep them based on my experience implementing a few plugins with them in place.

+ function getConfig(Editor $editor) {
+ return array();
Why provide a default implementation of this method?

Not all plugins return a configuration. Some times a plugin is either just enabled or disabled and doesn't have options. So returning an empty array is a reasonable default.

+ public function getLibraries(Editor $editor);
IMO the Editor $editor parameter should be removed.

I don't think it applies to the drupalimage plugin, but in theory a plugin may only depend on a particular library based on the plugin's configuration. Passing it in is trivial and I think it makes sense to keep it.

If there is a valid use case to keep it, then it should also exist for getDependencies() (and it should be documented in the docblock).

True, sounds good to me.

+ // This library ensures that the Drupal AJAX and jQuery UI libraries are
+ // loaded for dialogs. The JS/CSS for dialogs is included directly in the
+ // drupal.editor library.
Why? Why not have the CSS/JS for dialogs be part of this library?

We could separate the the dialog support into new JS/CSS files. The amount of CSS and JS needed for dialogs (currently) is minimal. It seemed a waste of requests to separate the two.

@wwalc: Ahhhh thanks for that explanation. I did not understand how requiredContent and allowedContent were interacting.

plugin.js, line 37
TypeError: imageDOMElement is null
for (var key = 0; key < imageDOMElement.attributes.length; key++) {

Doh. We can fix that easily by simply doing a check if imageDOMElement exists. Obviously if the element doesn't exist yet, we don't have any defaults to send to Drupal.

Addressing remaining feedback (and revising some of my earlier statements) in this patch.

+ format.editorSettings.drupal = {
+ format: format.format
Why?

(If a certain plugin wants to know this information, it, it's up to that plugin's getConfig() implementation to pass it to the plugin as a JS setting.)

You're right. I thought passing in the format would be helpful here because I was using it in the dialog URL and figured other plugins could do the same, but then I realized my URL wouldn't work without Clean URLs enabled, so that whole idea is out. This new approach passes in a setting like other CKEditor plugins. The name of my variable seems strange at first, it's drupalImage_dialogUrl, which seems crazy but it's the same format as other CKEditor variables. It's standard to use pluginName_settingName, i.e. scayt_srcUrl (see http://docs.cksource.com/ckeditor_api/symbols/CKEDITOR.config.html). I'm not sure how important this is, users will never see this setting so perhaps consistency with other plugins is not important.

- Double-clicking the image to open the dialog does not work.
- Modal title: "Insert/Edit Image" should read "Insert image" when inserting and "Add image" when adding.
- Width & height form field labels should probably be inline with the actual form fields. They both should be on the same line, too.

All fixed. Open to suggestions on making width x height fields look prettier. We should probably have some consistency with the configuration of dimensions on image fields (though image field uses the wrong terminology "image resolution", which I've avoided).

For some reason, it shows an error message for a pre-existing image: "URL cannot be longer than 128 characters but is currently 172 characters long." — but it only shows this *after* clicking "save", and the only if you start editing the same image *again*.

Validation now works properly. I made URL required (as it should be) so you can test validation errors more easily. I increased the URL and Alt title length from the Drupal textfield default of 128 to 2048 (which is the max allowed URL length for IE8 apparently).

I also specified the types of all arguments to the new functions, fixed the insertion of new images, and rolled back the allowedContent = TRUE setting now that I know how the allowedContent/requiredContent properties work within plugins.

Additional test coverage still necessary, though I'm not sure how we'll test most of the modal dialog functionality. We can still test new dependency systems.

Fixes a few tests.

Gahh. Removing binary image.png file. :P

Fixing exceptions.

Just wanted to note that the bugfix this patch includes (as noted in #19) is now no longer at #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), but at #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended instead, now with full test coverage.

Review of reroll soonish.

I've made a followup issue that elaborates on my plans for image handling, which provides some explanation for why this is a server-side request: #1932652: Add image uploading to WYSIWYGs through editor.module

#29:

RE: "I think you may be overstating this concern."
And I think you're understating it :)

Additionally if we would like to support file uploading, we'll probably do so through the #type = 'managed_file' element. It's also likely that we'd want to do server-side processing if we add any image manipulation abilities. If we want to provide a image browser or link browser via Views, it needs to come from the server side. There are dozens of things that can be done server-side that we can't do in a JS-only dialog.

Absolutely!

My concern is this: "Even with a slight delay, the flexibility is worth the server-side request." — it should read like this: "Even with a slight delay, the flexibility is worth the server-side request for some actions."

It's fine to use the server to fill to render the contents of the dialog — when it is absolutely necessary to do so (or would otherwise prevent us from using Drupal's server side APIs). It does not make sense to have this requirement for every dialog. That's all I'm saying.

Existing CKEditor dialogs don't require a round trip to the server. Currently, the direction of this issue is to have every dialog being rendered/served by the server. If we override some dialogs to be server-side instead of client-side generated, that's fine, as long as we also explicitly, from the ground up, allow for client-side generated dialogs.

AFAICT, this patch can never be RTBC unless it also covers at least one 100% client-side dialog. For example: the "insert symbol" one.

Also, I'm not sure if you're aware of this, but dialogs in CKE 4.2 will e.g. automatically remove the "Alternative text" dialog form item if no alt attribute is allowed on <img> tags. I.e., the dialogs adapt automatically. This should continue to work for Drupal server-side rendered dialogs AFAICT.

RE: "I have no intention of entirely replacing CKEditor dialogs. I'm only planing on replacing dialogs for link and image handling."
It was my understanding that we wanted all dialogs triggered by CKEditor to 1) have the same look and feel, 2) work the same from an a11y POV. For th

Without it, ajax.js loads AFTER ckeditor.js, but we need it to load before because ckeditor.js now contains a few AJAX command actions.

You meant editor.js instead of ckeditor.js.
The correct solution, then, is to declare the array('system', 'drupal.ajax') library as a dependency of the drupal.editor library. This will guarantee the correct load order.

However, editor.js logically does not depend on drupal.ajax. It's only because you made the drupal.editor-dialog library not have any CSS/JS files. Your reasoning for that was this:

The amount of CSS and JS needed for dialogs (currently) is minimal. It seemed a waste of requests to separate the two.

But that's wrong: that's why we have CSS aggregation, so we can logically separate things, and then use only what's necessary. Especially if you're going to introduce bogus dependencies just for the sake of having it all in one file, it seems like you're pushing it too far.

So I think this can be completely solved by just using "proper" libraries, and not "pseudo" libraries.
I did all of the above, and … it's still not working. Something's wrong with Drupal's JS dependency handling here. I briefly talked to nod_ and he says this is caused by hardcoding weights for some libraries. drupal.ajax is one of those. By removing the manual weight setting, it works. In theory, it should be safe to remove this weight now that we have dependency handling. Sure enough, it works in the case encountered by this patch.
So… I'm trying to do just that — let's see which tests fail (hopefully none!)…

RE: modifying CKEditorPluginBase
I disagree. The base class shouldn't have to cater for every use case, it should cater for the typical use case. And, as you already indicate, the current DrupalImage CKEditor plugin is actually again proof that it's the typical use case, because that plugin also implements the CKEditorPluginButtonsInterface interface. I'd like to see this reverted.
If we want to change that, we should do it in a separate issue, because changing that is completely irrelevant to the issue at hand.

RE: default getConfig() implementation
Indeed, not all plugins return editor configuration. But I think that by not implementing this in the base implementation, we encourage developers to consciously think about this. As I said: this default implementation was omitted intentionally.
I don't feel not nearly as strongly about this as I do about CKEditorPluginBase though. This is reasonable, you just follow a different train of thought :)

#30: AWESOME changes!

It's fine to use the server to fill to render the contents of the dialog — when it is absolutely necessary to do so (or would otherwise prevent us from using Drupal's server side APIs). It does not make sense to have this requirement for every dialog. That's all I'm saying.

So looking to existing systems for examples again, other systems are currently using *both* a custom dialog system that integrates with the CMS and the modal system that comes with their WYSIWYG library (WordPress I know for sure, I *think* this is the case for Confluence also, but I'll need to set up another demo site to check).

I think you're right that doing a round-trip for special symbols or tables doesn't make any sense, but either attempting to make a CKEditor-compatible modal system or replacing the existing plugins so they use a different dialog system is a lot of work (and duplicated work at that) and seems like it would be prone to bugginess.

To quote @FredCK in in #7:

- Easier (do not confuse with "easy") / limited: Write independent dialogs that are simply opened by the editor but using their own logic. Still not all dialogs will be covered, just the implemented ones.

- Harder / complete: replace the "rendering layer" of the dialog system of CKEditor (dialog plugin) with a custom system, which makes use of the "dialog definitions" we have in CKEditor. In this way, all dialogs will be covered, including plugin contributed ones.

We have never worked on any of the above solutions, so it is still unknown how to make them happen and how easy/hard it may be. I'll certainly be happy to have it investigated. Still after CKEditor 4.1.

I have been pursuing the "Easier" route. You might notice in the existing patch that we don't even use/provide a dialog plugin for CKEditor. The dialog support is all directly in editor.js. We'd need to keep this integration even if we make a replacement for CKEditor's modals. The existing modal code is only a few hundred lines. If we were to attempt to replicate the rendering layer of CKEditor's modal system, we'll be looking at thousands of lines in addition to the current approach.

AFAICT, this patch can never be RTBC unless it also covers at least one 100% client-side dialog. For example: the "insert symbol" one.

Perhaps we need to rescope this issue? Replacing the CKEditor dialogs entirely is going to be a large undertaking and it's going to be in addition to everything we've done here. As far as I can see, there is almost no overlap between the code for handling server-side dialogs and code for handling client-side dialogs (jQuery UI handles the entire client-side functionality in the current patch). In the default configuration of an editor, the only modals necessary are links and images, so this approach would have consistent presentation for the basic editor. Tables and symbols are the built-in dialog-dependent plugins we're using currently. What I'm proposing as a interim solution (read: probably for the D8 cycle) is to continue using the CKEditor dialogs for client-side forms.

RE: modifying CKEditorPluginBase

Fine we can roll back these changes. I don't care strongly enough.

So looking to existing systems for examples again, other systems are currently using *both* a custom dialog system that integrates with the CMS and the modal system that comes with their WYSIWYG library

To clarify, WordPress uses jQuery UI modals for link handling only. It oddly uses tinyMCE modals with an iframe for image handling. Special symbol dialog it uses the default tinyMCE dialog. They're styled so that they look similar with CSS.

#38/#39: Agreed on all counts. Identical superficial looks are IMHO fine — in my personal, humble opinion we don't have to override CKEditor's dialogs at all. I think the whole reason for that plan of attack (and the original purpose of this issue) was simply the need/want for consistency. I think it's fair to create a new issue with the current issue title, and rename this issue to something like "Add the ability to insert images in CKEditor in a Drupal-optimized way".
You're absolutely right that it's a massive undertaking to go the "harder/complete" route as FredCK put it, with very small gains.

Reroll now that #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended is fixed. We no longer need to bundle that patch's fixes to get tests passing.

This patch also reverts the base class changes. I'm still positive that the class arrangement is totally whack. The "CKEditorPluginBase" isn't a base plugin for CKEditor, it's a base plugin for "CKEditor plugins that provide buttons". Hence, not a base class for all plugins like its name implies. With the two additional methods added to CKEditorPluginInterface, this means that a plugin that doesn't provide a button now needs to provide FOUR methods that have a high probability of returning nothing but an empty or obviously default value: isInternal(), getDependencies(), getLibraries(), and getConfig() (See the Llama test plugin for an example, which does exactly this). This will affect all plugins that simply want an on/off toggle, such as plugins that might provide matching filters or plugins that provide one particular piece of functionality (such as fixing the image resize handles or enhancing a single particular existing plugin). Anyway, the changes that prevent such an unfortunate class structure have been reverted, despite the fact that the first time I wrote a plugin its exactly what I encountered. </rant>.

On the plus side it makes for less changes. Now that I got that out of my system we can keep rolling along.

Coming here from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases during #sprintweekend.

I rerolled the patch in #41 because it didn't apply anymore (Drupal\Core\Annotation\Plugin -> Drupal\Component\Annotation\Plugin) as well as an errant debugger-Statement and a missing image.

Would love to help w/ bringing together the different implemetations - currently views.module, editor.module and the patch in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases have considerable overlaps.

Played with this last patch in SimplyTest.me. I found some issues with the images in CKEditor. I could drag images I'd uploaded right into the WYSIWYG, but they wouldn't show up.

That's not a problem with the dialogs though. Those seemed to work just fine.

I could drag images I'd uploaded right into the WYSIWYG, but they wouldn't show up.

CKEditor doesn't have any native handling for drag and drop uploading, we have to write this integration manually. Though #1932652: Add image uploading to WYSIWYGs through editor.module won't handle drag and drop uploads yet either, that's were we'll start managing the uploading of files through WYSIWYG (starting with just a normal upload field and then expanding it later, hopefully to include paste from clipboard as well as drag and drop).

Would love to help w/ bringing together the different implemetations - currently views.module, editor.module and the patch in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases have considerable overlaps.

Thanks @frega! I'll give this a review shortly. I think the latest patches could indeed help consolidate code a little bit, though using the Drupal dialog system will of course add another JS file to the page... such is how it goes.

Besides quicksketch' review of frega's work in #42, what else needs to happen here? I.e. is this ready for final reviews, or are there things that still need to happen? And now that #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases has landed, do we still need the custom AJAX commands?

We were basically stuck while #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases was getting done. We can now remove the CKEditor-specific commands for handling dialogs and use the actions provided by core. The current approach currently necessitates loading jQuery UI at page-load time, but with the new core commands, we'll want to make sure that jQuery UI loaded on-demand the first time a dialog is needed. I'm kind of side-tracked by #1851414: Convert Views to use the abstracted dialog modal right now too, which will help the styling of diologs everywhere in core, but that's not a requirement for moving forward here.

Is that blocker now gone?

This one seems to be fixed #1870764-105: Add an ajax command which makes it easy to use the dialog API in complex cases but the other one isn't. Bump.

#42: ckeditor_image_dialog-1879120-42.patch queued for re-testing.

@quicksketch: Do you have any idea when you can/will continue working on this, if at all?

Yeah I'd like to pick this up and keep working on it. Sorry I was completely derailed first by the modal abstractions and then by trying to fight PSR-0 in another issue. At this point we're in good position to integrate modals into CKEditor using the new systems and gut the duplicate code from this patch.

No problem, and thanks for your awesome work on this :)

I'm working on rerolling this patch, but several other issues are currently working in the same space that need to be finished first. So far these are:

#1992744: Add a dedicated @Editor plugin annotation
#1985528: Convert filter_tips_long() to Controller

Both are currently RTBC, so hopefully they'll be in by the time I get a working patch up.

Another dependent issue that @larowlan pointed out to me: #1842036: [META] Convert all confirm forms to use modal dialog

Apparently using a _form router item in a dialog has some glitches that are fixed in that issue.

Yay! :) Following those issues. Ping me whenever you need reviews!

Here's an update just to show some progress. We are now fully Drupal 8 new-school, APIzed. This new patch uses the new router system for all the callbacks, and FormInterface _form router callbacks, while utilizing the new Dialog API. Yes, that's right, THREE new APIs since the last version.

This patch still needs work, but it will at least provide a button, open a dialog, accept values and insert/update an image. Yay! The good news is this seems like quite a bit less code than the previous versions, since we're leveraging the dialog API for everything except sending the values back to the editor.

I want to incorporate the link dialog while we're in here too. I'll continue revising this in a followup.

Oh, this patch depends on ALL the 3 issues listed above. So we're going to be stuck until all of those land.

Yay :) Nice work! Your work on improving the dialog API etc. has been fruitful, then!

Here's a patch that could use some review. Unfortunately it still requires all 3 of the above patches:

#1992744: Add a dedicated @Editor plugin annotation
#1985528: Convert filter_tips_long() to Controller
#1842036: [META] Convert all confirm forms to use modal dialog

This patch incorporates a fair bit of the changes recommended in #1851414: Convert Views to use the abstracted dialog modal, including the auto-resize code and some standardization of our CSS for dialogs. Since that issue is more difficult to complete, it'd be nice to make those improvements here so the Views patch becomes more straightforward.

This patch includes Drupal-based replacements for both links and images. Unfortunately 3/4 of the normal link plugin is handling for named anchors. However because that code is based around fakeobjects instead of widgets, I didn't think porting that functionality was a worthwhile effort since widgets will be replacing fakeobjects. When widgets are part of our distribution, we should re-add named anchor support using that new API. Sound feasible?

Screenshots:

Dialogs are loaded server-side. After a short delay, if the dialog is taking a long time this loading throbber slides down from the toolbar:

Here's the image dialog:

And the link dialog:

The dialogs currently resize and automatically center themselves after this patch. On mobile, it works okay but still needs some work.

Obviously these patches are not the final product, but I think it's a solid starting point and as far as we should take the first round. Things like file support will be a significant undertaking and I've outlined a proposal for it over here: #1932652: Add image uploading to WYSIWYGs through editor.module. Things like a recent-image browser or link browser are also followups, but this lays the groundwork for making those easily with minimal JS work.

Wow, that definitely looks like a solid starting point! Your plan is to get those three other patches committed first and then work on this one? I.e. this is the final scope for this issue? That'd be fine by me, just asking :)

Let's open a new issue for making the dialog controller work with form routes, I'd bundled the fix with #1842036: [META] Convert all confirm forms to use modal dialog but that fix need not hold this up

Your plan is to get those three other patches committed first and then work on this one? I.e. this is the final scope for this issue?

Yep, some followups are:
- Add a file-upload field to the image dialog.
- Add alignment to the image dialog.
- Add captions to the image dialog.
- Add a link browser (i.e. a View with a search box) to the link dialog.
- Add a file browser (i.e. a View of thumbnails) to the image dialog.
- Add named anchor support using CKEditor widgets (instead of fakeobjects)

So, lots of work to be done. However as far as this issue, I'd like to just get the current functionality polished and make sure the implementation is satisfactory.

Using core dialogs means we will need to alter the buttons to use dropbuttons if we're going to solve #1831894: Users miss "save" button and can't distinguish "editable" and "preview" areas by hiding the "apply to this display/apply to all displays" by moving the choice to the confirmation.

Using core dialogs means we will need to alter the buttons to use dropbuttons

Hmm, this could be a significant problem. jQuery UI has some strange ways of providing buttons. We don't have full control over the buttons within a dialog because they have to be provided as configuration to the $.dialog() method. See this documentation on setting the buttons option. Using the jQuery UI buttons is necessary for properly handling resizing the dialog and accessibility (I think). So in short, making the dialogs use drop buttons is going to be difficult.

@jessebeach: However I'm not sure how any of this applies to CKEditor. We don't need a drop-button there. Did you mean to post your comment to #1851414: Convert Views to use the abstracted dialog modal?

The dependent issues other than #1842036: [META] Convert all confirm forms to use modal dialog have now been committed. It looks like that issue is getting held-up, so for the time being, I think I'll simply not use the "_form" approach to my router controllers, instead just using "_content" and return the AJAX-commands directly. In this situation, we don't need to provide non-JS versions of these dialogs. I was using some work-arounds anyway (such as setting the dialog title through JS), that would be avoided using a _content handler.

@quicksketch, please see #1998698: Allow Dialog Controller to work with form/entity form routes instead of #1842036: [META] Convert all confirm forms to use modal dialog much closer there.
Some fails from the dialog tests and needs some new tests, should have time to work on it tonight.

re: #66, I think I did mean to post that to #1851414: Convert Views to use the abstracted dialog modal.

I'm trying to review this patch here, but having trouble understanding what it does, exactly. I mean, could someone who has worked on it provide context in the summary? What is the general theme of the changes? Many files are touched. Are we giving up any functionality or flexibility with these changes? Should I look at some of the files in particular for more focused review? Any guidance will help me review and get us to RTBC faster!

@jessebeach: Thanks, the issue summary is indeed woefully inadequate. I revised it with the standard template.

@quicksketch, awesome write-up, thx!

@quicksketch, awesome write-up, thx!

So looks like everything is going smoothly after the inclusion of #1992744: Add a dedicated @Editor plugin annotation and #1985528: Convert filter_tips_long() to Controller. I rerolled this patch with additional testing on mobile, and now this patch works significantly better on iOS, avoiding unnecessary zooming-in and preventing scrolling. There are still some shortcomings on mobile... opening the dialogs to edit existing images/links is particularly challenging.

Patch still depends on #1998698: Allow Dialog Controller to work with form/entity form routes, on which @larowlan is making progress.

Not too keen on some changes to dialog.js

The default for autoResize should be set at the top in drupalSettings.dialog.
only need to check for settings.autoResize === false, no need for the string version.

And actually I think the whole autoresize thing needs to be in a separate file or at least separate part of the code, that's why we have dialog events triggered. I have a problem making autoResize the default, the default should be a draggable and resizable dialog that doesn't autoresize.

I'm torn about the buttons as well. In any case the button selector is a problem, it'll catch the show/hide row weight and responsive table buttons. Since the form actions are only inputs (at least in core) we should only select them.

it'll catch the show/hide row weight and responsive table buttons. Since the form actions are only inputs (at least in core) we should only select them.

I hadn't meant to include just "button" in the selector, it's supposed to find only buttons within the .form-actions wrapper in all cases:

.form-actions input[type=submit], .form-actions button

But you're right we don't ever have a button element inside of form-actions anyway, it can probably just be dropped.

only need to check for settings.autoResize === false, no need for the string version.

This was added out of necessity after finding that using the data-dialog-options attribute, which sends the request to the server, and then returns it back to the client as a JSON string. For some reason all data returns as strings... not sure why that happens.

And actually I think the whole autoresize thing needs to be in a separate file or at least separate part of the code, that's why we have dialog events triggered

We can move it to the contents of an event, I think that'd be fine. But the ability to keep a dialog centered on the page when resizing (or scrolling), should be a centrally provided option. It doesn't make sense to separate it out into a different file than dialog.js, since it's a small amount of code that I think most implementations will desire, including most (or all) core implementations: confirm dialogs, config diffs, editors, and Views.

@quicksketch: What do you need here to move forward? It depends on #1998698: Allow Dialog Controller to work with form/entity form routes, but does it make sense to fully review this patch already as-is?

P.S.: a very quick glance at the patch shows one thing that can be improved easily: the images contain a bunch of metadata. Throw them through http://imageoptim.com/ and they'll be optimized :)

Not sure where came those images (GIFs), but it may violate copyright if metadata is blindly removed.

I'm assuming quicksketch created those himself and is hence putting them under the GPL2 license that d.o requires. Patches are implied to be GPL2. The patch poster is responsible for ensuring the patches are GPL2 compatible and don't carry copyright restrictions.

Stripping metadata is valid then. It's unacceptable for any image file in Drupal core to include unnecessary metadata.

Not sure where came those images (GIFs), but it may violate copyright if metadata is blindly removed.

The images are from CKEditor's repository, which is GPL, LGPL and MPL.

@quicksketch: please see the first part of #76 — how can I help? :)

@WimLeers: This patch could use an extensive review, yes. I'd love a full review, considering it looks like #1998698: Allow Dialog Controller to work with form/entity form routes is going to continue being held-up. I tried to convert this patch to just use _content handlers instead of the new _form handlers, but that approach didn't seem like it worked either. I kept getting 406 Not Acceptable errors when I tried to make a dialog request to the _content handler. If possible, I'd like to short-circuit the wait on that other issue and just wrap a _content handler around the call to drupal_get_form(), since _content handlers are supposed to work with any dialog AJAX request.

Here's a straight reroll of #73. Two hunks no longer applied. And one small change was required to make the dialogs work properly, because of #2019481: JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides.

NR because #1998698: Allow Dialog Controller to work with form/entity form routes got committed.

Review

Changes

• Fixed JSHint errors.
• Drupal.ckeditor.openDialog actually generates and clicks a link to use Drupal.ajax. This is unnecessary; it can just generate an internal event and trigger that event. What's worse, this link is visible and can be clicked by the user, which causes the page to be reloaded: this is wrong. So, fixed the use of Drupal.ajax.
• Nitpick: the "Loading…" message was selectable by the user. If the user double clicks the link or image he wants to edit, then it would be selected. I added a bit of CSS that prevents that.
• Nitpick: optimized the icon for the Link button.
• Keyboard shortcut for "link" is CTRL+L, but it should be CTRL+K, like it already is being configured for CKE's link plugin.

Remarks

• drupalimage/plugin.js:
  

  allowedContent: 'img[alt,src,width,height,class,id,lang,longdesc,title]'
  

  We don't allow class, id, lang, longdesc or title to be set via our dialog, so why mark them as allowed? A problem here is that the form may be altered by contrib modules to allow for more attributes to set (for example the ones I just pointed out that are marked as allowed despite the user not being able to set them via the dialog), but that's a server-side thing. So, this setting should ideally be generated based on metadata of the altered resulting end form.
  Analogously for drupallink/plugin.js.

• Ideally, the "enter" button would also always press save no matter in which field you are for the "image" dialog, just like for the "link" dialog. Or is there a specific reason to not do this?

• You're right. I thought passing in the format would be helpful here because I was using it in the dialog URL and figured other plugins could do the same, but then I realized my URL wouldn't work without Clean URLs enabled, so that whole idea is out.

  No, you were right :) We have Drupal.url() in JS now to deal with this. Please restore your original behavior!

Note: I'm working on integrating this with CKEditor Widgets. Unless Nate wants to do that here, I think it's better to have the code for that be in a follow-up.

This issue should be very close to RTBC.

Has there been any progress made in regards to integrating a file upload field to the image dialog? Be gentle, I'm just curious Drupal fan that has been looking forward to this feature for quite a while.

#87: that's in #1932652: Add image uploading to WYSIWYGs through editor.module :)

And now with fixed test coverage: not only to fix the easy superficial problems in #84, but also to actually test the DrupalImage and DrupalLink plugins being configured as the default ones.

#1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings got committed, hence #89 didn't apply. Straight reroll.

… with the exception of additional changes in CKEditorPluginManagerTest.php, where I had apparently not run the tests again after the latest changes I made for #89. See interdiff.

+++ b/core/misc/dialog.jsundefined
@@ -32,11 +36,43 @@ Drupal.dialog = function (element, options) {
+    $element.dialog('option', adjustedOptions);

I'm pretty sure nod_ wanted to keep dialog.js separate from jquery.ui.dialog, should we use a trigger and callback instead?

Tagging for JS team.

Took this for a spin on simplytest.me, it works great!

I've given the code a quick read-over and it looks very solid.

Will come back for a thorough review shortly.

More accurate title :)

A bunch of unrelated fails, but also CKEditorAdminTest, which I did not run the tests for. Stupid :) That'll be a trivial fix though.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.phpundefined
@@ -192,12 +194,25 @@ public function getJSSettings(EditorEntity $editor) {
+   * Implements \Drupal\editor\Plugin\EditorPluginInterface::getLibraries().

+++ b/core/modules/ckeditor/tests/modules/lib/Drupal/ckeditor_test/Plugin/CKEditorPlugin/Llama.phpundefined
@@ -33,6 +33,20 @@
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginInterface::getDependencies().
...
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginInterface::getLibraries().

@@ -47,7 +61,7 @@ function getFile() {
+   * Implements \Drupal\ckeditor\Plugin\CKEditorPluginInterface::getConfig().

should be {@inheritdoc}

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.phpundefined
@@ -192,12 +194,25 @@ public function getJSSettings(EditorEntity $editor) {
+    $manager = drupal_container()->get('plugin.manager.ckeditor.plugin');

We should switch the plugin manager factory to use ContainerFactory so we can inject the ckeditor plugin manager service into the Editor plugin. See https://drupal.org/node/2012118, regardless drupal_container() is deprecated, even more so in OO code.

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.phpundefined
@@ -0,0 +1,136 @@
+    return 'editor_image_dialog';
...
+    $form['#prefix'] = '<div id="editor-image-dialog">';

Do these two collide? Won't the form api want to give the form that id too? Assuming drupal_html_id keeps it clean but you may end up with unintended incrementing. At the very least shouldn't this use drupal_html_id?

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorLinkDialog.phpundefined
@@ -0,0 +1,103 @@
+    return 'editor_link_dialog';
...
+    $form['#prefix'] = '<div id="editor-link-dialog">';

Same here

+++ b/core/modules/editor/lib/Drupal/editor/Form/EditorLinkDialog.phpundefined
@@ -0,0 +1,103 @@
+    $form['#suffix'] = '</div>';

Themers really hate #prefix/#suffix, is there any way we can add a #theme and use a html.twig template instead that adds the wrappers?

Themers really hate #prefix/#suffix, is there any way we can add a #theme and use a html.twig template instead that adds the wrappers?

In all cases, this ID is a functional requirement, since it's what our AJAX behaviors are targeting. It would be preferable if this was kept away from themers as much as possible to prevent them from breaking functionality. In any case, using prefix/suffix is very standard when doing AJAX, it's even in our documentation. Not saying it's the best idea, but it's standard anyway. We shouldn't need to use drupal_html_id() either, because if we did have a conflict, the AJAX replace command isn't going to work.

This reroll addresses ALL known feedback. Please review so we can commit this ASAP.

#74: (see interdiff-74.txt)

The default for autoResize should be set at the top in drupalSettings.dialog.
only need to check for settings.autoResize === false, no need for the string version.

First sentence: done.
Second sentence: not done, see #75.

And actually I think the whole autoresize thing needs to be in a separate file or at least separate part of the code, that's why we have dialog events triggered.

I think quicksketch rightfully argued in #75 that it's an essential, basic feature that adds very little code, so it makes sense to have that in dialog.js.

I have a problem making autoResize the default, the default should be a draggable and resizable dialog that doesn't autoresize.

The dialogs remain draggable! I agree that this is important/useful.

They don't remain resizable. Which I think is an utterly useless and annoying feature anyway: why would you want the user waste his time with resizing the dialog? The dialog should just fit the content, period.

I'm torn about the buttons as well.

But you're right we don't ever have a button element inside of form-actions anyway, it can probably just be dropped.

So: dropped them.

#84: (see interdiff-84.txt)

• Fixed the incorrect allowedContent plugin settings.
• This is a problem of Drupal's Dialog, not a problem of this patch.
• Restored drupalSettings.editor.formats[formatName].format = formatName, so now we don't need to pass "dialog URL" settings anymore: we can just do Drupal.url('editor/dialog/image/' + editor.config.drupal.format) to generate it client-side.

#95: tests fixed.

#96: (see interdiff-96.txt)

Do these two collide? Won't the form api want to give the form that id too? Assuming drupal_html_id keeps it clean but you may end up with unintended incrementing. At the very least shouldn't this use drupal_html_id?

Good point! Fixed. Also fixed for EditorLinkDialog.

@quicksketch: it worked just fine, but there was indeed a conflict, of the form

<div id="editor-image-dialog">
  <form id="editor-image-dialog" …>
    …
  </form>
</div>

I just renamed it to editor-image-dialog-form.

Themers really hate #prefix/#suffix, is there any way we can add a #theme and use a html.twig template instead that adds the wrappers?

As quicksketch already says: this div exists for function, not form: to know which part of the DOM to replace in the AJAX commands.

Restored drupalSettings.editor.formats[formatName].format = formatName, so now we don't need to pass "dialog URL" settings anymore: we can just do Drupal.url('editor/dialog/image/' + editor.config.drupal.format) to generate it client-side.

I'm not too put-off by this change, but in general I think passing the URL via a JS setting is a better approach. The JS Drupal.url() function doesn't work with URL aliases or hook_url_alter(). And passing via a JS setting allows it to be altered via hook_js_alter(). Drupal.url() is a convenience, but not one that necessarily works very well. In this case, it doesn't make much difference, but I don't think Drupal.url() is the best approach in all situations; it's just available for convenience.

-      allowedContent: 'img[alt,src,width,height,class,id,lang,longdesc,title]',
+      allowedContent: 'img[alt,src,width,height]',

So if I'm not mistaken, this prevents adding a class, id, etc. to all img tags, right? Shouldn't those be allowed in the input, even if there's no UI for inserting them? I'm concerned that a user that manually types in classes/IDs via the source button, they're going to have them stripped out by the editor JS, even though there's no actual limitation on those things in the PHP filter.

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.phpundefined
@@ -192,12 +194,25 @@ public function getJSSettings(EditorEntity $editor) {
+ $manager = drupal_container()->get('plugin.manager.ckeditor.plugin');

I still think we should avoid using drupal_container() in OO code but happy to make that a follow up - as it requires changing the plugin manager factory too - so that could be the gist of the followup.

This just removes the calls to (now deprecated) drupal_container added in this patch as well as any existing ones in that plugin.

@larowlan: I'd love to see this done the "right" way. If you'd have time to reroll with *just* that change to the factory, I'd like to see how that's done. I know we're not *supposed* to use drupal_container(), but every time it comes up I still haven't figured out how to "fix" it. Since apparently switching from drupal_container() is always a bit of a hassle, I'd like to learn the pattern so I can avoid it in the future.

EDIT: Nevermind, looks like you did exactly this while I was writing the comment. Thanks!!

So if I'm not mistaken, this prevents adding a class, id, etc. to all img tags, right?

Ah, well that would have been the case, but our new changes in #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings override these settings anyway. So basically it doesn't matter what we put in there, because Drupal is overriding the setting anyway.

This patch needs a little work yet:

- Per Wim's suggestion and my own experience, the "Enter" key should close the image dialog like it does for the link dialog.
- The loading throbber isn't working currently, leading to unexplained delays while opening the dialogs. The throbber being added any everything to the page, but it's not sliding into place like it is supposed to.

I'll reroll with these fixes.

Well I thought I was going to reroll it. I'm so depressed by D8 today (like most days) I can't do it. Wim please continue working on the issue.

I don't get those fails locally.

EDIT: Yes I do, digging

So after digging into this with WimLeers it seems as though the fails are because we have a stale container somehow/somewhere.
I say we leave it as per #98 and come back to the best practises later.

meh

Figured out what was going on.
We needed to recreate the ckeditor plugin in the tests after enabling the new module, ie the ckeditor instance in the test is stale, compiled against the old container which didn't include the new module.

#99:

I'm not too put-off by this change, but in general I think passing the URL via a JS setting is a better approach. The JS Drupal.url() function doesn't work with URL aliases or hook_url_alter(). And passing via a JS setting allows it to be altered via hook_js_alter(). Drupal.url() is a convenience, but not one that necessarily works very well. In this case, it doesn't make much difference, but I don't think Drupal.url() is the best approach in all situations; it's just available for convenience.

Agreed. But these URLs are not user-facing, and they're in JS specifically built for this endpoint. So I don't see the harm here. For user-facing URLs, you're absolutely right.
Where I disagree is that a best practice would be to pass URLs via JS settings. This should be avoided whenever possible, because it's already pretty ridiculous how much JSON is being generated for drupalSettings.

So if I'm not mistaken, this prevents adding a class, id, etc. to all img tags, right? Shouldn't those be allowed in the input, even if there's no UI for inserting them? I'm concerned that a user that manually types in classes/IDs via the source button, they're going to have them stripped out by the editor JS, even though there's no actual limitation on those things in the PHP filter.

You're mistaken :)
allowedContent should solely specify what this plugin allows you to define, nothing else. If you can't set the class attribute using the plugin, then it should not be here.
Note that this is only used when ACF is running in its default mode, which is: "hey, all plugins, tell me what content you require and allow!". But, we specifically don't use that anymore in Drupal core, because this can very easily cause a mismatch between what Drupal's text filters allow and what CKEditor's ACF allows. #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings fixed that. If the text format allows a class attribute, then CKEditor will allow it too :)

… you figured this out by #103 already… d'oh.

#102:

I know we're not *supposed* to use drupal_container(), but every time it comes up I still haven't figured out how to "fix" it.

+1! :D

#101, #105: debugging this made me feel like #104. It's a nightmare to debug (#106). Thanks to larowlan's persistence, it got solved in #108, but it's clear that this doesn't really make things more robust.

#103:

- Per Wim's suggestion and my own experience, the "Enter" key should close the image dialog like it does for the link dialog.

This should be done as an improvement to the dialog system, it's not something we need to solve here.

- The loading throbber isn't working currently, leading to unexplained delays while opening the dialogs. The throbber being added any everything to the page, but it's not sliding into place like it is supposed to.

For me, it still looks exactly like you showed earlier in this issue: https://drupal.org/files/dialog-loading.png. Maybe you had a caching issue?

Changes

1. Thanks to #1907418-52: Make CKEditor's alignment + underline buttons available (but not enabled by default), I discovered #2028457: Configuring CKEditor's ACF broke CKEditor configuration -> filter setting syncing, which also affects this issue. Related to that, I noticed that loading of "extra plugins" was broken for the hidden CKEditor instance, so fixed that.
2. I also noticed that DrupalLink and DrupalImage's required content was not being picked up, so I debugged that too. Turns out that both our uses of allowedContent in the plugins was wrong.
3. Renamed the "image" command to "drupalimage", "link" to "drupallink" and "unlink" to "drupalunlink". The convention within CKEditor is for the button name to match the command name. @quicksketch: If you disagree with this change, then we should scrap the "drupal" prefix from the button names.
4. #108: I very much dislike that in some places you use $this->container and in others \Drupal::getContainer(). They should be one and the same. Fixed that. Tests still pass.

…

Renamed the "image" command to "drupalimage", "link" to "drupallink" and "unlink" to "drupalunlink". The convention within CKEditor is for the button name to match the command name. @quicksketch: If you disagree with this change, then we should scrap the "drupal" prefix from the button names.

I think that's fine. I had left them as "image" and "link" because if other plugins wanted to open link/image dialogs I wanted them to be compatible. However it turns out this isn't common and the commands aren't really compatible anyway. Renaming to be Drupal-specific is a good move.

For me, it still looks exactly like you showed earlier in this issue: https://drupal.org/files/dialog-loading.png. Maybe you had a caching issue?

I tried this both locally and Drupalize.me simplytest.me I can't see the progress throbber slide down. I'm using Chrome Mac, latest. Doesn't seem to work in Firefox latest either.

I tried this both locally and simplytest.me I can't see the progress throbber slide down. I'm using Chrome Mac, latest. Doesn't seem to work in Firefox latest either.

Nevermind, it looks like this is actually simplytest.me specific. The countdown timer they use on the page causes window.setTimeout() calls to be queued up. My guess is they're also using timeouts for the timer and the two of them are conflicting. I'd still like to figure out at least *why* the link dialog closes properly with the enter key, but the image dialog does not. Otherwise this is good to go in my eyes.

This patch addresses the following problems:

• Dialogs + In-place editor was causing a problem (see the attached screenshot). The z-indexes of the in-place editor were higher than jQuery UI's dialog rules. Unfortunately because the CSS for the dialog is loaded on-the-fly but jQuery UI is already on the page, we can't set a z-index in the dialog.css file that has any effect unless we use !important. Rather than resort to !important, I simply decreased the z-indexes on the in-place editor. I couldn't find any problems caused by this reduction.
• I fixed the enter-key problem in the image dialog. This was caused by a weird browser behavior rather than any JS we'd added. If you have multiple text fields AND you've hidden the submit button, browsers will not submit the form on enter. However if you only have one text field (like the Link dialog), browsers would submit fine. To solve this, instead of using display: none, I hide the buttons by setting their dimensions to 0. This was the recommendation from http://mattsnider.com/how-forms-submit-when-pressing-enter.
• Also related to fixing enter-key problems, I had to switch the event on the submit buttons from "mousedown" to "click". Since these buttons are hidden anyway, I doubt that will have any significant user-impact.

I gave this another read-through this morning. Looks RTBC to me. Any bold souls want to confirm?

RE: fixing the enter problems: wow!

larowlan cleaned up the PHP. As per #98 all JS feedback was addressed.

RTBC indeed.

The changes in #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute don't affect this patch, so still RTBC.

Committed to 8.x. Thanks.

.

Found two minor bugs so far:

• #2067119: 'width' and 'height' HTML placeholder attributes in image dialog are not translated, yet should be lcfirst
• #2067427: Drupal.editors.ckeditor.attachInlineEditor() should pass Drupal-specific settings like Drupal.editors.ckeditor.attach() does

Revising summary to use standard template.