Update CKEditor 5 to 45.0.0

yarn build:ckeditor5 results in some warnings

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstyleediting.js 240:16-21
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle.js 5:0-87 40:12-37
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 8:0-54 24:2-20

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle/drupalelementstyleediting.js 241:27-32
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalelementstyle.js 5:0-87 40:12-37
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 8:0-54 24:2-20

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediacaption/drupalmediacaptionui.js 37:14-27
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediacaption.js 5:0-77 17:39-59
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 9:0-54 23:2-20

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/mediaimagetextalternative/mediaimagetextalternativeui.js 69:14-29
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 12:0-98 21:2-29

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/mediaimagetextalternative/ui/textalternativeformview.js 53:6-17
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/mediaimagetextalternative/mediaimagetextalternativeui.js 18:0-67 102:21-44
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 12:0-98 21:2-29

WARNING in ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/mediaimagetextalternative/ui/textalternativeformview.js 63:6-18
export 'icons' (imported as 'icons') was not found in 'ckeditor5/src/core' (possible exports: Command, Context, ContextPlugin, DataApiMixin, Editor, ElementApiMixin, MultiCommand, PendingActions, Plugin, attachToForm, secureSourceElement)
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/mediaimagetextalternative/mediaimagetextalternativeui.js 18:0-67 102:21-44
 @ ./modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/index.js 12:0-98 21:2-29

Needs work for #3 plus test failures.

Duplicate of 📌 Update CKEditor 5 to 45.0.0 Active ?

Looks like a result of breaking change in CKEditor 5 v45.0.0:

All CKEditor 5 icons are now available in the @ckeditor/ckeditor5-icons package.

https://ckeditor.com/docs/ckeditor5/latest/updating/guides/changelog.htm...

I didn't get an warning from yarn. But there are many test failures, 2 1/2 pages worht.

Looking at this now: the icons were moved to @ckeditor/ckeditor5-icons and RENAMED.

https://github.com/ckeditor/ckeditor5/pull/17750/files

Finally got all tests passing.

There are some breaking changes:

1. The icon name changes as mentioned in #12
2. table tags now will all have the table class added to them: https://github.com/ckeditor/ckeditor5/pull/17889/files
3. Also more documented here: https://ckeditor.com/docs/ckeditor5/latest/updating/guides/changelog.htm...

There are also changes in the UI for linking images and media.

Before (inserting media with link):

After (inserting media with link):

Adding the release notes tag - we'll want to document the UI changes, and maybe the API changes if we think they could affect contrib, in the 11.2.0 release notes.

we'll want to document the UI changes

There might be others, but these were the ones that surfaced from failed tests. :)

https://github.com/ckeditor/ckeditor5/releases/tag/v45.2.0 is out, we should try to bump to that I guess.

Thanks for working on this. I've reviewed the MR and the changes all make sense, not too worried about any of the test changes.

For the icons we have some choices: I think this initial BC layer is a good idea. I think we should also issue a deprecation from the PHP side if we detect an old icon name; this way we can remove the BC layer in the future. Alternatively we could also move the entire BC layer to the PHP side, and it doesn't look too hard to support all the icon names they changed, there aren't actually that many.

On the other hand, we are running up against the rc1 deadlines, so the quicker we can just get this in the better; maybe we could add the deprecation and improved BC layer in a followup.

NW for updating to v45.2.0.

Updated to v45.2.0.

I agree with pushing off a better BC layer to a follow up. It's non-trival effort, and use cases seem to be limited to adding buttons to the Media dialog. Also, one thing about specifying icons is that they can either be the SVG markup or name of an existing icon, so the BC detection needs to account for that as well.

Added a small release note. Not sure worth mentioning the highlights from the release

We fixed the copy-paste scenario in the read-only mode.
Tables pasted from Office, especially with borderless layouts, should preserve styling in the editor similar to the ones in the source file.
Improved the adoption of the fullscreen feature on smaller screens and includes subtle visual tweaks.

A CKEditor release that requires bug fixes usually needs more detail in the release note in case they also affect contrib or custom modules, so unless someone can say for sure none of the bugs on this issue would have done that, we might need more that highlights the specific disruptive changes. We also generally include a link to CKE's own release notes for a disruptive CKE update. Thanks!

I think the icon break is the biggest one so explicitly called that out with a link to the CR, otherwise not sure which ones are important so added a catch-all and linked to the CKE upgrade notes.

Looks great, thanks @longwave!

This all looks good to me.

I noticed one outdated license reference in the MR, looks like it's a pre-existing issue from a long time ago though.

For a 10.5.x backport, I think we could make all the same changes but without triggering any deprecations - e.g. a silent bc layer which then becomes un-silent from 11.2.x

