636042 - When two fonts with the same name but different available characters exist we should be able to use characters from either one

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Dave Townsend [:mossop]]

Since upgrading my Epic to Android 2.2 (first on a leaked ROM and then yesterday on the official update) Fennec only ever displays serif fonts everywhere. I tried a clean profile with no success.

Comment 1

[Dave Townsend [:mossop]]

Blassey pointed me to http://lassey.us/fonts.html. On my mobile the 2nd line looks the same as the 1st and the 6th looks the same as the 5th suggesting there are bold sans-serif fonts but no normal sans-serif fonts.

I pulls /system/fonts/DroidSans.ttf and looked at it on my mac and it looked like a normal sans-serif font for me and the startup log from Fennec says it is seeing that font file.

Comment 2

[Jonathan Kew [:jfkthame]]

WFM on HTD Desire HD (Android 2.2), both with beta4 and current Fennec nightly. I wonder why it behaves differently for you.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Since this has only been reproducible for one person on one device, we're not going block on it

Comment 4

[Dave Townsend [:mossop]]

After some debugging I've figured out I'm hitting problems here: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#647

The fontgroup contains the Droid Sans font (twice for some reason) however HasCharacter for Droid Sans returns false and after that it falls through to WhichSystemFontSupportsChar which finds Droid Serif.

So either the font file is broken or the font code is for some reason reading it wrong just for me.

Comment 5

[Dave Townsend [:mossop]]

Pulled DroidSans.ttf from my Nook running CM7 where Fennec renders fine and put it on my Epic and still no serif fonts, suggesting it is something to do with the Epic's environment, not the font file specifically.

Comment 6

[Dave Townsend [:mossop]]

The problem turns out to be that my phone has both DroidSans.ttf and DroidSans_Subset.ttf, the latter of which doesn't contain the ascii character set, but whenever Fennec tries to use a sans font it ends up consulting DroidSans_Subset.ttf. Removing this file solves the problem but really we should be checking both fonts for characters. I think it is falling down because they have the same font name or something.

Comment 7

[Dave Townsend [:mossop]]

Looks like Opera has a similar problem here and that the offending font file ships with make Galaxy S models with Froyo including the Galaxy S 4G.

http://my.opera.com/community/forums/topic.dml?id=909431

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #4)
> After some debugging I've figured out I'm hitting problems here:
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#647

That should have been http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#2514

Comment 9

[Dave Townsend [:mossop]]

I'm re-requesting blocking on this based on the new info. I don't believe this is just one person on one device, I think we're just not seeing many reports from other Galaxy S users because the Froyo rollout has been so slow. Of course that is an argument that we don't need to fix it immediately but I'd rather the drivers made that call in knowledge of the facts than letting it slip.

The downside is that from what I can tell our font APIs can't really cope with multiple font files defining fonts with the same family and style making this tricky to fix properly. One hacky fix would be to just ignore DroidSans_Subset.ttf when loading fonts I guess.

Comment 10

[Stuart Parmenter]

We could detect that they are different and only use them for fallback, but this is a non-trivial patch that breaks some assumptions used on all platforms at the moment.  Would take an extremely well tested patch, but won't block on it at this point

Comment 11

[Rob Marshall [tH]]

For what it's worth, this started happening on my i9000 after the update from 2.2 to 2.2.1.

Comment 12

[Dave Townsend [:mossop]]

Attachment: WIP idea

So this is one idea. The code checks a bunch of different places for fonts for the character. The last one is to just find any system font for it which is really a last resort. Before then this goes through every font in the font group, gets the family for it and asks for any font in that family that has the character ideally with the same style as the original font. This ought to give us a closer match than just a random system font.

I needed to add something to gfxFontEntry to get to the family, dunno if that counts as an API change, but this does work. Not sure how to test it right now though.

Comment 13

[John Daggett (:jtd)]

Attachment: cmaps from Epic Froyo Droid Sans family

What a clusterf**k.  On this device the Droid Sans family includes three fonts:

  DroidSans.ttf
  DroidSans_Subset.ttf
  DroidSans-Bold.ttf

DroidSans has 954 entries, DroidSans_Subset 428 cmap entries and the bold face 12,458 (!!!), consisting mostly of glyphs for Hangul Syllables.  The DroidSans_Subset face is missing glyphs for the ASCII range and for Georgian.

Comment 14

[Dave Townsend [:mossop]]

Attachment: blacklist approach

This is an alternate approach that works by adding a blacklist of font filenames to preferences allowing us to just ignore the bad font file. Should be even safe than mucking with the font selection code.

Comment 15

[Dave Townsend [:mossop]]

Attachment: font blacklist

And this is the blacklist for the offending file.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 519337 [details] [diff] [review]
font blacklist

