SystemBrandingBlock has identical anchor title and link content (image alt)

removing "a11y" tag since we aren't using it.

I'm in favour of removing the title attribute from the site logo link.

That duplication of title and link content (alt tag in this case) is recommended against. See WCAG H33,SE Journal.

Note that WCAG technique H33 doesn't actually warn against this duplication. However I'm aware of plenty of other articles which do recommend avoiding it. It's one of the things Drupal gets called out for in Tenon Research First Glimpse: The Best & Worst Of Content Management Systems.

a) those changes can be done to Classy or Stable themes?

We're not supposed to make changes to Classy or Stable unless it's an important bug. There have been a few a11y fixes in these themes before now, mind. Will this one be allowed? On the one hand, removal of a title attribute seems like a very low risk change: it doesn't affect any element names, classes, or IDs; and none of our CSS selectors involve this title attribute. On the other hand, this issue isn't a very serious one. We'll see what the front-end framework manager says (most likely they'd be the one who commits any change to Stable/Classy).

I'd proceed with a patch anyway. It's worth checking the Umami demo theme too.

b) if it would need a test - I could test that the raw html in each theme is being rendered without the title but that's about it.

I wouldn't bother with this. We'd be testing for the absence of something we don't generate... seems like a pointless thing to write a functional test for. This is the sort of thing that can be picked up by some a11y testing frameworks, hopefully one day we'll have one of those in Drupal's CI.

I updated the issue summary based on #4. I'll take a stab at it.

Adding a tag for the event.

Thank you for taking the time to update the issue summary, @annajl.

Here's a patch--I removed the title field.

Thank you for the patch, @annjl. I'm not sure why this got set to 7.x testing by default. I added a retest on 8.7.x PHP 5.6 MySQL 5.5 and PHP 7.1 MySQL 5.7. Setting back to Needs review.

The patch no longer contains the title attribute.

Patch looks good to me. Thanks @mradcliffe & @annajl.

We just need someone to test it and mark it RTBC.

Tagging for DrupalEurope. Please add screenshots and if possible use Orca, NVDA, JAWS and VoiceOver to test and update the issue summary with the rests of the test. I pinged @laurii on this the other day as this still needs a front-end framework manager review for changing markup.

This also needs to be fixed in demo_umami. I'm setting this back to Needs work because of that.

Ugh, crap. Fixing tag.

Hi,
here is a patch that adresses #13
Thanks

#15 patch looks good for fixing it in umami as well.

There are 5 SystemBrandingBlock templates in core, but so far the patches only change 2 of them.

I've updated the tasks list.

updated with better articles on the topic

hi,
here is a patch with the 5 files that need updates
Thanks

I tested this for all listed cases. Looks and works fine.

Thank you for the updated patches, @izus, and thank you for testing, @hiway.

I think we still need review per the Needs framework manager review tag so I'm setting this to Needs review. It would probably make sense to ping @laurii on Drupal Slack in #contribute if someone would like to nudge the issue.

Also it looks like the issue summary only mentions Classy and Stable. We should update the issue summary to reflect the patch and current state of the issue per comments #17, #19 and #20, and once that's done, remove the issue tag. In the issue summary we mention that Classy and Stable are "low-risk" changes. I think that @laurii can confirm that this is a low-risk change across the 5 templates that we're changing.

Is the change to Bartik a low-risk change?

I've removed issue credit for @kalyansamanta - they've contacted me and appear to be aware of their mistake.

The markup for the site brand logo should use this markup to be WCAG 2.0 compliant

<a href="{{ path('<front>') }}" title="{{ 'Go to the homepage of the site'|t }}" rel="home" class="site-logo">
  <img src="{{ site_logo }}" alt="" />
</a>

Here is an article on how JAWS, NVDA, and VoiceOver read image alt text and images wrapped in a link
https://www.oomphinc.com/notes/2019/01/images-alt-tags-out-loud-experien...

I'm working on this issue as part of the Global Contribution Weekend.

