CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin

Hey I'm able to reproduce this error using additional modules such as media_entity_twitter.

Let me know if you can reproduce it as well.

I see this is happening when there is no range selected within the CKeditor DOM. It means the user did not click inside the editor properly.

This is happening because the getSelection method of the ckeditor cannot get a proper range because the DOM element is kind of locking the edition area:

The source code is:

<drupal-entity data-embed-button="media_twitter_button" data-entity-embed-display="view_mode:media.preview" data-entity-type="media" data-entity-uuid="40f58ee8-9e0e-4cb9-8e00-5861500aa252"></drupal-entity>

So in Editor Mode, you need to use special break lines clicking in the red arrows of the Ckeditor UI to make a "focus" within the editor area.

Steps to reproduce it:

1. Edit a node and embed an entity in the ckeditor
2. Save the node.
3. Edit the node and hit directly the embed button without clicking the CKeditor edition area.
4. Clicking the submit button in the final step you will get the error in the console.

I was tested and debug a bit this bug and i can confirm conclusions from #4, if we don't have focus to editable area of CKEditor on final submit step we will have error (I tested in firefox and chrome, and error appears just in chrome). There is some reported bugs for that error in CKEditor, and this one maybe is related to problem we mention here https://dev.ckeditor.com/ticket/13620.

Confirmed.

