Update CKEditor library to 4.4

There will first be a beta: http://ckeditor.com/blog/CKEditor-Weekly-for-September-09-2013.

Updated CKEditor package to: https://github.com/cksource/ckeditor-dev/commit/0e3084a8

Patch contains also a prototype of Drupal's captioned image reusing CKEditor's image2 plugin. It replaces current drupalimagecaption which implements part of the image2's logic itself.

Known issue: Icons that Drupal provides for its buttons in CKEditor's toolbar are not displayed.

Hurray, thanks Reinmar! :)

I'm looking into that known issue now and reviewing the changes on the Drupal side :)

Together with Reinmar and w_walc, I debugged the missing icons and we found the cause:

      editor.ui.addButton('DrupalLink', {
        label: Drupal.t('Link'),
        command: 'drupallink',
        icon: this.path.replace(/plugin\.js.*/, 'link.png')
      });

this.path used to be http://yoursite.com/core/modules/ckeditor/js/plugins/drupallink/plugin.js, but now it's just http://yoursite.com/core/modules/ckeditor/js/plugins/drupallink/. Hence the breakage. This is a bug in CKE 4.3, and they will fix it.

I will continue in the mean time with everything else.

Reroll that fixes the icons, and makes them HiDPI-enabled as well.

.

The path has changed because of http://dev.ckeditor.com/ticket/10913. At the beginning we thought that it is a bug, but it turned out to be broken before 10913. The path should not contain a filename and since CKEditor 4.2.2 and 4.3 it will not.

So, to add a button it is now enough to add filename to the path:

      editor.ui.addButton('DrupalLink', {
        label: Drupal.t('Link'),
        command: 'drupallink',
        icon: this.path + 'link.png'
      }); 

#7: great, the patch in #5 already assumes that that is the case, so it will work fine! :)

I'm attaching a full drupalimagecaption plugin.js file with code comments. It's also aligned to the latest changes in CKEditor's image2 plugin.

Note to self: the updated drupalimagecaption plugin.js file works against the latest 4.3, not the 4.3 beta, because bugs have been fixed since the 4.3 beta that we need, so I must update the CKE 4.3 build included in preceding patches, to the latest of https://github.com/cksource/ckeditor-dev/tree/d8, i.e. https://github.com/cksource/ckeditor-dev/commit/aaf6fa42080bf92f38b1be81....

This issue is now blocked on CKEditor.

Reinmar has graciously updated our code to use CKEditor 4.3, and better yet, gotten rid of our custom drupalimagecaption plugin and instead leveraging CKE 4.3's image2 plugin to achieve the same results, and more (e.g. live image resizing).
However, image2 has several significant regressions:

1. the placeholder text is just default text rather than placeholder text
2. not the same class attributes are generated, which means Drupal users wouldn't be able to style an image caption differently from a blockquote caption anymore while in CKEditor
3. it relies on inline styles rather than class-based styling with all the CSS in a CSS file — which is even worse than the preceding point: it's entirely impossible to provide custom caption styling and have that show up while in CKEditor, the image caption inside CKEditor would always look the same, even if the front-end Drupal theme overrides the caption styling and provides CKEditor with that styling as well.

See the "The thing about flexible content styling
" thread in the CKEditor-dev mailing list.

So, for now, postponed.

As per #11.

This does not block #2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images.

4.3 has been released: http://ckeditor.com/blog/CKEditor-4.3-Released.

However, due to #11, we cannot yet start using it.

Where do we go from here?

We wait for the CKE guys to fix this.

Reinmar has graciously updated our code to use CKEditor 4.3, and better yet, gotten rid of our custom drupalimagecaption plugin and instead leveraging CKE 4.3's image2 plugin to achieve the same results, and more (e.g. live image resizing).
However, image2 has several significant regressions:

Would it make sense to update to 4.3 anyway, but still use drupalimagecaption, and then open a new issue for switching to image2?

