Closed Bug 42917 Opened 25 years ago Closed 24 years ago

Linux Mozilla makes poor choices for glyph substitution

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: tenthumbs, Assigned: erik)

Details

Attachments

(6 files)

PNG screen shot collection
13.91 KB, image/png
Details
Screenshot
25.93 KB, image/png
Details
Testcase
281 bytes, text/html
Details
Patch to nsFontMetricsGTK.cpp
249 bytes, patch
Details | Diff | Splinter Review
patch for this bug in diff -u format
667 bytes, patch
Details | Diff | Splinter Review
this patch is a little more striaght forward than the last one.
715 bytes, patch
Details | Diff | Splinter Review
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.
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Hmmm... the euro works for me, but I have simmilar problem with ISO-8859-2 characters. Look at the attachments.
Attached image Screenshot —
Attached file Testcase —
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.
Good catch! Thank you.
Should I make a patch to the other nsFontMetrics files? Would someone make a review?
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.
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?
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?
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.) :-)
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
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.
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.
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.
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.
r=pavlov
sr=blizzard
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?
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?
Wonderful! It's fixed (tested with 2000111421 trunk nightly). Could someone mark it fixed? (I haven't the right to)
The patch seems to have cured this particular problem. I'm tempted to mark this as fixed unless someone still sees a problem.
Marking this fixed. If the problem reoccurs then we will re-open it.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Marking verified per last comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: