Closed Bug 1254978 Opened 4 years ago Closed 4 years ago

Advanced font dialog should not display the first font in the dropdown if the current font in prefs is unavailable on the system

Categories

(Firefox :: Preferences, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: emk, Assigned: emk)

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
1. Make sure HWA is enabled.
2. Make sure you don't install "Arial AMU".
3. Go Options > Content > Fonts & Colors > Advanced.
4. Select Armenian from "Font for:" dropdown.

Actual result:
Sans-serif font and monospace font would be some random font that happens to be first in alphabetical order. It is very unlikely to be appropriate for Armenian.
Also, even if by any chance it is suitable for Armenian and you click OK on the Fonts dialog, you cannot actually use the "selected" font for Armenian. This is very confusing.

Expected result:
Sans-serif font and monospace font should be blank. If HWA is disabled, you will get the expected result.
We already use an empty string if the firstChild happens to be a menu separator (this is the case when HWA is disabled).
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8728402 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8728402 [details] [diff] [review]
1254978_fix_random_font_in_dropdown

How does HWA influence this code, and under what circumstances is a separator the first element here?

Reading through the rest of the file, neither of those questions have a clear answer for me.

Separately, I'm concerned that this will change whether the default for other languages is correctly selected when the list is initially populated. If not this code, what code takes care of this?

Can we write/add a test for some of this behaviour? It seems we have https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_basic_rebuild_fonts_test.js and we should probably add a test there if possible, both for the case where we currently do the right thing, and for the case (like for Armenian) where we do not.
Attachment #8728402 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #2)
> How does HWA influence this code, and under what circumstances is a
> separator the first element here?
> 
> Reading through the rest of the file, neither of those questions have a
> clear answer for me.

The font list will have the following items:
1) Fonts that are suitable for the selected language and type (serif/sans-serif/monospace).
2) The menu separator
3) All other fonts that are not listed in 1.
2) and 3) are present only if 1) does not list all available fonts on the system.
Here is the code that creates the list:
https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/toolkit/mozapps/preferences/fontbuilder.js#25
EnumerateFonts will return 1) and EnumerateAllFonts will return 3).

But EnumerateFonts will take care of language only with GDI and Fontconfig backends because only those backends actually implement SupportsLangGroup:
https://mxr.mozilla.org/mozilla-central/search?string=SupportsLangGroup
All other backends (including the DWrite backend) will use the default implementation that always returns true:
https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/gfx/thebes/gfxFontEntry.h#274

So if HWA is on (i.e. with the DWrite backend), 1) will list the installed fonts regardless of the language.
If HWA is off (i.e. with the GDI backend) and the system has no Armenian font, 1) will be empty and the first entry of the font dropdown will be the menu separator.

> Separately, I'm concerned that this will change whether the default for
> other languages is correctly selected when the list is initially populated.
> If not this code, what code takes care of this?

The initial defaultValue (.firstChild.firstChild.getAttribute("value")) is used only when no pref fonts are installed on the system. It will work only if the font backend respects the language.
Maybe we could set blank if the dropdown has no menu separator? (That is, when 1) lists all fonts on the system.)

> Can we write/add a test for some of this behaviour? It seems we have
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/in-content/tests/browser_basic_rebuild_fonts_test.js and we
> should probably add a test there if possible, both for the case where we
> currently do the right thing, and for the case (like for Armenian) where we
> do not.

I'm not sure it is possible to write a reliable test because the test result will depend on the installed fonts (hence the Operating System language and version) and whether HWA is enabled.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > How does HWA influence this code, and under what circumstances is a
> > separator the first element here?
> > 
> > Reading through the rest of the file, neither of those questions have a
> > clear answer for me.
> 
> ...

Thanks for the explanation!

> > Separately, I'm concerned that this will change whether the default for
> > other languages is correctly selected when the list is initially populated.
> > If not this code, what code takes care of this?
> 
> The initial defaultValue (.firstChild.firstChild.getAttribute("value")) is
> used only when no pref fonts are installed on the system. It will work only
> if the font backend respects the language.
> Maybe we could set blank if the dropdown has no menu separator? (That is,
> when 1) lists all fonts on the system.)

That sounds more correct, yes.

> > Can we write/add a test for some of this behaviour? It seems we have
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/
> > preferences/in-content/tests/browser_basic_rebuild_fonts_test.js and we
> > should probably add a test there if possible, both for the case where we
> > currently do the right thing, and for the case (like for Armenian) where we
> > do not.
> 
> I'm not sure it is possible to write a reliable test because the test result
> will depend on the installed fonts (hence the Operating System language and
> version) and whether HWA is enabled.

We could overwrite the FontBuilder.enumerator property before calling buildFontList to enforce deterministic behaviour?
> Maybe we could set blank if the dropdown has no menu separator? (That is,
> when 1) lists all fonts on the system.)

This is implemented. Also added a test using a mock enumerator.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87cf4ad0749d
Attachment #8728402 - Attachment is obsolete: true
Attachment #8729774 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8729774 [details] [diff] [review]
Set blank if the current font is not installed and the font backend does not support language-specific enumeration

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

::: browser/components/preferences/in-content/tests/browser_basic_rebuild_fonts_test.js
@@ +39,5 @@
> +  win.FontBuilder._allFonts = null;
> +  win.FontBuilder._langGroupSupported = false;
> +
> +  doc.getElementById("font.language.group").value = "x-armn";
> +  doc.getElementById("selectLangs").value = "x-armn";

Nit: seems like the test will be slightly easier to read if you add some local variables here to keep the language group, selectLangs and serif elements.

@@ +46,5 @@
> +
> +  doc.getElementById("font.language.group").value = "x-western";
> +  doc.getElementById("selectLangs").value = "x-western";
> +
> +  // Simulate a font backend supporting language-specific enumeration

Nit: can you elaborate on this comment by saying this works because _allFonts has by now been cached to have 3 elements, ie more than just the 1 we're adding to _list here?

(I had to look for quite a bit to understand why changing the 'backend' of the fake enumerator would work this way...)
Attachment #8729774 - Flags: review?(gijskruitbosch+bugs) → review+
Egh, forgot to say this in the initial comment for the patch: thank you so much for the test! Makes it much easier to review this. :-)
https://hg.mozilla.org/integration/fx-team/rev/7277824fbd1dea85a59d0bade0a44ccfb5186703
Bug 1254978 - Set blank if the current font is not installed and the font backend does not support language-specific enumeration. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/7277824fbd1d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.