When opening the prefs I get: document.l10n is undefined fontbuilder.js:46 And the font list in the prefs is empty. Probably a fallout of bug 1457021.

Zibi, what do we need to do to make the list work again? This would be the first time we need to use a ftl file.

Oh, fun... Well, I assume you need Fluent. Which involves a couple things that should be easy :) - include l10n.js just like https://searchfox.org/mozilla-central/source/browser/components/preferences/fonts.xul#22 does - copy https://searchfox.org/mozilla-central/source/browser/locales/en-US/browser/preferences/fonts.ftl to some location in thunderbird and expose it. This is frankly suboptimal. Those strings should live where fontbuilder lives - in toolkit - but they don't. So you need an FTL file to supply two strings: - fonts-label-default - fonts-label-default-unnamed - Once you have it, you need to initialize Localization in your product, just like Firefox does it: 1) https://searchfox.org/mozilla-central/source/browser/locales/jar.mn#10 - this will make your product include omni.ja:/localization with the path to the new file 2) https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#727-730 - this will make your product register the new FileSource in L10nRegistry so that Fluent can pick it up. I'm happy to review patches and help you debug, since it seems like we just triggered the first case where Fluent leaked onto Toolkit. Sorry for not giving you a warning - I didn't realize that it'll be it!

Alternatively, of course, you can fork the fontbuilder.js to keep using the .properties and file a bug to move the strings used by fontbuilder.js to toolkit (which is where they should be). But then we'll have to initialize toolkit FileSource for L10nRegistry anyway in Tb (and Fx), so not that much of a win :)

This works for me. The registering in L10nRegistry was already done in bug 1431260. Zibi, do we need to do also something like https://hg.mozilla.org/mozilla-central/rev/af2f85201ec2 ? Or is this when we would migrate the complete files?

Thanks, I'll look at this soon as it breaks the 2 tests as seen on c-c trunk.

Comment on attachment 8974996 [details] [diff] [review] fluentFonts.patch that looks great! Please, monitor bug 1455649 which I hope to land next week and which will make the <script src="...l10n.js"/> unnecessary :)

Added a try{} catch (e) {} in preferences.xml to stop a NS_ERROR_FAILURE.

