Bug 47452 - [Gtk] style="font-family: courier" makes text disappear
Summary: [Gtk] style="font-family: courier" makes text disappear
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-09 06:12 PDT by Holger Freyther
Modified: 2010-12-10 01:54 PST (History)
2 users (show)

See Also:


Attachments
Illustrate the issue. (143 bytes, text/html)
2010-10-09 06:13 PDT, Holger Freyther
no flags Details
Attach config showing the issue. (3.85 KB, text/plain)
2010-10-10 13:07 PDT, Holger Freyther
no flags Details
App to illustrate the issue (6.02 KB, text/plain)
2010-10-23 08:16 PDT, Holger Freyther
no flags Details
Attempt to simplify the issue (6.42 KB, text/plain)
2010-10-23 11:12 PDT, Holger Freyther
no flags Details
Patch for this issue (23.93 KB, patch)
2010-12-09 16:53 PST, Martin Robinson
xan.lopez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2010-10-09 06:12:46 PDT
It appears that there is an error in finding a fallback font. If one is using the above CSS rule, no text is being displayed. Most likely we are not finding the right fallback font, or are not using it.
Comment 1 Holger Freyther 2010-10-09 06:13:45 PDT
Created attachment 70351 [details]
Illustrate the issue.
Comment 2 Holger Freyther 2010-10-09 06:24:41 PDT
IIRC WebCore maps 'courier' to 'courier new', and if I use a bogus font name it is displaying just fine. So it might be a problem with Red Hat's fontconfig configuaration.
Comment 3 Holger Freyther 2010-10-09 06:27:43 PDT
Maybe also of interest is

$fc-match 'Courier'
coue1255.fon: "Courier" "Regular"
Comment 4 Martin Robinson 2010-10-09 08:15:03 PDT
Do you mind running with fc-match -v? Perhaps there is some property of the font which is causing the render to fail (e.g. not being scalable).
Comment 5 Holger Freyther 2010-10-09 08:37:58 PDT
Do you want to see the result with an -a as well? I can attach it to the bugreport.

$ fc-match -v 'courier'
Pattern has 27 elts (size 32)
	family: "Courier"(s)
	style: "Regular"(s)
	slant: 0(i)(s)
	weight: 80(i)(s)
	width: 100(i)(s)
	size: 12(f)(s)
	pixelsize: 13(f)(s)
	spacing: 100(i)(s)
	foundry: "unknown"(s)
	antialias: FcFalse(s)
	hintstyle: 3(i)(s)
	hinting: FcTrue(s)
	verticallayout: FcFalse(s)
	autohint: FcFalse(s)
	globaladvance: FcTrue(s)
	file: "/usr/share/fonts/wine-courier-fonts/coue1255.fon"(s)
	index: 0(i)(s)
	outline: FcFalse(s)
	scalable: FcFalse(s)
	dpi: 75(f)(s)
	scale: 1(f)(s)
	charset: 
(s)
	lang: (s)
	fontversion: 0(i)(s)
	fontformat: "Windows FNT"(s)
	embeddedbitmap: FcTrue(s)
	decorative: FcFalse(s)