Updated patch with markup noted in #26.

@starshaped I tested your path and it's works fine for all cases.
Nut I think we also should create a follow up issues from this ticket about the approaches used to show the site name blocks in the header as for Unami theme, so for the Bartik, Classy, Stable and system branding blocks.

In case of Unami theme we are using this peace of code:

{% if site_logo %}
    <a href="{{ path('<front>') }}" title="{{ 'Go to the homepage of the site'|t }}" rel="home" class="site-logo">
      <img src="{{ site_logo }}" alt="" />
    </a>
  {% endif %}
  {% if site_name %}
    <div class="site-name visually-hidden">
      <a href="{{ path('<front>') }}" title="{{ 'Go to the homepage of the site'|t }}" rel="home">{{ site_name }}</a>
    </div>
  {% endif %}

This means, that screen reader will read almost the same link twice in case we have site logo and site name showed.

The same for Bartik, Classy, Stable and system branding blocks. We have a little bit different code there, but the same problem, if site name and site logo are both showed (in most cases this is default theme settings), the screen readers will pronouns almost the same links twice and just second after the first one. This is confusing UX behavior.

Thanks for working on this during the Contribution Weekend.

I think this issue took a wrong turn with #26. The markup corresponds with case 5c in the linked Oomph article, which makes a detailed comparison of the utterances of various screen readers. The author doesn't explicitly say why they consider this markup to be "most ideal". Comparing it with the other examples there, it seems they either think that it's undesirable to convey the fact that the link is an image, and/or they think it necessary to include a phrase like "Navigate to ..." in case the user doesn't understand the concept of a hyperlink? I don't understand their reasoning.

Whatever the intent of this markup, it's a case of over-optimizing for screen readers, at the expense of other scenarios. This markup has a problem for sighted users: when images fail to load for some reason, they won't know the purpose of the link because there is no text alternative for the image. Here are some screenshots from Bartik to show the problem:

1. Using patch #23 (or, before using any patch from this issue). When the image fails to load the alt attribute is displayed. The link purpose is conveyed ("Home").
   
2. Using patch #28. When the image fails to load there is just a broken image icon, but no text alternive. Sighted keyboard-only users, and sighted speech control users, will not know the purpose of the link even though they can operate it.
   

The approach in patch #23 is preferable, as it doesn't favour one group of users over another.

#29:

This means, that screen reader will read almost the same link twice in case we have site logo and site name showed [...] This is confusing UX behavior.

I agree, this isn't great. The problem you describe is addressed by WCAG technique H2: Combining adjacent image and text links for the same resource. I think it's worth having a follow-up for that, but it's out of scope here. It's a more ambitious markup improvement which needs to consider backwards-compatibility. It would need to be done in a way that avoids causing layout bugs for existing sites.

In the case of the Umami demo where the logo is an image-of-text, the double-link problem can be addressed after #2780293: Add GUI to configure the site's logo alt attribute is complete. Then Umami can use alt="Umami Food Magazine - homepage" and turn off the site name. Umami already has #3000724: Fix accessibility problems in Umami's branding block for this.

The patch from #23 is preferable, so reposting it here as the latest one.

This needs sign-off from the front-end framework manager for Stable/Classy backwards compatibility policy. Otherwise RTBC.

I forgot to re-post the patch. This is identical to #19 and #23, which is the preferred approach.

Is it still RTBC then? Who are the folks we need to talk to with Stable & Classy?

Sorry for the delay on my side, and thank you everyone for your work on this issue.

Committed f4420c5 and pushed to 8.8.x. Thanks!

Leaving open for backport to 8.7.x once the code freeze is over.

Removing Novice tag from this as there isn't anything more to do until 8.7.0 is released.

Changing to path to be ported as that's the correct status. Hopefully will go in in the next couple of days.

Is this still eligible for 8.7.x? It's been tagged for backport for a while now.

Discussed with @catch - we're done with 8.7.x bugfixes - this is going in 8.8.0.