[PP-1] Make ready for CKEditor 5

Asked the CKEditor team for guidance.

We are using this module on a project that is in arabic, hence the ckeditor needs to support "rtl" direction, we also are defaulting to "rtl" while showing both directions in the ckeditor toolbar.

I've tested the MR!1 by:

1. Enabling the "CKEditor 5" experimental core module
2. In /admin/config/content/formats choose a format and configured to use the "CKEditor 5" in the Text Editor dropdown field
3. After clearing cache I was able to see the new button for the paragraph direction, and I added it to the "Active toolbar"
4. UPDATE:
   Only one really minor issue in here: the styling for the button seems to be a bit broken (showing like a dropdown arrow above the icon). It should be a really quick fix to maybe remove it on .ckeditor5-toolbar-button-direction::after on bidi_direction.admin.css line 22
   Please disregard this, I was not using Claro theme to test this :S. On Claro there are no issues with the button in the config page:
   
5. Saved changes in the text format. No errors or issues to report.
6. Finally I've created a new node, and I was able to use the BIDI button to set the paragraphs direction. No errors.
   

The only thing missing to behave exactly as is CK4 is not related with this plugin/module, but with the contentsLangDirection https://docs-old.ckeditor.com/ckeditor_api/symbols/CKEDITOR.config.html#... that we were using to set rtl as the default direction. Just commenting this here in case it helps someone else for the upgrade: https://ckeditor.com/docs/ckeditor5/latest/installation/getting-started/...
UPDATE: to set the default language/direction in CK5 you can use the following hook:

/**
 * Implements hook_editor_js_settings_alter.
 */
function module_name_editor_js_settings_alter(array &$settings) {
  foreach ($settings['editor']['formats'] as $name => $value) {
    $settings['editor']['formats'][$name]['editorSettings']['language'] = [
      'ui' => 'en',
      'content' => 'ar'
    ];
  }
}

That uses the language config for CK5 API and sets a different lang for the UI and the editor content for all editor formats.

The only thing missing to behave exactly as is CK4 is not related with this plugin/module, but with the contentsLangDirection https://docs-old.ckeditor.com/ckeditor_api/symbols/CKEDITOR.config.html#... that we were using to set rtl as the default direction. Just commenting this here in case it helps someone else for the upgrade: https://ckeditor.com/docs/ckeditor5/latest/installation/getting-started/...
UPDATE: to set the default language/direction in CK5 you can use the following hook:

/**
 * Implements hook_editor_js_settings_alter.
 */
function module_name_editor_js_settings_alter(array &$settings) {
  foreach ($settings['editor']['formats'] as $name => $value) {
    $settings['editor']['formats'][$name]['editorSettings']['language'] = [
      'ui' => 'en',
      'content' => 'ar'
    ];
  }
}

That uses the language config for CK5 API and sets a different lang for the UI and the editor content for all editor formats.

Woah! WHAT A FIND! Extracted into a Drupal core issue, thank you! 🙏 See #3312816: CKEditor 5 should explicitly set negotiated content language, not just UI language — credited you!

Actually … isn't this already covered by the Language toolbar item in core, which uses the language.TextPartLanguage CKEditor 5 plugin?

Tested it and … looks like the difference is that that plugin can only produce

<p>
    <span lang="ar" dir="rtl">asdf;sladkfjadsf;lkasdfj</span>
</p>

whereas the BiDi plugin can produce

<p lang="ar" dir="rtl">
    asdf;sladkfjadsf;lkasdfj
</p>

So close, so unfortunate! 😬 We could've hugely simplified all this otherwise! 😞

So are you staying we may not need this module? Should the language button be updated to allow for a default language? and apply to p tags.

Sadly, no, because of the "so close, so unfortunate" bit 😞

Advertised this: https://twitter.com/wimleers/status/1578360118336688129 🤓

• The current merge request removes the CKEditor 4 plugin. That's bad: it means that the existing text editors using CKEditor 4 will immediately be broken upon installing this version of the module! It's very strongly recommended to support both CKEditor versions simultaneously.
• Because of 👆, it actually was impossible to write a upgrade path completeness test too!

In working on the upgrade path test coverage, I ran into two core bugs: #3314478: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified and #3314511: CKEditor 4 → 5 upgrade path may trigger warnings in some edge cases, making upgrade path tests impossible! This is blocked on the first only (it'll make the second disappear).

Until that core bug is fixed, this will fail with:

1) Drupal\Tests\ckeditor_bidi\Kernel\UpgradePathTest::test with data set "both CKEditor 4 buttons" ('ckeditor_bidi_both', array(), array(array(array('bold', 'heading', '|', 'direction')), array(array(array('heading2', 'heading3')), array(false))), '', array(), array(), array())
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'<p dir="ltr rtl"> <h2 dir="ltr rtl"> <h3 dir="ltr rtl">'

But already without that core patch applied, you'll see one test failure:

1) Drupal\Tests\ckeditor_bidi\Kernel\UpgradePathTest::test with data set "LTR CKEditor 4 button only" ('ckeditor_bidi_ltr_only', array(), array(array(array('bold', 'heading', '|', 'direction')), array(array(array('heading2', 'heading3')), array(false))), '', array(), array(), array())
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
         'items' => Array &2 (
             0 => 'bold'
             1 => 'heading'
-            2 => '|'
-            3 => 'direction'
         )
     )
     'plugins' => Array &3 (
@@ @@
                 0 => 'heading2'
                 1 => 'heading3'
             )
-        )
-        'ckeditor_bidi_ckeditor5' => Array &6 (
-            'rtl_default' => false
         )
     )
 )

(I've already left a comment on the MR where I point out the root cause.)

Yay, the upgrade path test now passes! (With #3314478: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified applied to Drupal core.)

Other one is how do we setup translations?

Created #3314680: Incorporate translations support into the starter template for that. Not sure either, I have barely worked on the JS side of CKEditor 5!

I think that for now, you can choose to not handle translations. Or, if you modify the plugin further, you could choose to use Drupal.t()…

Good news: per https://softwareengineering.stackexchange.com/a/105926 (and per @lauriii!), you can in fact relicense MIT under the GPL. You just have to include the original license file.
This also means that you can easily handle translations in this case by adding Drupal.t() calls 👍

#3314511: CKEditor 4 → 5 upgrade path may trigger warnings in some edge cases, making upgrade path tests impossible landed!

Posted another review round! 👍

This is getting very close to the finish line! 🤩

Just addressed all of the feedback on #3314478: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified, which is still blocking this actually 🙈 See #3314478-27: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified.

Actually, two issues are blocking this, thanks to @smustgrave confirming that on the merge request. #3312816: CKEditor 5 should explicitly set negotiated content language, not just UI language is the second.

#3314478: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified just landed, so one fewer blocker! 👍

That leaves only #3312816: CKEditor 5 should explicitly set negotiated content language, not just UI language.

Per #23, AFAICT the only blocker to this working is #3312816: CKEditor 5 should explicitly set negotiated content language, not just UI language. If you could push that forward, I would be eligible to RTBC 😇🙏