Problem/Motivation

CKEditor v47 is out: https://github.com/ckeditor/ckeditor5/releases/tag/v47.0.0

It appears to have a large amount of breaking changes, so ideally we will get this in early in the Drupal 11.3 cycle to avoid the problems we had updating late to CKEditor v45 in Drupal 11.2.

Steps to reproduce

Proposed resolution

Update to CKEditor v47.

Remaining tasks

None

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3538768

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

longwave created an issue. See original summary.

longwave’s picture

Title: Update CKEditor 5 to 46.0.0 » Update CKEditor 5 to 47.0.0

We didn't do this as quickly as I hoped, and v47 is now out; let's try to upgrade to that.

longwave’s picture

Status: Active » Needs work

yarn build:ckeditor5-types deletes core/modules/ckeditor5/js/build/ckeditor5.types.jsdoc, with the following output:

[09:19:08] CKEditor 5 types have been generated: 0 declarations aliased, 1041 files ignored

Need to figure out what has broken here.

longwave’s picture

Status: Needs work » Postponed
Related issues: +#3527914: Use UMD installation method for CKEditor5

The big problem here is that DLL builds no longer seem to be supported, so this is apparently blocked on #3527914: Use UMD installation method for CKEditor5

longwave’s picture

Status: Postponed » Needs review

Actually DLL builds still work, we just need to make some plugin updates I think. A few exports have been changed and injectCssTransitionDisabler() is now CssTransitionDisablerMixin which I couldn't find an upgrade guide for, but seems to work.

longwave’s picture

Issue summary: View changes
longwave’s picture

Status: Needs review » Postponed

I don't really understand the jsdoc build step, but in PHPStorm I can get contextual documentation about any of the imports in the custom plugins without the jsdoc file anyway. I think the break is because of https://ckeditor.com/docs/ckeditor5/latest/updating/nim-migration/migrat... - but perhaps we just don't need this any more?

longwave’s picture

Status: Postponed » Needs review

Didn't mean to change status.

smustgrave’s picture

Ugh another big update? Some contrib modules have barely bounced back from the last update that broke everything

longwave’s picture

The changes aren't too bad, just if you are importing code from the API you might need to make some minor renames as per the linked document. The much bigger change comes in #3527914: Use UMD installation method for CKEditor5 which we will have to adapt to soon, because they are stopping support of the DLL loading method at the end of the year.

Some of the jsdoc types need updating as per https://ckeditor.com/docs/ckeditor5/latest/updating/nim-migration/migrat... - this is quite tedious and perhaps can be deferred to a followup. As an example

@param {module:engine/model/element~Element} modelElement

becomes

@param {module:engine/model/element~ModelElement} modelElement

I'm still not clear about #8, because PHPStorm appears to discover some types whether or not the types whether ckeditor5.types.jsdoc exists or not.

longwave’s picture

I think I fixed the jsdoc parser so it outputs the correct information for CKEditor5 v46 and above, still not sure if this is needed but I think it's as correct now as it was in the previous version.

I've also updated some of the existing jsdoc references for the renames that have occurred in CKEditor.

I think PHPStorm is now picking up the TypeScript types from CKEditor which is why I don't see much difference locally either way, but then e.g. this line still doesn't work locally and I don't understand why:

@extends module:core/plugin~Plugin

Still, that's probably out of scope here now given the jsdoc file appears to be largely correct again.

longwave’s picture

Issue summary: View changes
godotislate’s picture

MR changes for CKEditor libraries look pretty good, though I had one question on the MR about the changes to the jQuery UI map files and whether they are expected/in scope here.

injectCssTransitionDisabler() is now CssTransitionDisablerMixin

Found the PR https://github.com/ckeditor/ckeditor5/pull/13222/files where injectCssTransitionDisabler was deprecated, and it was not really otherwise documented.

@extends module:core/plugin~Plugin

