Add GUI to configure the site's logo alt attribute

That sounds more like a theme system issue

This could go in to 8.x, could you provide a patch @Benia?

An issue for this has already been discussed and it was decided to provide documentation rather than a patch. See here: https://www.drupal.org/node/717708

Documentation page here: https://www.drupal.org/docs/7/theming/howto/how-to-edit-alt-tag-on-your-... (though it's for Drupal 7 at the moment)

Adding new tag to this issue. Let's get a fresh opinion on this, since the linked issue is over 4 years old.

Andrew? Mike?

Don't need to know SQL, so you are half way. Anyways here's a start, may need some UX review on the labels and we need some tests but it looks to work for me:)

@markconroy I think we can get this in core, sorry cross posted with you.

Some screenshots so we can review the text labels would be good.

I'll test it and add some screenshots for the issue.

The patch worked fine for me and adds the mentioned fields to the logo settings form of all the themes.

Attached the screenshots:

Custom image/ Default Image

Output in theme

SteffenR

+++ b/core/themes/bartik/templates/block--system-branding-block.html.twig
@@ -11,20 +11,22 @@
+    <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home" class="site-branding__logo">
+      <img src="{{ site_logo }}" alt="{{ site_logo_alt }}" />
...
+          <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home">{{ site_name }}</a>

Could we use the |default('Home'|t) here so that we don't break this for existing sites? I believe this change could be done also to the Stable and Classy templates since this is a non-breaking change!

Thanks for the screenshots. The labels look good. Maybe have a look at the options for an image field, I suspect there's descriptions there that show what an alt, what a title attribute is for.

@lauriii, we can add |default() twig filter but it shouldn't break existing sites because I added that text in the default configuration settings.

And oh yeah it could go into Stable/Classy, good thinking

I've addressed #17 with the #description from core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php:227 and the #maxlength

@joelpittet the default configuration gets only imported on install so existing installations are affected if we don't use the default filter. Not sure if there should be update hook instead to set the default values.

Oh that makes sense, thanks for explaining that @lauriii. I did it to all of them.

