Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 6

Status

()

Core
Graphics
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mossop, Assigned: blassey)

Tracking

({verified-aurora})

Trunk
mozilla8
ARM
Android
verified-aurora
Points:
---

Firefox Tracking Flags

(firefox6 fixed, firefox7 fixed, fennec6+)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.

Updated

7 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

7 years ago
Component: General → Graphics
Product: Fennec → Core
QA Contact: general → thebes
(Reporter)

Comment 1

7 years ago
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
tracking-fennec: ? → 2.0-
(Reporter)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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.
Summary: All fonts are serif → When two fonts with the same name but different available characters exist we should be able to use characters from either one
(Reporter)

Comment 7

6 years ago
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
(Reporter)

Comment 8

6 years ago
(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
(Reporter)

Comment 9

6 years ago
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.
tracking-fennec: 2.0- → ?

Comment 10

6 years ago
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
tracking-fennec: ? → 2.0-

Comment 11

6 years ago
For what it's worth, this started happening on my i9000 after the update from 2.2 to 2.2.1.
(Reporter)

Comment 12

6 years ago
Created attachment 519319 [details] [diff] [review]
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

6 years ago
Created attachment 519333 [details]
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.
(Reporter)

Comment 14

6 years ago
Created attachment 519336 [details] [diff] [review]
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.
(Reporter)

Comment 15

6 years ago
Created attachment 519337 [details] [diff] [review]
font blacklist

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.

Comment 18

6 years ago
(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.
(Reporter)

Comment 19

6 years ago
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
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.

Comment 22

6 years ago
(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.
(Reporter)

Comment 23

6 years ago
(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
(Assignee)

Updated

6 years ago
Duplicate of this bug: 643896

Updated

6 years ago
Duplicate of this bug: 670479

Updated

6 years ago
Priority: -- → P2
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?
Created attachment 548890 [details] [diff] [review]
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/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.
Assignee: nobody → jfkthame
Attachment #519606 - Attachment is obsolete: true

Comment 31

6 years ago
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.
Created attachment 549015 [details] [diff] [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.
Attachment #549015 - Flags: review?
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.

Updated

6 years ago
Attachment #549015 - Flags: review? → review?(jdaggett)

Comment 34

6 years ago
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).
Attachment #549015 - Flags: review?(jdaggett) → review+
Created attachment 549020 [details] [diff] [review]
patch to prefer standard android fonts over extras
Attachment #549015 - Attachment is obsolete: true
Attachment #549020 - Flags: review?(jdaggett)

Updated

6 years ago
Attachment #549020 - Flags: review?(jdaggett) → review+
(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...)
http://hg.mozilla.org/mozilla-central/rev/f8cc3b228502
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
> 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?
(Assignee)

Updated

6 years ago
Attachment #549020 - Flags: approval-mozilla-beta?
Attachment #549020 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #549020 - Flags: approval-mozilla-beta?
Attachment #549020 - Flags: approval-mozilla-beta+
Attachment #549020 - Flags: approval-mozilla-aurora?
Attachment #549020 - Flags: approval-mozilla-aurora+

Comment 39

6 years ago
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
Assignee: jfkthame → blassey.bugs
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
Created attachment 549541 [details] [diff] [review]
follow up to fix warning
Attachment #519336 - Attachment is obsolete: true
Attachment #519337 - Attachment is obsolete: true
Attachment #548890 - Attachment is obsolete: true
Attachment #549541 - Flags: review?(jdaggett)
Thanks, my Firefox Mobile looks so much nicer now. :-)

Updated

6 years ago
Attachment #549541 - Flags: review?(jdaggett) → review+
http://hg.mozilla.org/mozilla-central/rev/113ec09ac6a3

Comment 45

6 years ago
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?
status-firefox7: --- → fixed
Keywords: verified-aurora
http://hg.mozilla.org/releases/mozilla-beta/rev/88b97e068306
http://hg.mozilla.org/releases/mozilla-beta/rev/fe5db4dba428
tracking-fennec: - → 6+
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.