[drupalMedia] Support choosing a view mode for <drupal-media>

Let's do this after #3246385: [drupalMedia] Support captions on <drupal-media>.

#3260554: [drupalMedia] Support alignment on <drupal-media> landed. This can happen alongside #3246385: [drupalMedia] Support captions on <drupal-media>.

Currently I have the view modes I have on my site hard coded and the options show up as balloon buttons in the UI.

Progress! 🥳 :D

Note to self: toolbar dropdown currently commented out to avoid breaking the site

Current behavior: When clicking "tiny mode" button, the model and view updates correctly, but the command does not. HOWEVER, when I then click one of the align buttons, the command tab updates and will show both the tiny and the proper align as shown in the screenshot. In either case, the image preview doesn't change to the chosen view mode.

One of my guesses is that it has something to do with the if condition inside of the execute method in DrupalElementStyleCommand.js.

   if (
        !requestedStyle ||
        this._styles[group].get(requestedStyle[modelGroupName]).isDefault
      ) {

When I change to a different align type, it hits the "writer.removeAttribute(modelGroupName, element)" line inside the if condition but it does not hit that line for when I change to a different view mode type.

Things left to do:
1. dynamically generate config in Media.php (include bundles)
2. add a 'none' view mode
3. write tests
4. fix code so custom commands pass

1. Currently, all the view modes appear in the dropdown regardless of bundle type. It's being dynamically generated in Media.php now, but I am not sure how to generate the toolbar items based which selected bundle/content the toolbar is associated with.

(Screenshot at bottom with a dropdown on a media video)

2. I added a 'no view mode' but there is also an existing 'default view mode'. The 'no view mode' adds nothing to the model nor the view nor the command. The 'default view mode' adds the 'default' value to the model, view, and command. Should I keep this 'no view mode' or would the default view mode suffice?

3. I've gotten several "iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax" because I'm using a lot of for loops. I suppressed all those complaints in the code for now. Is it fine to keep the suppressions on these for loops or should I eventually fix them?

Just wanted to say: LOVE that demo screenshot 🤩🤣🤣👏

#15:

1. To know the bundle on the client side: expand \Drupal\ckeditor5\Controller\CKEditor5MediaController::mediaEntityMetadata() to return the bundle from the server side. #3262492: Refactor isMediaUrl to more generic API that supplies frontend metadata about media entities recently refactored that already, and we specifically ensured that adding this would be simple. See #3262492-9: Refactor isMediaUrl to more generic API that supplies frontend metadata about media entities. You can see which other files (in particular JS files) that that issue/MR modified; those are probably all the pointers you need to figure this out 😊🤓
2. Default view mode suffices. But! Whenever the default view mode is active, then the downcast should omit the data-view-mode attribute. We need to do that to match the behavior of the filter. See \Drupal\media\Plugin\Filter\MediaEmbed::settingsForm(), which says:
   '#description' => $this->t('The view mode that an embedded media item should be displayed in by default. This can be overridden using the <code>data-view-mode attribute.'),
   
3. You probably would be able to remove those eslint-disable-* lines by doing some refactoring: create named functions for parts of the logic, then call those. That tends to improve legibility of the code too!

P.S.: I'm getting PHP notices like this, because an array index that does not exist is being accessed:

Notice: Undefined index: default in Drupal\ckeditor5\Plugin\CKEditor5Plugin\Media->getDynamicPluginConfig() (line 55 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/Media.php).

1. Having trouble getting site back to normal after merging new alignment work.

2. I have not tested thoroughly yet but the list dropdowns are bundle specific now and hides the appropriate view modes buttons. But the case of a view mode that is not enabled for any bundle still needs to be handled.

The main change I made today was I followed the pattern used in mediaimagetextalternativeediting.js and added a listener for inserting content in drupalelementstyleui.js Inside the listener, I upcast the 'drupalMediaBundle' so it has the attribute. However when I check the bundleType inside toggleButtonVisibility() it's undefined. The function too fast before the attribute is set idk.

I didn't get to fixing the commands that stopped working after merging new changes but if that can be looked at too, that'd be great.

Failed loading /usr/local/opt/php@8.0/lib/php/20200930/opcache.so:  dlopen(/usr/local/opt/php@8.0/lib/php/20200930/opcache.so, 0x0009): tried: '/usr/local/opt/php@8.0/lib/php/20200930/opcache.so' (no such file), '/usr/local/lib/opcache.so' (no such file), '/usr/lib/opcache.so' (no such file)

Getting this error when trying to run what is there so far for \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testViewMode.

@lauriii Is there a way to create Media to the same node inside of my view mode test instead of inside of the setup() in MediaTest.php? I added created another Media inside of setup() to test different dropdown items for testViewModeDifferentBundles() but it seems to be affecting some of the other tests because this new media exists in every test now.

Also, still some test failures but the view mode tests pass and addressed all feedback.

First review round. More to come — still digging into the details, but what I posted so far will already help make this MR easier to review 😊

🤓 FYI: this explains the currently failing SmartDefaultSettingsTest!

Looked at this in more detail just now with @hooroomoo. This is going to make the scope expand too much. I told them to just expect a bigger superset for now — that's definitely not causing data loss issues anyway. We'll make the upgrade path perfect (i.e. more narrow; no more superset) in #3269657: [drupalMedia] The CKEditor 4 → 5 upgrade path for the media_embed filter should not forcefully allow the `data-view-mode` attribute on `<drupal-media>`.

This will allow us to port:

1. \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testViewMode()
2. \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testDialogAccess()

I can confirm this MR is porting ::testViewMode(). It is not yet porting ::testDialogAccess(), and that actually makes sense — that does not actually test anything related to media embeds' view modes. Will investigate more closely.

But in any case, that means I can remove the Needs tests tag already 😊

Will apply new review feedback tomorrow.

Remaining failing tests I am stumped on:
1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton
Image doesn't insert.

2) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testAlt
I think same problem of image not inserting.

3) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testViewModeDifferentBundles
Passes locally, fails on CI :(

4) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testAlignment
Fails when checking if the default 'Break text' button is selected on insertion. After inserting an image on my site manually, the break text balloon button is selected, but not in the test.

Epic progress here! Passing tests now and all feedback addressed.

I did a quick round of review of the changes you made, and have only a few new remarks.

I still need to finish reviewing the test coverage in detail, will get that done today!

Will address test feedback and the failing tests tomorrow

Thanks for https://git.drupalcode.org/project/drupal/-/merge_requests/1796/diffs?co..., that made the logic for generating metadata in PHP for the JS to consume slightly easier to understand. 👍

But it's still too hard too understand IMHO, and that makes it very likely that we have bugs (and suboptimal code).

So I started digging into it and found:

1. one piece of dead code 🤓
2. one bit of logic that can be simplified
3. an early return that can be added

Once you've addressed those 3 things, I think it'll become far simpler to simplify the code further/break it down into easier to understand functions that each do only one thing :)

All feedback should be addressed now

All feedback should be addressed now

#3270765: Add test coverage for createDropdown in drupalElementStyles landed, which means the path is paved for this MR! 😊

I think this is now ready for a review from another committer. I have been working on this along @hooroomoo, but all of the heavy lifting is done by them. They have been also reviewing my changes along the way, and we've received additional reviews from @Wim Leers as well. So from my perspective: 🚢🚀

We will still have to generate 10.0.x patch for this, but I don't feel that should block the RTBC.

I provided feedback in the MR. Most/all of it should be easy to address.

1. Works splendidly in manual testing!!!!!!!!! 🤩👏
2. I left comments on 25 of the 31 unresolved threads — most are resolved. The six remaining ones are all from @lauriii, and @hooroomoo said they fixed it, but these were less trivial for me to check — even though none of these 6 are for substantial changes. It'd take @laurii only a few minutes in total to check, and would take me much more 🤓
3. I again compared \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testViewMode() (CKE4) with the new \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testViewMode() (CKE5), and it's now definitely test coverage that is as comprehensive as we had for CKEditor 4. 👍🥳 I also couldn't spot any mistakes.
4. I found a few comment/whitespace nits, which I fixed.
5. \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testDialogAccess() was created as the CKE5 equivalent of \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testDialogAccess(), but it's irrelevant. It was a stub that was added months ago, before the CKE5 Media integration had been finalized. Now it has. EditorMediaDialog is not being used by CKEditor 5, hence there's also no need to test it. Actually, this is still necessary — for example, when filter_caption is disabled, the "toggle caption" toolbar button should disappear. This works, but test coverage for it is still missing. But in the interest of finishing this mammoth issue, let's do that in a follow-up. Created #3275120: [drupalMedia] alt_field setting on "Image" media not respected for that.

• 100% certain random fail in the 10 test: LayoutBuilderQuickEditTest
• 100% certain random infra fail in the 9.4 test: lots of JS tests failing because of high load presumably
• 80% certain random fail in the 9.3 test: single fail in MediaTest::testDrupalMediaStyleInDropdown

Marking RTBC again because the tests for 10 just passed 👍🥳

Committed to 10.0.x and 9.4.x. I reproduced the 9.3.x test failures locally, so that needs to be addressed before a backport can happen.

ckeditor5_drupalelementstyle_test.module was missing changes to get it current with the rest of the patch. Lets see if this gets 9.3.x passing.