I pulled this down locally, and I see this issue in PHPStorm as well, along with other ones like @param {module:link/link~LinkDecoratorDefinition} decorator or @param {module:core/editor/editor~Editor} editor. Though other ones like @extends module:core/command~Command are picked up just fine, so not sure whether there's an issue with the JSDoc or PHPStorm.

Also, it seems like the script might not pulling all the exports? For example, this was added to the JSDoc

/**
 * Declared in file @ckeditor/ckeditor5-adapter-ckfinder/src/utils.js
 *
 * @typedef {module:adapter-ckfinder/utils} module:adapter-ckfinder/utils~getCsrfToken
 */

But export function getCookie and export function setCookie in the same file aren't in the JSDoc, but this seems like an existing issue with the script and nothing introduced here. On a related note, there's this in the CKEditor v46 update docs, https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-46....

We spent the last several months discussing how to prepare the CKEditor 5 API layer for the years to come. This release is the result of the rules we are introducing from now on:

Every public API must be exported via the package’s index.ts.
Every internal API must be marked as such explicitly with @internal.
Exported names should follow a descriptive and unique naming pattern aligned with their purpose and context.
There should be no export default or export * from statements in source files.

Maybe JSDoc build script needs some updating, but I think that would be for a follow up.

longwave’s picture

Oops, didn't spot those jQuery UI changes, I remember this happening before but I don't remember why, the only change is that ,"ignoreList":[] is added/removed from the end of the map. Reverted this.

Re #14 I think the missing parts are a new bug in the jsdoc script. Previously it only looked for a single default export per file, now there can be multiple exports but I think we only pick up the first. Will see if I can improve this.

longwave’s picture

Fixed up the jsdoc script to handle multiple exports per file. The second regex wasn't matching anything any more so I have removed that one.

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

OK, jsdoc is picking up all typedefs from each file now, so I think this looks good.

One note is that I don't think PHPStorm is integrating with the jsdoc file at all, and even with some Googling, I'm not finding how to work with jsdoc files in any IDE, but I'm not sure that needs to be addressed here.

longwave’s picture

I suspect we can remove the jsdoc script and helper file entirely, will open a followup to discuss that.

I think I figured out why PHPStorm works with some files and not others; it's picking up the TypeScript definitions instead of the jsdoc, but CKEditor doesn't have @module tags in all the TS definitions.

command.d.ts has

/**
 * @module core/command
 */
...
export declare class Command extends /* #__PURE__ */ Command_base {

and this makes @extends module:core/command~Command work. But editor.d.ts doesn't have a @module tag so it doesn't get detected. Unsure if this is deliberate or not on CKEditor's part.

  • catch committed 62242b1b on 11.x
    Issue #3538768 by longwave, godotislate: Update CKEditor 5 to 47.0.0
    
catch’s picture

Version: 11.x-dev » 10.6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x, thanks! Better to get this in asap.

I think we should probably backport this to 10.6.x too, so moving to 'to be ported'.

godotislate’s picture

Status: Patch (to be ported) » Needs review
longwave’s picture

Status: Needs review » Reviewed & tested by the community

I get the same output in 10.6.x with

yarn
yarn vendor-update
yarn build:ckeditor5
yarn build:ckeditor5-types

  • catch committed 8a6af5e1 on 10.6.x
    Issue #3538768 by longwave, godotislate: Update CKEditor 5 to 47.0.0
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.6.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

catch’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jurriaanroelofs’s picture

@catch @longwave

I was wondering if beyond the dll build major changes, whether anyone noticed and tested there are breaking CSS styling changes from CKEditor5 v46. The CKEditor5 team decided to add opinionated CSS styles, affecting things like font-family and font-size within the editor.

In my DXPR Builder module this is especially annoying because we use the inline editor, but in Drupal core's use case this will probably still be a breaking change as users are accustomed to seeing the typography defined in Claro theme or Gin theme in their node form text editors, but now it will be Helvetica/Arial with different font sizes.

I created an issue about the problem here: https://github.com/ckeditor/ckeditor5/issues/19414