If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Linux Mozilla makes poor choices for glyph substitution

VERIFIED FIXED in M17

Status

()

Core
Layout
P3
normal
VERIFIED FIXED
18 years ago
13 years ago

People

(Reporter: tenthumbs, Assigned: Erik van der Poel)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

18 years ago
The particular problem in this bug is Mozilla's choice within the same font
family.

Consider a simple page like this:

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN">
<HTML>
<HEAD>
<TITLE>53px Euro Test</TITLE>
</HEAD>
<BODY>
<P style="font: 53px arial,helvetica,sans-serif">
Here's a &euro;.
</p>
</BODY>
</HTML>

On my system, Mozilla defaults to iso8859-1 and Arial fonts are available so
the X server reports:
-monotype-arial-medium-r-normal--0-0-0-0-p-0-iso8859-1 and
-monotype-arial-medium-r-normal--0-0-0-0-p-0-iso8859-15 plus a number of other
charset variants. While Mozilla decides to load
-monotype-arial-medium-r-normal--53-*-*-*-p-*-iso8859-1, it never loads the
iso8859-15 font for the euro symbol.

It appears to load the first iso8859-15 font it finds. That's usually
-misc-fixed-medium-r-normal--13-120-75-75-c-70-iso8859-15 and its "scalable"
form -misc-fixed-medium-r-normal--0-0-75-75-c-0-iso8859-1. (Note that this is
not a proportional font.) In this case Mozilla goes for the scalable form and
loads -misc-fixed-medium-r-normal--53-*-*-*-c-*-iso8859-15 with the predictably
ugly results.

If I make X use only unscaled fonts from that directory (which is probably a
good idea) then Mozilla goes for
-misc-fixed-medium-r-normal--13-120-75-75-c-70-iso8859-15 with an even uglier
result.

If I reorder the font path to put the misc-fixed fonts last, it's obvious that
Mozilla really is using the first font it finds because it selects
-altsys-augsburger initials-medium-r-normal--53-*-*-*-p-*-iso8859-15 which
delivers something, but it's not a euro. If a add an iso10646-1 Arial later in
the font path, I get the same result.

If I put the iso10646-1 Arial before the other forms in the font path then
Mozilla finally starts to get the idea. It loads
-monotype-arial-medium-r-normal--53-*-*-*-p-*-iso10646-1 for both the ascii text
and the euro. However, that imposes a performance penalty on loading the entire
font.

See the attached screenshot collection to see what happens.

Obviously, Mozilla needs a better selection scheme but I'm not sure right now
what that might be.
(Reporter)

Comment 1

18 years ago
Created attachment 10313 [details]
PNG screen shot collection
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M17

Comment 2

17 years ago
Hmmm... the euro works for me, but I have simmilar problem with ISO-8859-2
characters. Look at the attachments.

Comment 3

17 years ago
Created attachment 15813 [details]
Screenshot

Comment 4

17 years ago
Created attachment 15814 [details]
Testcase

Comment 5

17 years ago
I've found an error in the nsFontMetricsGTK.cpp file. If there is a list of
font-families in the style specification and mozilla wants to load a font with
character which is not present in the previously loaded font it tries to load
font from the next family from the list of possible families.
This error is probably repeated in other nsFontMetricsXXX.cpp files, but esp. on
Windows it doesn't show itself as the basic fonts on Windows are UNICODE and
therefore they have all required glyphs.

Comment 6

17 years ago
Created attachment 18537 [details] [diff] [review]
Patch to nsFontMetricsGTK.cpp
(Assignee)

Comment 7

17 years ago
Good catch! Thank you.

Comment 8

17 years ago
Should I make a patch to the other nsFontMetrics files? Would someone make
a review?
(Assignee)

Comment 9

17 years ago
Sorry, I think I may have spoken too soon. I had another look at
nsFontMetricsGTK.cpp, and it seems that it is supposed to be trying all of the
charsets within a family, storing them in the mLoadedFonts array, before moving
to the next family.

For example, let's suppose we have font-family: arial, helvetica, sans-serif.
Let's suppose we have the Arial TrueType font, and we have used ttmkfdir to
generate a bunch of charsets for arial (e.g. iso8859-1, iso8859-2, etc). As far
as I can tell from the code, TryFamily is supposed to be calling SearchNode for
each charset in the family, and it is testing font && font->SupportsChar before
moving to the next charset. This means that for each charset in the family, it
is supposed to be adding that charset to the mLoadedFonts array, and then
testing to see whether that charset includes the character that we are looking
for.