+++ b/core/modules/system/system.module
@@ -831,6 +831,14 @@ function system_preprocess_block(&$variables) {
+        $variables['site_logo_alt'] = $variables['content']['site_logo_alt']['#markup'];
...
+        $variables['site_logo_title'] = $variables['content']['site_logo_title']['#markup'];

On this conversion we lose the #access from the render array. We should instead copy the whole render array. I'm not sure if we need to do this at all and could just access the variable inside content?

There's still some needs tags that needs to be also addressed.

+++ b/core/includes/theme.inc
@@ -476,6 +476,12 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
+    elseif ($key == 'alt') {

Is there a reason this isn't a switch statement?

Re #24 My guess is that the last two don't really qualify for a normal switch statement, though I could be wrong. Attached how it'd look as a switch statement.. not too pretty either.

Reroll, #23 needs to be addressed still.

I've just looked at #23, and I don't think it's relevant for this issue, as it '#access' is lost to all other site variables as well. Fixing that to pass the whole render array should be a new issue. But indeed, it needs tests.

I have reviewed and tested the patch for different use cases (adding only alt text, adding only title, adding both, also adding custom logo...), it worked all good.

The issue summary should be update to include the status of the issue with regard to what has been done so far and what needs to be done.

- To address a11y review tag the a11y gate should be helpful: https://www.drupal.org/core/gates#accessibility
- The Needs tests tag still seems to apply with the latest patch so that should include testing the new markup in a browser test.

@jcnventura, could you go into more depth about why #23 does not apply?

Patch in #27 worked for me. Looking further into #23 and how the issue summary should be updated based on what's been done.

I've applied the patch from #27 and tested with WAVE.

Updated the issue summary. Looks like for the implementation part we need to decide whether it's okay #access is not part of the render array for $variables['site_logo_alt'] and $variables['site_logo_title'].

@jcnventura argues that other defined variables like $variables['site_name'] don't have #access, and so $variables['site_logo_alt'] and $variables['site_logo_title'] don't need it.

Also a browser test and accessibility review are still needed.

@mradcliffe: What do you mean by testing the new markup in a browser test?

Had a chat with @mradcliffe at DrupalCon Baltimore about what he meant when he said we needed a browser test. He means we need a BrowserTestBase web test as described here: https://www.drupal.org/node/2735005. I updated the issue summary to clarify this in the remaining tasks section.

A WebTest exists for the branding block here: https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Tests%...

There's a testThemeSettings() function in there which tests the theme settings which seems like a good place to test the title and alt attributes are actually showing up. Should we add our tests here to testThemeSettings()?

I believe the WebTestBase is now deprecated, and we're supposed to be using BrowserTestBase, although converting the test from WebTestBase to BrowserTestBase may be outside of the scope of this issue.

From my discussion with the mentors it sounds like we should be adding our test to the WebTestBase that exists, and leave upgrading the test for another issue. I'm going to take a stab at updating the test to check for the title and alt attribute.

I've taken a crack at testing the alt attribute and title in the logo settings form in testThemeSettingsLogo().

I think we need to add some additional steps inside of testThemeSettings() to test the title and alt attributes actually show up in the markup as has been done for the img src.

Just catching up with this now. As @markconroy said, this feature request previously came up during the d7 cycle, in #717708: Document how to set logo in theme with proper alt tags. I took a look back over that issue to see why we decided not to implement it. (Thanks for tagging this @mark.)

As I recall, we got stuck on the issue of what should happen if a site builder uploaded a custom logo image, but did not specify a custom text alternative. Or vice-versa, kept the default image but customized the text alternative. The decision to won't-fix happened at the Montreal Drupal a11y sprint in 2012.

With hindsight, I think we were worrying too much about that. Nowadays, I'd rather we simply trust the site builder to use the settings responsibly. It's not as if the concept of a simple image link and text alternative are a Drupal-specific thing.

The previous issue was filed by an a11y maintainer at the time, and I was keen on it too. Now, I'm one of the a11y maintainers and I'd like to see this go ahead. Removing the "needs accessibility review tag".

Good to see that @lauriii thinks this is a non-breaking change for Stable/Classy (comment #17). If it wasn't an acceptable change for stable/classy, that would have made things much more difficult I think (because would have ended up with a feature which wouldn't work out of the box for themes which relied on Stable or Classy as a base). The use of |default() looks like a good safeguard.

I tried the patch from #40 and it seems to work well, but I haven't looked at the tests carefully.

Nice work! Really glad to see this.

This text will be used by screen readers, search engines, or when the image cannot be loaded.

I don't like this mention of search engines in the help text. While it's certainly true that search engines like Google appear to make use of image text alternatives, they are not the intended audience. It's quite common to find images where an author has used the ALT attribute for SEO keyword stuffing, and it's very unhelpful for any of the intended scenarios. I don't want to encourage that in Drupal's UI.

Cool, thanks for the detailed suggestions @andrewmacpherson. I'm updating the description to include what's needed at this point including your suggestion to remove search engines from the help text.

Changing the help text and expanding the tests to check that the alt and title attributes are properly rendered (in bartik).

Right on @Manuel Garcia, thanks for updating the wording and adding that test!

I applied your patch to 8.4.x and the text appears updated as shown in the attached screenshot.
https://www.drupal.org/files/issues/logo-settings-new-text.png

Also for both the default logo and a custom logo, the title and alt attributes are showing up in the markup. See screenshots attached.
https://www.drupal.org/files/issues/bartik-default.png
https://www.drupal.org/files/issues/bartik-custom-logo.png

I believe we're really close, and need a review from someone with more experience with tests. My review is a thumbs up, if the tests look good and there are no other issues found I believe this is good to go!

Updating work needed in description to reflect we need further review of tests.

Applied patch from #45 and it worked as expected. Attached the reference screenshot. New title and the title update worked fine.

in #46, jamesdixon says:

I believe we're really close, and need a review from someone with more experience with tests.

We're still waiting on feedback about the tests, so setting this back to needs-review.

I've had a look at the added tests and they all look OK.

As we've added some variables available to themers, a change record should be drafted.

There's also a "Needs issue summary update" tag. I see @jamesdixon updated it last in comment #35. Does the summary need updating further or does the tag need to be removed?.

Once they're done I think it'll be RTBC.

Thanks @John Cook!
Added the CR: https://www.drupal.org/node/2878236

Further to what I said in #43 about the search engines. It turns out that image fields in general have the same problem, encouraging SEO abuse. I'll file a separate issue about that.

The labels for the alt and title attributes differ to those on image field widgets.

In patch #45 here they are called "Logo alt attribute" and "Logo title attribute". On imagefield widgets they are called "Alternative text" and "Title".

Can we update these for consistency with imagefield widgets please? I think we can still keep the word "logo" in there.

The patch in #45 changes ImageWidget.php in Image module. That's probably out of scope for this issue, which is otherwise just about the theme settings form and branding block template.

Can we remove this, and deal with it in a separate issue?

diff --git a/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
index 735cfce..e1c41da 100644
--- a/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
+++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
@@ -224,7 +224,7 @@ public static function process($element, FormStateInterface $form_state, $form)
       '#title' => t('Alternative text'),
       '#type' => 'textfield',
       '#default_value' => isset($item['alt']) ? $item['alt'] : '',
-      '#description' => t('This text will be used by screen readers, search engines, or when the image cannot be loaded.'),
+      '#description' => t('This text will be used by screen readers, or when the image cannot be loaded.'),
       // @see https://www.drupal.org/node/465106#alt-text
       '#maxlength' => 512,

Removed the Change in ImageWidget.php as in #54

Thanks for the review, and good catch @andrewmacpherson.
Interdiff in #55 looks good, thank you @pritish.kumar

Addressing #53 on this one, and updading the CR to reflect the new theme settings field labels:
https://www.drupal.org/node/2878236/revisions/view/10483116/10483415

I opened #2878238: Image Field Alternative text description should not encourage SEO abuse to deal with the help text on FieldAPI image fields.

Tests have been given the OK, CR has been added, and the minor changes requested on #53 and #54 have been addressed. Back to RTBC.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -207,6 +207,12 @@ theme_settings:
   +          type: string
   ...
   +          type: string
   

   Should be type label so they can be translated.

2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -156,10 +156,20 @@ public function build() {
   +      '#alt' => theme_get_setting('logo.alt') ?: $this->t('Home'),
   +      '#title' => theme_get_setting('logo.title') ?: '',
   

   You don't need the ternary here - the config will always default to Home.

3. +++ b/core/modules/system/templates/block--system-branding-block.html.twig
   @@ -11,18 +11,20 @@
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt|default('Home'|t) }}" />
   ...
   -    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">{{ site_name }}</a>
   
   +++ b/core/themes/bartik/templates/block--system-branding-block.html.twig
   @@ -11,20 +11,22 @@
   -    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-branding__logo">
   -      <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" />
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home" class="site-branding__logo">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt|default('Home'|t) }}" />
   ...
   -          <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
   +          <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">{{ site_name }}</a>
   
   +++ b/core/themes/classy/templates/block/block--system-branding-block.html.twig
   @@ -15,13 +15,13 @@
   -    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
   -      <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" />
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home" class="site-logo">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt|default('Home'|t) }}" />
   ...
   -      <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
   +      <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">{{ site_name }}</a>
   
   +++ b/core/themes/stable/templates/block/block--system-branding-block.html.twig
   @@ -15,12 +15,12 @@
   -    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">
   -      <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" />
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt|default('Home'|t) }}" />
   ...
   -    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title|default('Home'|t) }}" rel="home">{{ site_name }}</a>
   

   Same here we can have less logic. No need for all the default handling.

