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.
Comment | File | Size | Author |
---|---|---|---|
#46 | 1848552-46-localized-toolbar-icons-just-test.patch | 3.76 KB | shnark |
#46 | 1848552-46-localized-toolbar-icons.patch | 4.36 KB | shnark |
#42 | 1848552-42-localized-toolbar-icons-just-tests.patch | 4.36 KB | YesCT |
#42 | 1848552-42-localized-toolbar-icons.patch | 4.36 KB | YesCT |
#42 | interdiff-34-42.txt | 1.23 KB | YesCT |
Comments
Comment #1
Gábor HojtsyHa! 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).
Comment #2
Gábor HojtsyShorter title.
Comment #3
rvilarI attached a patch that uses the 'link_title' element in the $item array. I think can be a correct solution.
Comment #4
rvilarChanging the title
Comment #5
Gábor HojtsySo based on the docs on _menu_link_translate(), link_title would indeed be the non-localized title:
Not sure how title callbacks are involved, but looking at this code it seems to be right.
Comment #6
bforchhammer CreditAttribution: bforchhammer commentedMarked #1848102: Toolbar icons broken in languages other than English as duplicate.
Comment #7
Gábor HojtsyAlso looked through how the localization happens in _menu_item_localize() and this patch looks good to me.
Comment #8
Shyamala CreditAttribution: Shyamala commentedtagging
Comment #9
YesCT CreditAttribution: YesCT commentedto 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.
Comment #10
YesCT CreditAttribution: YesCT commentedneeds tests?
We should be able to make a test that will fail without the fix.
with the fix,
and without that is missing, so we can write a test for that, right?
Comment #11
YesCT CreditAttribution: YesCT commentedwe 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?
Comment #12
dcrocks CreditAttribution: dcrocks commentedA 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.
Comment #13
Gábor HojtsyI'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?
Comment #14
YesCT CreditAttribution: YesCT commentedthis issue is listed under bugs in #1846970: [meta] Responsive Toolbar follow-ups
so that might help get this some visibility
Comment #15
Gábor HojtsyThis has been reproduced by various people in http://groups.drupal.org/node/276518
Comment #16
YesCT CreditAttribution: YesCT commentedNext 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)
Comment #17
rvilar@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).
Comment #18
Gábor Hojtsy@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.
Comment #19
YesCT CreditAttribution: YesCT commented@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.
Comment #20
rvilar@YesCT, @Gábor: thanks. I'll work on this test now and I'll post as soon as it works.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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...
Comment #22
rvilarI do a test but I don't know why it fail. Anyone that can help me on this?
Comment #24
YesCT CreditAttribution: YesCT commentedwhy change the default language?
Comment #25
YesCT CreditAttribution: YesCT commentedWhen 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.
Comment #26
YesCT CreditAttribution: YesCT commentedit thinks there are only 45 strings to be translated.
what is the function to call?
Comment #27
YesCT CreditAttribution: YesCT commentedI 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
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.
Comment #28
YesCT CreditAttribution: YesCT commentedThis 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).
Comment #29
YesCT CreditAttribution: YesCT commentedComment #30
YesCT CreditAttribution: YesCT commentedcopy and paste error. the comment should be something like: Make sure the menu item was translated.
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.
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.
Comment #31
YesCT CreditAttribution: YesCT commentedOh, 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.
Comment #32
YesCT CreditAttribution: YesCT commentedI fixes some comments. And generated the tests only patch correctly this time.
Comment #33
Gábor HojtsyLooks good, except :)
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 :)
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.
Double quotes should be single quotes as per coding standards :)
Needs space before/after =.
Comment #34
YesCT CreditAttribution: YesCT commentedthanks!
Comment #35
YesCT CreditAttribution: YesCT commentedComment #36
rvilar@YesCT, It looks very, very good! Now I understand how a test has to be developed. Very thanks to you and Gábor.
Comment #37
Gábor HojtsyLooks good to me now. Great extensive test coverage too :) Congrats :)
Comment #38
YesCT CreditAttribution: YesCT commented@rvilar Thanks to you too! I'm just learning tests also and I would not have gotten as far without your work first! :)
Comment #39
Wim LeersLooks good, but found some typos/bad sentences in the comments. Can be a follow-up "nitpicks commit" though :)
s/translationed/translated/
-> Make sure the menu item is translated.
-> 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.
Comment #40
YesCT CreditAttribution: YesCT commented@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:Comment #41
Gábor Hojtsy@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 :)
Comment #42
YesCT CreditAttribution: YesCT commentedI left in the check. Fixed the other two comments. Locally the tests work. If the bot comes back ok, I think this is ready.
Comment #43
Gábor HojtsyComment #44
webchickHm. Why didn't 1848552-42-localized-toolbar-icons-just-tests.patch fail?
Comment #45
YesCT CreditAttribution: YesCT commenteddoh! my test only patch has the fix!
new patch coming soon.
Comment #46
shnark CreditAttribution: shnark commenteduploading 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
Comment #48
Gábor HojtsyThe just tests patch is exhibiting the right fail. The actual fix patch exhibits block upgrade path test fails. Totally unrelated.
Comment #49
Gábor Hojtsy#46: 1848552-46-localized-toolbar-icons.patch queued for re-testing.
Comment #50
Gábor HojtsyComment #51
webchickAwesome, thanks so much for this fix! :)
Committed and pushed to 8.x.
Comment #52
Gábor HojtsyWoot, this was a grand WTF for D8MI testers, so huge thanks!
Comment #54
philipz CreditAttribution: philipz commentedI 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.
Comment #55
Gábor HojtsyYeah 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?!
Comment #56
philipz CreditAttribution: philipz commentedYes, you're absolutely right the issue you mentioned should fix my problem. Thank you for quick reply!
Changing back to fixed as it was.