Fix Bug 975020 - Translation Bar and non-locale redirects don't work as expected in some cases; use the user's accept language list to fix it #1718

Merged
merged 0 commits into from Mar 11, 2014

6 participants

@kyoshino
Mozilla member

This makes the Translation Bar work even if a French user is using the English version of Chrome. Other JS code can also refer Tabzilla.user.language to get the info.

@alexgibson
Mozilla member

This makes the Translation Bar work even if a French user is using the English version of Chrome.

I've tested this locally and works as described if I use an English version of Chrome with a non-English user language 👍 Also tested in IE versions and the behavior seems consistent.

Perhaps either @jgmize or @pmclanahan can review the Python parts, but front-end changes look good to me.

@kyoshino
Mozilla member

Forgot to apply the js_escape filter.

Also, should Tabzilla.user.language be a canonical value, like fr-FR vs fr-fr? A canonicalization code in Tabzilla.setupTransbar could be used here. I'm making those changes.

@kyoshino
Mozilla member

I'll also add a simple validation in the Python bit to avoid any potential issue.

@jgmize jgmize and 1 other commented on an outdated diff Feb 24, 2014
bedrock/tabzilla/views.py
def tabzilla_js(request):
- return _resp(request, 'tabzilla/tabzilla.js', 'text/javascript')
+ str = request.META.get('HTTP_ACCEPT_LANGUAGE', '').split(',')[0].strip()
+ match = re.match(r'^([a-zA-Z]{2,3})(?:-([a-zA-Z]{2}))?', str)
@jgmize
Mozilla member
jgmize added a note Feb 24, 2014

Why are we doing a server side regex here at all, since it's just passing in an additional context variable and we're doing validation in js anyway?

@pmclanahan
Mozilla member

We do need to validate since it's untrusted information, but we may not need to go to these lengths. Also, is it possible to use the locale code from the Prefixer stuff in funfactory?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kyoshino
Mozilla member

Updated the code to use Prefixer @pmclanahan mentioned as it looked nice 😃 and removed the navigator.language fallback and redundant regex validation in JS.

One issue I've found is if the accept language is en, get_language() returns en-GB instead of en-US. It might be a non-blocker bug in funfactory.

@alexgibson
Mozilla member

There are a couple of the JS tests failing with this latest change, specifically related to the lang-switcher with path values:

expect(setup('en-us', 'fr')).toBeTruthy();
expect(setup('EN-US', 'fr')).toBeTruthy();

Would these tests still be valid scenario's given the use of Prefixer?

@kyoshino
Mozilla member

Thanks @alexgibson! Removed those tests which are no longer necessary since Tabzilla.user.language should always be a canonicalized value.

@alexgibson
Mozilla member

The main difference I see with this change is if a user's preferred language isn't supported as a locale on bedrock. Previously, a notification would not occur in this scenario, whereas now they can get redirected to the next preferred language in the list.

For example, if I have ['fj', 'en-US'] in my user language list and visit a page that is /fr, I will now get a transbar notification that will redirect to en-US. This could be seen as the correct (improved) behavior I think, since it is defaulting to the next preferred user language?

If I only have fj in the user language list and visit /fr, I also get a notification that redirects to en-US (default locale).

What do people think, should this be the correct behavior for the transbar? Any thoughts, @cmore?

@kyoshino
Mozilla member

I can confirm that behaviour. Looks like get_language() always returns en-US if the language is not in the platform settings. It's rather a bug in the funfactory code, though I think it doesn't block this change.

@kyoshino
Mozilla member

But... maybe get_language() does the right thing because it's a part of urlresolvers. We could go back to a simple Accept-Language parsing rather than relying on get_language(). Give me a sec.

@alexgibson
Mozilla member

Looks like get_language() always returns en-US if the language is not in the platform settings.

I should note that if my languages are ['fj', 'fr', 'en-US'] then I get redirected to an /fr page, which makes sense to me? Only if there are no matching languages do I seem to get redirected to en-US

@kyoshino
Mozilla member

Oh, in that case get_best_language() is working well, but this is still an issue:

If I only have fj in the user language list and visit /fr, I also get a notification that redirects to en-US (default locale).

@kyoshino
Mozilla member

So here's a simple detection again. If the accept language is fj or fj,(other lang), Tabzilla.user.language will be fj. navigator.language is also fj in that case, and the Translation Bar won't be shown as before. If the accept language is en or en,(other lang), Tabzilla.user.language will be en-US as a fallback.

The second accept language is not recognized as before, but it might be okay.

@alexgibson
Mozilla member

Hmm, so now if my languages are ['fj', 'fr', 'en-US'] I get no translation bar because it no longer finds the best language match from the list?

