search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Update CKEditor 5 to v42.0.2 #6481

Closed klonos closed 1 month ago

klonos commented 5 months ago

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 commented 5 months ago

PR at https://github.com/backdrop/backdrop/pull/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.

indigoxela commented 5 months ago

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 commented 5 months ago

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):

quicksketch commented 4 months ago

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 commented 3 months ago

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

indigoxela commented 3 months ago

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 commented 3 months ago

With a "slight" :clown_face: update our improvised build script spits out an editor, that initializes. :partying_face:

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 commented 3 months ago

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.

indigoxela commented 3 months ago

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. :wink:

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

olafgrabienski commented 3 months ago

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 commented 3 months ago

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. :stuck_out_tongue_winking_eye:

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. :pray:

indigoxela commented 3 months ago

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. :shrug:

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 commented 3 months ago

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. :grimacing:

indigoxela commented 3 months ago

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 commented 3 months ago

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.

indigoxela commented 3 months ago

I closed my previous PR (with 41.4.2) as outdated, and rebased my 42.0.0 PR to get a fresh sandbox.

More testing, maybe? There can never be too much testing. :grinning:

https://github.com/backdrop/backdrop/pull/4820

olafgrabienski commented 2 months ago

There can never be too much testing.

Indeed! Generally the editor seems to work fine. Testing the image handling in the sandbox I found two issues:

(1) Pasting an image from my computer to CKEditor works, but I get two messages:

(2) Editing an image with a caption does not work. To edit such an image, I had to switch to source mode and remove the data-caption="My capition" part from the image tag.

indigoxela commented 2 months ago

@olafgrabienski wow, many thanks for testing, I can confirm both problems.

The "max_dimensions" thing (1) should be easy to fix. (Though I can't reproduce on my dev box - only in the Tugboat sandbox.)

The latter (2, broken figure/caption handling) causes an uncaught JS error: Uncaught CKEditorError: t.hasStyle is not a function, which probably means, our custom backdrop-image plugin isn't compatible anymore. :grimacing: Or they juggled again in their internal dependencies? Who knows...

indigoxela commented 2 months ago

The Undefined array key "max_dimensions" problem should be fixed in latest commit.

The CKEditor error... no clue.

Another way to get the dialog working again is to toggle the caption off in the bubble toolbar. Then the dialog opens again without problems. Maybe parsing the caption content causes this ... or some conversion problem.

Uncaught CKEditorError: t.hasStyle is not a function
Read more: https://ckeditor.com/docs/ckeditor5/latest/support/error-codes.html#error-t.hasStyle is not a function

^^ That "Read more" link doesn't lead anywhere. :shrug:

klonos commented 2 months ago

@indigoxela do any of these help?:

indigoxela commented 2 months ago

@klonos thanks for the links, whoever will pick this issue up, will appreciate this additional info. :wink: That's not me, though.

olafgrabienski commented 2 months ago

The Undefined array key "max_dimensions" problem should be fixed in latest commit.

Confirmed, tested in the sandbox: The warnings when inserting an image via copy & paste and via drag & drop are gone.

... toggle the caption off in the bubble toolbar

Thanks, good to know – I hadn't noticed that button so far.

indigoxela commented 2 months ago

LOL, I "fixed" the problem with image captions. :clown_face:

https://github.com/backdrop/backdrop/pull/4820/files#diff-c1199e94dc6724dee9b7b7722ef93d58dba9fccbc413f6720a225cfdc2445015R854-R859

At least, I narrowed the problem down to one line in backdrop-image.js. And we have a weird workaround.

I seems like the whole conversion at this point is futile. We only need the attribute to be set - the value seems irrelevant. At least, in my testing. Can anyone confirm that? My assumption: it's because the dialog doesn't handle the caption content, anyway. It just toggles it on/off, so the value doesn't matter.

indigoxela commented 2 months ago

Off topic: the spellchecker's acting again with property/method/file names. So I'm just ignoring it. Additionally it's unrelated (unchanged) code, anyway.

argiepiano commented 2 months ago

@indigoxela I got my feet into this rabbit hole a bit. Apparently, in the new version of CKE5, we can't use the method DataProcessor::toData()as in line 855 (where it was commented out to avoid the error). imageCaption.getChild(0) returns an object of class Node, but the toData() method now enforces receiving an argument of type viewFragment, which is the reason we were getting that console error.

I've done some hacking/testing, and have come to realize that BackdropImageCommand::execute() actually does not even need to set up existingValues['data-caption']! The execute() function is used when you edit an image within CKE5 - it basically sends info to the "Edit image details" Backdrop modal form. That modal form is built in filter_format_editor_image_form(), which actually doesn't need $values['data-caption'] at all! It only needs $values['data-has-caption'], which is used in the form for the "Has caption" checkbox.

So, I tried commenting out the line existingValues['data-caption'] = ''; in backdrop-image.js, and the editor and modal both kept working just fine. So, I believe we can get rid of the whole if statement that surround your "Fun fact" comment.

When CKE5 sets up the image when editing, the actual caption is set up by js function converter(). In there, the toData() method is used correctly - it does received a viewFragment this time, in line 429.

So, I don't know what the rationale for using toData to pass the caption content back to the modal - but in my view, this is not needed at all, and in fact, the existing code for the modal form supports this. Perhaps at some point the person who wrote this was planning to include the ability to actually set the caption in the modal itself?

indigoxela commented 2 months ago

I got my feet into this rabbit hole a bit.

:grinning: many thanks for your digging!

I believe we can get rid of the whole if statement that surround your "Fun fact" comment.

Wow, confirmed, filter_format_editor_image_form() doesn't need that value at all. Everything works as expected without it. :+1:

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

Maybe? :thinking: We'll figure out in the reviews. For now I'm glad that we can skip this part completely. (It wouldn't have worked, anyway, as only the first child got extracted.)

argiepiano commented 2 months ago

The part of the code I reviewed (related to the caption) works for me with the new changes. I hesitate to remove the needs testing and needs code review labels, as I have not tested the whole CKE5 functionality. Perhaps @olafgrabienski could do that, since he did more thorough testing?

izmeez commented 2 months ago

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

indigoxela commented 2 months ago

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

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

olafgrabienski commented 2 months ago

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 commented 2 months ago

I've also done some testing of https://github.com/backdrop/backdrop/pull/4820 and it's working smoothly.

indigoxela commented 2 months ago

Truly ongoing... :wink: 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 commented 2 months ago

@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 commented 2 months ago

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. :+1:

izmeez commented 2 months ago

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

izmeez commented 2 months ago

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 commented 2 months ago

...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 commented 2 months ago

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

indigoxela commented 1 month ago

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. :grimacing:

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 commented 1 month ago

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 commented 1 month ago

I merged https://github.com/backdrop/backdrop/pull/4820 into 1.x for 1.29.0! :tada:

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 commented 1 month ago

Follow-up issue filed (keeping the same summary and title as this issue had previously): https://github.com/backdrop/backdrop-issues/issues/6695