getgrav / grav

Modern, Crazy Fast, Ridiculously Easy and Amazingly Powerful Flat-File CMS powered by PHP, Markdown, Twig, and Symfony
https://getgrav.org
MIT License
14.48k stars 1.4k forks source link

Always add an alt attribute to images for valid markup #1253

Closed indigoxela closed 7 years ago

indigoxela commented 7 years ago

Hi, an alt attribute is required for valid image html markup.

It would be cool, if public function parsedownElement would always add it, even if not set by users in markdown.

Instead of: <img src="some.jpg" /> (invalid) insert <img alt="" src="some.jpg" /> (valid)

To me it looks like a tiny change in system/src/Grav/Common/Page/Medium/Medium.php:

236a237,239
>             else {
>                 $attributes['alt'] = '';
>             }

This also outputs an empty alt attribute for any images handled with methods like cropZoom. Right now alt attributes are lost for those images, even if set by users in markdown for the original image (in a blog article...).

Or is there a good reason to skip alt attributes if empty and drop them if image is changed?

flaviocopes commented 7 years ago

Adding an empty alt tag is not a bad idea I think, while an empty alt tag is not really useful per se, it helps passing HTML validation for those that are interested in it.

Alt attribues are preserved when processing images:

![ALT test](test.jpg?cropZoom=100,100))

⬇️

<img alt="ALT test" src="/grav-develop/images/9/3/4/5/9/93459bbe7f7babd39d7f46e164bc470825e32cce-test.jpeg">

Maybe you are testing with cache enabled and don't see your changes reflected.

indigoxela commented 7 years ago

Alt attribues are preserved when processing images:

I must be more precise: Yes, if adding images directly in markdown, alt attributes are preserved.

But if for instance for an overview page of a collection, all the child page images are processed in a template loop, all alt attributes are lost.

As an example: I have a template "blog_item.html.twig". In a loop I get the first image of all blog item pages to render them as small linked images:

    <a href="{{ page.url }}">
    {{ page.media.images|first.cropZoom(200, 200).html }}
    </a>

In the resulting page listing, all alt attributes are gone.

Btw.: alt attributes are also gone in search result pages (plugin simplesearch).

flaviocopes commented 7 years ago

Ok in this case you're iterating over page.media which is an array of the media included in the page. Is the alt text provided in an image meta file? https://learn.getgrav.org/content/media#metafiles or you just defined it inside the page?

indigoxela commented 7 years ago

Is the alt text provided in an image meta file?

The alt text is not provided in a meta file but directly in markdown: ![the image alt](image.jpg)

By the way - many thanks for having a look.

flaviocopes commented 7 years ago

Ok then if you're iterating over page.media in a Twig file, Grav has no idea that you set the alt tag of an image in the page markdown. It cannot look up the page MD, search where the image is used, and "scrap" the alt tag used. What if you had the image 2 times with different alt tags, for example?

It needs to have the alt tag in the metafile to work.

indigoxela commented 7 years ago

It needs to have the alt tag in the metafile to work.

I see. Adding an extra yaml file for every image seems a bit laborious to me. I can live with an empty alt tag in that case.

Are automatic empty alt tags still something you consider to be useful? (markup validation)

flaviocopes commented 7 years ago

I think so, let's see if others step into the discussion

rhukster commented 7 years ago

I think we probably should add the empty alt tag.

BTW, the html() method takes some values, including the alt tag.

 public function html($title = null, $alt = null, $class = null, $id = null, $reset = true)
indigoxela commented 7 years ago

BTW, the html() method takes some values, including the alt tag.

Thanks! I'm rather new to Grav/Twig and appreciate every useful hint.

A quick idea to improve accessibility:

    <a href="{{ page.url }}">
    {{ page.media.images|first.cropZoom(200, 200).html('', 'Read more about ' ~ page.title) }}
    </a>
hughbris commented 7 years ago

A quick idea to improve accessibility: {{ page.media.images|first.cropZoom(200, 200).html('', 'Read more about ' ~ page.title) }}

Sorry, that's a zero improvement to accessibility. That alt text does not the describe the image but the link that is wrapping it, and rather redundantly at that. What benefit does that provide a user or other agent that can't see the image themselves?

indigoxela commented 7 years ago

What benefit does that provide a user or other agent that can't see the image themselves?

It was just an idea an a little off-topic here anyway. At least this would describe, where the link leads to. The link has no text otherwise and without alt a screen reader would probably say "image" or the image filename, which is not helpful at all. As far as I know, screen readers ignore images with alt="".

flaviocopes commented 7 years ago

While I'm not an expert of the topic, I've also read that screen readers ignore images with alt="" so I guess it's not really a good default to have for images that do not specify an alt tag.

Best would be you (site creator) adding the alt information to all the page images, or you risk screen readers to skip all images and you don't even realize it.

indigoxela commented 7 years ago

Hi, well, web accessibility is a rather complex topic. I'm no expert too and it might be beyond the scope of this issue anyway.

I think that it's OK, if images that do not contain important information are ignored (alt=""). It would rather be annoying for users to present a pointless info like an image filename or the word "image" in this case (when there's no alt at all).

A different issue are images that are navigation elements (image links), like in my example on the page collection overview page. There both - the missing alt attribute and the empty alt attribute are a problem. But also the original image's alt attribute wouldn't be the best choice.

If you or someone else would like to discuss this topic further, I'd suggest to do this in a forum. Just post a link here.

A---- commented 7 years ago

But there's is no way to put an empty alt attribute at the moment. In the Markdown, if you write ![](...), it'll be omitted. Same thing if you specifiy .html('title', '').

I'll PR what I use as a fix.

rhukster commented 7 years ago

This has been added.