The default display for new media types should only show the source field

3. #138: I'm … surprised by the descoping of non-image media source media types. IMHO the goal of this issue was to make embedding of arbitrary media look/be formatted as expected. I'm fine with tightening scope, but then the follow-up needs to also be at least major, and also should be a must-have. As an absolute minimum, we need this to work well for YouTube videos out of the box in 8.8. I see #3081233: The default display for new media types should only show the source field is not "major", but "normal", so bumping priority.

To evaluate the Media Library roadmap I could use some detail on this issue. Could we add steps to reproduce and maybe a screenshot comparing what happens with Image vs. the other types?

Also, is this only about the UI, or does this also affect the data model itself of the displays created in config? If so, this issue becomes more important to the roadmap as it affects the data models people end up with when using the module (and it's difficult to go back if you set up a data model you don't want). If it would change the actual definitions created, a side-by-side comparison of those would help as well.

Thanks!

But it doesn't affect data models at all, and almost certainly will not need an update path.

Correct. It doesn't affect data models, but it does affect concrete EntityViewDisplays. But we'd only do this for newly created ones, we wouldn't provide an update path for already-created ones. I think? 🤔 Perhaps that should happen?

The view displays we ship with are already doing fine in this regard, for example <code>core/profiles/standard/config/optional/core.entity_view_display.media.remote_video.default.yml:

content:
  field_media_oembed_video:
    type: oembed
    weight: 0
    label: hidden
    settings:
      max_width: 0
      max_height: 0
    third_party_settings: {  }
    region: content
hidden:
  created: true
  name: true
  thumbnail: true
  uid: true

As you can see, this shows the source field and hides all others.

This issue is about ensuring that this happens by default when creating new EntityViewDisplays are created for @MediaSources other than image.

Tested.

On Standard install, added Media and Media Library at the same time.

• 👍 At /admin/structure/media/manage/audio/display, only the Audio file field displays.
• 👍 At /admin/structure/media/manage/document/display, only the Document field displays.
• 👍 At /admin/structure/media/manage/image/display, only the Image field displays.
• 👍At /admin/structure/media/manage/remote_video/display, only the Video URL field displays.
• 👍At /admin/structure/media/manage/video/display, only the Video field displays.

I created new media types for each source, and for each, only the source field was displayed.

I'm going to mark it back as needs work because I think the test coverage should use a count assertion to assert that only one component is enabled. Right now, we're only adding an assertion that the thumbnail isn't enabled. If we don't assert only one component is enabled there could be a regression without the test catching it.

@@ -62,8 +62,10 @@ public function testMediaFileSource() {
     // Get the media entity view URL from the creation message.
     $this->drupalGet($this->assertLinkToCreatedMedia());
 
-    // Make sure the thumbnail is displayed.
-    $assert_session->elementAttributeContains('css', '.image-style-thumbnail', 'src', 'generic.png');
+    // Make sure a link to the file is displayed.
+    $assert_session->linkExists($test_filename);
+    // The thumbnail should not be displayed.
+    $assert_session->elementNotExists('css', '.image-style-thumbnail');

Once that is fixed, I think this is ready.

We also still need screenshots. Could either one of you do that, @oknate or @phenaproxima? 🙏

This looks great now. Marking RTBC, dependent on all the tests on #10 being green. Nice improvement!

#12 weren't quite the screenshots I was after, more the end-user experience... @phenaproxima did a walkthrough, however, and this is a huge improvement, so big +1.

Before, when embedding a new media type:

While a site builder can go in after the fact and clean the display settings up, that's an annoying process, and it's also completely disconnected from the media insertion point.

After, when embedding a new media type:

MUCH better. The only one remaining thing is we should set the Label to "visually hidden." While there might be some use case out there somewhere for exposing Drupal's internal field label to site visitors, core definitely doesn't need that, and so it makes sense for the default to be just the rendered media and that's it.

Also eyeballed the patch; this is great! It's removing code and making the defaults sensible for everyone. Awesome work!

So "needs work" just to tweak the label setting, and then we are good to go!

I manually retested. I created a new media type with each source that ships with core. For each:

• there was just the one expected component, and
• it had a visually hidden label.

Marking RTBC again, assuming the tests pass.

Awesome stuff.

We had a code review wherein I pointed out that the old tests weren't quite testing "foreach media type, is the label hidden?" but rather if each individual media type's label was being hidden; that's since been fixed.

I was also confused about why we need code like:

diff --git a/core/modules/media/src/Plugin/media/Source/AudioFile.php b/core/modules/media/src/Plugin/media/Source/AudioFile.php
index 22291434e9..1e21593085 100644
--- a/core/modules/media/src/Plugin/media/Source/AudioFile.php
+++ b/core/modules/media/src/Plugin/media/Source/AudioFile.php
@@ -33,6 +33,7 @@ public function createSourceField(MediaTypeInterface $type) {
   public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
     $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
       'type' => 'file_audio',
+      'label' => 'visually_hidden',
     ]);
   }
 

But @phenaproxima dug into it with me, and basically, at one point in the display definition process, the default values get merged in and those default values set label to "above." Changing that would mean a change to the field system as a whole (as well as changing like 10+ years of expected behaviour), soooo... definitely out of scope here. ;)

There was an earlier concern raised about changing data models. We don't do that here, but rather defaults, and only for new media types, so there's no impact to existing sites (by design; we don't want to mess with their config, which they've probably already done to work around this problem).

GREAT work here, folks!

Committed and pushed to 8.8.x. Thanks!