4. +++ b/core/modules/system/config/install/system.theme.global.yml
   @@ -12,3 +12,5 @@ logo:
   +  alt: 'Home'
   +  title: 'Home'
   

   We need an update path to set this on all existing installations. And also I think we need to care about the theme settings for all installed themes too.

Thank you @alexpott for the review, spending sometime on this today.

Addressing #59 and adding upgrade path for the global theme settings as well as all installed themes.

@Manuel Garcia thanks for the new patch - The update path is going to need a test.

1. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -212,7 +212,22 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
   +      $form['logo']['alt'] = [
   +        '#type' => 'textfield',
   +        '#title' => $this->t('Logo alternative text'),
   +        '#description' => $this->t('This text will be used by screen readers, or when the image cannot be loaded.'),
   +        // @see https://www.drupal.org/node/465106#alt-text
   +        '#maxlength' => 512,
   +        '#default_value' => theme_get_setting('logo.alt', $theme),
   +      ];
   +      $form['logo']['title'] = [
   +        '#type' => 'textfield',
   +        '#title' => $this->t('Logo title'),
   +        '#description' => $this->t('The title is used as a tool tip when the user hovers the mouse over the image.'),
   +        '#maxlength' => 1024,
   +        '#default_value' => theme_get_setting('logo.title', $theme),
          ];
   

   What should we do about requiring these fields - \Drupal\image\Plugin\Field\FieldWidget\ImageWidget::process() has some complex logic for this.

2. +++ b/core/themes/classy/templates/block/block--system-branding-block.html.twig
   @@ -15,13 +15,13 @@
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home" class="site-logo">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt }}" />
   ...
   +      <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home">{{ site_name }}</a>
   
   +++ b/core/themes/stable/templates/block/block--system-branding-block.html.twig
   @@ -15,12 +15,12 @@
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home">
   +      <img src="{{ site_logo }}" alt="{{ site_logo_alt }}" />
   ...
   +    <a href="{{ path('<front>') }}" title="{{ site_logo_title }}" rel="home">{{ site_name }}</a>
   

   New variables to document.

• Added test for the upgrade path (my first!)
• Re #62.2 - Added documentation for the new variables to classy and stable (which was also missing it). Thanks @alexpott for your good eyes... I've also checked the other two templates, they all have these now.

As for #62.1, ImageWidget uses '#alt_field_required' for this, though I'm not sure I follow what you mean here, this is not a field, just plain old form? Should just make the logo alt and title required?

Also, I've been scratching my head for a while, but I don't see why we've got failing tests from UpdatePathTestBase::runUpdates(), it is complaining that stable.settings and classy.settings have no schema. However the configuration does save properly... can anyone help with this one?

@Manuel, I found this dealing with schemea problem: https://www.drupal.org/node/2654810. Maybe it will be of help here.

Thanks @John Cook, I've spent a good hour trying to figure this out, but no luck.

Here's what I've found, in hope that someone else can continue this:

