Windows vector font rendering is broken

RESOLVED FIXED in mozilla1.9

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: tkloos, Assigned: roc)

Tracking

({fonts, regression, testcase})

Trunk
mozilla1.9
x86
Windows XP
fonts, regression, testcase
Points:
---
Bug Flags:
blocking1.9 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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

Recent changes have cause the "vector" fonts supplied with Windows to be displayed with the characters compressed together.   Fortunately these fonts aren't used too often, but IE and Opera get them right!

Reproducible: Always

Steps to Reproduce:
1. Display test case test_fonts_2.html with recent minefield.

Actual Results:  
See attachment ff3_bug_0.png, lines 10 and 11.

Expected Results:  
See attached screen-shot sm0426.png from Seamonkey 26 May 08 tinderbox.  (Current tinderbox Seamonkey is now broken just like Firefox.)

This change appears to have taken place with recent tweaks to fix Type 1 printing and other font problems?  (Type 1 printing is still broken!)
(Reporter)

Comment 1

11 years ago
Created attachment 319215 [details]
Test case
(Reporter)

Comment 2

11 years ago
Created attachment 319216 [details]
Display of problem...  Lines 10 and 11.
(Reporter)

Comment 3

11 years ago
Created attachment 319217 [details]
Expected display of lines 10 and 11 from older tinderbox.
(Reporter)

Updated

11 years ago
Flags: blocking1.9?
Keywords: fonts

Comment 4

11 years ago
It's a recent regression.

Works:  20080429_2037
Broken: 20080429_2221

So... bug 427411.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Version: unspecified → Trunk
Blocks: 427411
I don't think this should block since the fonts are rarely used and the text is still readable.
It's really hard to see how the patch in bug 427411 could have done this.
I just made a build with 427411 backed out, and it still showed this bug.
No longer blocks: 427411

Comment 8

11 years ago
Hmm, that's very odd, then.  The only other checkin during that regression range was bug 429510.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1209526620&maxdate=1209532859
Can you post a link to the nightly build where you did not see the bug?

Comment 10

11 years ago
I used the builds at the archive here: <http://hourly-archive.localgho.st/>

20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
20080429_2221_firefox-3.0pre.en-US.win32.zip (broken)
> 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)

It's broken in this build for me.

Comment 12

11 years ago
(In reply to comment #11)
> > 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
> 
> It's broken in this build for me.
> 

I just tried that build with a new profile on two different computers (my main XPSP2 box and a VistaSP1 test install that I have in a VPC), and it rendered correctly both times...
Hmm. Anyone else want to try it? :-)

Updated

11 years ago
Keywords: qawanted
Assignee: nobody → pavlov
Flags: blocking1.9? → blocking1.9+

Comment 14

