[CKEditor] Update image URL failed after saving

Is this a bug in CKEditor itself?

I can confirm this bug - I was just about to log it as well. It happens in both D9 & D10 with CKEditor5. I'm not sure where the fix needs to be... but it's the source that doesn't get updated. If you update the image and switch to view the source you'll see the old image url in the "src" attribute, so when you switch back it shows the original image.

I created a new patch for drupal 10.1.4, I agree with @nsciacca, this issue persisted in CKEditor 39.0.1

You patched the minified CKEditor 5 JS 😅

This will never get merged into Drupal. This must be fixed upstream. Sounds like you already know exactly what to do to fix this upstream though, so should be easy? 😊

It seems to be a Drupal side issue after all.
It's reproducible only when the text format has an HTML filter disabled (e.g. Full HTML default format).
As far as I understand it has to do with the fact that without HTML filter htmlSupport.GeneralHtmlSupport plugin is enabled and when updating the image it doesn't properly update the image src in the model.

Wow, excellent research, and especially: excellent test coverage!

Test-only patch indeed fails locally:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlUpdateTest::testImageUrlUpdateWidget
Failed asserting that '<img src="/core/misc/druplicon.png">' contains "/core/misc/help.png".

Review

The fix looks solid.

The only complaint I have is about the organization of the test coverage, which causes unnecessary extra/duplicate test runs:

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageUrlUpdateTest.php
   @@ -0,0 +1,115 @@
   +class ImageUrlUpdateTest extends ImageTestBase {
   

   👎 This causes all of the inherited tests to run again, despite this being identical to \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlTest.

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageUrlUpdateTest.php
   @@ -0,0 +1,115 @@
   +  public function testImageUrlUpdateWidget(): void {
   

   🙏 Please just add this test method to \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlTest instead 😊

Moved the test to ImageUrlTest class and created MR!5124
Test failures from patches seems random

Test failure seems legit to issue.

Yes, these failures are legit.
So something weird happens with \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTestBase::testAlignment
It fails if there are 2 text formats created in SetUp, even though the host uses the proper format.
If you add sleep(1) before $page->pressButton('Save') test passes.
Also, it fails if the host value is img or plain text (not wrapped in p)

I had a similar issue when I was writing a new test, I was writing it in the ImageUrlTest class first and tried to save the form to assert that the new image was saved, and it was failing unless I added a sleep(1) call before clicking form submit.

I've no idea why it happens. Seems like submitting works before ckeditor could update the hidden textarea. But why does it happen only when there are 2 text formats?
I could move the test into a separate class that will not inherit ImageTestBase.

I'll investigate. This probably needs one more "wait" for some specific thing 😬

I decided to give this another try and found the issue.

This can be reproduced even by bare hands by making the first change to CKEditor after the page reloads and instantly submitting.

The reason why this happens only with 2 or more text formats lies in 2 places
web/core/modules/editor/js/editor.js:332
web/core/modules/editor/src/Element.php:117

The editor is changing the hidden text area before submitting as it is supposed to, but the data attribute isn't changed if the form is submitted right after the change.
The only place where data-editor-value-is-changed is set to false is Element.php with the condition that there is more than 1 format. With the single format that attribute is simply not set and the condition in js is false.

So to fix the test it is enough to add something like waitForElement('css', '[data-editor-value-is-changed="true"]');

But should we create a follow-up to this and fix it, so that there is no way to submit without change applied?

If there is a separate issue, may need to open a follow up bug report ticket to address that. And postpone this ticket on that one.

Postponed by https://www.drupal.org/project/drupal/issues/3396742 🐛 CKEditor5 doens't save updated value if form submitted right after the change Active

Support Drupal 10.2.3

Reviving this since https://www.drupal.org/project/drupal/issues/3396742 🐛 CKEditor5 doens't save updated value if form submitted right after the change Active was merged.
I made some changes to the test due to the CKEditor version update.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

testbot is having issues

Looking good, but I think the tests can be simplified slightly. Once those simplifications are in, I think this can be RTBC 👍

Create a patch for Drupal 10.2.3.

Patch in #23 works for me, 10.2.2

This should apply for Drupal 10.3.0

Rebased branch. Simplified tests by @wim leers suggestions.
Test-only changes in the pipeline are green because the pipeline skips the tests for some reason. Although seems like they did run in the default job.