Problem/Motivation

Currently the image-to-iframe media replacement functionality is not accessible.

Steps to reproduce

Create a media field. Allow oembed / remote-video media. Set Display Blazy Media Replacement settings to "Image to Iframe".

Proposed resolution

  • Make current play and close icons buttons.
  • Add title and aria-label to play and close buttons to explain what they do.
  • Add a default alt tag and title to the placeholder image to explain it is a preview for the video.
  • Add aria-live="polite" to the container to notify the user that the iframe is being loaded and closed.
  • Potentially add a data-title to the buttons and then use that to add a title to the generated iframe to give the user context.

Remaining tasks

  • Make current play and close icons buttons.
  • Add title and aria-label to play and close buttons to explain what they do.
  • Add a default alt tag and title to the placeholder image to explain it is a preview for the video.
  • Add aria-live="polite" to the container to notify the user that the iframe is being loaded and closed.
  • Potentially add a data-iframe-title to the buttons and then use that to add a title to the generated iframe to give the user context.

Data model changes

Pass title from media into settings to give context to the blazy item.

CommentFileSizeAuthor
#12 image_to_iframe_alt-3210635-12.patch4.7 KBjastraat
#7 example.jpg86.95 KBAnonymous (not verified)
#2 3210635-02.patch4.32 KBgrathbone

Comments

grathbone created an issue. See original summary.

grathbone’s picture

grathbone commented
Status: Active » Needs review
StatusFileSize
new 3210635-02.patch4.32 KB

Patch attached

gausarts’s picture

gausarts commented

Thank you.

I haven't been able to review it properly. Just 2 minors:

+ $attributes['alt'] = t('Preview image for the video "@label" – @alt.', $translation_replacements);
I am on cell now, what is the characters before @alt :)

To avoid undefined index, consider adding title here:
https://git.drupalcode.org/project/blazy/-/blob/8.x-2.x/src/BlazyDefault...
So far, title was only for Views settings.

gausarts’s picture

gausarts commented
Status: Needs review » Needs work

Additional reviews:

    $settings['title']        = $media->label();

This overrode title as a form option which points to a field name with a text.

We need to rename title here to something else, says accessible_title and adjust the rest accordingly since title is already preserved for title field name at Media entities or Views option, not the actual text.

I think that is the only problem.

Minor update:
The t() functions at PHP classes need changing into FormattableMarkup to pass CS.

Hopefully that will be all for now.

Thanks.

Anonymous’s picture

Anonymous (not verified) commented

Same issue for Blazy to Colorbox using oEmbed Videos.

The thumbnails do not include an alt tag or Aria label.

Anonymous’s picture

Anonymous (not verified) commented

Is it possible to use field mapping for oEmbed. I.e. Allow the site builder to use the media title field or other field a builder would add to the media type?

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new example.jpg86.95 KB
Anonymous’s picture

Anonymous (not verified) commented

Another option would to use an aria-label which can utilize a media field to get the label text.

Anonymous’s picture

Anonymous (not verified) commented

Added the patch to a Drupal 9 dev site it works with blazy image to colorbox.

gausarts’s picture

gausarts commented

@Bwolf , thanks for additional reviews.

> Another option would to use an aria-label which can utilize a media field to get the label text.
Noted, thanks.

> Added the patch to a Drupal 9 dev site it works with blazy image to colorbox.
Unfortunately, this patch overrode settings.title which points to a field name as a form option, not as its value, nor other meaningful texts. See #4.

We'll postpone this till the next release till we have time to fix it.

Feel free to update your patch as per #4 and #8. Thanks.

Anonymous’s picture

Anonymous (not verified) commented

Thanks @gausarts!

I look forward to the patch/update. :)

As soon at the module is updated with this functionality I can move from VEF to oEmbed video on all my projects.

jastraat’s picture

jastraat commented
Status: Needs work » Needs review
StatusFileSize
new image_to_iframe_alt-3210635-12.patch4.7 KB

Updated patch to address gausarts' concerns with the title setting being overridden. (Does not use aria-label.)

gausarts’s picture

gausarts commented

Looks good, thank you.

Two nit issues, aside from CS:

  • Button styling, we can add a small CSS to remove background to avoid breaking changes with the previous styling.
  • accessible_title is not a valid IMG attribute, it should be just regular IMG title. I understand my comment at #4 might confuse you about $settings['title'] vs. $settings['accessible_title'] which was translated into $attributes['accessible_title'] while should be $attributes['title'] .

We should have provided a cache clear hook_post_update since it modified twig, but not crucial, manual clearing will do. Aside there was already a hook_post_update for the new native grid which should take care of this as well.

  • gausarts committed 678c3a0 on 8.x-2.x authored by jastraat 
    Issue #3210635 by grathbone, jastraat, Bwolf: Lazyloaded image-to-...
gausarts’s picture

gausarts commented
Status: Needs review » Fixed

Committed with minor fixes for the above-mentioned issues.

Also removed unused data-iframe-title from close button. Only needed for play button.

If any side issues or additional improvements to follow up, feel free to create a new thread.

Thank you all for kind contributions.

Status: Fixed » Closed (fixed)

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