Rename ambiguous "Menu" toggle in Bartik for accessibility

@mgifford

Do you mind explaining a suggestion what exactly should be done? If written properly, I hope this could be a novice task.

Would this be the right approach?

This is what it would look like if there were multiple menus in the primary menu region. A question: wouldn't it be better if the expansion icon was on the left instead of the right? That would make it consistent with the toolbar menu. And more intuitive I think.

This is what it might look like if the expansion icon was on the left side.

Patch based on mgifford's suggestion.

I'm not sure if it couldn't be simpler.

Don't we have a problem with capitalisation... eg. "Hide Main navigation" and "Show Main navigation"?

'Main navigation' is generated from config and is the way the menu name is displayed other places, eg., admin/structures/block. But it is easy to change. Who would decide?

This is what it looks like currently with 2 menus in the primary menu region. It really should get in before RC.

Here is a patch that addresses case issue

Nice!

This still applies without error. Why the reroll?

As far as I can tell the test never finished and I can't see how to resubmit it. So here is the same patch again. The diff was 0.

Shouldn't this use trans? i.e. something like:

{% trans %}Book outline for {{ book.title }}{% endtrans %}

The current patch simply hardcodes a translate verb followed by a translatable menu name. Not every language has this word order.

Yup, the word order should be modifiable. Also, why do we t()-ed the label in twig? Is it coming from a loaded config object? That should already be loaded in the right language unless you explicitly loaded in its original form. So should not be t()-ed. Anyway, configuration should not be t()-ed in general.

I don't think that is how you represent variables with {%trans%}. See examples in https://www.drupal.org/node/2047135.

{% trans %} is part of the Twig "i18n" extension, which also requires the GETTEXT extension installed on PHP. I don't see that extension installed in Drupal, and gettext is not the default on mac OS X. So that seems to mean that we have to use the t filter.

@dcrocks: Drupal uses the same trans tag but implements it our own way to integrate with Drupal. No need for getting or anything else. It works the same as the t filter.

I created a Spanish version of drupal from a current clone of 8.0.0 and it doesn't appear to be working. I've attached images to show the English and Spanish version of the sites. I also show an image of the structure/menu form which shows that the menu titles(and machine names) for the main and account menus aren't translated while their menu link titles are. The menu titles of all other menus are translated but their machine names aren't. Also note that the lower case filter doesn't seem to apply.
While experimenting with the code I noticed if I separate the 2 parts of the translated string to separate calls to (% trans %} the literal string(show/hide) will be translated but the menu label is not.

<a class="menu-toggle" href="#{{ show_anchor }}">{% trans %} Show {% endtrans %}
    {% trans %} {{ configuration.label|lower }} {% endtrans %}</a>

So I don't know how translation is actually done in drupal. Is it a table lookup for single words and phrases, or is each word in a phrase translated against a dictionary separately?

So the issue is that not all menu titles have translations. 'Main navigation' and 'Account' fall into this category. In particular Show or Hide concatenated onto a menu title will most likely not have a translation. But some menus titles are already translated and don't need translation in this template. So I modified the code to:
<a class="menu-toggle" href="#{{ show_anchor }}">{% trans %} Show {% endtrans %} {{ configuration.label|lower }}</a>
I added a menu with a translated title to the primary menu region and the attached image shows it works as expected. But this will not work for RTL languages. One step at a time.

@dcrocks that a string that this issue would add does not already have a translation for some language would not surprise me. The issue proposes to add the new source string, so just natural :) The new source string should however show up in the interface translation UI and let you translate it. Separating the string to translate a word and a placeholder does not allow for swapping the order where a language translation requires "show" to come after the name of the thing. Not all languages use the same word order.

I test-drove this on simplytest.me. The strings appear as source strings "Hide @configuration.label" and "Show @configuration.label". The Hungarian translation would require the word to go after the label, so "@configuration.label megjelenítése" for example. Translating to that of course worked with interface translation. Basically two things:

1. The lowercasing does not seem to actually work.
2. If the order of the words is swapped (as required by the Hungarian translation), then the "sentence" starts off with a lowercase character, is that desired?
3. Do we use dotted variable names elsewhere? I am not sure. Twig can IMHO reassign that value to a simpler variable name, so the placeholder for translation is simpler.

Translated both the string and the menu name for this screenshot (neither is translated in the Hungarian translation either ATM).

The parser behind localize.drupal.org does not seem to be expecting dotted variable names. See #2610436: Twig templates incorrectly use % trans % with arbitrary filters for an issue around that.

