Opened 4 years ago

Closed 4 years ago

#16937 closed defect (fixed)

The browser.startup.homepage pref has been translated in the Korean bundle

Reported by: boklm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201510R
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In the file src/chrome/locale/ko/browser.properties from torbutton.git, the browser.startup.homepage pref which should be about:tor has been translated. Because of this, the about:tor page is not loaded when starting the Korean bundle.

We should probably remove from Transifex the things we don't want translators to translate.

Child Tickets

Change History (20)

comment:1 Changed 4 years ago by mcs

Cc: brade mcs added
Keywords: TorBrowserTeam201509R added; TorBrowserTeam201509 removed
Status: newneeds_review

Kathy and I created a patch to fix the browser.properties files (fix ko strings, use '-' instead of '_' in the spellchecker.dictionary values, remove unused search engine strings from all translations). The patch is here:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug16937-01&id=0725f976b560b77b0ff00fea7c6ea29f6afdd184

We would also like to do something to prevent this problem in the future, but do not want to over-engineer things. We could:

  1. Do nothing.
  2. Remove the browser.startup.homepage string from browser.properties. If we do this, we would need to figure out how to override the browser.startup.homepage pref. in a different way. Or we would need to post-process the browser.properties files during the Torbutton build process to add the browser.startup.homepage property back in. Note that we would still have a potential problem with the spellchecker.dictionary string which was also translated into Korean.
  3. Modify trans_tools/import-translations.sh to complain loudly if the certain strings look wrong when we pull them in from Transifex.

Opinions? Kathy and I do not think approach 2 is worth the effort. Approach 3 would be ugly but would be helpful, assuming that the person who updates the translations notices that import-translations.sh failed.

comment:2 Changed 4 years ago by gk

Can't we do something like I did in #11236 (comment:9:ticket:11236): if the value is locale independent AND it must not get translated AND this is the value for a preference in Firefox then move that into 000-tor-browser.js? That would avoid language string bloat and accidental translation.

comment:3 in reply to:  2 Changed 4 years ago by mcs

Replying to gk:

Can't we do something like I did in #11236 (comment:9:ticket:11236): if the value is locale independent AND it must not get translated AND this is the value for a preference in Firefox then move that into 000-tor-browser.js? That would avoid language string bloat and accidental translation.

Kathy and I are trying to keep all of the about:tor functionality together (inside Torbutton). But it looks like the browser.startup.homepage override is in Torbutton anyway (which makes sense), so maybe we can just do as you suggest. We will try it and post a revised patch soon.

comment:4 Changed 4 years ago by mcs

It turns out that the default values for browser.startup.homepage and spellchecker.dictionary cannot be directly placed in a prefs file (such as defaults/preferences/
preferences.js within Torbutton). This is because the browser retrieves them using the getComplexValue() prefs API. So we must continue to use a level of indirection to a properties file.

If we just want to solve the about:tor problem, we can put the value in a file such as src/chrome/content/non-localized.properties within Torbutton, change defaults/preferences/
preferences.js to point to that new file, and then we are done.

