With the new toolbar added in core, the menu navigation links icons are added via CSS classes. The problem is that, at the moment, this classes are based on the menu title:

function toolbar_menu_navigation_links(&$tree) {
  foreach ($tree as $key => $item) {
    // Configure sub-items.
    if (!empty($item['below'])) {
      toolbar_menu_navigation_links($tree[$key]['below']);
    }
    // Make sure we have a path specific ID in place, so we can attach icons
    // and behaviors to the items.
    $tree[$key]['link']['localized_options']['attributes'] = array(
      'id' => 'toolbar-link-' . str_replace(array('/', '<', '>'), array('-', '', ''), $item['link']['link_path']),
      'class' => array(
        'icon',
        'icon-' . strtolower(str_replace(' ', '-', $item['link']['title'])),
      ),
    );
  }
}

When we enable a new language, then the link title changesand therefore its CSS classes and all the icons disapears.

We have to change the method to create this classes to be language-independent.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI

Ha! Haha! Do we have anything in $item that would get us the pre-localisation title? That might be a good stop-gap unless we want to move it to depend on paths instead, which are more stable (but also need to make sure not aliased or prefixed when checked).

Gábor Hojtsy’s picture

Title: Icons in toolbar disapears when enabling new language » Toolbar icons disappear with translated menu

Shorter title.

rvilar’s picture

Title: Toolbar icons disappear with translated menu » Icons in toolbar disapears when enabling new language
Status: Active » Needs review
FileSize
612 bytes

I attached a patch that uses the 'link_title' element in the $item array. I think can be a correct solution.

rvilar’s picture

Title: Icons in toolbar disapears when enabling new language » Toolbar icons disappear with translated menu

Changing the title

Gábor Hojtsy’s picture

So based on the docs on _menu_link_translate(), link_title would indeed be the non-localized title:

 * @return
 *   Returns the map of path arguments with objects loaded as defined in the
 *   $item['load_functions'].
 *   $item['access'] becomes TRUE if the item is accessible, FALSE otherwise.
 *   $item['href'] is generated from link_path, possibly by to_arg functions.
 *   $item['title'] is generated from link_title, and may be localized.
 *   $item['options'] is unserialized; it is also changed within the call here
 *   to $item['localized_options'] by _menu_item_localize().

Not sure how title callbacks are involved, but looking at this code it seems to be right.

bforchhammer’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Also looked through how the localization happens in _menu_item_localize() and this patch looks good to me.

Shyamala’s picture

Issue tags: +toolbar-followup

tagging

YesCT’s picture

to make it super easy to review.

steps to reproduce:
plain D8
enable languages and locale modules, under extend
add a language
translate a menu word that has an icon (like content)
add language code to url (or use the language switcher block)
see icon disappear

apply patch.
reload page.
icon is back.
icons-s01-2012-11-26_1034.png

icons-s02-2012-11-26_1035.png

icons-s03-steps-icon-gone-2012-11-26_1039.png

icons-s04-withpatch-2012-11-26_1042.png

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
FileSize
228.59 KB
222 KB

needs tests?

We should be able to make a test that will fail without the fix.

with the fix,

.icon-content:before {
    background-image: url("../images/icon-content.png");
}

and without that is missing, so we can write a test for that, right?

icons-test-s01-2012-11-26_1147.png

icons-test-s02-2012-11-26_1148.png

YesCT’s picture

we should be able to find the test from the toolbar going in and copy one of them. and add a new test with a few steps, similar to the steps to reproduce.

I'm not sure what issue that was...
#1839514: Expand test coverage for Toolbar module: test a top-level tab without a tray
Oh, maybe it does not have tests?

dcrocks’s picture

A much simpler and more flexible way to this is to add a 'menu-data' attribute to every menu link and use icon fonts. I submitted a feature request at #1836160: Support icon fonts in menu links but so far no input. I will propose a patch in the next day or so. It is Language independent and not dependant on any static code elements. It would also provide the ability for anyone to design their own icon set to serve regionalization and creativity.

Gábor Hojtsy’s picture

Issue tags: +language-ui

I'd love if we can at least fix this. The resulting UX is pretty disconcerting especially with #1848490: Import translations automatically during installation that makes it very easy to set up Drupal in a foreign language with just a couple clicks. Anybody up for writing tests?

YesCT’s picture

this issue is listed under bugs in #1846970: [meta] Responsive Toolbar follow-ups
so that might help get this some visibility

Gábor Hojtsy’s picture

This has been reproduced by various people in http://groups.drupal.org/node/276518

YesCT’s picture

Next steps here are
to write tests for this (and submit a patch with just the tests to show the testbot will fail)