#16: drupalimagecaption would need to be changed significantly, because the Widget API has been changed significantly. (As far as I know at least.)

Any updates here? Is the CKEditor team aware of the bugs listed in #11, and are they tracked somewhere upstream?

I confirm that CKEditor is aware of bugs listed in #11 :). They are already fixed, but the changes on CKEditor side have not yet been finally reviewed (due to some small pending tasks), so we didn't propose the final solution.

* Ticket on CKEditor's issue tracker: http://dev.ckeditor.com/ticket/11300
* Branch in which we keep the progress of image2 and drupalimagecaption integration: https://github.com/oleq/drupal/tree/ckeditor

Thanks for the update, @Reinmar!

Based on the upstream issue, we will need to update to 4.4 instead of 4.3.

Would it make sense if we would replace our copy in core with a dev snapshot of https://github.com/oleq/drupal/tree/ckeditor, so we can help with testing, and so as to ensure that we won't run into unexpected issues with 4.4?

@sun: we can't just update to 4.4, because Drupal's custom image plugin would break completely due to changes in the Widget API. I've been closely collaborating with the CKEditor developers on this issue. We're removing as much as possible from Drupal's custom image plugin, and relying instead on CKE >=4.3's image2 plugin. The custom plugin will continue to exist, but will be a wrapper that is as thin as possible, causing us to use a Drupal-native dialog (and alterable form) instead of a CKEditor dialog.
For these reasons, it's more than a simple update, it's a collaboration where we (Drupal) help ensure the CKEditor Widget API is as expressive as it needs to be, where we (Drupal + CKEditor) help make the image2 as flexible and generic as necessary, so that Drupal can upgrade to the latest version of CKEditor without breaking the existing image integration.

Hopefully that's sufficiently clarifying.

Over the past few months, the CKEditor team has been steadily chipping away at fixing all of the remaining things for Drupal:

1. The image2 plugin which allows us to remove almost all complexity in our drupalimage and drupalimagecaption plugins for CKEDitor — image2 is also much more complete and much more robust than the code we had (it even supports images in display:inline parents, which was impossible before). All that the drupalimage and drupalimagecaption plugins have to do now, is selectively extending and overriding image2. At a glance:
   1. The "Image" button in Drupal 8's CKEditor corresponds to the drupalimage plugin (just like in current HEAD). The drupalimage plugin extends image2 with two specific things: handling/requiring the data-editor-file-uuid attribute, and integrating with \Drupal\editor\Form\EditorImageDialog instead of CKEditor's own dialog.
      90% of the code in drupalimage is about integrating with Drupal's dialogs, and drupalimagecaption then uses that too.
   2. When you also enable the filter_caption plugin, the drupalimagecaption plugin will be loaded automatically (again, just like in current HEAD). The drupalimagecaption plugin extends the drupalimage plugin, with again two specific things: handling the data-caption & data-align attributes, and a customized upcast to handle upcasting from a plain <img> tag with data- attributes (image2's upcasting happens from a full <figure><img><figcaption></figure> structure, which Drupal doesn't want in its DB).

   This hugely improves the maintainability of those plugins: much, much easier to understand code, and less logic to maintain. Plus, we get to benefit from future improvements to image2 at the same time — one example is the fact that this has made images on-the-fly resizable!

2. #2023629: Remove our CKEditor ACF work-around once ACF supports blacklisting exists to remove the hacks we have in HEAD's CKEditor integration to enforce the blacklisting of style and on* attributes that Drupal's filter_html performs. http://dev.ckeditor.com/ticket/10202 and http://dev.ckeditor.com/ticket/10276 were committed in the last week, so now we can get rid of that hack. And in fact we have to do it as part of this issue, because our existing hack won't work anymore.

Just before posting this patch, I noticed one bug: with the drupalimagecaption plugin (which is enabled by default for the "Basic HTML" text format), it's impossible to insert a new image without a caption. I'm looking into that bug. But I don't want to delay any code reviews.

