Closed Bug 1488442 Opened Last year Closed 10 months ago

Support disabled language packs in multilingual UI

Categories

(Firefox :: Preferences, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: mstriemer, Assigned: mstriemer, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

If a language pack is disabled it can still be returned by the getRequestedLocale(s) API.

Currently disabled language packs are hidden from the dropdown but the disabled one language pack is still "selected" so there is no value set in the dropdown.

In the alternatives dialog the disabled language packs are displayed in the list, which would allow you to switch to a disabled language pack. There should be some indicator that a language pack is disabled and possibly allow enabling it.

Restarting into a disabled language pack should not be allowed.
Priority: -- → P3
Assignee: nobody → mstriemer
Priority: P3 → P2
Generally, this switches Services.locale.requestedLocales calls to use
Services.locale.appLocalesAsBCP47.
Hmmm, I'm not sure if this is the right strategy. `requested` is meant to represent user intent, and as such it should allow the user to request something that is not available.

User can ask for `de` and there may not be a `de` langpack, or it may be disabled. In both cases, it's a valid request and we should handle it properly.

Switching `requested` to be a list of resolved locales, seems like a move that will change how we negotiate for this UI. If that's intentional, maybe we should rename the variables as well?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> Hmmm, I'm not sure if this is the right strategy. `requested` is meant to
> represent user intent, and as such it should allow the user to request
> something that is not available.
> 
> User can ask for `de` and there may not be a `de` langpack, or it may be
> disabled. In both cases, it's a valid request and we should handle it
> properly.

What would "properly" be in this case? If I set the pref to "de,en-US" but only have en-US installed, what should happen?

> Switching `requested` to be a list of resolved locales, seems like a move
> that will change how we negotiate for this UI. If that's intentional, maybe
> we should rename the variables as well?

I was thinking that once the user makes a change in this UI that would be their new "requested". If you go through this UI then you can't request something that is disabled or isn't installed.

We don't have any way to handle disabled or uninstalled langpacks in the requested section right now, and I think supporting that would significantly increase the complexity of this.
Flags: needinfo?(gandalf)
>  What would "properly" be in this case? If I set the pref to "de,en-US" but only have en-US installed, what should happen?

If the user requests "de,en-US", then this should be their `requested locales`. The fact that we don't have `de` doesn't mean it is not requested.

====

```
let requested = this.requestedLocales || Services.locale.appLocalesAsBCP47;
let availableSet = new Set(Services.locale.availableLocales);
requested = requested.filter(locale => availableSet.has(locale));
```

What this code does is that it takes `appLocales` which are negotiated `requested x available` and then filters out the ones that are not in available.

That indicates to me that something with the logic is off - the result of `appLocales` is already filtered because it's the result of negotiations.

If you want to have all locales that are available and the user requested, `appLocales` is exactly that (sorted by requested, and filtered so that if the user requested `de` and we have `de-DE` we'll show `de-DE`).

If you want the locales user requested, but only those that are also exactly available, then

```
let requested = Services.locale.requestedLocales.filter(locale => Services.locale.availableLocales.contains(locale))
```

seems like what you're looking for.

I may be confused, but it seems like at the very least we need some doc strings to explain this logic :)
```
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> >  What would "properly" be in this case? If I set the pref to "de,en-US" but only have en-US installed, what should happen?
> 
> If the user requests "de,en-US", then this should be their `requested
> locales`. The fact that we don't have `de` doesn't mean it is not requested.

I'm curious about what the implications are of removing it from the list. Is there a concern that a user would have `de` requested and be disabling/enabling a langpack for it?

> ```
> let requested = this.requestedLocales || Services.locale.appLocalesAsBCP47;
> let availableSet = new Set(Services.locale.availableLocales);
> requested = requested.filter(locale => availableSet.has(locale));
> ```
> 
> What this code does is that it takes `appLocales` which are negotiated
> `requested x available` and then filters out the ones that are not in
> available.
> 
> That indicates to me that something with the logic is off - the result of
> `appLocales` is already filtered because it's the result of negotiations.

`appLocales` are filtered, but if the dialog is used then we use the previous selection until about:preferences is closed. That list might need to be filtered (select de,en-US and accept, disable de, open dialog again).

> If you want to have all locales that are available and the user requested,
> `appLocales` is exactly that (sorted by requested, and filtered so that if
> the user requested `de` and we have `de-DE` we'll show `de-DE`).
> 
> If you want the locales user requested, but only those that are also exactly
> available, then
> 
> ```
> let requested = Services.locale.requestedLocales.filter(locale =>
> Services.locale.availableLocales.contains(locale))
> ```
> 
> seems like what you're looking for.

What would happen if the user requested `de` and has a langpack for `de-DE` here? It seems like it wouldn't be in the list, and the logic for mapping `de` to `de-DE` is in `appLocalesAsBCP47`?

> I may be confused, but it seems like at the very least we need some doc
> strings to explain this logic :)
> ```

I can add some comments.
Flags: needinfo?(gandalf)
> I'm curious about what the implications are of removing it from the list. Is there a concern that a user would have `de` requested and be disabling/enabling a langpack for it?

Removing from the list means "user does not want `de` locale" which may or may not be true, depending on if your UI is meant to control their request list of not.

If it is, I believe you should write to requestLocales not just read from it.

In other words, I think that maybe you should just read/write on requestedLocales and then filter out against available (which you already do) to not have the unavailable in the UI that allows user to construct the requested list?
Flags: needinfo?(gandalf)
So I switched most of the appLocales calls back to requestestedLocales but there's an issue when the user sets requested manually and none of those locales are installed. So if I have "en-US" and "it" and I set requested to "fr" then the available and requested list is empty, but the appLocale (likely?) would be "en-US".

This UI should be dealing with what the locales actually are, rather than what a user might be "requesting" I think. It's working with the actual langpacks not asking the user to pick a locale code.

One drawback of this would be that if you wanted to set the requested list manually, using this UI would remove anything that isn't installed once you accept the changes. I think this would be expected though since you've just confirmed the list that you want.

It looks like using the requested pref you could install "pt-BR" and set requested to "pt" and it will use "pt-BR". This means that requestedLocales filtered by availableLocales would be empty.
Flags: needinfo?(gandalf)
I added some comments and tried to clean things up a little, but I couldn't find a way to stop using appLocales and have it work in a non-broken way.
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d14b467d422
Part 1: Only list available locales in the requested set r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/d683ee5ff088
Part 2: Ensure added langpack is enabled r=aswan
Backed out 2 changesets (bug 1488442) for browser-chrome failures in browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=213274385&repo=autoland&lineNumber=3239

INFO - TEST-START | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js
23:25:03     INFO - TEST-INFO | started process screenshot
23:25:03     INFO - TEST-INFO | screenshot: exit 0
23:25:03     INFO - Buffered messages logged at 23:24:18
23:25:03     INFO - Entering test bound testDisabledBrowserLanguages
23:25:03     INFO - Buffered messages logged at 23:24:19
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | Check the proper URL is loaded - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | Element should not be null, when checking visibility - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | Overlay is visible - 
23:25:03     INFO - found chrome://browser/skin/preferences/preferences.css
23:25:03     INFO - found chrome://global/skin/in-content/common.css
23:25:03     INFO - found chrome://browser/skin/preferences/in-content/preferences.css
23:25:03     INFO - found chrome://browser/skin/preferences/in-content/dialog.css
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | All expectedStyleSheetURLs should have been found - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | pl is disabled - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | pl is the old 1.0 version - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | The right number of locales are selected - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | The selected locales are in order - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | The right number of locales are available - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | The available locales match - 
23:25:03     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | The first row is a label - 
23:25:03     INFO - Buffered messages finished
23:25:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | Test timed out - 
23:25:03     INFO - GECKO(972) | 1542842703546	addons.xpi-utils	WARN	Add-on langpack-pl@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - GECKO(972) | 1542842703546	addons.xpi-utils	WARN	Add-on langpack-fr@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - GECKO(972) | 1542842703547	addons.xpi-utils	WARN	Add-on langpack-he@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - Console message: 1542842703546	addons.xpi-utils	WARN	Add-on langpack-pl@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - Console message: 1542842703546	addons.xpi-utils	WARN	Add-on langpack-fr@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - Console message: 1542842703547	addons.xpi-utils	WARN	Add-on langpack-he@firefox.mozilla.org is not correctly signed.
23:25:03     INFO - GECKO(972) | JavaScript error: , line 0: uncaught exception: undefined
23:25:03     INFO - GECKO(972) | JavaScript error: , line 0: uncaught exception: undefined
23:25:03     INFO - GECKO(972) | MEMORY STAT | vsize 793MB | vsizeMaxContiguous 632MB | residentFast 268MB | heapAllocated 119MB
23:25:03     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | took 45067ms
23:25:03     INFO - Not taking screenshot here: see the one that was previously logged
23:25:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_browser_languages_subdialog.js | Found a tab after previous test timed out: about:preferences#general - 
23:25:03     INFO - checking window state
23:25:03     INFO - Console message: [JavaScript Error: "uncaught exception: undefined"]
23:25:03     INFO - Console message: [JavaScript Error: "uncaught exception: undefined"]
23:25:03     INFO - Console message: [JavaScript Error: "messageBar.querySelector(...) is null; can't access its "removeAttribute" property" {file: "chrome://browser/content/preferences/in-content/main.js" line: 846}]
23:25:03     INFO - hideConfirmLanguageChangeMessageBar@chrome://browser/content/preferences/in-content/main.js:846:5
23:25:03     INFO - browserLanguagesClosed@chrome://browser/content/preferences/in-content/main.js:1031:5
23:25:03     INFO - close@chrome://browser/content/preferences/in-content/subdialogs.js:127:9
23:25:03     INFO - _onContentLoaded/this._frame.contentWindow.close@chrome://browser/content/preferences/in-content/subdialogs.js:252:7
23:25:03     INFO - _onUnload@chrome://browser/content/preferences/in-content/subdialogs.js:209:7
23:25:03     INFO - handleEvent@chrome://browser/content/preferences/in-content/subdialogs.js:194:9
23:25:03     INFO - _endRemoveTab@chrome://browser/content/tabbrowser.js:3161:5
23:25:03     INFO - removeTab@chrome://browser/content/tabbrowser.js:2859:7
23:25:03     INFO - Tester_waitForWindowsState@chrome://mochikit/content/browser-test.js:563:9
23:25:03     INFO - nextTest@chrome://mochikit/content/browser-test.js:893:5
23:25:03     INFO - async*timeoutFn@chrome://mochikit/content/browser-test.js:1190:9
23:25:03     INFO - setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:1152:9
23:25:03     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:982:9
23:25:03     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
23:25:03     INFO - 
23:25:03     INFO - TEST-START | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js
23:25:04     INFO - Not taking screenshot here: see the one that was previously logged
23:25:04     INFO - Buffered messages logged at 23:25:03
23:25:04     INFO - Entering test bound 
23:25:04     INFO - Buffered messages logged at 23:25:04
23:25:04     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | Search pane was selected - 
23:25:04     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | main-content should be scrolled 50 pixels - 
23:25:04     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | Switching to a different category should reset the scroll position - 
23:25:04     INFO - Buffered messages finished
23:25:04     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | A promise chain failed to handle a rejection: (Unable to convert rejection reason to string.) - stack: (No stack available.)
23:25:04     INFO - Rejection date: Wed Nov 21 2018 23:25:03 GMT+0000 (GMT) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
23:25:04     INFO - Stack trace:
23:25:04     INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
23:25:04     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1118
23:25:04     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
23:25:04     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
23:25:04     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
23:25:04     INFO - Not taking screenshot here: see the one that was previously logged
23:25:04     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | A promise chain failed to handle a rejection: (Unable to convert rejection reason to string.) - stack: (No stack available.)
23:25:04     INFO - Rejection date: Wed Nov 21 2018 23:25:03 GMT+0000 (GMT) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
23:25:04     INFO - Stack trace:
23:25:04     INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
23:25:04     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1118
23:25:04     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
23:25:04     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
23:25:04     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
23:25:04     INFO - Leaving test bound 
23:25:04     INFO - GECKO(972) | MEMORY STAT | vsize 764MB | vsizeMaxContiguous 632MB | residentFast 240MB | heapAllocated 95MB
23:25:04     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_bug1018066_resetScrollPosition.js | took 731ms

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=d683ee5ff0887a5eaa509ff4f9d32e631302bdd3&selectedJob=213263506

Backout:
https://hg.mozilla.org/integration/autoland/rev/ec31fba4ef982c5db303374e79d1c39844512c6f
Flags: needinfo?(mstriemer)
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/796ce1c75aa4
Part 1: Only list available locales in the requested set r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/7bc33731a895
Part 2: Ensure added langpack is enabled r=aswan
https://hg.mozilla.org/mozilla-central/rev/796ce1c75aa4
https://hg.mozilla.org/mozilla-central/rev/7bc33731a895
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: needinfo?(mstriemer)
You need to log in before you can comment on or make changes to this bug.