Optionally allow choosing a view mode for embedded media in EditorMediaDialog

Some sites will not want this, so it should be optional. By default, the author should not be able to choose a view mode other than the default one, because view modes are a very abstract concept that on many sites authors do not know nor need to know.

1. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -104,3 +104,14 @@ media.source.field_aware:
   +    dialog_view_modes:
   
   +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
   +    $form['dialog_view_modes'] = [
   

   🤔 The dialog in the dialog_view_modes seems weird. How about allowed_view_modes?

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -160,6 +172,33 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      elseif (in_array($filter_default_view_mode, array_keys($options))) {
   

   🤓 Nit: should use strict comparison.

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -238,4 +283,37 @@ protected function getMediaImageSourceField(MediaInterface $media) {
   +    // Only return view modes that are enabled and selected in the editor
   +    // settings.
   

   👍

4. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
   +      '#description' => $this->t("If more than one view mode is selected, a field will appear in the contextual dialog allowing users to change an embed's view mode."),
   

   🤔 "contextual dialog"

I am not convinced by #12. Obviously it makes the configuration UI a lot more complicated. But more importantly, it does not match the concept/the architecture of entity view modes: view modes are created for an entity type, not for a bundle. All bundles for the same entity type by definition have the same view modes. So it's up to the site builder to ensure that each view mode has a sensible configuration for every bundle.

I think I'm leaning towards Wim's point of view. This seems like quite a complex configuration option that I'm not sure will benefit the 80% use case.

My preference would be to simply allow the selected view modes for all media types, and leave type-specific fine tuning to contrib.

#15:


But whether those view modes are enabled with entity display configurations is unique per bundle.

No, it is expected if an entity type has a view mode X that every entity can be viewed in view mode X — regardless of bundle.

What you describe is something you can do on your project, but it violates the architecture that core has established. Whether that architecture is flawed or not is a different matter. But we should just comply with the existing architecture, not change or challenge it here.

1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -252,4 +296,37 @@ protected function getMediaImageSourceFieldName(MediaInterface $media) {
   +    // Get all media view modes.
   ...
   +    // Get the view modes configured in the editor form.
   ...
   +    // Only return view modes that are enabled and selected in the editor
   +    // settings.
   

   These comments are documenting trivialities. Let's remove them.

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -252,4 +296,37 @@ protected function getMediaImageSourceFieldName(MediaInterface $media) {
   +    $view_mode_options = array_filter(array_map(function ($view_mode) use ($media_view_modes) {
   +      if (!empty($media_view_modes[$view_mode]) && $media_view_modes[$view_mode]["status"] === TRUE) {
   +        return $media_view_modes[$view_mode]['label'];
   +      }
   +      return NULL;
   +    }, $allowed_view_modes));
   

   I'd have expected to be able to use array_intersect_key() here.

3. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
   +      '#description' => $this->t("If more than one view mode is selected, a field will appear in the dialog allowing users to change an embed's view mode."),
   

   s/change/choose/
   s/an embed/a media embed/

RE: checking a view mode's status: this is an excellent question! I actually think you should not have to do that, because there are dozens of calls to \Drupal\Core\Entity\EntityDisplayRepository::getViewModeOptions() in core. If the status needs to be checked, it's up to that method to do it. So I think the code can be simplified to array_intersect_key($repository->getViewModeOptions('media'), $editor_settings['plugins']['drupalmedia']['allowed_view_modes']);

+++ b/core/modules/media/config/schema/media.schema.yml
@@ -104,3 +104,14 @@ media.source.field_aware:
+    allowed_view_modes:

All values listed here also need to trigger dependencies on the listed ViewMode config entities.

We can consider adding a onDependencyRemoval() implementation to ensure that the list of allowed view modes is updated when view modes are deleted.

I didn't read the tests yet...

1. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -104,3 +104,14 @@ media.source.field_aware:
   +ckeditor.plugin.drupalmedia:
   +  type: mapping
   +  label: 'Media Embed'
   +  mapping:
   +    allowed_view_modes:
   +      type: sequence
   +      label: 'View modes selectable in the "Edit media" dialog'
   +      sequence:
   +        type: string
   

   I think the sequence itself also needs a label, like so:

   sequence:
     type: string
     label: 'View mode ID'
   

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +170,33 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      $filter_media_embed = $filters->get('media_embed');
   +      $filter_default_view_mode = $filter_media_embed->settings['default_view_mode'];
   

   We don't need $filter_media_embed to be its own variable.

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +170,33 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      if (!empty($media_embed_element['data-view-mode']) && in_array($media_embed_element['data-view-mode'], array_keys($view_mode_options))) {
   

   The in_array() call should be array_key_exists().

4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +170,33 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      elseif (in_array($filter_default_view_mode, array_keys($view_mode_options), TRUE)) {
   

   Same here.

5. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +170,33 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      $form['default_view_mode'] = [
   +        '#type' => 'hidden',
   +        '#value' => $filter_default_view_mode,
   +      ];
   

   I think this needs a comment. What's it here for?

   Ah, I see it's used in submitForm(). If that's the case, I don't think this should be a hidden field. We can just store it in $form_state directly with $form_state->set(). Or, we can set '#type' => 'value' on this element, which will put it into $form_state's values, but not output any markup.

6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -254,4 +298,23 @@ protected function getMediaImageSourceFieldName(MediaInterface $media) {
   +   * @return array
   +   *   An array of view mode options keyed by machine name.
   

   This should be string[].

7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -254,4 +298,23 @@ protected function getMediaImageSourceFieldName(MediaInterface $media) {
   +    if (empty($editor_settings['plugins']['drupalmedia']['allowed_view_modes']) || count($editor_settings['plugins']['drupalmedia']['allowed_view_modes']) < 2) {
   

   The empty() check is redundant. If there are 0 allowed view modes, there is still less than 2 and an empty array will be returned.

   But, more to the point, I don't think we should have this "return nothing if there are less than 2 view modes allowed" opinion in this method. This should be the condition for the #access flag of the select list itself, which will give contrib some wiggle room to extend or override. In other words, in buildForm(), we should set '#access' => count($view_mode_options) >= 2. I think that would allow us to pretty much remove this method entirely.

8. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
   +  public function settingsForm(array $form, FormStateInterface $form_state, Editor $editor) {
   

   It's too bad that $editor is not type hinted as EditorInterface, but that's inherited from upstream.

9. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
   +    if (isset($settings['plugins']['drupalmedia'])) {
   +      $config = $settings['plugins']['drupalmedia'];
   +    }
   

   And what if $settings['plugin']['drupalmedia'] is not set? Should we return early?

10. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
    @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
    +      return $view_mode["status"] === TRUE ? $view_mode['label'] : NULL;
    

    Ubernit: $view_mode["status"] should be $view_mode['status'] (single quotes).

11. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
    @@ -129,4 +143,30 @@ public function getCssFiles(Editor $editor) {
    +    $form['allowed_view_modes'] = [
    +      '#title' => t("View modes selectable in the 'Edit media' dialog"),
    +      '#type' => 'checkboxes',
    +      '#options' => $options,
    +      '#default_value' => !empty($config['allowed_view_modes']) ? $config['allowed_view_modes'] : [],
    +      '#description' => $this->t("If more than one view mode is selected, a field will appear in the dialog allowing users to choose a media embed's view mode."),
    +    ];
    

    Surely we want some default configuration value (for example, ensuring that the 'embed' view mode is pre-selected)?

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -110,8 +111,24 @@ public function calculateDependencies() {
+    // @todo This should be abstracted so that there's an API for text editor's plugins's dependencies.
+    if (is_a($editor_plugin, CKEditor::class, TRUE)) {
+      if (isset($this->getSettings()['plugins']['drupalmedia'])) {

😨 This should not be necessary. #2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait and #2904550: PluginDependencyTrait::calculatePluginDependencies() does not handle theme provided plugins added the ability to let specific plugin instances declare their own dependencies. But it seems Layout is the only thing that uses this.

Pinging Plugin system maintainer @tim.plunkett for his input.

I think we're going to be moving the setting to the MediaEmbed filter settings, per our conversation in slack. In that case, we don't need to fix the editor's dependency issues.

Please summarize that conversation on this issue. Not only can most people not access that conversation, the few people who can will no longer be able to access it in a few short days due to Slack's policies.

The dialog is not editor specific and by putting the config into the CKEditor plugin, if we reference those settings in the dialog, we have created an unwanted dependency on CKEditor.

Makes sense! 👍

Didn't have time to read the tests, but this seems very clean and good to me. Nice work!

1. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -2,6 +2,7 @@
   +use Drupal\ckeditor\Plugin\Editor\CKEditor;
    use Drupal\Core\Config\Entity\ConfigEntityBase;
    use Drupal\editor\EditorInterface;
    
   @@ -110,7 +111,8 @@ public function calculateDependencies() {
   
   @@ -110,7 +111,8 @@ public function calculateDependencies() {
        $this->addDependency('config', $this->getFilterFormat()->getConfigDependencyName());
        // @todo use EntityWithPluginCollectionInterface so configuration between
        //   config entity and dependency on provider is managed automatically.
   -    $definition = $this->editorPluginManager()->createInstance($this->editor)->getPluginDefinition();
   +    $editor_plugin = $this->editorPluginManager()->createInstance($this->editor);
   +    $definition = $editor_plugin->getPluginDefinition();
   

   It looks like all of these changes can be reverted.

2. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -104,3 +104,17 @@ media.source.field_aware:
   +    allowed_view_modes:
   +      type: sequence
   +      label: 'View modes selectable in the "Edit media" dialog'
   

   I was hemming and hawing on this label, but in truth, the "Edit media" dialog is the only thing which, currently, cares about the allowed view modes. If that changes, we can easily change this label as well. So, 👍

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,42 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      if (!empty($media_embed_element['data-view-mode']) && array_key_exists($media_embed_element['data-view-mode'], $view_mode_options)) {
   +        $default_view_mode = $media_embed_element['data-view-mode'];
   +      }
   +      elseif (array_key_exists($filter_default_view_mode, $view_mode_options)) {
   +        $default_view_mode = $filter_default_view_mode;
   +      }
   

   This code still feels a little convoluted to me; I think it might benefit from a comment explaining the thought process behind it. Not so much what we're doing here, as why we're doing it.

4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -213,6 +261,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    if ($form_state->getValue(['attributes', 'data-view-mode']) === $form_state->getValue('default_view_mode')) {
   

   🤔 This seems like it might accidentally run afoul of PHP's type juggling. (For example, what if attributes[data-view-mode] is '', but default_view_mode is NULL?)

5. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -129,4 +143,24 @@ public function getCssFiles(Editor $editor) {
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * @see \Drupal\media\Form\EditorMediaDialog
   +   */
   +  public function settingsForm(array $form, FormStateInterface $form_state, Editor $editor) {
   +
   +    return $form;
   +  }
   +
   +  /**
   +   * Form element validation handler.
   +   */
   +  public static function elementValidateAllowedViewModes(&$element, FormStateInterface $form_state) {
   +    // Filters the #value property so only selected values appear in the
   +    // config.
   +    $element['#value'] = array_filter($element['#value']);
   +    $form_state->setValueForElement($element, $element['#value']);
   +  }
   +
   

   We can probably revert all this, plus the rest of the changes this file. AFAICT, it's no longer being used.

6. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -134,16 +137,43 @@ public static function create(ContainerInterface $container, array $configuratio
      public function settingsForm(array $form, FormStateInterface $form_state) {
   +
   +    $options = $this->entityDisplayRepository->getViewModeOptions('media');
   

   There is an errant blank line here. :)

7. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -134,16 +137,43 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (empty($this->settings['allowed_view_modes'])) {
   +      $this->settings['allowed_view_modes'] = [];
   +    }
   

   Nit: This is equivalent to $this->settings += ['allowed_view_modes' => []];

8. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -134,16 +137,43 @@ public static function create(ContainerInterface $container, array $configuratio
   +      '#description' => $this->t("If more than one view mode is selected, a field will appear in the dialog allowing users to choose a media embed's view mode."),
   

   A field will appear in "the dialog" -- that's a bit vague :) I would suggest changing this to: "If more than one view mode is selected, users will be able to choose a view mode for each embedded media item."

9. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -134,16 +137,43 @@ public static function create(ContainerInterface $container, array $configuratio
   +  public static function elementValidateAllowedViewModes(&$element, FormStateInterface $form_state) {
   

   &$element should be type hinted as an array.

10. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +502,28 @@ public static function trustedCallbacks() {
    +    $types = $this->entityDisplayRepository->getViewModes('media');
    

    $types is a misleading name for this variable. How about something like $available_view_modes instead?

11. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +502,28 @@ public static function trustedCallbacks() {
    +        $dependencies['config'][] = 'core.entity_view_mode.media.' . $view_mode;
    

    Ideally we'd load the view mode entity and use getConfigDependencyName() here.

12. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +502,28 @@ public static function trustedCallbacks() {
    +    if (!empty($dependencies['enforced'])) {
    +      unset($dependencies['enforced']);
    +    }
    

    We don't need the if (!empty()) check. If the 'enforced' key doesn't exist, unset() will just be a no-op.

This will need a reroll now that #3066802: Ensure that embedded image media items do not display at unreasonable sizes by default is in.

Still didn't read the tests, but I like this a lot!

1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -35,14 +36,24 @@ class EditorMediaDialog extends FormBase {
   +  public function __construct(EntityRepositoryInterface $entity_repository, EntityDisplayRepositoryInterface $entity_display_repository) {
        $this->entityRepository = $entity_repository;
   +    $this->entityDisplayRepository = $entity_display_repository;
   

   This class is explicitly marked internal, and this code has not made it into a tagged release yet, so it's okay for us to add this constructor argument without a deprecation. 👍

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -102,6 +114,7 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    $media_embed = $filters->get('media_embed');
   

   I'd rename this to $filter_media_embed or $media_embed_filter, so it's clearer what it is.

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    $allowed_view_modes = !empty($media_embed->settings['allowed_view_modes']) ? $media_embed->settings['allowed_view_modes'] : [];
   

   I'm not sure we need this ternary logic; I'd imagine that the setting will always be there, and that it will always at least be an empty array, seeing as how it's part of the filter's plugin definition.

4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +
   +
   +    $filter_default_view_mode = $media_embed->settings['default_view_mode'];
   

   Nit: There's an empty extra line here.

5. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    // view mode matches the default, we can drop the attribute.
   

   Can we say which attribute we'll be dropping? :)

6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    if (count($view_mode_options) >= 2) {
   

   Ideally we'd save the result of count($view_mode_options) >= 2 into a variable, and use it for both the if statement and the #access property of the select list.

7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      elseif (array_key_exists($filter_default_view_mode, $view_mode_options)) {
   +        $default_view_mode = $filter_default_view_mode;
   +      }
   +
   +    }
   

   Nit: There's an empty blank line here too. Also, what happens if the $default_view_mode stays FALSE? How will that affect the usability of the select list? Do we have test coverage for that case?

8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    $form['view_mode'] = [
   +      '#title' => t("Display"),
   +      '#type' => 'select',
   +      '#options' => $view_mode_options,
   +      '#default_value' => $default_view_mode,
   +      '#parents' => ['attributes', 'data-view-mode'],
   +      '#access' => count($view_mode_options) >= 2,
   +    ];
   

   "Display" seems like a strange term here, but I'm not sure what might be better. "Display as", perhaps? Also, a #description on this field might be useful. Additionally, should we make it #required if #access is TRUE?

9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -213,6 +265,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    if (!empty($form_state->getValue('filter_default_view_mode')) && $form_state->getValue(['attributes', 'data-view-mode']) === $form_state->getValue('filter_default_view_mode')) {
   +      $form_state->setValue(['attributes', 'data-view-mode'], FALSE);
   +    }
   

   I hope we have test coverage to prove that the attribute is actually removed. :)

10. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $this->settings += ['allowed_view_modes' => []];
    

    Since we're adding the setting to the plugin definition, I'm not sure we even need this line anymore :)

11. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $options = $this->entityDisplayRepository->getViewModeOptions('media');
    

    Can we call this $view_mode_options, for clarity?

12. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#title' => t("View modes selectable in the 'Edit media' dialog"),
    

    Should be $this->t().

13. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#default_value' => $this->settings['allowed_view_modes'],
    

    I think this is supposed to be $this->settings['default_view_mode']. The fact that the tests didn't catch this means we are missing a bit of coverage :)

14. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t("If more than one view mode is selected, users will be able to choose a view mode for each embedded media item."),
    

    This is not accurate; at least two view modes must be selected. Also, we should probably say what happens if no view modes are selected.

15. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +  /**
    +   * Form element validation handler.
    +   */
    +  public static function elementValidateAllowedViewModes(array &$element, FormStateInterface $form_state) {
    

    I think this needs @param doc comments for $element and $form_state.

16. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -134,16 +137,40 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $element['#value'] = array_filter($element['#value']);
    +    $form_state->setValueForElement($element, $element['#value']);
    

    Nit: This can be one line.

17. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +499,25 @@ public static function trustedCallbacks() {
    +    $view_modes = !empty($this->settings['allowed_view_modes']) ? $this->settings['allowed_view_modes'] : [];
    

    As I previously mentioned, I'm not sure we need this ternary logic.

18. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +499,25 @@ public static function trustedCallbacks() {
    +    if (!empty($this->settings['default_view_mode'])) {
    +      $view_modes[] = $this->settings['default_view_mode'];
    +    }
    

    I don't think this setting should ever be empty. Why don't we change the configuration form so that it's required?

19. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -472,4 +499,25 @@ public static function trustedCallbacks() {
    +    $storage = $this->entityTypeManager->getStorage('entity_view_mode');
    +    foreach ($view_modes as $view_mode) {
    

    We can actually do this a bit more slickly:

    $view_modes = $this->entityTypeManager->getStorage('entity_view_mode')->loadByProperties([
      'targetEntityType' => 'media',
      'mode' => $view_modes,
    ]);
    

    If we do this, we can remove the if statement in the foreach loop.

😍!

#45:
> There’s some weirdness with the naming here, especially as we now know when you select “Default” it’s the equivalent of ‘data-view-mode=“default”’ which isn’t really a view mode, but a view display.

Ugh, allowing default here seems really odd to me in the first place. (But i guess that went in earlier and is under BC protection?)

1. #45.8 I see you've matured into noticing imperfect referring to Drupal concepts. 🤓😃👏
2. #47: 😃
3. To #47 and #48's discussion about the sense or nonsense of Default: we should just follow what Drupal core does elsewhere. And that is encoded in \Drupal\Core\Entity\EntityDisplayRepository::getDisplayModeOptions():
   

     protected function getDisplayModeOptions($display_type, $entity_type_id) {
       $options = ['default' => t('Default')];
       foreach ($this->getDisplayModesByEntityType($display_type, $entity_type_id) as $mode => $settings) {
         $options[$mode] = $settings['label'];
       }
       return $options;
     }
   

4. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -104,3 +104,17 @@ media.source.field_aware:
   +      label: 'Default view mode'
   

   🤔 'The view mode that is used by default'

   (To avoid confusion with the default view mode.)

5. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -104,3 +104,17 @@ media.source.field_aware:
   +        label: 'View mode ID'
   

   🤓 Either we must say "view mode ID" everywhere, or we should just say "View mode" or here.

6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    $allowed_view_modes = $media_embed_filter->settings['allowed_view_modes'];
   

   $allowed_view_mode_ids

7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    // Only load the default if there are enough options to display the view
   +    // mode selector.
   +    $sufficient_view_mode_options = count($view_mode_options) >= 2;
   +    if ($sufficient_view_mode_options) {
   +      // If the current media embed ($media_embed_element) has a set view mode,
   +      // we want to use that as the default in the select form element,
   +      // otherwise we'll want to use the default for all embedded media.
   +      if (!empty($media_embed_element['data-view-mode']) && array_key_exists($media_embed_element['data-view-mode'], $view_mode_options)) {
   +        $default_view_mode = $media_embed_element['data-view-mode'];
   +      }
   +      elseif (array_key_exists($filter_default_view_mode, $view_mode_options)) {
   +        $default_view_mode = $filter_default_view_mode;
   +      }
   +    }
   

   We're making this form definition contain a lot of logic. Let's move this logic into a helper function.

8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +171,46 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      '#title' => t("Display"),
   

   I also have my doubts about this form label.

   I looked at #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 to hopefully get inspiration from there. That uses Use image style as the label for an image style dropdown.

   But I think Use view mode is probably even worse.

   What about Display media as? I suspect that'd be fraught with localization issues.

   What about Format, Style, Formatting, Styling?

9. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -134,16 +137,42 @@ public static function create(ContainerInterface $container, array $configuratio
   +      '#description' => $this->t('The view mode that an embedded media item should be displayed in by default. This can be overridden by using the <code>data-view-mode

   attribute.'),
   

   🤓 Nit: I think the "by" in "by using" can be omitted?

10. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -475,4 +504,22 @@ public static function trustedCallbacks() {
    +    // Combine the view modes from both config parameters.
    +    $view_modes = $this->settings['allowed_view_modes'] + [$this->settings['default_view_mode']];
    +    $view_modes = array_unique(array_values($view_modes));
    

    👍

11. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -1066,6 +1067,139 @@ public function testAlignment() {
    +    // Test MediaEmbed's allowed_view_modes option setting enables
    +    // a view mode selection field.
    

    🤓 Übernit: 80 cols.

12. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -1066,6 +1067,139 @@ public function testAlignment() {
    +    // Test that the downcast drupal-media element contains the data-view-mode
    ...
    +    // `data-view-mode` attribute.
    

    🤓 Übernit: inconsistent comment styles.

13. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -1066,6 +1067,139 @@ public function testAlignment() {
    +    // Having entered source mode means we need to reassign an id to the
    

    🤓 Übernit: s/id/ID/

14. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -1066,6 +1067,139 @@ public function testAlignment() {
    +    // the field (It requires more than one option).
    

    🤓 Übernit: s/It/it/

15. +++ b/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.es6.js
    @@ -15,6 +15,7 @@
    +              '!data-view-mode': true,
    

    🥳 Love that all our careful designing and planning in previous issues means only a single line needs to be changed here! :D

16. +++ b/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.js
    @@ -60,4 +61,4 @@
    -})(Drupal, CKEDITOR);
    \ No newline at end of file
    +})(Drupal, CKEDITOR);
    

    🤓 Übernit: this change is unnecessary, can be reverted.

3. True. This is super confusing. But pre-existing. Out of scope here.
4. I'm referring to \Drupal\Core\Entity\EntityDisplayRepositoryInterface::DEFAULT_DISPLAY_MODE.

6. Hm I see now that this kind of contradicts 5. Thoughts?

1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -292,6 +276,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * Get the default value for the view mode select element.
   ...
   +   *   The array of options for the view mode select element.
   ...
   +   *   The default value for the view mode select element.
   

   s/select/form/

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -292,6 +276,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +  protected function getViewModeDefaultValue(array $view_mode_options, FilterInterface $media_embed_filter, $element_view_mode) {
   

   Can be static.

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +172,29 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    // Store the default from the MediaEmbed filter, so that if the selected
   +    // view mode matches the default, we can drop the 'data-view-mode'
   +    // attribute.
   +    $form['filter_default_view_mode'] = [
   +      '#type' => 'value',
   +      '#value' => $media_embed_filter->settings['default_view_mode'],
   +    ];
   

   Why store this in form state? Why not look this up?

   I don't think I've seen this anywhere else in Drupal core.

4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -234,6 +276,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // The select element won't display without at least two options,
   +    // so if that's the case, just return NULL.
   

   Nit: 80 cols.

#53 is an improvement on all fronts! 👍

1. Manually tested this. Works great. But found an edge case where it breaks:
   1. Embed one image media item
   2. Turn on captions for it, hit "save"
   3. Do not enter a caption
   4. Change the view mode, hit "save"
   5. Now there's a JS error:
      

      plugin.js?t=pxzjwt:238 Uncaught TypeError: Cannot read property 'getFirst' of null
          at f._setUpEditButton (plugin.js?t=pxzjwt:238)
          at plugin.js?t=pxzjwt:174
          at Object.success (plugin.js?t=pxzjwt:321)
          at c (jquery.min.js?v=3.4.1:2)
          at Object.fireWith [as resolveWith] (jquery.min.js?v=3.4.1:2)
          at l (jquery.min.js?v=3.4.1:2)
          at XMLHttpRequest.<anonymous> (jquery.min.js?v=3.4.1:2)
      

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +172,25 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    // We'll only present the field if there is more than one option.
   

   This comment is now obsolete, the #access line makes it crystal clear.

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -158,7 +172,25 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +      '#access' => (count($view_mode_options) >= 2),
   

   🤓 Nit: the surrounding parentheses are unnecessary.

   🤔 Better yet: why not wrap this entire thing in:

   if (count($view_mode_options) >= 2) {
     $form['view_mode'] = …
   }
   

   ?

   +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -234,6 +272,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // The select element won't display without at least two options, so if
   +    // that's the case, just return NULL.
   +    if (count($view_mode_options) < 2) {
   +      return $default_value;
   +    }
   

   Then this could go away too!

4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -213,6 +245,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // If the selected view mode matches the default on the filter, remove
   +    // the attribute.
   

   🤓 Nit: 80 cols.

5. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -234,6 +272,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * Get the default value for the view mode form element.
   

   🤓 Nit: Gets

6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -234,6 +272,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * @param string $element_view_mode
   

   🤔 Would $media_element_view_mode, $media_element_view_mode_attribute or $view_mode_attribute be more clear?

There's 5 more coding standards violations to fix, but once that's done, I think this is RTBC 👍

Also, the issue summary still says there's new configuration on the CKEditor plugin, but it's now on the filter. So the issue summary needs to be updated, and it should document the reason for this decision.

Thanks for the issue summary update, @oknate! I clarified it further.

The current screenshot in the issue summary is outdated. We also should have a screenshot for each of the two UI additions.

+++ b/core/modules/media/config/schema/media.schema.yml
@@ -104,3 +104,17 @@ media.source.field_aware:
+    default_view_mode:
+      type: string
+      label: 'The view mode that is used by default'

Note: This should have been added as part of #2940029: Add an input filter to display embedded Media entities, but wasn't.

I think it's okay to do it as part of this patch instead of moving that to a separate issue/patch.

Like I wrote in #58: RTBC! 🥳

Okay, reviewed this today with @oknate.

So first of all, the actual functionality is awesome, and a really good direction for us to go in.

The UX is... :\ not... so... awesome.... however. :\

This is a classic "pogo-sticking" UI. You're given a selection of options ("Full content", "Media library") but absolutely no idea what those will result in when selected. Your only recourse is to select one, save it, see what it looks like in the WYSIWYG editor, go back, select another one, see what it looks like in the WYSIWYG editor, repeat for however many options are available, then finally make your final selection, several (annoying) clicks later than the task would ideally take. After doing this exercise 3-6 times, you will eventually learn that "Media library" means "Small" and "Full content" means "Big," but the next content author after you is going to have to go through the same (frustrating) learning experience. :\ It is definitely not optimal.

Nate and I brainstormed about several possible options to resolve this:

1. Expose the image style size in the label (so "Full content (480x600)"; "Media library (200x200)"). Cons: Not easy from an implementation POV; assumes images vs. any general media type; doesn't account for responsive image styles that can be many different sizes depending on the breakpoint definitions.
2. Provide a preview of what the media size will be below/beside the selection Could even be just a grey box, though ideally would be the media itself. Cons: Could turn into a "dragon"; lots of work to stick in front of what is currently a "must-have" media issue, which we don't want to do.
3. Allow site builders to offer a "content author-friendly" label/description for their view modes on the configuration page Cons: requires re-jiggering the configuration form into probably more like a table vs. a set of checkboxes, as well as additional data storage, etc.

I'm curious of others' thoughts. It's debatable whether we want to block this issue on solving the "pogo-sticking" problem or push it to a follow-up. The benefit here is high, even with the current UX, and Nate pointed out that Entity Embed (I think?) currently provides the same UI so we're not "regressing" from there. OTOH, it's creating new "UX debt" for us to clean up "later," which is not optimal. :\

The only other thing here is I would not name this "Format." We already have "input formats" which are a completely different concept, so please let's not conflate the concepts. I would name this "Size" or something that is more indicative of what it is.

For the record, The Phantom of Phenaproxima apparated in from his vacation to vote for option #65.3. ;)

This is a classic "pogo-sticking" UI. […] no idea what those will result in when selected

Correct, this is a limitation of entity view modes. They do not offer previews, they do not have icons.

Nate and I brainstormed about several possible options to resolve this:

Agreed that none of those are viable. The second one is technically viable but would result in a horrendous UX.

I would name this "Size" or something that is more indicative of what it is.

I do agree that usually it will be about size though. But view modes are not guaranteed to map to "size". Different fields may be displayed for example: in case of "painting" media, it could be that you're using the "painter information" view mode or the "pannable painting" view mode, which would display completely different information.

The root cause of all this: view modes are a powerful abstraction, but they're so abstract that the content author must know about them. This is honestly why I personally believe this does not belong in Drupal core but in a Drupal contrib module.

The reason I RTBC'd it nonetheless is that:

this <select> is only ever visible if you select at least two checkboxes in . Otherwise, the default view mode is still used (which we just improved at #3066802: Ensure that embedded image media items do not display at unreasonable sizes by default to look sane out-of-the-box). In other words: only sites that A) create multiple view modes, B) specifically configure this to enable content authors to pick a specific view mode will ever get to see this dropdown in the EditorMediaDialog!

In other words: only sites that A) create multiple view modes, B) specifically configure this to enable content authors to pick a specific view mode will ever get to see this dropdown in the EditorMediaDialog!

Correct. However, the reason this issue is in the "must-have" list is to prevent this situation from happening, since it prevents you from editing your content:

In order to resolve this, we need the ability to resize the embedded media, which implies the existence of at least two view modes in most cases (I think? Though maybe not?). This issue is one possible way of solving that. #3078287: Constrain the width of aligned images, media, blockquotes etc to a max of 75% is another, and would be my preference, since it doesn't add any complexity to the UI, but that one seems not to be as easy to get in as we thought. :\ (But @oknate seemed to think it was making headway, which is great.)

Fair point on why not "Size." "Display" or something could work in that case then.

Correct. However, the reason this issue is in the "must-have" list is to prevent this situation from happening, since it prevents you from editing your content:

That is not my understanding.

#3078287: Constrain the width of aligned images, media, blockquotes etc to a max of 75% is another.

That is the only way to solve the problem you demonstrated in the screenshot.

We don't want content authors to pick a different view mode because it looks bad in the assistive text editor. In fact, with this issue they still cannot "solve" that unless the configuration changes that I mentioned are made.

This issue is about allowing content authors to pick the view mode that is appropriate for the context it will be displayed in to end users. A number of people who have been involved in the Media Initiative for years were very vocal that on complex site use cases, this is essential. IIRC @seanB and @marcoscano felt very strongly about this. Perhaps they can chime in here?

"Display" or something could work in that case then.

Hah — that's what it used to be! 😂 But both @phenaproxima in #44.8 and I in #50.8 thought that was too confusing and odd. I provided some suggestions in #50.8, and @oknate went with Format. I'm happy to go with Display if you feel that is clear after all :)

Personally, I would never enable the 'Media library' view mode for use in WYSIWYG editor. When you need multiple ways of displaying media in the editor, you would create your specific view modes, name them properly, and then only allow the ones that are meant to be embedded.

So for me a sensible default would be: only enable the 'default' display (this also means by default editors wouldn't have to choose anything!), and allow users to enable other things as they see fit after they created new/more view modes.

If I may contribute my 2ct from the sidelines regarding #65:

TLDR:

• Don't block on #65, go with a follow-up, if needed.
• Go with #65.3.
• Don't use "Size" as option label. If we need to change it, go with "Display".

Overall, I see the point, but I think it's more like a should-have. The issue is mitigated by the fact, that editors will learn, which view mode they would need to choose to get the desired as they will get familiar with the site. At the same time, the existing implementation is already a major improvement and if you would ask me, I would be in the group of the very vocal people mentioned by Wim, who argue this is a feature needed in more complex setups. I can imagine that basic sites could get away with just one view mode, but for most of our projects for example, that are usually more complex, we just need the option. The implementation in the patch would work very well for us already and what Sean said in #72 makes a lot of sense, so why not just go with it as is and work on a solution for #65 in a follow-up?

W/r/t the options:

1. Please don't. The major con is mentioned already in #65. Also, some media may not have an image field at all or multiple image fields. I think it would just be too difficult to come up with something that would even work well for the 80% use case.
2. This would be the ideal solution, but I think it's not practical to implement in core. This might be something that could be added in contrib until it is robust enough.
3. I think this would be the best option. If we really don't want to go with the view mode label, it is a good compromise between what would be the ideal solution (in-dialog media preview) and what is practical to implement. In fact, this is something we have often done programmatically for our projects in D8 and also in D7 (implementation based on contrib).

Regarding the label of the option:

• Like webchick, "format" makes me think of a "text format". So +1 for coming up with something else.
• Please don't name it "size". As already mentioned by Wim, a different view mode may not be about size, but about different appearance in front-end or different fields. For example, we often have a compact view mode with just some fields exposed and something like a full view mode with more fields or meta data exposed for our media items. Both media items may have the same size in the front-end and the size may change in different break points/devices. For example, a media item that takes 50% of the content column on a desktop computer may take 100% of the content column on a smart phone. Basically, pro/con is equal to option 1) for the view mode label above and therefore I think it's not a good idea.
• I looked at the options in #50.8. Personally, I don't see why we couldn't just use "view mode". After all, that's what we want the user to choose, so why not just use that as a label? On the other hand, I can understand, that the term is maybe too technical. I see the localization problem with "Display as", so +1 on not using that. "Style" would be ok, maybe even "Layout". But (as a non-native speaker) I think "Display" would be even better. I think editors will understand that they may choose different types of display of the media item in the front-end and it's also nicely in line with "Manage display", where you configure the view modes as a site builder. So +1 on that one.
• The most important thing in my opinion would be to provide a good translation context for the string, so that site builders would be able to change the label easily, if needed.

And finally, thanks for working on this! Really looking forward to using all the new features in production :).

oknate asked for my opinion on the mentioned option in #65, so here it is:

1. Only works for images, which is kind of limiting and hard when responsive image styles are configured (or no image style).
2. I don’t really see how/where to put the preview and how that is better than selecting something and then seeing the preview. I mean, it saves you a click, but you still have to go through all options to know what the best one is.
3. Better labels help, but view modes already have machine names and display labels, so not sure what an admin label would add? Why not give the view mode a helpful name.

So I would go for option 3, if using the display name of the view mode is not good enough.

I also agree with everything in #72. Thanks FeyP for taking the time to write this.

Late to the party, but I'm also in favor of not blocking this on #65

I get the concern though... To me, it boils down to: depending on which hat you are using, you'll see one side of the vase... :)

If I'm thinking from a wordpress-ish perspective, under the expectation that Drupal evaluator's experience should "just work" without UX hurdles, it is indeed somewhat annoying having to click through all the options to find something that suits what you are doing. To me in that scenario, the solution would possibly involve directly embedding, and then allowing for resizing (🙈), or having a friendly tooltip or something that would let users easily "make this bigger/smaller". That has a world of ramifications and complexities, especially when dealing with non-image media. (and for the record, I personally _don't_ think we should go this route)

If I'm thinking from the ambitious-digital-experience-site perspective, where editors are relatively well-trained and do that all day long, I think it's fairly reasonable to expect that they will be familiar with the relationship between "viewmode labels -> how things look".

And I reiterate what others said... yes, I think the ability to choose the representation of the "embedded thing" while embedding it is a must on all sites I've worked on. Also fair to say that these sites wouldn't have a problem in using a contrib module to get that extra bit of functionality...

TBH, I am not really sure anymore which sites are in the 80% Drupal use case at this point... 🤷‍♀️

Also, a minor question about:

+++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
@@ -134,16 +135,42 @@ public static function create(ContainerInterface $container, array $configuratio
+    $form['allowed_view_modes'] = [
+      '#title' => $this->t("View modes selectable in the 'Edit media' dialog"),
+      '#type' => 'checkboxes',
+      '#options' => $view_mode_options,
+      '#default_value' => $this->settings['allowed_view_modes'],
+      '#description' => $this->t("If two or more view modes are selected, users will be able to update the view mode that an embedded media item should be displayed in after it has been embedded.  If less than two view modes are selected, media will be embedded using the default view mode and no view mode options will appear after a media item has been embedded."),
+      '#element_validate' => [[get_class($this), 'elementValidateAllowedViewModes']],

Would it be worth disabling the options that have instances using them?

Not sure if it's worth the complexity of checking this, but I can see a situation where the dev/site builder "thinks" that disallowing one viewmode here will make it magically disappear/change from wherever it's being used. Not letting them disallow viewmodes that are in use in the first place might be a simple way to avoid all that confusion.

#75: Thanks!

It would be a lot of code to determine the entity usage, and the allowed_view_modes only control access to this form element, at this point.

I agree it's a hard thing to solve... The confusion I wanted to point out was not so much concerning the form, but more in regards to the situation when the site builder disallows a viewmode, and existing content continues to be rendered in that viewmode. There might be an expectation that it wouldn't happen (even though it's clear that if you don't provide a replacement, the system can't guess what it should use to render your existing content, but...)

Since this is a site-building/dev facing setting, maybe with an additional help text like "Unselecting these options will not change existing content that was embedded with them", or "Will only affect new content" could be enough?

1. @seanB in #71++ … but your recommendation is exactly what this patch does 🤓
2. @FeyP in #72: thanks for chiming in with so much detail and nuance — this is super valuable! 👏🙏

   Just one remark: the problem with using the label "view mode" is that only a Drupal site builder or developer understands what this means. Then again … perhaps it's not really more unclear than "Display" 🤓 Agreed that good translation context is essential.

3. @marcoscano in #74: Hah, that vase expression is funny! 😃
   

   Would it be worth disabling the options that have instances using them?

   We cannot detect this both reliably and efficiently. Furthermore, this is only a UI-level suggestion setting: this does not restrict the view modes actually used and respected in filtering. It's still possible to specify any valid view mode ID in the data-view-mode attribute.

   So it does not work in the way you describe in #76 — that was considered to be undesirable. This setting does not affect neither existing content nor new content: it only affects the EditorMediaDialog's UI elements.

Awesome, thanks for all of the thoughtful feedback, esp. from folks using Media with content editors day-to-day! Very valuable. :)

I think I'm convinced that we can discuss the "pogo-stick" problem separately, and not put it in front of this patch. It's a good point that the impact might be moot (or at least far lessened) if the site builders choose view mode labels that make sense to content authors. And they can always be trained, that is true; it's just definitely less ideal than an intuitive UI that doesn't need training. :) For that, we need something more akin to what @marcoscano brings up, where the resizing happens on the front-end, but that's definitely a huge UX dragon to slay, and also not a must-have we want in front of Media Library. ;)

So then I think this is only "needs work" for the label; sounds like we're coalescing around "Display", but we need to sort out the translation context.

This looks great, leaving at RTBC cause there's only a personal preference nit I can find

1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -145,6 +145,7 @@
   +        // eslint-disable-next-line new-cap
   
   @@ -223,7 +224,10 @@
   -            } else if (this.data.hasCaption && !this.oldData.hasCaption) {
   +            } else if (
   +              this.data.hasCaption &&
   +              !this.data.attributes['data-caption']
   +            ) {
   
   @@ -266,11 +270,13 @@
   +          // eslint-disable-next-line new-cap
   ...
   +            // eslint-disable-next-line new-cap
   
   @@ -462,7 +468,10 @@
   +         * @param {array} data
   +         *   The data to stringify into a hash.
   ...
   +         *   A hash string.
   
   +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -160,7 +160,7 @@
   -            } else if (this.data.hasCaption && !this.oldData.hasCaption) {
   +            } else if (this.data.hasCaption && !this.data.attributes['data-caption']) {
   

   these changes seem out of scope - can you advise why they're required - edit: nevermind found comment #55

2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -35,14 +37,24 @@ class EditorMediaDialog extends FormBase {
   +  public function __construct(EntityRepositoryInterface $entity_repository, EntityDisplayRepositoryInterface $entity_display_repository) {
        $this->entityRepository = $entity_repository;
   +    $this->entityDisplayRepository = $entity_display_repository;
   

   note to self/other reviewers - this class has not yet been in a tagged release, so we don't need to do the BC dance here. In addition, we also don't need an upgrade path for the config changes 🎉

3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -234,6 +271,43 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $default_value = $media_element_view_mode_attribute;
   ...
   +    elseif (array_key_exists($filter_default_view_mode, $view_mode_options)) {
   +      $default_value = $filter_default_view_mode;
   

   ubernit: we can return early here and avoid the elseif

#81.1: I discovered that bug in #54.1. @oknate included the fix here, but alternatively #3081988: Disappearing edit button and caption on media embed when using dialog could be committed first. Or actually, we could even omit it from this issue/patch altogether. I think that'd indeed be preferable.

+++ b/core/modules/media/config/schema/media.schema.yml
index 68b10fdf8c..0719ba5d09 100644
--- a/core/modules/media/js/plugins/drupalmedia/plugin.es6.js

🤓 Nit: Now all changes in this file seem pointless too — literally nothing is changing but eslint compliance "fixes" and a docs addition. Let's omit that from this issue too: tighter scope is easier to review, commit and git blame :)

Issue credit.

Ok, gave this another once-over; apart from this line of horror and doom that we should probably move to its own method in a follow-up...

+    if ((empty($form[‘alt’]) || $form[‘alt’][‘#access’] === FALSE) && $form[‘align’][‘#access’] === FALSE && $form[‘caption’][‘#access’] === FALSE && $form[‘view_mode’][‘#access’] === FALSE) {

...this looks good to me, and looks like @larowlan's feedback has been addressed as well. The UX issues identified in #65 are somewhat mitigated by a) we don't show a view mode selector in the dialog unless there are at least two options selected by the site builder, and b) site builders are able to name these whatever they want, including "Small" and "Big" or whatever.

I can see this being a really powerful feature for lots of sites (including Drupal.org!) so let's get this sucker in, and tag it as a highlight!

Committed and pushed to 8.8.x. Woohoo!!

Drupal\media\FormEditorMediaDialog::buildForm():

    // Store the default from the MediaEmbed filter, so that if the selected
    // view mode matches the default, we can drop the 'data-view-mode'
    // attribute.

Dropping the 'data-view-mode' attribute can result in corruption of user data. I've opened a critical bug to address: #3109289: MediaEmbed conflates default view mode with user-selected view mode

Superb find, @Chris Burge! 👏🙏