The CKEditor developers will review the drupalimage and drupalimagecaption code also.

Oh man, this is huge :) I'm glad to see that on balance, the changes in /core/modules show a net decline in lines of code:

12 files changed, 505 insertions(+), 754 deletions(-)

-      this._ACF_HACK_to_support_blacklisted_attributes(element, format);

I'm glad that something so obviously indicated as a hack is being removed from /core/modules/ckeditor/js/ckeditor.js. How was this hack resolved?

-     * This is a huge hack to do ONE thing: to allow Drupal to fully mandate what
-     * CKEditor should allow by setting CKEditor's allowedContent setting. The
-     * problem is that allowedContent only allows for whitelisting, whereas
-     * Drupal's default HTML filtering (the filter_html filter) also blacklists
-     * the "style" and "on*" ("onClick" etc.) attributes.

I assume we can now blacklist content with the CKEditor update? I see changes in generateAllowedContentSetting that seem to suggest this, but now we also have getDefaultDisallowedContentConfig?

+        // Override init(): parse the original data-editor-file-uuid attribute.
+        var originalInit = widgetDefinition.init;
+        widgetDefinition.init = function () {
+          originalInit.call(this);
+          this.setData('data-editor-file-uuid', this.parts.image.getAttribute('data-editor-file-uuid'));
+        };

This is not lovely, but then, that's JavaScript. Would be nice to get callback registration for events, but that's not blocking progress here.

+        widgetDefinition._dataToDialogValues = function (data) {
+          var dialogValues = {};
+          var map = widgetDefinition._mapDataToDialog;
+          Object.keys(widgetDefinition._mapDataToDialog).forEach(function(key) {
+            dialogValues[map[key]] = data[key];
+          });
+          return dialogValues;
+        };

Using ES5 objects and methods worries me here. If this were a straight-up admin-facing feature, it would fit our guidelines. But a site might put a CKEditor format on a comment box and allow images. Now, anyone using IE8 is going to get an error just leaving a comment with an image on the site. I think we need to back off to more common objects and methods.

How was this hack resolved?

See #22, point 2.

I see changes in generateAllowedContentSetting that seem to suggest this, but now we also have getDefaultDisallowedContentConfig?

The latter only exists in a test, to get the default expected value for disallowedContent.
The former is "actual code" and lives in the Internal CKEditor plugin (which represents all plugins that are part of our CKEditor build). It used to be responsible for calling FilterFormat::getHtmlRestrictions() and then generating the allowedContent setting. But now we also need to generate disallowedContent from FilterFormat::getHtmlRestrictions(). Therefore, I've renamed generateAllowedContentSetting() to generateACFSettings(), which now generates both allowedContent and disallowedContent. Since this is a protected method, this does not constitute an API change.

Would be nice to get callback registration for events, but that's not blocking progress here.

Not sure what you mean by this. Could you clarify?

Using ES5 objects and methods worries me here. […] But a site might put a CKEditor format on a comment box and allow images. […]
If that's the case, they're also going to load file.js, which also calls Object.keys(). And node-new-comment-links.js also uses Object.keys().
Using Object.keys() in these situations was decided a long time ago AFAIK. Let's not reopen that bikeshed in this issue.

From #22:

Just before posting this patch, I noticed one bug: with the drupalimagecaption plugin (which is enabled by default for the "Basic HTML" text format), it's impossible to insert a new image without a caption. I'm looking into that bug. But I don't want to delay any code reviews.

This patch fixes that.

It also fixes some JS coding style things. And finally, it removes '#return_value' => TRUE from the hasCaption checkbox, since that leads to 0 or true to be returned, rather than false or true. Better to be consistent then, and just have it return 0 or 1, which is the default.

What an education reading that.

Yes it's a 3M patch but there's only little change needed to use the new hotness. Nothing seems wrong or out of place.