🥳🚢

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
@@ -1167,11 +1191,238 @@ function (ConstraintViolation $v) {
+
+    // Test that a media with no view modes configured will be
+    // set to the default view mode.
+    $filter_format->setFilterConfig('media_embed', [
+      'status' => TRUE,
+      'settings' => [
+        'default_view_mode' => 'view_mode_1',
+        'allowed_media_types' => [],
+        'allowed_view_modes' => [],
+      ],
+    ])->save();

One question: what happens is media is embedded with a specific view mode, and that view mode is later removed from the site? Do we just fall back to default when rendering? And what happens when you edit the content again?

#50: The data-view-mode attribute would be removed when the element is upcast in CKEditor 5, which would result in the embedded media being rendered in the default view mode.

#50: To add to what #51 wrote: that really ought to be prevented by using config dependencies. The FilterFormat config entity definitely does not handle config dependencies correctly yet 😢 It only handles removing filter plugins correctly (see \Drupal\filter\Entity\FilterFormat::onDependencyRemoval()). Heck, it does not even handle Role dependencies correctly yet! See #2569741: [PP-1] FilterFormat should add the roles as config dependencies.

EDIT: oh yay, fortunately view modes are handled correctly! Yay for us learning from past mistakes! See \Drupal\media\Plugin\Filter\MediaEmbed::calculateDependencies() — I just confirmed that it results in something like this:

uuid: 0e692f51-91c5-41ad-99e0-57d1840ea212
langcode: en
status: true
dependencies:
  config:
    - core.entity_view_mode.media.full
    - core.entity_view_mode.media.media_library
  module:
    - editor
    - media
_core:
  default_config_hash: P8ddpAIKtawJDi5SzOwCzVnnNYqONewSTJ6Xn0dW_aQ
name: 'Basic HTML'
format: basic_html
weight: 0
filters:
…
  media_embed:
    id: media_embed
    provider: media
    status: true
    weight: 100
    settings:
      default_view_mode: default
      allowed_view_modes:
        full: full
        media_library: media_library
      allowed_media_types: {  }

👍

Also, after posting #52 I realized that was not quite what @catch was asking, although it is related.

@lauriii's response is accurate.

what happens is media is embedded with a specific view mode, and that view mode is later removed from the site?

I just tried it. I had embedded Media using the Media Library view mode. Then I uninstalled the Media Library module. To my horror, this resulted in the Basic HTML text format getting removed 😭😱 That seems like a Major bug in the way that FilterFormat handles config dependencies getting removed… but definitely out of scope here.

Basically, we need to expand \Drupal\filter\Entity\FilterFormat::onDependencyRemoval(), like #2348925: Uninstalling a filter plugin removes text formats did.

Wanted to create an issue for this, but apparently this is a pretty fundamental problem. See #2575547: Formalize patterns for delete/uninstall of configuration dependencies, #2773205: Come up with a design for highly destructive operations in confirm forms, #2912466: Warning about deleting configuration is easy to overlook. Worse, the config dependency system's architecture in \Drupal\Component\Plugin\DependentPluginInterface does not seem to allow for this … 🤔 It only supports calculating dependencies, not removing them. Reacting to dependency removals is only supported at the config entity level, not at the "config entity using plugins that have configurable dependencies" level.

Not sure what to do here. But it's most definitely out of scope/pre-existing.

EDIT: to get this problem space moving forward again, I pushed #3167198-32: Deprecate the 'roles' property of text formats and remove it from the UI forward 🤓

Do we just fall back to default when rendering?

This is the relevant piece of logic from \Drupal\media\Plugin\Filter\MediaEmbed::process():

      $view_mode = NULL;
      if ($view_mode_id !== EntityDisplayRepositoryInterface::DEFAULT_DISPLAY_MODE) {
        $view_mode = $this->entityRepository->loadEntityByConfigTarget('entity_view_mode', "media.$view_mode_id");
        if (!$view_mode) {
          $this->loggerFactory->get('media')->error('During rendering of embedded media: the view mode "@view-mode-id" does not exist.', ['@view-mode-id' => $view_mode_id]);
        }
      }

      $build = $media && ($view_mode || $view_mode_id === EntityDisplayRepositoryInterface::DEFAULT_DISPLAY_MODE)
        ? $this->renderMedia($media, $view_mode_id, $langcode)
        : $this->renderMissingMediaIndicator();

And what happens when you edit the content again?

Again confirmed what @lauriii said: https://www.drupal.org/files/issues/2022-04-15/configured%20view%20mode%...

Thanks that makes sense, but yes we definitely shouldn't just delete filter formats like that. There's also all the limbo state with 'disabled' filter formats too.

Another question: media provides a 'thumbnail' formatter for all media items, which just shows the image without actually rendering the entity. Should it be an option to do a similar thing in the editor (i.e. specify thumbnail + image style)? (See related discussion in #2947796: Responsive image format for media and #3247795: Add text filter plugin to support <img loading="lazy"> and remove it from editor_file_reference). Would be a separate option to view mode obviously, but would need to integrate with what's been added here cleanly.

There's also all the limbo state with 'disabled' filter formats too.

Yep. And TIL \Drupal\views\Plugin\DependentWithRemovalPluginInterface exists but … is Views-only 😭

This also made me look to check how CKEditor 4 has been dealing with config dependencies. In short: poorly. See #2950795, and my detailed response at #2950795-29: CKEditor 5 plugin module dependency not added to text format configuration. tl;dr: the plugin/config system is missing some infrastructure to make this possible.

Should it be an option to do a similar thing in the editor (i.e. specify thumbnail + image style)?

I … don't know. This is also out of scope here IMHO. This is just making sure that we retain feature parity with CKEditor 4, and all content created with CKEditor 4 continues to be editable. See #2940029: Add an input filter to display embedded Media entities for all the considerations that went into the "media embed" filter + CKEditor 4 functionality.

I've been reviewing #3247795: Add text filter plugin to support <img loading="lazy"> and remove it from editor_file_reference for months; for months I've been asking for embedded Media support there. So far, the UX that has been RTBC'd is just to only allow one to override the default behavior in CKEditor 4 (or 5) by switching to source editing and adding the lazy=… attribute there. And I think that makes sense: we don't want to throw such an advanced (and rare) decision in the face of content creators all the time. But that is definitely independent of view mode.

I was not aware of #2947796: Responsive image format for media . That reminds me of #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 and #2822389: Allow responsive image style to be selected in Text Editor's image dialog (necessary for structured content) which I've been trying to land ever since CKEditor 4 was added to Drupal 8 almost 10 years ago 😔 #2947796 has no integration at all yet for CKEditor (4 or 5) — it's only for media fields. It would also need to update \Drupal\media\Plugin\Filter\MediaEmbed to allow overriding the new responsive_image_style field property. But the good news is that we already have something like this for alt and title — see \Drupal\media\Plugin\Filter\MediaEmbed::applyPerEmbedMediaOverrides() (introduced in #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`). And for that, the CKEditor 5 integration (just like the CKEditor 4 integration) already has the ability to override the image alt stored in the Media Library. We could do the same for the responsive image style. But that'd definitely result in a confusing UX. That's always the trade-off: which level of granularity do we expose? In MediaEmbed (#2940029: Add an input filter to display embedded Media entities), we chose to expose view modes, not responsive image styles — in no small part because responsive image styles do not work for media other than images. There's nothing stopping you from creating one view mode per responsive image style, and have that view mode only be meaningful (i.e. have config) for images. CKEditor 5 will only show those view modes for the Image media type that actually have a view display configured. Besides, some responsive image styles may not make sense for image media — maybe they're specifically used for user profile pictures for example. So I do not think #2947796: Responsive image format for media is a wise approach.

@catch: Just a sanity check: can you please confirm that all of these (excellent!) questions you're asking are only about tangential concerns, not concerns with this MR specifically? 🙏

Everything in the CKEditor 5 experience matches what you can do in the CKEditor 4 experience functionality-wise. It just has a better UX 🤓

@Wim Leers the stuff about image styles are tangential concerns, but also trying to trace the future path of what this might look like if we allow image style configuration and want parity with that for media. #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 is the issue I was looking for but couldn't find.

The view mode removal question isn't really tangential - even though we might not want to fix it here, we'd want to make sure we haven't made the situation worse than it was with ckeditor4, sounds like this is the case.

I know we're trying to provide parity with ckeditor4 so the upgrade path doesn't destroy existing content/editor configurations, but also what ckeditor4 does is not necessarily what we'd want to do if we were adding ckeditor5 to core without having to deal with ckeditor4 configuration on top, especially as features are getting added to media/image styles over time. As long as we've got paths open to modernise things later, that's the main thing.

The view mode removal question isn't really tangential - even though we might not want to fix it here, we'd want to make sure we haven't made the situation worse than it was with ckeditor4, sounds like this is the case.

As long as we've got paths open to modernise things later, that's the main thing.

100% agreed!

If anything, it has become simpler to add more capabilities (or refine existing ones) in the future, because we can now be confident about the configuration that is stored, and how the text format and editor are configured together.

Thanks for being vigilant and forward-thinking! 😊👍

We really still need to get this committed to 9.3.x.

The #48 patch appears to still work fine for me locally, not sure why it didn't apply the first time. I just requeued the test.

#48 doesn't apply using the patch application mechanism that DrupalCI uses. Fetched patch, git apply -3v'd it, then recreated.

Also updated issue metadata to reflect that this is a CKEditor 5 stable blocker.

Committed 69a6d5b and pushed to 9.3.x. Thanks!