@Hojtsy Filters don't work inside a {% trans %}{% endtrans %} so you have to do that outside of there, eg,

 {% set menu_show_label = 'Show ' ~ configuration.label|lower %}
 {% set menu_hide_label = 'Hide ' ~ configuration.label|lower %}
...
...
...
<a class="menu-toggle" href="#{{ show_anchor }}">{% trans %} {{ menu_show_label }} {% endtrans %}</a>
...

But you would still have to manually provide a translation for the complete string. And also that clearly has problems with RTL language display. I did spend some time to see if there was a 'ltr!rtl' variable available to use code logic to build the string properly but if you have to hand translate each string manually it doesn't seem like much point.
This means that in all cases an admin would have to provide translated strings for this display of the menu for their site.

ps. I don't speak Spanish, I just chose it because I thought it would be fairly easy to navigate the site.

@dcrocks: Drupal has been doing well with variable placeholders in translatable strings for a decade, so you can just reformat:

{% set menu_label = configuration.label|lower %}
<a class="menu-toggle" href="#{{ show_anchor }}">{% trans %} Show {{ menu_label }} {% endtrans %}</a>

Which will end up as "Show @menu_label" for translation. All built-in menu labels are translated separate from the "Show @menu_label" string on localize.drupal.org and Drupal combines them to form the resulting string. Otherwise how could we do "Dear @username" and such? We cannot expect the site owner to translate the string for each username.

@hojtsy {%trans %} resolves the entire string before it tries to translate. You should try your method with menus that don't already have a translation.

@dcrocks: I don't get what the problem is. How do all the other % trans % blocks work in core with placeholders then? :)

Good question. It didn't work for me. I used <a class="menu-toggle" href="#{{ show_anchor }}">{% trans %} Show {{ menu_label }} {% endtrans %}</a>
an it didn't work. I got 'Show herramientas'

@dcrocks: did you go and translate "Show @menu_label" as well in your interface translation UI?

No. Is that the trick?

@dcrocks: how would Drupal know the translation of that brand new string that never was part of Drupal 8 but added with this patch?

Ok, I'll look up the documentation on that. Have to go now, respond in a few hours.

Sorry, server update bollixed my system. Ok, I see how to add that definition through the language interface. But still, doesn't that mean I'll have to add that definition each time I install drupal in a different language?

Here is a the revised patch. I tested in english and spanish. Someone else should check, hopefully also in a RTL language.

@dcrocks: re #54, no, if we would only ever add "new" strings which are already translatable / translated, then we could never add new strings. Once this patch lands, the next release will make the string available on localize.drupal.org for translation and teams will be able to translate it, making it available on your sites later on as already translated.

Reviewing the patch again:

+++ b/core/themes/bartik/templates/block--system-menu-block.html.twig
@@ -9,13 +9,14 @@
+{% set menu_label = configuration.label|lower %}

Once again I am wondering if we want to lowercase this on the expense of it coming out as an all lowercase "sentence" for those who need to swap the verb with the menu name (such as in Hungarian). Is the lowercasing of more value for the languages where the order of the verb does not need to be swapped?

Fair argument. In English eyes it is the correct grammatical representation. But perhaps we could add some markup that would allow for two capitalization's in the phrase. Possibly Show - Main menu?

Sounds good to me.

We will need input from others before we can go forward.

Here is an example where I used a special character.

This patch eliminates lower casing the menu label in deference to languages that may not have the same capitalization rules as english. It also adds a double arrow separator to imply '(Show/Hide) This'.

sigh

It looks fine from a translatability perspective. I am not sure about the use of the special char, but nothing particularly against it. Looks good for translation.

I can use named entities instead of the numeric entities to make it more understandable for the blind, assuming they understand english. I also have looked at other characters that might be more meaningful(visually) but I can't find much that matches the drupal icon set:

'⇒' '⇾' '∷' '↦' '⇛' '⇥' '∴' '∷' '≔' '≫' '⊳' '⊸' '⋙' '⌂' '▸' '⟶' '⟹' '⩸' '⧁'

For example, I could enter '⟹' as '⟶, which would be helpful for the blind.

Makes sense :)

Time to get this done. @mgifford Good research.

@catch, @alexpott, and I discussed this issue. The patch includes the addition of two strings and changes the translated text on a user-facing link. The affected text is only for accessibility purposes (screen readers, etc.), but is also almost useless for that usecase in HEAD. Changing it does not affect the display of the site otherwise. The changed string also includes an already-translated label, so even for screen reader users on non-English sites, there is some context for the changed text until translations are updated.

For these reasons, we decided it would be best to commit this to 8.0.x for the next patch release despite the disruption for translations.

Thanks for the issue report, fix, and review for translatability! Committed and pushed to 8.1.x and cherry-picked to 8.0.x.