Then, either use the patch in #3
or
postpone this on getting icon fonts in (maybe via #1836160: Support icon fonts in menu links)

rvilar’s picture

@YesCT how can I help with tests? What has to be tested? I don't know how can I test this type of things (visual problems).

Gábor Hojtsy’s picture

@rvilar: the test would add a language, translate a menu item, then check if the class is still properly set on the toolbar. Without the fix, the test would fail, with the fix it would pass.

YesCT’s picture

@rvilar #10 might help.

I think class that should be tested for is icon-content. in #10 with the fix it has the class icon and icon-content, the second screenshot, there is the class icon and icon-content-af but no actual icon.

rvilar’s picture

Assigned: Unassigned » rvilar

@YesCT, @Gábor: thanks. I'll work on this test now and I'll post as soon as it works.

Anonymous’s picture

I tested the patch on a French install and it looks good to me. I'm still new to writing tests so I'll leave that part to someone with more experience...

rvilar’s picture

Status: Needs work » Needs review
FileSize
3.38 KB

I do a test but I don't know why it fail. Anyone that can help me on this?

Status: Needs review » Needs work

The last submitted patch, 1848552-22-localized-toolbar-icons.patch, failed testing.

YesCT’s picture

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,85 @@
+    // Change the default language.
+    $edit = array(
+      'site_default' => $langcode,
+    );
+    $this->drupalPost(NULL, $edit, t('Save configuration'));

why change the default language?

YesCT’s picture

When I do what the test does, but by hand, and I search for Structure:
it does not find it.

Something about needing to load pages that have the strings so it knows they need to be translated by t.

no-string-2013-01-19_1533.png

YesCT’s picture

it thinks there are only 45 strings to be translated.

what is the function to call?

need-more-2013-01-19_1536.png

YesCT’s picture

I was looking for an example of a test that was using the translate interface ui to see how to trick the system into knowing that Structure was a string that should be translatable. but..

In core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.php
from #1832614-30: Unchanged strings on page become customized when editing others

they just

+    // Create test source string
+    $string = locale_storage()->createString(array(
+      'source' => $this->randomName(100),
+      'context' => $this->randomName(20),
+    ))->save();
+
+    // Create translation for new string and save it as non-customized.
+    $translation = locale_storage()->createTranslation(array(
+      'lid' => $string->lid,
+      'language' => 'de',
+      'translation' => $this->randomName(100),
+      'customized' => 0,
+    ))->save();
+
+    // Reset locale cache
+    locale_reset();

So I have two questions,

Should we do it like that in the test without using the interface to translate?

If, I wanted to use the interface to translate something I know is on the page, like the word "Home". How do I trick they system into knowing that that's a translatable string?

Hmm. I have a patch coming shortly.

YesCT’s picture

This might have extra things, but I think tests that the class on the icon is not the title (menu title?)

Setting the default language did not seem to make the url inside the tests go to /xx/. I checked the verbose message screenshots, the url had /en/... explicitly.

So I told it to go to /xx/whatever and looked for the class there.

I'm hoping that the tests only comes back failing on core. with just one fail (the class).
there are other asserts in there, that just make sure that the menu item was in fact translated. those might be taken out.

I fixed some comments that were not sentences (ending in periods).

YesCT’s picture

Status: Needs work » Needs review
YesCT’s picture

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -54,32 +54,52 @@ function testToolbarClasses() {
+    // Make sure will be able to translate the menu item.
+    $this->assertText($menu_item_translated, 'Search found the menu item as translated: ' . $menu_item_translated . '.');

copy and paste error. the comment should be something like: Make sure the menu item was translated.

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -54,32 +54,52 @@ function testToolbarClasses() {
+    // Tests if the menu item class changes.

Since this last bit is the point of the whole test, expand the comment to something like: Ensure icon images show. Icon images show only if the class does not change.

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,105 @@
+/**
+ * Tests the translation of menu items and the correctness of assigned classes
+ */
+class ToolbarMenuTranslationTest extends WebTestBase {

similarly here. we should mention the point is to test if the images show on translated items, we do that by checking the class. (or something about the icon images) And it should end in a period as it should be a sentence.

YesCT’s picture

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,105 @@
+    // Tests if the menu item class changes.
+    $xpath = $this->xpath('//a[contains(@class, "icon-structure")]');
+    $this->assertEqual(count($xpath), 1, "The menu item class doesn't change.");

Oh, I forgot to mention, one of the main things is the change from class= to class contains. Since the class is not just "icon-structure" but, for example: "icon icon-structure", because there can be more than one class on something.

YesCT’s picture

I fixes some comments. And generated the tests only patch correctly this time.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks good, except :)

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,107 @@
+    $langcode = 'xx';
+    // The English name for the language.
+    $name = $this->randomName(16);
+
+    // Add custom language.
+    $edit = array(
+      'predefined_langcode' => 'custom',
+      'langcode' => $langcode,
+      'name' => $name,
+      'direction' => '0',
+    );
+    $this->drupalPost('admin/config/regional/language/add', $edit, t('Add custom language'));

You can spare most of this if you use a built-in language. Many of the tests pick German, French, Spanish, Hungarian, etc. based on the personal pick of the test writer :)

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,107 @@
+    // Add string 'Structure' so it is translatable.
+    $menu_item = 'Structure';
+    t($menu_item, array(), array('langcode' => $langcode));
+    // Reset locale cache.
+    locale_reset();

A more natural way to get this into the database would be to visit a page which shows the toolbar in the foreign language. If you want to keep t(), the langcode is not relevant for it to be added to sources. Also the cache clear does not seem to be needed since you only check the site via HTTP later too.

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,107 @@
+    $this->assertEqual(count($xpath), 1, "The menu item class ok before translation.");
...
+    $this->assertEqual(count($xpath), 1, "The menu item class is the same.");

Double quotes should be single quotes as per coding standards :)

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,107 @@
+    $menu_item_translated=$this->randomName();

Needs space before/after =.

YesCT’s picture

YesCT’s picture

Status: Needs work » Needs review
rvilar’s picture

@YesCT, It looks very, very good! Now I understand how a test has to be developed. Very thanks to you and Gábor.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now. Great extensive test coverage too :) Congrats :)