11 years ago
20080429_2037 WFM on XP SP2. (though, at first I was clicking the screen shot and wondering why it didn't :P)
I strongly feel this should not be a blocker. These fonts are very rarely used because they look crap even if there isn't a spacing bug. They've been obsolete ever since Windows got Truetype support (i.e. 1991 or thereabouts).
Created attachment 319406 [details]
my testcase

This is my testcase.

Comment 17

11 years ago
OK, this is strange:
-The first testcase always works if I load it first.
-rocs testcase works only if I load the first testcase in this bug (loading the 2 at the same time via session restore seems to work as long as I look at the first testcase tab first). 
-if I load rocs testcase first then the first one doesn't work.
(In reply to comment #10)
> I used the builds at the archive here: <http://hourly-archive.localgho.st/>
> 
> 20080429_2037_firefox-3.0pre.en-US.win32.zip (works)
> 20080429_2221_firefox-3.0pre.en-US.win32.zip (broken)
> 

I get the same result on Vista (without SP1).

Backing out bug 427411's patch fixes the regression.

(In reply to comment #17)
> OK, this is strange:
> -The first testcase always works if I load it first.
> -rocs testcase works only if I load the first testcase in this bug (loading the
> 2 at the same time via session restore seems to work as long as I look at the
> first testcase tab first). 
> -if I load rocs testcase first then the first one doesn't work.

OK, I see this too. Thanks for tracking that down.

So it looks like there's a deeper bug here that's being exposed in more situations by the patch in bug 427411. More reason to not block IMHO.

Comment 20

11 years ago
(In reply to comment #15)
> I strongly feel this should not be a blocker. These fonts are very rarely used
> because they look crap even if there isn't a spacing bug. They've been obsolete
> ever since Windows got Truetype support (i.e. 1991 or thereabouts).
> 
In addition to that, Windows vector fonts weren't supported at all in Firefox 2, so this isn't a big loss.
Testing using the 20080429_2037_firefox-3.0pre.en-US.win32.zip build: 3-A works, 3-B doesn't. The only difference is that in 3-A the font-family is set on the <span>, in 3-B the font-family is set on the <body>. Disturbing...
(Reporter)

Comment 24

11 years ago
(In reply to comment #20)
> In addition to that, Windows vector fonts weren't supported at all in Firefox
> 2, so this isn't a big loss.
> 

But it works fine in IE7 and Opera (haven't tried Safari yet).  Don't give folks ammunition for dumping Firefox.  At the very least, the characteristics shouldn't change depending upon previously loaded pages.

There are problems with other font rendering too.  "Courier" sometimes renders as the default font (very bad, since the default is proportionally spaced versed fixed -- line 3 should NOT look the same a line 7), or sometimes as garbage characters, or sometimes fine.

See bug 432065 and bug 432071 too.
> Don't give folks ammunition for dumping Firefox.

This is silly. You could use this "argument" to make almost any bug a priority.
We're initializing the gfxWindowsFont with the same parameters both times.

I tried always calling SetupFont and setting mScriptCache to nsnull before we call ScriptShape and ScriptPlace. That doesn't make the bug go away, which suggests this is not a problem with SCRIPT_CACHE.

I tried forcing the InitTextRunGDI path, where instead of GetGlyphIndices I just set each glyph to the character value. That displays the right glyphs and avoids Uniscribe, but the bug still happens. (In fact, it's worse.)

Stuart, is it possible the text advances are coming back in the wrong units somehow?
Is there any font size at which the text advances are correct?  Are you getting advances from a smaller font size, and then some magic scaling on draw?

Comment 28

11 years ago
It might be due to the printer, but I just noticed while testing bug 432071 that these fonts print properly (although very light.)
I tried making DCFromContext always create a new DC instead of reusing the surface DC (if available) and using the GDI path. That made no difference.

Comment 30

11 years ago
Interestingly these fonts had the exact opposite problem in bug 425336, the spacing was too wide except for the numbers and the 'mp' in jumps for some reason.
I've been trying various things. I tried to get Uniscribe out of the path, by using GDI with GetTextExtentPointW instead of GetTextExtentPointI. GetTextExtentPointW gives better results than GetTextExtentPointI but it doesn't solve the problem, the characters are still too close together. GetTextExtentPointA doesn't help. I changed cairo-win32-font.c to not use ETO_GLYPH_INDEX for non-Truetype fonts, but that hasn't helped either.

I compared our rendering of Modern against IE's and Notepad's. Turns out we're rendering the glyphs horizontally stretched compared to IE at the same font-size --- the vertical sizes are identical. I think the advances we're getting would be about right for the unstretched glyphs.
It seems that this horizontal stretching is due to cairo_win32_scaled_font_select_font performing SetGraphicsMode (hdc, GM_ADVANCED). If I comment that out, plus the _win32_scaled_font_set_world_transform call following (which fails if we don't use GM_ADVANCED), this bug goes away. It's not the call to _win32_scaled_font_set_world_transform, because if I comment out just that call we still have the bug.
Created attachment 319532 [details] [diff] [review]
victory

Okay! So the key thing here is that setting GM_ADVANCED on the DC stretches the text horizontally, both for drawing and metrics. Therefore it is essential that we either always have GM_ADVANCED set when drawing or measuring text, or else we never have it set. It's easier to have it always set because that will let us do world-transform effects like rotation and shearing. So this patch establishes the invariant that the DC in a cairo surface is always in GM_ADVANCED mode. We should probably document this somewhere, but I'm not sure where --- any idea, Vlad? We also need to make sure temporary DCs used in gfxWindowsFonts are also in GM_ADVANCED mode.

I think the key offender here was in gfxWindowsFont::ComputeMetrics... I'm not sure, but my theory is that the first DC you use an HFONT with does some font metrics caching that depends on the mode of that DC. If the first thing we did was get metrics, we used the HFONT first with the GetDC() result from ComputeMetrics which was in GM_COMPATIBLE mode. I think this made GDI always return narrow metrics for the HFONT but still draw wide glyphs when the target DC is in GM_ADVANCED mode (which it always is when we're drawing). On the other hand, if the first thing you did with the font was build a textrun, we called down into cairo_win32_scaled_font_select_font which put the DC into GM_ADVANCED mode before we did anything with the font so we got wide metrics and wide glyphs and everything's fine.

So, putting the font style on the block made us compute line-height (requiring font metrics) before building the textruns inside the block, triggering the bad case, whereas putting the font style on the span we were building textruns before font metrics were needed to reflow the span, not triggering badness. Then bug 427411 changed things so we computed metrics eagerly for the first font in the fontgroup, which meant we always hit the bad case.

This patch does mean we still get different rendering from IE. I don't much care since the text is perfectly readable, in fact I think "Modern" looks better when it's wider, and in any case it is definitely a GDI issue and I refuse to care.
Assignee: pavlov → roc
Status: NEW → ASSIGNED
Attachment #319532 - Flags: review?(pavlov)
Whiteboard: [needs review]
Whiteboard: [needs review] → [has patch][needs review pavlov]
Decided not to block on this at the triage during the Firefox meeting.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+

Updated

11 years ago
Attachment #319532 - Flags: review?(pavlov)
Attachment #319532 - Flags: review+
Attachment #319532 - Flags: approval1.9?
Whiteboard: [has patch][needs review pavlov] → [has patch]
This patch regresses printing. We ain't taking it.
Created attachment 319621 [details] [diff] [review]
victory v2

This fixes printing. Very simple ommission.
Attachment #319532 - Attachment is obsolete: true
Attachment #319621 - Flags: review?(pavlov)
Attachment #319532 - Flags: approval1.9?

Updated

11 years ago
Attachment #319621 - Flags: review?(pavlov) → review+
This still breaks printing of native theme stuff. I think we can't rely on our DCs staying in GM_ADVANCED mode. We need to persist with the current approach which sets them to GM_ADVANCED mode just before an operation that needs that mode.
Created attachment 319733 [details] [diff] [review]
victory v3

Much safer patch.

This is enough to fix the bug. We don't make assumptions about the DC staying in GM_ADVANCED mode. We just make sure that the extra DCs we get in gfxWindowsFonts are put in GM_ADVANCED mode. This is super-safe.
Attachment #319733 - Flags: review?(pavlov)
Attachment #319621 - Attachment is obsolete: true

Comment 39

11 years ago
Comment on attachment 319733 [details] [diff] [review]
victory v3

I'd like to take this.
Attachment #319733 - Flags: review?(pavlov)
Attachment #319733 - Flags: review+
Attachment #319733 - Flags: approval1.9?

Updated

11 years ago
Attachment #319733 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/gfx/thebes/src/gfxWindowsFonts.cpp 	1.206 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed, qawanted
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla1.9

Updated

11 years ago
Flags: in-testsuite?
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.