In Drupal core's drupalimage CKEditor plugin, we have this:

        widgetDefinition._createDialogSaveCallback = function (editor, widget) {
          return function (dialogReturnValues) {
            var firstEdit = !widget.ready;

            if (!firstEdit) {
              widget.focus();
            }
…
            var container = widget.wrapper.getParent(true);
            var image = widget.parts.image;

            var data = widgetDefinition._dialogValuesToData(dialogReturnValues.attributes);
            widget.setData(data);

            widget = editor.widgets.getByElement(image);

            if (firstEdit) {
              editor.widgets.finalizeCreation(container);
            }
…

Unfortunately, we cannot simply port over this code to the drupalentity plugin, because its "save callback" is completely unaware of the widget object, and there's non way to access that object using the current code architecture.

The code quoted above was introduced in #2039163: Update CKEditor library to 4.4 in Drupal 8 core. That landed on May 9, 2014. The drupalentity CKEditor plugin landed on May 27, 2014. So, quite likely, the drupalentity plugin was developed against CKEditor 4.3 and earlier, whereas CKEditor 4.4 finalized the CKEditor Widget API.

So, as far as I can tell, Entity Embed never had its CKEditor plugin updated to use the final architecture introduced in CKEditor >=4.4, and that would solve this problem.

Hence its JS should be rewritten, based on that of core's drupalimage plugin. That would likely

This is now blocking #2813813: CKEditor's "undo" functionality not working.

This also blocks #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog).

Closed #2956745: CKEditor js plugin upcast does ot check widget name because thanks to the new scope of this issue since #8, we'll basically get that bugfix "for free", so there's no point in keeping that separate issue around.

Thank you Wim for pushing this issue forward

Expect more progress soon — progress in patch form :)

"Soon" — that did not happen :( All my time and attention went to JSON:API until recently. But now I'm back :)

The original bug report, which I was able to reproduce in #8, is no longer reproducible. Presumably CKEditor updates in the mean time have fixed this (Entity Embed's CKEditor plugin hasn't changed at all). Although https://github.com/ckeditor/ckeditor-dev/issues/2517 and https://github.com/ckeditor/ckeditor-dev/issues/3027 are still open and seem to be related.

Thanks to @oknate, #2466013: Write test for CKEditor integration is nearly ready to be committed, and will give us peace of mind :)

Nevertheless, we should still do what this issue title says, because it'll fix #2813813: CKEditor's "undo" functionality not working and #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog).

As part of working on this, I dug deep into #3026433: `entity_embed.dialog` route should not use `_theme: ajax_base_page`; no longer required as of Drupal 8.2 (now descoped since it was a dupe and fixed) + #2844822: The preview in CKEditor does not use the same Twig template as the one on the front end (default theme) (embed previews apparently intended to allow CSS + JS to be loaded), because the most prominent aspect of the CKEditor plugin is its preview capability, and both of those issues are related to that.

While working on this, I noticed that #2813813 could be done independently of this quite easily, and actually reduces risk, since it results in more JS test coverage ensuring this issue gets done well. See #2813813-12: CKEditor's "undo" functionality not working for details. It just landed! 🥳

While struggling with #2511404-73: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor and a solid amount of cursing, I think I've found a way forward there, that also happens to address what we wanted to achieve in this issue, as well as unblocking #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog). In other words: a breakthrough :) I have a meeting scheduled with the CKEditor 4 lead developer tomorrow afternoon, so he'll be able to confirm whether this is a good direction or not.

This is a pretty significant refactor of the CKEditor Widget. Rather than storing the truth in the DOM, even though it's ephemeral, not actually the truth, and heavily affected by the use of the Drupal AJAX system, this tries to use the CKEditor Widget API "as intended" (as far as I can see). It uses the facilities that API provides to simplify code. This patch makes the code simpler, more robust, and despite there being lots of TODOs, it actually yields a net reduction in lines of code.

See #2282957-13: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog) and #2511404-74: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor for screenshots of what this enables 🥳

Now: Wim → 🛌😴

More tomorrow.

Looks very promising.

+++ b/js/plugins/drupalentity/plugin.js
@@ -94,53 +86,58 @@
+            // Now that the caption is available in the DOM, make it editable.
+            event.sender.initEditable('caption', event.sender.definition.editables.caption);

Oops, this line should've been part of #2282957-13: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog).

Green again, yay!

Let's now make this Drupal AJAX-powered fetching of a server-side preview less hacky.

Been making progress with captioning support over at #2282957-17: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog). In working on that, I've noticed that it's now re-fetching a preview from the server even when only the caption has been changed. The caption doesn't need to be rendered by the server.

The re-fetching problem has always existed, until #2813813: CKEditor's "undo" functionality not working fixed it. But the refactor in #18/#22 made that fetching logic less hacky, and in doing that, it is again re-fetching. #23 then properly isolated that logic.

Now that the code has been refactored, I think the next step is to make it conditionally re-fetch: only if it makes sense to do so. The following changes should not trigger a server-side re-render:

1. data-caption changes AKA changes in caption editable (because it looks exactly like the end result already)
2. changes to the link (because changes in the link do not cause visual changes; it just wraps the embedded entity)
3. …

(Also, I forgot to remove generateEmbedId() in #23, that's now dead code!)

Sorry, bad at multitasking today.

Yesterday, I met with Krzysztof Krztoń, the CKEditor 4 lead developer. I walked him through what I did in the patch on this issue, plus how #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog) and #2511404: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor build on top of it.

To my relief, he liked most of what he saw. He actually spent a few hours building a proof of concept based on what I told him we need. He built https://codepen.io/f1ames/pen/arQaYJ on top of the embedbase plugin in CKEditor core. Codewise it is fairly similar. Key differences:

• overriding much of what embedbase does to avoid integration with notifications
• tightly integrated with the dialogs that embedbase provides, which we won't be able to use
• relatively complicated request handling using the infrastructure that embedbase provides, because that is oEmbed-centric and hence JSONP-centric
• furthermore, Entity Embed does currently actively support loading CSS and JS (into a CKEditor <iframe> instance) that is associated with the rendered embedded entity — we can't just break this despite at least the JS loading being questionable (see #2844822-14: The preview in CKEditor does not use the same Twig template as the one on the front end (default theme))
• it requires embedbase to be loaded, and hence requires core to adopt a new CKEditor build, and would prevent us from shipping this with this contrib module (not to mention the additional JS bytes to load, parse and execute that we'd never use)
• the existing Entity Embed CKEditor plugin has garnered (despite its flaws) ~40K users; completely rewriting it now introduces avoidable backwards compatibility break risk

Given those considerations, I've decided to continue refining what we already have rather than restarting from scratch. Given his approval of the approach taken in this patch (plus #2282957 and #2511404), I now feel very confident we can make this work great.

One thing I learned from https://codepen.io/f1ames/pen/arQaYJ is https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_plugins_widget.h... — which is a single property that automatically masks the embedded content:

the widget's element will be covered with a transparent mask. This will prevent its content from being clickable, which matters in case of special elements like embedded Flash or iframes that generate a separate "context".

Yay, exactly what we need for Youtube embeds for example!

Unfortunately using that masks the entire widget, we need the caption editable to still be available for interaction in #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog). He came up with a clever hack to solve this: figcaption { position: relative; z-index: 1; }. 😀

Unfortunately … it doesn't work at all for us. 😩 Our combination of display: table plus float: right (for a right-aligned captioned embed) on <figure class="caption align-right"> means the widget CKEditor plugin's mask functionality doesn't work, because it relies on position: absolute; top:0; left:0; width:100%; heigh:100%; display:block; to cover the parent exactly through pure CSS. But our CSS — which we've had for more than half a decade and we cannot change since it'd break BC — makes that trick impossible.

I spent 2 hours trying various things, but came up empty-handed :(

I will ask Krzysztof to see if he has any more clever ideas to solve this.

The best ideas we can't do now because of BC concerns could become postponed tickets for 8.x-2.x branch or 9.x-1.x branch.

I did not let the hours of CSS attempts go to waste: I finished #3037316: Show an outline around the <drupal-entity> element within CKEditor and got it committed — that's a definite step forward compared to our current CSS. The patch attempt in #28 is also relative to that, so now it can easily be applied.

#29: actually, mask support is going to be critical to meet the accessibility gate in Drupal core.

That being said, mask support is not a hard blocker right now for this issue; this issue can make many other things better, so we can proceed :)

I discovered problems with undo support (separate from #2813813: CKEditor's "undo" functionality not working, which made undo not work at all, right now it works but it's flaky), but those are apparently pre-existing. Krzysztof is looking into it.

To reproduce:

1. go to /media/add/image to create an image and add it to the media library
2. go to /node/add/article to create an article
3. type "foo" on one line, "bar" on the next
4. in between those lines, create a captioned left-aligned Media entity embed of an image
5. save the article
6. edit the article
7. double-click the captioned image, change alignment to "right" in the dialog, save the dialog
8. now the "undo" button becomes available — makes sense!
9. Click the undo button.

Expected: undo button is now grayed out, and you're in the original state.

Actual: undo button is not grayed out, you're not in the original state. Upon clicking the "undo" button again, it's in the original state.

Possibly related: this appears in the browser console:

[CKEDITOR] Error code: selection-not-fake.
[CKEDITOR] For more information about this error go to https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#selection-not-fake

The same exact steps to reproduce actually also work in HEAD. So I won't make that block this patch from landing: this is not a regression.

Created #3060245: [upstream] CKEditor's "undo" functionality not working correctly for embeds: offers multiple undo steps where there should be only one for #32. Also contains a screencast demonstrating the problem.

Another thing I noticed while digging through the CKEditor Widget plugin code: we're not yet setting the pathName property.

Which is why you see drupal-entity here:

Wouldn't it be nice if that showed Embedded entity instead? Or better yet, if it matched what the context menu is showing you? (Just like #2993323: CKEditor Contextual Menu Item should be more specific.)

Turns out that pathName is just the Widget plugin's API abstraction for the underlying API, and hence doesn't allow for the dynamism that we need (multiple entity types could be embedded). Fortunately, there is a way to achieve that:

nice!

Needs tests was added in #7, based on #4. But that is no longer reproducible. It looks like Chrome's behavior changed and hence fixed this. Pointless trying to write tests for this if the problem wasn't on our end; we could not even write a test for this if we tried.

drupalimage doesn't listen to the doubleclick event on the entire editor and then tries to check if the clicked element was the widget. This is costly and brittle.

It's better to rely on the existing events.

Bringing that over from drupalimage.

Put differently: this makes triggering the editdrupalentity command more reliable.

#37 removed one call to isEditableEntityWidget(). There's two calls left: one in getSelectedEmbeddedEntity() (another helper) and one in the context menu listener.

Turns out that isEditableEntityWidget() was introduced by #2544018: Manual embed should not get an option to Edit Entity, to specifically allow for people to type arbitrary <drupal-entity> markup, and have those be turned into previews, without requiring a corresponding CKEditor toolbar button. Which means that those entities that don't have a corresponding button should not be editable through the UI.

I have … concerns about this. But I won't be changing this, because doing so would result in a BC break.

(I certainly won't bring that into core. But it's easier to do that in core, since core already is going to limit this to only Media entities, so we're going to be dealing with a far smaller surface area.)

IOW, these two edge cases must not break CKEditor:

<drupal-entity data-embed-button="non-existent or perhaps removed" …></drupal-entity>

<drupal-entity …></drupal-entity>

They currently do. #34 also regressed against this. Fixed now.

+++ b/js/plugins/drupalentity/plugin.js
@@ -101,9 +101,18 @@
+          if (this.parts.link) {
+            this.setData('link', CKEDITOR.plugins.image2.getLinkAttributesParser()(editor, this.parts.link));
+          }

Oops, that shouldn't have been there. Bad rebase. (This comes from #2511404: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor.)

That explains the test failures.

This should do better.

Time for a final round of clean-up, then let's land this 🥳

WHEW.

#28 was not yet done, created #3064572: Add mask to prevent interaction with embedded media for that.