• Since the settings form for bartik and seven have never been saved, they've got no configuration.
• UpdatePathTestBase::runUpdates() seems to treat that situation as a fail for some reason (Saving them when they didn't exist before).
• However, shortcut_install() does the exact same thing for seven theme.
• I've been trying to see how ThemeSettingsForm does it, but I see nothing different so far.
• The update hook itself works, as my test added on #63 proves. I've also tested the code manually on a local installation.

I'm guessing we should be checking if we've got configuration for each theme, if so then load it, set the new values, and save it. Otherwise, create a new one "properly", set the new values and save it.

\Drupal::configFactory()->getEditable('bartik.settings')->set('logo.alt', 'Home')->save(TRUE); doesn't seem to be the way to go about it, although you can definitely do this in custom code. Any pointers would be very welcome :)

+++ b/core/modules/system/system.install
@@ -1927,3 +1927,23 @@ function system_update_8400(&$sandbox) {
+  foreach ($theme_handler->listInfo() as $theme) {
+    \Drupal::configFactory()->getEditable($theme->getName() . '.settings')
+      ->set('logo.alt', 'Home')
+      ->set('logo.title', 'Home')
+      ->save(TRUE);
+  }

You only should be writing is the theme settings have been customised.

  foreach ($theme_handler->listInfo() as $theme) {
    $theme_settings = \Drupal::configFactory()->getEditable($theme->getName() . '.settings');
    // Only update if a theme settings exists.
    if (!$theme_settings->isNew()) {
      $theme_settings
        ->set('logo.alt', 'Home')
        ->set('logo.title', 'Home')
        ->save(TRUE);
    }
  }

As base themes classy and stable are not customisable. This means you need to ensure that bartik.settings actually exists in the update path test.

Re: #62.1 - the logo image absolutely MUST have alt text, because it is the only content inside a link. Without it, we would have a link without a text value:

<a href="/" title="" rel="home" class="site-branding__logo">
  <img src="/core/themes/bartik/logo.svg" alt="">
</a>

That's a WCAG 2 failure at level A , specifically F89: Failure of Success Criteria 2.4.4, 2.4.9 and 4.1.2 due to not providing an accessible name for an image which is the only content in a link.

I thought this was why we had the Twig default values like this: alt="{{ site_logo_alt|default('Home'|t) }}". They were flagged in #59.3 and removed in patch #61.

We could just make the alt field mandatory in the them settings form, but I'm a bit worried that there isn't an easy way for a user to set it back to the default. With the Twig default, leaving it blank would still result in ALT text, and the description could say it defaults to "Home" if left blank. (Caveat: can this default vary per theme?)

The logo title attribute doesn't have to be mandatory.

@andrewmacpherson it's not the twig template's job to make it easy to revert to default. Making it required in the form is best because then when the user tries to set it to empty they get an error rather than the alt text being unexpectedly set to Home for reasons that are not obvious.

I've changed the update function as suggested by alexpotts in #67. It works when testing locally, we'll se what testbot makes of it.

I've also made the alt text field mandatory from the comments by andrewmacpherson (#68) and alex (#69.

Added the same code for the update function (from #67) to the test to ensure that only active themes have their settings checked.

+++ b/core/modules/system/src/Tests/Update/SiteLogoAltTitleAddedTest.php
@@ -0,0 +1,61 @@
+    // Check that each enabled theme has the expected values after the update.
+    foreach ($this->themeHandler->listInfo() as $theme) {
+      $theme_settings = $this->config($theme->getName() . '.settings');
+      if (!$theme_settings->isNew()) {
+        $this->assertEqual($theme_settings->get('logo.alt'), 'Home');
+        $this->assertEqual($theme_settings->get('logo.title'), 'Home');
+      }
+    }

Rather than a foreach here it would be great to be more explicit as this is a test.

I've changed the test so that in only checks the setting for the seven theme, as suggested by alexpott in #74.

I have tested the patch mentioned in comment #75 and it is working as expected.
PFA screengrab before and after patch.

Ah beautiful thank you @John Cook for the work, and @alexpott for the explanations, makes perfect sense now.

To answer #68 very valid concern on accessibility, there is no need to handle defaults on the twig files like alex said, drupal already handles this for us. Double checked just in case, on a brand new drupal installation with this patch, markup looks like this:

<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/block--system-branding-block.html.twig' -->
<div id="block-bartik-branding" class="contextual-region clearfix site-branding block block-system block-system-branding-block">
  
    <div data-contextual-id="block:block=bartik_branding:langcode=en"></div>
        <a href="/" title="Home" rel="home" class="site-branding__logo">
      <img src="/core/themes/bartik/logo.svg" alt="Home" />
    </a>
        <div class="site-branding__text">
              <div class="site-branding__name">
          <a href="/" title="Home" rel="home">drupal8</a>
        </div>
                </div>
  </div>

<!-- END OUTPUT from 'core/themes/bartik/templates/block--system-branding-block.html.twig' -->

Changes to the tests look fine, to me, tempted to RTBC, just one thing:

+++ b/core/modules/system/src/Tests/Update/SiteLogoAltTitleAddedTest.php
@@ -48,11 +48,11 @@ public function testSiteLogoAltTitleAdded() {
-    // Check that each enabled theme has the exepected valuesafter the update.
-    foreach ($this->themeHandler->listInfo() as $theme) {
-      $this->assertEqual($this->config($theme->getName() . '.settings')->get('logo.alt'), 'Home');
-      $this->assertEqual($this->config($theme->getName() . '.settings')->get('logo.title'), 'Home');
-    }
+    // Check the settings for the seven theme to make sure that they are the
+    // expected values after the update.
+    $theme_settings = $this->config('seven.settings');
+    $this->assertEqual($theme_settings->get('logo.alt'), 'Home');
+    $this->assertEqual($theme_settings->get('logo.title'), 'Home');

Should we be testing for other core themes as well or does checking seven suffice?

@Manuel, I checked when it looped which themes passed and seven was the only one that did.

I think that seven is the active theme when doing the test, with no other themes enabled.

@John Cook ah, perfect then, thanks for clarifying.

The current upgrade path is missing support for translations. This is a BC break which we don't want to introduce. I suggest that we iterate through all the languages site has enabled, and create configuration translation in each language. We should also extend the test coverage for this use case.

@Manuel Garcia asked for help on IRC, so I jumped in and took a look as how this should look like.

+++ b/core/modules/system/system.install
@@ -1927,3 +1927,26 @@ function system_update_8400(&$sandbox) {
+/**
+ * Set the default Logo alt and title.
+ */
+function system_update_8302() {
+  // Set the values for the global theme settings.
+  \Drupal::configFactory()->getEditable('system.theme.global')
+    ->set('logo.alt', 'Home')
+    ->set('logo.title', 'Home')
+    ->save(TRUE);
+
+  // Set the values for all currently installed themes.
+  $theme_handler = \Drupal::service('theme_handler');
+  foreach ($theme_handler->listInfo() as $theme) {
+    $theme_settings = \Drupal::configFactory()->getEditable($theme->getName() . '.settings');
+    if (!$theme_settings->isNew()) {
+      $theme_settings
+        ->set('logo.alt', 'Home')
...
+        ->save(TRUE);
+    }
+  }
+}

We would need there something along:

  /** ConfigurableLanguageManager $manager */
    if ($manager->isMultilingual()) {
      $languages = $manager->getLanguages();
      foreach ($languages as $langcode => $language) {
        $configOverrideStorage = $manager->getLanguageConfigOverrideStorage($langcode);
        if ($configOverrideStorage->exists('system.theme.global')) {
          $configOverride = $manager->getLanguageConfigOverride($langcode, 'system.theme.global');
          // Hey, how are you getting proper translations?
          $configOverride->set('logo.alt', t('Home', [], ['langcode' => $langcode]))
            ->set('logo.title', t('Home', [], ['langcode' => $langcode]))
            ->save();
        }
      }
    }

Here I assumed that we only want to write the configuration translation if it was already translated to that language.

Open questions: how do we get default values? t('Home', [], ['langcode' => $langcode]) is one option, but in most cases there won't be a translation already in the site.

Thanks @laurii for the review and thanks @penyaskito for kindly explaining this.

Attached the updated upgrade path, which creates a new configuration translation for each language (or updates it if it was there already).

Tested this manually with a fresh install of Drupal in English and Spanish, then ran the update - seems to work as expected... I'd love a quick review before extending the test coverage though :)

+++ b/core/modules/system/system.install
@@ -1949,4 +1949,24 @@ function system_update_8302() {
+      else {
+        $configOverrideStorage->write('system.theme.global', ['logo' => ['alt' => $value_to_save, 'title' => $value_to_save]]);
+      }

Not sure if you want to create the override if it didn't existed before.

+++ b/core/modules/system/system.install
@@ -1927,3 +1927,46 @@ function system_update_8400(&$sandbox) {
+    $theme_settings = \Drupal::configFactory()->getEditable($theme->getName() . '.settings');

You also want to edit these config translations in the same way

Thanks again @penyaskito for the review & explanations, here some progress hopefully addressing #86 :)

One thing that still worries me about all this is that I don't see these to be translated on /admin/config/regional/config-translation - is this expected or? How would one go about translating these two then?

Rerolling, a new update hook was added to system.install.

Here is my attempt at adding test coverage for the multilingual part of the upgrade path, however it is failing for me.

I'm not sure whether the test is wrong or whether the changes made since #75 need to be looked at (or both).

In any case this is as far as I can go - any help would be very welcome =]

The Umami demo profile has a real need for this. (I know, that seems the wrong way around. Umami is supposed to be a demo of Drupal's existing features, rather than driving requests for new features...)

Currently the Umami branding block uses a logo image which shows the site name in a fancy font, AND it adds .visually-hidden to the site name display. The upshot of this is that the Umami branding block shows the site name to sighted users, and has a visually-hidden site name for screen readers.

But there's a huge accessibility problem with this. The visually hidden site name is a link, so there's an operable link in there which cannot be perceived + understood by sighted keyboard users. It's a WCAG failure of "Focus Visible".

A better approach for Umami would be to uncheck the site name display option, and set the logo alt text to "Umami Food Magazine", which is exactly what THIS issue would provide.

I think the situation with the Umami header design reflects a common scenario, so it justifies this feature.

Nearly forgot about this one, re-rolling for now...

Cleaning up the old test to be in line with current times.

Some more cleanup and hopefully fixing one of the failing tests.

A bit more cleanup and getting the new SiteLogoAltTitleAddedTest to pass up to where we check for the language config override of seven.settings.

Not sure why it fails there, I've debugged the update function and it seems to run correctly.

Perhaps its because of why we're setting up the test here?

Any pointers would be welcome at this stage, not very familiar with this part of core, so I'm blocked for now...

I cant reproduce this failure shown by the bot when I run the test locally:
Schema key block.block.bartik_account_menu:settings.cache failed with: missing schema

Good to see this moving, thanks @Manuel.

I tried out the UI, and it's looking good. We recently updated the FAPI #descriptions for the alt text in Editor and Image modules, in #2991149: Improve placeholder text for image upload. There, we got rid of bad advice which said the alt text was for search engines, so I wanted to check what the logo alt #description said here. I'm happy with it. Looking back over this issue history, it seems I first had the idea to remove the bad SEO advice in comment #43 here.

I skim-read the update hook here. I haven't tested it, but going by the code comments it looks like it's all in hand, and there's a test for the update.

I'll let someone else review the update path and tests in detail. The GUI and output are ready.

I added the DrupalEurope issue tag and we can work on this. Update tests can be difficult to debug, but this could be a missing module in the test that needs to be included in this test in addition to the base class.

I am also getting different results:

1) Drupal\Tests\system\Functional\Update\SiteLogoAltTitleAddedTest::testSiteLogoAltTitleAdded                       
Failed asserting that null matches expected 'Home'

Don't know if this helps anyone.

Yeah that is what I see locally too @bserem - though I don't know why it happens, my guess is we're setting up the test incorrectly, but I don't know how to set it up otherwise...

@tashaharrison80, @sysosmaster, and I are working on this together at DrupalEurope.

The update is running fine in the test, but we believe that the themes are not installed correctly (particularly seven).

We found that the test failure noted by @bserem is correct. We've (@sysosmaster) concluded the test should pass if the translation is working as it is supposed to be.

We (@tasharobinson80) tested manually and found that the alt/title are not translated while using a different language and a non-default theme. However it is being translated when a theme is the global theme.

We confirmed the update is running successfully as well on multiple environments and themes (seven, bartik, stark).

This might be a bug in the language configuration overrides or the theme settings in this patch.

I think the next step is to look at the rest of the patch and see how the configuration is coming in.

Actually, @tasharobinson80 pointed out that the last part of the update isn't running at all so the config override doesn't exist for the theme settings and thus isn't getting set.

It's also hard to look at the issue because we're not sure the user interface for language config overrides quite yet. Probably need to look up the documentation around that.

worked on this with @tashaharrison80 and @mradcliffe. @ Drupaleurope

Since 8.6 logo can be svg or image https://www.drupal.org/node/2939152

It means that theme should be smart enough to display "alt" depending on type of logo
- alt for images
- aria-label for svg for accessibility

+++ b/core/modules/system/config/install/system.theme.global.yml
@@ -12,3 +12,5 @@ logo:
+  alt: 'Home'

Why alt default value is "Home" instead of "Logo" which is should explain what this element is about

@andypost - no, the suggestions in #144 are going off-track.

That change record is about specifying a default logo path & filename in info files. It didn't change the types of image which could be used as a logo, just removed a hard-coded default filename. We've always been able to use SVG, JPEG, PNG, as a custom logo in D8.

It doesn't matter whether the logo is SVG or a raster format, the file is used as the <img src>. So there's no need to use aria-label - the first rule of using ARIA is that you don't use ARIA when a HTML mechanism is available. When an <img src> points to an SVG, the alt attribute is the correct way to provide a text alternative.

Aside: on the other hand, if the <svg> document was embedded in the HTML directly, then aria-label would be appropriate. That's not the case with the branding block though.

Why alt default value is "Home" instead of "Logo" which is should explain what this element is about

Remember the logo image is also a link, so we must convey the purpose of the link. That's more important than describing the image itself. "Logo" helps nobody because it's too vague - it describes neither the image itself, nor the purpose of the link.

"Home" describes the purpose of the link, and is currently hard-coded in the branding block template. It's a reasonable default, and this patch allows profiles to override it with installation config.

Some examples may explain this better.

1. In Standard profile, Bartik's branding block shows the the default druplicon logo, followed by the site name. Both are links to the home page. The logo image contains no text, and mainly serves as decoration. However it's an operable link, and we must convey the purpose. We won't use the site name here, because that's present in nearby text. So alt="home" will suffice. The same applies for the Minimal profile with the Stark theme, and many contrib distros.
2. In the Umami demo profile, the default logo is an image of text. In this case the logo's text alternative should match the the text it contains, i.e. alt="Umami Food Magazine" (or perhaps alt="Umami Food Magazine - Home"). This is an image used alone as a linked logo. See also example 9 in Understanding WCAG SC 1.1.1 Non-text Content. Since the logo image has the site name, we don't want to display the site name text link at all. Currently it is visually-hidden, but this causes problems for sighted keyboard users, so we'll be turning it off in the branding block config once this patch is in. This scenario applies to many web sites, and perhaps some distros too.

Adding sprint credits from #112-113

Rerroll of #102, simple conflict on system.install.

Just a small cleanup to use public static $modules = ['language']; to enable the language module in the test.

I think the last failures on the test might've been because of random test bot failures, Schema key block.block.bartik_account_menu:settings.cache failed with: missing schema doesnt seem to be related to this patch.

This should get the test to actually run.

I noticed that the previous failure is still happening:

There was 1 failure:

1) Drupal\Tests\system\Functional\Update\SiteLogoAltTitleAddedTest::testSiteLogoAltTitleAdded
Failed asserting that null matches expected 'Home'.

/var/www/drupal8/core/tests/Drupal/Tests/BrowserTestBase.php:699
/var/www/drupal8/core/modules/system/tests/src/Functional/Update/SiteLogoAltTitleAddedTest.php:100

So I added assertions to check that the language config overrides exist, and apparently for seven.settings it doesnt:

There was 1 failure:

1) Drupal\Tests\system\Functional\Update\SiteLogoAltTitleAddedTest::testSiteLogoAltTitleAdded
Configuration override for seven.settings in Spanish exists
Failed asserting that false is true.

/var/www/drupal8/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/drupal8/core/modules/system/tests/src/Functional/Update/SiteLogoAltTitleAddedTest.php:103

I've tried reseting the language manager and rebuilding the container after creating the language, but no luck.

If anyone knows why this is happening I'd be very keen to hear it :)

OK did a bit more digging, it seems that actually these theme settings are not currently translatable as far as I can see...

I tried adding this to the system.config_translation.yml file but getting access denied when accessing /admin/appearance/settings/translate:

system.theme_settings:
  route_name: system.theme_settings
  title: 'Appearance settings'
  base_route_name: system.theme_settings
  names:
    - theme_settings.alt
    - theme_settings.title

So still blocked, but the update test seems to have picked up an actual bug in the current patch so that’s good news.

Seems to me like we'd need #2154743: Support route parameters for *.config_translation.yml files' base_route_name properties for this to happen?
I don't think core theme settings are actually translatable at the moment? I believe this is blocked unless someone figures out a way forward.

Its also worth noting that the [theme_name].settings config is not really getting installed as part of theme install. Only config we can see when installed with standard profile is seven.settings and looks like it exists only because of the 3rd party settings from shortcut module.

//seven.settings.yml
a:2:{s:20:"third_party_settings";a:1:{s:8:"shortcut";a:1:{s:11:"module_link";b:1;}}s:4:"logo";a:2:{s:3:"alt";s:4:"Home";s:5:"title";s:4:"Home";}}

Fixing tags

The accessibility tag has been behaving strangely over the last month. I'll ask the drupal.org webmasters for help, but for the meantime leave both tags in place.

Fixing tagging.

I am working on this issue at DrupalCon 2019 Seattle, though a newbie and may not be helping much! No longer looking at this. Sorry!

@sonu.raj.chauhan Hi, thanks for collaborating. Please always set a comment describing your work over this case and too a interdiff file that describe the changes on git.

BTW patchs 123, 124, 135 and 136 (by @sonu.raj.chauhan) are the same code.

Patch 122 rerolled and some update to twigs files and system.install

I updated Umami's theme to use the new settings for the logo.

I added a missing empty line at the end of system.install file.

Tagging for DrupalCamp Colorado 2019 (Sunday August 4)

Here is my patch

Oops, sorry, was a wrong patch.
This one should be better.

So glad to see other people pushing this forward, I totally hit a brick wall on this and had lost hope :)

Re #146:

@@ -149,7 +265,7 @@
-+        if ($configOverrideStorage->exists($theme_settings_name)) {
++        if (!$configOverrideStorage->exists($theme_settings_name)) {

I'm not entirely sure this is what we want to do... shouldn't we be updating only if the language configuration override exists already?

The site logo no longer uses a title attribute since #2920395: SystemBrandingBlock has identical anchor title and link content (image alt).

A GUI to set the logo alternative text is still desirable, but there's no longer a need for a GUI to set the logo title attribute. The parts dealing with the logo title attribute can be removed from this patch. I'll work on that today.

Setting to NW because template translation should not be used

Also I think it could be slightly simplified by extending the block setting... I see no reason to introduce a theme setting additionally

+++ b/core/includes/theme.inc
@@ -480,6 +480,12 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
+      $config->set('logo.alt', $value);
...
+      $config->set('logo.title', $value);

+++ b/core/modules/system/config/install/system.theme.global.yml
@@ -12,3 +12,5 @@ logo:
+  alt: 'Home'
+  title: 'Home'

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -239,6 +239,22 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
+        '#default_value' => theme_get_setting('logo.alt', $theme),
...
+        '#default_value' => theme_get_setting('logo.title', $theme),

+++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
@@ -159,10 +159,20 @@ public function build() {
-      '#alt' => $this->t('Home'),
+      '#alt' => theme_get_setting('logo.alt'),
+      '#title' => theme_get_setting('logo.title'),
...
+    $build['site_logo_alt'] = [
+      '#markup' => theme_get_setting('logo.alt'),

+++ b/core/modules/system/templates/block--system-branding-block.html.twig
@@ -11,18 +11,20 @@
-      <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" />
+    <a href="{{ path('<front>') }}" title="{{ site_logo_title|t }}" rel="home">
+      <img src="{{ site_logo }}" alt="{{ site_logo_alt|t }}" />
...
-    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
+    <a href="{{ path('<front>') }}" title="{{ site_logo_title|t }}" rel="home">{{ site_name }}</a>

Both title and alt stored in config and should use config translation instead of template level translation (it's easy to miss quotes in template)

On other hand block config form could be improved to allow configure alt and title per block instance (it will also use config translation for block) - then theme setting could be used as default... but block already has own defaults

A regression crept in with patch #145:

diff --git a/core/modules/system/src/Form/ModulesListForm.php b/core/modules/system/src/Form/ModulesListForm.php
index 258930e..cabbe2e 100644
--- a/core/modules/system/src/Form/ModulesListForm.php
+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -2,6 +2,7 @@

 namespace Drupal\system\Form;

+use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Config\PreExistingConfigException;
 use Drupal\Core\Config\UnmetDependenciesException;
 use Drupal\Core\Access\AccessManagerInterface;
@@ -323,7 +324,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
     /** @var \Drupal\Core\Extension\Dependency $dependency_object */
     foreach ($module->requires as $dependency => $dependency_object) {
       if (!isset($modules[$dependency])) {
-        $row['#requires'][$dependency] = $this->t('@module (<span class="admin-missing">missing</span>)', ['@module' => $dependency]);
+        $row['#requires'][$dependency] = $this->t('@module (<span class="admin-missing">missing</span>)', ['@module' => Unicode::ucfirst($dependency)]);
         $row['enable']['#disabled'] = TRUE;
       }
       // Only display visible modules.

This doesn't appear in the interdiff in #145 so I went looking via git blame. It looks like this is an unintentional reversion of #3042999: Remove ucfirst() from missing dependencies on the Extend page.

This patch fixes that.

I'm working on #148 just now.

#150:

I think I see what you mean about template translation. I try that after addressing #148.

Also I think it could be slightly simplified by extending the block setting... I see no reason to introduce a theme setting additionally

But the logo image is specified at the theme settings page. The alt text should be set in the same form.

This patch addresses #148, by removing everything to do with the custom title attribute. Now the patch just provides a GUI for the logo's text alternative.

It doesn't address #150 yet.

Also, I noticed that the template files were updating the title attribute on the site-name link too:

   {% if site_name %}
-    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
+    <a href="{{ path('<front>') }}" title="{{ site_logo_title|t }}" rel="home">{{ site_name }}</a>
   {% endif %}

That's out of scope for this issue, which is just about the logo image, not the site-name. I've undone this change in each template.

Addresses the template translation part of #150.

I don't really grok the config translation issue that Manuel Garcia linked in #125... #2154743: Support route parameters for *.config_translation.yml files' base_route_name properties

Probably not a novice issue any more.

Thanks a bunch for working on this.

Looks like we dropped the update path test SiteLogoAltTitleAddedTest from the patch since #146, setting to needs work because of that. It'll probably also need updating as well since we are removing everything to do with the custom title attribute.

Adding back the update path test, removing the parts that were testing the logo.title configuration since we're no longer adding it.

Hi everyone
We have been using patch from #122 and than an updated patch from #135 for a long time, now since then this issue has been updated a lot and now the correct update hook is 8703, Now if we apply the latest patch on our sites it will result in data loss, To avoid this i am attaching an updated patch which reads the existing alt text values from db first and than apply the values.

Please review.
Thanks

Re #151:
Thanks for reporting about running the patch for a long time, looks like its working correctly I assume :)

I'm not sure core should account for such situations where the update hook updates the value of your patched config. That said, I dont think it hurts to check it just in case. In my opinion tough if the configuration has an existing value, we should not save it again with the same value.

A quick eyeball review of the interdiff on #151:

+++ b/core/modules/system/system.install
@@ -2379,9 +2380,8 @@ function system_update_8703() {
@@ -2397,17 +2397,17 @@ function system_update_8703() {

@@ -2397,17 +2397,17 @@ function system_update_8703() {
       $value_to_save = t('Home', [], ['langcode' => $langcode]);
       // Update the override for global theme settings.
       if ($configOverrideStorage->exists('system.theme.global')) {
-        $language_manager->getLanguageConfigOverride($langcode, 'system.theme.global')
-          ->set('logo.alt', $value_to_save)
-          ->save();
+        $global_config_translated = $language_manager->getLanguageConfigOverride($langcode, 'system.theme.global');
+        $alt_value = $global_config_translated->get('logo.alt') ?: $value_to_save;
+        $global_config_translated->set('logo.alt', $value_to_save)->save();
       }
       // Update the override for each installed theme settings.
       foreach ($installed_themes as $theme) {
         $theme_settings_name = $theme->getName() . '.settings';
         if (!$configOverrideStorage->exists($theme_settings_name)) {
-          $language_manager->getLanguageConfigOverride($langcode, $theme_settings_name)
-            ->set('logo.alt', $value_to_save)
-            ->save();
+          $theme_settings_translated = $language_manager->getLanguageConfigOverride($langcode, $theme_settings_name);
+          $alt_value = $theme_settings_translated->get('logo.alt') ?: $value_to_save;
+          $theme_settings_translated->set('logo.alt', $value_to_save)->save();
         }
       }
     }

These are still saving the new configuration value even if there was an existing one.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Looks like WCAG SC 1.1.1 may still be a problem. I have not tested if this is the case with the latest patch.