432065 - Resizing text display broken with Type 1 fonts, especially Courier (fixed spacing)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tom Kloos]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042609 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050311 Minefield/3.0pre

Changing the displayed text size while view a page may display garbage for Type 1 fonts.

Reproducible: Always

Steps to Reproduce:
1. Display the attachment test_fonts_2.html (note that you MUST have some Type 1 fonts installed!)
2. Increase and decrease displayed text size with "ctrl +" and "ctrl -".
3. Observe!
Actual Results:  
See attachment ff3_bug+1.png (after 1 upsize).  Note that going up two sizes displays correctly. 

Expected Results:  
See attachment opera927+1.png.

IE7 has it's resizing problems too.  It seems that Opera gets it mostly right?
In reality, lines 7 and 8 should always be the same length -- yet another bug.

Comment 1

[Tom Kloos]

Attachment: Test case

Comment 2

[Tom Kloos]

Attachment: Bad display of Type 1 Courier font

Comment 3

[Tom Kloos]

Attachment: Expected rendering (from Opera)

Comment 4

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050404 Minefield/3.0pre

I was not able to reproduce this.

Comment 5

[j.j.]

Ria, are you sure?
Usually Courier on Windows is a bitmap font, not "Type 1" 

Comment 6

[Tom Kloos]

See comments 8 and 9 on bug 432071.  There can be three forms of "Courier" on a Windows box.  The Courier that's broken is Type 1, probably not bit-mapped or "TrueType".  The bit-mapped and TrueType come standard on Windows.  "Type 1" does not and may be added by publishing, other software, or purchased.

Comment 7

[Tom Kloos]

On another WXP SP2 box (maintained by big corporate IT), I get different results...  "Courier" (line 7) is always displayed as the default font which is proportional, not fixed.  Line 3 and line 7 should never look the same.  Again, this works fine on IE7 and Opera.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Decided not to block on this at the triage during the Firefox meeting.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Then Stuart convinced us to block on it.

Comment 10

[Stuart Parmenter]

Attachment: fix

(this has part of the patch from 432062 in it)

In addition to the cairo fixes from roc and adrian, this patch causes us to prefer certain fonts when you have multiple fonts with the same name.  Because of the default font mappings on Windows that map Helvetica to Arial, when you have Helvetica (type1) installed, EnumFontFamiliesEx will return both the Helvetica type1 and Arial.  This patch will prefer type1 fonts over everything, to avoid chosing arial in the previous case.  I'd like to do something better there, but I'm really not sure what.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        return (!mUnicodeFont || mSymbolFont || mIsType1());

I'm not sure what mIsType1() compiles to.

+    GFX_FONT_TYPE_DEVICE,
+    GFX_FONT_TYPE_RASTER,
+    GFX_FONT_TYPE_TRUETYPE,
+    GFX_FONT_TYPE_PS_OPENTYPE,
+    GFX_FONT_TYPE_TT_OPENTYPE,
+    GFX_FONT_TYPE_TYPE1

Need to comment what these are. Also, the enum should have a name, then you can use it as the type of mFontType.

+    PRBool IsTrueType() const {

PS OpenType fonts aren't really Truetype are they? maybe we need a better name here, but I'm not sure what.

     for (PRUint32 i = 0; i < ff->mVariations.Length(); ++i) {
         fe = ff->mVariations[i];
+        if (feType > fe->mFontType) {
+            ff->mVariations.RemoveElementAt(i);
+            i = 0;
+        }
+    }

i = 0 seems wrong. The value of i for the next iteration will be 1, so if we have "<badfont> <badfont>", we'll remove the first one but the second one won't be removed. Just do --i.

+    // check and see if this font type is different than the one we've settled on
+    if (ff->mVariations.Length() > 0 && ff->mVariations[0]->mFontType != feType)
+        return 1;

Slightly simpler to replace this with a check "if (feType < fe->mFontType)" in the previous loop?

Otherwise OK.

Comment 12

[Stuart Parmenter]

(In reply to comment #11)
> I'm not sure what mIsType1() compiles to.

That was just a typo I fixed right after posting this patch.


> 
> +    GFX_FONT_TYPE_DEVICE,
> +    GFX_FONT_TYPE_RASTER,
> +    GFX_FONT_TYPE_TRUETYPE,
> +    GFX_FONT_TYPE_PS_OPENTYPE,
> +    GFX_FONT_TYPE_TT_OPENTYPE,
> +    GFX_FONT_TYPE_TYPE1
> 
> Need to comment what these are. Also, the enum should have a name, then you can
> use it as the type of mFontType.
> 
yeah, ok


> +    PRBool IsTrueType() const {
> 
> PS OpenType fonts aren't really Truetype are they? maybe we need a better name
> here, but I'm not sure what.
> 
They're not actually TrueType.  Not sure what a better name would be since some truetype fonts aren't opentype...  TrueOrOpenType? :/


> i = 0 seems wrong. The value of i for the next iteration will be 1, so if we
> have "<badfont> <badfont>", we'll remove the first one but the second one won't
> be removed. Just do --i.
> 

> Slightly simpler to replace this with a check "if (feType < fe->mFontType)" in
> the previous loop?

yeah, whoops.  new patch coming shortly.

Comment 13

[Stuart Parmenter]

Attachment: better fix

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 319826 [details] [diff] [review]
better fix

 +enum gfxWindowsFontTypes {

gfxWindowsFontType

Take out those GM_ADVANCED changes. We probably don't want them, and if we do, we should land them separately.

Comment 15

[Mike Schroepfer]

Comment on attachment 319826 [details] [diff] [review]
better fix

a+ based on triage meeting during Moz2 today