you should probably put this pref in all.js, since its all apps on Android will want it, not just Fennec.

Comment 17

[Jonathan Kew [:jfkthame]]

I'm not entirely comfortable with the blacklist - apart from the fact that blacklists in general are an ugly workaround, it'd be a problem if a manufacturer suddenly decided to ship a device with _only_ the DroidSans_Subset.ttf face; it'd also become a maintenance headache if manufacturers start doing this with additional fonts. But if we need a quick-and-dirty fix as a temporary measure we could do it.

Mossop, in the font list on your device, do the DroidSans and DroidSans_Subset fonts end up as part of the same gfxFontFamily? I'm assuming they do (and if they don't, that's a bug in font enumeration that we should fix).

In that case, I think the way forward here is to revise gfxFontFamily::FindFontForStyle so that rather than returning a single gfxFontEntry, it can return an array of font entries to be used for the requested style - in this case, that'd be both the subset and the "full" DroidSans entries - and we'd check all the returned entries for the current character. This would also handle the (quite plausible) situation where a manufacturer might decide to split a font into several _disjoint_ subsets, so that simply blacklisting one of them isn't a solution at all. And it would also be a step towards supporting the unicode-range descriptor for @font-face, which leads to a similar situation where multiple font entries need to be considered together as representing a single "face" within a family.

Comment 18

[John Daggett (:jtd)]

(In reply to comment #17)
> Mossop, in the font list on your device, do the DroidSans and DroidSans_Subset
> fonts end up as part of the same gfxFontFamily? I'm assuming they do (and if
> they don't, that's a bug in font enumeration that we should fix).

Yes, the DroidSans_Subset is a poorly subsetted version of DroidSans, the name tables are identical (*sigh*).

While I do think it's possible that Samsung could change the _Subset face, I think in this case it's an exceptional case that we're going to need to hack around one way or another.  The underlying problem is that we're reading the /system/fonts folder but Android is loading fonts via Skia code that the manufacturer may have hacked up in random ways.  There's a potential for a disconnect between the two.  Kato-san has been looking at similar problems with Sharp devices where they are hard coding fonts in /tmp/xxx locations (?!?) and hacking the Skia code to load them from those locations.

The best thing to do might be to contact Samsung directly and ask them what they're doing with this font and base our hack/fix on that information.

I agree that having the ability to deal with "composite" faces would be useful here but that's a much bigger change than this one problem warrants.  And it has a much wider impact than just Android code.

Comment 19

[Dave Townsend [:mossop]]

I think the likelihood of any manufacturer shipping with DroidSans_Subset.ttf instead of DroidSans.ttf is pretty small. The former is something Samsung seems to have made up for some reason, the latter is the font file in the original Android source code that all manufacturers base their builds on.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

DroidSans.ttf is one of 7 fonts that are required on android devices, so we can be reasonably sure that it will always be there

reference: https://www.google.com/codesearch/p?hl=en#nScvmqlsOdU/Development/Depends/skia/src/ports/SkFontHost_android.cpp&q=FileTypeface&d=5&l=409

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: WIP - support multiple font entries for the same style

Beginnings of a patch to support multiple font faces sharing a common style; basically works on OS X and Windows, but fails a few of the @font-face reftests; won't build at all on Linux desktop as I haven't touched the Fc/Pango code yet.

Mossop, there's an android tryserver build at http://ftp.mozilla.org/pub/mozilla.org/mobile/tryserver-builds/[e-mail]-14670ddbb704/try-mb-br-andrd-r7-bld/fennec-4.0b6pre.en-US.eabi-arm.apk - I'd be interested to know whether this resolves the Droid Sans problem on your device, if you have a chance to try it.

Comment 22

[John Daggett (:jtd)]