nitpicks in drupalimagecaption:

        // Protected; keys of the widget data to be sent to the Drupal dialog.
        // Append to the values defined by the drupalimage plugin.
        // @see core/modules/ckeditor/js/plugins/drupalimage/plugin.js
        CKEDITOR.tools.extend(widgetDefinition._mapDataToDialog, {
          'align': 'data-align',
          'data-caption': 'data-caption',
          'hasCaption': 'hasCaption',
        });

extra comma.

          var img = findElementByName(element, 'img'),
            caption = this.editables.caption,
            captionHtml = caption && caption.getData(),
            attrs = img.attributes;

we use separate var to declare variables. Comes up in few places.

Would prefer less nesting in the drupalimagecaption code. A few functions don't need to be declared inside other functions.

And about the ES5 things, like wim said, it's good to go. It's the kind of stuff that's easy to polyfill in ie8 module.

Le sigh. The IE8 module is going to be a soft dependency for Drupal 8 then. Not just for this patch given Object.keys in file.js. Well, that ship has sailed.

#29: We can still choose to change this later — it's just out of scope to be discussing it here.

First of all - nice to see all components we developed together during last two years starting to work together :)! But it's a review, so now a darker part :P.

Legend: DI = Drupal image, DIC = Drupal image caption, #Line.

Functional

1. DIC does not upcast non-captioned images (<img> without data-caption). That's because none of the conditions inside DIC's upcast handles this case.
2. Image caption allows some block elements but not <img>. This is very similar to configuring editor to use enter mode BR. This mode is full of unclear cases because it mixes inline content with block content (somehow like MS Word). Setting editor to use this mode is not recommended and I saw that in nested editable it will be handled worse than in normal editable. Therefore I highly recommend making caption a block content container or inline content container. For image2 we chose only inline content.

Code

1. DI+DIC - no need to require widget plugin if you require image2.
2. DI#29-33 - image2 does not use features.image, so I think that this isn't required. Perhaps you confused it with widget definition's requiredContent which is defined below.
3. DI#37 - since DI uses only <img> form (like DIC), it should override allowedContent completely. Otherwise DI will allow figure and figcaption. DIC should override allowedContent too. BTW. This allowedContent should equal this of editdrupalimage command, however, I think that you don't have to set allowedContent for that command at all. The DrupalImage button uses 'image' command which inherits allowedContent from widget, so toolbar configurator should understand that.
4. DI#41 - copying this data attribute can done like in DIC - in the upcast method. Additionally, I think that DI should redefine image2's upcast completely, to upcast only <img> elements in any conditions.
5. DI#54 - downcast doesn't need to return element if it's an "in-place" downcast.
6. DI#168 - there's editdrupalimage command defined, but context menu uses drupalimage... which isn't defined :D. I guess that it's why right click on image throws error (Note: for some reason on OSX it works very rarely in general what was reported in http://dev.ckeditor.com/ticket/11306, but it can be verified on other OSes).
7. DI#184 - shouldn't the 'toolbar' property be defined? Or Drupal does not use the default CKE's toolbar layout resolving at all?

Other

Disallowed content rules do not have to blacklist style attribute, because style and class attributes are handled separately. So if you didn't allow styles in allowed content rules, then you don't need to disallow them.

#32: thanks for the review!

Functional

1. Fixed!
2. Great feedback — thank you. I've followed the lead of CKEditor (only allowing inline content): now restricted to the default set of tags allowed by Xss::filter(), minus the block-level content ones.

Code

1. Done. Also removed the dependency on image2 from DIC, since DIC depends on DI, which depends on image2 already. (I thought that CKEditor did not resolve dependencies, which is why I did things this way.)
2. Oops, you're right — thanks; removed.
3. Fixed.
4. Fixed.
5. Ok, verified and fixed.
6. Oh, that explains it! Fix applied and confirmed in Firefox, now following CKEditor ticket 11306.
7. Correct, Drupal does not use CKEditor's "toolbar layout resolving". (i.e. Drupal 8 never uses "Toolbar groups configuration", it always uses "Item by item configuration".)

