332926 - Tiny -moz-field and -moz-list fonts in cairo Linux builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Zbarsky [:bzbarsky]]

OS:  Fedora Core 4

BUILD:  Current trunk Linux Cairo build

STEPS TO REPRODUCE:
1)  Start the browser
2)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=229391 (or any other bug)
3)  Try to read the text in the comboboxes.

I'll attach a screenshot.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Screenshot

So it looks like the only reason that text inputs are bigger there is that bugzilla styles their font family and size (sets the latter to "small").  If I just have a text input unstyled, it gets the same tiny font.  So it looks like mFieldFont is really small for some reason in this case in nsSystemFontsGTK2::GetSystemFont.

None of my other GTK2 apps use that tiny a font...

Comment 2

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

Any chance you could step through or put printfs in the code in nsSystemFontsGTK2::GetSystemFontInfo and see which numbers look wrong?  (The idea there is to go from pango's notion of points, to pixels, to mozilla's notion of points, where Mozilla and pango may have different concepts of DPI.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So I added some printfs (couldn't debug because of bug 330957).  We call GetSystemFontInfo for the field font, then get:

1) aPixelsToTwips = 14 (makes sense at 100dpi)
2) PANGO_SCALE = 1024
3) float(pango_font_description_get_size(desc) / PANGO_SCALE) == 10
4) PTR_pango_font_description_get_size_is_absolute is non-null
5) MOZ_pango_font_description_get_size_is_absolute returns false
6) GetXftDPI() returns 0 (because XGetDefault returns null)
7) So we end up with a 10px font size as far as Gecko is concerned.

This happens for all of the system fonts, actually.

If I log into GNOME instead of my usual non-GNOME login, then at step 3 I get "12" instead of "10", and I get "96" from GetXftDPI(), so I end up with a 16px font instead of a 10px one...



, so we do end up calling it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> , so we do end up calling it.

No idea where this came from; ignore it.  ;)

Comment 5

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

Could there be some GNOME/GTK thing that's not being initialized properly with the new codepaths, but happens to work OK if you're running in GNOME?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Given http://scanline.ca/dpi/ I'd bet the difference is that when I'm running GNOME I'm also running "gnome-settings-daemon".  See the last paragraph of "How X applications choose DPI" at that URL.

Since looking for the Xft.dpi resource is what we do here, it makes sense that if "gnome-settings-daemon" isn't running we come out with nothing set...  I think it makes the most sense to fall back on either the X-server-reported DPI or 96 instead of doing no scaling at all.  Or is there something better we can do?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Compare what GetOSDPI() at http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsDeviceContextGTK.cpp#1063 does, fwiw.  Note that if it fails to get the Xft.dpi property it falls back on the screen size.  And that before it goes for the Xft.dpi thing it checks the return value of gtk_settings_get_default().

Comment 8

[Boris Zbarsky [:bzbarsky]]

When running not under GNOME, the GTK2 code returns -1 from the GTK settings, 0 from XFT and 100 from the "measure the screen" fallback (100 is what the X server uses as its DPI in my case).

Comment 9

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