If we also want to avoid bad translations of the spellchecker.dictionary values, we should take those out of translators hands as well. But we do want different values for different locales, so solving this is a harder. We could:

  1. Remove the spellchecker.dictionary values entirely, in which cases users will get en-US spellchecking and they will need to choose their dictionary after they download and install it.
  2. Create a parallel hierarchy of locale files within Torbutton that do not go to/from Transifex, e.g., src/chrome/locale-not-for-translators/. The makexpi.sh script would need to be modified to merge the new files into the torbutton xpi so the Mozilla code will find them, but that should not be too difficult. Introducing a parallel locale hierarchy would be ugly because we have a lot of locales.
  3. Change our import-translations.sh script to ignore browser.properties so it will never be overwritten with changes from Transifex and de-list that file at Transifex. Since it would be confusing to developers to have a file under locale/*/ within Torbutton that is not maintained via Transifex, we would probably want to rename the file to something like not-for-localizers.properties. Then we would have:

src/chrome/locale/af/not-for-localizers.properties
src/chrome/locale/ak/not-for-localizers.properties
...
src/chrome/locale/en/not-for-localizers.properties
etc.

Opinions? Does anyone have a better idea?

comment:5 in reply to:  4 Changed 4 years ago by gk

Replying to mcs:

It turns out that the default values for browser.startup.homepage and spellchecker.dictionary cannot be directly placed in a prefs file (such as defaults/preferences/
preferences.js within Torbutton). This is because the browser retrieves them using the getComplexValue() prefs API. So we must continue to use a level of indirection to a properties file.

Given that a lot of our branding is in tor-browser/branding/official (icons for exmaple) and this is the place Mozilla has for such general, browser related things: what speaks against patching locales/browserconfig.properties to point to about:tor instead of about:home? This would leave all the implementation of that homepage in Torbutton (which is fine to me), would use the same mechanism Mozilla has for these things, would omit an additional file in Torbutton and, above all, would avoid the localization problem.

Last edited 4 years ago by gk (previous) (diff)

comment:6 in reply to:  4 Changed 4 years ago by gk

Replying to mcs:

If we also want to avoid bad translations of the spellchecker.dictionary values, we should take those out of translators hands as well. But we do want different values for different locales, so solving this is a harder. We could:

  1. Remove the spellchecker.dictionary values entirely, in which cases users will get en-US spellchecking and they will need to choose their dictionary after they download and install it.

Okay, I spent quite some time here. I think we should go with this plan. The reason why they actually get en-US spellchecking is that we ship that dictionary in all locales. So, shipping this and then setting the spellchecker.dictionary pref to the respective bundle locale does not make much sense to me. If we want to the user to spellcheck in the bundle locale it is in fact easier to just ship the en-US dictionaries in the en-US bundle and let the user download the missing dictionary (which they need to do anyway) other ones. This ensures that they get the spellcheck locale activated right away without the need to have spellchecker.dictionary at all. If we, on the other hand, want to allow the user to spellcheck with en-US dictionaries then the spellchecker.dictionary pref is superfluous (too).

comment:7 Changed 4 years ago by brade

Replying to gk:

Given that a lot of our branding is in tor-browser/branding/official (icons for exmaple) and this is the place Mozilla has for such general, browser related things: what speaks against patching locales/browserconfig.properties to point to about:tor instead of about:home? This would leave all the implementation of that homepage in Torbutton (which is fine to me), would use the same mechanism Mozilla has for these things, would omit an additional file in Torbutton and, above all, would avoid the localization problem.

In the past, we made an explicit decision to keep all of the components for the home page in Torbutton. Separating the home page setting from the actual page would cause things to break if the user ever disables Torbutton. (If the setting is in Torbutton and Torbutton is disabled, the user gets Firefox's default "about:home" page rather than a broken page.)
I think we should keep the setting with the implementation of about:tor (in Torbutton).

In regards to the spellchecker.dictionary pref values, I am OK with removing them, but removing them means that the spellchecker will automatically do red-underlining based on the built-in English dictionary which is less than ideal. [Note: today the spellchecker.dictionary values contain underscores which Firefox doesn't handle which means no one gets spellchecking by default.]

My preference is for option 3 from comment:4. We should remove the entire file from Transifex and not let the localizers touch the file. The spellchecker pref shouldn't blindly be set. For example, there isn't a dictionary for Japanese and I suspect localizers wouldn't realize that and they would set it anyway.

comment:8 in reply to:  7 Changed 4 years ago by gk

Replying to brade:

Replying to gk:

Given that a lot of our branding is in tor-browser/branding/official (icons for exmaple) and this is the place Mozilla has for such general, browser related things: what speaks against patching locales/browserconfig.properties to point to about:tor instead of about:home? This would leave all the implementation of that homepage in Torbutton (which is fine to me), would use the same mechanism Mozilla has for these things, would omit an additional file in Torbutton and, above all, would avoid the localization problem.

In the past, we made an explicit decision to keep all of the components for the home page in Torbutton. Separating the home page setting from the actual page would cause things to break if the user ever disables Torbutton. (If the setting is in Torbutton and Torbutton is disabled, the user gets Firefox's default "about:home" page rather than a broken page.)
I think we should keep the setting with the implementation of about:tor (in Torbutton).

Okay, that is a good argument. I am fine with that plan.

In regards to the spellchecker.dictionary pref values, I am OK with removing them, but removing them means that the spellchecker will automatically do red-underlining based on the built-in English dictionary which is less than ideal. [Note: today the spellchecker.dictionary values contain underscores which Firefox doesn't handle which means no one gets spellchecking by default.]

My preference is for option 3 from comment:4. We should remove the entire file from Transifex and not let the localizers touch the file. The spellchecker pref shouldn't blindly be set. For example, there isn't a dictionary for Japanese and I suspect localizers wouldn't realize that and they would set it anyway.

I totally agree that the file should be out of reach for translators I am just not sure how the spellcheck part should work in practice. Do we really want to ship en-US dictionaries to everybody just to set the spellchecker.dictionary pref to the bundle locale in order to not get the en-US spellchecking going? (I agree this would be suboptimal). If the answer is, "yes", then why? If, "no", then let us omit the dictionaries (for non-us bundles) and don't bother with the pref.

Last edited 4 years ago by gk (previous) (diff)

comment:9 Changed 4 years ago by mcs

It seems that Mozilla ships a dictionary with some but not all of their localized Firefox builds (a page I found hints that for licensing reasons they may not be allowed to directly bundle some dictionaries). For example, the French builds include a fr dictionary but the es-ES builds do not include a dictionary at all. Some maybe we should:

  1. Not set the spellchecker.dictionary pref. (Mozilla does not).
  2. Not ship a dictionary with our non en-US builds (currently, we ship the en-US dictionary with all of our builds).

comment:10 Changed 4 years ago by mcs

The dictionary selection code seems to be in nsEditorSpellCheck::DictionaryFetched(), e.g.,
http://mxr.mozilla.org/mozilla-esr38/source/editor/composer/nsEditorSpellCheck.cpp#712

comment:12 Changed 4 years ago by mikeperry

The patch seems Ok to me. I just pushed it for 5.5a3 for a local test build. Do we think this should also go into 5.0 now, too?

Also, are the non en-US dictionaries in the langpack XPIs? And if not, does it fall back to the OS dictionary, or get it from elsewhere/download it at that stage? The search order wrt that is still a little unclear to me, given all the LANG, pref, and local checks that are in the way of it.

comment:13 Changed 4 years ago by yawning

~If we do ship the ja-JP locale in a 5.0 release, it would be nice for this patch to be included since first impressions are important, and a messed up about:tor page won't provide that good of one.~

~Maybe look and see what happens in the 5.5a3 build?~

Oops. Wrong ticket sorry!

Last edited 4 years ago by yawning (previous) (diff)

comment:14 in reply to:  12 Changed 4 years ago by gk

Replying to mikeperry:

The patch seems Ok to me. I just pushed it for 5.5a3 for a local test build. Do we think this should also go into 5.0 now, too?

I wonder if we shouldn't wait for an alpha and check whether there are user complaints about the spellchecking things. I remember e.g. vaguely that we had shipped the en-US dictionary even in the german profile back in my JonDos days as this was actually pretty convenient for non en-US speakers and removing it caused some complaints. While I think the patch does the right thing there might be pushback that shows I am wrong here...

comment:15 in reply to:  12 Changed 4 years ago by mcs

Replying to mikeperry:

The patch seems Ok to me. I just pushed it for 5.5a3 for a local test build. Do we think this should also go into 5.0 now, too?

Like Georg, I would say "not yet." Let's see what feedback we get from our alpha users. For 5.0.3, if you like, you could at the last minute edit src/chrome/locale/ko/browser.properties and set browser.startup.homepage=about:tor (a short term fix).

Also, are the non en-US dictionaries in the langpack XPIs? And if not, does it fall back to the OS dictionary, or get it from elsewhere/download it at that stage? The search order wrt that is still a little unclear to me, given all the LANG, pref, and local checks that are in the way of it.

I don't think any dictionaries are included in the langpacks. Users will need to use the "Languages|Add Dictionaries" context menu item (or the add-ons manager) to add a dictionary. The behavior is just that users do not have a spellchecker until they install a dictionary. Apparently there are some licensing restrictions that prevent Mozilla from bundling the dictionaries, at least for some languages.

comment:16 Changed 4 years ago by gk

Moving needs_review tickets to October 2015.

comment:17 Changed 4 years ago by gk

Keywords: TorBrowserTeam201510R added; TorBrowserTeam201509R removed

Batch modification for realz now.

comment:18 Changed 4 years ago by gk

Severity: Normal
Status: needs_reviewneeds_information

mcs, brade: Do you think we should backporting that one now given an alpha release without complaints? I am inclined to say "yes".

comment:19 in reply to:  18 Changed 4 years ago by mcs

Replying to gk:

mcs, brade: Do you think we should backporting that one now given an alpha release without complaints? I am inclined to say "yes".

Yes, I agree. Do you have time to backport or should I do it? If you want me to do it, please let me know which are the correct destination branches for Torbutton and builders/tor-browser-bundle.

comment:20 Changed 4 years ago by gk

Resolution: fixed
Status: needs_informationclosed

Thanks, I've done it with commits 0b7e7fc8ce80057cf1e909b67a7e49ce18a62b9a (maint-1.9.3) and 984992951026f3bee2766b8c922665590a30c1d3 (maint-5.0).

Note: See TracTickets for help on using tickets.