Other

Done.

Besides the above, also updated to CKEditor 4.4.0, which was released in the mean time.

Three more things:

• DI and DIC still allows <p> and <div> elements which image2 uses for centering. DI and DIC do not need them so in both cases you can delete allowedContent.p&div
• Drupal filter removes <br />s from the caption. Similarly like in a blockless editor (e.g. editor initialized on <h1> element) in caption enter inserts <br />s. We were analyzing the possibility to totally block enter, but that would require introducing totally new enter mode and was beyond our capabilities. However, I would like to see this feature one day.
• Linking of images goes crazy right now. However, we were polishing some stuff regarding that in 4.4.1, because it was introduced in 4.4.0, so I think that it would make sense to wait a little bit with working on that in Drupal, to avoid too frequent changes.

All the rest looks good.

#34:

1. Great catch; fixed.
2. Right, sorry, this is an oversight from #33. Fixed.
3. Reinmar investigated how to fully disable image2's link support/integration and sent me this snippet of code that does it: https://gist.github.com/Reinmar/84fddafd02bf17bd647c. That's now integrated into drupalimage/plugin.js.

With that, no known problems remain! :) (FYI: JSHint also reports zero errors.)

Can I has RTBC? :) Will be good to finally close this after many, many months!

Perfecto! :D

Wicked, thanks Reinmar!

Given this has sign-off from both Wim (Drupal side) and Reinmar (CKEditor side), I feel comfortable committing this and fixing whatever fallout in subsequent patches. In glancing through the changes outside of core/assets/vendor/ckeditor and .../ckeditor/plugins/ it only seemed like good clean-up that removes a ton of stuff. I did ping nod_ for a review, but wasn't able to catch him. I think though that if there are things that need clean-up after this, it's going to be easier to do in dedicated follow-ups.

Therefore!

Committed and pushed to 8.x. Thanks!

Awesome — glad to finally close this one! This also allowed us to remove the hack work-around we had: #2023629: Remove our CKEditor ACF work-around once ACF supports blacklisting.

The last important step for CKEditor in Drupal 8 UX-wise is #2239419: Include CKEditor's AutoGrow plug-in.

Another important step would be doing sg with CKEditor's Android incompatibility http://ckeditor.com/support/faq/features#question9

OT, but will we upgrade core ckeditor to v5 after D8 is out or is this postponed to D9 than? Android support sounds crucial to me.

Kinda gray area, minor 4.x releases sure. I think it'll depend on how much API change there'll be between 4.x and 5.x. In my mind it'd be possible to do so in 8.x but it really depends on what 5.x looks like.

FYI : #2268941: Removing caption from a previously captioned image fails to remove the caption-related classes

As for the Android support, we made a research recently about CKEditor on iOS and Android. We have been surprised because it turned out that CKEditor works out of the box on Chrome on Android. This is more or less the same browser than on desktop and AFAIK it's preinstalled on some Android devices and is available for most of them.

Of course, the support is not perfect, mostly because of the UX. We are willing to improve this by fixing most important bugs, so we are looking for funding of this task, because it will be very demanding and time consuming. That's about CKEditor 4.

As for CKEditor 5, we plan to have real support for most popular mobile environments. This most likely means having totally separate UI for mobiles and desktops. However, we'll never support every available mobile browsers (especially on Android). We will focus only on most promising ones with good support for contenteditable.

The CKEditor 5 design and development process will be made open soon and we invite the Drupal community to participate in it. So far we worked together on most important CKEditor 4 features introduced recently (like ACF and widgets) and the feedback we got helped in making them better even before release. We are sure that the Drupal community can help us in making CKEditor 5 too :).

Some links:

• Updated FAQ section
• Updated Browser Compatibility Guide
• Results of research on CKEditor support for mobile environments