Update CKEditor 5 to 39.0.0

There's no 38.2.0 stable release yet, only an alpha: https://github.com/ckeditor/ckeditor5/releases/tag/v38.2.0-alpha.1 😅

But +1 to everything you said 😊

Note that this will never be committed to 10.0.x, only to 10.1.x.

All changes must first land in 11.x.

If this really doesn't break any contrib integrations, I think we should backport it to 10.1.x, but let's get it into 11.x first.

Reran tests twice but they consistently fail.

A bit more context on the versioning from our side:

CKE5 ecosystem is around ~100 pkgs now. In the past, we versioned them independently, just like semver dictates. Some were in e.g. 5.0.0, other, that changed slower were 1.1.10 let's say. All good.

But, that created an awful DX – you could never know which packages are actually meant to work together. It'd be fine if you were using the very latest versions of all of them, but sometimes you can't. So, when trying to hardlock to some older CKE5 version, you'd need to somehow figure out versions of all CKE5 packages that were shipped at the same time.

Theoretically, you may ask, why is it necessary to get those shipped at the same time if you use semver and track all BCs? Couldn't I get some of the latest, but keep some of the older ones if... well, if what?

Let's welcome npm on stage. And transient dependencies.

Let's say you use two CKE5 features – FeatA and FeatB. cke5-featA@1.0.0 requires cke5-engine in version ^1.0.0, cke5-featB@1.0.1 relies on a newer version of the engine (^2.0.0) as it was released a year later. As a developer integrating CKE5 you'll only see the versions of featA and featB pkgs. So you think all's good.

What will npm do if you try to install this setup?

It will install the cke5-engine package twice to satisfy both feature packages needs. That makes some sense in Node.js, for which npm was built, although, causes issues there too from time to time. But it makes zero sense for the web, where you don't want and very often cannot include one library twice in different versions. Hence our "ckeditor-duplicated-modules" check.

This isn't a problem isolated to CKEditor 5 itself. We've seen people running into similar issues with other projects too. And we researched how did some of them resolved this madness.

The most sane resolution was to ditch semver. It kinda works (but again, not often) for libraries, but makes no sense for ecosystems of packages. I don't remember anymore which projects we followed (I think Babel and Angular) but we realized that by having all our pkgs in the same version we're resolving many integrators issues. (EDIT: To add to that – very recently we started hardlocking transient dependencies to specific versions instead of using the so-called caret-ranges to fix another npm's problem).

Those changes helped a lot and we've seen a decline in issue reports showing mismatched versions of CKE5.

But then, the side effect is that a braking change in one package needs to somehow be reflected in the version of all packages. Something that happened in this case. A very narrowly used CKBox package had to have a BC due to a new major version of CKBox itself that had to rebuild its configuration. This isn't a widely used package yet so investing our limited capacity into building compat layer and/or shipping this without a BC on CKE5 side could not be justified. It's literally cheaper to go on a call with everyone using this package and resolving this for them :D

So, my recommendation is to analyze the changelog of CKE5 itself where we're trying (not always successfully, but we're hopefully improving on this) to list all BCs (that we divide into two categories based on their impact) and figure out whether this is a major or minor release for your use case. The version number of CKEditor 5 is a secondary thing and I started thinking we should use only the X of X.Y.Z :D

Looking into test-failures.

Looks like CKEditor replaced the inner workings of <figcaption>-element in 39.0.0.

Where it was a data-placeholder-attribute with the text in it's value, now the text lives inside the <figcaption>-element.

So at the very least the now failing \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption needs refactoring to adapt to those changes.

Unsure if this is an intentional CKEditor5 behaviour change or not, so let's await a verdict on that before actually changing it.

The failure:

There was 1 error:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testEditableCaption
Behat\Mink\Exception\ElementHtmlException: The attribute "data-placeholder" was not found in the element matching css "figcaption".