Opened [#/3529141].

I noticed one outdated license reference in the MR

Looks like ckeditor5.essentials in core.libraries.yml is at version "35.1.0" and linked to the the corresponding license as well. Didn't see any history about that with a quick look at the git blame.

@godotislate Could we either followup, or actually just make the change a suggestion (using the GitLab "suggest feature") and discuss in the comment thread on the current MR? In generalt two MRs on one issue is something we should avoid except when we're working with multiple branches or comparing different architectual approaches.

Thanks so much for your assistance on this issue and all the ones like it! It's a big help for core releases.

@xjm added MR comment about the ckeditor5.essentials version.

New MR 12331 is for 10.5.x and WIP.

OK, MR for 10.5.x with changes per #27 for no deprecation warnings: https://git.drupalcode.org/project/drupal/-/merge_requests/12331

If I'd thought about it more, would have waited to see if any package versions would be changed in 11.x MR, but should be easy enough to copy over if so. Held off on 10.6.x for that reason, though, assuming 10.6.x needs an MR as well.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks! I accepted the one version string license suggestion, and made and accepted another one.

I have not reviewed the 10.5.x MR yet and it hasn't been independently RTBCed, so moving to needs review for that.

catch → closed merge request !12060

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Not sure what the bot's on about.

think the bot checks against 11.x

It's unfortunate that this landed before ✨ Drastically improve Drupal's default linking experience in text fields Needs work as the upgrade has BC breaks for stuff we were using there. I'm trying to figure out how to fix it but it's not clear at all how to.

If anyone is more familiar with CKE5 objects, please let me know :)

https://www.drupal.org/project/drupal/issues/3317769#comment-16141773 ✨ Drastically improve Drupal's default linking experience in text fields Needs work

@acbramley That's a helpful data point; thank you. From a security and release management perspective, it's very important for us to be on the latest CKEditor version prior to a minor release, but the fact that the update broke a feature under development is good forewarning about what we might expect to happen when the release goes out. If you do discover what caused the breakage, please also report back here so we can add to the CR and release note about it.

@xjm sure, I understand that, just a bit of a vent tbh since that feature has been so close to getting in since a month ago :(

The breaking change for this particular thing is described here https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-45....

It may affect modules such as Linkit and Editor Advanced Link but as far as I can see they aren't altering the actionsView in the same way we are in the core issue.

Regarding item 2 from #13:

table tags now will all have the table class added to them

It might be worth calling that out in the release note. If a theme uses the .table class to apply styles, CKEditor might now apply table styles where that wasn't previously the case. The CKE release notes don't mention the added class.

For example: in our Bootstrap-based theme, the .table class is used to opt-in to Bootstrap's table styles, and we already add the class to all tables (via a filter) in our "Basic HTML" text format. But, we also provide a more permissive text format where advanced contributors can omit the class if they need specific table styling. Unless I'm misunderstanding something, after this change CKEditor will essentially force those styles in all cases.

To #45, agreed. I recall working on projects where the FE theme was styling tables based on presence of the "table" class, so I thought CKEditor adding that class to the table element to solve an editor display issue wrt centering was unfortunate.

The icon bc layer backport looks good - all the same except it's silent instead of issuing deprecations, test coverage the same except for not expecting deprecations. Didn't check the rest of the MR but I think that is all identical to the 11.x MR anyway.

@acbramley, took a peek at ✨ Drastically improve Drupal's default linking experience in text fields Needs work and, unfortunately it looks like the pain there would have been pain here had it gone in first, so lose-lose either way.

For #45 and #46, can you propose an edit to the release note in the issue summary for that? Then we can update the RC1 release notes and also use it in the 11.2.0/10.5.0 release notes. Thanks!

CKEditor also now adds the "table" class to all <table> elements, so themes using CSS rules that style tables differently based on the presence of that class may need those rules changed.

Add above to RN snippet in IS.

git check-ignore -v core/assets/vendor/ckeditor5/show-blocks/translations/be.js
.git/info/exclude:14:vendor	core/assets/vendor/ckeditor5/show-blocks/translations/be.js

So I reran the steps and got the same result for core/assets/vendor, the first time I was missing all new files thanks to an over zealous local gitignore

sorry for the noise, RTBC

BC break reported in Linkit: 🐛 Future compatibility fix for v45 of CKEditor5 Active

catch → closed merge request !12331

We should try to feed the linkit and ✨ Drastically improve Drupal's default linking experience in text fields Needs work bc breaks back into the change record when there's enough known to write it up in case it affects other contrib/custom modules, and maybe explicitly link to a new linkit release once there is one in the 10.5.0/11.2.0 release notes.

Committed/pushed to 10.5.x, thanks!

BC break reported in Editor Advanced Link: 🐛 D11.2: Uncaught CKEditorError: Cannot read properties of undefined (reading 'viewUid') Active

Also experiencing a BC break in CKEditor Abbreviation: 🐛 Incompatible with Drupal Core 10.5 / CKEditor 45 Active

Just in case anyone else arrives here looking for answers:
This update broke the Anchor Link module too. See 🐛 Missing icon (pencil) is breaking CKEDitor5 Active .

I can report another BC break in Embedded Content → module: 🐛 Uncaught CKEditorError: Cannot read properties of undefined (reading 'pencil') Active