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

CKEditor 5 - Problem creating links #6359

Closed
stpaultim opened this issue Jan 9, 2024 · 22 comments · Fixed by backdrop/backdrop#4630
Closed

CKEditor 5 - Problem creating links #6359

stpaultim opened this issue Jan 9, 2024 · 22 comments · Fixed by backdrop/backdrop#4630

Comments

@stpaultim
Copy link
Member

stpaultim commented Jan 9, 2024

Description of the bug

We are in the process of upgrading a Drupal 7 site to Backdrop CMS. We updated to the 1.27 preview release and upgraded to CKEditor 5.

We were not able to create links until we added the image tool to the toolbar. Once this was added, things worked as expected. However, I think we've decided that this is a bug that needs resolving.

See: #6359 (comment)

image

@stpaultim
Copy link
Member Author

As I expected, this is not a problem on a fresh site running CKEditor 5. So it might be something specific to our site or it might be something related to the upgrade.

We'll try to do some more investigations and report our results. But, if anyone has any suggestions, please let us know.

@indigoxela
Copy link
Member

indigoxela commented Jan 9, 2024

If you enable the image plugin (drag the icon into the active toolbar) on admin/config/content/formats/filtered_html, does the error persist?

Edit: another idea, can you provide the json export for your current text format?

Go to admin/config/development/configuration/single/export and export "Text formats" - "Basic". Or whatever your textformat currently is.

@stpaultim
Copy link
Member Author

stpaultim commented Jan 9, 2024

@indigoxela - Adding the image plugin/icon to toolbar solved the problem.

This seems like a bug to me, because we were not trying to add a link to an image. There is no obvious connection between these two.

I'm checking to see if this is an issue on a new site. Maybe it's something about our upgraded configuration?

EDIT: I tried removing the image plugin from the toolbar on a new site to see if that would break the link tool. It did not.

Any ideas why that happened on our upgraded site? Is this a bug or something that can be fixed in the CKEditor 4 to 5 upgrade process?

@quicksketch
Copy link
Member

@stpaultim Yes this is definitely a bug. @indigoxela probably thinks so too, but by adding the Image button it makes available the missing ImageUtils plugin. Looks like we need to wrap a section of logic in an if statement to check if images are even supported before trying to add link support to them. Or we could make the BackdropLink plugin depend on ImageUtils even if it's not always used. The tag filter will still block <img> tags if needed and the logic could stay as-is.

@quicksketch quicksketch added this to the 1.27.0 milestone Jan 10, 2024
@indigoxela
Copy link
Member

indigoxela commented Jan 10, 2024

@stpaultim my suggestion to try enabling the image toolbar button wasn't an attempt to claim this isn't a bug. 😉 It is.

It was an attempt to verify that there's something wrong in the BackdropLink vs. ImageUtils logic. Although I didn't see the problem when disabling the image button in toolbar. A bit more digging might be necessary.

@stpaultim
Copy link
Member Author

Thanks. That all makes sense?

Let me know if there is something I can test or do to help.

@quicksketch
Copy link
Member

I filed a PR at backdrop/backdrop#4630 that just loads the missing plugin. Since the entire ckeditor-dll.js file is loaded regardless of which plugins are requested, there is no difference in the network file size. And having ImageUtils available all the time is a lot easier than several conditional statements to see if it's available. Note loading ImageUtils does not mean images are allowed, it just helps detect them if they're present. And it helps in the edge-case of a text format that might allow <img> tags even if the backdropImage button is not in the toolbar.

@indigoxela
Copy link
Member

Let me know if there is something I can test or do to help.

@stpaultim In fact, there is. I'm still struggling to replicate the console error from your screenshot. What I tried so far: add a linked image in content, then go and remove the image icon from toolbar, then open the node form again and try to add a fresh text link. But that works for me. I do see some nagging in console, but no error and adding or editing links just seems to work fine.

So, while the PR makes sense to me, I can't verify yet, that it fixes the actual problem you're facing.

@quicksketch
Copy link
Member

quicksketch commented Jan 11, 2024 via email

@indigoxela
Copy link
Member

indigoxela commented Jan 11, 2024