At some point, I may find some time to take a look at this in a debugger, but if
you have time, please take a look at TryFamily's loop and make sure mLoadedFonts
is being filled as it goes around the loop. If not, we have a different bug,
somewhere else.

Also, this bug is not likely to occur on Windows, since we use the Unicode
cmap there, and so each family has a single mLoadedFont, unlike the Unix
version. Symbol fonts do not contain Unicode cmaps, but we have our own maps for
those, and it is still a single mLoadedFont per family, unlike Unix. There is a
special subclass for Japanese Windows 95, but it loads each charset within a
font and loops over them in nsRenderingContextWinA (see the "subsets").

The Mac version was not written by me, so it has a rather different design. Take
a look if you are interested.

Comment 10

17 years ago
Well, if the TryFamily is supposed to load all charsets in the family then it
shouldn't return immediately when it finds fonts with the searched glyph. It
should to proceed to next charsets, load them all and return just one of the
loaded fonts (doesn't matter which one). I'll try to patch it this way, but I am
not sure if it's the most efficient way how to handle this.
Or I didn't understand it well?
(Assignee)

Comment 11

17 years ago
Created attachment 18910 [details] [diff] [review]
patch for this bug in diff -u format
(Assignee)

Comment 12

17 years ago
Good point, Tom. I have come up with a fix and attached it to this bug report.
Instead of resetting mFontsIndex all the way back to zero, I keep it at the
current value unless we failed to find any font that supports the current char,
in which case we increment it by one just before going back to the top of the
loop. This way, we avoid re-processing all the previous fonts, and just process
the current font family until we have exhausted it. I have tested it with your
test case attached to this report, and verified that it was broken before my fix
and fixed after my fix.

Would someone like to review the fix?

Comment 13

17 years ago
Yes, this is probably the best solution. Hopefully someone reviews this so this
can be checked into the trunk. (This bug bothered me for long long time.) :-)

Comment 14

17 years ago
This looks and sounds reasonable, but r=timeless is probably not worth 
anything. Why not ask for r=bstell or r=nhotta?
Keywords: approval, patch, review

Comment 15

17 years ago
Created attachment 18951 [details] [diff] [review]
this patch is a little more striaght forward than the last one.

Comment 16

17 years ago
My patch is slightly different. It pulls the increment into the loop which makes
things a little more explicit and the increment can't get lost in any if stmnt
logic inside the loop.

r=valeski.
(Assignee)

Comment 17

17 years ago
Thanks for looking into this, Jud. I don't think your patch is equivalent,
because we actually want mFontsIndex to stay at the current value until we fail
to find a font for the requested character. That's why I increment it at the end
of the loop, after everything has failed, and before we try the next family in
the list. When we succeed, we don't want to increment mFontsIndex because there
may be another font in the current family that contains a glyph for a character
that we haven't bumped into yet in the document.
(Assignee)

Comment 18

17 years ago
http://mozilla.org/hacking/reviewers.html says that the module owner should
review the patch. Adding pavlov to Cc list. Pav, would you please review my
patch (11/07/00 15:52)?

bstell, it wouldn't hurt if you also reviewed the patch since you will be
working on this code.
(Assignee)

Comment 19

17 years ago
timeless, thanks for reviewing. By asking pavlov and bstell to review the patch,
I didn't mean to imply that your review isn't worth much. I'm just following the
rules stated in the reviewer document. Additional reviews are OK too.

Comment 20

17 years ago
r=pavlov
sr=blizzard

Comment 22

17 years ago
I believe that:
 patch attachment id=18951 will not fix the problem.
 patch attachment id=18910 (Erik's) will fix the problem.

The family index should only be incremented after failing to find the font in 
the current family. It is only after we fail to find the font in any of the 
current family's encodings that we should try the next family. If we move to the 
next family before trying (loading) all the encodings for the current family 
then encoding that come after the found encoding for this family will not be 
used. Hence subsequent calls will ignore those encodings.

r=bstell for Erik's patch

just to be clear: 
pavlov: which patch did you approve?
blizzard: which patch did you approve?


(Assignee)

Comment 23

17 years ago
Checked my patch into the trunk. We may want to get this into the next Netscape 6
release. Bob, what shall we do about this bug? Mark it FIXED? Add some keyword?

Comment 24

17 years ago
Wonderful! It's fixed (tested with 2000111421 trunk nightly). Could someone mark
it fixed? (I haven't the right to)
(Reporter)

Comment 25

17 years ago
The patch seems to have cured this particular problem. I'm tempted to mark this
as fixed unless someone still sees a problem.

Comment 26

17 years ago
Marking this fixed.

If the problem reoccurs then we will re-open it.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 27

17 years ago
Marking verified per last comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.