Comment 6 Martin Robinson 2010-10-09 08:41:38 PDT
(In reply to comment #5)
>     scalable: FcFalse(s)

This appears to be the issue. We need to be more stringent in the types of fonts that accept from FontConfig, I think.
Comment 7 Holger Freyther 2010-10-09 08:54:20 PDT
I was moving the font away, just to have a .fon font that is not displayed either. I agree that we need to check if our display code can load the font. In theory freetype should be able to load the Windows Font. At least it is listed here: http://freetype.sourceforge.net/freetype2/index.html.
Comment 8 Martin Robinson 2010-10-09 21:38:24 PDT
I think you might be right about this. Do you mind attaching the font file and any pertitent configuration info in /etc/fonts to the bug? I'll try to have this fixed soon.
Comment 9 Holger Freyther 2010-10-10 12:49:04 PDT
It is less a issue with the font but more one with the config. In FontCacheFreeType.cpp we run into 


    if (strcasecmp(reinterpret_cast<char*>(familyNameAfterConfiguration),
            reinterpret_cast<char*>(familyNameAfterMatching)) && !isFallbackFontAllowed(familyNameString)) 
        return 0;
    

this happens as we go from 'courier new' to 'Liberation Mono'. I am going to attach my /etc/fonts in a second, I am also trying to identify the rule responsible for that. I assume it is the following, but I will verify this.

        <alias>
                <family>Bitstream Vera Sans Mono</family>
                <family>DejaVu Sans Mono</family>
                <family>Liberation Mono</family>
                <family>Inconsolata</family>
                <family>Courier New</family>
                <family>Courier</family>
                <family>Andale Mono</family>
                <family>Luxi Mono</family>
                <family>Cumberland AMT</family>
                <family>Cumberland</family>
                <family>Nimbus Mono L</family>
                <default><family>monospace</family></default>
        </alias>
Comment 10 Holger Freyther 2010-10-10 13:07:30 PDT
Created attachment 70411 [details]
Attach config showing the issue.

It appears to be cause by a rule such as:


       <alias binding="same">
          <family>Courier New</family>
          <accept>
          <family>Liberation Mono</family>
          <family>Cumberland</family>
          <family>Cumberland AMT</family>
          </accept>
        </alias>
Comment 11 Holger Freyther 2010-10-10 13:16:13 PDT
Okay this got introduced by bug #36548 which landed in r68406. The question is, why don't we fallback to anything else? How can we know that there is nothing else? I read the comment about CSS font matching rule.. but according to my fontconfig config, 'Courier New' is the same as 'Monospace'.

Any idea how to resolve it?
Comment 12 Holger Freyther 2010-10-10 14:27:58 PDT
I have poked it a bit more... but I still have no conclusion.

1.) it tries 'Courier New' finds Liberation Mono... rejects it...
2.) FontCache.cpp takes the alternate path which will try 'Courier' and this works.
     a FontPlatformData is created with the FcPattern. 

    PlatformRefPtr<cairo_font_face_t> fontFace = adoptPlatformRef(cairo_ft_font_face_create_for_pattern(m_pattern.get()));

    I have printed the cairo_font_face_status of fontFace and it is 0 (success).. I have also dumped the FcPattern..

Pattern has 24 elts (size 32)
	family: "Courier"(s)
	style: "Regular"(s)
	slant: 0(i)(s)
	weight: 80(i)(s)
	width: 100(i)(s)
	pixelsize: 13(f)(s)
	spacing: 100(i)(s)
	foundry: "unknown"(s)
	antialias: FcFalse(s)
	hintstyle: 3(i)(s)
	hinting: FcTrue(s)
	verticallayout: FcFalse(s)
	autohint: FcFalse(s)
	globaladvance: FcTrue(s)
	file: "/usr/share/fonts/wine-courier-fonts/coue1255.fon"(s)
	index: 0(i)(s)
	outline: FcFalse(s)
	scalable: FcFalse(s)
	charset: 
(s)
	lang: (s)
	fontversion: 0(i)(s)
	fontformat: "Windows FNT"(s)
	embeddedbitmap: FcTrue(s)
	decorative: FcFalse(s)

so I wonder why something in webkit is rejecting and not attempting to draw the text.
Comment 13 Martin Robinson 2010-10-11 15:00:30 PDT
Maybe the issue here is the call to cairo_scaled_font_create. Perhaps for unscalable fonts, we should just use the original cairo_font_face_t as the target font.
Comment 14 Holger Freyther 2010-10-12 08:00:09 PDT
(In reply to comment #13)
> Maybe the issue here is the call to cairo_scaled_font_create. Perhaps for unscalable fonts, we should just use the original cairo_font_face_t as the target font.

it does not return an error... status is 0. I will try to build a debug version during my train trip and then single step through the code... compiling/linking/make dependencies just take too long here.
Comment 15 Holger Freyther 2010-10-23 06:12:21 PDT
I am working on a fontconfig/cairo code reduction now to isolate the issue. A debug build prints a nice runtime error.. somehow we do not get any glyphs from the font.
Comment 16 Holger Freyther 2010-10-23 08:16:45 PDT
Created attachment 71641 [details]
App to illustrate the issue

The code is copy and pasted from the WebCore sources, it will call abort() on all error paths of the WebCore code. In case it aborts in the GlyphPage::fill function, it will also print the Pattern. The code aborts with 'courier' but it does not abort with 'liberation sans'.

The code lacks error checking on the created cairo fonts but this turned out to be okay. I assume there is an error with characterset conversion of the font... but this will be the next step...
Comment 17 Holger Freyther 2010-10-23 11:12:48 PDT
Created attachment 71649 [details]
Attempt to simplify the issue

This will load the coue1255.fon (md5sum 3ae6da5157fe37d0aba30b817277662b) directly via FreeType2 and then uses FcFreeTypeCharIndex on the font face. We now need to look into the implementation to figure out the real meaning of a zero return of this function (the man page does not say).
Comment 18 Holger Freyther 2010-10-23 11:27:40 PDT
(In reply to comment #17)
> Created an attachment (id=71649) [details]
> Attempt to simplify the issue
> 
> This will load the coue1255.fon (md5sum 3ae6da5157fe37d0aba30b817277662b) directly via FreeType2 and then uses FcFreeTypeCharIndex on the font face. We now need to look into the implementation to figure out the real meaning of a zero return of this function (the man page does not say).

I can use the freetype-demos (ftdump, ftview) on the font, calls to FT_Get_Char_Index also return sensible values... so we will need to dig into FontConfig to figure out what is going wrong... The FT_Face also has a charmap set...
Comment 19 Holger Freyther 2010-11-09 07:14:13 PST
(In reply to comment #18)

> I can use the freetype-demos (ftdump, ftview) on the font, calls to FT_Get_Char_Index also return sensible values... so we will need to dig into FontConfig to figure out what is going wrong... The FT_Face also has a charmap set...

My email to the fontconfig list got stuck but behdad was kind enough to respond to private emails. Right now it is not possible to query for certain encodings of the font. Behdad proposed that we will have to have proper font fallback then.

So two ways to go forward:

 a) Before loading the font, get the FT_Face and check if it is supported by FontConfig. We could try to get this method into fontconfig itself

b) Handle that we do not have any 'accessible' glyphs and have a proper fallback, or have a fallback code to just try a different font.


For b) we will need to  change WebCore internals, for a) we have to face the penalty on selecting the font.
Comment 20 Martin Robinson 2010-11-10 09:49:14 PST
(In reply to comment #19)

>  a) Before loading the font, get the FT_Face and check if it is supported by FontConfig. We could try to get this method into fontconfig itself