Comment on attachment 8975178 [details] [diff] [review] fluentFonts.patch Review of attachment 8975178 [details] [diff] [review]: ----------------------------------------------------------------- This looks good when playing with it manually, but the test still fails locally (can't push a try run as the tree is closed). The error now is: SUMMARY-UNEXPECTED-FAIL | test-font-chooser.js | test-font-chooser.js::test_font_name_displayed EXCEPTION: Timeout waiting for font picker serif to populate. at: utils.js line 406 TimeoutError utils.js:406 13 waitFor utils.js:444 11 MozMillController.prototype.waitFor controller.js:687 3 verify_advanced test-font-chooser.js:85 7 Runner.prototype.wrapper frame.js:579 7 startTest test-window-helpers.js:326 11 WindowWatcher_notify test-window-helpers.js:358 9 _verify_fonts_displayed test-font-chooser.js:98 3 test_font_name_displayed test-font-chooser.js:124 27

Also visible on try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b2bee81db3d1838e2d77373965edb3392c786c44 So there may be some subtle timing issue, that is not visible manually.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/a15a0e87508f temporarily disable failing test test-font-chooser.js. rs=bustage-fix

With 2018-05-15 and 2018-05-18 nightly on windows, the General options tab is shown and clicking other tabs is ineffective. Different issue?

It would be a different issue, if it were an issue. I can't reproduce this with 62.0a1 (2018-05-19) (64-bit) (just downloaded). If you can reproduce reliably, please file another bug with exact STR.

I don't see this issue. And then, it would be a different one for a new bug.

Aceman and I have been looking at this for a few days/nights now. This patch brings back the font chooser in the in-content prefs, however, the "Advanced" button still doesn't work. Sadly, when applying the patch, pane-switching in the "regular old-fashioned" prefs doesn't work any more. So I can't even land this as a first step. Adding the error dump and Cu.reportError(e); I see: [Show/hide message details.] [Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/preferences/preferences.xml :: _selectPane :: line 827" data: no] _selectPane chrome://messenger/content/preferences/preferences.xml:827:21 showPane chrome://messenger/content/preferences/preferences.xml:763:13 prefwindow_XBL_Constructor chrome://messenger/content/preferences/preferences.xml:663:9 document.l10n.ready< chrome://global/content/l10n.js:54:5 Looks this the new system is geared towards in-content prefs and we might have to kiss the "regular old-fashioned" prefs good-bye.

> Looks this the new system is geared towards in-content prefs If you mean Fluent than no, it doesn't care and is not tied to any model. I suspect a lot of issues that Tb is facing will go away once we land bug 1455649 (in review) because it'll expose Fluent "for free" to all Gecko chrome documents. You may also be affected by regular regressions that I need to fix like bug 1462675. Sorry for that!

Probably not Fluent itself (unless we are still missing some import of l10n.js somewhere), but the changes to /preferences/* in browser. Like removing all the preferences bindings (which we adopted for now) and switching to Preferences instead of <preference> elements. We try to follow the changes but in this case we differ from m-c for now.

Aceman beat me to an answer here ;-( With "new system" I was referring not so much to Fluent but to its friends, like l10n.js (which is intl/l10n/l10n.js, I assume). Maybe that's more geared towards in-content prefs, which we also have in TB since TB 38, but hidden behind a pref. Most users still use the "regular old-fashioned" prefs stand-alone window that FF also had once upon a time. As Aceman said, we're still using <preference> elements and don't write the preference values straight away. Some (sub-)panels still even have a "Cancel" button allowing to discard changes made on the panel. So preference processing has pretty much diverged from FF preferences, and moving the latter to Fluent was just the straw that broke the camel's back. My suggestion would be to remove the "regular old-fashioned" prefs and align the code used in the in-content prefs with FF. That will a) reduce the maintenance load and b) make it easier to port things in the future.

> With "new system" I was referring not so much to Fluent but to its friends, like l10n.js (which is intl/l10n/l10n.js, I assume). That's not true. l10n.js is just a wrapper for any document to bind it to DOMLocalization (from the Fluent package). Once again, it doesn't take any assumptions and should work all the same for any XUL/XHTML/HTML document. And this script will be replaced with nsIDocumentL10n once bug 1455649 lands which will give `document.l10n` property available on all documents in Gecko. > So preference processing has pretty much diverged from FF preferences, and moving the latter to Fluent was just the straw that broke the camel's back. My concern is that Fluent may be a red herring here. I don't understand what breaks, but I don't think Fluent should impact your work except that if you include any of the Firefox preferences code you may have to provide it with Fluent l10n.js/resources now. I can try to build Tb and debug it if you want me later this week.

Thanks for the offer, Zibi. I think it's about time we remove the stand-alone options window in TB, see bug 1465061, and announcement here: http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-May/001192.html We should focus on fixing the font lists behind the "Advanced" button and not waste time on getting the stand-alone window going again.

I found that the "Default (*)" label wasn't shown. It seems the [localization] @AB_CD@.jar: messenger (%messenger/**/*.ftl) doesn't work. As a workaround I added the fonts.ftl to copy to the normal locale directory. Now, the label is filled. I also tried to add the @RESPATH@/messenger/localization/* to the package-manifest as I saw in bug 1402069, but this does also not help to create the 'localization/en-US/messenger/preferences' directory like FX does fro browser. I see nothing in this bug which is missing. Zibi, what do we miss to create this directory automatically?

This aligns the code in fonts.js/.xul with what the Firefox version has and makes the dialog work for me (with prefs in a tab). It even converts away from <prefpane> to a <dialog> but without visible difference. There is still one behaviour difference to Firefox, in that when you select a language where you didn't set user specified fonts (e.g. Thai), you get no fonts selected in the menulists (as if empty is selected), but you can choose a font from the list. In Firefox, in this case the default fonts get preselected in the menulists. I do not yet see where the difference may lie. The patch contains some debugging and unneeded changes that will be removed in the final version. This patch is to be applied on top of Paenglab's.

We should really land bug 1465061 first. (In reply to :aceman from comment #22) > There is still one behaviour difference to Firefox, in that when you select > a language where you didn't set user specified fonts (e.g. Thai), you get no > fonts selected in the menulists (as if empty is selected), ... Related to comment #21? ("Default (*)" label wasn't shown).

Backout patch to re-enable tests.

I've landed bug 1465061 to cut through the Gordian Knot of the interdependency of the two bugs. Here's the rebased patch, and unless I messed up, I'm sad to say that it doesn't work. I get: JavaScript error: about:preferences, line 1: ReferenceError: gDisplayPane is not defined

Just saw it, I messed up.

Works, after I corrected the merge mistake. Sorry about the noise. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d939a4e27b6be2e05b151352b658125b6b5ff70

Comment on attachment 8983317 [details] [diff] [review] 1460721.patch - further tweaks of fonts dialog (rebased and cleaned up, take 2) The font chooser tests still fail. But I'm landing this now, since it's been broken for much too long. Then we fix the finer points later.

Comment on attachment 8982870 [details] [diff] [review] fluent-fonts.patch with working label in "Display" Thanks, working better than the previous version. I'm going to land this now. We'll have to follow up and fix the tests anyway.

Without the non-working hunk: [localization] @AB_CD@.jar: messenger (%messenger/**/*.ftl)

Removed unnecessary "async" keyword and fixed indentation issue.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b395522db4caa6a2ffc8377f3b08d07b0e1cd82d failed with Missing file(s): Thunderbird Daily.app/Contents/Resources/messenger/localization/* Looks like Richard already knew that, see comment #21.

Thanks for the cleanup. Does the "Default" label work now? Also, are the menulists for other languages populated now? The function FontBuilder.readFontSelection() does return empty when the default font should be used, but I do not understand how that is converted to display the default font (in Firefox prefs). The tests may still fail as we probably need to add some more waiting into them as the menulists are populated even more async now and it is so slow I can see it redrawing. Oh the progress...

Also removed the hunk in package-manifest.in. Let's see now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3dfc8a1b4329ac2151e27b1a905e051fc5bb65e4

(In reply to :aceman from comment #33) > Does the "Default" label work now? Also, are the menulists for other > languages populated now? Yes, as far as I can see both work. As soon as the try run is done, I'm going to land this, so we can stop shuffling various versions around in multiple bugs and focus on the real issue. Maybe it still won't work on server builds, but at least it appears to be working locally.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/2ffa15315fa9 Port bug 1457021 to TB: Migrate the JS of Preferences::Fonts to Fluent. r=zibi,jorgk https://hg.mozilla.org/comm-central/rev/045dc8345aea Align font preferences processing with M-C. r=jorgk

OK, let's take it from here: - fix tests - fix packaging issues - further issues, like: _readDefaultFontTypeForLanguage() is different in M-C. Do we need this? https://hg.mozilla.org/comm-central/rev/045dc8345aea#l2.159 preference = document.createElementNS("...", "preference");

Creating of <preference> elements will go once we convert all the pref panes to Preferences object. Now only fonts.* was fully converted. The conversion could mimic the m-c patch for that. We should file a new bug for it.

Looks like the landings in comment #36 busted the Linux builds: make[1]: *** [automation/l10n-check] Error 2 And the log says: [task 2018-06-05T09:55:11.863Z] 09:55:11 INFO - l10n-check> RuntimeError: File "chrome/messenger/preferences/fonts.ftl" not found in /builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales/merge-dir/x-test/mail, /builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales/x-test/mail So who can help us out with this? Zibi sadly hasn't replied, so let's make some noise.

Looks like I broke Windows as well :-( - The only platform that's building is Mac and that's the one I used for the try. I'd rather not back this out if we can fix the build issues quickly. It's really painful not to have a build person, since all those build issues come back to regular developers.

Maybe affected what I asked in comment 4 about porting https://hg.mozilla.org/mozilla-central/rev/af2f85201ec2. Maybe the .ftl files aren't automatically added to l10n like the .dtd and .properties.

Sorry, I can't really help when it comes to build system. Re comment 4: migration is completely unrelated, it's about moving strings from DTD/properties to FTL files to avoid retranslation, it doesn't affect build.

Let me take this off of everybody else's radar, I'll follow up. The jar.mn part won't work in https://hg.mozilla.org/comm-central/rev/2ffa15315fa9#l4.12, you'll need to get back to the earlier versions. I don't have a build to try out, and won't for a couple of hours, but here's my thinking: hg mv mail/locales/en-US/chrome/messenger/preferences/fonts.ftl mail/locales/en-US/messenger/preferences/fonts.ftl i.e., move it away from /chrome/ Remove the line that packs it as chrome in jar.mn. Add a hunk like [localization] @AB_CD@.jar: messenger (%messenger/**/*.ftl) That hunk didn't work for you on try because you had the file landed in en-US/chrome/messenger instead of en-US/messenger.

As per comment #43, thanks Axel! https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3c9ad25e2bd93860616b9209a04955a0b5bfab2

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/1e99d17fb28f Follow-up: Fix packaging issues. rs=bustage-fix CLOSED TREE DONTBUILD

The Daily run based in this looks successful. Richard, can you confirm? So now we need to fix the test.

Yes, also the Advanced dialog works. :-)

my comment 47 was wrong. It worked only because the fonts.ftl was still in my normal locales directory. Removing it showed again the broken "Advanced" dialog. It doesn't work because the path for the L10nRegistry wasn't correct. This patch fixes it for me.

Comment on attachment 8983894 [details] [diff] [review] Bug1460721-fup.patch Review of attachment 8983894 [details] [diff] [review]: ----------------------------------------------------------------- This looks right to me.

Comment on attachment 8983894 [details] [diff] [review] Bug1460721-fup.patch I'm really not the expert here. I'll land with r=Pike, rs=jorgk.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/82206ad8748f Follw-up: Fix the localization link in mailGlue.js. r=Pike, rs=jorgk

This all works now on my local build. But in the Dailies it doesn't work. I found that the localization files (also the devtools file) aren't packaged in the omni.ja. Manually packaging the "localization" directory in the root of the omni.ja makes it working. Pike or Zibi, do you know what is missing during the packaging?

https://dxr.mozilla.org/comm-central/source/mozilla/browser/installer/package-manifest.in#49 needs to be added here: https://dxr.mozilla.org/comm-central/source/mail/installer/package-manifest.in#52

Many thanks. Yes this was needed. Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9030b5291544d55ee9c9dd9111fd03ccbd3c4e8e&selectedJob=182843099

Like this? We won't use "browser" as in: @RESPATH@/browser/localization/* Richard, you can package locally, can't you?

Comment on attachment 8984869 [details] [diff] [review] 1460721-fix-packaging-take2.patch Richard was faster, I assume he tested his try run.

Yes, I tested it.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/13b6979b94b0 Follow-up: Add the localization path to the package manifest. r=jorgk

This should update the font chooser tests for the new behaviour when displaying the default fonts. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central

Comment on attachment 8989022 [details] [diff] [review] 1460721-tests.patch Review of attachment 8989022 [details] [diff] [review]: ----------------------------------------------------------------- I can't say that I enjoyed reading this code. But great that we can finally close this bug. Let's wait for the Mac result, I guess it will be closer to Linux. In this case, check for "darwin". Did you know that Darwin is the name of the BSD system onto macOS is built? ::: mail/test/mozmill/pref-window/test-font-chooser.js @@ +96,5 @@ > + // only the label, in the form "Default (font name)". > + if (AppConstants.platform == "linux") { > + // On Linux the prefs we set contained only the generic font names, > + // like 'serif', but here a specific font name will be shown, but it is system > + // dependent what it will be. So we just check for the 'Default' prefix. system-dependent. http://www.btb.termiumplus.gc.ca/tcdnstyl-chap?lang=eng&lettr=chapsect2&info0=2.03 @@ +141,5 @@ > let fontTypes = kFontTypes.map(fontType => expected[fontType]); > + _verify_fonts_displayed(false, ...fontTypes); > + > + for (let [fontType, fontList] of Object.entries(gRealFontLists)) { > + Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage); Do we need this here or can this go to teardown? @@ +192,5 @@ > let fontTypes = kFontTypes.map(fontType => expected[fontType]); > + _verify_fonts_displayed(true, ...fontTypes); > + > + for (let [fontType, fakeFont] of Object.entries(kFakeFonts)) { > + Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage); Should this go to the teardown as well?

(In reply to Jorg K (GMT+2) from comment #60) > Let's wait for the Mac result, I guess it will be closer to Linux. In this > case, check for "darwin". Did you know that Darwin is the name of the BSD > system onto macOS is built? Sure. > @@ +141,5 @@ > > let fontTypes = kFontTypes.map(fontType => expected[fontType]); > > + _verify_fonts_displayed(false, ...fontTypes); > > + > > + for (let [fontType, fontList] of Object.entries(gRealFontLists)) { > > + Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage); > > Do we need this here or can this go to teardown? > Should this go to the teardown as well? OK, to teardownTest() actually.

Most parts landed on TB 62 and the test fix will be uplifted, so making the Target Thunderbird 62.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/bc158a134fe5 fix test-font-chooser.js tests. r=jorgk

Beta (TB 62): https://hg.mozilla.org/releases/comm-beta/rev/7794684605a9fdc55575de4ff3cc21d4a8d0e5a1