Disable CKEditor 5's automatic link decorators (in Drupal filters should be used instead)

Discussed during last week's CKEditor 5 meeting. @lauriii, @Reinmar and I agreed on an approach.

Explicit test coverage for manual link decorators on <img> (still to do: <drupal-media>) added. It doesn't work correctly in any of the 4 image linkability cases, not even in the case where Drupal does not intervene at all: the linked inline image case.

Expected test output:

Testing /Users/wim.leers/Work/d8/modules/ckeditor5/tests/src/FunctionalJavascript
FFEF                                                                4 / 4 (100%)

Time: 1.33 minutes, Memory: 8.00 MB

There was 1 error:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testLinkability with data set "INLINE image, restricted" ('inline', false)
Behat\Mink\Exception\ElementNotFoundException: Element matching css "a[href="http://www.drupal.org/association"][foo=bar] img[src*="image-test.png"]" not found.

/Users/wim.leers/Work/d8/vendor/behat/mink/src/WebAssert.php:418
/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/WebAssert.php:968
/Users/wim.leers/Work/d8/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php:253
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

--

There were 3 failures:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testLinkability with data set "BLOCK image, restricted" ('block', false)
Failed asserting that actual size 0 matches expected size 1.

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:119
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/wim.leers/Work/d8/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php:226
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

2) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testLinkability with data set "BLOCK image, unrestricted" ('block', true)
Failed asserting that actual size 0 matches expected size 1.

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:119
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/wim.leers/Work/d8/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php:226
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

3) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testLinkability with data set "INLINE image, unrestricted" ('inline', true)
Failed asserting that actual size 0 matches expected size 1.

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:119
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/wim.leers/Work/d8/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php:242
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

1. The INLINE RESTRICTED case is an upstream bug: https://github.com/ckeditor/ckeditor5/issues/10828.
2. The INLINE UNRESTRICTED case is 98% likely caused by the same upstream bug, it just fails earlier because than the RESTRICTED case due to an additional explicit assertion that adding an arbitrary class in source mode works.
3. The two BLOCK cases are bugs in our code, but could also require a bugfix upstream, possibly this same upstream bug, TBD

Update IS to clarify what work remains here.

Transfering the MR…

Hm … none of my recently pushed commits are listed here… 🤔

Drupal core merge request 1418 definitely does have the equivalent of CKEditor 5 merge request 160 now (just with the phpcs "fix" commit applied to the commits where the violations were introduced).

Reported this in #drupal-infrastructure in Drupal Slack: https://drupal.slack.com/archives/C51GNJG91/p1637073092169800

Yay, https://github.com/ckeditor/ckeditor5/issues/10828 has been fixed upstream! Should be in the next release, December 7.

We should try this with the latest upstream CKEditor 5, to see if this then passes more tests. But … then we first need tests to actually work again 😳 I'm working with the Drupal Association to figure this out…

Seems like it's pretty complicated to pin to a specific commit of a package in package.json 😬

The problem is that the specific commit does get installed, but npm/yarn then inspect the package.json in that specific checkout.

Solution per @Reinmar: use yarn workspaces:

1. Clone the CKE 5 repo, check out specific commit
2. Configure yarn workspaces, like so: https://github.com/ckeditor/ckeditor5/blob/master/package.json#L192, pointing to the code checked out in step 1.
3. yarn install will then first look in that locally available checkout instead of using the external repository of npm modules

@lauriii understands this better than I do, so together we should be able to figure it out :) The downside is that each person working on an MR will have to repeat some of these manual steps.

Rather than going through the painful and uncertain process in #19, in the meantime a new version of CKEditor 5 was released that includes the commit this MR needs.

So we can instead wait for #3258250: Update to CKEditor5 v31.1.0 to land.

#3258250: Update to CKEditor5 v31.1.0 is itself blocked on #3258371: fix yarn vendor-update command — updating title.

Both #3258371: fix yarn vendor-update command and #3258250: Update to CKEditor5 v31.1.0 landed, this is now unblocked!

Self-assigning since I last worked on this.

I'm pretty sure this is stable-blocking.

#3248228: Unable to change selection after linking inline media when manual decorators have been defined is adding a core/modules/ckeditor5/tests/modules/ckeditor5_manual_decorator_test test module that is far better than the one I wrote for this MR. Let's wait for that MR to land first.

#3248228: Unable to change selection after linking inline media when manual decorators have been defined landed, which means this is unblocked.

Quite a lot of this was fixed in #161217: URL filter breaks generated href tags but it can still cause unpredictable bugs with other modules from what I've seen.

Ahh! This explains a lot, I was really surprised that filter_url generally works okay with CKEditor 4 and 5. (We recently refined the upgrade path to not complain loudly when you're using filter_url in #3273312: Upgrading from CKEditor 4 for a text format that has FilterInterface::TYPE_MARKUP_LANGUAGE filters enabled).

I think it would be good to eventually discourage it in favour of encouraging URLs to be explicitly linked in content,

+1

does this de-emphasise that option, or is it just the target="_blank" and similar?

This is 100% unrelated to/independent of filter_url.

But it is related to additional filters, and the fact that we want to encourage/emphasize that over using CKEditor 5 magic, because that is not guaranteed to bring equally consistent results.

• "manual link decorators" in CKE5: "They match links against pre–defined rules and manage their attributes based on the results." → no interaction, for example automatically assigning target="_blank" for external URLs
• "automatic link decorators" in CKE5: "They match links against pre–defined rules and manage their attributes based on the results. → interaction required, for example to expose a checkbox to toggle target="_blank" or set the download attribute

See https://ckeditor.com/docs/ckeditor5/latest/api/module_link_link-LinkConf....

The issue title says Disable CKEditor 5's automatic link decorators (in Drupal filters should be used instead) → the point is that target="blank_" getting auto-added should not happen inside CKEditor 5, but should happen during Drupal's filtering, through a @Filter plugin.

Perfect, glad we’re on the same page! 😄

I forgot to ask earlier: what *are* those problems you are still seeing with using filter_url?

I think this needs to start a new merge request — part of the current MR (manual decorator test coverage) already landed in #3248228: Unable to change selection after linking inline media when manual decorators have been defined — see #25.

That'll be easier than updating this one. You can still copy some parts of it over though :)

Hm, maybe #3248228 only landed explicit manual link decorator test coverage for Media, not for Images.

I think that's reasonable.

The stable blocking piece here is that we want to ensure not a single Drupal site starts using automatic link decorators, because forbidding that in the future would be a backwards compatibility break once ckeditor5.module is stable.

Retitling.

This actually makes the scope a lot clearer! 🥳

For #37. That follow-up won't be stable-blocking.

This looks great, but one part is not yet done:

Prevent automatic link decorators on client side.

But … after this many months since I looked at the details, I actually do not recall how one could set up automatic link decorators client side?

I think the idea is that you could modify drupalSettings.editor — either on the client side through JS or on the server side through hook_js_settings_alter().

To catch that, we could add detection logic in core/modules/ckeditor5/js/ckeditor5.es6.js. But thinking about that now, that seems fairly extreme. That'd also violate the separation of concerns: we'd make the generic CKEditor 5 instantiation logic tightly coupled to the link plugin. That seems disproportionate? 🤔

So I would be fine with omitting that part.

Thoughts?

I'm wondering if it would be a bit excessive to write test coverage for that 🤔

I think it would be.

Let's run all tests now!