MediaThumbnailFormatter produces unhelpful text alternative and title attributes for media thumbnails

This is actually by design. The thumbnail formatter shows an automatically created image as the thumbnail. It's properties are not expected to be user-submitted:

in \Drupal\media\Entity\Media::updateThumbnail() we have:

...

    // Set the thumbnail alt.
    $media_source = $this->getSource();
    $plugin_definition = $media_source->getPluginDefinition();
    if (!empty($plugin_definition['thumbnail_alt_metadata_attribute'])) {
      $this->thumbnail->alt = $media_source->getMetadata($this, $plugin_definition['thumbnail_alt_metadata_attribute']);
    }
    else {
      $this->thumbnail->alt = $this->t('Thumbnail', [], ['langcode' => $this->langcode->value]);
    }
...

so if you are using core media images, the string Thumbnail is expected there.

If you are only dealing with images, it's true that it doesn't make much sense, so you could try to use the "Rendered Entity" formatter instead, and show a custom viewmode where you can configure the real image field (where the Alt text submitted by the user will be displayed).

I spotted this alt="Thumbnail" issue in a review of #2962110: Add the Media Library module to Drupal core during a UX meeting today.

Re: #2

This is actually by design. The thumbnail formatter shows an automatically created image as the thumbnail. It's properties are not expected to be user-submitted:

This is a bad design for text alternatives, so I'm setting it back to bug report, with priority major because it's an issue at WCAG level A.

What's that intended use-case for MediaThumbnailFormatter - like what pages was it created for? What I've seen is that the text alternative is always just "Thumbnail", whether it's an audio icon, document icon, or an image derived from an uploaded file.

It's used on the table at admin/content/media, where the purpose of that page is to review all of the available media items. A visually-impaired content manager using a screen reader may be trying to find a particular image, knowing full well what alternative text they are expecting to hear, because they've previously encountered it. If all the image media items have their text alternatives replaced with "Thumbnail", a visually impaired editor will face difficulty finding the right media item.

I realized you can't get specific alt text for audio media items, but alt="Thumbnail" helps nobody.

Resolution:

1. For image media items, the MediaThumbnailFormatter should use the alt text from the image field on the media item.
2. For other media types like audio, we could either use alt="" so there's effectively an empty table cell at admin/content/media, or else use a more specific message like alt="Thumbnail not available". It depends on how many places the formatter could be used.
3. MediaThumbnailFormatter could have a setting to tailor the fallback text alternative for particular displays.

Re: #3

It is bug according to SEO.

Search engines aren't the intended audience for the image alt attribute, although they will index it. The text alternative is for users.

Would it be possible to add something to Views so you could nominate a field or groups of fields for an output item designated as Alt text then have display code pick it up?

You could build the alt text as a non display field using existing Views facilities and conventions. When you designate a field as an image for output, or whatever, you could attach the alt field. This can then be used for any display using views.

@andrewmacpherson thanks for pointing this out.

It turns out our API already allows for an easy way of exposing the default thumbnail alt, but our implemented source plugins are not making use of that so far.

This patch makes the default thumbnail alt to be:
- The image asset alt if existing, in media types using the image source.
- A string like "{source plugin label} thumbnail" as a fallback for other plugins.

Does this wording make sense?

After applying the patch, core's Media types would have something like this by default:

Setting back to NR, because we need some feedback from @andrewmacpherson

Sounds good.
(Tests were not updated, hence the expected failures.)

Will review soon.

Leave the "accessibility" tag on please, that denotes the topic area, separate from the "needs accessibility review" request.

1. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -69,6 +70,9 @@ public function getMetadata(MediaInterface $media, $attribute_name) {
   +      case 'thumbnail_alt_value':
   +        return $this->t('@type thumbnail', ['@type' => $this->pluginDefinition['label']]);
   

   A thumbnail is a reduced-size version of pictures or videos. We are using icons for other media types mostly for layout purposes. In a list of PDF's for example, having the File thumbnail text for every row is not very helpful for visually impaired users and probably even annoying. I think the suggestion of andrewmacpherson to leave it empty is probably best. We have to make sure the empty alt text is added in the HTML though.

2. +++ b/core/modules/media/src/Plugin/media/Source/AudioFile.php
   @@ -15,7 +15,8 @@
   + *   default_thumbnail_filename = "audio.png",
   + *   thumbnail_alt_metadata_attribute = "thumbnail_alt_value"
   
   +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -17,7 +17,8 @@
   + *   default_thumbnail_filename = "generic.png",
   + *   thumbnail_alt_metadata_attribute = "thumbnail_alt_value"
   
   +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -22,7 +22,8 @@
   + *   default_thumbnail_filename = "no-thumbnail.png",
   + *   thumbnail_alt_metadata_attribute = "thumbnail_alt_value"
   
   +++ b/core/modules/media/src/Plugin/media/Source/VideoFile.php
   @@ -15,7 +15,8 @@
   + *   default_thumbnail_filename = "video.png",
   + *   thumbnail_alt_metadata_attribute = "thumbnail_alt_value"
   

   Instead of adding a thumbnail_alt_metadata_attribute everywhere, why not just change the default value in Media::updateThumbnail()? This will be easier for contrib/custom media sources.

3. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -138,6 +139,10 @@ public function getMetadata(MediaInterface $media, $name) {
   +      case 'thumbnail_alt_value':
   +        $image_field_alt = $media->get($this->configuration['source_field'])->alt;
   

   I think images and probably the new remote video media types are the only sources for which the thumbnail_alt_metadata_attribute actually makes sense. Nice!

When adding a remote video, you don't know what the screenshot will actually be, since it will be a particular still of the video. Youtube for instance leaves the alt tag empty on those thumbnails. Also, when uploading videos, a general icon is shown.

So in my opinion a valid alt text is only applicable to images.

I updated the patch and also updated the test script.

Forgot something when changing the test. Patch attached

Argh.... forgot some other assertEquals...

Saw this working and the test passes, RTBC.

I'm not sure this is something we should do as-is, since it could be considered a regression. Is bad alt text better than no alt text? I think we need accessibility and potentially UX review of this before we can RTBC. Sorry :(

I'm OK with the new approach, so +1 for me to RTBC this.

However, I still wanted to have accessibility sign-off before officially marking this as ready to go, so leaving it in NR for that.

Thanks!

EDIT: cross-post! But I'm glad we're on the same page... :)

Is bad alt text better than no alt text?

An argument was brought that thumbnails are in most situations decorative images:

https://www.w3.org/WAI/tutorials/images/decorative/

and in that context, having the same ALT text repeated all over would produce more harm than good. But let's hear from the experts :)

There are tests in the patch, so removing the tag.

If you could just remove the title attribute from this string I'd be happy to remove the "needs accessibility review" tag.

Title on images is just a bad idea and not well supported:

https://developer.paciellogroup.com/blog/2016/02/short-note-on-use-of-al...

We need to know that assistive technology is responding consistently to the information that is presented.

In converstions from the accessibility slack channel the current patch looks like this for text files:

<img src=“/sites/default/files/styles/thumbnail/public/media-icons/generic/generic.png?itok=GI0cAs8-” alt=“” title=“Test text file 2” typeof=“foaf:Image” class=“image-style-thumbnail” width=“100" height=“100”>

For images it looks like this:

<img src=“/sites/default/files/styles/thumbnail/public/2018-07/Screen%20Shot%202018-07-04%20at%208.06.22%20PM.png?itok=8mMtfLqA” alt=“sample screen shot” title=“screenshot” typeof=“foaf:Image” class=“image-style-thumbnail” width=“57" height=“100”>

With the title removed from both this would be good given that this information is repeated later in the table.

The one thing that might be useful to add is that the first column in a table is often a header. This isn't the case here if we remove that first item. Is it possible to reorder the table so that the file name & link come up first? This would make it easier for assistive tech users to quickly navigate through it.

Let's remove the title for thumbnails in this issue, since it directly relates to the empty alt attribute as stated in #27. We should have a follow up to fix the order in which the information is shown for the media table to make it more accessible.

@Nitebreed when applied patch #21 via composer update drupal/core, it failed. and the MediaSourceTest.php is rejected. I am on Drupal 8.6.1

Previous patch faield (#22), trying a new one. I wish I know I can delete it.

Sorry for the blast, but the #24 can be applied now. If someone can add some test would be great. Thanks.

Could you provide an interdiff as well?

interdiffs are good. Just running the bot against #33

Re-roll + removing the title, as requested by #27.

Triggering the testbot so it tells me what tests need to be updated.

Except for these two things, this patch looks right to. Since the 'title' value is gone, as @mgifford requested in #27, I'm also removing the "needs accessibility review" tag.

1. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -138,6 +139,10 @@ public function getMetadata(MediaInterface $media, $name) {
   +        $image_field_alt = $media->get($this->configuration['source_field'])->alt;
   +        return !empty($image_field_alt) ? $image_field_alt : parent::getMetadata($media, 'thumbnail_alt_value');
   

   This can be streamlined a bit:

   return $media->get($this->configuration['source_field'])->alt ?: parent::getMetadata($media, $attribute_name).

   Additionally, can we not directly refer to $this->configuration['source_value']? We have an API for that, so let's use it ($this->getSourceFieldDefinition($media->type->entity)->getName()).

2. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -293,14 +293,14 @@ public function testThumbnail() {
   +    $this->assertEquals('', $media->thumbnail->alt, 'Alt text was not set on the thumbnail.');
   

   We should use assertSame() when comparing strings.

Thanks for reviewing!

Addressed #40. For #40.1, is there anything preventing us from using ::getSourceFieldValue() here? I guess it makes more sense to use the API, but maybe I'm missing something.

Of course there is a reason... 🤦

1. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -138,6 +139,9 @@ public function getMetadata(MediaInterface $media, $name) {
   +      case 'thumbnail_alt_value':
   +        return $media->get($this->configuration['source_field'])->alt ?: parent::getMetadata($media, $name);
   

   I would expect a test change related to this. Perhaps in \Drupal\Tests\media\FunctionalJavascript\MediaSourceImageTest::testMediaImageSource()

2. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -350,7 +347,7 @@ public function testThumbnail() {
        $this->assertSame('This will be alt.', $media->thumbnail->alt, 'Alt text was not set on the thumbnail.');
   

   Let's remove the assertion message here. It's wrong and confusing. We could choose to remove the messages from the changed assertions too if you want.

Thanks for the feedback!

Fixing small typo on IS.

#46.2++++++++
#46.1: I definitely should have seen that… 😇

+++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
@@ -253,16 +253,16 @@ public function testThumbnail() {
+    $this->assertEmpty($media->thumbnail->title);
+    $this->assertSame('', $media->thumbnail->alt);

I asked @marcoscano why assertSame('', …) is preferred over assertEmpty(…). The reason is to make it explicitly about an empty string. Seems reasonable.

Committed and pushed e6dfd046db to 8.7.x and c2c4d3d04a to 8.6.x. Thanks!

Backported to 8.6.x since it is a bug.

HI folks, the situation of decorative images with descriptive alt text within a link wrapper always bugged me.
I see now that the approach of using the 'thumbnail' display formatter has a purpose! I've used a 'rendered entity' and a custom display mode of the media image.

This

+++ b/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php
@@ -167,13 +167,13 @@ protected function getExpectedNormalizedEntity() {
-          'alt' => 'Thumbnail',
+          'alt' => '',
           'width' => 180,
           'height' => 180,
           'target_id' => (int) $thumbnail->id(),
           'target_type' => 'file',
           'target_uuid' => $thumbnail->uuid(),
-          'title' => 'Llama',
+          'title' => NULL,

The same changes are necessary in JSON:API's tests: #3001193: CommentTest::testPostIndividualDxWithoutCriticalBaseFields() fails on 8.7 since #2885809

Hi,

Attached patch use source_field if exist to use alt stored.

Hi,

Attached patch rerolled for d10.

Rerolled for 9.5.11