If I understand correctly, Fontconfig should already deliver a list of fallback fonts. We could examine if that gives reasonable alternatives. I don't think the performance hit would be too bad there.
Comment 21 Holger Freyther 2010-11-10 10:10:10 PST
(In reply to comment #20)
> (In reply to comment #19)
> 
> >  a) Before loading the font, get the FT_Face and check if it is supported by FontConfig. We could try to get this method into fontconfig itself
> 
> If I understand correctly, Fontconfig should already deliver a list of fallback fonts. We could examine if that gives reasonable alternatives. I don't think the performance hit would be too bad there.

True, and we don't have perf checks in place and wouldn't notice. I will try to find some time over the weekend, but if you have a pressing need then please don't stop yourself.
Comment 22 Martin Robinson 2010-11-10 10:15:39 PST
(In reply to comment #21)

> True, and we don't have perf checks in place and wouldn't notice. I will try to find some time over the weekend, but if you have a pressing need then please don't stop yourself.

I entirely agree that the lack of a performance testing for WebKitGTK+ is seriously bad / uncomfortable.
Comment 23 Martin Robinson 2010-12-09 16:53:32 PST
Created attachment 76136 [details]
Patch for this issue
Comment 24 Martin Robinson 2010-12-09 16:59:29 PST
After Xan had this issue too and I thought about it a bit, I decided to write a patch to simply never select fonts that do not have one of the three valid Fontconfig encodings. On Ubuntu these fonts never seem to be selected, because they are bitmap fonts and Ubuntu doesn't select bitmap fonts by default.
Comment 25 Holger Freyther 2010-12-09 17:02:40 PST
Looks very reasonable. Thanks.
Comment 26 Xan Lopez 2010-12-09 17:12:25 PST
Comment on attachment 76136 [details]
Patch for this issue

Looks great to me.
Comment 27 Holger Freyther 2010-12-09 17:21:45 PST
Comment on attachment 76136 [details]
Patch for this issue

View in context: https://bugs.webkit.org/attachment.cgi?id=76136&action=review

> WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:198
> +    if (!platformData->hasCompatibleCharmap()) {

I wonder if we could do this with a static method but I assume it will be difficult as we would have to go from FcPattern to FT_Face without cairo's help... or replicate a lot what the FontPlatformData is doing.
Comment 28 Martin Robinson 2010-12-09 23:57:41 PST
(In reply to comment #27)
> I wonder if we could do this with a static method but I assume it will be difficult as we would have to go from FcPattern to FT_Face without cairo's help... or replicate a lot what the FontPlatformData is doing.

Here it might be useful to replicate Cairo's logic for loading the FT_Face and then passing that to Cairo. Having the FT_Face before the cairo_scaled_font_t is also useful for vertical text rendering, I think.
Comment 29 Martin Robinson 2010-12-10 01:54:39 PST
Committed r73695: <http://trac.webkit.org/changeset/73695>