1460721 - No Font list in Prefs > Display - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Richard Marti (:Paenglab)]

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.

Comment 1

[Richard Marti (:Paenglab)]

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.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

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!

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :)

Comment 4

[Richard Marti (:Paenglab)]

Attachment: fluentFonts.patch

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?

Comment 5

[:aceman]

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

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :)

Comment 7

[Richard Marti (:Paenglab)]

Attachment: fluentFonts.patch

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

Comment 8

[:aceman]

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

Comment 9

[:aceman]

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.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1460721-disable-test.patch

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a15a0e87508f
temporarily disable failing test test-font-chooser.js. rs=bustage-fix

Comment 12

[Wayne Mery (:wsmwk)]

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?

Comment 13

[Jorg K (CEST = GMT+2)]

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.

Comment 14

[Richard Marti (:Paenglab)]

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

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: fluentFonts.patch

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.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 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!

Comment 17

[:aceman]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

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.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 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.

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: fluent-fonts.patch with working label in "Display"

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?

Comment 22

[:aceman]

Attachment: 1460721.patch - further tweaks of fonts dialog

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.

Comment 23

[Jorg K (CEST = GMT+2)]

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).

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: backout-a15a0e87508f

Backout patch to re-enable tests.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1460721.patch - further tweaks of fonts dialog (rebased and cleaned up)

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

Comment 26

[Jorg K (CEST = GMT+2)]

Just saw it, I messed up.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1460721.patch - further tweaks of fonts dialog (rebased and cleaned up, take 2)

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

Comment 28

[Jorg K (CEST = GMT+2)]

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 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: fluent-fonts.patch with working label in "Display" - modified by JK

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

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1460721.patch - further tweaks of fonts dialog (rebased and cleaned up, take 3)

Removed unnecessary "async" keyword and fixed indentation issue.

Comment 32

[Jorg K (CEST = GMT+2)]

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.

Comment 33

[:aceman]

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...

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: fluentfonts.patch with working label in "Display" - modified by JK, take 2

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

Comment 35

[Jorg K (CEST = GMT+2)]

(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.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Jorg K (CEST = GMT+2)]

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");

Comment 38

[:aceman]

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.

Comment 39

[Jorg K (CEST = GMT+2)]

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.

Comment 40

[Jorg K (CEST = GMT+2)]

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.

Comment 41

[Richard Marti (:Paenglab)]

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.

Comment 42

[Francesco Lodolo [:flod]]

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.

Comment 43

[Axel Hecht [:Pike]]

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.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1460721-packaging.patch

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

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e99d17fb28f
Follow-up: Fix packaging issues. rs=bustage-fix CLOSED TREE DONTBUILD

Comment 46

[Jorg K (CEST = GMT+2)]

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

Comment 47

[Richard Marti (:Paenglab)]

Yes, also the Advanced dialog works. :-)

Comment 48

[Richard Marti (:Paenglab)]

Attachment: Bug1460721-fup.patch

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 49

[Axel Hecht [:Pike]]

Comment on attachment 8983894 [details] [diff] [review]
Bug1460721-fup.patch

Review of attachment 8983894 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right to me.

Comment 50

[Jorg K (CEST = GMT+2)]

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.

Comment 51

[Pulsebot]

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

Comment 52

[Richard Marti (:Paenglab)]

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?

Comment 53

[Axel Hecht [:Pike]]

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

Comment 54

[Richard Marti (:Paenglab)]

Attachment: bug1460721-manifest.patch

Many thanks. Yes this was needed.

Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9030b5291544d55ee9c9dd9111fd03ccbd3c4e8e&selectedJob=182843099

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: 1460721-fix-packaging-take2.patch

Like this? We won't use "browser" as in: @RESPATH@/browser/localization/*

Richard, you can package locally, can't you?

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 8984869 [details] [diff] [review]
1460721-fix-packaging-take2.patch

Richard was faster, I assume he tested his try run.

Comment 57

[Richard Marti (:Paenglab)]

Yes, I tested it.

Comment 58

[Pulsebot]

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

Comment 59

[:aceman]

Attachment: 1460721-tests.patch

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 60

[Jorg K (CEST = GMT+2)]

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?

Comment 61

[:aceman]

(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.

Comment 62

[:aceman]

Attachment: 1460721-tests.patch v1.1

Comment 63

[Jorg K (CEST = GMT+2)]

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

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bc158a134fe5
fix test-font-chooser.js tests. r=jorgk

Comment 65

[Jorg K (CEST = GMT+2)]

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