(In reply to comment #21)
> Created attachment 519606 [details] [diff] [review]
> WIP - support multiple font entries for the same style
> 
> Beginnings of a patch to support multiple font faces sharing a common style;
> basically works on OS X and Windows, but fails a few of the @font-face
> reftests; won't build at all on Linux desktop as I haven't touched the Fc/Pango
> code yet.
> 
> Mossop, there's an android tryserver build at
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tryserver-builds/[e-mail]-14670ddbb704/try-mb-br-andrd-r7-bld/fennec-4.0b6pre.en-US.eabi-arm.apk
> - I'd be interested to know whether this resolves the Droid Sans problem on
> your device, if you have a chance to try it.

I really, really don't think this is a good idea.  The bug here is basically a "font bug" and what's needed is a way to workaround it on a subset of devices.  I agree that having multiple faces per style-combination is needed for unicode-range support but that's a more constrained use case.  I don't see a great need to do fallback for fonts that share an identical name table, those fonts are going to have all sorts of problems to begin with on any system.

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #21)
> Created attachment 519606 [details] [diff] [review]
> WIP - support multiple font entries for the same style
> 
> Beginnings of a patch to support multiple font faces sharing a common style;
> basically works on OS X and Windows, but fails a few of the @font-face
> reftests; won't build at all on Linux desktop as I haven't touched the Fc/Pango
> code yet.
> 
> Mossop, there's an android tryserver build at
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tryserver-builds/[e-mail]-14670ddbb704/try-mb-br-andrd-r7-bld/fennec-4.0b6pre.en-US.eabi-arm.apk
> - I'd be interested to know whether this resolves the Droid Sans problem on
> your device, if you have a chance to try it.

It does resolve the problem

Comment 26

[Aaron Train [:aaronmt]]

Seeing this on the Samsung Galaxy S II right off the bat (07/18 Nightly)

Comment 27

[Cameron McCormack (:heycam)]

Same here.  First thing I installed when I got my Galaxy S II a couple of months ago was Firefox, and most of the UI is shown using Droid Serif (I assume).  It looks a bit odd, but I have kind of got used to it. ;)

Comment 28

[Alex Limi (:limi) — Firefox UX Team]

If someone can whip up a tryserver build again, I can test it on the standard build on Samsung Galaxy S II.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

John - As Samsung grows in market share (it is) this bug is becoming more important. Can we get some traction on this soon?

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: patch, support multiple font entries with the same style attributes within a family

Despite John's reservations in comment #22, I think this is the right way to resolve this problem - and incidentally, it also provides a basis for implementing @font-face unicode-range support (bug 475891), and fixes bug 465414 so that several currently-failing (or skipped) reftests can work properly.

This patch now passes reftests, including the newly-enabled font-face/order-{2,3} and font-face/enable-sheet-{2,3,6,7}, on all try platforms.

Builds available (temporarily) at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a31f10602da0/ for anyone who'd like to test on a particular device.