YesCT’s picture

@rvilar Thanks to you too! I'm just learning tests also and I would not have gotten as far without your work first! :)

Wim Leers’s picture

Looks good, but found some typos/bad sentences in the comments. Can be a follow-up "nitpicks commit" though :)

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,100 @@
+ * Tests maintaining inclusion of icons for translationed menu items.

s/translationed/translated/

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,100 @@
+    // Make sure will be able to translate the menu item.

-> Make sure the menu item is translated.

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,100 @@
+    // The icons are included based on the menu item class link title. Test that
+    // class remains the same after translation.

-> Toolbar icons are included based on the presence of a specific class on the menu item. Ensure that class also exists for a translated menu item.

YesCT’s picture

Status: Reviewed & tested by the community » Needs work

@Wim Leers Thanks. Those are good finds.

Make sure will be able to translate the menu item. is actually making sure that the string Structure is found untranslated. Because I was having trouble while getting the test to work, that it was not available for translation. Now that the drupalGet is in there, and it is finding it. I think the test to check it *was* translated is enough and this test can come out:

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarMenuTranslationTest.phpundefined
@@ -0,0 +1,100 @@
+    // Search for the menu item.
+    $search = array(
+      'string' => $menu_item,
+      'langcode' => $langcode,
+      'translation' => 'untranslated',
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $search, t('Filter'));
+    // Make sure will be able to translate the menu item.
+    $this->assertNoText('No strings available.', 'Search found the menu item as untranslated.');
Gábor Hojtsy’s picture

@YesCT: the translation submission form for the Structure menu item text would also fail, so this part of the test might not be strictly required, but it does not hurt IMHO :)

YesCT’s picture

I left in the check. Fixed the other two comments. Locally the tests work. If the bot comes back ok, I think this is ready.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Why didn't 1848552-42-localized-toolbar-icons-just-tests.patch fail?

YesCT’s picture

doh! my test only patch has the fix!

new patch coming soon.

shnark’s picture

uploading a just test patch that has just the test.
reuploading the whole patch with no diff, no difference between this and the patch in comment #42

Status: Needs review » Needs work

The last submitted patch, 1848552-46-localized-toolbar-icons.patch, failed testing.

Gábor Hojtsy’s picture

The just tests patch is exhibiting the right fail. The actual fix patch exhibits block upgrade path test fails. Totally unrelated.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks so much for this fix! :)

Committed and pushed to 8.x.

Gábor Hojtsy’s picture

Woot, this was a grand WTF for D8MI testers, so huge thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

philipz’s picture

Issue summary: View changes
Status: Closed (fixed) » Active

I hope I'm not mistaken but I've just experienced this issue in alpha-5 release so it must have been introduced back somehow.
I've changed "Content" in Administration menu and the icon is gone. I've put it back to Content and it works again. Seems exact the same issue.

Gábor Hojtsy’s picture

Yeah if you change the text the icon will not apply anymore. We do have #2115025: Content admin views title saved localized to the menu table for the content page. I think your issue would be covered there? Unless you actually change the original value in which case how would core know you still want to keep the icon?!

philipz’s picture

Status: Active » Closed (fixed)

Yes, you're absolutely right the issue you mentioned should fix my problem. Thank you for quick reply!
Changing back to fixed as it was.