-
Notifications
You must be signed in to change notification settings - Fork 40
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
Comments
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:
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. |
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. |
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):
|
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. |
Several new versions of CKEditor have come out, we should update before testing this further. |
Bad news, our build script fails with ckeditor 41.4.2 (probably all 41.4.x) Probably has to do with this "minor breaking change":
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). |
With a "slight" 🤡 update our improvised build script spits out an editor, that initializes. 🥳 As 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 I bet, our reworked build script will break again, soon. |
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. |
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. |
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? |
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. 🙏 |
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.
And then there's also the language problem (currently there's no language pack available as download). |
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. 😬 |
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). 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. |
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. |
Please clarify which PR is the one to test. Thank you. |
@izmeez it's the newer one - the one with 42.0.0. |
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 |
I've also done some testing of backdrop/backdrop#4820 and it's working smoothly. |
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? |
@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) |
Yes, I seem to have sufficient permissions for both. 👍 |
I can also edit the pages when I think of stuff to add. |
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. |
@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. |
@indigoxela Thanks, yes that was it. I think I was told to do that before and forgot. So yes, everything works flawlessly. |
Note that in the meantime there's a new release again - v43.0.0, but that one looks like a real breaker. 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). |
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. |
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. |
Follow-up issue filed (keeping the same summary and title as this issue had previously): #6695 |
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
The text was updated successfully, but these errors were encountered: