Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CKEditor 5 to v42.0.2 #6481

Closed
4 of 5 tasks
klonos opened this issue Apr 27, 2024 · 42 comments · Fixed by backdrop/backdrop#4820
Closed
4 of 5 tasks

Update CKEditor 5 to v42.0.2 #6481

klonos opened this issue Apr 27, 2024 · 42 comments · Fixed by backdrop/backdrop#4820

Comments

@klonos
Copy link
Member

klonos commented Apr 27, 2024

It would be nice to be updating to the latest release regularly (I am proposing with each minor release - we could do it in the 2 weeks between feature freeze and final release). ...if we don't keep up and we are several versions behind, then if a security release comes out we risk rushing to release the secure version, which might be introducing breaking changes, and we won't have enough time to test/adjust/fix things.

We are currently shipping core with v5, build 40.2.0. A few 41.x builds have been released since:

https://ckeditor.com/blog/categories/releases highlights the main improvements with new releases:

More resources related to releases

@quicksketch
Copy link
Member

PR at backdrop/backdrop#4726 updates us to 41.3.1.

I couldn't find any substantial changes that affect Backdrop's CKEditor installation. Some notes about new features and how they (don't) affect us:

  • List plugin is deprecated and DocumentList is now the only supported list plug. We are already using DocumentList
  • New Dialog UI Backdrop provides its own dialogs and does not use any CKEditor plugins that use dialogs.
  • Multi-level LIsts plugin This is a premium-only feature and not available in Backdrop
  • New Case Change Plugin We could decide to include this plugin if we think it is useful.

So seems like a straight-forward update process. I followed the UPGRADING.md file and there doesn't seem to be any issues in my testing.

@quicksketch quicksketch added this to the 1.28.0 milestone May 2, 2024
@indigoxela
Copy link
Member

New Dialog UI

Haven't had a chance to take a closer look, but would this help us with notifications? That's still pending. We're using an alert at one place and other messages are simply missing (file upload stuff).

I don't think, this has to be part of 1.28. I guess, we can update CKE any time we need. Especially, if the changes are minimal, anyway.

@klonos
Copy link
Member Author

klonos commented May 2, 2024

New Case Change Plugin We could decide to include this plugin if we think it is useful.

Yeah, I like that one. It helps with the styling of headings + sometimes people copy/paste text from other sources where others have typed everything in all-caps (a habit for some on mobile devices). That feature allows fixing that with a couple of clicks.

...other features like the following also seem useful and pretty standard (content editors expect to have those available):

  • import from Word
  • export to Word/PDF
  • Math Equations (.edu site content editors would appreciate that OOTB)
  • Find and replace
  • Table of contents
  • Font family (would help in our documentation site when formatting code with monospace for example)
    ...if these aren't premium of course.

@quicksketch
Copy link
Member

I don't think this would be a good idea to shove into 1.28.0 (today). Let's target this for 1.28.1, even though it's less-than-ideal that it will be in a bug fix release.

@quicksketch
Copy link
Member

Several new versions of CKEditor have come out, we should update before testing this further.

@indigoxela
Copy link
Member

indigoxela commented Jun 22, 2024

Bad news, our build script fails with ckeditor 41.4.2 (probably all 41.4.x)
We end up with an unusable 9.7MB file, the editor fails to initialize.

Probably has to do with this "minor breaking change":

The ckeditor5 package now lists all other official open-source @ckeditor/ckeditor5-* packages as dependencies.

See full changelog here: https://github.com/ckeditor/ckeditor5/releases/tag/v41.4.1

So we probably need a new build strategy with 41.4+

For now I'd suggest to stick with v41.3.1, which seems to be the newest version we can use without overhauling our build process (again).

@quicksketch that means, your current PR is up-to-date and only needs a rebase (to get a Tugboat sandbox).

@indigoxela
Copy link
Member

indigoxela commented Jun 22, 2024

With a "slight" 🤡 update our improvised build script spits out an editor, that initializes. 🥳

As npm install ckeditor5 now downloads each and every OSS plugin, we can use our plugin list as filter instead.

TBH, for my understanding the 41.4.x release notes contain the word "breaking" too often. Not sure if we should really switch to the latest, yet. Maybe later, after some intense testing?
And some plugins seem deprecated (DocumentList, DocumentListProperties (they actually just got renamed and there's still fallback behavior)), so the change for 41.4 doesn't seem suitable for a patch release (or is it?).

And I bet, our reworked build script will break again, soon.

@quicksketch
Copy link
Member

Thanks @indigoxela, I really appreciate your work to chase CKEditor. Maybe next Backdrop LIVE we have a session on replacing CKEditor. Seems like a big shift for our users but it's worth considering at least.

I'll take a look at this tomorrow, it does seem like a big jump for a bug fix release, so we might want to postpone it, at least until we have more time for adequate testing.

@quicksketch quicksketch modified the milestones: 1.28.1, 1.28.2 Jun 24, 2024
@indigoxela
Copy link
Member

indigoxela commented Jun 24, 2024

I took a quick look, if we could benefit from the newly introduced CKE5 "dist" folder(s) and contents. I don't think so. It'll just cause another step on our side to collect (concat) js files. We'll eventually need an independent build strategy, as soon as they drop the "build" folder (dll) concept. Not sure, what their approach for projects like ours will be, honestly. I'm not even sure, whether they understand what we'd actually need. 😉

However, let's move on here.
Testing, we need some testing.
A fresh PR sandbox is available, waiting for curious testers. 🙌

@olafgrabienski
Copy link

Testing, we need some testing.

I've edited existing content on the sandbox site and created a test page. At first sight, the editor works fine. Are there particular things to look at?

@indigoxela
Copy link
Member

indigoxela commented Jun 25, 2024

At first sight, the editor works fine. Are there particular things to look at?

Gosh, if we knew that in advance, life would be so much easier. 😜

Editing existing content sounds interesting. Like something copied over from an existing site (into raw html format, then switch). Or... drag-and-drop image upload. Content copied from a document or other sites. Trying out different plugin constellations (by enabling/disabling in the filter settings and checking, if the editor chokes on something).

But basically, any testing is helpful. 🙏

@indigoxela
Copy link
Member

indigoxela commented Jun 29, 2024

Okay... CKEditor 42.0.0 has been released. https://github.com/ckeditor/ckeditor5/releases/tag/v42.0.0

It now ships with a new installation method, which means, we'll have to consider that again. 🤷

Hm... quite an overhaul...

I did a quick check, if we could use their new CDN download with our setup and custom plugins - no, we can't.

Uncaught SyntaxError: export declarations may only appear at top level of a module

And then there's also the language problem (currently there's no language pack available as download).

@indigoxela
Copy link
Member

For now our overhauled build script still works (it's only one week old, glad that it's not broken again...).

Here's a PR that updates to 42.0.0. Seems to work.

Long term I'm afraid, we'll have to overhaul our CKE integration. And we really have to keep an eye on their move to deprecate and drop dll builds. There's no statement for an EOL, yet, but they seem to move fast. 😬

@indigoxela
Copy link
Member

indigoxela commented Jun 30, 2024

With their 42.0.0 release CKEditor5 made another step away from DLL builds. Their new approach would require to either overhaul, how we integrate the editor, or to create the DLLs on our own (in a custom build step).
Both seems quite some effort.

There aren't many contrib CKE5 plugins, but some do exist (feef, video_filter, ckeditor_inline_image_style), so changing our API would have consequences for those.

Our current CKE5 version falls behind. We should really try to catch up again, so we're not running into major trouble, if a security release comes out.
For now our improvised build approach still works (with modifications). But how long?

@quicksketch
Copy link
Member

quicksketch commented Jul 1, 2024

For now our improvised build approach still works (with modifications). But how long?

I'm in support of getting a least the next release or three using our new build process, which continues to use DLLs. But the next minor release it might be good to switch to the new CKEditor loading mechanism. If it's somehow possible to use both the new loading mechanism and DLLs at the same time, that would be ideal. But yeah... headaches.

@jenlampton jenlampton removed this from the 1.28.2 milestone Jul 3, 2024
@izmeez
Copy link

izmeez commented Jul 15, 2024

Please clarify which PR is the one to test. Thank you.

@indigoxela
Copy link
Member

Please clarify which PR is the one to test. Thank you.

@izmeez it's the newer one - the one with 42.0.0.

@olafgrabienski
Copy link

I have not tested the whole CKE5 functionality. Perhaps @olafgrabienski could do that?

Not the whole functionality, but at least something: I've created a test page, inserted some images in different ways, tested many text formatting options, added some buttons to the Basic format, and all that worked fine. So works for me, but more testing is welcome.

@izmeez
Copy link

izmeez commented Jul 17, 2024

I've also done some testing of backdrop/backdrop#4820 and it's working smoothly.

@indigoxela
Copy link
Member

Truly ongoing... 😉 there have been two patch releases of CKE5 in the meantime, so I updated the PR to catch up. Now 42.0.2.

@klonos as this is a meta issue (sort of), I'm not sure, where we could put the "CKE checklist for manual tests". Is this issue's description even a good place?
FTR: this has been discussed briefly in our chat: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/CKE5.20testing.20catalogue

@klonos
Copy link
Member Author

klonos commented Aug 3, 2024

@indigoxela I believe that https://github.com/backdrop/backdrop-issues/wiki would be the best place for such a thing (and we should also have one such checklist for testing jQuery upgrades). I went ahead and created them:

Can you please check and let me know if you have the right permissions to edit those? (and anyone else interested to have edit access)

@indigoxela
Copy link
Member

indigoxela commented Aug 4, 2024

Can you please check and let me know if you have the right permissions to edit those?

Yes, I seem to have sufficient permissions for both. 👍

@izmeez
Copy link

izmeez commented Aug 4, 2024

I can also edit the pages when I think of stuff to add.

@izmeez
Copy link

izmeez commented Aug 4, 2024

Testing CKE5 v42.0.2 and noticed there is no option for Header 2. Is this a separate issue, why there is no Header 2 option? I vaguely recall encountering it before.

@indigoxela
Copy link
Member

...noticed there is no option for Header 2

@izmeez That's a setting, h2 is off by default, but you can easily turn it on. Go to admin/config/content/formats/filtered_html, in section "Headings" (vertical tabs) there are checkboxes for h1 to h6 - turn on the ones you need.

@izmeez
Copy link

izmeez commented Aug 5, 2024

@indigoxela Thanks, yes that was it. I think I was told to do that before and forgot. So yes, everything works flawlessly.

@indigoxela
Copy link
Member

Note that in the meantime there's a new release again - v43.0.0, but that one looks like a real breaker.
Let's just try to get v42 in, so we catch up a bit, and see, what we can do with 43 later. 😬

https://github.com/ckeditor/ckeditor5/releases/tag/v43.0.0

I just rebased my (active) 42.0.2 branch, won't provide a PR with their new release until after our next minor release (1.29).

@quicksketch
Copy link
Member

Perhaps at some point the person who wrote this was planning to include the ability to actually set the caption in the modal itself?

The was probably me, and yes I think that was my intention at one point in time. But since captions can support links and nested formatting (like bold/italic), editing the caption in the dialog really wasn't practical. I think that previous code was attempting to convert the caption DOM element into raw HTML so that it could be edited. But as pointed out by @indigoxela, the dialog doesn't actually show a field for caption so the JS to send the caption data is unnecessary. If we add caption editing to the dialog, we can add back a replacement that works.

I code reviewed and everything looks good to me. I also gave it another testing and I found no issues.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4820 into 1.x for 1.29.0! 🎉

It took a while to draw the line but with 1.29.0 around the corner we had to do it eventually. I give a big thank you to @indigoxela for being persistent and resilient chasing updates and fixing API breaks! Thank you @argiepiano for diving in and helping with the code!

And thank you @izmeez, @klonos, and @olafgrabienski for QA testing!

I think we should retitle this issue and open another one for further CKEditor updates, this issue has gotten long and confusing enough.

@quicksketch quicksketch changed the title [ONGOING] Keep CKEditor 5 up to date (preferably with each minor release, unless it is a security-related update) Update CKEditor 5 to v42.0.2 Aug 30, 2024
@quicksketch
Copy link
Member

Follow-up issue filed (keeping the same summary and title as this issue had previously): #6695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment