Closed Bug 228518 Opened 21 years ago Closed 20 years ago

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

Categories

(Core Graveyard :: GFX: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: bryner)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #137438 - Flags: superreview?(dbaron)
Attachment #137438 - Flags: review?(blizzard)
Comment on attachment 137438 [details] [diff] [review]
patch

sr=dbaron, but would this make things better or worse if Xft weren't enabled?
Attachment #137438 - Flags: superreview?(dbaron) → superreview+
afaik it would have no effect at all if xft was disabled, since fontconfig is
what introduces the concept of aliases.
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?
Everything I changed is inside #ifdef MOZ_WIDGET_GTK2, so no.
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.
Attachment #137438 - Flags: review?(blizzard) → review-
blizzard, can you comment on why you minused the patch?
[ we talked about this on irc later. ]
Attached patch alternate patch (obsolete) — Splinter Review
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.
Attachment #137438 - Attachment is obsolete: true
Attachment #139823 - Flags: superreview?(dbaron)
Attachment #139823 - Flags: review?(blizzard)
[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]
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.
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.
Attachment #139823 - Flags: review?(blizzard) → review-
> 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.
Attachment #139823 - Flags: superreview?(dbaron) → superreview-
Attached patch againSplinter Review
Attachment #139823 - Attachment is obsolete: true
Attachment #140087 - Flags: superreview?(dbaron)
Attachment #140087 - Flags: review?(blizzard)
Comment on attachment 140087 [details] [diff] [review]
again

What about comment 11?
Attachment #140087 - Flags: superreview?(dbaron) → superreview-
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...)
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 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.
Attachment #140087 - Flags: superreview- → superreview+
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?
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.
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).
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.
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.
Attachment #140087 - Flags: review?(blizzard) → review+
checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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);
                ^

Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: