Port bug 1488467 - Part 1 to TB: Support adding and removing installed browser languages

RESOLVED FIXED in Thunderbird 64.0

Status

defect
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Since bug 1488467 on FX the language packs can be added and removed from the alternatives list. Also does the disabling of a language pack remove it from the list in the prefs.
Posted patch Languages.patch (obsolete) — Splinter Review
This is a direct port of https://hg.mozilla.org/releases/mozilla-beta/rev/6224d9ad89ba with adding three language strings.

Please can you try also changing the languages and check the dialog's height if the buttons are hidden again? I added the min-height again and saw this issue not on my computer. If you see this, I have to remove the min-height again.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9009369 - Flags: review?(jorgk)
This stuff is so bug ridden that it's not fun to port or review.

Here is what I have:
From http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/win64/xpi/
I downloaded de, fr, es.

I added de and fr to the alternatives. Order: fr, de, English. Restart.
First time I open the Alternatives, the panel comes up small.
The pulldown menu under the list has no label (since in French it's missing and the
default mechanism doesn't work).
If I open the panel again, it's bigger and the pulldown menu has "Select a language to add ...".

Next I *disable* fr in the add-on manager. The chosen language still shows "Français", in the panel I see some German and the rest in English.

Restart. Now the selection is empty. Opening the panel the first time, it's still too small with the pulldown label missing, the second time is OK.

I will test this in FF Nightly now :-(
Posted patch languages-no-min-height.patch (obsolete) — Splinter Review
The same patch but without the min-height: 250px; to check, if this fixes the hidden buttons.
Posted patch languages-no-min-height.patch (obsolete) — Splinter Review
Correct patch without the min-height: 250px;
Attachment #9009386 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #2)
> I will test this in FF Nightly now :-(
I have to eat my works on almost everything I said. Apart when disabling a language pack (bug 1488442), everything works fine in FF. Of course the mileage varies depending on what has been translated which is still a bit patchy.

However, browser/localization/**/browser/preferences/languages.ftl is translated in the langpacks I tried
es: Seleccione un idioma a añadir
de: Wählen Sie eine Sprache zum Hinzufügen aus
fr: Choisir une langue à ajouter
and even renaming languages-customize-select-language will apply the correct fallback language.

===

Back to TB. The new patch makes little difference.

On first opening of the alternatives panel
a) the panel is too small
b) the pulldown list at the bottom has lost its label. It works on second opening of the panel using the en-US label.

After making French the main language and adding languages-customize-select-language in French, I get even more strange behaviour:

On first opening of the alternatives panel
a) the panel is too small
b) the pulldown list at the bottom uses the French label. Now on the second opening it's empty.

I haven't seen any of that in FF, so we must be missing something when launching the panel.
Looking at the error console, I see:
No chrome package registered for chrome://messnger/content/utilityOverlay.js
No chrome package registered for chrome://browser/skin/preferences/preferences.css

Cut&paste errors and typos from here:
https://hg.mozilla.org/comm-central/rev/f5e4d855c2a8#l5.12
https://hg.mozilla.org/comm-central/rev/f5e4d855c2a8#l5.27

We should see whether fixing those makes things better.
Doesn't seem to help, but we should fix it anyway.
Here's another one when resizing the subdialogue:
JavaScript error: chrome://messenger/content/preferences/subdialogs.js, line 355: TypeError: frame is undefined; can't access its "style" property