/Users/wim.leers/core/vendor/behat/mink/src/WebAssert.php:829
/Users/wim.leers/core/vendor/behat/mink/src/WebAssert.php:563
/Users/wim.leers/core/vendor/behat/mink/src/WebAssert.php:606
/Users/wim.leers/core/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php:531
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Debugged test with:

diff --git a/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
index 4c03af0ea2..d62272728a 100644
--- a/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -526,6 +526,8 @@ public function testEditableCaption() {
     $this->assertNotEmpty($assert_session->waitForElementVisible('css', '.ck-widget.drupal-media img'));
     $assert_session->elementExists('css', '[data-drupal-media-preview][aria-label="Screaming hairy armadillo"]');
     $assert_session->elementContains('css', 'figcaption', '');
+    var_dump($assert_session->elementExists('css', 'figcaption')->getOuterHtml());
+    $this->assertSession()->waitForElement('css', 'body.foo', 900*1000);
     $assert_session->elementAttributeContains('css', 'figcaption', 'data-placeholder', 'Enter media caption');
 
     // Test if you leave the caption blank, but change another attribute,

Unfortunately there is a legitimate regression here:

Before

<figcaption class="ck-editor__editable ck-editor__nested-editable ck-placeholder" data-placeholder="Enter media caption" role="textbox" contenteditable="true"><br data-cke-filler="true"></figcaption>

After

<figcaption class="ck-editor__editable ck-editor__nested-editable" role="textbox" contenteditable="true"><br data-cke-filler="true"></figcaption>

i.e. what I see is worse than what @Spokje describes: the placeholder text is just gone for me? 😅

Looks like this is likely a consequence of this minor breaking change in the changelog:

engine: The enablePlaceholder() helper now uses a placeholder property of the passed element. It no longer takes the placeholder text as a text argument.

— https://github.com/ckeditor/ckeditor5/issues/9925 → https://github.com/ckeditor/ckeditor5/commit/a7e094703b17c4ca29beb4d85ec...

We do

        enablePlaceholder({
          view,
          element: figcaptionElement,
          text: Drupal.t('Enter media caption'),
          keepOnFocus: true,
        });

in core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediacaption/drupalmediacaptionediting.js.

👍 Just pushed a fix per #14.

👍 Fortunately, not a single Drupal contrib module has JS that calls enablePlaceholder(), so it should still be safe to update to CKEditor 5 39 despite this BC break. The fact that it solves #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements alone has far more positive Drupal ecosystem impact than the potential negative.

😰 But … in a basic manual test, I see that there's a hard crash when adding the drupalMedia button to the Basic HTML text editor, that causes a crash even before the editor boots (meaning: it never even shows up):

ckeditor5.js?ryt4dm:472 TypeError: Cannot read properties of null (reading 'parent')
    at s (drupalMedia.js?ryt4dm:1:1553)
    at Object.getRelatedElement (drupalMedia.js?ryt4dm:1:9664)
    at q._updateToolbarsVisibility (ckeditor5-dll.js?v=39.0.0:5:620814)
    at n.<anonymous> (ckeditor5-dll.js?v=39.0.0:5:619839)
    at n.fire (ckeditor5-dll.js?v=39.0.0:5:569001)
    at n.update (ckeditor5-dll.js?v=39.0.0:5:534585)
    at Object.fire (ckeditor5-dll.js?v=39.0.0:5:569001)
    at We (ckeditor5-dll.js?v=39.0.0:5:160498)
    at He.fire (ckeditor5-dll.js?v=39.0.0:5:159889)
    at Mo.<anonymous> (ckeditor5-dll.js?v=39.0.0:5:217112)

Debugging that now.

i.e. what I see is worse than what @Spokje describes: the placeholder text is just gone for me? 😅

That's because @Spokje described it rather poorly, luckily you found it yourself. 😅

getClosestSelectedDrupalMediaWidget() was last refined ~1.5 year ago in #3264775: [drupalMedia] Toolbar should be visible when element inside <drupalMedia> is focused (back when we were stabilizing CKEditor 5!) and looks like this:

export function getClosestSelectedDrupalMediaWidget(selection) {
  const viewElement = selection.getSelectedElement();
  if (viewElement && isDrupalMediaWidget(viewElement)) {
    return viewElement;
  }

  let parent = selection.getFirstPosition().parent;

  while (parent) {
    if (parent.is('element') && isDrupalMediaWidget(parent)) {
      return parent;
    }

    parent = parent.parent;
  }

  return null;
}

What's happening is that selection.getFirstPosition() returns null and hence the .parent expression causes a fatal error.

What's missing is:

  // Perhaps nothing is selected.
  if (selection.getFirstPosition() === null) {
    return null;
  };

Curiously:

1. this seems to have been a bug all this time? :O Because getFirstPosition() has been documented to return null if there's no range in the selection since 7 years ago.
2. but it worked fine for the past several years
3. no mention of getFirstPosition() behaving differently in the CKE5 39 release notes, although it has many mentions of Selection-related changes
4. When testing manually: In 38.1.0, Selection.getFirstPosition() returns the first element in the document, even when there is NO SELECTION 😳 — and that is what 39.0.0 changes and arguably fixes
5. When in a FunctionalJavascript test powered by chromedriver: this does NOT happen, only when using Chrome directly/in manual testing 😳, because inside a chromedriver-booted browser, even manual debugging steps show the behavior described above for 38.1.0!

So we seem to have ourselves a https://en.wikipedia.org/wiki/Heisenbug, although it's one with a trivial and safe fix… 🙃

(All using Chrome 115.)

Discussing this with the CKEditor 5 team in a few hours 👍

htmlSupport.allowEmpty is a new configuration option that will allow us to solve the #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements data loss critical. In manual testing I can already confirm it works 👍

What's missing: test coverage. Tackling that first over at #3337298-22: [upstream] [GHS] CKEditor 5 removes empty inline elements.

Since markup like <i class="fab fa-drupal"></i> or <span class="icon my-icon"></span> can only be created through the SourceEditing CKEditor 5 plugin, I added the necessary test coverage there: #3337298-23: [upstream] [GHS] CKEditor 5 removes empty inline elements.

Copying the test from that issue into this MR, because it's necessary to justify this update: this either fixes a critical data loss bug or it doesn't.

First, let's verify that the test fails even on version 39 of CKEditor 5, because the test alone of course does not specify the htmlSupport.allowEmpty config.

I discussed #19 in detail with @Reinmar and @witeksocha.

Conclusion:

1. For unrestricted text formats, #3337298-23: [upstream] [GHS] CKEditor 5 removes empty inline elements MUST NOT assume that all tags can have empty inline elements.
2. Instead, even for unrestricted text formats, which elements ought to support empty elements should be explicitly configured.
3. This matches the behavior in CKEditor 4, where it could not even be configured through the UI, it required custom code — see #3337298-7: [upstream] [GHS] CKEditor 5 removes empty inline elements.

So, adjusting the test coverage slightly for that: even for unrestricted text formats, we should expect

<p>Before <i class="fab fa-drupal"></i>

to be transformed to

<p>Before and after.</p>

Cherry-picked #3337298-25: [upstream] [GHS] CKEditor 5 removes empty inline elements here 👍

And now the actual fix for #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements, which is possible only here, thanks to CKEditor 5 39 containing the upstream fix that we must correctly configure 🤓

The fix is very simple: configure htmlSupport.allowEmpty, with the list of HTML tags that is configured in the Source Editing view. That means this is must be manually configured. There is no automatic upgrade path. Again, #3337298-7: [upstream] [GHS] CKEditor 5 removes empty inline elements is crucial here: in CKEditor 4 it didn't require manual configuration, but custom code!

Added release notes for that reason — for both the next minor as well as the next patch release.

Following up on the Heisenbug I discovered in #17:

1. Kuba Niegowski from the CKEditor 5 team confirmed that this same bug was solved ~1 year ago in the ckeditor5-image package, where the drupalMedia plugin had copied this pattern from. See https://github.com/ckeditor/ckeditor5/issues/11972.
2. Previously, the isFocused state was waiting until a 50 ms timeout had passed or the selection was changed, that was changed in https://github.com/ckeditor/ckeditor5/pull/14648. Thanks again to Kuba for pinpointing this! 👏

   So there indeed was a slight behavior change, but even then, the bugfix here (https://git.drupalcode.org/project/drupal/-/merge_requests/4530/diffs?co...) is still necessary — we basically got lucky that we didn't trigger this bug until now 😅

FWIW: I couldn't reproduce neither in latest Chrome nor FF whilst being on commit 571b949481

Which again just confirms that it's very likely a browser execution timing thing which we should not be relying on 👍

Issue summary updated, ready for final review now!

You might want to fix the remaining test failure first? 😈😇

I forgot to adjust the expectations for dynamic plugin configuration Unit test coverage in SourceEditingPluginTest, fixed 👍

The test that really mattered and was failing before, \Drupal\Tests\ckeditor5\FunctionalJavascript\SourceEditingTest::testEmptyInlineElement() was in fact already passing! 😊 That's a FunctionalJavaScript test, so much more important.

EDIT: cross-posted with @Spokje in #24 😅

Test updates look good. I took a look at #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements but should that be postponed first until this lands.

#26: #3337298: [upstream] [GHS] CKEditor 5 removes empty inline elements is adding only test coverage — and I've included that test coverage in this MR. If this MR gets merged, then we can mark that other issue as Fixed too 😊

Committed f5de3b7 and pushed to 11.x. Thanks!

Patch does not apply cleanly to 10.1.x, leaving open for backport as previously discussed.

On it.

Failure seems legit to the issue. FYI marking https://www.drupal.org/project/drupal/issues/3337298 as fixed too since it got merged 11.x

Odd, I literally cherry-picked the fix for that from the MR that was merged into 11.x. I thought I had maybe built it wrong … but no, rm -rf'ing all built files and running the yarn install && yarn build && yarn build:ckeditor5-types commands again results in an empty diff!

I failed to cherry-pick it correctly 🙈

Passes tests now 👍

All green!

I think this could use a change record, we could probably move the details about the empty inline elements to the CR then too.

IMO ckeditor is applying semver overly strictly and should give themselves a bit more leeway with @internal to reduce their major release frequency. Given that and that the only change required here fixed a hidden testing bug, I think we should get this into the next patch release and unblock ckeditor4 migrations a bit more.

Done: https://www.drupal.org/node/3379650.

RE "more @internal" — yes, I've had that exact conversation with @Reinmar 1–2 years ago. The struggle they have is the same Drupal had ~5 years ago and to some extent still has today: not explicitly marking what the actual public API is means that everything is an API 🙈 Their move to TypeScript recently should help with that, but in vanilla JavaScript there is no such thing as private/protected which makes this even harder for them!

https://github.com/ckeditor/ckeditor5/releases/tag/v39.0.1 has been released, with the sole BC break that affected Drupal fixed: the enablePlaceholder() thing mentioned in #14 and #15.

I propose to first merge this MR as-is, and to open a new issue for 39.0.1? That will reduce risk further for the Drupal ecosystem.

However, if core committers prefer that, I can instead update this 10.1.x MR to update to 39.0.1 immediately, but then I'll need a new MR for this issue against 11.x, since the already merged one only updated us to 39.0.0.

Given it's fixing a bc break we already changed code for, and we're not going to change the code back, let's do a new issue. We've still got 2-3 weeks before the next 10.1 patch release so time to monitor a bit. CR looks good.

Committed 3cc755c and pushed to 10.1.x. Thanks!

Done: #3380637: Update CKEditor 5 to 39.0.1.

CR published (checked with @catch first).