Refactor Linkit plugin for CKEditor5 v45+

jannakha → changed the visibility of the branch 3535479-refactor-linkit-plugin to hidden.

@mark_fullmer
Hi Mark,
Linkit and other modules with plugins for CKEditor5 are having lots of issues with v45 since it had some big changes.
I was testing linkit on OOTB 10.5 and 11.2 and it had some inconsistent behaviour (loosing attributes, not working with other CKEditor plugins) (Also CKEditor5 v45 have updated how "Displayed text" is being updated).

I've started refactoring of the plugin - let me know your feedback.

cheers,
Janna

Hi Jannahka,

Thanks so much for digging into the CKEditor v45 changes. These are certainly a priority. I'd begun some similar work, refactoring tests and refactoring for the problematic behavior in 🐛 Cannot replace 'Displayed text' of existing links Active . Ideally, I'd like to make only the minimal changes necessary and see if it is possible to retain backward compatibility with previous versions of CKEditor so that we don't have to change the core_version_requirement in a new release.

To that end, it will be important to document exactly what is known to be breaking, with steps to reproduce, so that we can ensure that we're only changing what is necessary and not breaking other things.

If it helps, an example of two modules that are removing LinkIt attributes when used:

editor_advanced_link -- https://www.drupal.org/project/editor_advanced_link/issues/3534044 🐛 Adding "Open in new window" removes all other attributes Active

ckeditor_link_styles -- https://www.drupal.org/project/ckeditor_link_styles/issues/3533302 🐛 LinkIt url aliases removed after link style applied Active

ckeditor5_plugin_pack -- if "open in a new tab" property is used

example of two modules that are removing LinkIt attributes when used:

Okay, thanks for helping connect the dots, @chrisla. Per https://www.drupal.org/project/editor_advanced_link/issues/3534044#comme... 🐛 Adding "Open in new window" removes all other attributes Active , another developer suggests that, contrary to intuition, this is not the responsibility of those modules to not remove other attributes, but rather, the responsibility of LinkIt to ensure the "persistence" of its attributes. I'll start looking into that.

Patch from MR122 cannot be applied to LinkIt 7.0.7

jannakha → changed the visibility of the branch 3535479-refactor-linkit to hidden.

minimal refactoring for CKEditor 45+ to make sure all linkit attributes persist.
tested on D11.2 with ckeditor5_plugin_pack link attributes and editor_advanced_link with patch from https://www.drupal.org/project/editor_advanced_link/issues/3534699 📌 Refactor custom JS for CKEditor5 v45+ Active

@mark_fullmer have a look at MR 127 - bare min changes

MR 127 does not apply for me with LinkIt 7.0.7 and Drupal 11.2.2

Thanks for the rework! Assigning myself for review...

Wasn't able to apply patch from MR 127. Code in build/linkit.js from Linkit 7.0.7 did not match contents of MR. Created my own patch here.

The patch in #18 applies cleanly and fixes the issue raised in 🐛 Adding "Open in new window" removes all other attributes Active

I'm not sure how many people need to say it works before it can be moved to RTBC?

Drupal core: 10.5.1
Linkit: 7.0.7
Editor Advanced Link: 2.3.1 (with aforementioned patch)

I manually tested and also executed the relevant automated tests against Drupal 10.4.8 to ensure that this is backwards compatible with sites that are using Linkit with CKEditor v44 or below. Everything checks out, so this change does not need to change the core version compatibility for the 7.x branch of Linkit. Thanks, everyone for the contributions!

@mark_fullmer - Linkit plugin needs to be refactored for CKEditor 45+ anyway - 7.0.8 is just a temp solution:
here's a reported issue 📌 Refactor custom JS for CKEditor5 v45+ Active

@mark_fullmer - Linkit plugin needs to be refactored for CKEditor 45+ anyway - 7.0.8 is just a temp solution:

I assume by that you mean that the refactoring would require breaking backwards compatibility with CKEditor <= v45, in which case, we'd want to reflect this in semantic versioning, i.e., switching from 7.0.x to 7.1.x...

Unfortunately yes.

Unless MR122 works on D10.4
I haven’t tested.

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