Do not override the users alt texts

carolinan commented 5 years ago

The theme must respect the users settings.

If the user has added an alt text to their featured images, (or intentionally left it blank) then the theme must show the correct text, not the title attribute.

samikeijonen commented 5 years ago

Is this issue somewhere in _s codebase, or general note?

carolinan commented 5 years ago

It is in template-tags.php. (I tried to add the code but I'm on mobile at the moment and it just got all messed up. )

carolinan commented 5 years ago

Here: https://github.com/Automattic/_s/blob/master/inc/template-tags.php#L137 <?php the_post_thumbnail( 'post-thumbnail', array( 'alt' => the_title_attribute( array( 'echo' => false, ) ), ) ); ?>

samikeijonen commented 5 years ago

The <a> element is already hidden from AT using aria-hidden="true" to avoid extra noise. But would be nice to test how img alt inside it works, can it be accessed somehow.

If the link wasn't hidden, I'd still rather use the_title because in this case it works as a link text.

carolinan commented 5 years ago

But a link text alternative could also easilly be accomplished without overriding any user settings.

grappler commented 5 years ago

I agree the title does not always describe the featured image correctly.

samikeijonen commented 5 years ago

It doesn't but if it is used as a link text it's not about image itself anymore.

crunnells commented 5 years ago

I'd like to put in a check to see if the image has an alt tag, and if not use the title as a fallback (which I think is the spirit of the original code). The other option would be to just use alt and title fields, regardless if they're blank or not. I'd rather just update the code so it uses the title as a fallback, but figured it's worth discussing other options.

anthonyhartnell commented 5 years ago

Hello, I would like to contribute and make my first pull request on this project. I've cloned the repo and have it set up locally and can see where to edit this code. I agree with @carolinan that the title should either be what is set in the media library or blank as the title is not necessarily helpful. On the Wordpress sample page, for example, the alt text is "Hello world" which is not descriptive to the image I have used.

If anyone could advise me on the steps for how to do a pull request that would be really useful as I've never contributed to an existing project before. Thank you.

grappler commented 5 years ago

@WebAssembler It would be nice if you could create a PR.

The steps that are involved are.

Create a fork of this repo
Create a new branch In this case you could name it something like 1328-thumbnail-alt
Create a PR

I hope this helps.

samikeijonen commented 5 years ago

Note that I don't 100% agree using alt text from accessibility point of view. Current code looks like this:

<a class="post-thumbnail" href="<?php the_permalink(); ?>" aria-hidden="true" tabindex="-1">
    <?php
    the_post_thumbnail( 'post-thumbnail', array(
        'alt' => the_title_attribute( array(
            'echo' => false,
        ) ),
        ) );
    ?>
</a>

aria-hidden="true" removes link element and all of its descendants, also image, from the accessibility tree.
tabindex="-1" remove link from tab order.

This was done to avoid extra noise to assistive technology (AT) users since article title already provide the same information.

With that said even the alt is changed in current code, image is still skipped from AT users. Image is purely decorative.

As a summary, I'm OK using image alt text in this case.

What if we remove `aria-hidden="true"` and `tabindex="-1"`?

Rendered markup would look something like this:

<a class="post-thumbnail" href="https://example.com/test-article">
   <img src="image-url" alt="Article title or image alt text">
</a>

In this case alt text is going to be link text. And for link text article title makes more sense from accessibility point of view.

Automattic / _s