Could all be related to not showing the panel properly.
We really have a problem with launching that subdialogue that clearly FF doesn't have since they moved all the preferences stuff to another technology already :-(
Filed bug 1491601 for the TypeError. That's always been there, even in TB 60.
Fixed the issues from comment #6.
Attachment #9009387 - Attachment is obsolete: true
This now really fixes the error re. utilityOverlay.js. Sadly it doesn't help.
Attachment #9009404 - Attachment is obsolete: true
Posted patch languages.patch (obsolete) — Splinter Review
I compared and updated the files with the FX files. The JS file has now only one difference: |var gMessengerLanguagesDialog = {| instead of |var gBrowserLanguagesDialog = {|. The XUL has more because of the changes "browser" to "messenger" and my change to flex the richlistbox. The differences shouldn't cause the empty label problem.

The strange thing is, I tried it around twenty times and the label was always shown, then it begun to be missing. From then it was missing very often.

I don't know why this happens. Maybe it's something with the difference we load the tab or something like the findbar.ftl loading.
Attachment #9009369 - Attachment is obsolete: true
Attachment #9009407 - Attachment is obsolete: true
Attachment #9009369 - Flags: review?(jorgk)
Attachment #9009435 - Flags: feedback?(jorgk)
Comment on attachment 9009435 [details] [diff] [review]
languages.patch

Yes, this is better, the subdialogue comes up in the right size every time.

(In reply to Richard Marti (:Paenglab) from comment #13)
> The strange thing is, I tried it around twenty times and the label was
> always shown, then it begun to be missing. From then it was missing very
> often.
Here's my setup:
Français
Anglais (États-Unis)
Allemand.

I've manually added languages-customize-select-language to localization/fr/messenger/preferences/languages.ftl.
The first time I launch the panel, I see my added French string, from then on, I see no label.

> I don't know why this happens. Maybe it's something with the difference we
> load the tab or something like the findbar.ftl loading.
Surely.

We can take it like this and follow up the missing label in another bug. Or we can ask Aceman to look at it together with bug 1491601.
Flags: needinfo?(acelists)
Attachment #9009435 - Flags: feedback?(jorgk) → feedback+
(In reply to Richard Marti (:Paenglab) from comment #14)
> Could this be needed to fix the empty label problem:
> https://searchfox.org/comm-central/source/mozilla/browser/components/
> preferences/in-content/subdialogs.js#265 ?
That's very well possible. I can easily test this with my reproducible case above.
Posted patch additional-patch.patch (obsolete) — Splinter Review
I added this and sadly it makes no difference. The first launch shows the label, subsequent launches don't. Richard, can you reproduce this as per my comment #15.
Comment on attachment 9009445 [details] [diff] [review]
additional-patch.patch

Yes, no markable change with this patch. It still happens seldom here. The string is only in en-US here, French and German don't have it. When en-US is on the second position, I can't see the issue. On third position then it happens sometime.
I've added languages-customize-select-language to both de and fr, so de, fr and en-US *all* have it now. If de or fr are on the first position, I get the label the first time, but then never again although all languages have it now. As far as I can see, positions two and three don't matter.

I fact, I can reduce the test to this: de and en-US, fr removed, uninstalled, gone. First time the subdialogue launches, I get the German label, after that I get nothing.

I compared subdialogs.js to the FF version and don't see much difference. In fact, I plugged the FF version into TB and the behaviour doesn't change.
If removed
<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
<script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>
since they were incorrect before and therefore weren't necessary.

I merged the other patch into this one since I believe that it's the right thing to do, even if it doesn't help.

I suggest to land it now, then we can file another bug and ask Zibi for help for the missing label. What do you think?
Attachment #9009435 - Attachment is obsolete: true
Attachment #9009445 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #9009449 - Flags: review+
Attachment #9009449 - Flags: feedback?(richard.marti)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a49b53bb306ecd6fb57baafd7cf2b93362973943
Just making sure the change to the subdialog code doesn't cause any problem.
Comment on attachment 9009449 [details] [diff] [review]
languages.patch (JK final)

Yes, it's not a main UI for all users. So we can go and fix it hopefully in a new bug.
Attachment #9009449 - Flags: feedback?(richard.marti) → feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e385d269fb25
Port bug 1488467 to TB: Support adding and removing installed languages in Alternatives subdialog. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
I'll file a follow-up for the missing label once I've tested an opt build. It's good to have a test case for Fluent strings.
Target Milestone: --- → Thunderbird 64.0
Blocks: 1491704
You need to log in before you can comment on or make changes to this bug.