Closed Bug 432065 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tkloos, Assigned: pavlov)

Details

(Whiteboard: [has patch][has review][has approval])

Attachments

(4 files, 1 obsolete file)

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.
Attached file Test case
Flags: blocking1.9?
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.
Ria, are you sure?
Usually Courier on Windows is a bitmap font, not "Type 1" 
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.
Assignee: nobody → pavlov
Flags: blocking1.9? → blocking1.9+
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.
Decided not to block on this at the triage during the Firefox meeting.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9- → blocking1.9+
Status: NEW → ASSIGNED
Then Stuart convinced us to block on it.
Attached patch fix (obsolete) — Splinter Review
(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.
Attachment #319729 - Flags: review?(roc)
+        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.
(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.
Attached patch better fixSplinter Review
Attachment #319729 - Attachment is obsolete: true
Attachment #319826 - Flags: review?(roc)
Attachment #319729 - Flags: review?(roc)
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.
Attachment #319826 - Flags: review?(roc) → review+
Attachment #319826 - Flags: approval1.9?
Comment on attachment 319826 [details] [diff] [review]
better fix

a+ based on triage meeting during Moz2 today
Attachment #319826 - Flags: approval1.9? → approval1.9+
Whiteboard: [has patch][has review][has approval]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.