Problem/Motivation

Media derive their thumbnail images and alt text for those images from their media source plugins. For image-based media, that's the "Image" media source plugin. When the plugin is asked to present the alt text for its thumbnail image, it returns the alt text stored in the image field of the media entity. This works great, unless the alt text is an empty string. The Image plugin treats this as a "falsey" value and ends up returns NULL for the alt text instead of the empty string.

An empty string value for the alt text attribute (alt="") has important meaning for accessibility. It instructs assistive tech that the image is decorative and should be ignored by screen readers. Drupal's image formatter template will correctly output the alt="" attribute on the image element in this case. If the alt is NULL, then Drupal's image formatter template won't print the alt attribute at all. This is very problematic as assistive tech may instead announce the image filename when it comes upon the image, because it's assuming the missing alt attribute is a mistake and the image may be important for the user to know about.

Steps to reproduce

  1. Install media module.
  2. Edit the image field on the Image media type and uncheck the Alt text required checkbox and save.
  3. Add an Image media entity - upload an image and leave empty alt text field.
  4. Visit the media library page: /admin/content/media and inspect the thumbnail image. Observe that the alt attribute is missing.
  5. Then inspect image tag. you 'll notice if nothing pass in alt text field then alt tag 'll not display.

Proposed resolution

This is a simple fix. Modify the getMetadata method of the Image media source plugin so that it if the alt text of the associated media entity is an empty string, return the same empty string.

Remaining tasks

User interface changes

API changes

Data model changes

No. Could have an update function to update all existing media thumbnails to swap NULL thumbnail alt values with an empty string instead, but that's probably not worth it. Some sites will have tens of thousands of media items and I guess this update function could take a while to run.

Release notes snippet

CommentFileSizeAuthor
#5 patch_applied.jpg89.45 KBgaurav-mathur
#5 afterpatch.jpg60.54 KBgaurav-mathur
#2 3319601-thumbnail-empty-alt-2.patch729 bytesbkosborne

Issue fork drupal-3319601

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bkosborne created an issue. See original summary.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3319601-thumbnail-empty-alt-2.patch729 bytes

Gitlab integration is currently down, so cannot create an issue fork. In the meantime, here's a normal patch. I guess this needs tests but lets see if this causes any to fail.

ressa’s picture

ressa
he/him
commented

See these Gitlab issue, hopefully it will be fixed soon:

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Issue summary: View changes
gaurav-mathur’s picture

gaurav-mathur commented
StatusFileSize
new afterpatch.jpg60.54 KB
new patch_applied.jpg89.45 KB

Applied patch #2 on the drupal version 10.1.x successfully. It looks good to me.
Refer to the screenshot
Thankyou

rutiolma’s picture

rutiolma
Location 🇵🇹
commented
Status: Needs review » Needs work

In fact the problem doesn't seem to be only with the thumbnail on content administration but any image rendered as a thumbnail doesn't have an alt attribute.

To test, follow the described steps on this issue and then just add a media image field to a content type and set its view display as "thumbnail". You'll notice that there's no alt attribute for the image when you view the node.
This problem doesn't exist if you set the view display as "render entity".

Not sure if this is by design but there are several related issues on contrib modules.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Does the patch not resolve that though? Did you test it? Wondering why you set the status back to Needs Work

rutiolma’s picture

rutiolma
Location 🇵🇹
commented

The patch does work for the administration view (which this issue is about) but IMHO this should be tackled in a more generic way because the problem seems to be related with thumbnails in general.

meeni_dhobale’s picture

meeni_dhobale commented

I tried to reproduce this issue, and observed that, this thumbnail issue only occur on /admin pages, I tried to reproduce in other pages as well. I create one view and add some content images as thumbnail, but in the view there is already alt attribute is present. For admin pages this patch which mentioned in #2 are work properly.

@rutiolma can you please provide some steps to reproduce same issue which you talk about.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: +wcag111

Adding tag for WCAG SC 1.1.1

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sunlix made their first commit to this issue’s fork.

sunlix opened merge request !8548

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Status: Needs work » Needs review

Hey together,

