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

Status: RESOLVED FIXED

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

Comment 4

[Ian Neal]

https://hg.mozilla.org/mozilla-central/diff/0b45ef5f7702377bded5a67c6af0a713eccfc41e/toolkit/mozapps/preferences/fontbuilder.js is also needed

Comment 5

[Ian Neal]

Attachment: 1437393-fontsaysnc-25319.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.19
Risk to taking this patch (and alternatives if risky): medium
String changes made by this patch: none

Also included port of Bug 1399202 - encapsulate language group selection:
https://hg.mozilla.org/releases/mozilla-esr60/diff/73ecf69d752dc882de38e684359531ea6ff1277d/browser/components/preferences/fonts.js

Comment 6

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9400143 [details] [diff] [review]
1437393-fontsaysnc-25319.patch

LGTM. There is one superfluous line break after _selectLanguageGroupPromise and I prefer to have brackets around one line if and loop statements. Will submit a followup patch.

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1437393-fontsasync-format-25319.patch

[Approval Request Comment]
Regression caused by (bug #): cosmetic only
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.19b1pre
Risk to taking this patch (and alternatives if risky): trivial
String changes made by this patch:--

The header was clearly wrong with tab width 4. Took a current one from comm-central which omits this infromation.
Brackets around one line statements and some reformating to not go over the 80 column mark. I left the prfs as is. Firefox did it differently and I think keeping as is is better. Given that we have no automatted formatter tools and rules no harm done.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3c3ba7ee7ca7
Font lists in preferences are no longer grouped by font type, port asynchronous handling like Bug 1399206. r=frg

Comment 9

[Ian Neal]

Comment on attachment 9406164 [details] [diff] [review]
1437393-fontsasync-format-25319.patch

[Triage Comment]
LGTM r/a=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f832b5c2f08f
Add missing brackets around one liners. r=IanN