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.

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.

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

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

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.

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.

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.

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

(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

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.

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

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

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.

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.

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.

And this is the blacklist for the offending file.

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.

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.

(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.

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.

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

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/jkew@mozilla.com-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.

(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/jkew@mozilla.com-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.

(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/jkew@mozilla.com-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

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

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. ;)

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

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

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/jkew@mozilla.com-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.

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.

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.

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 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).

(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...)

> 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?

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

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

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

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

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

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

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?

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