I have research a little bit on this and I am sure, that the patch in die issue fork solve this.
The related property here is thumbnail__alt (Database column in media_field_data which comes from the BaseFieldDefinition in the Media Entity itself.
This property/column stay's null if you upload an image to a media entity based on the image source plugin and left the alt field empty.
But the ImageItem field plugin already has a default value for alt which is an empty string.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

For remote video media entities based on the oembed source plugin this issue doesn't come up because the default value is set on the updateThumbnail method in the Media entity.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

So the issue is "only" related to media entities which are based on media source plugins that have $thumbnail_alt_metadata_attribute defined.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Need tests

Feels like something that should probably have some kind of test coverage even though it's a small change.

pooja_sharma made their first commit to this issue’s fork.

pooja_sharma’s picture

pooja_sharma commented
Issue summary: View changes

After applying patch , if you want test alt attribute display or not if nothing pass in alt text field then test with new media

Don't try to update alt text of existing media it is part of other issue: https://www.drupal.org/project/drupal/issues/3232414

I got bit confused whether patch is actually working or not.

pooja_sharma’s picture

pooja_sharma commented

Added test coverage in a distinct test file to assess specific scenario.

Please review, move NR

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review
sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

@pooja_sharma

Thank you a lot for pushing this forwars with a functional test. :-)
I have changed the test cases a bit to have to specific test case for given strings and a test case with no given alt attribute.
Both cases have to result in having a rendered alt attribute.

I am unsure if we have to add a specific alt = NULL testcase, because this is not intended by the used field API here. ImageItem have default values for it's properties which should not altered or?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work

Nice, the test coverage isn't triggering the bug it seems though, the test-only run https://git.drupalcode.org/issue/drupal-3319601/-/jobs/2003864 is green

pooja_sharma’s picture

pooja_sharma commented

Thanks for review, recent code changes lead to pass test case in every scenario which is not correct , so revert back changes.

Alt attribute field is bind with image media entity if alt attribute not attach with empty string then it 'll not be use of adding this specific test case as in manual test if we keep empty "attribute field" then alt tag not render on FE which leads to issue.

PLease review, moving to NR

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review
sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

mea culpa, I did make an understanding failure. Thought it is defaulting to the ImageItem default settings, which is not, because here is no field api involved in the test szenario. :-)
Thank you for pointing that out, did learn something new with testing! :)
Should be good to go.

pooja_sharma’s picture

pooja_sharma commented

Rebased the MR with latest code, seems working fine

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on the MR.

pooja_sharma’s picture

pooja_sharma commented

Addressed the mentioned suggestions & removed the newly created test file as test code moved to existing suggested test file.

Please review , moving NR

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left some comments on the MR

pooja_sharma changed the visibility of the branch 3319601-media-image-thumbnail to hidden.

pooja_sharma changed the visibility of the branch 3319601-media-image-thumbnail to active.

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review

Applied suggestion & left comments as well.

Please review, moving NR

smustgrave’s picture

smustgrave commented

Will leave in review then for others but think the other test location makes more sense since the test there is checking the media content view while MediaSourceImageTest is not.

pooja_sharma’s picture

pooja_sharma commented

Applied the suggestions, Moved test coverage code from MediaSourceImageTest to MediaOverviewPageTest

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Need tests +Needs Review Queue Initiative

Re-ran failure for the editor module and appeared to be random.

Believe all feedback for this one has been addressed.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Thanks for continuing to work on this!

I left some comments in the MR. I am also not sure that this is the best way to test this. I'll ask about that.

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review

Addressed the feedback, Please review the MR, moving NR.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

Added a review for proposing null coalescing operator. So the change will be just 1 character. :-P
Thank you @pooja_sharma for your fast interaction on this!

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Feedback from #38

Yes thanks @pooja_sharma for keeping this one alive.

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review

Addressed the requested changes, Please review, moving NR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback has been addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Some nits about the test case.

pooja_sharma’s picture

pooja_sharma commented
Assigned: Unassigned » pooja_sharma
pooja_sharma’s picture

pooja_sharma commented
Assigned: pooja_sharma » Unassigned
Status: Needs work » Needs review

Addressed the feedback, rebased the MR, apart from it nothing seems to be left.

Please review, moving NR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback around test has been addressed.

quietone’s picture

quietone commented

I didn't find any unanswered questions here. Updated the credit, though it should be check by the committer.

I made suggestions to comments in the test and applied them. They are both minor and I double checked them, so I am leaving this at RTBC

  • catch committed 06914f5f on 10.3.x 
    Issue #3319601 by pooja_sharma, sunlix, bkosborne, quietone, smustgrave...

  • catch committed 5f4c47f3 on 10.4.x 
    Issue #3319601 by pooja_sharma, sunlix, bkosborne, quietone, smustgrave...

  • catch committed cc9bd24b on 11.0.x 
    Issue #3319601 by pooja_sharma, sunlix, bkosborne, quietone, smustgrave...

  • catch committed 22ee9bdb on 11.x 
    Issue #3319601 by pooja_sharma, sunlix, bkosborne, quietone, smustgrave...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch closed merge request !8548

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.