Closed Bug 42917 Opened 24 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: