Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal

Fix markup.

Hah, wow, that is a geniusly simple solution, @lauriii 🤩 😄

This also means that we need to completely redo the issue summary, because the solution is far more elegant, and far simpler than we originally thought/feared! 🥳

Review

🤔 But … isn't there one scenario where that is inadequate: the Full HTML scenario? Because then we should not restrict ourselves to just the block-level HTML elements that are supported, but to all of them? → NOPE, because in that case

    // Only enable the General HTML Support plugin on text formats with no HTML
    // restrictions.
    // @see https://ckeditor.com/docs/ckeditor5/latest/api/html-support.html
    // @see https://github.com/ckeditor/ckeditor5/issues/9856
    if ($editor->getFilterFormat()->getHtmlRestrictions() !== FALSE) {
      unset($definitions['ckeditor5_htmlSupport']);
    }

will skip the if-body, which means this will be present:

ckeditor5_htmlSupport:
  ckeditor5:
    plugins: [htmlSupport.GeneralHtmlSupport]
    config:
      htmlSupport:
        allow:
          -
            name:
              regexp:
                pattern: /.*/
            attributes: true
            classes: true
            styles: true
  drupal:
    label: Arbitrary HTML support
…

And this allows all tags, plus all styles, classes and attributes. Which means that anything that a <$block> wildcard tag could possibly allow will be allowed here too.

✅👏

Proof of criticality and fix: tests

I think it'd be great to prove that without this MR, there'd be data loss, and with there won't be. i.e. prove the criticality, and prove the fix.

I believe doing that would require test coverage similar to #3259493: [GHS] Unable to limit attribute values: ::allowedElementsStringToHtmlSupportConfig() does not generate configuration that CKEditor 5 expects. Simply expanding the test coverage there would be 10x simpler. And the test coverage there is literally a functional JS test for this CKE5 plugin and the way it behaves depending on how Drupal configures it, so exactly what we need here. So let's wait for \Drupal\Tests\ckeditor5\FunctionalJavascript\SourceEditingTest to exist first? 🤓

Per #5.

@lauriii added tests 👍

I posted several remarks on the MR, these are the 3 key ones:

1. https://git.drupalcode.org/project/drupal/-/merge_requests/1873?commit_i...
2. https://git.drupalcode.org/project/drupal/-/merge_requests/1873/diffs#no...
3. https://git.drupalcode.org/project/drupal/-/merge_requests/1873/diffs#no...

The test that was failing before is passing now — the fix @lauriii added was simple: the kernel test was creating an Editor without a matching FilterFormat.

This is the scary part from the issue summary that we absolutely need to fix:

This means that the automatic upgrade path we are providing from CKEditor 4 to CKEditor 5 is not guaranteed to support all attributes on block-level elements through CKEditor 5 plugins it enabled, on text formats with HTML restrictions that list allowed tags/attributes/attribute values (essentially: those using filter_html).

This MR implements the solution proposed in the MR. It for example aims to allow right-aligning an address "block-level" HTML tag whenever:

• The "align right" button is enabled
• The <address> tag is added to the Source Editing plugin's configuration of extra allowed elements

… except that this part is not yet implemented! We need to ensure that whichever wildcard HTML tags are allowed by CKEditor 5 plugins (in this example: the ckeditor5_alignment.right plugin) expands onto the full set of concrete elements.

The proposed solution in the issue summary does say:

Configure GHS with a configuration where wildcards have been resolved based on Drupal's assumption on what the wildcard should be resolved into.

… but "configure GHS" is not the same as "configure the Source Editing functionality": we can still generate the config.htmlSupport.allow = [{name: "address", classes: ["text-align-right"]}] configuration.

In fact, we need to configure GHS even when the Source Editing functionality is disabled, otherwise we still suffer the same data loss risk. So whenever a CKEditor 5 plugin is enabled which uses a wildcard tag, we should also enable the htmlSupport.GeneralHtmlSupport plugin generate the aforementioned configuration.

Which brings me to a separate point: somehow along the way we got sidetracked in this MR: we instead focused on adding support for <$block> to the Source Editing plugin's configuration, even though that's never been mentioned as being in scope. I'm fine with adding this ability (it's valuable!), so I'm fine with keeping this. But we've definitely not yet solved the critical portion of this — and looks like both @lauriii and I missed that so far 🙈

tl;dr: we built the necessary infrastructure, used it for a potential manual work-around for the mismatch, but forgot to do the automatic work-around for the mismatch!

I really like what you did at first glance, digging into it… 🤓👏

I really like what you did, @lauriii, but I believe it can be simplified, and make it significantly easier to understand. Less cognitive load to understand/maintain, by treating the two very distinct concerns ("arbitrary/full HTML" and "wildcard tags mismatch") as two distinct plugins: 489eaab4200aa22e03679df3199df4983e279bf7.

:)

As far as I'm concerned this issue is now RTBC.

I contributed only a tiny part of this MR. @lauriii was the lead developer on this. We only bounced back and forth on:

1. one specific algorithm, which is gone since #11 🤣 (the original algorithm @lauriii had added to the SourceEditing plugin for resolving wildcard tags specified by the end user in the allowed tags for source editing)
2. the organization of how to generate GHS config: in a single plugin definition tackling multiple concerns (which was what @lauriii implemented after #11) or in two plugin definitions each tackling one concern (which is what I proposed in the commits of March 4). I could have expressed that in words in a written review to let @lauriii make the code changes, but that'd have been far less efficient.

It looks like @lauriii liked with the changes I proposed on March 4. He's still the architect of this MR, so he can't RTB. I still can.

Therefore: 🚢

Only thing missing: patches for 10.0.x and 9.3.x 😊

This blocks #3259367: [upstream] Allow configuring *which* HTML tags can be aligned + #3268307: $block wildcard resolves into a superset of the actual $block tags.

FYI, from Drupal Slack:

catch: Looks tidy to me.
alexpott: I think I’d still try to completely remove ckeditor5_wildcardHtmlSupport from ckeditor5.ckeditor5.yml
alexpott: But that could happen in a follow-up.

So I think they're +1 :)

I personally don't think we should remove ckeditor5_wildcardHtmlSupport. It adds a clarity we'd otherwise lack:

• it enables us to have two sibling/related plugins for different purposes: one for arbitrary HTML support, one for wildcard HTML support
• this also makes it very clear that if we already have arbitrary HTML support enabled, that we don't need wildcard HTML support too
• this in turn allows getEnabledDefinitions() to have very explicit logic to decide which one of these two plugins, if any, we need
• and that in turn enables ::getCKEditor5PluginConfig() to have very explicit logic to only compute the configuration for wildcard HTML support if it is actually needed — because at that point it is too late to change which plugins will be enabled (at that point only configuration for plugins can be set, not which plugins are enabled)

But if @alexpott feels we should still remove the "wildcard" HTML support plugin, happy to do that in a follow-up.

Meanwhile, we have their blessing, so let's keep CKEditor 5 criticals moving forward 🤞