[drupalMedia] The CKEditor 4 → 5 upgrade path for the media_embed filter should not forcefully allow the `data-view-mode` attribute on `<drupal-media>`

(Documenting the thinking I've done around this.)

Clarification: we should match the logic and spirit in #3074608: Optionally allow choosing a view mode for embedded media in EditorMediaDialog. Prior to that, data-view-mode was an attribute required to be allowed. That's essentially what #3245720: [drupalMedia] Support choosing a view mode for <drupal-media> is doing too.

That means that <drupal-media data-view-mode> should be enabled whenever >=2 options are selected in media_embed filter plugin's allowed_view_modes setting.

The existing CKEditor 5 plugin conditions (toolbarItem, imageUploadStatus, filter, requiresConfiguration, plugins) cannot express this.

This is why the suggested solution in the IS says:

2. Add a configuration UI for the CKEditor 5 media plugin: a checkbox that indicates whether content creators are allowed to override the default view mode

… but this would sort of be duplicating the logic in \Drupal\media\Plugin\Filter\MediaEmbed::settingsForm().

The reason this is so tricky? Because the filter settings are de facto also the editor settings in this case:

    $form['allowed_view_modes'] = [
      '#title' => $this->t("View modes selectable in the 'Edit media' dialog"),

(That is literally referring to EditorMediaDialog, which CKEditor 5 won't use!)

We cannot even create a new requiresFilterConfiguration which would be similar to requiresConfiguration (which operates on this CKEditor 5 plugin's configuration) because it cannot be expressed using that.

The best we can do really is to do what is outlined in the current plan in the issue summary, and use the same approach that #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing used: a new piece of configuration.

I wrote this in the proposed resolution roughly a month ago:

2. Add a configuration UI for the CKEditor 5 media plugin: a checkbox that indicates whether content creators are allowed to override the default view mode

… but I failed to realize that this is basically duplicating configuration stored by the media_embed filter's settings:

Then again, that is pretty weird too: the filter settings dictate if the text editor should offer the user the choice. Even though filters are supposed to not be aware of Text Editors at all. This is of course independent of CKEditor 5 work — this is pre-existing and works in CKEditor 4.

Conclusion:

1. Adding a new setting to the Media CKEditor 5 plugin really is the only possible way forward. Even though it is kinda confusing in terms of UX.
2. To ensure the best possible UX, we should add a validation constraint that verifies that the new allow_view_mode_override setting for the Media CKE5 plugin is in fact in sync with the filter settings. This requires adding an additional validation constraint that is able to inspect both the Editor config entity and the FilterFormat config entity. To help you get started:
   

   diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
   index 12b21a0bd4..ae786f3d4c 100644
   --- a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
   +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
   @@ -71,6 +71,17 @@ public function validate($toolbar_item, Constraint $constraint) {
        // Finally: ensure the CKEditor 5 configuration's ability to generate HTML
        // markup precisely matches that of the text format.
        $this->checkHtmlRestrictionsMatch($text_editor, $constraint);
   +
   +    if (isset($text_editor->getSettings()['plugins']['media_media'])) {
   +      $cke5_plugin_overrides_allowed = $text_editor->getSettings()['plugins']['media_media']['allow_view_mode_override'];
   +      $filter_allowed_view_modes = $text_editor->getFilterFormat()->filters('media_embed')->getConfiguration()['settings']['allowed_view_modes'];
   +      // Whenever the CKEditor 5 plugin is configured to allow overrides, the
   +      // filter must be configured to allow 2 or more view modes.
   +      if ($cke5_plugin_overrides_allowed !== count($filter_allowed_view_modes) >= 2) {
   +        $this->context->buildViolation('CKE5 media plugin config out of sync with media_embed filter settings!!!!')
   +          ->addViolation();
   +      }
   +    }
      }
    
      /**
   

   → this should basically work, but it really should be A) moved into a new validation constrain validator, B) that constraint should be added to ckeditor5.pair.schema.yml and C) this needs test coverage in \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::providerPair(). Start with C: add a test that should throw a validation error for the configuration not being in sync. Then adding it will make the MR green 👍

Updated issue summary accordingly.

#8:

1. We don't want that behavior. The current behavior is fine. The Media CKEditor 5 plugin is enabled whenever the media_embed filter is active. When that filter is active, media can be embedded. Which also means the filter's default view mode can be overridden.
   The button you're referring to is the Media Library button. That button is not required to be able to embed media — it merely offers a better UX for embedding media (using the media library).

   (I get that this is a bit odd, but it's the separation of concerns that Drupal core has, and so we must respect it too. The easiest way to observe this: uninstall the Media Library module. The Media Embed filter can still be enabled, and you can still override the view mode. There literally is no Media Library available. Lots of Drupal 8 sites are using Media without Media Library, because the latter was only added after the Media module was added and stabilized!)

   In other words: nothing left to be done here! 😄

2. This basically boiled down to a typo 🤓 Fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/2079/diffs?co...

So all that remains here now is updating a bunch of tests to ensure that their CKEditor 5 configuration no longer violates the new validation constraint! 🥳

Yay, 252d1ffb fixed the ValidatorsTest failures 👍

I also took a look at the SmartDefaultSettingsTest failure and provided pointers for that on the MR just now.

Almost there, a few more failures to fix :)

#11:

1. @lauriii found out today that some configurations of allowed view modes cause CKE to fail to initialize. So it might be related to that. We are planning to meet tomorrow about that.

   I'm pretty sure it's the case of configuring the media_embed filter to either A) less than two view modes (which the filter happily accepts because it unfortunately lacks sufficient validation logic), B) the two view modes do not exist for all media types, but only for some media types. So when editing media of a type for which <2 of the allowed view modes are allowed, the editor crashes.

   Neither of those are bugs on our end; they're both bugs in the media_embed filter's logic. That should not block this issue.

2. Reproduced. 🧐 That is a bug. Good find! #3276627: CKEditor5::shouldHaveVisiblePluginSettingsForm() does not correctly handle configurable CKE5 plugin that has a filter condition.

#3276627: CKEditor5::shouldHaveVisiblePluginSettingsForm() does not correctly handle configurable CKE5 plugin that has a filter condition is now ready for review.

#11.2 means this issue is blocked on #3276627: CKEditor5::shouldHaveVisiblePluginSettingsForm() does not correctly handle configurable CKE5 plugin that has a filter condition.

Also, let's make it more clear that this is a highly important issue: a CKEditor 5 stable blocker.

#3276627: CKEditor5::shouldHaveVisiblePluginSettingsForm() does not correctly handle configurable CKE5 plugin that has a filter condition landed! That means this is unblocked, and this needs a rebase.

#3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) landed earlier today and modified \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getProvidedElements(). It introduced a bug that only is exposed by this plugin so far:

The root cause: HtmlRestrictions::fromString('<tag foo> <tag bar>') will only actually parse the last of the two tags. Which is not entirely unreasonable.

Simple work-around: don't declare multiple identically named tags within one plugin's elements metadata:

diff --git a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
index 9fb0492094..09d5f8ec59 100644
--- a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
@@ -644,8 +644,7 @@ media_media:
     library: ckeditor5/drupal.ckeditor5.media
     class: Drupal\ckeditor5\Plugin\CKEditor5Plugin\Media
     elements:
-      - <drupal-media data-entity-type data-entity-uuid alt>
-      - <drupal-media data-view-mode>
+      - <drupal-media data-entity-type data-entity-uuid alt data-view-mode>
     conditions:
       filter: media_embed
 

Beyond that, we now need to update the test expectations here slightly because <ol type> no longer needs to be supported through SourceEditing — we get native support for it now 👍

Soooooooo close! 😅😅😅😅

One failure, but unrelated. Re-testing.

Two more remarks: one about the labels being wrongly hardcoded, the other about needless code complexity 🤓

The rationale is documented in #7.

Curious about your thoughts :)

Responded on the MR!

I previously RTBC'd this in #23, then again in #26. I just added a trivial fix for @lauriii's remark.

Back to RTBC :)