CKEditor's "undo" functionality not working

I looked into this -- admittedly quite briefly, but I think I have a handle on what the problem might be.

First and foremost, I'm convinced that this problem is firmly in Entity Embed's province. Obviously Lightning is affected, but Lightning is not specifically doing anything which should cause this. Our direct integration with CKEditor is very, very light. So I'm moving this to Entity Embed's issue queue.

Secondly, I suspect (gut feeling, I have no scientific evidence that this is the case) that this is a conflict between the way Entity Embed works and the way CKEditor expects its undo functionality to work.

When you embed an entity into CKEditor, Entity Embed does a covert AJAX request to the server to retrieve a rendered version of the entity. Meanwhile, CKEditor's undo system is periodically taking "snapshots" of the editor's state. As far as I know, these two processes are unaware of each other. At the time that you embed the entity, the editor gets an empty <drupal-entity> tag, which is filled with the rendered entity when the AJAX request completes. I believe that CKEditor is taking an undo snapshot during the AJAX request, before the rendered entity has arrived...and after the AJAX request has completed, and the rendered entity has been inserted into the editor. To the undo manager, these would look like two different states...which would explain why you have to click Undo twice, and very quickly, to be rid of the <drupal-entity> tag.

So why do you need to double-click quickly? Because I think that after the first click, the rendered entity is removed by the undo system...at which point Entity Embed sees an empty <drupal-entity> tag and thinks that it needs to retrieve the rendered entity again. So it does, and probably quite quickly (the browser may have cached the response). Thus it seems like the thing you undid, has been redone -- because it has.

Again, this is a theory. I have no idea how to begin to solve it. But hopefully it provides a place to start.

I can confirm the bug still exists, the "undo" button is not working for embedded entities the same way it works for core embedded images.

This case still exist with entity_embed / 8.x-1.0-beta2.
Any progress on this issue?

#2823691: After adding an embed, hitting undo doesn't work reported the same problem:

So here's what happens:

1- Embed an entity in the wysiwyg
2- Hit the undo button (if you have it enabled on the wysiwyg) or just hit ctrl+z
3- The element you embedded with disppear momentarily, but then re-appears again!
4- Now double-click the undo button (or just hit ctrl+z twice real quick, as if double-clicking it)
5- Now the element you embedded goes away for good

Analysis:

I think that embedding an entity actually has 2 steps:
1- Adding the code
2- Actually adding the HTML to render the entity. (Triggered by adding the code)

And hitting undo, only undoes step 2. But as CKEditor re-renders the content after the undo, step 2 is triggered again because the code from step 1 is still there.

Possible solution:

I'm not experienced with CKEditor's API, but maybe it's possible to smartly trigger an extra undo when needed?

If not possible, maybe someone else has a suggestion?

Closed that as a duplicate.

In core, we added undo support in #2071957: Cannot change existing images or links through Text Editor's image/link dialogs. It's quite simple: we just need to do editor.fire('saveSnapshot'); both before and after the changes are made.

Unfortunately, this does not actually work. It looks like the CKEditor plugin's JS is doing some strange things that probably get in the way. Admittedly, the CKEditor plugin/widget APIs are quite strange too. So it's easy to get wrong. The easiest way to solve this would be to rewrite the drupalentity CKEditor plugin to closely match core's drupalimage CKEditor plugin…

(Like most Drupal 8 installations, I don't have an explicit "undo" button in CKEditor. It exists, but it's rather pointless for most of us, who are used to CTRL/CMD+Z to undo.)

#2544020 should happen first, see #2544020-8: CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin.

Actually, having started to work on #2544020: CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin, this is probably easier to solve independently first.

It's quite easy to observe this misbehaving by looking at CKEDITOR.instances['edit-body-0-value'].undoManager.snapshots in a browser. It'll jump from 1 to 3 snapshots upon changing the embedded entity, instead of changing from 1 to 2.

Specifically:

This is using https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#even....

Patch wasn't relative to HEAD, my bad.

I can definitely confirm the problem in manual testing. However, the patch doesn't fix it for me; the undo button simply becomes ineffective. No JavaScript errors are thrown.

I think this could probably use tests...

It took me four hours to figure out a work-around for having CKEditor make its Undo button clickable, because despite an undo snapshot history having been accumulated, the Undo button didn't become activated 😩 This is probably because CKEditor is making some assumptions and/or because Selenium/chromedriver are doing some things differently.

Anyway, I did find a work-around at last! This test is now working reliably.

@phenaproxima was right, I was able to reproduce #13 not fixing the entire problem. #13 fixes changing existing embeds, and it also works when first typing text and then inserting an embed, but it fails if the first thing you do in CKEditor is inserting an embed.

That's also why the proposed fix is now failing in the test coverage.

Here's an improved fix.

+++ b/js/plugins/drupalentity/plugin.js
@@ -123,6 +125,9 @@
+          if (element.$.children.length > 0) {
+            return;
+          }

Note that this also means we don't re-fetch previews when undoing/redoing! Big front-end performance win.

But we can clean up this code a little bit. Did that.

Created patches from the wrong branch. Rerolled them from the right branch. Interdiffs and test-only patch were right.

lol:

+    $this->getSession()->executeScript("CKEDITOR.instances['edit-body-0-value'].setData('<p>Goodbye world!</p>');");

Excellent work with the helper function assertCkeditorUndoOrRedo().

One thing I noticed waitOnCkeditorInstance() duplicates and improves upon the method in the base class, waitForEditor().

Can we put waitOnCkeditorInstance into the base class EntityEmbedDialogTest.php as a replacement for waitForEditor() and update references to waitForEditor()?

#22: Glad you're entertained by the single line that came out of my four hours of trial and error 😜

One thing I noticed waitOnCkeditorInstance() duplicates and improves upon the method in the base class, waitForEditor().

Oh, hah, I copy/pasted that from \Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest::waitOnCkeditorInstance()! Done.

We could go even further in test coverage: we could assert the specific undo snapshot count, but I think the above test coverage already asserts which snapshots exist, without going into as much detail, so I think #23 is preferable.

In regard to

I copy/pasted that from \Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest::waitOnCkeditorInstance()

That's funny. I independently came up with a very similar solution.

I manually tested #20 and it works great, marking RTBC.

In regards to #24. I think snapshot count addition is worthy and should be added. One of the best things about tests is it helps developers understand the functionality. By documenting the snapshot counts, you will teach future generations how to debug the snapshot count.

WFM.

I ran #26 against the coder linter and didn't see any issues.

🚢

One more problem with "undo" found: #3060245: [upstream] CKEditor's "undo" functionality not working correctly for embeds: offers multiple undo steps where there should be only one.