After multiple flush-cache I had the quick idea to also remove the "Heading" dropdown button from the toolbar... Wow. Without that I can't type even plain text. 😨 But that seems unrelated here. Issue created: #6363

Started fresh (removed editor from text format, added it back after saving), but I get the same result:

Lots of image-style-missing-dependency... warnings, but adding links in the editor works just fine.

@stpaultim
Copy link
Member Author

stpaultim commented Jan 13, 2024

@quicksketch - I am not able to recreate the problem using your steps. I originally experienced this problem on a site that had been upgraded from Drupal 7 and had existing content in it after upgrading from CKEditor 4.

I have not yet been able to recreate this problem on a new site that starts with CKEditor 5 as default option.

EDIT: After much experimentation. I can not recreate the problem with links in a fresh site using current dev branch.

@stpaultim
Copy link
Member Author

stpaultim commented Jan 13, 2024

@quicksketch or @indigoxela - In trying to recreate this issue on a fresh site, I found a different problem, which may be related or maybe it's a new issue.

  1. Spin up a brand new site with dev branch
  2. Remove the "Headings" icon from the CKEditor 5 Toolbar (Basic text editor)
  3. Try to create a new page

Note it is impossible to type anything in the body field.

Do we think this might be related or should I open a new issue for this?

EDIT: Darn, I'm too late. @indigoxela already found this problem and opened a new ticket.
#6363

And I thought I was sooooo, clever.

@herbdool
Copy link

I couldn't recreate the problem either. Must be only under a full moon or something. Even with the PR I see the image-style-missing-dependency.

@stpaultim
Copy link
Member Author

@herbdool - The problem was originally reported from a site that had been upgraded from Drupal 7 to Backdrop CMS using CKEditor 4 and then CKEditor 4 upgraded to CKEditor 5. So, I expect that there a large number of potential factors in that scenario that might be different from trying to recreate the problem on a fresh Backdrop site.

If I remove the "Image" icon from the toolbar, the problem returns. I have not tried to apply the PR to this site yet to test it. But, I hope to do that tonight.

@bdalomgir
Copy link

I have applied this PR it fixed the link issue, and I found another issue with the image icon it is unable to add images by clicking the image icon.

after-clicking-the-image-icon

Note: The image icon is working fine in the fresh backdrop installation.

@stpaultim
Copy link
Member Author

stpaultim commented Jan 15, 2024

@bdalomgir

Do you know for sure that the image icon worked before you added the PR? Or is it possible that the image icon never worked on this site.

Can you confirm that after adding the PR, you can remove the image icon/tool and the link icon/tool still works?

NOTE: If anyone is not clear @bdalomgir is working on the project with me and this is the site that originally reported this problem.

@bdalomgir
Copy link

@stpaultim

  1. After adding the PR I can create link in either case by removing the image icon from the toolbar or not.
  2. The image icon was broken even if before adding PR also after adding PR, the image icon is working for CKEditor 4.

@herbdool
Copy link

It's important to try to create the error and the PR with a clean site. You can also test it with an existing site, but only after we've confirmed this all with a clean site.

@herbdool
Copy link

I can now reliably recreate the error and the fix if I create a new text format before and after.

@quicksketch The PR isn't enough on its own. I had to also enable either "Float images left and right using the data-align attribute" or "Convert image captions to figure and figcaption elements" option.

@indigoxela
Copy link
Member

I can now reliably recreate the error and the fix if I create a new text format before and after.

@herbdool can you export the JSON config of such a broken format? Any obvious difference? Or is it a (format) caching problem?

@herbdool
Copy link

I take it back. I should have taken my own advice and tested with a completely clean site. I still had the contrib ckeditor5 sitting there. Once I removed that completely it worked for me.

@quicksketch
Copy link
Member

Thanks folks!

I take it back. I should have taken my own advice and tested with a completely clean site. I still had the contrib ckeditor5 sitting there. Once I removed that completely it worked for me.

Yeah I had that hiccup a few times as well. Since the contrib module "wins" fixes to core don't seem to apply.

I merged backdrop/backdrop#4630 into 1.x for 1.27.0. There are other related issues like #6366 that we'll work on separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants