1437393 - Font lists in preferences are no longer grouped by font type, port asynchronous handling like Bug 1399206

Status: ASSIGNED

(SeaMonkey :: Preferences, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1435034 +++

Bug 1399206 - update callers of FontBuilder.buildFontList to handle async font enumeration [Thunderbird]
https://hg.mozilla.org/comm-central/diff/e480213445a5/mail/components/preferences/fonts.js

Bug 1375978 - Enumerating fonts should not block about:preferences from being displayed
https://hg.mozilla.org/mozilla-central/diff/f255ec4e8c36/browser/components/preferences/fonts.js
https://hg.mozilla.org/mozilla-central/diff/f255ec4e8c36/toolkit/mozapps/preferences/fontbuilder.js

(Quoting rsx11m from bug 1435034 comment #17)
> Reference: attachment 8948518 [details] [diff] [review]
> More comprehensive fix (v2) [landed]
> 
> This patch addresses the immediate port of bug 1378366 to our pref-fonts.js
> implementation, now showing "Default [(...)]" labels correctly. However, the
> lists do no longer separate the fonts of the family that we are interested
> in from the remaining fonts (2.49.x still do). This happens with or without
> this patch.
> 
> Further debugging revealed that "fonts" already contains all the fonts for
> gAllFonts already, thus are all considered part of the desired font type. In
> other words, GetFontEnumerator().EnumerateFonts(aLanguage, aFontType, {});
> seems to ignore
> the aFontType specification.
> 
> Given that the implementation of nsThebesFontEnumerator::EnumerateFonts()
> hasn't changed in ages beyond housekeeping patches, my hunch is that either
> bug 1344990 changed some behavior with the introduction of the "auto" pseudo
> font family, or it's a timing issue after bug 1375978 made font enumeration
> asynchronous (but we are not using the EnumerateFontsAsync function). For
> the latter, maybe parallel running font enumerations lead to fonts filling
> up completely in the same list, but I don't know enough about how it works
> and what the implications are.
> 
> Following the fontbuilder.js revision log, I didn't see any other changes
> that seem relevant.
> 
> My suggestion would be to land this and then to deal with menulist behavior
> in a separate bug, possibly porting bug 1375978. An alternative is obviously
> to switch to Toolkit's fontbuilder.js, mirroring how Thunderbird uses it in
> mail/components/preferences/fonts.js (with SM-specific adaptions of course,
> differences appear substantial wrt language-group implementation). The
> latter would imply further l10n changes, given that fontbuilder.js expects a
> bundlePreferences definition for string properties.

(Quoting rsx11m from bug 1435034 comment #23)
> I'm still a bit undecided whether or not we should switch to using Toolkit's
> FontBuilder, which would have further l10n implications given that it needs
> an explicit stringbundle "bundlePreferences" just for the two strings
> changed/introduced here. On the other hand, Thunderbird's fonts.js uses only
> one of the functions, FontBuilder.buildFontList(), and implements the
> remaining functions itself. Thus, there seems to be limited value in using
> it.
> 
> So, if we want to keep using our own buildFontList() in pref-fonts.js, there
> will be a couple of more asynchronous functions to be awaited for than in
> bug 1399206 attachment 8907218 [details] [diff] [review], but those may be
> obvious from mozilla-central changeset f255ec4e8c36.
> 
> Thus, I'll land this to close this bug for now, as it solves the immediate
> problem, and will look into making pref-fonts.js asynchronous in a follow-up
> bug (i.e., without using fontbuilder.js). If that doesn't work with a
> reasonable effort, I'd go to try a direct adoption of Thunderbird's code.

Comment 1

[rsx11m]

> Further debugging revealed that "fonts" already contains all the fonts for
> gAllFonts already, thus are all considered part of the desired font type.

This was observed on Windows. Interestingly, the behavior seen on Linux is different. Here, the recognized font types are "serif", "sans serif", and "monospace" and grouped into the main section. Neither of the other fonts is put there, though, thus they are all put into the "other" section of the menu.

I've tried a naive approach and added async/wait around the respective functions in pref-fonts.js, calling (the now async) BuildFontList directly instead of a window.setTimeout() environment. I've also copied the _selectLanguageGroup* snippets, but given that all functions are global, with a global gSelectLanguageGroupPromise variable as well. This doesn't work, I assume for exactly that reason (possibly a single instance here, vs. Thunderbird and Firefox define a gFontsDialog object containing those functions, thus apparently creating multiple instances of it). I may have to get up to speed how those Promise things are supposed to work.

Comment 2

[rsx11m]

Attachment: WIP patch (non-functional)

Next step is to put SelectFontLanguageGroup() and BuildFontList(), along with GetFontEnumerator(), into a var gFontDialog environment similar to FF/TB, thus containing all asynchronous functions within individual instances.