Given that we will be shipping a uni-lingual (English) build, part of the first-run experience must be to let a user choose the language in which he or she would like to use Fennec.
Design work proceeding on the feature page: https://wiki.mozilla.org/Fennec/Features/langchoice

I saw the Brian's mocks but I think it doesn't work well:
https://wiki.mozilla.org/images/b/b6/LanguagePacks_Ideal1stRunUX.pdf
Please note that many of non-English user cannot understand any English label.
So "Continue in English" or "Choose Different language" menu doesn't work.
Firstrun page itself should be localized by default depending on the system locale. If it's difficult to load langpack before we see the firstrun, only the page should be multi-language even in the English build.

(In reply to comment #1)
> Firstrun page itself should be localized by default depending on the system
> locale. If it's difficult to load langpack before we see the firstrun, only the
> page should be multi-language even in the English build.
I was thinking that we would put localized strings in the en-US locale for as many languages as we could. We would only fallback to en-US strings if we didn't have a localized version.
We could also try pulling the string from the langpack XPI.

I believe the intended approach is to include those basic strings ("continue in [language name]", etc.) in the set of languages available for android. That way, we can detect at least the OS language and match that. Continuing will get the user the full langpack. For users in locales where Firefox has a translation that is not available for the OS, at least the user will see the first message in a language they're likely to understand, given that it's what the phone is set to.

Is this a one time offer for the user? What happens if Fennec don't yet have a localization that matches the OS locale, and the user is forced to choose another language. Will the user get notified if a more suitable Fennec lang pack becomes available?

(In reply to comment #2)
> I was thinking that we would put localized strings in the en-US locale for
> as many languages as we could. We would only fallback to en-US strings if we
> didn't have a localized version.
I think language (locale) list like Mac OS X installer's first dialog ("Use English for the main language" label in every locales) is better for the fallback, not en-US UI.
(In reply to comment #4)
> Is this a one time offer for the user? What happens if Fennec don't yet have
> a localization that matches the OS locale, and the user is forced to choose
> another language. Will the user get notified if a more suitable Fennec lang
> pack becomes available?
Uses can switch the locale in the config page. But I agree it's better to offer once again when the OS locale is switched or when new locale become available.
# it may be another bug?

This needs love, but sorta works. Need to get a repo that has some working language addons to really test with.
We will eventually also need to ship strings in the en-US locale from all of our supported locales. I started to work on a script to do this (Probably buried somewhere in the WIP), and with some work on the build script side we can go that way (they will have download all of the language repos for us just to build the en-US one).
Or we can start looking into some alternatives like just keeping these strings in the en-US repo or doing this through some sort of "hidden" addon.

Not sure what you wanted on the last one. I haven't attempted to clean this patch up at all, but overall it seems to work fairly well. I have a (slowly growing) list of little problems to clean up with this before I put it up for review or feedback.

I've pulled out most of the build system changes required here and put them in a separate patch (coming in a second). Beyond that, this works fairly well. I'll post some screenshots up in flickr in a minute.

These are much more WIP quality, but I'd like feedback on the approach at least. We'll also have to make changes to mozharness for this. Does this look like an ok approach mfinkle, or would you rather I look for a different route?

Flickr set:
http://www.flickr.com/photos/digdug2k/5727250593/in/set-72157626736328448/
Also I should note two things:
This should update the UI language whenever you select a language. If there is no localized string for a particular string, we should show the en-US string. Since this only has en-US strings right now, it only ever shows en-US strings.
Also, the pref is downloading the list from my dropbox. I did that because the amo links I had either gave me nothing compatible, or a single compatible localization which doesn't actually exist. Its a pref now. Easy to change.

Build to play with:
http://dl.dropbox.com/u/72157/fennec-localePicker.apk
This will only show when a new profile is created, so you may have to clear your data every time you want to play with it. You can try going to about:locales, but I haven't tested there at all.

Thanks for the quick review! V2 fixes most of what you asked for. To recap and answer a few questions along the way:
* Only show if the fennecLocale != systemLocale. Hope this isn't to contentious?
* Moved LocaleRepository to a .jsm, and cleaned a bunch of useless stuff out. Probably more can go.
* +Cu.import("resource://gre/modules/NetUtil.jsm"); in LocaleRepository.jsm
NetUtils is needed (unless we want to unneed it)
* Cu.import("resource://gre/modules/Geometry.jsm"); moved to input.js
This is needed by input.js (which we need for scrolling).
* So these are the strings we would need to have localized?
Yep. A few are less necessary than others though.
* Moved global variables, as well as a bunch of functions into LocaleUI. (Artifacts from an older version)
* Do we want to fallback to en-US ?
In updateString I call getString with the requested locale. If that throws, I call back in with the system locale. If that fails, I try one last time with the current fennec locale. Fixed a few bugs here. I've left some es-US (Espanol) strings that were translated online for me here for testing as well. I'll pull them out before pushing.
> Why do we need a restart?
Some of our browser startup code must rely on being called early. If I just close one window and open the other, the sidebars are positioned incorrectly and there is no browser window. If I then bring up the awesome screen and then switch back things are fine. We can probably make this work. Just need to dig into where the problems are (likely near the resize handler).
> Given how much trouble we have with about:config in XUL, this frightens me
Trying to keep the scrolling code simple. input.js works well with nsIScrollBoxObjects, so I stuck with XUL. This is more like the addons pane than about:config though. browser.xul isn't even loaded here.
> I don't think we want to use this. Why not use the old code (with the indent fixed)? (r.e. BrowserCLH changes)
We will already have a profile the second time around, but still need to show about:firstrun. I don't know of a better way to force this.

(In reply to comment #18)
Some replies before the next review:
> * Only show if the fennecLocale != systemLocale. Hope this isn't to
> contentious?
Seems like we want to always show the language selector window on first run
> * Cu.import("resource://gre/modules/Geometry.jsm"); moved to input.js
> This is needed by input.js (which we need for scrolling).
Moving is good
> > Why do we need a restart?
> Some of our browser startup code must rely on being called early. If I just
> close one window and open the other, the sidebars are positioned incorrectly
> and there is no browser window. If I then bring up the awesome screen and
> then switch back things are fine. We can probably make this work. Just need
> to dig into where the problems are (likely near the resize handler).
OK. I agree with doing the restart now and looking into how we can stop doing it later.
> > Given how much trouble we have with about:config in XUL, this frightens me
> Trying to keep the scrolling code simple. input.js works well with
> nsIScrollBoxObjects, so I stuck with XUL. This is more like the addons pane
> than about:config though. browser.xul isn't even loaded here.
I completely missed the fact that this window loaded before browser.xul. I was assuming it was an about:locales page, loaded in the browser. Do we need about: page support? Maybe for later?
> > I don't think we want to use this. Why not use the old code (with the indent fixed)? (r.e. BrowserCLH changes)
> We will already have a profile the second time around, but still need to
> show about:firstrun. I don't know of a better way to force this.
We no longer want to use about:firstrun at all. We will be moving straight to about:home (or whatever the home page is set to be)
That will be filed as a separate bug, but we can worry less about firstrun in this bug.
on to the review...

Incorporates changes from a talk with mfinle, bdils, and madhava. Added a background logo image behind the main section (the current splash we use has a black background so I can't use it). Now shows installed locales in the list as well as the downloaded ones. Removed button icons. Always shows when a new profile is made. A few other little tweaks here and there (resizing should be better).
I also made the matching algorithm for locales a bit more complex. On my phone the system has an en-US locale and an es-US locale (US Spanish). We ship en-ES (spanish Spanish). I think, unless there is an es-US locale in Fennec, those should probably match? Added a function to check that and to bring up the issue.
Haven't tested this on my actual phone yet, but will as soon as I get to work. Wanted to put it up anyway to make sure things kept rolling.

Comment on attachment 533866[details][diff][review]
Patch v4
>diff --git a/mobile/chrome/content/aboutLocales.js b/mobile/chrome/content/aboutLocales.js>+ _showPanel: function() { },
Do we need this?
>+ while(!string && localeIndex < potentialLocales.length) {
Add a space between the while and (
Your use of "elt" and elts" seems to make me sad. Please use full name, it makes it easier to read:
"element" (as in aPref.element) and "elements"
>+}
>\ No newline at end of file
Add newline
Does this really need to be an about: page? Can't we just use a chrome:// URI?
File followup bugs for:
* Why does the UI get horked if we close and open a window?
* Passing the commandline URL through to the locale window and use it during the restart
r+ with nits fixed and bugs filed
If we want to turn this off later, on Aurora, removing (or commenting out) the code from BrowserCLH.js would seem to do the job, right?

Sorry to add another round on this. This undoes all of my fancy language picking stuff. Instead we are shipping the locale strings normally and refreshing the chrome when you select one. Its a bit tricky maintaining state across refreshes, so I added a pref "localepicker.skipMainPage" to tell us whether not to show the locale list or the main page. i.e. if you tap on Italian we refresh the page, but jump straight to the list.
For downloaded locales, hopefully AMO will send us strings in the future. I haven't addressed that issue here beyond changing the locale strings to read "Continue in Greek" after you install the Greek package.
Also adds a pref "browser.firstrun.show.localepicker" that can be used to hide this during browser-chrome tests.
I'm still testing this on my phone, but it works fine on desktop.

Trying very hard to get it in by then. I'm a bit busy (moving) this weekend, but I'll try to check in often and get to any changes as quickly as I can. We've set this up to be easy to pref off if we have to.

Comment on attachment 532709[details][diff][review]
Build changes
The build stuff here will need more work to support l10n-merge.
i.e., the localized file can be in three different locations, in the LOCALE_MERGEDIR, in the localized srcdir, or in en-US src.
Also, I think you want to move the
+ @$(MAKE) localepicker
into the chrome: target so that it's also run for en-US, and not just repacks?
There are parts of this patch that I didn't try to understand, but r- based on the lack of l10n-merge support here.

(In reply to comment #28)
> We may be able to use common pool for downloading localized strings in
> addition to those we ship. Gandalf?
Yeah, once we have CP on production server we should be able to deliver data to Firefox as well :)

Comment on attachment 534143[details][diff][review]
Patch v5
>+// True if this is the first time we are showing about:firstrun
>+pref("browser.firstrun.show.uidiscovery", true);
>+pref("browser.firstrun.show.localepicker", true);
>+
>+// Used when reloading the locale picker to skip the main page and go directly to the locales list
>+pref("localepicker.skipMainPage", 0);
>\ No newline at end of file
Add a new line
>diff --git a/mobile/chrome/content/firstrun/firstrun.xhtml b/mobile/chrome/content/firstrun/firstrun.xhtml> function init() {
>+ let prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService)
>+ .QueryInterface(Ci.nsIPrefBranch2);
Services.prefs ?
if you keep the raw XPCOM, no wrapping please
>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js>+ cancelPicker: function() {
>+ // restore the last known "good" locale
>+ this.language = Services.prefs.getCharPref("localepicker.lastGoodLocale");
Does this need to be a preference?
>+ cancelInstall: function () {>+ Services.prefs.setIntPref("localepicker.skipMainPage", 1);
Tell me more about this pref. Why do we need it? I'd like to remove these temporary prefs if possible.
>+ // since we keep reloading the page when the user clicks on locales, we save
>+ // the last "good" locale as a pref so that we can restore it if the user cancels
>+ // out of the dialog
>+ Services.prefs.setCharPref("localepicker.lastGoodLocale", LocaleUI.language);
Why not just keep a calss variable? And why do we need to keep reloading the page?
>\ No newline at end of file
Add one
>diff --git a/mobile/components/BrowserCLH.js b/mobile/components/BrowserCLH.js>+ // Show the locale selector if we have a new profile
>+ // XXX - if a url was passed in, we need to save it and load it after the restart
>+ if (needHomepageOverride() == "new profile" && Services.prefs.getBoolPref("browser.firstrun.show.localepicker")) {
>+ win = openWindow(null, "chrome://browser/content/localePicker.xul", "_blank", "chrome,dialog=no,all", "newProfile");
Do you need to pass "newProfile" ? Remove if you can.
>diff --git a/mobile/themes/core/localePicker.css b/mobile/themes/core/localePicker.css>+richlistitem .checkbox {
>+ width: 30px;
>+ height: 30px;
>+ list-style-image: url("chrome://browser/skin/images/check-unselected-30.png");
>+}
>+
>+richlistitem[selected] .checkbox {
>+ list-style-image: url("chrome://browser/skin/images/check-selected-30.png");
>+}
Hmm, the gingerbread patch removes the 30px checkboxes and uses 46px checkboxes. I guess we would need to make a gingerbread specific CSS file.
regarding the temporary prefs, let's talk. We could add something to the localrepository.jsm to hold that data.

This just comments out a few lines so we can do some/most of this without having to refresh the page all the time. The commented out code was added in bug 597296, purely to keep the UI from getting into strange states just because the locale pref was changed.
Wanted to put this up for my own bookkeeping. Also pushed to try to see what fails.

This patch revert the changes from bug 597296 but keep the UpdateSelectedLocale() method since it fires a message with the observer service.
I've tried running xpcshell tests and they are still working with this change.

Comment on attachment 537622[details][diff][review]
Patch v7
>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js>+function resizeHandler() {
>+ let elements = document.getElementsByClassName("window-width");
>+ for (let i = 0; i < elements.length; i++) {
>+ elements[i].setAttribute("width", Math.min(800,window.innerWidth));
>+ }
nits:
No {} needed for the "for"
Math.min(800,window.innerWidth) -> Math.min(800 ,window.innerWidth)
>diff --git a/mobile/chrome/content/localePicker.xul b/mobile/chrome/content/localePicker.xul
I'll grumble about the camel cased element IDs in this file.
>diff --git a/mobile/locales/en-US/chrome/localepicker.properties b/mobile/locales/en-US/chrome/localepicker.properties>+chooseLanguage=Choose a language
This seems to be used as a title, so "Choose a Language" is better
>diff --git a/mobile/themes/core/gingerbread/localePicker.css b/mobile/themes/core/gingerbread/localePicker.css>+window {
>+ background-color: black;
>+}
Is this needed? ::root should handle this case
>+.link {
>+ color: @color_text_link@;
>+ text-decoration: underline;
>+}
"link" seems a bit too generic. Can we change to "text-as-link" ?
>+#installerPage {
>+ background-color: black;
>+ color: white;
>+}
Can you add /* force */ to this in both files? We do this in other places where we are not using the defines
background-color: black; /* force */
color: white; /* force */
Remember to make the changes in both files (core and core/gingerbread) as needed
r+ with nits fixed and we need to wait for the platform patch to land as well

> list.loading=Loading...
The default is to use the single unicode character "…" instead "..." (I know you're going to hate this, but if you fix it you need to update the key name).
Another question: I tried to read all comments but I couldn't find that information. Is it possible to test this feature in desktop builds? If that's not possible, an updated screenshot of the page would be much appreciated.

(In reply to comment #46)
> I agree on using hellip, I disagree on the entity change, the choice of the
> ellipsis is up to the localizer anyway.
Ok, good to know
> For desktop, try installing some language packs, and then reset the firstrun
> prefs from
> http://hg.mozilla.org/mozilla-central/diff/fc090d032d45/mobile/app/mobile.
> js#l1.33. I could picture that working.
Thanks, I'll try with the next batch of builds (Gecko/20110607 7.0a1 still hasn't those keys).

Comment on attachment 537870[details][diff][review]
disable firstrun for talos via prefs (1.0)
This looks good to me, too. I would like to see a note added to the prefs section of mobile.config indicating that this pref is set so that it is easier to track. Also, if you ever want to run the dirty profile tests we'll need to find a new solution for this.

(In reply to comment #45)
> > list.loading=Loading...
>
> The default is to use the single unicode character "…" instead "..." (I know
> you're going to hate this, but if you fix it you need to update the key
> name).
>
> Another question: I tried to read all comments but I couldn't find that
> information. Is it possible to test this feature in desktop builds? If
> that's not possible, an updated screenshot of the page would be much
> appreciated.
That's actually not (currently) used in the UI. I'll file a bug to remove it for now.

In mobile/locales/en-US/chrome/localepicker.properties, what is the context of "name"? Can you add a localization note about that? Will the word be used on its own, in a list, as part of a sentence, as the first word in a sentence?

You need to log in
before you can comment on or make changes to this bug.