Perhaps we should discuss what would be best here, before changing the code again?

@chrismore
Mozilla member

Yes, let's make sure we consider all of the scenarios and we are on the same page before going back and forth with multiple pull requests.

@kyoshino
Mozilla member

I think languages Bedrock doesn't support are edge cases which could be solved in a separate bug/PR. We could consider the quality value of each language (e.g. fj,fr;q=0.7,en;q=0.3) and show an alternate message say "Would you like to see this page in French?" instead of "Would you like to see this page in your language?" to avoid confusion, if we use the second accept language.

@alexgibson
Mozilla member

I think languages Bedrock doesn't support are edge cases which could be solved in a separate bug/PR.

I agree here.

We could consider the quality value of each language (e.g. fj,fr;q=0.7,en;q=0.3) and show an alternate message say "Would you like to see this page in French?" instead of "Would you like to see this page in your language?" to avoid confusion, if we use the second accept language.

This is a bigger change and would require l10n for all locales for this new string. I'd be interested in hearing opinions from others here. Personally, I think using the preferred language list is a good improvement. But if that warrants a string change is up for discussion.

Any thoughts here, @cmore, @pmclanahan or @jgmize?

@pascalchevrel
Mozilla member

That would mean translating and storing all the language names and having localizers to update that list every time we add a new locale, I am also not sure that a simple replacement if a variable for the language would work for all locales as there may be grammar rules (inflection) that change the word for the language of other surronding words depending on the initial spelling of the word. To do that programmatically we may have to integrate in tabzilla the l20n.js library that manages grammar rules in l10n. Honestly I think it's overkill and that we shouldn't design features around edge cases.

@alexgibson
Mozilla member

So from discussion in the bug, should the best course of action be for this PR to use Prefixer, as originally suggested by @pmclanahan?

@kyoshino
Mozilla member

Prefixer looks the best solution here. I'll update the PR!

@kyoshino
Mozilla member

Actually get_best_language() in Prefixer only returns the first matching accept language. If my intl.accept_languages pref is fj, fr, en, the function returns fj and the Translation Bar still won't appear in this case. Tabzilla.user.language should have ['fj', 'fr', 'en'] instead.

I'll utilize parse_accept_lang_header() to generate the array, then iterate <link rel="alternate" hreflang="ab-CD"> to find the best possible option.

@alexgibson
Mozilla member

Are you sure? Seemed to return fr for me when I tried ['fj', 'fr', 'en-US'] ?

@kyoshino
Mozilla member

Ah true, I've got fr. Was seeing the current result... It should work.

By the way I was thinking a different case. For example, if I have ['ko', 'fr', 'en-US'] and visit http://www.mozilla.org/firefox/os/, I'm redirected to en-US instead of fr because get_best_language() returns ko but that page doesn't have the ko translation. I should be redirected to fr here because it's the second preferred language.

The same holds for the Translation Bar and that's why we might have to have an array instead of a single value.

@kyoshino
Mozilla member

So do we need to fix Prefixer and the redirector also?

@kyoshino
Mozilla member

/firefox/os/ 301 MOVED PERMANENTLY in funfactory/middleware.py
-> /ko/firefox/os/ 302 FOUND in l10n_utils/__init__.py
-> /en-US/firefox/os/ 200 OK

funfactory doesn't know about the activated locales and does the right thing. l10n_utils can probably be fixed along with the Translation Bar.

@alexgibson
Mozilla member

@pmclanahan or @jgmize - what's the easiest way for @kyoshino to match transbar behavior to the locale detection already in use on Bedrock? Can you advise here? ^

@kyoshino
Mozilla member

The updated PR changes both the translbar behaviour and the redirector. Still needs some fixes in the redirector to consider lowercase (e.g. en-us) or long (e.g. fr-FR) locale names. I'll add tests also.

@pmclanahan
Mozilla member

@kyoshino yeah... I think this is due to funfactory only knowing about our total list of supported locales, and l10n_utils looking at which locales are supported for the particular page (via the activation tags in the .lang files). Or do you think something else is going on?

@kyoshino
Mozilla member

@pmclanahan I think so, too. My updated PR tries to fix the l10n_utils part.

@sgarrity sgarrity merged commit d4d4846 into mozilla:master Mar 11, 2014

1 check passed

Details default Jenkins build 'bedrock_github' #3448 has succeeded
@alexgibson
Mozilla member

Looks like something has gone amiss here, 0 commits merged?

@kyoshino
Mozilla member

Oops, something goes wrong. Reverting...

@kyoshino
Mozilla member

And actually I had to add tests for redirects. I'll resend a pull request for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment