421265 - Let the user easily override the language used to display HTML pages

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Larsen]

User-Agent:       Opera/9.26 (Windows NT 5.1; U; en)
Build Identifier: 3.0.3

We have a Bugzilla with German templates installed. My browser (Opera) is set to use de,en as preferred languages. This normally is fine, but for Bugzilla I´d prefer english.

Therefore, it would be nice if I could set my preferred language in my Bugzilla user preferences.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Frédéric Buclin]

Not sure I want an additional user pref for this. Bugzilla is making its best guess by looking at your browser preferred language.

Comment 2

[Larsen]

Yes, but it´s just "guessing" =)

Comment 3

[Frédéric Buclin]

Maybe we could reuse the same user pref as for email notifications? Confirming for now as this RFE makes sense.

Comment 4

[Frédéric Buclin]

Or we could store the information in a cookie. Many sites have fr | de | en ... in a corner of the page so that you can easily shift between languages.

Comment 5

[Vitaly Fedrushkov]

Use cases for this feature:

1. Internet cafes where user cannot change browser settings easily.
2. Bugzilla instance administrators (try http://landfill.bugzilla.org/bugzilla30l10n/)
3. Buggy or incapable browsers (Opera on Nokia 9500 for example)

Firefox users would use https://addons.mozilla.org/ru/firefox/addon/1333, except for case 1

Comment 6

[Jacques Supcik]

Attachment: Layout suggestion (screenshot)

Here is a screenshot of a possible layout. I had to add some "div"s and I also had to adjust the CSS in the skins.

Is this compatible with the Bugzilla standard?

Comment 7

[Frédéric Buclin]

sure

Comment 8

[Vitaly Fedrushkov]

How would it render with more languages?

What about ISO-3166 country code subtags?  See bug 258246 for example.

Do you prefer to keep language codes untranslated in localized templates?  Ain't it too technical for a user?  Compare to windows language bar: current value is indicated by two letter code, but selection offers human readable localized list.

Comment 9

[Jacques Supcik]

(In reply to comment #8)
Currently, I'm using the language list given by "Bugzilla->languages" and this gives the sub-directory names inside the template directory. I've checked with the english, french and german templates (from http://www.bugzilla.org/download/#localizations) and the sub-directory name seems to always be *short* name (e.g "en", "fr" or "de". This is actually OK for me.

I guess that the link list is OK for up to 4 languages. With more languages, a "<select>" or a drop-down menu (YUI?) would probably look better. For my usage, I will not need more than 3 languages, so this would be OK for me. But if you think that people will use it with more than 5 active languages, then I could make the layout smart enough to choose either a link list or a select according to the number of active languages.

What would you prefer:
[ ] Always a link list with short names (EN, FR, DE)
[ ] Always a drop-down menu with short names
[ ] A smart selection of the above
[ ] Always a drop-down menu with long names (English, Français, Deutsch)
[ ] Other idea?

Comment 10

[Frédéric Buclin]

Don't bother with the > 5 locales case. I doubt this will ever happen in real installations. https://landfill.bugzilla.org/bugzilla_l10n/ has all known locales, and they are only 8. So I would skip the select box option. Short names look good to me.

Comment 11

[Vitaly Fedrushkov]

I would like to ask UE team opinion

Comment 12

[A. Shimono [:himorin]]

(In reply to comment #10)
> Don't bother with the > 5 locales case. I doubt this will ever happen in real
> installations. https://landfill.bugzilla.org/bugzilla_l10n/ has all known
> locales, and they are only 8. So I would skip the select box option. Short
> names look good to me.

Isn't it better to use the 'raw' language code provided by the l10n packages?
Of cource, these are the same as the short names currently.

Comment 13

[Marc Schumann [:Wurblzap]]

I don't like this additional preference because I think it's an edge case that somebody doesn't want the browser preference to be the one that counts.

If we go with one, though, I like the idea of comment 9 best (using the subdir names).

Comment 14

[Jacques Supcik]

(In reply to comment #13)

Thank you for your comment. The majority seems to prefer the subdir names and this is also my favorite :-)

The case seems like an edge one, but I had the following issue:

My browser prefers English and this is OK most of the time. But I am living in Switzerland and sometimes, I have to help German speaking people (who are using browser that prefers German) or I'm demonstrating some features of Bugzilla in front of a French speaking community (who are using a browser that prefers French)... It's not easy to live in a country with so many national languages :-)

So my problem is not a bad guess due to a bad browser. I would just an easy way to *change* the language on-the-fly. This is why I started to work on this bug.

If you don't like this additional preference, I could make it optional.

Comment 15

[Frédéric Buclin]

Don't add any user pref for this. By default, the browser should choose the language based on Accept-Language, as it currently does. If a user wants to select another language, he clicks one of DE | EN | FR | JP | RU and this information will be stored in a cookie, which will take precedence over Accept-Language. Of course, if there is only one language available, the lang short name should not be displayed at all in the header.

Comment 16

[Jacques Supcik]

Attachment: v1

Bugzilla/Template.pm
  - added the "languages" and "current_language" global variables

Bugzilla/Install/Util.pm
  - added the cookie parsing and processing

js/global.js
  - function for setting the cookie

skins/standard/global.css
  - CSS stuff (e.g. float)
  - "padding-top:0.25em" is there to align both sides. Maybe there is a
    better way to vertically align both sides. 

template/en/default/global/common-links.html.tmpl
  - some additional <div> to make the left <-> right layout in the header
  - language switch rendering (only on the top bar)

template/en/default/global/useful-links.html.tmpl
  - additional <div> in the bottom bar

Comment 17

[Max Kanat-Alexander]

Comment on attachment 389738 [details] [diff] [review]
v1

This is cool, but I don't want to add a whole line to the top of every page just for this. Maybe a drop-down selector that appears when you click? Once we move the "Login" stuff into the upper bar (which we plan to do), this could go there, too.

You don't need to create a "languages" var, because you can do this in templates:

[% USE Bugzilla %]
[% SET languages = Bugzilla.languages %]

Comment 18

[Jacques Supcik]

Attachment: Screeenshot for login screen

Comment 19

[Max Kanat-Alexander]

Comment on attachment 389738 [details] [diff] [review]
v1

>Index: Bugzilla/Template.pm
>+            # List of all available languages
>+            'languages' => sub { return Bugzilla->languages; },

  Just access Bugzilla.languages in the templates directly.

>+
>+            # Currenly active language
>+            'current_language' => sub {
>+                my ($language) = include_languages();

  I'm concerned this might be slow, and we're doing it on every page. Is include_languages cached? If not, is it possible to just get this out of the template object again somehow?

>@@ -142,6 +143,16 @@
>     }
>     else {
>         @wanted = _sort_accept_language($ENV{'HTTP_ACCEPT_LANGUAGE'} || '');
>+        if (defined $ENV{'HTTP_COOKIE'}) {

  No, don't access HTTP_COOKIE directly. Use the cookie methods from Bugzilla::CGI. See how we do it in Auth/Persist/Cookie.pm

  Also, we should probably just call the cookie something like 'LANG'. We only use "Bugzilla_something" for the id and login right now, and ALL_CAPS_SOMETHING for the other cookies.

>Index: js/global.js
>+function set_language_cookie( value ) {
>+    document.cookie = 'Bugzilla_language=' + escape(value) +
>+    ';expires=Fri, 01 Jan 2038 00:00:01 GMT ; path=/';

  You should use the YUI cookie methods for this instead. See how I did it in js/tui.js

>Index: skins/standard/global.css
> [snip]

  Instead of modifying all this, I think you should just be only adding .lang_links in global/header.html.tmpl.

>Index: template/en/default/global/common-links.html.tmpl

>+[% IF languages.size > 1 %]
>+<ul>
>+  [% FOREACH lang = languages.sort %]
>+    <li>[% IF NOT loop.first %]<span class="separator"> | </span>[% END %]
>+    [% IF lang == current_language %]
>+      [% lang | upper %]

  Make it bold, too.

>+    [% ELSE %]
>+      <a href="" onclick="set_language_cookie('[% lang %]');">[% lang | upper %]</a>

  You could make the href a javascript: href, and then it would be valid HTML.

>+<div class="clear_links"></div>

  I'm not sure that's necessary, is it?

Comment 20

[Jacques Supcik]

Attachment: v2

Comment 21

[Jacques Supcik]

(In reply to comment #19)
> Just access Bugzilla.languages in the templates directly.

Done

> I'm concerned this might be slow, and we're doing it on every page. Is
> include_languages cached? If not, is it possible to just get this out of the
> template object again somehow?

Humm... this might indeed be slow, but because each page can change the
language, I guess that we have to parse the cookie for each page as well.
Do you have a suggestion to make it faster? 

> No, don't access HTTP_COOKIE directly. Use the cookie methods from
> Bugzilla::CGI. See how we do it in Auth/Persist/Cookie.pm

Done. But I'm still checking for the "HTTP_COOKIE" environment variable because
this module is also used for "checksetup.pl" and then I think that
Bugzilla->cgi is not defined. Do you have a "cleaner" way to find out if we
are running "checksetup.pl"?

Another point is that I had to do "require Bugzilla" in the if statement. A
"use Bugzilla" at the top of the code broke "checksetup.pl"

> Also, we should probably just call the cookie something like 'LANG'. We only
> use "Bugzilla_something" for the id and login right now, and
> ALL_CAPS_SOMETHING for the other cookies.

Done

> You should use the YUI cookie methods for this instead. See how I did it in
> js/tui.js

Done. But I had to add "js/yui/cookie.js" in the global header. I hope that
this is OK.

>  Instead of modifying all this, I think you should just be only adding
> .lang_links in global/header.html.tmpl.

These are not so really changes, but additional selectors. These additional
styles control the position (float:left|right) and take into account the fact
that the links (as <ul> list) are now inside a <div>.

But maybe I did not understand your remark well.

> Make it bold, too.

Done

> You could make the href a javascript: href, and then it would be valid HTML.

Done. I used "javascript:void(0)"

> I'm not sure that's necessary, is it?

Yes it is. Otherwise there is a problem with the size or the container <div>.
But maybe there is another way to solve it.

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> Humm... this might indeed be slow, but because each page can change the
> language, I guess that we have to parse the cookie for each page as well.
> Do you have a suggestion to make it faster? 

  Parsing the cookie for each page is fine. But running include_languages twice or more on a single page is not, really, unless its return value is cached in Bugzilla->request_cache.

> Done. But I'm still checking for the "HTTP_COOKIE" environment variable because
> this module is also used for "checksetup.pl" and then I think that
> Bugzilla->cgi is not defined. Do you have a "cleaner" way to find out if we
> are running "checksetup.pl"?

  i_am_cgi() will tell you if you are running in a CGI or not.

> Another point is that I had to do "require Bugzilla" in the if statement. A
> "use Bugzilla" at the top of the code broke "checksetup.pl"

  Never "use" or "require" Bugzilla inside of a module--our top-level CGIs always do that for you.

> Done. But I had to add "js/yui/cookie.js" in the global header. I hope that
> this is OK.

  Probably it's OK. I'll look at the patch and let you know.

> Yes it is. Otherwise there is a problem with the size or the container <div>.
> But maybe there is another way to solve it.

  Yeah, there should be. We should never have to resort to extraneous HTML to fix CSS problems.

Comment 23

[Jacques Supcik]

Attachment: V3

Comment 24

[Jacques Supcik]

(In reply to comment #22)
> Parsing the cookie for each page is fine. But running include_languages twice
> or more on a single page is not, really, unless its return value is cached in
> Bugzilla->request_cache.

OK. I couldn't find a better way, so I cached the value.

> i_am_cgi() will tell you if you are running in a CGI or not.

OK. I used it.

> Never "use" or "require" Bugzilla inside of a module--our top-level CGIs
> always do that for you.

OK

> Yeah, there should be. We should never have to resort to extraneous HTML to
> fix CSS problems.

Done.

Comment 25

[Max Kanat-Alexander]

Comment on attachment 390308 [details] [diff] [review]
V3

>Index: Bugzilla/Template.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v
>retrieving revision 1.102
>diff -u -r1.102 Template.pm
>--- Bugzilla/Template.pm	16 Jul 2009 01:30:48 -0000	1.102
>+++ Bugzilla/Template.pm	23 Jul 2009 20:31:56 -0000
>@@ -723,6 +723,14 @@
>             # Currently logged in user, if any
>             # If an sudo session is in progress, this is the user we're faking
>             'user' => sub { return Bugzilla->user; },
>+           
>+            # Currenly active language
>+            'current_language' => sub {
>+                my $cache = Bugzilla->request_cache;
>+                $cache->{include_languages} ||= [include_languages()];

  Hmm. Why not cache it inside include_languages itself?

>Index: Bugzilla/Install/Util.pm
>+use Bugzilla::Util;

  You can't "use Bugzilla::Util" in this file. You'll just have to copy the function into this file. (Not move it, just copy it.)

>Index: js/global.js
>+        expires: new Date('January 1, 2038'),
>+        path: '/'

  No, that's the wrong path. Do it the same way I do it in tui.js.

>Index: skins/standard/global.css
>+    #header div.links {

  Generally I think it's best not to specify the type of element unless you have multiple different elements with the same class.

>-        padding: 0.5em;
>+        padding: 0.5em 0.5em 0.8em 0.5em;

  Why add more padding on the bottom?

>+    #header div.lang_links {
>+        text-align: right;
>+        padding-top:0.25em;

  Nit: Space after the colon.

>+    span.lang_current {
>+        font-weight: bold;
>     }

  Nit: No need to specify that it's a <span>.

>-    ul.links {
>+    .links ul {

  This change affects plugins, so I'd rather not do it if we don't have to. Couldn't you make the container div named something like "links-container"?

>Index: template/en/default/global/common-links.html.tmpl
>+[%# Languages selector. Only on the top bar %]
>+[% IF qs_suffix == "_top" %]

  Instead of doing this, it really seems like you could just add this in global/header.html.tmpl. I do recall you explained something about it, but I didn't quite get it (or I wasn't convinced, or something?).

>Index: template/en/default/global/header.html.tmpl
>     <script src="js/yui/yahoo-dom-event.js" type="text/javascript"></script>
>+    <script src="js/yui/cookie.js" type="text/javascript"></script>

  Cool, that's fine, but make sure to remove the cookie.js from every other template.

Comment 26

[Jacques Supcik]

Attachment: V4

> Hmm. Why not cache it inside include_languages itself?

OK. I did it. But I had to make sure that I was not running checksetup.pl
and that the function was called without any argument. I used a flag
($to_be_cached) instead of checking again SERVER_SOFTWARE and @_ at the
end because it is quite far away from the begin of the procedure and it is
hard to ensure that @_ has not been altered.

> You can't "use Bugzilla::Util" in this file. You'll just have to copy the
> function into this file. (Not move it, just copy it.)

OK. But it was just one line (checking $ENV{SERVER_SOFTWARE}) so I just used
it in-line. I hope that this is OK.

>  No, that's the wrong path. Do it the same way I do it in tui.js.

Done.

> Generally I think it's best not to specify the type of element unless you
> have multiple different elements with the same class.

OK. The problem is that in the header the <ul> has also the class "links".
The rounded corners and the border width is for the <div> and not the <ul>

> Why add more padding on the bottom?

It looked better. At least to me and on my browser. But you don't seem to
like it, so I changed it back :-)

> Nit: Space after the colon.

Fixed.

> Nit: No need to specify that it's a <span>.

Fixed.

> This change affects plugins, so I'd rather not do it if we don't have to.
> Couldn't you make the container div named something like "links-container"?

Done.

> Instead of doing this, it really seems like you could just add this in
> global/header.html.tmpl. I do recall you explained something about it, but I
> didn't quite get it (or I wasn't convinced, or something?).

Done. Good idea!

> Cool, that's fine, but make sure to remove the cookie.js from every other
> template.

Done.

Comment 27

[Frédéric Buclin]

Max, have time to review it? It would be nice to have for 3.6.

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> Max, have time to review it? It would be nice to have for 3.6.

  Yeah, I should eventually before 3.6.

Comment 29

[Max Kanat-Alexander]

Comment on attachment 390559 [details] [diff] [review]
V4

>Index: Bugzilla/Template.pm
>+            # Currenly active language
>+            'current_language' => sub {
>+                my ($language) = include_languages();
>+                return $language;

  Don't we already have Bugzilla->language to indicate what the current language is?

>Index: Bugzilla/Install/Util.pm
> sub include_languages {
>+    # If we are in CGI mode (not in checksetup.pl) and if the function has
>+    # been called without any parameter, then we cache the result of this
>+    # function in Bugzilla->request_cache. This is done to improve the
>+    # performance of the template processing.

  That seems like something we should do in a separate bug, but it's OK to have it here too.

>+    .common_links_container {
>+        float: left;
>+    }

  That's not an ideal class name, because common-links appears in both the header and footer.

>Index: template/en/default/global/header.html.tmpl
>+      <a href="javascript:void(0)" onclick="set_language_cookie('[% lang %]');">[% lang | upper %]</a>

  Usually we do href="#" for void links.

  Also, it might be nice to have a CGI-based way to set this cookie as well, for non-JS users (not that non-JS users are that important). Otherwise, this menu shouldn't be visible if JS is off (easily accomplished by adding a bz_default_hidden class and then removing it via JS).

  Otherwise (though I haven't tested this yet) this looks fine. Do you want to post a new patch with these things adjusted?

Comment 30

[Frédéric Buclin]

Comment on attachment 390559 [details] [diff] [review]
V4

>Index: Bugzilla/Template.pm

>+            # Currenly active language
>+            'current_language' => sub {
>+                my ($language) = include_languages();
>+                return $language;
>+            },

Is this really useful? You only need it to decide if a language shortname should be linkified or not. IMO, to avoid complexity, we should simply linkify language shortnames all the time. The user knows what the current language is, and it doesn't matter if he clicks the current language link again.



>Index: template/en/default/global/header.html.tmpl

The indentation should be fixed.

>+    [% IF lang == current_language %]

This is the single place which needs current_language(). IMO, this is overkill, as said above.


>+      <span class="lang_current">[% lang | upper %]</span>

Does this pass our tests? I'm pretty sure 008filter.t doesn't understand |. Should be FILTER upper, for consistency.


I will test the patch before committing it.

Comment 31

[Jacques Supcik]

> Is this really useful? You only need it to decide if a language shortname
> should be linkified or not. IMO, to avoid complexity, we should simply linkify
> language shortnames all the time. The user knows what the current language is,
> and it doesn't matter if he clicks the current language link again.

Right. For me, it was nicer to somehow highlight the current language (it seems that almost all multi-lingual web sites does so) and it also gives a feedback to the user that his action (changing the current language) actually worked, even if the page has not yet been translated.

> The indentation should be fixed.

OK. I'll do it

> Does this pass our tests? I'm pretty sure 008filter.t doesn't understand |.
> Should be FILTER upper, for consistency.

OK, I'll fix it.

Comment 32

[Max Kanat-Alexander]

Also, "FILTER upper" is not a security filter--it also needs FILTER js or FILTER html as appropriate.

I'll hold off on checkin until we get an updated patch.

Comment 33

[Jacques Supcik]

Attachment: V5

> Don't we already have Bugzilla->language to indicate what the current
> language is?

As far as I understand, Bugzilla->languages gives the list of available languages, but there is no Bugzilla->language. But I might be wrong...
 
> That's not an ideal class name, because common-links appears in both the
> header and footer.

O.K. I renamed "common_links_container" to "header_links_container". Actually I called it so to allow having the language selector in the footer as well. It is just a "link container" and it is up to the template designer to use it in the header, in the footer, or in both. But I agree that it might never be used in the footer.

>  Usually we do href="#" for void links.

OK. Fixed.

> Also, it might be nice to have a CGI-based way to set this cookie as well,
> for non-JS users (not that non-JS users are that important). Otherwise, this
> menu shouldn't be visible if JS is off (easily accomplished by adding a
> bz_default_hidden class and then removing it via JS).

Actually I did it so in V1 and you told me to use JS (see Comment #19). So I followed your suggestion and I used JS to unhide it. 

> The indentation should be fixed.

Fixed. But good indent is not easy in TT/HTML so I would be pleased if you could check it again and give me some feedback.

> Does this pass our tests? I'm pretty sure 008filter.t doesn't understand |.
> Should be FILTER upper, for consistency.

Fixed. And I also now passes the test.

Besides this, I had to add an empty <div> with the style "clear:both" in the header.html.tmpl. In Comment #24 I wrote that I found another way to solve it but it did not work when the language selector block was hidden (when only one language is installed). I would need some help from a CSS guru to get it right without the additional <div>.

Concerning the piece of code used to not "linkify" the selected language, I still think that the result is nicer so and that the result is worth the effort. But if you really insist, I will (reluctantly) remove it.

Comment 34

[Max Kanat-Alexander]

I think de-linkifying the current language is important.

I agree that having a CGI way to set the language is not so important, we can require JS for that. :-)

Comment 35

[Max Kanat-Alexander]

Comment on attachment 412438 [details] [diff] [review]
V5

Okay, this is great. I have a version with the HTML slightly moved around and the CSS fixed up, and I'm going to attach that here and check it in.

Here's what was going on with your CSS:

1) Floats must have a fixed size (or be wrapped in a table), or they don't work, because they are 100% of the width of the screen and don't "float" anywhere.

2) For a "float: right", the HTML has to be *above* the object it should float to the right of.

Comment 36

[Max Kanat-Alexander]

Attachment: v6

Here's what's getting checked in.

Comment 37

[Max Kanat-Alexander]

Thank you so much for this patch, Jacques! It's really nice! :-)

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.114; previous revision: 1.113
done
Checking in Bugzilla/Install/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Util.pm,v  <--  Util.pm
new revision: 1.20; previous revision: 1.19
done
Checking in js/global.js;
/cvsroot/mozilla/webtools/bugzilla/js/global.js,v  <--  global.js
new revision: 1.4; previous revision: 1.3
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.69; previous revision: 1.68
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.100; previous revision: 1.99
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.65; previous revision: 1.64
done

Comment 39

[Max Kanat-Alexander]

Added to the release notes in bug 547466.