Generate CKEditor 5 configuration based on pre-existing text format configuration

💯 Thanks for opening this issue! It'll be another essential next step on top of #3201637-2: Figure out how to prevent data loss during upgrade/migration path.

Postponed on #3201637-21: Figure out how to prevent data loss during upgrade/migration path.

This is postponed on #3201637: Figure out how to prevent data loss during upgrade/migration path because only then will be able to write thorough tests.

But between #3201641: Improve the HTML filter configuration UX having landed and that issue being very close to shipping, I figured it's time to get us started here 😊

See the before vs after experience 🤩

1. +++ b/src/Plugin/Editor/CKEditor5.php
   @@ -103,6 +103,24 @@ class CKEditor5 extends EditorBase implements ContainerFactoryPluginInterface {
   +      if ($old_editor->getEditor() === 'ckeditor') {
   +        $upgraded_settings = $this->createSettingsFromCKEditor4($old_editor->getSettings());
   +        $editor->setSettings($upgraded_settings);
   +        $editor->setImageUploadSettings($old_editor->getImageUploadSettings());
   +      }
   

   This part is 100% done for Drupal core CKEditor 4 plugins.

2. The part above we'll also need in the drush command that #3201637: Figure out how to prevent data loss during upgrade/migration path is adding.

3. +++ b/src/Plugin/Editor/CKEditor5.php
   @@ -103,6 +103,24 @@ class CKEditor5 extends EditorBase implements ContainerFactoryPluginInterface {
   +        // @todo compute based on HTML restrictions.
   

   This part is still TODO.

4. +++ b/src/Plugin/Editor/CKEditor5.php
   @@ -261,4 +279,151 @@ class CKEditor5 extends EditorBase implements ContainerFactoryPluginInterface {
   +      case 'RemoveFormat':
   +      case 'Cut':
   +      case 'Copy':
   +      case 'Paste':
   +      case 'PasteText':
   +      case 'PasteFromWord':
   +      case 'SpecialChar':
   +      case 'ShowBlocks':
   +      case 'Source':
   +      case 'Maximize':
   +      case '-':
   +        return NULL;
   +
   +      // @see \Drupal\ckeditor\Plugin\CKEditorPlugin\StylesCombo
   +      case 'Styles':
   +        return NULL;
   

   This made it very easy to see which buttons we do not yet have in CKEditor 5. Some of these probably should have an equivalent in CKEditor 5?

#3201637: Figure out how to prevent data loss during upgrade/migration path landed.

Per #3215466-9: Attribute values not accounted for in CKEditor5PluginManager::getProvidedElements, this issue is blocked on that.

#3215466: Attribute values not accounted for in CKEditor5PluginManager::getProvidedElements landed :)

Assigning to myself. I made some nice progress, but this isn't in a state I'd want anyone to have to resume work on.

@bnjmnm great work here!

Pull in the latest commits against 1.0.x, which solve:

• #3222842: <a hreflang> + <blockquote cite>
• #3222838: Configure basicStyles.Italic to output <em> instead of <i>

Thanks to pulling those in, switching Basic HTML from CKEditor 4 to 5 no longer shows:

but instead shows:

So … progress! 🥳🥳🥳

Based on #14 posted #3211049-5: [META] Add all plugins which are available in Drupal core's build of CKEditor 4 + #3222801-3: [META] Ensure CKE5 equivalent plugins of CKE4 generate/support equivalent markup 🥳🤓

Okay so ab80a78df resulted in the same number of test failures, but only 3 instead of 5 test case failures in ValidatorsTest. The INVALID: the modified restricted_html text format (with filter_autop and filter_url removed) and INVALID: HTML format: empty toolbar + default allowed HTML tags test cases are now passing again. I'm not sure I properly understood why you originally did this, @bnjmnm. Perhaps you can walk me through it? 🙏😊

Okay so ab80a78df resulted in the same number of test failures, but only 3 instead of 5 test case failures in ValidatorsTest. The INVALID: the modified restricted_html text format (with filter_autop and filter_url removed) and INVALID: HTML format: empty toolbar + default allowed HTML tags test cases are now passing again. I'm not sure I properly understood why you originally did this, @bnjmnm. Perhaps you can walk me through it?

These can stay as-is, this was largely due to my habit of quickly changing the tags to allow ONLY <p><br> during repeat manual testing. Adding those tags allows me to continue and enjoy the full functionality of what we're adding here.

The failing tests are all tests expected to fail because the editors being switched from include tags/attributes not currently supported by CKEditor 5. There are existing issues for all of these.

There's undoubtedly additional work to do, but it's also at a good point to pause so we can attend to some of the issues needed for tests here to pass, and a bit of time away from this complex issue will provide some perspective that will make completion easier.

The failing tests are all tests expected to fail because the editors being switched from include tags/attributes not currently supported by CKEditor 5. There are existing issues for all of these.

Can confirm.

From

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "basic_html can be switched to CKEditor 5 without problems" ('basic_html', array('filter_autop', 'filter_url'), array(array(array('bold', 'italic', '|', 'link', '|', 'bulletedList', 'numberedList', '|', 'blockQuote', 'uploadImage', '|', 'heading', '|', 'sourceEditing', '|', 'code', 'textPartLanguage')), array(array('placeholder'), array('placeholder'))), array())
The following tags/attributes are not allowed in the updated text format:Array
(
    [cite] => 
    [ul] => Array
        (
            [type] => 1
        )

    [ol] => Array
        (
            [start] => 1
            [type] => 1
        )

    [dl] => 
    [dt] => 
    [dd] => 
    [h2] => Array
        (
            [id] => 1
        )

    [h3] => Array
        (
            [id] => 1
        )

    [h4] => Array
        (
            [id] => 1
        )

    [h5] => Array
        (
            [id] => 1
        )

    [h6] => Array
        (
            [id] => 1
        )

    [span] => 
    [img] => Array
        (
            [height] => 1
            [width] => 1
        )

)

to

(
    [ul] => Array
        (
            [type] => 1
        )

    [ol] => Array
        (
            [start] => 1
            [type] => 1
        )

    [dl] => 
    [dt] => 
    [dd] => 
    [h2] => Array
        (
            [id] => 1
        )

    [h3] => Array
        (
            [id] => 1
        )

    [h4] => Array
        (
            [id] => 1
        )

    [h5] => Array
        (
            [id] => 1
        )

    [h6] => Array
        (
            [id] => 1
        )

    [span] => 
)

thanks to #3222847: <img width height> and #3222851: <cite>.

So merged in those two commits from 1.0.x and pushed them in 98d127d94d488fb70046e41df55d7781f81151f7.

The remaining tags will be addressed by:

• #3222840: <ol start>
• #3220534: [PP-1] [needs upstream feature] Type attribute not configurable for <ul> and <ol>
• #3222852: <dl> <dt> <dd> by introducing "Manually editable HTML tags" configuration on Source Editing
• #3224256: <h* id> (or more generically: <$block id>)

So let's exempt those with explicit deprecation errors and @todos. Did that in b8280230a1d000c9ce05fd2e6087af6280718ea6.

That brought down the failures to just

Testing Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest
F..                                                                 3 / 3 (100%)

Time: 00:02.047, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "basic_html can be switched to CKEditor 5 without problems" ('basic_html', array('filter_autop', 'filter_url'), array(array(array('bold', 'italic', '|', 'link', '|', 'bulletedList', 'numberedList', '|', 'blockQuote', 'uploadImage', '|', 'heading', '|', 'sourceEditing', '|', 'code', 'textPartLanguage')), array(array('placeholder'), array('placeholder'))), array())
The following tags/attributes are not allowed in the updated text format:Array
(
    [span] => 
)

Failed asserting that an array is empty.

The sole exception is <span>. What to do about that one … 🤔 Currently the automatically generated settings (and hence also the upgrade path) will allow the textPartLanguage plugin, which in turn will enable <span lang dir> … which is actually a superset. So this will not result in data loss. But it will result in an additional button being enabled for sites that do have <span> in their allowed HTML but not the "Language of parts" button in their toolbar. Then again, that is the only way to add <span> right now. Plus, the goal is to not have data loss. The goal is to have a sensible default upgrade path. That extra button is not harmful at all, and if anything, it improves accessibility — by encouraging content creators to think about other languages.

So, while not perfect, I'm going to go ahead and explicitly allow a superset to be generated.

So to do #20, I'm adding the ability to specify the expected superset. i.e. we're going from just <span> in the original to <span lang dir> in the generated/upgraded text format+text editor.

Pushed that in 50a02a502ca5a6547e6d3885ea821f013a0875a6 🤞

Ugh, SmartDefaultSettingsTest is not passing due to the deprecation notices! Removing them. The @todos are in there anyway.

Finished reviewing. Increased test coverage to ensure there are explicit (and helpful) failures.

I think this is ready to be pushed across the finish line by @bnjmnm!

The final test failures are due to the allowed HTML field not updating with the added toolbar items. The field updates correctly if anything additional is done to trigger an AJAX rebuild (change a plugin setting, move a toolbar item, etc).

Perhaps the changes to the toolbar plugins are happening after the form_alter that gathers the provided tags based on the current plugin conifg? I tried adding a second update of the allowed_html filter settings during the AJAX rebuild in _update_ckeditor5_html_filter(). This updates the field value to the tags supported by the current toolbar config, but this happens after validation so that's not a good option.

It's not immediately clear how/where to implement the rebuild so this doesn't happen but it seems like the sort of thing that is easier to figure out after stepping away for a moment.

#3226011: Introduce CKEditor5PluginConfigurableInterface::getDefaultSettings() would solve https://git.drupalcode.org/project/ckeditor5/-/merge_requests/71/diffs#n.... I think that's a simple issue to do first 🤓

Merged in 1.0.x because #3226011: Introduce CKEditor5PluginConfigurableInterface::getDefaultSettings() was merged.

Let me start off by saying this is fantastic work! 🤩👏

I just posted a thorough/comprehensive (well, I reviewed 90%, will do the remaining 10% next) review of the merge request. But there's one thing that I cannot easily capture in there. Hence this comment!

When switching the Standard install profile's Restricted HTML text format (and first allowing <p> <br> and disabling the filter_url and filter_autop filters) to use CKEditor 5, I see this:

Note that it is not talking about the Heading, Bold and Italic buttons/features! That's because those are pre-configured by \Drupal\ckeditor5\Plugin\Editor\CKEditor5::getDefaultSettings() — the message only informs the user about the additional buttons/features.

Imagine we would change CKEditor5::getDefaultSettings() to return return ['toolbar' => ['items' => []]];. Then this would happen:

Is that not much better/more informative? 😊Better still: we should keep CKEditor5::getDefaultSettings() unchanged because I do think it's a sensible default. I just think that when the starting point is a pre-existing text format (FilterFormat::isNew() === FALSE) that has no text editor associated at all (Editor::load($format_id) === NULL) we should use ['toolbar' => ['items' => []]] as the starting point. That means that for completely new text formats, we still do use CKEditor5::getDefaultSettings() as it exists today.

Follow-up created for that review round I just posted: #3226335: Follow-up for #3216015: allow contrib & custom Drupal modules providing CKEditor 4 plugins to specify their CKEditor 5 equivalents + settings to be migrated.

#3226673: API addition: \Drupal\editor\Plugin\EditorPluginInterface::getDefaultSettings() should accept old Editor + Format to generate smart defaults to make the whole SmartDefaultSettings thing not an afterthought that we need to bolt on, but something built in to the Text Editor module. It's an obvious addition, and would allow us to remove this unnecessarily confusing & distracting code:

    // When enabling CKEditor 5, generate sensible settings from the
    // pre-existing text editor/format rather than the hardcoded defaults
    // whenever possible.
    // @todo Remove after https://www.drupal.org/project/drupal/issues/3226673.
    $format = $form_state->getFormObject()->getEntity();
    assert($format instanceof FilterFormatInterface);
    if ($editor->isNew() && !$format->isNew()) {
      assert($editor->getSettings() === $this->getDefaultSettings());
      $editor = $this->smartDefaultSettings->computeSmartDefaultSettings($editor, $format);
    }

Added @todos for this in 997edc912c06ff47703b183e142f584f64224581.

There are two negative consequences of using the messenger service directly in \Drupal\ckeditor5\SmartDefaultSettings::computeSmartDefaultSettings():

1. drush ckeditor5:audit output looks like this:
   
2. When switching restricted_html to CKEditor 5, you do not only see the fundamental compatibility constraint violation message, but also the messages from the computing of smart defaults:
   
   (only the circled message should've been visible)

Just like with the original config validation logic first being unable to convey multiple violation messages, the new smart default settings is unable to return multiple messages — it just "sends them globally".

Created #3226694: Follow-up for #3216015: refactor SmartDefaultSettings to return messages rather than sending them for that.

Meanwhile, we need a work-around at least for point 2, because that is visibly interfering. Point 1 is merely a nuisance at this time.

Also discovered #3226486: Investigate CKE5's GHS plugin rewriting certain user input and #3226478: CKEditor5::getDefaultSettings() has a stale comment and returns invalid settings thanks to this issue!

I walked @bnjmnm yesterday night through the changes I'd made yesterday, and described what the remaining problem was with the restricted_html test case in SmartDefaultSettingsTest. That's what all of today's changes were focused on … until I ran into #34. So added a work-around for that and a follow-up issue to solve it better.

@bnjmnm and I both believe this is solid, and while it may not be perfect, it certainly represents a vast leap forward — captured in 121 commits and 118 merge request threads over the course of multiple weeks. Merged this so we can keep moving forward! 🚢

Retroactively fixing priority 🤓