(In reply to comment #22)
> I don't
> see a great need to do fallback for fonts that share an identical name
> table, those fonts are going to have all sorts of problems to begin with on
> any system.

The issue isn't that the fonts have identical name tables, it is that the faces are indistinguishable in terms of CSS font-selection properties (style, width, weight). If a family has two faces that are equally good matches according to CSS, because they share these properties - even if one is named "Complete" and the other is named "Subset", for example - we have no reliable way to pick which one to use, and considering both as valid matches is the most reasonable option. This patch enables us to do this.

I agree that this isn't a likely scenario in the context of Mac or Windows font families, but generalizing the code so as to support it - in particular, changing FindFontForStyle (singular) to FindFontsForStyle (potentially plural) - does no harm, provides the foundation we'll need anyway for unicode-range, and incidentally fixes the real-world bug we're seeing here without the need to resort to ad-hoc font blacklist or similar hackiness.

Comment 31

[John Daggett (:jtd)]

The bug here is a Samsung-specific issue with a single font,
DroidSans_Subset.ttf, that is a subsetted *copy* of another font,
DroidSans.ttf. Why it's been dumped in the font folder is a mystery.,
the font is unnecessary, it's just a subsetted version of the default
font which also exists. There is absolutely no reason to use this font
in Fennec.

Your solution is to generalize this to all platforms to handle the
situation where multiple fonts exist with the same weight/width/style
attributes.  But that's really a *different* problem from the one
here, one with many different facets and comparably much higher risk
to performance.

> -            gfxFontEntry *fe = family->FindFontForStyle(mStyle, needsBold);
> -            // if ch in cmap, create and return a gfxFont
> -            if (fe && fe->TestCharacterMap(aCh)) {
> -                nsRefPtr<gfxFont> prefFont = fe->FindOrMakeFont(&mStyle, needsBold);
> -                if (!prefFont) continue;
> -                mLastPrefFamily = family;
> -                mLastPrefFont = prefFont;
> -                mLastPrefLang = charLang;
> -                mLastPrefFirstFont = (i == 0 && j == 0);
> -                return prefFont.forget();
> +            nsAutoTArray<gfxFontEntry*,1> entries;
> +            family->FindFontsForStyle(mStyle, entries, needsBold);
> +            for (PRUint32 k = 0; k < entries.Length(); ++k) {
> +                // if ch in cmap, create and return a gfxFont
> +                gfxFontEntry *fe = entries[k];
> +                if (fe->TestCharacterMap(aCh)) {
> +                    nsRefPtr<gfxFont> prefFont = fe->FindOrMakeFont(&mStyle, needsBold);
> +                    if (!prefFont) {
> +                        continue;
> +                    }
> +                    mLastPrefFamily = family;
> +                    mLastPrefFont = prefFont;
> +                    mLastPrefLang = charLang;
> +                    mLastPrefFirstFont = (i == 0 && j == 0);
> +                    return prefFont.forget();
> +                }

This change illustrates what I'm talking about.  Instead of selecting
a single face the code will now iterate over an array for which the
99.9999999% case is a *single* entry.  And where there is actually an
issue, for example the Minion Pro family which has different optical
faces that map to the same style attributes under OSX, the winner will
be the first face in the list.  So you're not really solving *that*
problem any better than the existing code does. I'm not opposed to
addressing that problem but I don't think that's a good way to solve
this problem.

But back to *this* problem.  Keeping a blacklist is no more hacky than
the underlying way we're constructing the font list on Android in the
first place. Since Android lacks a way to getting a list available
fonts we're forced to scan the list of fonts in the system font
folder, so we'll pick up whatever fonts a manufacturer puts there. 
Instead of contorting our font selection logic every time an odd font
is thrown into the font folder, a blacklist is a simple,
straightforward way to work around these problems.

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to prefer standard android fonts over extras

figured I'd throw a third option into the ring. The idea here is to give preference to the standard android fonts over any extra fonts that the OEM adds.

Comment 33

[Brad Lassey [:blassey] (use needinfo?)]

To be clear, I have no  particular objection to Jonathan's approach except that there seems to be a sentiment brewing that we want to fix this fast and possibly uplift to the release branches. A patch that size (with possible performance impact) may be hard to land in short order. And it'll be near impossible to get approval for branch landing.

Comment 34

[John Daggett (:jtd)]

Comment on attachment 549015 [details] [diff] [review]
patch to prefer standard android fonts over extras

I would prefer this, it's a simple workaround to the original
Samsung-specific problem.

> +            bool isStdFont = false;
> +            for (int i = 0; i < 14 && !isStdFont; i++) {
> +                isStdFont = strcmp(sStandardFonts[i], ent->d_name) == 0;
> +            }

>      closedir(d);
> +    for (int i = 0; i < 14; i++) {
> +        nsCString s(aFontsDir);

The '14' value in these two places should really be
NS_ARRAY_LENGTH(sStandardFonts) I think.

Also, have you confirmed that this code works when one of these
standard fonts is missing?  The code looks like it should but it
wouldn't hurt to add something like 'bogus.ttf' to the list and
verifies that works correctly.

It might be nice to clean out the use of nsPromiseFlatCString here, I
don't think it's needed (but I'm not a qualified NSString god).

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to prefer standard android fonts over extras

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to comment #32)
> Created attachment 549015 [details] [diff] [review] [review]
> patch to prefer standard android fonts over extras
> 
> figured I'd throw a third option into the ring. The idea here is to give
> preference to the standard android fonts over any extra fonts that the OEM
> adds.

If we're going to do something like this, you should set the mIsStandardFace flag on those font entries, so that we can handle them appropriately in any future revisions of the font-matching code, rather than just relying on the order of the faces in the array.

And in that case, you wouldn't necessarily need to add the faces in two separate passes; it might be simpler to add them all in a single loop over the directory, and then ensure gfxFontFamily::SortAvailableFonts() is called on each family to prioritize the standard ones.

(Untested, just a thought...)

Comment 37

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f8cc3b228502

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> for (int i = 0; i < NS_ARRAY_LENGTH(sStandardFonts) && !isStdFont; i++) {

So this causes a build warning because i is signed and NS_ARRAY_LENGTH(sStandardFonts) isn't, right?

Comment 39

[LegNeato]

Is comment 38 going to be addressed when this is landed on aurora and beta?

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

I'll address comment 36 and comment 38 before landing on branch

Comment 41

[Aaron Train [:aaronmt]]

Verified Fixed on Nightly
Tested with a Samsung Galaxy SII - Android 2.3.3
Mozilla/5.0 (Android; Linux Armv7l; rv:8.0a1) Gecko/20110729 Firefox/8.0a1 Fennec/8.0a1

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up to fix warning

Comment 43

[Cameron McCormack (:heycam)]

Thanks, my Firefox Mobile looks so much nicer now. :-)

Comment 44

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/113ec09ac6a3

Comment 45

[LegNeato]

FWIW these don't transplant nicely into mozilla-aurora or mozilla-beta. If you want this in Firefox 6 it needs to come in today or very early tomorrow

Comment 46

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/e0521aa0fec0
http://hg.mozilla.org/releases/mozilla-aurora/rev/450664de3adc

Comment 47

[Aaron Train [:aaronmt]]

Verified Fixed on Aurora

Tested with a Samsung Galaxy SII - Android 2.3.3
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110803 Firefox/7.0a2 Fennec/7.0a2

This landing on beta?

Comment 48

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-beta/rev/88b97e068306
http://hg.mozilla.org/releases/mozilla-beta/rev/fe5db4dba428