228518 - [gtk2][xft] If system fonts are set to a fontconfig alias, CSS generic font may be used instead

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Brian Ryner (not reading)]

I have the following setup:

gtk2 Application font is set to "Sans" (a fontconfig alias, which should map to
Luxi Sans on my system)
browser Serif font is set to Times New Roman

result:

UI (menus, buttons) uses Times New Roman instead of Luxi Sans

We'll set up the fontconfig pattern with two families, 'sans' and 'Times New
Roman'.  It appears that fontconfig gives priority to Times New Roman over the
alias list for 'sans'.

One way we could work around this (and it's probably faster anyway) is to do
fontconfig substitution on the gtk pango font when we get it from the gtk
widget, to get a real family name.  I'll attach a patch that does this.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 137438 [details] [diff] [review]
patch

sr=dbaron, but would this make things better or worse if Xft weren't enabled?

Comment 3

[Brian Ryner (not reading)]

afaik it would have no effect at all if xft was disabled, since fontconfig is
what introduces the concept of aliases.

Comment 4

[tor]

I haven't really followed the history of pango, so I may be wrong here:
would this patch force a dependency of gtk2 for xft builds, since gtk1
didn't come with pango?

Comment 5

[Brian Ryner (not reading)]

Everything I changed is inside #ifdef MOZ_WIDGET_GTK2, so no.

Comment 6

[Christopher Blizzard (:blizzard)]

So here's another issue that's related, maybe it belongs here, maybe it doesn't.
 If you have a preference set in the font pref panel for web fonts and one of
those aliases happens to be the same name as the system font, the font specified
in the preference is used as an alias for the system font.  This has always
seemed wrong to me.

I think that we're doing something similar here, but causing the result by side
effect.

Comment 7

[Brian Ryner (not reading)]

blizzard, can you comment on why you minused the patch?

Comment 8

[Christopher Blizzard (:blizzard)]

[ we talked about this on irc later. ]

Comment 9

[Brian Ryner (not reading)]

Attachment: alternate patch

This is an approach that blizzard and I discussed on irc.  It adds a new bit to
the font struct that is set for system fonts and indicates that no generic
fallbacks should be added for this font.  This is needed because fontconfig
will prefer an exact match in the generic fallback family over a match on an
alias for the original family.	The only way to ensure that we get the font gtk
is telling us about is to add that family, and only that family, to the
pattern.

Comment 10

[Brian Ryner (not reading)]

[the bit also prevents the font from being treated as generic, so we don't look
up the pref value for the generic font family]

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't understand the changes to nsFontMetricsXft.cpp.  Your change to
nsFont::EnumerateFamilies already ensures that |mGenericFont| won't be set to
something that shouldn't be considered a generic.  Are these changes necessary
to fix the bug?  If so, the bug is going to occur in more than just system
fonts, and we need a better fix.

Comment 12

[Christopher Blizzard (:blizzard)]

I suspect that the use of a PRPackedBool here isn't what you want, is it?  It's
an aligned bool, not a small bool.  Or is that why you change the previous to a
bitfield?

Anyway, I also don't like the use of "neverGeneric."  Shouldn't it be
"systemFont" or something?  It's affecting the way we're using our web page font
prefs, not generics which can still be included as a system font.  Also, I hate
it when people use negatives in variable names.  It leads to a lot of double
negatives used in the code.

Substantially the code seems fine, though.

Comment 13

[Brian Ryner (not reading)]

> I suspect that the use of a PRPackedBool here isn't what you want, is it?  It's
> an aligned bool, not a small bool.  Or is that why you change the previous to a
> bitfield?

Yeah, that's confusing.  I think I'll change it to 'unsigned int'.  It's really
only 1 bit of storage in a bitfield, and per the spec, the only types allowed
for that are 'signed' and 'unsigned'.

> Anyway, I also don't like the use of "neverGeneric."

I can rename it.

Comment 14

[Brian Ryner (not reading)]

Attachment: again

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140087 [details] [diff] [review]
again

What about comment 11?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140087 [details] [diff] [review]
again

Also, the new bit is unnecessary -- we already have a mechanism for doing this
(since we use CSS family name strings in nsFont::name, and CSS already requires
that we have a mechanism for storing the name of a font that is non-generic). 
You can just put quotes around the fonts name instead of adding this new bit.

(And if the changes I mentioned in comment 11 really are necessary to fix the
bug, you should find a new way to do that anyway...)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Testcase for the real problem:
data:text/html;charset=utf-8,%3C!DOCTYPE%20HTML%20PUBLIC%20%22-%2F%2FW3C%2F%2FDTD%20HTML%204.0%2F%2FEN%22%3E%0D%0A%3Chtml%20lang%3D%22en%22%3E%0D%0A%20%3Chead%3E%0D%0A%20%20%3Ctitle%3ETest%3C%2Ftitle%3E%0D%0A%20%20%3Cstyle%20type%3D%22text%2Fcss%22%3E%0D%0A%0D%0A%20%20%20%20body%20%7B%20font-family%3A%20%22sans-serif%22%2C%20%22Luxi%20Mono%22%3B%20%7D%0D%0A%0D%0A%20%20%3C%2Fstyle%3E%0D%0A%20%3C%2Fhead%3E%0D%0A%20%3Cbody%3E%0D%0A%20%20%3Cp%3EThis%20should%20be%20in%20sans-serif%2C%20not%20monospace.%3C%2Fp%3E%0D%0A%20%3C%2Fbody%3E%0D%0A%3C%2Fhtml%3E%0D%0A

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140087 [details] [diff] [review]
again

ok, sr=dbaron, although I think I would slightly prefer making the code the
fills in the nsFont::name for the system font put quotes around it rather than
making the change to EnumerateFamilies here.

Comment 19

[Brian Ryner (not reading)]

As far as the testcase given (I think this touches a lot on bug 227889 too), I
think I found the culprit.  "sans-serif" is getting marked as a 'strong' binding
in the pattern, and hence wins even if it comes later in the pattern.  Aliases
are automatically expanded into weak pattern values, it appears, and we add all
of our families as strong values, so the originally specified families (even if
they come later) win.

I may be wrong, but I'm not sure we want this.  What if we add all our families
as weak?

Comment 20

[Christopher Blizzard (:blizzard)]

I think that LANG will win over the font names.  Not sure if we want that, either.

As for dbaron's comment, I think it's unrelated as I understand it.  We're not
talking about just generic fonts here.  We're talking about whether or not we
use the prefs as set by the user for UI fonts vs. web page fonts.  It's an
entirely different system.  Someone could have an alias in the prefs (there's
nothing stopping them) for having the same name for a system font.  Unless
you're talking about using magic names for system fonts that you would assume no
one would stumble across.

Comment 21

[Brian Ryner (not reading)]

dbaron's comment is related to whether the family is marked as generic when you
enumerate the families.  If you put the name in quotes, it is not marked as
generic.  That's what I was after, so I changed the code in nsDeviceContextGTK
to put the family in quotes, and removed the change to
nsFont::EnumerateFamilies.  The rest of the patch remains the same (and it still
works).

Comment 22

[Brian Ryner (not reading)]

You're right, family should be higher priority than lang.  In fact, lang should
only be consulted when choosing a generic font, I think.

I wonder if we could "fix up" the pattern after calling XftDefaultSubstitute so
that all of the families are marked as strong?  That will preserve the
precedence of family over lang, but not make the originally specified families
win out over the substituted-in families.  Unfortunately, it seems that this
would involve reconstructing the pattern; there's no API to change the binding
in-place.

Comment 23

[Christopher Blizzard (:blizzard)]

The whole LANG-is-more-important-than-family-name thing is a bstell legacy
requirement.  It's also where the whole Weak thing came from in the FC api.

Comment 24

[Brian Ryner (not reading)]

checked in.

Comment 25

[cls]

This checkin busted the Irix tinderbox:

cc-1282 CC: ERROR File =
/build2/tinderbox/mozilla/build/IRIX_6.5_Depend/mozilla/gfx/src/ps/nsAFMObject.cpp,
Line = 264
  More than one instance of overloaded function "abs" matches the argument list.

            Function symbol function "std::abs(long double)" is ambiguous by
                      inheritance.
            Function symbol function "std::abs(double)" is ambiguous by
                      inheritance.
            Function symbol function "std::abs(float)" is ambiguous by
                      inheritance.
            Function symbol function "std::abs(long long)" is ambiguous by
                      inheritance.
            Function symbol function "std::abs(long)" is ambiguous by
                      inheritance.
            Function symbol function "std::abs(int)" is ambiguous by
                      inheritance.
            The argument types are:  (unsigned int).
        score+= abs(aFont.style-gSubstituteFonts[i].mStyle);
                ^