Alt tag missing on user images

Is alt required by the HTML5 spec? I'd recommend:

1. Having it generate one from the src if it doesn't exist
2. ... and passing that default one for user pictures

I believe you can do this here: /admin/config/people/accounts/fields/user.user.user_picture?bundle=user

Set the alt text to required.

This is also an a11y fail in Drupal 7 core.

Not sure if this is the best way, but we definately need a default alt tag on profile pictures. Thanks Mike!

@mgifford - Mike, three issues I'm finding with your solution (and I appreciate it's not a finished product, but a work in progress):

1) It enables the alt field which allows people to edit it - we could hide it with js or css, I guess
2) For some reason it's only showing the alt text on the frontend (as in, the Bartik theme) not on the backend (as in, the Seven theme)
3) The 'name' variable is not being passed in - the alt tag's default is blank when I tried this approach.

So, looks like we need to keep alt_field: false and come up with some way of dynamically injecting the info in there.

Back to the drawing thinking board for me.

Hi Mike,

I didn't apply either patch, I just edited the necessary file to check, but you're right, we need to edit more than that.

I can't make sense of the patch in #11. I don't think it should belong in this thread.

Looks like patch #11 was posted in the wrong thread.

I'm not sure that we should use the alt field for this because we should pass the alt text through translation. Another issue is that the user's photo field can be named anything since it is done through the field api, so setting it through a form alter won't be reliable. If we do only target the "user_picture" field maybe we could to it when the field is loaded?

Targeting the user_picture field seems fine, the goal is to provide proper behaviour out of the box. If a site builder is not using the existing, default, user_picture field then it is up to that site builder to ensure the behaviour of the used custom field.

I'm at #DrupalNorth sprint, I'll attempt a patch to alter the alt value for user_picture in a hook_user_load() implementation and ensure a non-empty value.

The attached patch ensure there is always an alt text for the user_picture field in an implementation of hook_ENTITY_TYPE_view_alter() for user entities.

Hi pbuyle,

I haven't applied your patch yet, but looks very good. And Crassx, well done to you for coming up with this as a solution.

added documentation to the function so there is no confusion in the future about why it exists.

+++ b/core/modules/user/user.module
@@ -378,6 +378,22 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
+  if (user_picture_enabled() && !empty($build['user_picture'])) {

Just wondering is a call to user_picture_enabled() is needed here. Wouldn't a check on empty($build['user_picture]) suffice?
Other than that: great fix.

++

I think we should keep user_picture_enabled so that that logic is centralized. Interestingly that function is only used in one other place.
We should add tests for this so that if someone decides to change the "User_picture" field name the alt text is not lost.

Removing old hashed tag.

+++ b/core/modules/user/user.module
@@ -378,6 +378,22 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
 /**
+ * Implements hook_ENTITY_TYPE_view_alter() for user entities.
+ * This function adds a default alt tag to the user_picture field to maintain
+ * accessibility. See issue 2358319 for more details.
+ */

Missing empty line after the first sentence.

I also wouldn't put in the issue reference and certainly not like this, just leave that sentence out.

We just added something like this to a custom theme, and we did it in preprocess_image_formatter(), there it can be changed directly on the resulting render array instead of indirectly through changing the data.

This could actually end up being saved in case someone views a user and then saves it.

Fixing for comments in #37 and added a couple of asserts in an existing test.

+++ b/core/modules/user/src/Tests/UserPictureTest.php
@@ -100,6 +100,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw(t('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]));

@@ -112,6 +113,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw(t('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]));

Why not have this check alt="Profile picture […]"? Then it'd be actually checking if it was in an alt attribute.

Fair point. Changing the test.

+++ b/core/modules/user/src/Tests/UserPictureTest.php
@@ -100,6 +100,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw(t('alt="Profile picture for user @username"', ['@username' => $this->webUser->getUsername()]));

@@ -112,6 +113,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw(t('alt="Profile picture for user @username"', ['@username' => $this->webUser->getUsername()]));

alt is not part of the translated text

Second time's the charm. :)

Yeah, in fact, we can just drop the t() call altogether, because this is a test, so it's never going to be translated.

Well done folks. Looks very good. One step closer to Drupal 8

+++ b/core/modules/user/src/Tests/UserPictureTest.php
@@ -100,6 +100,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw('alt="' . t('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]) . '"');

@@ -112,6 +113,7 @@ function testPictureOnNodeComment() {
+    $this->assertRaw('alt="' . t('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]) . '"');

This is not actually testing the alt text on the user image - it is testing any alt on the page. Seems brittle. I think we should be using xpath - look at Drupal\image\Tests\ImageThemeFunctionTest for examples.

@alexpott: That's true. However, all the code in that test is in the same fashion. It checks for the file URI's in the response using assertRaw, which is just as brittle. If we were to do the check with xpath here, we should change the test to use it throughout. Does that make sense as a followup or should we do it in this issue itself?

Taking a look at this. I don't think we should balloon the scope to change the whole test (that could be a follow-up task), but let's make sure what we add is solid.

This is more specific.

I think the test looks a lot better now. Just 1 nitpick.

+++ b/core/modules/user/src/Tests/UserPictureTest.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\SafeMarkup;

@@ -100,6 +101,9 @@ function testPictureOnNodeComment() {
+    $alt_text = SafeMarkup::format('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]);

@@ -112,6 +116,9 @@ function testPictureOnNodeComment() {
+    $alt_text = SafeMarkup::format('Profile picture for user @username', ['@username' => $this->webUser->getUsername()]);

We should avoid using SafeMarkup::format() in tests now. We can just replace it by a concatenated string #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that API

+++ b/core/modules/user/src/Tests/UserPictureTest.php
@@ -100,6 +101,9 @@ function testPictureOnNodeComment() {
+    $this->assertEqual(count($elements), 1, 'Found alt text for user image on user account page.');

Also shouldn't this say Found alt text for user image on node page.

Good points, thanks @geertvd! I will also try to update the img src checking to be more specific. The 'user account page' assertion text was based on this code comment but I'll check it closer:

// Verify that the image is displayed on the user account page.

This checks that the image is in the correct spot with the correct image style and the correct alt text, and the assertion messages should be more accurate.

This looks good, will RTBC when it's green.

This broke because #2214241: Field default markup - removing the divitis I still consider this RTBC

These are permission denied errors. It seems random. Tentatively setting back to RTBC as per #60.

The test failures of EntityFileTest are the same here as they are in in #2553661: KernelTestBase fails to set up FileCache, so I don't think these are related.

Is it possible that HEAD is broken?

The last couple tests were on bot 3128 which according to #2429617-352: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) was broken and has now been taken out of rotation. Going to hit re-test.

back to rtbc

Committed fc8a559 and pushed to 8.0.x. Thanks!

I was starting to work on a D7 backport for this, but when I added a profile picture it already has alt text (see screenshot).

In the file template_preprocess_user_picture the lines: 1512 to 1517:

$alt = t("@user's picture", array('@user' => format_username($account)));
      // If the image does not have a valid Drupal scheme (for eg. HTTP),
      // don't load image styles.
      if (module_exists('image') && file_valid_uri($filepath) && $style = variable_get('user_picture_style', '')) {
        $variables['user_picture'] = theme('image_style', array('style_name' => $style, 'path' => $filepath, 'alt' => $alt, 'title' => $alt));
      }

So, I think no action is required for D7.

thanks,
~Geoff

Cool, thanks @serundeputy!