1851618 - Cannot remove packaged locales from the locale alternatives list (multi-locale build)

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Henry Wilkes (they/them) [:henry-x]]

In tor browser we use a multi-locale build with all locales packaged with the browser at build time.

We have the following problem which would also effect a multi-locale build of firefox.

Steps to reproduce

1. Go to "Language" in "about:preferences".
2. Click "Set Alternatives...".
3. Add a language to the list (from the packaged locales).

Result

No way to remove the added language from the alternatives list because the "Remove" button is disabled. The only option is to move it up and down.

Expect

Be able to remove locales from the list.

Origin

Packaged locales are explicitly blocked from being removed. My guess is that the original idea was to prevent removing en-US, or because the packaged locale cannot be removed entirely as a language. But this doesn't work with multi-locale builds.

Comment 1

[Henry Wilkes (they/them) [:henry-x]]

@mstriemer you wrote the original code for this area, so I was hoping you might be able to help.

Packaged locales are explicitly blocked from being removed. My guess is that the original idea was to prevent removing en-US, or because the packaged locale cannot be removed entirely as a language. But this doesn't work with multi-locale builds.

I've looked through the code and I haven't found anything definite about the motivation for this line. Do you remember if there was any particular reason I should be aware about?

I've tested with canRemove being always true using a multi-locale packaged build, and removing the packaged locales doesn't seem to cause any problems. Technically this allows the user to remove the defaultLocale (en-US) from the requestedLocales (it will still appear at the end of appLocalesAsBCP47 though).

So, do you think we could simply change the condition to

canRemove: code != Services.locale.defaultLocale

Comment 2

[Mark Striemer [:mstriemer]]

I unfortunately don't recall why we use that check specifically, but I suspect we were only expecting 1/2 packaged locales.

Eemeli, do you know if we can relax this check at all?

Comment 3

[Eemeli Aro [:eemeli]]

I'm not sure.

The list of packaged locales is loaded from a text file and gets used by the L10n registry, and I'm not sure if we've tests that even consider what happens if they don't match.

Comment 4

[Henry Wilkes (they/them) [:henry-x]]

@eemeli and @mstriemer

I'm pretty sure the canRemove part is just whether it can be removed from Services.locale.requestedLocales:

• https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/browser/components/preferences/dialogs/browserLanguages.js#133-148
• https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/browser/components/preferences/dialogs/browserLanguages.js#482
• https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/browser/components/preferences/dialogs/browserLanguages.js#707-716
• https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/browser/components/preferences/dialogs/browserLanguages.js#469
• https://searchfox.org/mozilla-central/rev/b94e479d0b79b157029379832d05229df646e134/browser/components/preferences/main.js#1845-1875

I'm not aware of this being able to actually remove the underlying packaged locale. Testing myself I didn't notice anything like that.

In Tor Browser we have adopted this patch: https://gitlab.torproject.org/tpo/applications/tor-browser/-/commit/5be723a9be40487b91991cb1aaeb0d3538c67ad1.

Comment 5

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1851618 - Allow removing packaged locales from requestedLocales. r=#settings-reviewers

We open up the UI to allow the user to remove locales from their
requestedLocales list, except for the default locale.

Comment 6

[Eemeli Aro [:eemeli]]

Reviewed the patch and the history here; looks good. In particular this comment was likely why we ended up with our current code.

Comment 7

[Henry Wilkes (they/them) [:henry-x]]

@gijs can this land now? Or will someone else land this?

Also, will this naturally make its way to esr 128, or will it need an uplift request?

Comment 8

[:Gijs (he/him)]

Thanks for the ping and apologies for this not landing - I had assumed you had lando access? I have just triggered Lando for you.

For future reference, please ping me (or other reviewer(s)) if you need help landing the change. Or perhaps we should just get you access to Lando if you don't? :-)

I think you can still add the "check-in needed" project to phabricator revisions as well, though I keep hearing that is going to go away, AFAIK it hasn't yet. I'm not really sure what the deal is there.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6af26a1857ba
Allow removing packaged locales from requestedLocales. r=settings-reviewers,eemeli,Gijs

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6af26a1857ba

Comment 11

[Ada C.]

Reproduced the issue on Firefox 127.
Verified as fixed using the latest Nightly 129.0a1 and Firefox 128.0b9 on macOS 14 and Windows 11 x64.