Media Thumbnail Formatter: alt and title null after upgrade D9

Created on 18 April 2023, about 1 year ago
Updated 25 October 2023, 8 months ago

Problem/Motivation

Images viewed with Media Thumbnail Formatter haven't alt and title attribute.
The problem occurs after update at Drupal 9.

Steps to reproduce

Steps to reproduce:

  1. Install Media module
  2. Add an Image media entity with any image file and specify alt text
  3. Edit the image media entity and update the alt text and save
  4. Use the Media Thumbnail Formatter
  5. Observe that the image haven't alt and title text

Control:

- Spin up a new Umami site (I used https://ddev.github.io/ddev-gitpod-launcher/)
- Observe the alt text of the borscht in the card on the homepage - "Traditional Ukrainian soup with beets, tomatoes, and pork ribs"
- Go to Content > Media > "borscht" and edit the "Borscht with pork ribs" image
- Change the alt text to "Yummy traditional Ukrainian soup with beets, tomatoes, and pork ribs"
- Save the image
- Return to the homepage and observe the alt has been properly updated.

Failure case:

- Go to Admin > Structure > Content Types > Recipe > Manage Display > Card common alt (/en/admin/structure/types/manage/recipe/display/card_common_alt)
- Change the Media Image Format from "Rendered entity" to "Thumbnail" (no other settings matter)
- Save
- Return to the homepage and observe the alt text has reverted to "Traditional Ukrainian soup with beets, tomatoes, and pork ribs"
- (optional) Try clearing caches, because that could be a thing. No change on cache clear.
- Apply patch, clear caches
- Observe alt text properly updates
- Change alt text on media again, observe it properly updates again

Proposed resolution

I created a path to solve the problem.
The patch gets the item attribute from the source field.

๐Ÿ› Bug report
Status

Needs work

Version

10.1 โœจ

Component
Mediaย  โ†’

Last updated about 17 hours ago

Created by

๐Ÿ‡ฎ๐Ÿ‡นItaly marco.aresu Cagliari

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Accessibility

    It affects the ability of people with disabilities or special needs (such as blindness or color-blindness) to use Drupal.

Sign in to follow issues

Comments & Activities

  • Issue created by @marco.aresu
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly marco.aresu Cagliari
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States aaronpinero

    This seems like a pretty basic and important accessibility problem. I just noticed it today when doing a WAVE report on my website. This issue was the first error reported. I thought it was a content author mistake. It's a surprising oversight for the Media system. Can we bump this up to critical?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States aaronpinero

    I tested the patch with Drupal 9.5.8 and it appears to solve the issue. I am seeing alt attribute information on a media image thumbnail where previously there was none.

  • Status changed to Needs work about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States cilefen
  • last update about 1 year ago
    Patch Failed to Apply
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States aaronpinero

    One thing I noted about the site after the patch: if the image is missing alt text, an empty alt attribute is added to the image. Not sure if that's a desired outcome... would it be better to leave off the alt attribute entirely if there's no text available?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States aaronpinero

    I am uploading a new patch because the current one doesn't apply cleanly through composer with Drupal 9.5.9. There was an update in 9.5.9 to the media thumbnail formatter that need to be accounted for. That said, the patch is VERY similar.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States froboy Chicago, IL

    if the image is missing alt text, an empty alt attribute is added to the image. Not sure if that's a desired outcome...

    @aaronpinero as per the Web Accessibility Initiative guidelines:

    Decorative images donโ€™t add information to the content of a page. For example, the information provided by the image might already be given using adjacent text, or the image might be included to make the website more visually attractive.

    In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers.

    I think it's the correct behavior to add an empty alt if none exists so that the images at least behave unobtrusively.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States froboy Chicago, IL

    I'm seeing this issue in 10. Bumping the version.

  • last update 12 months ago
    Custom Commands Failed
  • @froboy opened merge request.
  • last update 12 months ago
    29,444 pass
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States froboy Chicago, IL

    I've opened a MR for Drupal 10.1 and added the diff here for folks to use with composer. I'm not great at tests, but if this needs them and someone could direct me to the right place I can make an attempt at adding them.

  • Status changed to RTBC 12 months ago
  • last update 12 months ago
    Custom Commands Failed
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine podarok Ukraine

    Looks good

    Here is a diff, no need to upload patches @froboy

    https://git.drupalcode.org/project/drupal/-/merge_requests/4291.diff

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States froboy Chicago, IL

    @podarok I'm aware, but I didn't want to introduce potential security risks โ†’ . :)

  • last update 12 months ago
    Custom Commands Failed
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine podarok Ukraine
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine podarok Ukraine
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 12 months ago
    Custom Commands Failed
  • last update 11 months ago
    Custom Commands Failed
  • last update 11 months ago
    Custom Commands Failed
  • last update 11 months ago
    Custom Commands Failed
  • last update 11 months ago
    Custom Commands Failed
  • Status changed to Postponed: needs info 11 months ago
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    What are the specific steps needed to reproduce this? I tried to follow the steps from the issue summary and couldn't reproduce this

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Chris Matthews

    Like @lauriii, I also tried to follow the step in the IS to reproduce this but I could not. Might be missing something simple but this issue is postponed until lauriii or another committer can reproduce.

  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States froboy Chicago, IL

    I believe I've been able to reproduce the issue on a clean environment:

    Control:

    - Spin up a new Umami site (I used https://ddev.github.io/ddev-gitpod-launcher/)
    - Observe the alt text of the borscht in the card on the homepage - "Traditional Ukrainian soup with beets, tomatoes, and pork ribs"
    - Go to Content > Media > "borscht" and edit the "Borscht with pork ribs" image
    - Change the alt text to "Yummy traditional Ukrainian soup with beets, tomatoes, and pork ribs"
    - Save the image
    - Return to the homepage and observe the alt has properly updated.

    Failure case:

    - Go to Admin > Structure > Content Types > Recipe > Manage Display > Card common alt (/en/admin/structure/types/manage/recipe/display/card_common_alt)
    - Change the Media Image Format from "Rendered entity" to "Thumbail" (no other settings matter)
    - Save
    - Return to the homepage and observe the alt text has reverted to "Traditional Ukrainian soup with beets, tomatoes, and pork ribs"
    - (optional) Try clearing caches, because that could be a thing. No change on cache clear.
    - Apply patch, clear caches
    - Observe alt text properly updates
    - Change alt text on media again, observe it properly updates again

    I hope that helps. Thanks for your time.

  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Thanks for providing steps!

    This is NW though for the tests and upgrade paths.

  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine podarok Ukraine
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States aaronpinero

    Hi all. I would definitely like to move this issue along. It's been indicated that tests are needed to move this back to "needs review". I would like to help but I am a novice developer and the only tests I've ever written for a module were based heavily on provided examples of Unit and Kernel tests. I don't know a whole lot about selecting the appropriate tests and then writing them.

    That said, I did a bit of digging into the media module code. For tests related to the thumbnail formatter (where the proposed patch is applied) there is a Kernel test (core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php) and a Functional test (core/modules/media/tests/src/Functional/FieldFormatter/MediaThumbnailFormatterTest.php). The Kernel test seems to provide only a test to verify settings of the field formatter. The Functional test appears to actually test the rendering of the formatted media field as it would be used in a node.

    Based on what I'm reading in the code, the appropriate test addition for this change to the module would be to modify the Functional test that already exists to include some check of the rendered alt text vs the alt text stored in the reference media entity. Leaving aside (for the moment) the details of actually executing this change to the functional test, does this seem like a reasonable plan? Are there additional tests that would need to be included that I haven't considered?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mikegodin

    Here's the patch from #7, adjusted for Drupal 10.1.5

  • last update 8 months ago
    29,674 pass
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States neclimdul Houston, TX

    This has a long history and a bunch of other issues trying to address is in different ways. This seems to currently be the most active, so linking them up.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States neclimdul Houston, TX

    It looks like the current patch remove the thumbnail entirely if there's an image. I guess that works, but does it really solve the problem? Should we get rid of the thumbnail entirely for all cases? Is there a use case where the thumbnail is still needed but you wouldn't be able to manage the thumbnail title and alt properties?

Production build 0.69.0 2024