(In reply to comment #7)
> Compare what GetOSDPI() at

But the GTK system font code uses GetXftDPI directly there too.  GetOSDPI is used as part of determining the Mozilla DPI, but GetXftDPI is used in the old code as well for determining our notion of Xft's DPI.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The GetXftDPI in the GTK2 case doesn't got to pixels and then twips.  It goes to points and then twips:

1024     if (dpi != 0) {
1025       // pixels/inch * twips/pixel * inches/twip == 1, except it isn't, since
1026       // our idea of dpi may be different from Xft's.
1027       size *= float(dpi) * aPixelsToTwips * (1.0f/1440.0f);
1028     }

followed by:

  aFont->size = NSFloatPointsToTwips(size);

in my case |dpi| is 0 so we just leave "size" at 10, but we treat it as 10 points, not 10px.  So it ends up closer to 14px or so...

Comment 11

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

So pango internally has a bunch of get_resolution stuff, but it looks like it differs depending on how you compile pango, and it's not exposed.  Which really isn't very helpful.

Comment 12

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

Er, it's exposed, it's just a whole bunch of objects away from what we have; I'll investigate more tomorrow.

Comment 13

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

Actually, I don't have time; somebody else can, or you can just fix the error handling so it goes back to the old slightly-less-bad-but-still-wrong error handling behavior.

Basically it looks like we need to make a pango context and put some text in it in order to get fonts from a font description; those fonts, in turn, will have a fontmap with a DPI.

However, we might be able to figure out where *that* gets the DPI from more easily.

It was really clever of whoever wrote these programs to all have a different concept of the DPI.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> However, we might be able to figure out where *that* gets the DPI from more
> easily.

On Linux, I suspect this is pangofc-fontmap.c, which does:

  FcPatternGetDouble (tmp, FC_DPI, 0, &dpi)

where |tmp| is a |FcPattern *|.

vlad, pavlov, do you know how we could get our hands on one of those here?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Note that just creating one via FcPatternCreate gives me a DPI prop of 0.... :(

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #14)
> On Linux, I suspect this is pangofc-fontmap.c, which does:
> 
>   FcPatternGetDouble (tmp, FC_DPI, 0, &dpi)
> 
> where |tmp| is a |FcPattern *|.
> 
> vlad, pavlov, do you know how we could get our hands on one of those here?

Just creating a FcPattern will give you an empty pattern, hence it will be 0 :)   Take a look at gfxPangoFont::GetMetrics -- you need to call pango_itemize with a pango context, then get the PangoItem and PangoFcFont out of that, and then the FcPattern will be in fcfont->font_pattern.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: OK, this fixes things for me

I'd like to resolve the XXX comments before landing; I just don't know what the right answers there are.

Note that this fixed things even without the point/pixel thing, but I figured I should put that in just in case anyway.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 217463 [details] [diff] [review]
OK, this fixes things for me


>@@ -198,22 +204,70 @@ nsSystemFontsGTK2::GetSystemFontInfo(Gtk
> 
>     aFont->weight = pango_font_description_get_weight(desc);
> 
>     float size = float(pango_font_description_get_size(desc) / PANGO_SCALE);
> 
>     // |size| is now either pixels or pango-points (not Mozilla-points!)
> 
>     if (!MOZ_pango_font_description_get_size_is_absolute(desc)) {
>         // |size| is in pango-points, so convert to pixels
>         PRInt32 dpi = GetXftDPI();
>+        if (dpi == 0) {
>+            // OK, need to do some work here to get the DPI from an
>+            // honest pango font.
>+            double dblDPI;
>+        
>+#ifndef THEBES_USE_PANGO_CAIRO
>+            PangoContext* ctx = pango_xft_get_context(GDK_DISPLAY(), 0);
>+            gdk_pango_context_set_colormap(ctx, gdk_rgb_get_cmap());
>+#else
>+            PangoContext* ctx =
>+                pango_cairo_font_map_create_context(
>+                  PANGO_CAIRO_FONT_MAP(pango_cairo_font_map_get_default()));
>+#endif
>+            // XXXbz if ctx is null, then what?  Callers don't
>+            // check our retval!

I'd say if any of these is null, we just go on with dpi being 0 and use your default case below.  If any of these ends up being null, though, we're in for a world of hurt anyway; it's not like we have a prayer of recovering from an out-of-memory situation anyway.

>+            // XXXbz why "a"?  Is that just the text we're looking at?
>+            GList *items = pango_itemize(ctx, "a", 0, 1, al, NULL);

Yeah, it's just dummy text we're asking for itemization for.  For purposes of DPI getting, it should be fine.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Cleaned up

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 217502 [details] [diff] [review]
Cleaned up

looks good

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed.  Thanks for the help with digging into this!